[analyzer] An ObjC for-in loop runs 0 times if the collection is nil.

In an Objective-C for-in loop "for (id element in collection) {}", the loop
will run 0 times if the collection is nil. This is because the for-in loop
is implemented using a protocol method that returns 0 when there are no
elements to iterate, and messages to nil will result in a 0 return value.

At some point we may want to actually model this message send, but for now
we may as well get the nil case correct, and avoid the false positives that
would come with this case.

<rdar://problem/13744632>

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@180639 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp b/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
index e0c3962cb668bcbc692b0a36d549a8a5742a7e2c..fba14a0fc4981845ea00710b9c390944554b4b5c 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
+++ b/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
@@ -759,38 +759,81 @@ static bool isKnownNonNilCollectionType(QualType T) {
   }
 }
 
-void ObjCLoopChecker::checkPostStmt(const ObjCForCollectionStmt *FCS,
-                                    CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
-  
-  // Check if this is the branch for the end of the loop.
-  SVal CollectionSentinel = State->getSVal(FCS, C.getLocationContext());
-  if (CollectionSentinel.isZeroConstant())
-    return;
-  
+/// Assumes that the collection is non-nil.
+///
+/// If the collection is known to be nil, returns NULL to indicate an infeasible
+/// path.
+static ProgramStateRef checkCollectionNonNil(CheckerContext &C,
+                                             ProgramStateRef State,
+                                             const ObjCForCollectionStmt *FCS) {
+  if (!State)
+    return NULL;
+
+  SVal CollectionVal = C.getSVal(FCS->getCollection());
+  Optional<DefinedSVal> KnownCollection = CollectionVal.getAs<DefinedSVal>();
+  if (!KnownCollection)
+    return State;
+
+  ProgramStateRef StNonNil, StNil;
+  llvm::tie(StNonNil, StNil) = State->assume(*KnownCollection);
+  if (StNil && !StNonNil) {
+    // The collection is nil. This path is infeasible.
+    return NULL;
+  }
+
+  return StNonNil;
+}
+
+/// Assumes that the collection elements are non-nil.
+///
+/// This only applies if the collection is one of those known not to contain
+/// nil values.
+static ProgramStateRef checkElementNonNil(CheckerContext &C,
+                                          ProgramStateRef State,
+                                          const ObjCForCollectionStmt *FCS) {
+  if (!State)
+    return NULL;
+
   // See if the collection is one where we /know/ the elements are non-nil.
-  const Expr *Collection = FCS->getCollection();
-  if (!isKnownNonNilCollectionType(Collection->getType()))
-    return;
-  
-  // FIXME: Copied from ExprEngineObjC.
+  if (!isKnownNonNilCollectionType(FCS->getCollection()->getType()))
+    return State;
+
+  const LocationContext *LCtx = C.getLocationContext();
   const Stmt *Element = FCS->getElement();
-  SVal ElementVar;
+
+  // FIXME: Copied from ExprEngineObjC.
+  Optional<Loc> ElementLoc;
   if (const DeclStmt *DS = dyn_cast<DeclStmt>(Element)) {
     const VarDecl *ElemDecl = cast<VarDecl>(DS->getSingleDecl());
     assert(ElemDecl->getInit() == 0);
-    ElementVar = State->getLValue(ElemDecl, C.getLocationContext());
+    ElementLoc = State->getLValue(ElemDecl, LCtx);
   } else {
-    ElementVar = State->getSVal(Element, C.getLocationContext());
+    ElementLoc = State->getSVal(Element, LCtx).getAs<Loc>();
   }
 
-  if (!ElementVar.getAs<Loc>())
-    return;
+  if (!ElementLoc)
+    return State;
 
   // Go ahead and assume the value is non-nil.
-  SVal Val = State->getSVal(ElementVar.castAs<Loc>());
-  State = State->assume(Val.castAs<DefinedOrUnknownSVal>(), true);
-  C.addTransition(State);
+  SVal Val = State->getSVal(*ElementLoc);
+  return State->assume(Val.castAs<DefinedOrUnknownSVal>(), true);
+}
+
+void ObjCLoopChecker::checkPostStmt(const ObjCForCollectionStmt *FCS,
+                                    CheckerContext &C) const {
+  // Check if this is the branch for the end of the loop.
+  SVal CollectionSentinel = C.getSVal(FCS);
+  if (CollectionSentinel.isZeroConstant())
+    return;
+
+  ProgramStateRef State = C.getState();
+  State = checkCollectionNonNil(C, State, FCS);
+  State = checkElementNonNil(C, State, FCS);
+
+  if (!State)
+    C.generateSink();
+  else if (State != C.getState())
+    C.addTransition(State);
 }
 
 namespace {

diff --git a/test/Analysis/objc-for.m b/test/Analysis/objc-for.m
index 1561ef8ddf852ac8d883a1542f576c4620d9c5da..ef149c4b14aae3f9d718a2f701fa59ec3e38caac 100644 (file)
--- a/test/Analysis/objc-for.m
+++ b/test/Analysis/objc-for.m
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=osx.cocoa.Loops,debug.ExprInspection -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,osx.cocoa.Loops,debug.ExprInspection -verify %s
 
 void clang_analyzer_eval(int);
 
@@ -56,3 +56,15 @@ void testWithVarInFor() {
     clang_analyzer_eval(x != nil); // expected-warning{{UNKNOWN}}
 }
 
+void testNonNil(id a, id b) {
+  clang_analyzer_eval(a != nil); // expected-warning{{UNKNOWN}}
+  for (id x in a)
+    clang_analyzer_eval(a != nil); // expected-warning{{TRUE}}
+
+  if (b != nil)
+    return;
+  for (id x in b)
+    *(volatile int *)0 = 1; // no-warning
+  clang_analyzer_eval(b != nil); // expected-warning{{FALSE}}
+}
+