[NFC][analyzer] Use CheckerBase::getName in checker option handling (#131612)

The virtual method `ProgramPointTag::getTagDescription` had two very
distinct use cases:
- It is printed in the DOT graph visualization of the exploded graph
(that is, a debug printout).
- The checker option handling code used it to query the name of a
checker, which relied on the coincidence that in `CheckerBase` this
method is defined to be equivalent with `getName()`.

This commit switches to using `getName` in the second use case, because
this way we will be able to properly support checkers that have multiple
(separately named) parts.

The method `reportInvalidCheckerOptionName` is extended with an
additional overload that allows specifying the `CheckerPartIdx`. The
methods `getChecker*Option` could be extended analogously in the future,
but they are just convenience wrappers around the variants that directly
take `StringRef CheckerName`, so I'll only do this extension if it's
needed.

Author: NagyDonat

clang/include/clang/Analysis/ProgramPoint.h

@@ -39,6 +39,9 @@ class ProgramPointTag {
 public:
   ProgramPointTag(void *tagKind = nullptr) : TagKind(tagKind) {}
   virtual ~ProgramPointTag();
+
+  /// The description of this program point which will be displayed when the
+  /// ExplodedGraph is dumped in DOT format for debugging.
   virtual StringRef getTagDescription() const = 0;
 
   /// Used to implement 'isKind' in subclasses.

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

@@ -516,18 +516,11 @@ class CheckerBase : public ProgramPointTag {
   }
 
   StringRef getTagDescription() const override {
-    // This method inherited from `ProgramPointTag` has two unrelated roles:
-    // (1) The analyzer option handling logic uses this method to query the
-    //     name of a checker.
-    // (2) When the `ExplodedGraph` is dumped in DOT format for debugging,
-    //     this is called to attach a description to the nodes. (This happens
-    //     for all subclasses of `ProgramPointTag`, not just checkers.)
-    // FIXME: Application (1) should be aware of multiple parts within the same
-    // checker class instance, so it should directly use `getName` instead of
-    // this inherited interface which cannot support a `CheckerPartIdx`.
-    // FIXME: Ideally application (2) should return a string that describes the
-    // whole checker class, not just one of it parts. However, this is only for
-    // debugging, so returning the name of one part is probably good enough.
+    // When the ExplodedGraph is dumped for debugging (in DOT format), this
+    // method is called to attach a description to nodes created by this
+    // checker _class_. Ideally this should be recognizable identifier of the
+    // whole class, but for this debugging purpose it's sufficient to use the
+    // name of the first registered checker part.
     for (const auto &OptName : RegisteredNames)
       if (OptName)
         return *OptName;

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

@@ -194,6 +194,13 @@ class CheckerManager {
   /// Emits an error through a DiagnosticsEngine about an invalid user supplied
   /// checker option value.
   void reportInvalidCheckerOptionValue(const CheckerBase *C,
+                                       StringRef OptionName,
+                                       StringRef ExpectedValueDesc) const {
+    reportInvalidCheckerOptionValue(C, DefaultPart, OptionName,
+                                    ExpectedValueDesc);
+  }
+
+  void reportInvalidCheckerOptionValue(const CheckerBase *C, CheckerPartIdx Idx,
                                        StringRef OptionName,
                                        StringRef ExpectedValueDesc) const;
 

clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

@@ -154,8 +154,7 @@ StringRef AnalyzerOptions::getCheckerStringOption(StringRef CheckerName,
 StringRef AnalyzerOptions::getCheckerStringOption(const ento::CheckerBase *C,
                                                   StringRef OptionName,
                                                   bool SearchInParents) const {
-  return getCheckerStringOption(
-                           C->getTagDescription(), OptionName, SearchInParents);
+  return getCheckerStringOption(C->getName(), OptionName, SearchInParents);
 }
 
 bool AnalyzerOptions::getCheckerBooleanOption(StringRef CheckerName,
@@ -178,8 +177,7 @@ bool AnalyzerOptions::getCheckerBooleanOption(StringRef CheckerName,
 bool AnalyzerOptions::getCheckerBooleanOption(const ento::CheckerBase *C,
                                               StringRef OptionName,
                                               bool SearchInParents) const {
-  return getCheckerBooleanOption(
-             C->getTagDescription(), OptionName, SearchInParents);
+  return getCheckerBooleanOption(C->getName(), OptionName, SearchInParents);
 }
 
 int AnalyzerOptions::getCheckerIntegerOption(StringRef CheckerName,
@@ -199,6 +197,5 @@ int AnalyzerOptions::getCheckerIntegerOption(StringRef CheckerName,
 int AnalyzerOptions::getCheckerIntegerOption(const ento::CheckerBase *C,
                                              StringRef OptionName,
                                              bool SearchInParents) const {
-  return getCheckerIntegerOption(
-                           C->getTagDescription(), OptionName, SearchInParents);
+  return getCheckerIntegerOption(C->getName(), OptionName, SearchInParents);
 }

clang/lib/StaticAnalyzer/Core/CheckerManager.cpp

@@ -50,11 +50,11 @@ bool CheckerManager::hasPathSensitiveCheckers() const {
 }
 
 void CheckerManager::reportInvalidCheckerOptionValue(
-    const CheckerBase *C, StringRef OptionName,
+    const CheckerBase *C, CheckerPartIdx Idx, StringRef OptionName,
     StringRef ExpectedValueDesc) const {
 
   getDiagnostics().Report(diag::err_analyzer_checker_option_invalid_input)
-      << (llvm::Twine() + C->getTagDescription() + ":" + OptionName).str()
+      << (llvm::Twine(C->getName(Idx)) + ":" + OptionName).str()
       << ExpectedValueDesc;
 }
 

clang/unittests/StaticAnalyzer/AnalyzerOptionsTest.cpp

@@ -37,35 +37,25 @@ TEST(StaticAnalyzerOptions, SearchInParentPackageTests) {
   Opts.Config["Outer.Inner:Option2"] = "true";
   Opts.Config["Outer:Option2"] = "false";
 
-  struct CheckerOneMock : CheckerBase {
-    StringRef getTagDescription() const override {
-      return "Outer.Inner.CheckerOne";
-    }
-  };
-  struct CheckerTwoMock : CheckerBase {
-    StringRef getTagDescription() const override {
-      return "Outer.Inner.CheckerTwo";
-    }
-  };
+  StringRef CheckerOneName = "Outer.Inner.CheckerOne";
+  StringRef CheckerTwoName = "Outer.Inner.CheckerTwo";
 
   // CheckerTwo one has Option specified as true. It should read true regardless
   // of search mode.
-  CheckerOneMock CheckerOne;
-  EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerOne, "Option"));
+  EXPECT_TRUE(Opts.getCheckerBooleanOption(CheckerOneName, "Option"));
   // The package option is overridden with a checker option.
-  EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerOne, "Option", true));
+  EXPECT_TRUE(Opts.getCheckerBooleanOption(CheckerOneName, "Option", true));
   // The Outer package option is overridden by the Inner package option. No
   // package option is specified.
-  EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerOne, "Option2", true));
+  EXPECT_TRUE(Opts.getCheckerBooleanOption(CheckerOneName, "Option2", true));
   // No package option is specified and search in packages is turned off. We
   // should assert here, but we can't test that.
   //Opts.getCheckerBooleanOption(&CheckerOne, "Option2");
   //Opts.getCheckerBooleanOption(&CheckerOne, "Option2");
 
   // Checker true has no option specified. It should get false when search in
   // parents turned on.
-  CheckerTwoMock CheckerTwo;
-  EXPECT_FALSE(Opts.getCheckerBooleanOption(&CheckerTwo, "Option", true));
+  EXPECT_FALSE(Opts.getCheckerBooleanOption(CheckerTwoName, "Option", true));
   // In any other case, we should assert, that we cannot test unfortunately.
   //Opts.getCheckerBooleanOption(&CheckerTwo, "Option");
   //Opts.getCheckerBooleanOption(&CheckerTwo, "Option");
@@ -74,21 +64,6 @@ TEST(StaticAnalyzerOptions, SearchInParentPackageTests) {
 TEST(StaticAnalyzerOptions, StringOptions) {
   AnalyzerOptions Opts;
   Opts.Config["Outer.Inner.CheckerOne:Option"] = "StringValue";
-
-  struct CheckerOneMock : CheckerBase {
-    StringRef getTagDescription() const override {
-      return "Outer.Inner.CheckerOne";
-    }
-  };
-
-  CheckerOneMock CheckerOne;
-  EXPECT_TRUE("StringValue" ==
-            Opts.getCheckerStringOption(&CheckerOne, "Option"));
-}
-
-TEST(StaticAnalyzerOptions, SubCheckerOptions) {
-  AnalyzerOptions Opts;
-  Opts.Config["Outer.Inner.CheckerOne:Option"] = "StringValue";
   EXPECT_TRUE("StringValue" == Opts.getCheckerStringOption(
         "Outer.Inner.CheckerOne", "Option"));
 }