[analyzer] Add ObjCLoopChecker: objects from NSArray et al are non-nil.

While collections containing nil elements can still be iterated over in an
Objective-C for-in loop, the most common Cocoa collections -- NSArray,
NSDictionary, and NSSet -- cannot contain nil elements. This checker adds
that assumption to the analyzer state.

This was the cause of some minor false positives concerning CFRelease calls
on objects in an NSArray.

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

4 files changed, 144 insertions, 4 deletions

diff --git a/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp b/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
index d4632488e9..c75c86faf4 100644
--- a/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
+++ b/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
@@ -27,6 +27,7 @@
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprObjC.h"
+#include "clang/AST/StmtObjC.h"
 #include "clang/AST/ASTContext.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringMap.h"
@@ -650,6 +651,75 @@ void VariadicMethodTypeChecker::checkPreObjCMessage(ObjCMessage msg,
 }
 
 //===----------------------------------------------------------------------===//
+// Improves the modeling of loops over Cocoa collections.
+//===----------------------------------------------------------------------===//
+
+namespace {
+class ObjCLoopChecker
+  : public Checker<check::PostStmt<ObjCForCollectionStmt> > {
+  
+public:
+  void checkPostStmt(const ObjCForCollectionStmt *FCS, CheckerContext &C) const;
+};
+}
+
+static bool isKnownNonNilCollectionType(QualType T) {
+  const ObjCObjectPointerType *PT = T->getAs<ObjCObjectPointerType>();
+  if (!PT)
+    return false;
+  
+  const ObjCInterfaceDecl *ID = PT->getInterfaceDecl();
+  if (!ID)
+    return false;
+
+  switch (findKnownClass(ID)) {
+  case FC_NSArray:
+  case FC_NSDictionary:
+  case FC_NSEnumerator:
+  case FC_NSOrderedSet:
+  case FC_NSSet:
+    return true;
+  default:
+    return false;
+  }
+}
+
+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;
+  
+  // 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.
+  const Stmt *Element = FCS->getElement();
+  SVal ElementVar;
+  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());
+  } else {
+    ElementVar = State->getSVal(Element, C.getLocationContext());
+  }
+
+  if (!isa<Loc>(ElementVar))
+    return;
+
+  // Go ahead and assume the value is non-nil.
+  SVal Val = State->getSVal(cast<Loc>(ElementVar));
+  State = State->assume(cast<DefinedOrUnknownSVal>(Val), true);
+  C.addTransition(State);
+}
+
+
+//===----------------------------------------------------------------------===//
 // Check registration.
 //===----------------------------------------------------------------------===//
 
