[PATCH] D11427: [Static Analyzer] Checker for Obj-C generics

[Anna Zaks]

zaks.anna added a comment.

Here is a partial review. Some comments below and others are inline.

Thanks!
Anna.

- Add at least one test that tests for the full error message.
- Add a test that tests path-sensitive output (the plist file) after testing that that looks correct in Xcode. For example, how is GenericsBugVisitor tested? That should be easy with $ awk '{print "// CHECK: "$0}' in.txt > out.txt
- nit: It would be helpful if you could use descriptive names for each test.


================
Comment at: lib/StaticAnalyzer/Checkers/Checkers.td:456
@@ +455,3 @@
+def ObjCGenericsChecker : Checker<"ObjCGenerics">,
+  HelpText<"Checks for type errors.">,
+  DescFile<"ObjCGenericsChecker.cpp">;
----------------
Please, provide better description of the checker.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:11
@@ +10,3 @@
+// This checker tries to find type errors that the compiler is not able to catch
+// due to the implicit conversions that was introduced for backward
+// compatibility.
----------------
"was" -> "were"

Please, add some documentation on what this checker is trying to catch and how it works.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:92
@@ +91,3 @@
+// ProgramState trait - a map from symbol to its type with specified params.
+REGISTER_MAP_WITH_PROGRAMSTATE(TypeParamMap, SymbolRef,
+                               const ObjCObjectPointerType *)
----------------
"type with specified params" -> "specialized type"?

Also, maybe we should move this above the GenericsBugVisitor declaration for greater visibility?

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:173
@@ +172,3 @@
+    const ObjCObjectPointerType *MostInformativeCandidate, ASTContext &C) {
+  // Checking if from and two are the same classes modulo specialization.
+  if (From->getInterfaceDecl()->getCanonicalDecl() ==
----------------
"two" -> "to"

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:197
@@ +196,3 @@
+static const ObjCObjectPointerType *
+getMostInformativeDerivedClass(const ObjCObjectPointerType *From,
+                               const ObjCObjectPointerType *To, ASTContext &C) {
----------------
Could you add a comment on what this does?
Is there a precondition on the arguments (To and From)? We seem to be walking up the hierarchy of To.
Is To always guaranteed to have a super class? What ensures that?


================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:224
@@ +223,3 @@
+
+  ASTContext &ASTCtxt = C.getASTContext();
+
----------------
Move this after the check?

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:229
@@ +228,3 @@
+
+  // In order to detect subtype relation properly, strip the kindofness.
+  OrigObjectPtrType = OrigObjectPtrType->stripObjCKindOfTypeAndQuals(ASTCtxt);
----------------
Could you explain more why are we skipping kindoff?
Is there a test case for this?
Let's add more __kindof tests.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:253
@@ +252,3 @@
+  // If OrigObjectType could convert to DestObjectType, this could be an
+  // implicit cast. Handle it as implicit cast.
+  if (isa<ExplicitCastExpr>(CE) && !OrigToDest) {
----------------
Comment does not seem to match the code below.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:257
@@ +256,3 @@
+    // However a downcast may also lose information. E. g.:
+    //   MutableMap<T, U> : Map
+    // The downcast to mutable map loses the information about the types of the
----------------
Should the Map be specialized in this example?

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:270
@@ +269,3 @@
+    }
+    return;
+  }
----------------
We ignore the cast on the else branch (mismatched type)?

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:280
@@ +279,3 @@
+      State = State->set<TypeParamMap>(Sym, OrigObjectPtrType);
+      C.addTransition(State);
+    }
----------------
Simplify the code by using early returns.

Also, try to add comments with intuition on what cases are covered.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:297
@@ +296,3 @@
+        reportBug(*TrackedType, DestObjectPtrType, N, Sym, C);
+        return;
+      }
----------------
This will not get caught by the check below?

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:326
@@ +325,3 @@
+static const Expr *stripImplicitIdCast(const Expr *E, ASTContext &ASTCtxt) {
+  const ImplicitCastExpr *CE = dyn_cast<ImplicitCastExpr>(E);
+  if (CE && CE->getCastKind() == CK_BitCast &&
----------------
Shouldn't we strip more than id casts?

================
Comment at: test/Analysis/generics.m:45
@@ +44,3 @@
+ at interface MutableArray<T> : NSArray<T>
+- (int)addObject:(T)obj;
+ at end
----------------
I'd prefer if these were copied directly from the headers, without modifications. Ex:
@interface NSMutableArray<ObjectType> : NSArray<ObjectType>
- (void)addObject:(ObjectType)anObject;



http://reviews.llvm.org/D11427