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

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

Addressed the suggestions.


================
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
----------------
zaks.anna wrote:
> Should the Map be specialized in this example?
No, this case handles buggy implementations, when the type parameters are not forwarded. I reworded the comments to make this more clear.

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

================
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 &&
----------------
zaks.anna wrote:
> Shouldn't we strip more than id casts?
We should. And we also should strip some sugar. I rewrote that function.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:425
@@ +424,3 @@
+         ASTCtxt.canAssignObjCInterfaces(ReceiverObjectPtrType,
+                                         *TrackedType))) {
+      const ObjCInterfaceDecl *InterfaceDecl =
----------------
zaks.anna wrote:
> What if ASTCtxt.canAssignObjCInterfaces is false here? Shouldn't we warn?
> Will we warn elsewhere? Let's make sure we have a test case. And add a comment.
The tracked type might be the super class of the static type (for example when the super type is specialized but the subtype is not). We should not warn in that case. When the tracked type is not in subtyping relationship with the static type, there was a cast somewhere, and the checker issued the warning there. I already have testcase for these cases. I updated the names of the tests to reflect this. 

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:444
@@ +443,3 @@
+  Optional<ArrayRef<QualType>> TypeArgs =
+      (*TrackedType)->getObjCSubstitutions(Method->getDeclContext());
+  // This case might happen when there is an unspecialized override of a
----------------
zaks.anna wrote:
> Should we use the type on which this is defined and not the tracked type?
The type arguments might be only available for the tracked type.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:453
@@ +452,3 @@
+    // We can't do any type-checking on a type-dependent argument.
+    if (Arg->isTypeDependent())
+      continue;
----------------
zaks.anna wrote:
> Why are we not checking other parameter types? Will those bugs get caught by casts? I guess yes..
The analyzer does not visit template definitions, only the instantiations. I removed this redundant check.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:495
@@ +494,3 @@
+    // accepted.
+    if (!ASTCtxt.canAssignObjCInterfaces(ParamObjectPtrType,
+                                         ArgObjectPtrType) &&
----------------
zaks.anna wrote:
> This would be simplified if you pull out the negation; you essentially, list the cases where we do not warn on parameters:
>  1) arg can be assigned to (subtype of) param
>  2) covariant parameter types and param is a subtype of arg
The check there was overcomplicated. Passing a subtype of a parameter as argument should always be safe.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:513
@@ +512,3 @@
+      ASTCtxt, *TypeArgs, ObjCSubstitutionContext::Result);
+  if (IsInstanceType)
+    ResultType = QualType(*TrackedType, 0);
----------------
zaks.anna wrote:
> We should not use TrackedType in the case lookup failed.
> Can the TrackedType be less specialized that the return type?
> Maybe rename as IsDeclaredAsInstanceType?
The lookup might fail when the tracked type is the super of the dynamic type (which might be the subtype of the static type). In some cases the tracked type might even end up be the super type of the static type (when that super type has more information about the type parameters.) 

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:532
@@ +531,3 @@
+
+  if (!ExprTypeAboveCast || !ResultPtrType)
+    return;
----------------
zaks.anna wrote:
> What are we doing here?
> I prefer not to have any AST pattern matching.
Right now in order to keep the state as low as possible, only generic types are tracked. If it is not a problem to have bigger state, a future improvement would be to track the dynamic type of every object, and utilize that info in this checker. Alternatively that could utilize the get/setDynamicTypeInfo API of ProgramState.

================
Comment at: test/Analysis/generics.m:45
@@ +44,3 @@
+ at interface MutableArray<T> : NSArray<T>
+- (int)addObject:(T)obj;
+ at end
----------------
zaks.anna wrote:
> I'd prefer if these were copied directly from the headers, without modifications. Ex:
> @interface NSMutableArray<ObjectType> : NSArray<ObjectType>
> - (void)addObject:(ObjectType)anObject;
> 
I added some additional methods for NSArray to do some additional testing, but I made the rest of the methods more similar to the ones in the actual header files.


http://reviews.llvm.org/D11427