[clang] [NFC][analyzer] Multipart checker refactor 2: NullabilityChecker (PR #132250)

[Balazs Benics via cfe-commits]

=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/132250 at github.com>


================
@@ -112,25 +112,30 @@ class NullabilityChecker
   void printState(raw_ostream &Out, ProgramStateRef State, const char *NL,
                   const char *Sep) const override;
 
-  enum CheckKind {
-    CK_NullPassedToNonnull,
-    CK_NullReturnedFromNonnull,
-    CK_NullableDereferenced,
-    CK_NullablePassedToNonnull,
-    CK_NullableReturnedFromNonnull,
-    CK_NumCheckKinds
+  // FIXME: This enumeration of checker parts is extremely similar to the
+  // ErrorKind enum. It would be nice to unify them to simplify the code.
+  enum : CheckerPartIdx {
+    NullPassedToNonnullChecker,
+    NullReturnedFromNonnullChecker,
+    NullableDereferencedChecker,
+    NullablePassedToNonnullChecker,
+    NullableReturnedFromNonnullChecker,
+    NumCheckerParts
   };
 
-  bool ChecksEnabled[CK_NumCheckKinds] = {false};
-  CheckerNameRef CheckNames[CK_NumCheckKinds];
-  mutable std::unique_ptr<BugType> BTs[CK_NumCheckKinds];
-
-  const std::unique_ptr<BugType> &getBugType(CheckKind Kind) const {
-    if (!BTs[Kind])
-      BTs[Kind].reset(new BugType(CheckNames[Kind], "Nullability",
-                                  categories::MemoryError));
-    return BTs[Kind];
-  }
+  // FIXME: Currently the `Description` fields of these `BugType`s are all
+  // identical ("Nullability") -- they should be more descriptive than this.
+  BugType BugTypes[NumCheckerParts] = {
+      {this, NullPassedToNonnullChecker, "Nullability",
+       categories::MemoryError},
+      {this, NullReturnedFromNonnullChecker, "Nullability",
+       categories::MemoryError},
+      {this, NullableDereferencedChecker, "Nullability",
+       categories::MemoryError},
+      {this, NullablePassedToNonnullChecker, "Nullability",
+       categories::MemoryError},
+      {this, NullableReturnedFromNonnullChecker, "Nullability",
+       categories::MemoryError}};
----------------
steakhal wrote:

I wonder if you could hoist stuff like this to make these fit in 1 line each.
```c++
  static constexpr llvm::StringLiteral Desc = "Nullability";
  static constexpr llvm::StringLiteral Cat = categories::MemoryError;
  BugType BugTypes[NumCheckerParts] = {
    {this, NullPassedToNonnullChecker, Desc, Cat},
    {this, NullReturnedFromNonnullChecker, Desc, Cat},
    {this, NullableDereferencedChecker, Desc, Cat},
    {this, NullablePassedToNonnullChecker, Desc, Cat},
    {this, NullableReturnedFromNonnullChecker, Desc, Cat},
  };
  const auto &NullPassedToNonnullBug = BugTypes[NullPassedToNonnullChecker];
  const auto &NullReturnedFromNonnullBug = BugTypes[NullReturnedFromNonnullChecker];
  const auto &NullableDereferencedBug = BugTypes[NullableDereferencedChecker];
  const auto &NullablePassedToNonnullBug = BugTypes[NullablePassedToNonnullChecker];
  const auto &NullableReturnedFromNonnullBug = BugTypes[NullableReturnedFromNonnullChecker];
```

This way later you could just directly refer to `NullPassedToNonnullBug`.

---

Every time I see this CheckerPart enum, I question myself, why do we need this? And I can't convince myself that this is the right way.

Correct me if I'm wrong, but we don't really need a BugType array. We also don't really need a checker part enum either.

We could design a system that is less coupled. Something like this:
```c++
// Assuming we have a strong-type something like this: struct CheckerPart {};
static inline CheckerPart NullPassedToNonnullChecker;
static inline CheckerPart NullReturnedFromNonnullChecker;
static inline CheckerPart NullableDereferencedChecker;
const BugType NullPassedToNonnullBug{this, NullPassedToNonnullChecker, Desc, Cat},
// more bug types.
```

The address of the CheckerPart object identifies the "part". The "CheckerPart" could contain a mutable bool flag to know if it's enabled or not.

https://github.com/llvm/llvm-project/pull/132250