[analyzer][NFC] Introduce SuperChecker<>, a convenient alternative to Checker<> for storing subcheckers

[Szelethus]

Summary

Please don't take a shot for each time I write checker, it'd end bad.

The term "super checker" or "modeling checker", and the term "subchecker" always existed conceptually, but they weren't ever formalized. In the last half a year or so, we referred to them as

• Super/modeling checker: A checker that models some C++ code, but doesn't (or, as of now, just shouldn't) emit any diagnostics, at least not under its own name.
• Subcheckers: Checkers that are a part of super/modeling checkers, enabling/disabling them (ideally) only toggles whether a diagnostic is emitted from the checker it is a part of. They don't possess a checker object on their own, and are basically glorified checker options.

While checker dependencies were in similar shoes (existed conceptually but not formalized), this change isn't as critical, it just removes boilerplate code. When you look at IteratorChecker, SecuritySyntaxChecker or RetainCountBase they all use a similar, some cases faulty implementation to keep track of which subcheckers are enabled and what their name is, so its about time we combined them.

I envision this interface to be used to enforce our currently non-existent specification on the checker system.

In detail:

• Introduce SuperChecker:
  • It is essentially the same as Checker, but requires an enum template argument to keep track of which subcheckers are enabled.
  • While previously we defined subcheckers as checkers that don't have a checker object on their own, SuperChecker does create a CheckerBase object for them, but they still can't have checker callbacks.
• Add CheckerManager::registerSubChecker adds a new checker to a SuperChecker. It is ensured runtime that the SuperChecker was previously registered, and that a subchecker isn't registered multiple times.
• Add thorough test cases for the new interface.

[NoQ]

I have mixed feelings. Removing boilerplate is good, but the very fact that we're legalizing this pattern indicates that our checkers will keep bloating up, while i always wanted to actually split them instead (like, make sub-checkers into their own separate classes, possibly spread out into different files, kinda micro checkers as opposed to monolithic checkers (?)). But i guess it's about whoever gets things done first :)

I'd love to see how this affects our actual checkers, did you already try porting them? Do you plan to help with tracking per-sub-checker bug types and check names?

SuperChecker

WDYT about MultiChecker? ("A checker that implements multiple checks and presents them as different checkers.")

clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

113

The CXX23ModelingDiagKind:: qualifier is unnecessary here, right? Or did you mean to make an enum class? Does it even work with enum classes?

[baloghadamsoftware]

In D67336#1664168, @NoQ wrote:

I have mixed feelings. Removing boilerplate is good, but the very fact that we're legalizing this pattern indicates that our checkers will keep bloating up, while i always wanted to actually split them instead (like, make sub-checkers into their own separate classes, possibly spread out into different files, kinda micro checkers as opposed to monolithic checkers (?)). But i guess it's about whoever gets things done first :)

I completely agree with you about splitting the checkers. I also plan for the iterator checkers to separate them from the modelling and make a few mini checker classes beside the still huge modelling class. I am confident that that is the right direction.

[Szelethus]

I have mixed feelings. Removing boilerplate is good, but the very fact that we're legalizing this pattern indicates that our checkers will keep bloating up, while i always wanted to actually split them instead (like, make sub-checkers into their own separate classes, possibly spread out into different files, kinda micro checkers as opposed to monolithic checkers (?)).

The subchecker system as it works now is more diverse than that. Some systems use them purely for diagnostics (this patch is targeting those), while some affect the modeling as well. I think we should allow purely diagnostic checkers, because we can't really justify making an entire class for them, let alone an entire file, and they really are an integral part of the checker. However, we absolutely shouldn't promote adding further modeling into an existing checker whenever its avoidable.

The unfortunate truth is (at least the way I see it) is that we can't really force anyone to write better code. I like to think this patch neither legalizes nor prevents someone from bloating a file further, but rather introduces a new tool to split the giant checkers up, or make future additions more manageable.

The high level idea would be that when CallDescriptionMap is too simple, allow checkers to create their own events, and register subcheckers into them. This would for instance solve the problem of the unknown callback order of regular callbacks among checkers, which might be a better alternative then leaving this to CheckerRegistry.

Btw I tried to write this comment for literally months now, but I admit that I don't yet the where such a subsystem could be deployed in the already existing checkers. I always think of MallocChecker, but I don't see how we could do it there just yet.

@NoQ, in San José, you mentioned an example with std::set that would really demand a strong checker infrastructure, but I've since forgotten it. Could you explain it again please?

