[Analyzer] Replace `CXXSelfAssignmentBRVisitor` with `NoteTags`

[baloghadamsoftware]

Summary

The cplusplus.SelfAssignment checker has a visitor that is added to every BugReport to mark the to branch of the self assignment operator with e.g. rhs == *this and rhs != *this. With the new NoteTag feature this visitor is not needed anymore. Instead the checker itself marks the two branches using the NoteTags.

[NoQ]

Yay, cool!

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

2388–2390

Looks like you almost invented note tags here :)

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

736–738

Could you assert that this is indeed the case? Like, check that the "from" CFGBlock of the edge is the current CFG's ENTRY block? 'Cause it's not obvious that the beginning of the function is the correct source location in all cases where the CFGTerminator is not present (you should also be able to assert that the CFGTerminator is not present, but i don't think it's the right thing to assert, as we might add more kinds of statement-less CFGTerminators in the future).

[Szelethus]

I love this.

include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h

public:
   /// to make all PathDiagnosticPieces created by this visitor.
   static const char *getTag();
 
   std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *Succ,
                                                  BugReporterContext &BRC,
                                                  BugReport &BR) override;
 };
 
-class CXXSelfAssignmentBRVisitor final : public BugReporterVisitor {
-  bool Satisfied = false;
-
-public:
-  CXXSelfAssignmentBRVisitor() = default;
-
-  void Profile(llvm::FoldingSetNodeID &ID) const override {}
-
-  std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *Succ,
-                                                 BugReporterContext &BRC,
-                                                 BugReport &BR) override;
-};
-
-
 /// The bug visitor will walk all the nodes in a path and collect all the
 /// constraints. When it reaches the root node, will create a refutation
 /// manager and check if the constraints are satisfiable
 class FalsePositiveRefutationBRVisitor final : public BugReporterVisitor {
 private:
   /// Holds the constraints in a given path
   ConstraintRangeTy Constraints;
 

lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp

void CXXSelfAssignmentChecker::checkBeginFunction(CheckerContext &C) const {
   if (!MD->isCopyAssignmentOperator() && !MD->isMoveAssignmentOperator())
     return;
   auto &State = C.getState();
   auto &SVB = C.getSValBuilder();
   auto ThisVal =
       State->getSVal(SVB.getCXXThis(MD, LCtx->getStackFrame()));
   auto Param = SVB.makeLoc(State->getRegion(MD->getParamDecl(0), LCtx));
   auto ParamVal = State->getSVal(Param);
+
   ProgramStateRef SelfAssignState = State->bindLoc(Param, ThisVal, LCtx);
-  C.addTransition(SelfAssignState);
+  const NoteTag *SelfAssignTag =
+    C.getNoteTag([MD](BugReport &BR) -> std::string {
+        SmallString<256> Msg;
+        llvm::raw_svector_ostream Out(Msg);
+        Out << "Assuming " << MD->getParamDecl(0)->getName() << " == *this";
+        return Out.str();
+      });
+  C.addTransition(SelfAssignState, SelfAssignTag);
+
   ProgramStateRef NonSelfAssignState = State->bindLoc(Param, ParamVal, LCtx);
-  C.addTransition(NonSelfAssignState);
+  const NoteTag *NonSelfAssignTag =
+    C.getNoteTag([MD](BugReport &BR) -> std::string {
+        SmallString<256> Msg;
+        llvm::raw_svector_ostream Out(Msg);
+        Out << "Assuming " << MD->getParamDecl(0)->getName() << " != *this";
+        return Out.str();
+      });
+  C.addTransition(NonSelfAssignState, NonSelfAssignTag);
 }
 
 void ento::registerCXXSelfAssignmentChecker(CheckerManager &Mgr) {
   Mgr.registerChecker<CXXSelfAssignmentChecker>();
 }
 
 bool ento::shouldRegisterCXXSelfAssignmentChecker(const LangOptions &LO) {
   return true;
 }

lib/StaticAnalyzer/Core/BugReporter.cpp

while (TrimG.popNextReportGraph(ErrorGraph)) {
 
     // Register refutation visitors first, if they mark the bug invalid no
     // further analysis is required
     R->addVisitor(llvm::make_unique<LikelyFalsePositiveSuppressionBRVisitor>());
 
     // Register additional node visitors.
     R->addVisitor(llvm::make_unique<NilReceiverBRVisitor>());
     R->addVisitor(llvm::make_unique<ConditionBRVisitor>());
-    R->addVisitor(llvm::make_unique<CXXSelfAssignmentBRVisitor>());
     R->addVisitor(llvm::make_unique<TagVisitor>());
 
     BugReporterContext BRC(Reporter, ErrorGraph.BackMap);
 
     // Run all visitors on a given graph, once.
     std::unique_ptr<VisitorsDiagnosticsTy> visitorNotes =
         generateVisitorsDiagnostics(R, ErrorNode, BRC);
 

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

for (const auto ParamDecl : parms) {
     if (BoundVal.isUndef() || BoundVal.isZeroConstant()) {
       BR.markInteresting(CEnter->getCalleeContext());
       return nullptr;
     }
   }
   return nullptr;
 }
 
-std::shared_ptr<PathDiagnosticPiece>
-CXXSelfAssignmentBRVisitor::VisitNode(const ExplodedNode *Succ,
-                                      BugReporterContext &BRC, BugReport &) {
-  if (Satisfied)
-    return nullptr;
-
-  const auto Edge = Succ->getLocation().getAs<BlockEdge>();
-  if (!Edge.hasValue())
-    return nullptr;
-
-  auto Tag = Edge->getTag();
-  if (!Tag)
-    return nullptr;
-
-  if (Tag->getTagDescription() != "cplusplus.SelfAssignment")
-    return nullptr;
-
-  Satisfied = true;
-
-  const auto *Met =
-      dyn_cast<CXXMethodDecl>(Succ->getCodeDecl().getAsFunction());
-  assert(Met && "Not a C++ method.");
-  assert((Met->isCopyAssignmentOperator() || Met->isMoveAssignmentOperator()) &&
-         "Not a copy/move assignment operator.");
-
-  const auto *LCtx = Edge->getLocationContext();
-
-  const auto &State = Succ->getState();
-  auto &SVB = State->getStateManager().getSValBuilder();
-
-  const auto Param =
-      State->getSVal(State->getRegion(Met->getParamDecl(0), LCtx));
-  const auto This =
-      State->getSVal(SVB.getCXXThis(Met, LCtx->getStackFrame()));
-
-  auto L = PathDiagnosticLocation::create(Met, BRC.getSourceManager());
-
-  if (!L.isValid() || !L.asLocation().isValid())
-    return nullptr;
-
-  SmallString<256> Buf;
-  llvm::raw_svector_ostream Out(Buf);
-
-  Out << "Assuming " << Met->getParamDecl(0)->getName() <<
-    ((Param == This) ? " == " : " != ") << "*this";
-
-  auto Piece = std::make_shared<PathDiagnosticEventPiece>(L, Out.str());
-  Piece->addRange(Met->getSourceRange());
-
-  return std::move(Piece);
-}
-
-
 FalsePositiveRefutationBRVisitor::FalsePositiveRefutationBRVisitor()
     : Constraints(ConstraintRangeTy::Factory().getEmptyMap()) {}
 
 void FalsePositiveRefutationBRVisitor::finalizeVisitor(
     BugReporterContext &BRC, const ExplodedNode *EndPathNode, BugReport &BR) {
   // Collect new constraints
   VisitNode(EndPathNode, BRC, BR);
 

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

if (Optional<BlockEdge> BE = P.getAs<BlockEdge>()) {
     if (BSrc->getTerminator().isVirtualBaseBranch()) {
       // TODO: VirtualBaseBranches should also appear for destructors.
       // In this case we should put the diagnostic at the end of decl.
       return PathDiagnosticLocation::createBegin(
           P.getLocationContext()->getDecl(), SMng);
 
     } else {
       S = BSrc->getTerminatorCondition();
+      if (!S) {
+        // If the BlockEdge has no terminator condition statement but its
+        // source is the entry of the CFG (e.g. a checker crated the branch at
+        // the beginning of a function), use the function's declaration instead.
+        assert(BSrc == &BSrc->getParent()->getEntry() && "CFGBlock has no "
+               "TerminatorCondition and is not the enrty block of the CFG");
+        return PathDiagnosticLocation::createBegin(
+            P.getLocationContext()->getDecl(), SMng);
+      }
     }
   } else if (Optional<StmtPoint> SP = P.getAs<StmtPoint>()) {
     S = SP->getStmt();
     if (P.getAs<PostStmtPurgeDeadSymbols>())
       return PathDiagnosticLocation::createEnd(S, SMng, P.getLocationContext());
   } else if (Optional<PostInitializer> PIP = P.getAs<PostInitializer>()) {
     return PathDiagnosticLocation(PIP->getInitializer()->getSourceLocation(),
                                   SMng);