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

[Anna Zaks]

zaks.anna added a comment.

Add tests for checking element retrieval and tracking with ObjC subscript syntax for arrays and dictionaries.

Also see:
http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return


================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:334
@@ +333,3 @@
+
+void ObjCGenericsChecker::checkPostObjCMessage(const ObjCMethodCall &M,
+                                               CheckerContext &C) const {
----------------
Add comment explaining that this callback is used to infer the types. Specifically, if a class method is called on a symbol, we use it to infer the type of the symbol.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:338
@@ +337,3 @@
+
+  ProgramStateRef State = C.getState();
+  SymbolRef Sym = M.getReturnValue().getAsSymbol();
----------------
Move State down, where it's used.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:346
@@ +345,3 @@
+  if (MessageExpr->getReceiverKind() != ObjCMessageExpr::Class ||
+      Sel.getAsString() != "class")
+    return;
----------------
Clarify what you are doing here.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:414
@@ +413,3 @@
+
+  // When the receiver type is id, or some super class of the tracked type (and
+  // kindof type), look up the method in the tracked type, not in the receiver
----------------
Split funding a tighter method definition into a subroutine.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:421
@@ +420,3 @@
+      MessageExpr->getReceiverKind() == ObjCMessageExpr::Class) {
+    if (ASTCtxt.getObjCIdType() == ReceiverType ||
+        ASTCtxt.getObjCClassType() == ReceiverType ||
----------------
Should checking for id, class and kindof be a helper method?

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:425
@@ +424,3 @@
+         ASTCtxt.canAssignObjCInterfaces(ReceiverObjectPtrType,
+                                         *TrackedType))) {
+      const ObjCInterfaceDecl *InterfaceDecl =
----------------
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.

================
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
----------------
Should we use the type on which this is defined and not 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;
----------------
Why are we not checking other parameter types? Will those bugs get caught by casts? I guess yes..

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:458
@@ +457,3 @@
+
+    QualType OrigParamType = Param->getType();
+    const auto *ParamTypedef = OrigParamType->getAs<TypedefType>();
----------------
Why isObjCTypeParamDependent is not used here?

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:485
@@ +484,3 @@
+    if (ArgSym) {
+      const ObjCObjectPointerType *const *TrackedType =
+          State->get<TypeParamMap>(ArgSym);
----------------
Let's not shadow TrackedType.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:495
@@ +494,3 @@
+    // accepted.
+    if (!ASTCtxt.canAssignObjCInterfaces(ParamObjectPtrType,
+                                         ArgObjectPtrType) &&
----------------
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

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:500
@@ +499,3 @@
+                                          ParamObjectPtrType))) {
+      ExplodedNode *N = C.addTransition();
+      reportBug(ArgObjectPtrType, ParamObjectPtrType, N, Sym, C);
----------------
This just returns the previous state. 

If you want to create a node, you need to tag it; see the tags on leak reports.


================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:513
@@ +512,3 @@
+      ASTCtxt, *TypeArgs, ObjCSubstitutionContext::Result);
+  if (IsInstanceType)
+    ResultType = QualType(*TrackedType, 0);
----------------
We should not use TrackedType in the case lookup failed.
Can the TrackedType be less specialized that the return type?
Maybe rename as IsDeclaredAsInstanceType?

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:532
@@ +531,3 @@
+
+  if (!ExprTypeAboveCast || !ResultPtrType)
+    return;
----------------
What are we doing here?
I prefer not to have any AST pattern matching.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:539
@@ +538,3 @@
+      !ASTCtxt.canAssignObjCInterfaces(ResultPtrType, ExprTypeAboveCast)) {
+    ExplodedNode *N = C.addTransition();
+    reportBug(ResultPtrType, ExprTypeAboveCast, N, Sym, C);
----------------
This returns predecessor..


http://reviews.llvm.org/D11427