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

[saugustine]

Summary

This reverts commit 33fb9cbe211d1b43d4b84edf34e11001f04cddf0.

That commit violates layering by adding a dependency from StaticAnalyzer/Core
back to StaticAnalyzer/FrontEnd, creating a circular dependency.

I can't see a clean way to fix it except refactoring.

[Szelethus]

Yup, I agree. Detailed my answer here a bit: D78126#inline-751477. Thanks for taking initiative!

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)
-      : BugReport(kind, bt, "", desc) {}
+      : K(kind), BT(bt), Description(desc) {}
 
   BugReport(Kind K, const BugType &BT, StringRef ShortDescription,
             StringRef Description)
       : K(K), BT(BT), ShortDescription(ShortDescription),
         Description(Description) {}
 
 public:
   virtual ~BugReport() = default;
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)
-      : PathSensitiveBugReport(bt, desc, desc, errorNode) {}
+      : BugReport(Kind::PathSensitive, bt, desc), ErrorNode(errorNode),
+        ErrorNodeRange(getStmt() ? getStmt()->getSourceRange()
+                                 : SourceRange()) {}
 
   PathSensitiveBugReport(const BugType &bt, StringRef shortDesc, StringRef desc,
                          const ExplodedNode *errorNode)
-      : PathSensitiveBugReport(bt, shortDesc, desc, errorNode,
-                               /*LocationToUnique*/ {},
-                               /*DeclToUnique*/ nullptr) {}
+      : BugReport(Kind::PathSensitive, bt, shortDesc, desc),
+        ErrorNode(errorNode),
+        ErrorNodeRange(getStmt() ? getStmt()->getSourceRange()
+                                 : SourceRange()) {}
 
   /// 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)
-      : PathSensitiveBugReport(bt, desc, desc, errorNode, LocationToUnique,
-                               DeclToUnique) {}
-
-  PathSensitiveBugReport(const BugType &bt, StringRef shortDesc, 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);
+  }
 
   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

CmdLineOption(StringRef OptionType, StringRef OptionName,
              "parameters to the addCheckerOption or addPackageOption method?");
       (void)Tmp;
 
       assert((DevelopmentStatus == "alpha" || DevelopmentStatus == "beta" ||
               DevelopmentStatus == "released") &&
              "Invalid development status!");
     }
 
-    LLVM_DUMP_METHOD void dump() const;
+    LLVM_DUMP_METHOD void dump() const { dumpToStream(llvm::errs()); }
     LLVM_DUMP_METHOD void dumpToStream(llvm::raw_ostream &Out) const;
   };
 
   using CmdLineOptionList = llvm::SmallVector<CmdLineOption, 0>;
 
   struct CheckerInfo;
 
   using CheckerInfoList = std::vector<CheckerInfo>;
CheckerInfo(InitializationFunction Fn, ShouldRegisterFunction sfn,
                 StringRef Name, StringRef Desc, StringRef DocsUri,
                 bool IsHidden)
         : Initialize(Fn), ShouldRegister(sfn), FullName(Name), Desc(Desc),
           DocumentationUri(DocsUri), IsHidden(IsHidden) {}
 
     // Used for lower_bound.
     explicit CheckerInfo(StringRef FullName) : FullName(FullName) {}
 
-    LLVM_DUMP_METHOD void dump() const;
+    LLVM_DUMP_METHOD void dump() const { dumpToStream(llvm::errs()); }
     LLVM_DUMP_METHOD void dumpToStream(llvm::raw_ostream &Out) const;
   };
 
   using StateFromCmdLine = CheckerInfo::StateFromCmdLine;
 
   /// Specifies a package. Each package option is implicitly an option for all
   /// checkers within the package.
   struct PackageInfo {
     StringRef FullName;
     CmdLineOptionList CmdLineOptions;
 
     // Since each package must have a different full name, we can identify
     // CheckerInfo objects by them.
     bool operator==(const PackageInfo &Rhs) const {
       return FullName == Rhs.FullName;
     }
 
     explicit PackageInfo(StringRef FullName) : FullName(FullName) {}
 
-    LLVM_DUMP_METHOD void dump() const;
+    LLVM_DUMP_METHOD void dump() const { dumpToStream(llvm::errs()); }
     LLVM_DUMP_METHOD void dumpToStream(llvm::raw_ostream &Out) const;
   };
 
   using PackageInfoList = llvm::SmallVector<PackageInfo, 0>;
 
 private:
   /// Default initialization function for checkers -- since CheckerManager
   /// includes this header, we need to make it a template parameter, and since
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);
-  /// 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();
-  template <bool IsWeak> void resolveDependencies();
 