[martong]

The way I see perhaps we need a 3rd class of checkers (beside super and sub). And that would be those checkers which are not dependent closely on any super checker but they do emit diagnostics. E.g. the PlacementNewChecker is implemented in it's own, it emits some diagnostics, and does not model. However, it depends on MallocChecker's modeling when we are interested in dynamically allocated buffers sizes. Or would you add PlacementNewChecker as MallocChecker's subchecker? That seems a bit overkill to me.

On the other hand I see that e.g. MallocChecker should have several subcheckers (doubleDelete, etc). And these subcheckers do closely operate on the data stored in their super checker.
(Another approach could be if we have a global data storage where every modeling checker puts its own data, and other checkers can access this, actually Regions is one example to this.)

What is a CallDescriptionMap? Could you please explain further?

clang/include/clang/StaticAnalyzer/Core/Checker.h

 
 } // end eval namespace
 
 class CheckerBase : public ProgramPointTag {
   CheckerNameRef Name;
   friend class ::clang::ento::CheckerManager;
 
 public:
+  CheckerBase() = default;
+  CheckerBase(CheckerNameRef Name) : Name(Name) {}
   StringRef getTagDescription() const override;
   CheckerNameRef getCheckerName() const;
 
   /// See CheckerManager::runCheckersForPrintState.
   virtual void printState(raw_ostream &Out, ProgramStateRef State,
                           const char *NL, const char *Sep) const { }
 };
 
+
+template <typename SubCheckerEnumTy>
+class SuperCheckerBase : public CheckerBase {
+  static_assert(std::is_enum<SubCheckerEnumTy>::value,
+                "SuperCheckers are required to provide an enum to keep track "
+                "of their subcheckers!");
+
+  using SubCheckerPair = std::pair<SubCheckerEnumTy, const CheckerBase>;
+  using SubCheckerVector = typename llvm::SmallVector<SubCheckerPair, 4>;
+
+public:
+  using SubCheckerTy = SubCheckerEnumTy;
+
+private:
+  SubCheckerVector Subcheckers;
+
+  typename SubCheckerVector::const_iterator
+  getSubCheckerPos(SubCheckerEnumTy SubCheckerKind) const {
+    return llvm::find_if(Subcheckers, [SubCheckerKind](const auto &E) {
+      return E.first == SubCheckerKind;
+    });
+  }
+
+public:
+  template <SubCheckerEnumTy SubCheckerKind>
+  void addSubChecker(CheckerNameRef Name) {
+    assert(getSubCheckerPos(SubCheckerKind) == Subcheckers.end() &&
+           "This subchecker was already added to the superchecker!");
+    Subcheckers.emplace_back(SubCheckerKind, CheckerBase(Name));
+  }
+
+  template <SubCheckerEnumTy SubCheckerKind>
+  const CheckerBase *getSubChecker() const {
+    typename SubCheckerVector::const_iterator Pos =
+        getSubCheckerPos(SubCheckerKind);
+    if (Pos == Subcheckers.end())
+      return nullptr;
+    return &Pos->second;
+  }
+};
+
 /// Dump checker name to stream.
 raw_ostream& operator<<(raw_ostream &Out, const CheckerBase &Checker);
 
 /// Tag that can use a checker name as a message provider
 /// (see SimpleProgramPointTag).
 class CheckerProgramPointTag : public SimpleProgramPointTag {
 public:
   CheckerProgramPointTag(StringRef CheckerName, StringRef Msg);
   CheckerProgramPointTag(const CheckerBase *Checker, StringRef Msg);
 };
 
