[PATCH] D93110: [analyzer] Implement fine-grained suppressions via attributes

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> What I want to avoid is having a static analyzer-specific suppression attribute, and a different one for clang-tidy, and a third one for the frontend.

Given that there are already multiple suppression mechanisms circulating around (clang-tidy's `// NOLINT`, frontend pragmas, static analyzer's `#ifdef __clang_analyzer__`, now also attribute), i guess a path forward would be to eventually add support for multiple mechanisms everywhere. This may be tedious to implement but benefits may be well worth it. Consistency across tools is important when tools are used together; for instance, when clang-tidy integrates static analyzer, static analyzer automatically starts supporting `// NOLINT` through clang-tidy's diagnostic consumer magic. This would also be amazing for discoverability: users can keep using their preferred mechanism and it'd just work out of the box when they add a new tool to their arsenal.



================
Comment at: clang/include/clang/Basic/Attr.td:2396
+  let Args = [VariadicStringArgument<"DiagnosticIdentifiers">];
+  let Subjects = SubjectList<[Var]>;
+  let Documentation = [AnalyzerSuppressDocs];
----------------
[evil mode]
What's the expected behavior when the same variable (esp. global) has multiple redeclarations and only some of them wear the attribute?
[/evil mode]

I guess this mostly applies to globals and we don't have much checkers that would warn on globals (maybe some of those WebKit checkers by @jkorous may be affected).


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:592-593
 
+  /// User-provided in-code suppressions.
+  BugSuppression UserSuppressions;
+
----------------
Even though the attribute is specific to the static analyzer and from that point of view it's appropriate to keep this info in the `BugReporter` object (that will always be specific to the static analyzer), i believe we should ultimately move this into a higher-level entity in `libAnalysis` such as `AnalysisConsumer`. This would allow other tools such as clang-tidy to access that information when integrated into the static analyzer (eg., a-la D95403) and the user treats the entire conglomerate of tools as just "the static analyzer" and expects consistency across checks. That, of course, relies on our ability to look up attributes by source locations.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h:30-31
+public:
+  /// Return true if the given bug report was explicitly suppressed by the user.
+  bool isSuppressed(const BugReport &);
+  /// Return true if the bug reported at the given location was explicitly
----------------
So, like, i wish this could be replaced with `bool isSuppressed(const PathDiagnostic &);` so that to untie this from Static Analyzer-specific `BugReport` object.

There's no straightforward way to jump from `BugReport` to its specific `PathDiagnostic`, especially given that the same bug report may produce different `PathDiagnostic`s for different consumers. But by the time suppressions kick in we can be certain that `PathDiagnostic` objects are fully constructed.

It's an interesting question whether different `PathDiagnostic`s for the same bug report may interact with suppressions differently. For now this can't happen because all of them will have the same location and the same uniqueing location. We should obviously avoid such differences, probably protect ourselves against them with an assertion.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h:38
+private:
+  // Overly pessimistic number, to be honest.
+  static constexpr unsigned EXPECTED_NUMBER_OF_SUPPRESSIONS = 8;
----------------
Depression fuel! :D


================
Comment at: clang/lib/StaticAnalyzer/Core/BugSuppression.cpp:70
+
+class CacheInitializer : public RecursiveASTVisitor<CacheInitializer> {
+public:
----------------
A recursive visitor would cause you to visit nested declarations (eg., lambdas, or methods of nested classes) multiple times. Is this necessary? A plain old `ConstStmtVisitor` that's taught to visit child statements recursively could easily avoid that. That's probably cheap though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93110/new/

https://reviews.llvm.org/D93110