[analyzer][NFC] Don't allow dependency checkers to emit diagnostics

[Szelethus]

Summary

The thrilling conclusion to the barrage of patches I uploaded lately! This is a big milestone towards the goal set out in http://lists.llvm.org/pipermail/cfe-dev/2019-August/063070.html. I hope to accompany this with a patch where the a coreModeling package is added, from which package diagnostics aren't allowed either, is an implicit dependency of all checkers, and the core package for the first time can be safely disabled.

[balazske]

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

153

Probably too much of assert here (but it works)? (There is not a way to get the dependency data "dynamically", like "Checker->isDependentOn(OtherChecker)"?)

[Szelethus]

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

153

Wow, you're totally correct, I didn't even consider that. Indeed, this doesn't work with plugins (or checkers from unit tests), but it should be doable, since CheckerManager owns CheckerRegistry (D75360). This is kind of ironic, considering how much I tend to block patches based on this :^)

[Szelethus]

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

153

Oh, to actually answer your question. The checker dependency information is stored in CheckerRegistry in this small struct:

struct CheckerInfo {
  enum class StateFromCmdLine {
    // This checker wasn't explicitly enabled or disabled.
    State_Unspecified,
    // This checker was explicitly disabled.
    State_Disabled,
    // This checker was explicitly enabled.
    State_Enabled
  };

  InitializationFunction Initialize = nullptr;
  ShouldRegisterFunction ShouldRegister = nullptr;
  StringRef FullName;
  StringRef Desc;
  StringRef DocumentationUri;
  CmdLineOptionList CmdLineOptions;
  bool IsHidden = false;
  StateFromCmdLine State = StateFromCmdLine::State_Unspecified;

  ConstCheckerInfoList Dependencies;
  //...
};

So the nice solution should make this info a bit more visible.

[martong]

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

153

I have no serious bad feelings with the macro solution, but if you can come up with a better structure then that would be so cool! :)

[Szelethus]

Make the solution handle dependencies from plugins as well via CheckerRegistry.

[Szelethus]

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

2126–2130

I'll put this in the function call the release bots wont complain.

[martong]

LGTM!

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

2132

Oh yeah! :)

[saugustine]

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

43

Unfortunately, this include creates a circular dependency from StaticAnalyzer/Core to StaticAnalyzer/Frontend back to StaticAnalyzer/Core.

I'm hoping to figure out a fix that doesn't involve reverting it, but if you have a quick fix, that would be really great.

[Szelethus]

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

43

Yup, this is a common theme with these changes, the divide in between the analyzer's libraries isn't too well defined.

CheckerRegistry definitely belongs to the frontend, and BugReporter isn't really moving anywhere either. The registry is responsible for parsing the command line arguments (which checker is enabled, values of checker options, resolving dependencies, etc), but this information is really desired in the rest of the analyzer as well.

I think the obvious (to me, that is) solution would be to move the containers to the Core library, and leave the rest of the logic in the frontend. While I dare claiming that I have great knowledge about this part of the codebase (its usually just me maintaining it), I would be more comfortable doing that without needing to rush the solution :)

An uncomfortable hack would be to simply implement the new functions in the frontend library -- but the former solution better describes where the data CheckerRegistry holds should lie.

I'll revert this. Thanks!

[Szelethus]

Reverted in rGe64059828f19f629081220bffeb3ef7582870111.

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

 #ifndef LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTER_H
 #define LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTER_H
 
 #include "clang/Analysis/PathDiagnostic.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/FoldingSet.h"
protected:
   std::string ShortDescription;
   std::string Description;
 
   SmallVector<SourceRange, 4> Ranges;
   SmallVector<std::shared_ptr<PathDiagnosticNotePiece>, 4> Notes;
   SmallVector<FixItHint, 4> Fixits;
 
   BugReport(Kind kind, const BugType &bt, StringRef desc)