-template <typename CHECK1, typename... CHECKs>
-class Checker : public CHECK1, public CHECKs..., public CheckerBase {
+namespace checker_detail {
+template <typename BaseTy, typename CHECK1, typename... CHECKs>
+class CheckerImpl : public CHECK1, public CHECKs..., public BaseTy {
 public:
   template <typename CHECKER>
   static void _register(CHECKER *checker, CheckerManager &mgr) {
     CHECK1::_register(checker, mgr);
-    Checker<CHECKs...>::_register(checker, mgr);
+    CheckerImpl<BaseTy, CHECKs...>::_register(checker, mgr);
   }
 };
 
-template <typename CHECK1>
-class Checker<CHECK1> : public CHECK1, public CheckerBase {
+template <typename BaseTy, typename CHECK1>
+class CheckerImpl<BaseTy, CHECK1> : public CHECK1, public BaseTy {
 public:
   template <typename CHECKER>
   static void _register(CHECKER *checker, CheckerManager &mgr) {
     CHECK1::_register(checker, mgr);
   }
 };
+} // end of namespace checker_detail
+
+template <typename... CHECKs>
+using Checker =
+    typename checker_detail::CheckerImpl<CheckerBase, CHECKs...>;
+
+template <typename SubCheckerEnumTy, typename... CHECKs>
+using SuperChecker =
+    typename checker_detail::CheckerImpl<SuperCheckerBase<SubCheckerEnumTy>,
+                                         CHECKs...>;
 
 template <typename EVENT>
 class EventDispatcher {
   CheckerManager *Mgr;
 public:
   EventDispatcher() : Mgr(nullptr) { }
 
   template <typename CHECKER>

 struct DefaultBool {
   bool val;
   DefaultBool() : val(false) {}
   /*implicit*/ operator bool&() { return val; }
   /*implicit*/ operator const bool&() const { return val; }
   DefaultBool &operator=(bool b) { val = b; return *this; }
 };
 
-} // end ento namespace
-
-} // end clang namespace
+} // namespace ento
+} // namespace clang
 
 #endif

clang/include/clang/StaticAnalyzer/Core/CheckerManager.h

//===----------------------------------------------------------------------===//
   CHECKER *getChecker() {
     CheckerTag tag = getTag<CHECKER>();
     assert(CheckerTags.count(tag) != 0 &&
            "Requested checker is not registered! Maybe you should add it as a "
            "dependency in Checkers.td?");
     return static_cast<CHECKER *>(CheckerTags[tag]);
   }
 
+  /// Used to register subcheckers. Subcheckers aren't traditional checkers in
+  /// the sense that they don't have checker callbacks, but there is checker
+  /// object associated with them, which is retrievable though the checker they
+  /// are possessed by.
+  ///
+  /// \returns a pointer to the super checker object.
+  template <typename SuperChecker,
+            typename SuperChecker::SubCheckerTy SubCheckerKind>
+  SuperChecker *registerSubChecker() {
+    SuperChecker *Super = getChecker<SuperChecker>();
+    Super->template addSubChecker<SubCheckerKind>(CurrentCheckerName);
+    return Super;
+  }
+
 //===----------------------------------------------------------------------===//
 // Functions for running checkers for AST traversing.
 //===----------------------------------------------------------------------===//
 
   /// Run checkers handling Decls.
   void runCheckersOnASTDecl(const Decl *D, AnalysisManager& mgr,
                             BugReporter &BR);
 

clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

void addLocIncDecChecker(AnalysisASTConsumer &AnalysisConsumer,
   });
 }
 
 TEST(RegisterCustomCheckers, CheckLocationIncDec) {
   EXPECT_TRUE(
       runCheckerOnCode<addLocIncDecChecker>("void f() { int *p; (*p)++; }"));
 }
 