-  DiagnosticsEngine &Diags;
-  AnalyzerOptions &AnOpts;
-
-public:
   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;
 
   /// 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;
   llvm::SmallVector<std::pair<StringRef, StringRef>, 0> WeakDependencies;
 
+  template <bool IsWeak> 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) {
-  assert(!isDependency(ErrorNode->getState()
-                           ->getAnalysisManager()
-                           .getCheckerManager()
-                           ->getCheckerRegistry(),
-                       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);

clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

static bool isInPackage(const CheckerRegistry::CheckerInfo &Checker,
 
   return false;
 }
 
 //===----------------------------------------------------------------------===//
 // Methods of CmdLineOption, PackageInfo and CheckerInfo.
 //===----------------------------------------------------------------------===//
 
-LLVM_DUMP_METHOD void CheckerRegistry::CmdLineOption::dump() const {
-  dumpToStream(llvm::errs());
-}
-
 LLVM_DUMP_METHOD void
 CheckerRegistry::CmdLineOption::dumpToStream(llvm::raw_ostream &Out) const {
   // The description can be just checked in Checkers.inc, the point here is to
   // debug whether we succeeded in parsing it.
   Out << OptionName << " (" << OptionType << ", "
       << (IsHidden ? "hidden, " : "") << DevelopmentStatus << ") default: \""
       << DefaultValStr;
 }
 
 static StringRef toString(CheckerRegistry::StateFromCmdLine Kind) {
   switch (Kind) {
   case CheckerRegistry::StateFromCmdLine::State_Disabled:
     return "Disabled";
   case CheckerRegistry::StateFromCmdLine::State_Enabled:
     return "Enabled";
   case CheckerRegistry::StateFromCmdLine::State_Unspecified:
     return "Unspecified";
   }
   llvm_unreachable("Unhandled CheckerRegistry::StateFromCmdLine enum");
 }
 
-LLVM_DUMP_METHOD void CheckerRegistry::CheckerInfo::dump() const {
-  dumpToStream(llvm::errs());
-}
-
 LLVM_DUMP_METHOD void
 CheckerRegistry::CheckerInfo::dumpToStream(llvm::raw_ostream &Out) const {
   // The description can be just checked in Checkers.inc, the point here is to
   // debug whether we succeeded in parsing it. Same with documentation uri.
   Out << FullName << " (" << toString(State) << (IsHidden ? ", hidden" : "")
       << ")\n";
   Out << "  Options:\n";
   for (const CmdLineOption &Option : CmdLineOptions) {
     Out << "    ";
     Option.dumpToStream(Out);
     Out << '\n';
   }
   Out << "  Dependencies:\n";
   for (const CheckerInfo *Dependency : Dependencies) {
     Out << "  " << Dependency->FullName << '\n';
   }
   Out << "  Weak dependencies:\n";
   for (const CheckerInfo *Dependency : WeakDependencies) {
     Out << "    " << Dependency->FullName << '\n';
   }
 }
 
-LLVM_DUMP_METHOD void CheckerRegistry::PackageInfo::dump() const {
-  dumpToStream(llvm::errs());
-}
-
 LLVM_DUMP_METHOD void
 CheckerRegistry::PackageInfo::dumpToStream(llvm::raw_ostream &Out) const {
   Out << FullName << "\n";
   Out << "  Options:\n";
   for (const CmdLineOption &Option : CmdLineOptions) {
     Out << "    ";
     Option.dumpToStream(Out);
     Out << '\n';