-      : K(kind), BT(bt), Description(desc) {}
+      : BugReport(kind, bt, "", desc) {}
 
   BugReport(Kind K, const BugType &BT, StringRef ShortDescription,
             StringRef Description)
       : K(K), BT(BT), ShortDescription(ShortDescription),
         Description(Description) {}
 
 public:
   virtual ~BugReport() = default;
 
   Kind getKind() const { return K; }
 
   const BugType& getBugType() const { return BT; }
 
   /// A verbose warning message that is appropriate for displaying next to
   /// the source code that introduces the problem. The description should be
   /// at least a full sentence starting with a capital letter. The period at
   /// the end of the warning is traditionally omitted. If the description
   /// consists of multiple sentences, periods between the sentences are
   /// encouraged, but the period at the end of the description is still omitted.
   StringRef getDescription() const { return Description; }
 
   /// A short general warning message that is appropriate for displaying in
protected:
   /// diagnostic.
   // FIXME: Allow shared_ptr keys in DenseMap?
   std::map<PathDiagnosticPieceRef, std::unique_ptr<StackHintGenerator>>
       StackHints;
 
 public:
   PathSensitiveBugReport(const BugType &bt, StringRef desc,
                          const ExplodedNode *errorNode)
-      : BugReport(Kind::PathSensitive, bt, desc), ErrorNode(errorNode),
-        ErrorNodeRange(getStmt() ? getStmt()->getSourceRange()
-                                 : SourceRange()) {}
+      : PathSensitiveBugReport(bt, desc, desc, errorNode) {}
 
   PathSensitiveBugReport(const BugType &bt, StringRef shortDesc, StringRef desc,
                          const ExplodedNode *errorNode)
-      : BugReport(Kind::PathSensitive, bt, shortDesc, desc),
-        ErrorNode(errorNode),
-        ErrorNodeRange(getStmt() ? getStmt()->getSourceRange()
-                                 : SourceRange()) {}
+      : PathSensitiveBugReport(bt, shortDesc, desc, errorNode,
+                               /*LocationToUnique*/ {},
+                               /*DeclToUnique*/ nullptr) {}
 
   /// Create a PathSensitiveBugReport with a custom uniqueing location.
   ///
   /// The reports that have the same report location, description, bug type, and
   /// ranges are uniqued - only one of the equivalent reports will be presented
   /// to the user. This method allows to rest the location which should be used
   /// for uniquing reports. For example, memory leaks checker, could set this to
   /// the allocation site, rather then the location where the bug is reported.
   PathSensitiveBugReport(const BugType &bt, StringRef desc,
                          const ExplodedNode *errorNode,
                          PathDiagnosticLocation LocationToUnique,
                          const Decl *DeclToUnique)
-      : BugReport(Kind::PathSensitive, bt, desc), ErrorNode(errorNode),
-        ErrorNodeRange(getStmt() ? getStmt()->getSourceRange() : SourceRange()),
-        UniqueingLocation(LocationToUnique), UniqueingDecl(DeclToUnique) {
-    assert(errorNode);
-  }
+      : PathSensitiveBugReport(bt, desc, desc, errorNode, LocationToUnique,
+                               DeclToUnique) {}
+
+  PathSensitiveBugReport(const BugType &bt, StringRef shortDesc, StringRef desc,
+                         const ExplodedNode *errorNode,
+                         PathDiagnosticLocation LocationToUnique,
+                         const Decl *DeclToUnique);
 
   static bool classof(const BugReport *R) {
     return R->getKind() == Kind::PathSensitive;
   }
 
   const ExplodedNode *getErrorNode() const { return ErrorNode; }
 
   /// Indicates whether or not any path pruning should take place

clang/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h

public:
   void printEnabledCheckerList(raw_ostream &Out) const;
   void printCheckerOptionList(raw_ostream &Out) const;
 
 private:
   /// Return an iterator range of mutable CheckerInfos \p CmdLineArg applies to.
   /// For example, it'll return the checkers for the core package, if
   /// \p CmdLineArg is "core".
   CheckerInfoListRange getMutableCheckersForCmdLineArg(StringRef CmdLineArg);
