[analyzer] Decouple NoteTag from its Factory

[vsavchenko]

Summary

This allows us to create other types of tags that carry useful
bits of information alongside.

[NoQ]

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

777

It sounds like NoteTag isn't<DataTag> despite inheriting from it.

[NoQ]

P.S. I like this design!

I'm trying to remember why we needed a factory in the first place. I think a lot of tags work just fine as static variables. In case of D104136 IIUC IdentityCall could always be discovered as the current program point and its Source is its only argument so all we need is a marker that says "we did indeed model this code as identity" so it could potentially be allocated statically. But when the tag really needs to carry more data then there's probably no way around the factory.

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

777

Wait nvm, DataTag doesn't provide a classof at all. This means we can't write isa<DataTag> at all, right?

[vsavchenko]

In D104135#2814081, @NoQ wrote:

P.S. I like this design!

I'm trying to remember why we needed a factory in the first place.

Who's gonna carry your callbacks for ya, huh?

I think a lot of tags work just fine as static variables.

Sure, but if you have data, let's say it's something that doesn't need to be stored somewhere to prolong its lifetime, you still need to have static variable for each possible value of that piece of data. Not really feasible in the vast majority of cases. Because you might need to refer to the same tag with different possible values.

In case of D104136 IIUC IdentityCall could always be discovered as the current program point...

That's true, I put this one as "just in case".

...and its Source is its only argument...

Not really, there is also case when the call is IdentityThis, so its Source is not its only argument.

...so all we need is a marker that says "we did indeed model this code as identity" so it could potentially be allocated statically. But when the tag really needs to carry more data then there's probably no way around the factory.

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

777

Yes, it's purely abstract. ProgramPointTag also doesn't have classof

[NoQ]

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

777

Something being purely abstract isn't a problem on its own; there are a lot of abstract classes that implement classof(), such as Expr. It's more of an artifact of the current implementation strategy.

Ok np though, this doesn't seem to be a real problem.

[vsavchenko]

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

777

That's true. And considering that we want different checkers to implement this on their own, we probably don't want to have one central enum listing all possible subtypes so that we can isa non-leaf types.
But unlike Expr, DataTag doesn't provide any information, so it's useless for people to check if something is a data tag or cast to it.

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

const SourceManager& getSourceManager() const {
     return BR.getSourceManager();
   }
 
   const AnalyzerOptions &getAnalyzerOptions() const {
     return BR.getAnalyzerOptions();
   }
 };
 
+/// The tag that carries some information with it.
+///
+/// It can be valuable to produce tags with some bits of information and later
+/// reuse them for a better diagnostic.
+///
+/// Please make sure that derived class' constuctor is private and that the user
+/// can only create objects using DataTag::Factory.  This also means that
+/// DataTag::Factory should be friend for every derived class.
+class DataTag : public ProgramPointTag {
+public:
+  StringRef getTagDescription() const override { return "Data Tag"; }
+
+  // Manage memory for DataTag objects.
+  class Factory {
+    std::vector<std::unique_ptr<DataTag>> Tags;
+
+  public:
+    template <class DataTagType, class... Args>
+    const DataTagType *make(Args &&... ConstructorArgs) {
+      // We cannot use std::make_unique because we cannot access the private
+      // constructor from inside it.
+      Tags.emplace_back(
+          new DataTagType(std::forward<Args>(ConstructorArgs)...));
+      return static_cast<DataTagType *>(Tags.back().get());
+    }
+  };
+
+protected:
+  DataTag(void *TagKind) : ProgramPointTag(TagKind) {}
+};
 
 /// The tag upon which the TagVisitor reacts. Add these in order to display
 /// additional PathDiagnosticEventPieces along the path.
-class NoteTag : public ProgramPointTag {
+class NoteTag : public DataTag {
 public:
-  using Callback =
-      std::function<std::string(BugReporterContext &,
+  using Callback = std::function<std::string(BugReporterContext &,
                                              PathSensitiveBugReport &)>;
 
 private:
   static int Kind;
 
   const Callback Cb;
   const bool IsPrunable;
 
   NoteTag(Callback &&Cb, bool IsPrunable)
-      : ProgramPointTag(&Kind), Cb(std::move(Cb)), IsPrunable(IsPrunable) {}
+      : DataTag(&Kind), Cb(std::move(Cb)), IsPrunable(IsPrunable) {}
 
 public:
   static bool classof(const ProgramPointTag *T) {
     return T->getTagKind() == &Kind;
   }
 
   Optional<std::string> generateMessage(BugReporterContext &BRC,
                                         PathSensitiveBugReport &R) const {
     std::string Msg = Cb(BRC, R);
     if (Msg.empty())
       return None;
 
     return std::move(Msg);
   }
 
   StringRef getTagDescription() const override {
     // TODO: Remember a few examples of generated messages
     // and display them in the ExplodedGraph dump by
     // returning them from this function.
     return "Note Tag";
   }
 
   bool isPrunable() const { return IsPrunable; }
 
-  // Manage memory for NoteTag objects.
-  class Factory {
-    std::vector<std::unique_ptr<NoteTag>> Tags;
-
-  public:
-    const NoteTag *makeNoteTag(Callback &&Cb, bool IsPrunable = false) {
-      // We cannot use std::make_unique because we cannot access the private
-      // constructor from inside it.
-      std::unique_ptr<NoteTag> T(new NoteTag(std::move(Cb), IsPrunable));
-      Tags.push_back(std::move(T));
-      return Tags.back().get();
-    }
-  };
-
+  friend class Factory;
   friend class TagVisitor;
 };
 
 } // namespace ento
 
 } // namespace clang
 
 #endif // LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTER_H

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

