[analyzer] NFC: Legalize state manager factory injection.

[NoQ]

Summary

When a checker maintains a program state trait that isn't a simple list/set/map, but is a combination of multiple lists/sets/maps (eg., a multimap - which may be implemented as a map from something to set of something), ProgramStateManager only contains the factory for the trait itself. All auxiliary lists/sets/maps need a factory to be provided by the checker, which is annoying.

So far two checkers wanted a multimap, and both decided to trick the ProgramStateManager into keeping the auxiliary factory within itself by pretending that it's some sort of trait they're interested in, but then never using this trait but only using the factory.

I decided to make it legal because this functionality seems useful and i didn't want to dig deeply into the template magic around state traits when there's already a one-liner solution, though i agree that it should be possible to implement this in a more straightforward manner.

One thing that becomes apparent once all pieces are put together is that these two checkers are in fact using the same factory, because the type that identifies it, ImmutableMap<const MemRegion *, ImmutableSet<SymbolRef>>, is the same. This situation is different from two checkers registering similar traits.

I also moved stuff around a bit because it always bothered me.

[george.karpenkov]

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

33

What happened to the comment about multiple TUs?
One still has to be very careful here, right?

[NoQ]

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

33

D52905#1257040.

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

 #define LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_CHECKERCONTEXT_H
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 
 namespace clang {
 namespace ento {
 
-  /// Declares an immutable map of type \p NameTy, suitable for placement into
-  /// the ProgramState. This is implementing using llvm::ImmutableMap.
-  ///
-  /// \code
-  /// State = State->set<Name>(K, V);
-  /// const Value *V = State->get<Name>(K); // Returns NULL if not in the map.
-  /// State = State->remove<Name>(K);
-  /// NameTy Map = State->get<Name>();
-  /// \endcode
-  ///
-  /// The macro should not be used inside namespaces, or for traits that must
-  /// be accessible from more than one translation unit.
-  #define REGISTER_MAP_WITH_PROGRAMSTATE(Name, Key, Value) \
-    REGISTER_TRAIT_WITH_PROGRAMSTATE(Name, \
-                                     CLANG_ENTO_PROGRAMSTATE_MAP(Key, Value))
-
-  /// Declares an immutable set of type \p NameTy, suitable for placement into
-  /// the ProgramState. This is implementing using llvm::ImmutableSet.
-  ///
-  /// \code
-  /// State = State->add<Name>(E);
-  /// State = State->remove<Name>(E);
-  /// bool Present = State->contains<Name>(E);
-  /// NameTy Set = State->get<Name>();
-  /// \endcode
-  ///
-  /// The macro should not be used inside namespaces, or for traits that must
-  /// be accessible from more than one translation unit.
-  #define REGISTER_SET_WITH_PROGRAMSTATE(Name, Elem) \
-    REGISTER_TRAIT_WITH_PROGRAMSTATE(Name, llvm::ImmutableSet<Elem>)
-
-  /// Declares an immutable list of type \p NameTy, suitable for placement into
-  /// the ProgramState. This is implementing using llvm::ImmutableList.
-  ///
-  /// \code
-  /// State = State->add<Name>(E); // Adds to the /end/ of the list.
-  /// bool Present = State->contains<Name>(E);
-  /// NameTy List = State->get<Name>();
-  /// \endcode
-  ///
-  /// The macro should not be used inside namespaces, or for traits that must
-  /// be accessible from more than one translation unit.
-  #define REGISTER_LIST_WITH_PROGRAMSTATE(Name, Elem) \
-    REGISTER_TRAIT_WITH_PROGRAMSTATE(Name, llvm::ImmutableList<Elem>)
-
-
 class CheckerContext {
   ExprEngine &Eng;
   /// The current exploded(symbolic execution) graph node.
   ExplodedNode *Pred;
   /// The flag is true if the (state of the execution) has been modified
   /// by the checker using this context. For example, a new transition has been
   /// added or a bug report issued.
   bool Changed;

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

 
 namespace clang {
 namespace ento {
 
   template <typename T> struct ProgramStatePartialTrait;
 
   /// Declares a program state trait for type \p Type called \p Name, and
   /// introduce a type named \c NameTy.
-  /// The macro should not be used inside namespaces, or for traits that must
-  /// be accessible from more than one translation unit.
+  /// The macro should not be used inside namespaces.
   #define REGISTER_TRAIT_WITH_PROGRAMSTATE(Name, Type) \
     namespace { \
       class Name {}; \
       using Name ## Ty = Type; \
     } \
     namespace clang { \
     namespace ento { \
       template <> \
       struct ProgramStateTrait<Name> \
         : public ProgramStatePartialTrait<Name ## Ty> { \
         static void *GDMIndex() { static int Index; return &Index; } \
       }; \
     } \
     }
 
+  /// Declares a factory for objects of type \p Type in the program state
+  /// manager. The type must provide a ::Factory sub-class. Commonly used for
+  /// ImmutableMap, ImmutableSet, ImmutableList. The macro should not be used
+  /// inside namespaces.
+  #define REGISTER_FACTORY_WITH_PROGRAMSTATE(Type) \
+    namespace clang { \
+    namespace ento { \
+      template <> \
+      struct ProgramStateTrait<Type> \
+        : public ProgramStatePartialTrait<Type> { \
+        static void *GDMIndex() { static int Index; return &Index; } \
+      }; \
+    } \
+    }
+
+  /// Helper for registering a map trait.
+  ///
+  /// If the map type were written directly in the invocation of
+  /// REGISTER_TRAIT_WITH_PROGRAMSTATE, the comma in the template arguments
+  /// would be treated as a macro argument separator, which is wrong.
+  /// This allows the user to specify a map type in a way that the preprocessor
+  /// can deal with.
+  #define CLANG_ENTO_PROGRAMSTATE_MAP(Key, Value) llvm::ImmutableMap<Key, Value>
+
+  /// Declares an immutable map of type \p NameTy, suitable for placement into
+  /// the ProgramState. This is implementing using llvm::ImmutableMap.
+  ///
+  /// \code
+  /// State = State->set<Name>(K, V);
+  /// const Value *V = State->get<Name>(K); // Returns NULL if not in the map.
+  /// State = State->remove<Name>(K);
+  /// NameTy Map = State->get<Name>();
+  /// \endcode
+  ///
+  /// The macro should not be used inside namespaces, or for traits that must
+  /// be accessible from more than one translation unit.
+  #define REGISTER_MAP_WITH_PROGRAMSTATE(Name, Key, Value) \
+    REGISTER_TRAIT_WITH_PROGRAMSTATE(Name, \
+                                     CLANG_ENTO_PROGRAMSTATE_MAP(Key, Value))
+
+  /// Declares an immutable map type \p Name and registers the factory
+  /// for such maps in the program state, but does not add the map itself
+  /// to the program state. Useful for managing lifetime of maps that are used
+  /// as elements of other program state data structures.
+  #define REGISTER_MAP_FACTORY_WITH_PROGRAMSTATE(Name, Key, Value) \
+    using Name = llvm::ImmutableMap<Key, Value>; \
+    REGISTER_FACTORY_WITH_PROGRAMSTATE(Name)
+
+
+  /// Declares an immutable set of type \p NameTy, suitable for placement into
+  /// the ProgramState. This is implementing using llvm::ImmutableSet.
+  ///
+  /// \code
+  /// State = State->add<Name>(E);
+  /// State = State->remove<Name>(E);
+  /// bool Present = State->contains<Name>(E);
+  /// NameTy Set = State->get<Name>();
+  /// \endcode
+  ///
+  /// The macro should not be used inside namespaces, or for traits that must
+  /// be accessible from more than one translation unit.
+  #define REGISTER_SET_WITH_PROGRAMSTATE(Name, Elem) \
+    REGISTER_TRAIT_WITH_PROGRAMSTATE(Name, llvm::ImmutableSet<Elem>)
+
+  /// Declares an immutable set type \p Name and registers the factory
+  /// for such sets in the program state, but does not add the set itself
+  /// to the program state. Useful for managing lifetime of sets that are used
+  /// as elements of other program state data structures.
+  #define REGISTER_SET_FACTORY_WITH_PROGRAMSTATE(Name, Elem) \
+    using Name = llvm::ImmutableSet<Elem>; \
+    REGISTER_FACTORY_WITH_PROGRAMSTATE(Name)
+
+
+  /// Declares an immutable list type \p NameTy, suitable for placement into
+  /// the ProgramState. This is implementing using llvm::ImmutableList.
+  ///
+  /// \code
+  /// State = State->add<Name>(E); // Adds to the /end/ of the list.
+  /// bool Present = State->contains<Name>(E);
+  /// NameTy List = State->get<Name>();
+  /// \endcode
+  ///
+  /// The macro should not be used inside namespaces, or for traits that must
+  /// be accessible from more than one translation unit.
+  #define REGISTER_LIST_WITH_PROGRAMSTATE(Name, Elem) \
+    REGISTER_TRAIT_WITH_PROGRAMSTATE(Name, llvm::ImmutableList<Elem>)
+
+  /// Declares an immutable list of type \p Name and registers the factory
+  /// for such lists in the program state, but does not add the list itself
+  /// to the program state. Useful for managing lifetime of lists that are used
+  /// as elements of other program state data structures.
+  #define REGISTER_LIST_FACTORY_WITH_PROGRAMSTATE(Name, Elem) \
+    using Name = llvm::ImmutableList<Elem>; \
+    REGISTER_FACTORY_WITH_PROGRAMSTATE(Name)
+
+
   // Partial-specialization for ImmutableMap.
   template <typename Key, typename Data, typename Info>
   struct ProgramStatePartialTrait<llvm::ImmutableMap<Key, Data, Info>> {
     using data_type = llvm::ImmutableMap<Key, Data, Info>;
     using context_type = typename data_type::Factory &;
     using key_type = Key;
     using value_type = Data;
     using lookup_type = const value_type *;
static void *CreateContext(llvm::BumpPtrAllocator& Alloc) {
       return new typename data_type::Factory(Alloc);
     }
 
     static void DeleteContext(void *Ctx) {
       delete (typename data_type::Factory *) Ctx;
     }
   };
 
-  /// Helper for registering a map trait.
-  ///
-  /// If the map type were written directly in the invocation of
-  /// REGISTER_TRAIT_WITH_PROGRAMSTATE, the comma in the template arguments
-  /// would be treated as a macro argument separator, which is wrong.
-  /// This allows the user to specify a map type in a way that the preprocessor
-  /// can deal with.
-  #define CLANG_ENTO_PROGRAMSTATE_MAP(Key, Value) llvm::ImmutableMap<Key, Value>
-
   // Partial-specialization for ImmutableSet.
   template <typename Key, typename Info>
   struct ProgramStatePartialTrait<llvm::ImmutableSet<Key, Info>> {
     using data_type = llvm::ImmutableSet<Key, Info>;
     using context_type = typename data_type::Factory &;
     using key_type = Key;
 
     static data_type MakeData(void *const *p) {

lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp

private:
 
   bool classHasSeparateTeardown(const ObjCInterfaceDecl *ID) const;
 
   bool isReleasedByCIFilterDealloc(const ObjCPropertyImplDecl *PropImpl) const;
   bool isNibLoadedIvarWithoutRetain(const ObjCPropertyImplDecl *PropImpl) const;
 };
 } // End anonymous namespace.
 
-typedef llvm::ImmutableSet<SymbolRef> SymbolSet;
 
 /// Maps from the symbol for a class instance to the set of
 /// symbols remaining that must be released in -dealloc.
+REGISTER_SET_FACTORY_WITH_PROGRAMSTATE(SymbolSet, SymbolRef)
 REGISTER_MAP_WITH_PROGRAMSTATE(UnreleasedIvarMap, SymbolRef, SymbolSet)
 
-namespace clang {
-namespace ento {
-template<> struct ProgramStateTrait<SymbolSet>
-:  public ProgramStatePartialTrait<SymbolSet> {
-  static void *GDMIndex() { static int index = 0; return &index; }
-};
-}
-}
 
 /// An AST check that diagnose when the class requires a -dealloc method and
 /// is missing one.
 void ObjCDeallocChecker::checkASTDecl(const ObjCImplementationDecl *D,
                                       AnalysisManager &Mgr,
                                       BugReporter &BR) const {
   assert(Mgr.getLangOpts().getGC() != LangOptions::GCOnly);
   assert(!Mgr.getLangOpts().ObjCAutoRefCount);

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

 #include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 
 using namespace clang;
 using namespace ento;
 
-using PtrSet = llvm::ImmutableSet<SymbolRef>;
-
 // Associate container objects with a set of raw pointer symbols.
+REGISTER_SET_FACTORY_WITH_PROGRAMSTATE(PtrSet, SymbolRef);
 REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, PtrSet)
 
-// This is a trick to gain access to PtrSet's Factory.
-namespace clang {
-namespace ento {
-template <>
-struct ProgramStateTrait<PtrSet> : public ProgramStatePartialTrait<PtrSet> {
-  static void *GDMIndex() {
-    static int Index = 0;
-    return &Index;
-  }
-};
-} // end namespace ento
-} // end namespace clang
 
 namespace {
 
 class InnerPointerChecker
     : public Checker<check::DeadSymbols, check::PostCall> {
 
   CallDescription AppendFn, AssignFn, ClearFn, CStrFn, DataFn, EraseFn,
       InsertFn, PopBackFn, PushBackFn, ReplaceFn, ReserveFn, ResizeFn,

lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

 // consider this region's information precise or not along the given path.
 namespace {
   enum DynamicDispatchMode {
     DynamicDispatchModeInlined = 1,
     DynamicDispatchModeConservative
   };
 } // end anonymous namespace
 
-REGISTER_TRAIT_WITH_PROGRAMSTATE(DynamicDispatchBifurcationMap,
-                                 CLANG_ENTO_PROGRAMSTATE_MAP(const MemRegion *,
-                                                             unsigned))
+REGISTER_MAP_WITH_PROGRAMSTATE(DynamicDispatchBifurcationMap,
+                               const MemRegion *, unsigned)
 
 bool ExprEngine::inlineCall(const CallEvent &Call, const Decl *D,
                             NodeBuilder &Bldr, ExplodedNode *Pred,
                             ProgramStateRef State) {
   assert(D);
 
   const LocationContext *CurLC = Pred->getLocationContext();
   const StackFrameContext *CallerSFC = CurLC->getStackFrame();