-
-  CheckerInfoList Checkers;
-  PackageInfoList Packages;
   /// Used for couting how many checkers belong to a certain package in the
   /// \c Checkers field. For convenience purposes.
   llvm::StringMap<size_t> PackageSizes;
 
+  void resolveCheckerAndPackageOptions();
+  void resolveDependencies();
+
+  DiagnosticsEngine &Diags;
+  AnalyzerOptions &AnOpts;
+
+public:
+  CheckerInfoList Checkers;
+  PackageInfoList Packages;
+
   /// Contains all (Dependendent checker, Dependency) pairs. We need this, as
   /// we'll resolve dependencies after all checkers were added first.
   llvm::SmallVector<std::pair<StringRef, StringRef>, 0> Dependencies;
-  void resolveDependencies();
 
   /// Contains all (FullName, CmdLineOption) pairs. Similarly to dependencies,
   /// we only modify the actual CheckerInfo and PackageInfo objects once all
   /// of them have been added.
   llvm::SmallVector<std::pair<StringRef, CmdLineOption>, 0> PackageOptions;
   llvm::SmallVector<std::pair<StringRef, CmdLineOption>, 0> CheckerOptions;
-
-  void resolveCheckerAndPackageOptions();
-
-  DiagnosticsEngine &Diags;
-  AnalyzerOptions &AnOpts;
   CheckerInfoSet EnabledCheckers;
 };
 
 } // namespace ento
 } // namespace clang
 
 #endif // LLVM_CLANG_STATICANALYZER_CORE_CHECKERREGISTRY_H

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"

 void BugType::anchor() {}
 
 void BuiltinBug::anchor() {}
 
 //===----------------------------------------------------------------------===//
 // Methods for BugReport and subclasses.
 //===----------------------------------------------------------------------===//
 
+static bool isDependency(const CheckerRegistry &Registry,
+                         StringRef CheckerName) {
+  for (const std::pair<StringRef, StringRef> &Pair : Registry.Dependencies) {
+    if (Pair.second == CheckerName)
+      return true;
+  }
+  return false;
+}
+
+PathSensitiveBugReport::PathSensitiveBugReport(
+    const BugType &bt, StringRef shortDesc, StringRef desc,
+    const ExplodedNode *errorNode, PathDiagnosticLocation LocationToUnique,
+    const Decl *DeclToUnique)
+    : BugReport(Kind::PathSensitive, bt, shortDesc, desc), ErrorNode(errorNode),
+      ErrorNodeRange(getStmt() ? getStmt()->getSourceRange() : SourceRange()),
+      UniqueingLocation(LocationToUnique), UniqueingDecl(DeclToUnique) {
+
+  const CheckerRegistry &Registry = ErrorNode->getState()
+                                        ->getAnalysisManager()
+                                        .getCheckerManager()
+                                        ->getCheckerRegistry();
+
+  assert(!isDependency(Registry, bt.getCheckerName()) &&
+         "Some checkers depend on this one! We don't allow dependency "
+         "checkers to emit warnings, because checkers should depend on "
+         "*modeling*, not *diagnostics*.");
+}
+
 void PathSensitiveBugReport::addVisitor(
     std::unique_ptr<BugReporterVisitor> visitor) {
   if (!visitor)
     return;
 
   llvm::FoldingSetNodeID ID;
   visitor->Profile(ID);
 
static void insertToInterestingnessMap(
   // and we wouldn't like to downplay their importance.
 
   switch (TKind) {
     case bugreporter::TrackingKind::Thorough:
       Result.first->getSecond() = bugreporter::TrackingKind::Thorough;
       return;
     case bugreporter::TrackingKind::Condition:
       return;
     }
 
     llvm_unreachable(
         "BugReport::markInteresting currently can only handle 2 different "
         "tracking kinds! Please define what tracking kind should this entitiy"
         "have, if it was already marked as interesting with a different kind!");
 }
 
 void PathSensitiveBugReport::markInteresting(SymbolRef sym,
                                              bugreporter::TrackingKind TKind) {
   if (!sym)
     return;
 
   insertToInterestingnessMap(InterestingSymbols, sym, TKind);