[analyzer][NFCI] Remove ad-hoc program point tagging (#142980)

Previously some checkers attached explicitly created program point tags
to some of the exploded graph nodes that they created. In most of the
checkers this ad-hoc tagging only affected the debug dump of the
exploded graph (and they weren't too relevant for debugging) so this
commit removes them.

There were two checkers where the tagging _did_ have a functional role:
- In `RetainCountChecker` the presence of tags were checked by
`RefCountReportVisitor`.
- In `DynamicTypePropagation` the checker sometimes wanted to create two
identical nodes and had to apply an explicit tag on the second one to
avoid "caching out".
 
In these two situations I preserved the tags but switched to using
`SimpleProgramPointTag` instead of `CheckerProgramPointTag` because
`CheckerProgramPointTag` didn't provide enough benefits to justify its
existence.

Note that this commit depends on the earlier commit "[analyzer] Fix
tagging of PostAllocatorCall" ec96c0c
and would introduce crashes when cherry-picked onto a branch that
doesn't contain that commit.

For more details about the background see the discourse thread
https://discourse.llvm.org/t/role-of-programpointtag-in-the-static-analyzer/

As a tangentially related changes, this commit also adds some comments
to document the surprising behavior of `CheckerContext::addTransition`
and an assertion in the constructor of `PathSensitiveBugReport` to get a
more readable crash dump in the case when the report is constructed with
`nullptr` as the `ErrorNode`. (This can happen due to "caching out".)

Author: NagyDonat

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

@@ -608,20 +608,6 @@ class EventDispatcher {
   }
 };
 
-/// Tag that can use a checker name as a message provider
-/// (see SimpleProgramPointTag).
-/// FIXME: This is a cargo cult class which is copied into several checkers but
-/// does not provide anything useful.
-/// The only added functionality provided by this class (compared to
-/// SimpleProgramPointTag) is that it composes the tag description string from
-/// two arguments -- but tag descriptions only appear in debug output so there
-/// is no reason to bother with this.
-class CheckerProgramPointTag : public SimpleProgramPointTag {
-public:
-  CheckerProgramPointTag(StringRef CheckerName, StringRef Msg);
-  CheckerProgramPointTag(const CheckerBase *Checker, StringRef Msg);
-};
-
 /// We dereferenced a location that may be null.
 struct ImplicitNullDerefEvent {
   SVal Location;

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h

@@ -171,6 +171,9 @@ class CheckerContext {
   ///        tag is specified, a default tag, unique to the given checker,
   ///        will be used. Tags are used to prevent states generated at
   ///        different sites from caching out.
+  /// NOTE: If the State is unchanged and the Tag is nullptr, this may return a
+  /// node which is not tagged (instead of using the default tag corresponding
+  /// to the active checker). This is arguably a bug and should be fixed.
   ExplodedNode *addTransition(ProgramStateRef State = nullptr,
                               const ProgramPointTag *Tag = nullptr) {
     return addTransitionImpl(State ? State : getState(), false, nullptr, Tag);
@@ -183,6 +186,9 @@ class CheckerContext {
   /// @param Pred The transition will be generated from the specified Pred node
   ///             to the newly generated node.
   /// @param Tag The tag to uniquely identify the creation site.
+  /// NOTE: If the State is unchanged and the Tag is nullptr, this may return a
+  /// node which is not tagged (instead of using the default tag corresponding
+  /// to the active checker). This is arguably a bug and should be fixed.
   ExplodedNode *addTransition(ProgramStateRef State, ExplodedNode *Pred,
                               const ProgramPointTag *Tag = nullptr) {
     return addTransitionImpl(State, false, Pred, Tag);

clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp

@@ -671,7 +671,6 @@ void CallAndMessageChecker::HandleNilReceiver(CheckerContext &C,
                                               ProgramStateRef state,
                                               const ObjCMethodCall &Msg) const {
   ASTContext &Ctx = C.getASTContext();
-  static CheckerProgramPointTag Tag(this, "NilReceiver");
 
   // Check the return type of the message expression.  A message to nil will
   // return different values depending on the return type and the architecture.
@@ -682,7 +681,7 @@ void CallAndMessageChecker::HandleNilReceiver(CheckerContext &C,
   if (CanRetTy->isStructureOrClassType()) {
     // Structure returns are safe since the compiler zeroes them out.
     SVal V = C.getSValBuilder().makeZeroVal(RetTy);
-    C.addTransition(state->BindExpr(Msg.getOriginExpr(), LCtx, V), &Tag);
+    C.addTransition(state->BindExpr(Msg.getOriginExpr(), LCtx, V));
     return;
   }
 
@@ -701,7 +700,7 @@ void CallAndMessageChecker::HandleNilReceiver(CheckerContext &C,
             Ctx.LongDoubleTy == CanRetTy ||
             Ctx.LongLongTy == CanRetTy ||
             Ctx.UnsignedLongLongTy == CanRetTy)))) {
-      if (ExplodedNode *N = C.generateErrorNode(state, &Tag))
+      if (ExplodedNode *N = C.generateErrorNode(state))
         emitNilReceiverBug(C, Msg, N);
       return;
     }
@@ -720,7 +719,7 @@ void CallAndMessageChecker::HandleNilReceiver(CheckerContext &C,
     // of this case unless we have *a lot* more knowledge.
     //
     SVal V = C.getSValBuilder().makeZeroVal(RetTy);
-    C.addTransition(state->BindExpr(Msg.getOriginExpr(), LCtx, V), &Tag);
+    C.addTransition(state->BindExpr(Msg.getOriginExpr(), LCtx, V));
     return;
   }
 

clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp

@@ -674,9 +674,16 @@ void DynamicTypePropagation::checkPostStmt(const CastExpr *CE,
   if (TrackedType &&
       !ASTCtxt.canAssignObjCInterfaces(DestObjectPtrType, *TrackedType) &&
       !ASTCtxt.canAssignObjCInterfaces(*TrackedType, DestObjectPtrType)) {
-    static CheckerProgramPointTag IllegalConv(this, "IllegalConversion");
-    ExplodedNode *N = C.addTransition(State, AfterTypeProp, &IllegalConv);
-    reportGenericsBug(*TrackedType, DestObjectPtrType, N, Sym, C);
+    // This distinct program point tag is needed because `State` can be
+    // identical to the state of the node `AfterTypeProp`, and in that case
+    // `generateNonFatalErrorNode` would "cache out" and return nullptr
+    // (instead of re-creating an already existing node).
+    static SimpleProgramPointTag IllegalConv("DynamicTypePropagation",
+                                             "IllegalConversion");
+    ExplodedNode *N =
+        C.generateNonFatalErrorNode(State, AfterTypeProp, &IllegalConv);
+    if (N)
+      reportGenericsBug(*TrackedType, DestObjectPtrType, N, Sym, C);
     return;
   }
 
@@ -885,8 +892,7 @@ void DynamicTypePropagation::checkPreObjCMessage(const ObjCMethodCall &M,
     // Warn when argument is incompatible with the parameter.
     if (!ASTCtxt.canAssignObjCInterfaces(ParamObjectPtrType,
                                          ArgObjectPtrType)) {
-      static CheckerProgramPointTag Tag(this, "ArgTypeMismatch");
-      ExplodedNode *N = C.addTransition(State, &Tag);
+      ExplodedNode *N = C.generateNonFatalErrorNode(State);
       reportGenericsBug(ArgObjectPtrType, ParamObjectPtrType, N, Sym, C, Arg);
       return;
     }

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

@@ -1043,8 +1043,7 @@ bool GenericTaintChecker::generateReportIfTainted(const Expr *E, StringRef Msg,
 
   // Generate diagnostic.
   assert(BT);
-  static CheckerProgramPointTag Tag(BT->getCheckerName(), Msg);
-  if (ExplodedNode *N = C.generateNonFatalErrorNode(C.getState(), &Tag)) {
+  if (ExplodedNode *N = C.generateNonFatalErrorNode(C.getState())) {
     auto report = std::make_unique<PathSensitiveBugReport>(*BT, Msg, N);
     report->addRange(E->getSourceRange());
     for (auto TaintedSym : getTaintedSymbols(C.getState(), *TaintedSVal)) {

clang/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp

@@ -747,9 +747,7 @@ void NonLocalizedStringChecker::reportLocalizationError(
   if (isDebuggingContext(C))
     return;
 
-  static CheckerProgramPointTag Tag("NonLocalizedStringChecker",
-                                    "UnlocalizedString");
-  ExplodedNode *ErrNode = C.addTransition(C.getState(), C.getPredecessor(), &Tag);
+  ExplodedNode *ErrNode = C.generateNonFatalErrorNode();
 
   if (!ErrNode)
     return;

clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp

@@ -73,7 +73,6 @@ void MPIChecker::checkUnmatchedWaits(const CallEvent &PreCallEvent,
     return;
 
   ProgramStateRef State = Ctx.getState();
-  static CheckerProgramPointTag Tag("MPI-Checker", "UnmatchedWait");
   ExplodedNode *ErrorNode{nullptr};
 
   // Check all request regions used by the wait function.
@@ -82,7 +81,7 @@ void MPIChecker::checkUnmatchedWaits(const CallEvent &PreCallEvent,
     State = State->set<RequestMap>(ReqRegion, Request::State::Wait);
     if (!Req) {
       if (!ErrorNode) {
-        ErrorNode = Ctx.generateNonFatalErrorNode(State, &Tag);
+        ErrorNode = Ctx.generateNonFatalErrorNode(State);
         State = ErrorNode->getState();
       }
       // A wait has no matching nonblocking call.
@@ -105,7 +104,6 @@ void MPIChecker::checkMissingWaits(SymbolReaper &SymReaper,
   if (Requests.isEmpty())
     return;
 
-  static CheckerProgramPointTag Tag("MPI-Checker", "MissingWait");
   ExplodedNode *ErrorNode{nullptr};
 
   auto ReqMap = State->get<RequestMap>();
@@ -114,7 +112,7 @@ void MPIChecker::checkMissingWaits(SymbolReaper &SymReaper,
       if (Req.second.CurrentState == Request::State::Nonblocking) {
 
         if (!ErrorNode) {
-          ErrorNode = Ctx.generateNonFatalErrorNode(State, &Tag);
+          ErrorNode = Ctx.generateNonFatalErrorNode(State);
           State = ErrorNode->getState();
         }
         BReporter.reportMissingWait(Req.second, Req.first, ErrorNode,

clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp

@@ -556,8 +556,7 @@ void MacOSKeychainAPIChecker::checkDeadSymbols(SymbolReaper &SR,
     return;
   }
 
-  static CheckerProgramPointTag Tag(this, "DeadSymbolsLeak");
-  ExplodedNode *N = C.generateNonFatalErrorNode(C.getState(), &Tag);
+  ExplodedNode *N = C.generateNonFatalErrorNode(C.getState());
   if (!N)
     return;
 

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

@@ -3098,8 +3098,7 @@ void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper,
   // Generate leak node.
   ExplodedNode *N = C.getPredecessor();
   if (!Errors.empty()) {
-    static CheckerProgramPointTag Tag("MallocChecker", "DeadSymbolsLeak");
-    N = C.generateNonFatalErrorNode(C.getState(), &Tag);
+    N = C.generateNonFatalErrorNode(C.getState());
     if (N) {
       for (SymbolRef Sym : Errors) {
         HandleLeak(Sym, N, C);

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

@@ -69,6 +69,7 @@ const char *getNullabilityString(Nullability Nullab) {
 }
 
 // These enums are used as an index to ErrorMessages array.
+// FIXME: ErrorMessages no longer exists, perhaps remove this as well?
 enum class ErrorKind : int {
   NilAssignedToNonnull,
   NilPassedToNonnull,
@@ -714,8 +715,7 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
   if (ChecksEnabled[CK_NullReturnedFromNonnull] && NullReturnedFromNonNull &&
       RetExprTypeLevelNullability != Nullability::Nonnull &&
       !InSuppressedMethodFamily) {
-    static CheckerProgramPointTag Tag(this, "NullReturnedFromNonnull");
-    ExplodedNode *N = C.generateErrorNode(State, &Tag);
+    ExplodedNode *N = C.generateErrorNode(State);
     if (!N)
       return;
 
@@ -750,8 +750,7 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
         Nullness != NullConstraint::IsNotNull &&
         TrackedNullabValue == Nullability::Nullable &&
         RequiredNullability == Nullability::Nonnull) {
-      static CheckerProgramPointTag Tag(this, "NullableReturnedFromNonnull");
-      ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
+      ExplodedNode *N = C.addTransition(State, C.getPredecessor());
 
       SmallString<256> SBuf;
       llvm::raw_svector_ostream OS(SBuf);
@@ -1299,8 +1298,7 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,
       ValNullability != Nullability::Nonnull &&
       ValueExprTypeLevelNullability != Nullability::Nonnull &&
       !isARCNilInitializedLocal(C, S)) {
-    static CheckerProgramPointTag Tag(this, "NullPassedToNonnull");
-    ExplodedNode *N = C.generateErrorNode(State, &Tag);
+    ExplodedNode *N = C.generateErrorNode(State);
     if (!N)
       return;
 
@@ -1342,8 +1340,7 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,
       return;
     if (ChecksEnabled[CK_NullablePassedToNonnull] &&
         LocNullability == Nullability::Nonnull) {
-      static CheckerProgramPointTag Tag(this, "NullablePassedToNonnull");
-      ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
+      ExplodedNode *N = C.addTransition(State, C.getPredecessor());
       reportBugIfInvariantHolds("Nullable pointer is assigned to a pointer "
                                 "which is expected to have non-null value",
                                 ErrorKind::NullableAssignedToNonnull,