public:
   /// node-by-node to restore the sequence of events that led to discovering
   /// a bug, you can add notes as you add your transitions.
   ///
   /// @param Cb Callback with 'BugReporterContext &, BugReport &' parameters.
   /// @param IsPrunable Whether the note is prunable. It allows BugReporter
   ///        to omit the note from the report if it would make the displayed
   ///        bug path significantly shorter.
   const NoteTag *getNoteTag(NoteTag::Callback &&Cb, bool IsPrunable = false) {
-    return Eng.getNoteTags().makeNoteTag(std::move(Cb), IsPrunable);
+    return Eng.getDataTags().make<NoteTag>(std::move(Cb), IsPrunable);
   }
 
   /// A shorthand version of getNoteTag that doesn't require you to accept
   /// the 'BugReporterContext' argument when you don't need it.
   ///
   /// @param Cb Callback only with 'BugReport &' parameter.
   /// @param IsPrunable Whether the note is prunable. It allows BugReporter
   ///        to omit the note from the report if it would make the displayed

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

private:
   /// The locations where we stopped because the engine aborted analysis,
   /// usually because it could not reason about something.
   BlocksAborted blocksAborted;
 
   /// The information about functions shared by the whole translation unit.
   /// (This data is owned by AnalysisConsumer.)
   FunctionSummariesTy *FunctionSummaries;
 
-  /// Add path note tags along the path when we see that something interesting
-  /// is happening. This field is the allocator for such tags.
-  NoteTag::Factory NoteTags;
+  /// Add path tags with some useful data along the path when we see that
+  /// something interesting is happening. This field is the allocator for such
+  /// tags.
+  DataTag::Factory DataTags;
 
   void generateNode(const ProgramPoint &Loc,
                     ProgramStateRef State,
                     ExplodedNode *Pred);
 
   void HandleBlockEdge(const BlockEdge &E, ExplodedNode *Pred);
   void HandleBlockEntrance(const BlockEntrance &E, ExplodedNode *Pred);
   void HandleBlockExit(const CFGBlock *B, ExplodedNode *Pred);
public:
 
   /// enqueue the nodes corresponding to the end of function onto the
   /// end of path / work list.
   void enqueueEndOfFunction(ExplodedNodeSet &Set, const ReturnStmt *RS);
 
   /// Enqueue a single node created as a result of statement processing.
   void enqueueStmtNode(ExplodedNode *N, const CFGBlock *Block, unsigned Idx);
 
-  NoteTag::Factory &getNoteTags() { return NoteTags; }
+  DataTag::Factory &getDataTags() { return DataTags; }
 };
 
 // TODO: Turn into a class.
 struct NodeBuilderContext {
   const CoreEngine &Eng;
   const CFGBlock *Block;
   const LocationContext *LC;
 

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

public:
   // FIXME: Remove when we migrate over to just using SValBuilder.
   BasicValueFactory &getBasicVals() {
     return StateMgr.getBasicVals();
   }
 
   SymbolManager &getSymbolManager() { return SymMgr; }
   MemRegionManager &getRegionManager() { return MRMgr; }
 
-  NoteTag::Factory &getNoteTags() { return Engine.getNoteTags(); }
-
+  DataTag::Factory &getDataTags() { return Engine.getDataTags(); }
 
   // Functions for external checking of whether we have unfinished work
   bool wasBlocksExhausted() const { return Engine.wasBlocksExhausted(); }
   bool hasEmptyWorkList() const { return !Engine.getWorkList()->hasWork(); }
   bool hasWorkRemaining() const { return Engine.hasWorkRemaining(); }
 
   const CoreEngine &getCoreEngine() const { return Engine; }
 

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp

void CoreEngine::HandleBlockEdge(const BlockEdge &L, ExplodedNode *Pred) {
   FunctionSummaries->markVisitedBasicBlock(Blk->getBlockID(),
                                            LC->getDecl(),
                                            LC->getCFG()->getNumBlockIDs());
 
   // Display a prunable path note to the user if it's a virtual bases branch
   // and we're taking the path that skips virtual base constructors.
   if (L.getSrc()->getTerminator().isVirtualBaseBranch() &&
       L.getDst() == *L.getSrc()->succ_begin()) {
-    ProgramPoint P = L.withTag(getNoteTags().makeNoteTag(
+    ProgramPoint P = L.withTag(getDataTags().make<NoteTag>(
         [](BugReporterContext &, PathSensitiveBugReport &) -> std::string {
           // TODO: Just call out the name of the most derived class
           // when we know it.
           return "Virtual base initialization skipped because "
                  "it has already been handled by the most derived class";
-        }, /*IsPrunable=*/true));
+        },
+        /*IsPrunable=*/true));
     // Perform the transition.
     ExplodedNodeSet Dst;
     NodeBuilder Bldr(Pred, Dst, BuilderCtx);
     Pred = Bldr.generateNode(P, Pred->getState(), Pred);
     if (!Pred)
       return;
   }