[PATCH] D78286: [analyzer] Track runtime types represented by Obj-C Class objects

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:135
+  if (MsgE->getReceiverKind() == ObjCMessageExpr::Class) {
+    return {MsgE->getClassReceiver()->getAs<ObjCObjectType>(), true};
+  }
----------------
Let's explain what `true` means here, i.e. `/*Precise=*/true`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:163
+
+  // Check if the receiver is 'self'.
+  if (const DeclRefExpr *DRE =
----------------
I suspect that `self`, unlike `this`, can be reassigned in the middle of the method.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:164-165
+  // Check if the receiver is 'self'.
+  if (const DeclRefExpr *DRE =
+          dyn_cast<DeclRefExpr>(RecE->IgnoreParenImpCasts())) {
+    const StackFrameContext *SFCtx = C.getStackFrame();
----------------
That's a good place for `const auto *`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:208-211
+  if (const auto *ReceiverInferredType =
+          dyn_cast<ObjCObjectPointerType>(InferredType)) {
+    return {ReceiverInferredType->getObjectType()};
+  }
----------------
When exactly does this situation happen? Can we predict it during pattern-matching instead of patching it up after the fact?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:217-218
 
 void DynamicTypePropagation::checkDeadSymbols(SymbolReaper &SR,
                                               CheckerContext &C) const {
   ProgramStateRef State = removeDeadTypes(C.getState(), SR);
----------------
This needs to be filled in for the new trait as well! It's very important :3


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:324-325
+        // We used to assume that whatever type we got from inferring the
+        // type is actually precise (which is not exactly correct), and some
+        // the features of the analyzer depend directly on this.
+        //
----------------
Looks like a missed word somewhere.


================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicType.cpp:37
 
+// A map from Class object symbols to the most likely contained type.
+REGISTER_MAP_WITH_PROGRAMSTATE(DynamicClassObjectMap, clang::ento::SymbolRef,
----------------
I'd rather word this as "pointed-to type".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78286/new/

https://reviews.llvm.org/D78286