@@ -672,3 +742,7 @@ void ento::registerClassReleaseChecker(CheckerManager &mgr) {
 void ento::registerVariadicMethodTypeChecker(CheckerManager &mgr) {
   mgr.registerChecker<VariadicMethodTypeChecker>();
 }
+
+void ento::registerObjCLoopChecker(CheckerManager &mgr) {
+  mgr.registerChecker<ObjCLoopChecker>();
+}
diff --git a/lib/StaticAnalyzer/Checkers/Checkers.td b/lib/StaticAnalyzer/Checkers/Checkers.td
index fb7b803f9e..28b0679d35 100644
--- a/lib/StaticAnalyzer/Checkers/Checkers.td
+++ b/lib/StaticAnalyzer/Checkers/Checkers.td
@@ -393,6 +393,10 @@ def ObjCSelfInitChecker : Checker<"SelfInit">,
   HelpText<"Check that 'self' is properly initialized inside an initializer method">,
   DescFile<"ObjCSelfInitChecker.cpp">;
 
+def ObjCLoopChecker : Checker<"Loops">,
+  HelpText<"Improved modeling of loops using Cocoa collection types">,
+  DescFile<"BasicObjCFoundationChecks.cpp">;
+
 def NSErrorChecker : Checker<"NSError">,
   HelpText<"Check usage of NSError** parameters">,
   DescFile<"NSErrorChecker.cpp">;
diff --git a/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
index c8ad70ad03..45ba0449f4 100644
--- a/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
+++ b/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
@@ -74,7 +74,6 @@ void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S,
   const Stmt *elem = S->getElement();
   ProgramStateRef state = Pred->getState();
   SVal elementV;
-  StmtNodeBuilder Bldr(Pred, Dst, *currentBuilderContext);
   
   if (const DeclStmt *DS = dyn_cast<DeclStmt>(elem)) {
     const VarDecl *elemD = cast<VarDecl>(DS->getSingleDecl());
@@ -86,10 +85,11 @@ void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S,
   }
   
   ExplodedNodeSet dstLocation;
-  Bldr.takeNodes(Pred);
   evalLocation(dstLocation, S, elem, Pred, state, elementV, NULL, false);
-  Bldr.addNodes(dstLocation);
-  
+
+  ExplodedNodeSet Tmp;
+  StmtNodeBuilder Bldr(Pred, Tmp, *currentBuilderContext);
+
   for (ExplodedNodeSet::iterator NI = dstLocation.begin(),
        NE = dstLocation.end(); NI!=NE; ++NI) {
     Pred = *NI;
@@ -126,6 +126,10 @@ void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S,
     Bldr.generateNode(S, Pred, hasElems);
     Bldr.generateNode(S, Pred, noElems);
   }
+
+  // Finally, run any custom checkers.
+  // FIXME: Eventually all pre- and post-checks should live in VisitStmt.
+  getCheckerManager().runCheckersForPostStmt(Dst, Tmp, S, *this);
 }
 
 void ExprEngine::VisitObjCMessage(const ObjCMessage &msg,
diff --git a/test/Analysis/objc-for.m b/test/Analysis/objc-for.m
new file mode 100644
index 0000000000..52a55b07db
--- /dev/null
+++ b/test/Analysis/objc-for.m
@@ -0,0 +1,58 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=osx.cocoa.Loops,debug.ExprInspection -verify
+
+void clang_analyzer_eval(int);
+
+#define nil ((id)0)
+
+@protocol NSFastEnumeration
+- (int)countByEnumeratingWithState:(void *)state objects:(id *)objects count:(unsigned)count;
+@end
+
+@interface NSObject
++ (instancetype)testObject;
+@end
+
+@interface NSEnumerator <NSFastEnumeration>
+@end
+
+@interface NSArray : NSObject <NSFastEnumeration>
+- (NSEnumerator *)objectEnumerator;
+@end
+
+@interface NSDictionary : NSObject <NSFastEnumeration>
+@end
+
+@interface NSMutableDictionary : NSDictionary
+@end
+
+@interface NSSet : NSObject <NSFastEnumeration>
+@end
+
+@interface NSPointerArray : NSObject <NSFastEnumeration>
+@end
+
+void test() {
+  id x;
+  for (x in [NSArray testObject])
+    clang_analyzer_eval(x != nil); // expected-warning{{TRUE}}
+
+  for (x in [NSMutableDictionary testObject])
+    clang_analyzer_eval(x != nil); // expected-warning{{TRUE}}
+
+  for (x in [NSSet testObject])
+    clang_analyzer_eval(x != nil); // expected-warning{{TRUE}}
+
+  for (x in [[NSArray testObject] objectEnumerator])
+    clang_analyzer_eval(x != nil); // expected-warning{{TRUE}}
+
+  for (x in [NSPointerArray testObject])
+    clang_analyzer_eval(x != nil); // expected-warning{{UNKNOWN}}
+}
+
+void testWithVarInFor() {
+  for (id x in [NSArray testObject])
+    clang_analyzer_eval(x != nil); // expected-warning{{TRUE}}
+  for (id x in [NSPointerArray testObject])
+    clang_analyzer_eval(x != nil); // expected-warning{{UNKNOWN}}
+}
+