1 files changed, 55 insertions, 39 deletions

diff --git a/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
index f5e64f4a5a8c6..d76b9cbcfaca4 100644
--- a/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
+++ b/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
@@ -42,6 +42,47 @@ void ExprEngine::VisitObjCAtSynchronizedStmt(const ObjCAtSynchronizedStmt *S,
   getCheckerManager().runCheckersForPreStmt(Dst, Pred, S, *this);
 }
 
+/// Generate a node in \p Bldr for an iteration statement using ObjC
+/// for-loop iterator.
+static void populateObjCForDestinationSet(
+    ExplodedNodeSet &dstLocation, SValBuilder &svalBuilder,
+    const ObjCForCollectionStmt *S, const Stmt *elem, SVal elementV,
+    SymbolManager &SymMgr, const NodeBuilderContext *currBldrCtx,
+    StmtNodeBuilder &Bldr, bool hasElements) {
+
+  for (ExplodedNode *Pred : dstLocation) {
+    ProgramStateRef state = Pred->getState();
+    const LocationContext *LCtx = Pred->getLocationContext();
+
+    SVal hasElementsV = svalBuilder.makeTruthVal(hasElements);
+
+    // FIXME: S is not an expression. We should not be binding values to it.
+    ProgramStateRef nextState = state->BindExpr(S, LCtx, hasElementsV);
+
+    if (auto MV = elementV.getAs<loc::MemRegionVal>())
+      if (const auto *R = dyn_cast<TypedValueRegion>(MV->getRegion())) {
+        // FIXME: The proper thing to do is to really iterate over the
+        //  container.  We will do this with dispatch logic to the store.
+        //  For now, just 'conjure' up a symbolic value.
+        QualType T = R->getValueType();
+        assert(Loc::isLocType(T));
+
+        SVal V;
+        if (hasElements) {
+          SymbolRef Sym = SymMgr.conjureSymbol(elem, LCtx, T,
+                                               currBldrCtx->blockCount());
+          V = svalBuilder.makeLoc(Sym);
+        } else {
+          V = svalBuilder.makeIntVal(0, T);
+        }
+
+        nextState = nextState->bindLoc(elementV, V, LCtx);
+      }
+
+    Bldr.generateNode(S, Pred, nextState);
+  }
+}
+
 void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S,
                                             ExplodedNode *Pred,
                                             ExplodedNodeSet &Dst) {
@@ -72,60 +113,35 @@ void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S,
   //    result in state splitting.
 
   const Stmt *elem = S->getElement();
+  const Stmt *collection = S->getCollection();
   ProgramStateRef state = Pred->getState();
-  SVal elementV;
+  SVal collectionV = state->getSVal(collection, Pred->getLocationContext());
 
-  if (const DeclStmt *DS = dyn_cast<DeclStmt>(elem)) {
+  SVal elementV;
+  if (const auto *DS = dyn_cast<DeclStmt>(elem)) {
     const VarDecl *elemD = cast<VarDecl>(DS->getSingleDecl());
     assert(elemD->getInit() == nullptr);
     elementV = state->getLValue(elemD, Pred->getLocationContext());
-  }
-  else {
+  } else {
     elementV = state->getSVal(elem, Pred->getLocationContext());
   }
 
+  bool isContainerNull = state->isNull(collectionV).isConstrainedTrue();
+
   ExplodedNodeSet dstLocation;
   evalLocation(dstLocation, S, elem, Pred, state, elementV, nullptr, false);
 
   ExplodedNodeSet Tmp;
   StmtNodeBuilder Bldr(Pred, Tmp, *currBldrCtx);
 
-  for (ExplodedNodeSet::iterator NI = dstLocation.begin(),
-       NE = dstLocation.end(); NI!=NE; ++NI) {
-    Pred = *NI;
-    ProgramStateRef state = Pred->getState();
-    const LocationContext *LCtx = Pred->getLocationContext();
-
-    // Handle the case where the container still has elements.
-    SVal TrueV = svalBuilder.makeTruthVal(1);
-    ProgramStateRef hasElems = state->BindExpr(S, LCtx, TrueV);
-
-    // Handle the case where the container has no elements.
-    SVal FalseV = svalBuilder.makeTruthVal(0);
-    ProgramStateRef noElems = state->BindExpr(S, LCtx, FalseV);
+  if (!isContainerNull)
+    populateObjCForDestinationSet(dstLocation, svalBuilder, S, elem, elementV,
+                                  SymMgr, currBldrCtx, Bldr,
+                                  /*hasElements=*/true);
 
-    if (Optional<loc::MemRegionVal> MV = elementV.getAs<loc::MemRegionVal>())
-      if (const TypedValueRegion *R =
-          dyn_cast<TypedValueRegion>(MV->getRegion())) {
-        // FIXME: The proper thing to do is to really iterate over the
-        //  container.  We will do this with dispatch logic to the store.
-        //  For now, just 'conjure' up a symbolic value.
-        QualType T = R->getValueType();
-        assert(Loc::isLocType(T));
-        SymbolRef Sym = SymMgr.conjureSymbol(elem, LCtx, T,
-                                             currBldrCtx->blockCount());
-        SVal V = svalBuilder.makeLoc(Sym);
-        hasElems = hasElems->bindLoc(elementV, V, LCtx);
-
-        // Bind the location to 'nil' on the false branch.
-        SVal nilV = svalBuilder.makeIntVal(0, T);
-        noElems = noElems->bindLoc(elementV, nilV, LCtx);
-      }
-
-    // Create the new nodes.
-    Bldr.generateNode(S, Pred, hasElems);
-    Bldr.generateNode(S, Pred, noElems);
-  }
+  populateObjCForDestinationSet(dstLocation, svalBuilder, S, elem, elementV,
+                                SymMgr, currBldrCtx, Bldr,
+                                /*hasElements=*/false);
 
   // Finally, run any custom checkers.
   // FIXME: Eventually all pre- and post-checks should live in VisitStmt.