+//===----------------------------------------------------------------------===//
+// Subchecker system.
+//===----------------------------------------------------------------------===//
+
+enum CXX23ModelingDiagKind { IntPointer, NonLoad };
+
+class CXX23Modeling
+    : public SuperChecker<CXX23ModelingDiagKind, check::ASTCodeBody> {
+public:
+  void checkASTCodeBody(const Decl *D, AnalysisManager &Mgr,
+                        BugReporter &BR) const {
+    BR.EmitBasicReport(D, this, "Custom diagnostic", categories::LogicError,
+                       "Sketchy C++23 code modeled",
+                       PathDiagnosticLocation(D, Mgr.getSourceManager()), {});
+
+    if (const CheckerBase *IntPointerChecker = getSubChecker<IntPointer>())
+      BR.EmitBasicReport(D, IntPointerChecker, "Custom diagnostic",
+                         categories::LogicError, "Sketchy C++23 int pointer",
+                         PathDiagnosticLocation(D, Mgr.getSourceManager()), {});
+
+    if (const CheckerBase *NonLoadChecker = getSubChecker<NonLoad>())
+      BR.EmitBasicReport(D, NonLoadChecker, "Custom diagnostic",
+                         categories::LogicError,
+                         "Sketchy C++23 pointer non-loaded",
+                         PathDiagnosticLocation(D, Mgr.getSourceManager()), {});
+  }
+};
+
+void registerCXX23IntPointer(CheckerManager &Mgr) {
+  Mgr.registerSubChecker<CXX23Modeling, CXX23ModelingDiagKind::IntPointer>();
+}
+
+void registerCXX23NonLoad(CheckerManager &Mgr) {
+  Mgr.registerSubChecker<CXX23Modeling, CXX23ModelingDiagKind::NonLoad>();
+}
+
+void addButDontSpecifyCXX23Modeling(AnalysisASTConsumer &AnalysisConsumer,
+                      AnalyzerOptions &AnOpts) {
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+    Registry.addChecker<CXX23Modeling>("test.CXX23Modeling", "Description", "");
+  });
+}
+
+void addAndEnableCXX23Modeling(AnalysisASTConsumer &AnalysisConsumer,
+                      AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages = {{"test.CXX23Modeling", true}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+    Registry.addChecker<CXX23Modeling>("test.CXX23Modeling", "Description", "");
+  });
+}
+
+void addButDisableCXX23Modeling(AnalysisASTConsumer &AnalysisConsumer,
+                      AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages = {{"test.CXX23Modeling", false}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+    Registry.addChecker<CXX23Modeling>("test.CXX23Modeling", "Description", "");
+  });
+}
+
+void addCXX23IntPointer(AnalysisASTConsumer &AnalysisConsumer,
+                        AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages.emplace_back("test.CXX23IntPointer", true);
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+    Registry.addChecker(registerCXX23IntPointer, CheckerRegistry::returnTrue,
+                        "test.CXX23IntPointer", "Description", "",
+                        /*IsHidden*/ false);
+    Registry.addDependency("test.CXX23IntPointer", "test.CXX23Modeling");
+  });
+}
+
+void addCXX23NonLoad(AnalysisASTConsumer &AnalysisConsumer,
+                     AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages.emplace_back("test.CXX23NonLoad", true);
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+    Registry.addChecker(registerCXX23NonLoad, CheckerRegistry::returnTrue,
+                        "test.CXX23NonLoad", "Description", "",
+                        /*IsHidden*/ false);
+    Registry.addDependency("test.CXX23NonLoad", "test.CXX23Modeling");
+  });
+}
+
+TEST(RegisterCustomCheckers, SuperChecker) {
+  std::string Output;
+  EXPECT_TRUE(runCheckerOnCode<addAndEnableCXX23Modeling>(
+      "void foo(int *a) { *a; }", Output));
+  EXPECT_EQ(Output, "test.CXX23Modeling:Sketchy C++23 code modeled\n");
+
+  Output.clear();
+  bool ReturnValue =
+      runCheckerOnCode<addAndEnableCXX23Modeling, addCXX23IntPointer>(
+          "void foo(int *a) { *a; }", Output);
+  EXPECT_TRUE(ReturnValue);
+  EXPECT_EQ(Output, "test.CXX23Modeling:Sketchy C++23 code modeled\n"
+                    "test.CXX23IntPointer:Sketchy C++23 int pointer\n");
+
+  Output.clear();
+  ReturnValue =
+      runCheckerOnCode<addAndEnableCXX23Modeling, addCXX23IntPointer,
+                       addCXX23NonLoad>("void foo(int *a) { *a; }", Output);
+  EXPECT_TRUE(ReturnValue);
+  EXPECT_EQ(Output, "test.CXX23Modeling:Sketchy C++23 code modeled\n"
+                    "test.CXX23IntPointer:Sketchy C++23 int pointer\n"
+                    "test.CXX23NonLoad:Sketchy C++23 pointer non-loaded\n");
+
+  Output.clear();
+  ReturnValue =
+      runCheckerOnCode<addButDontSpecifyCXX23Modeling, addCXX23IntPointer,
+                       addCXX23NonLoad>("void foo(int *a) { *a; }", Output);
+  EXPECT_TRUE(ReturnValue);
+  EXPECT_EQ(Output, "test.CXX23Modeling:Sketchy C++23 code modeled\n"
+                    "test.CXX23IntPointer:Sketchy C++23 int pointer\n"
+                    "test.CXX23NonLoad:Sketchy C++23 pointer non-loaded\n");
+
+  Output.clear();
+  ReturnValue =
+      runCheckerOnCode<addButDisableCXX23Modeling, addCXX23IntPointer,
+                       addCXX23NonLoad>("void foo(int *a) { *a; }", Output);
+  EXPECT_TRUE(ReturnValue);
+  EXPECT_EQ(Output, "");
+}
+
 } // namespace
 } // namespace ento
 } // namespace clang