[analyzer] Skip Pre/Post handlers for ObjC calls when receiver is nil.

[dcoughlin]

Summary

[analyzer] Skip Pre/Post handlers for ObjC calls when receiver is nil.

In Objective-C, method calls with nil receivers are essentially no-ops. They
do not fault (although the returned value may be garbage depending on the
declared return type and architecture). Programmers are aware of this
behavior and will complain about a false alarm when the analyzer
diagnoses API violations for method calls when the receiver is known to
be nil.

Rather than require each individual checker to be aware of this behavior
and suppress a warning when the receiver is nil, this patch
changes ExprEngineObjC so that VisitObjCMessage skips calling checker
pre/post handlers when the receiver is definitely nil. Instead, it adds a
new event, ObjCMessageNil, that is only called in that case.

The CallAndMessageChecker explicitly cares about this case, so I've changed it
to add a callback for ObjCMessageNil and moved the logic in PreObjCMessage
that handles nil receivers to the new callback.

rdar://problem/18092611

[krememek]

I think this is a great refinement overall, with a few minor nits. It also isn't clear what the test does.

include/clang/StaticAnalyzer/Core/CheckerManager.h

578

Can we break with tradition here and add a documentation comment?

lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp

64

Can we break with tradition here and actually add a small documentation comment?

test/Analysis/NSContainers.m

297

Can we add a comment above this test that makes it clear what this test is doing? It is not obvious at all from basic inspection.

There's also no matching 'no-warning' or 'expected-warning', so it is not clear at all what it is testing. Having the comment clearly say what the test is testing will make it more resilient to somebody accidentally deleting it.

[xazax.hun]

Looks good to me. There are some minor nits inline.

include/clang/StaticAnalyzer/Core/CheckerManager.h

96

I do not really like the name ObjCCheckerKind, because it is not kind of an Obj-C related checker. It is the kind of the visit of the message expression. Maybe ObjCMessageVisitKind? Or just MessageVisitKind to be a bit shorter?

lib/StaticAnalyzer/Core/ExprEngineObjC.cpp

153

The old code had a comment about merging two cases and a reference to a rdar. Is that rdar already fixed? Maybe it would be good to preserve the at least the first part of the commend?

[dcoughlin]

Update patch to address review comments. I've also updated the CheckerDocumentation checker to document the new ObjCMessageNil callback.

This version of the patch also restores the pre-patch behavior of assuming that the receiver is non-nil after a method call when the receiver is not definitely nil. I've added a test for this behavior (objc-message.m) and a comment based on a discussion with Jordan that explains why that behavior is important.

[dcoughlin]

include/clang/StaticAnalyzer/Core/CheckerManager.h

96

I've changed this to ObjCMessageVisitKind, as you suggested.

lib/StaticAnalyzer/Core/ExprEngineObjC.cpp

153

Thanks for pointing this out! This revealed a deeper issue where the previous patch did not drop the non-nil flow after a method call when it was unknown whether the receiver was nil. I've fixed the behavior, added a comment explaining why it is needed, and added a test for it.

[xazax.hun]

lib/StaticAnalyzer/Core/CheckerManager.cpp

237

nit: remove the break after the return.

lib/StaticAnalyzer/Core/ExprEngineObjC.cpp

197

Aren't we dropping the nil flow here instead of the non-nil? If that's the case, the comment should reflect that.

[dcoughlin]

lib/StaticAnalyzer/Core/ExprEngineObjC.cpp

197

Yes, you're right. I'll update the comment.

test/Analysis/objc-message.m

25

This comment is wrong too (we drop the *nil* flow). I'll update it.

[dcoughlin]

Update comments to correct nil/non-nil mistakes.

[xazax.hun]

Looks good to me.

[dcoughlin]

I've updated ExprEngine::VisitObjCMessage to create a pre-statement program point node for the ObjCMessageExpr when the receiver is definitely nil and changed the MessageNil checker handlers to run on a post-statement program point node. This makes sure we always have both a pre- and a post-statement program point for the message expression -- an invariant the path diagnostic machinery relies upon.

include/clang/StaticAnalyzer/Core/Checker.h

 public:
   template <typename CHECKER>
   static void _register(CHECKER *checker, CheckerManager &mgr) {
     mgr._registerForPreObjCMessage(
      CheckerManager::CheckObjCMessageFunc(checker, _checkObjCMessage<CHECKER>));
   }
 };
 
+class ObjCMessageNil {
+  template <typename CHECKER>
+  static void _checkObjCMessage(void *checker, const ObjCMethodCall &msg,
+                                CheckerContext &C) {
+    ((const CHECKER *)checker)->checkObjCMessageNil(msg, C);
+  }
+
+public:
+  template <typename CHECKER>
+  static void _register(CHECKER *checker, CheckerManager &mgr) {
+    mgr._registerForObjCMessageNil(
+     CheckerManager::CheckObjCMessageFunc(checker, _checkObjCMessage<CHECKER>));
+  }
+};
+
 class PostObjCMessage {
   template <typename CHECKER>
   static void _checkObjCMessage(void *checker, const ObjCMethodCall &msg,
                                 CheckerContext &C) {
     ((const CHECKER *)checker)->checkPostObjCMessage(msg, C);
   }
 
 public:

include/clang/StaticAnalyzer/Core/CheckerManager.h

class CheckName {
   friend class ::clang::ento::CheckerRegistry;
   explicit CheckName(StringRef Name) : Name(Name) {}
 
 public:
   CheckName() = default;
   StringRef getName() const { return Name; }
 };
 
+enum class ObjCMessageVisitKind {
+  Pre,
+  Post,
+  MessageNil
+};
+
 class CheckerManager {
   const LangOptions LangOpts;
   AnalyzerOptionsRef AOptions;
   CheckName CurrentCheckName;
 
 public:
   CheckerManager(const LangOptions &langOpts,
                  AnalyzerOptionsRef AOptions)
void runCheckersForStmt(bool isPreVisit,
                           const Stmt *S, ExprEngine &Eng,
                           bool wasInlined = false);
 
   /// \brief Run checkers for pre-visiting obj-c messages.
   void runCheckersForPreObjCMessage(ExplodedNodeSet &Dst,
                                     const ExplodedNodeSet &Src,
                                     const ObjCMethodCall &msg,
                                     ExprEngine &Eng) {
-    runCheckersForObjCMessage(/*isPreVisit=*/true, Dst, Src, msg, Eng);
+    runCheckersForObjCMessage(ObjCMessageVisitKind::Pre, Dst, Src, msg, Eng);
   }
 
   /// \brief Run checkers for post-visiting obj-c messages.
   void runCheckersForPostObjCMessage(ExplodedNodeSet &Dst,
                                      const ExplodedNodeSet &Src,
                                      const ObjCMethodCall &msg,
                                      ExprEngine &Eng,
                                      bool wasInlined = false) {
-    runCheckersForObjCMessage(/*isPreVisit=*/false, Dst, Src, msg, Eng,
+    runCheckersForObjCMessage(ObjCMessageVisitKind::Post, Dst, Src, msg, Eng,
                               wasInlined);
   }
 
+  /// \brief Run checkers for visiting an obj-c message to nil.
+  void runCheckersForObjCMessageNil(ExplodedNodeSet &Dst,
+                                    const ExplodedNodeSet &Src,
+                                    const ObjCMethodCall &msg,
+                                    ExprEngine &Eng) {
+    runCheckersForObjCMessage(ObjCMessageVisitKind::MessageNil, Dst, Src, msg,
+                              Eng);
+  }
+
+
   /// \brief Run checkers for visiting obj-c messages.
-  void runCheckersForObjCMessage(bool isPreVisit,
+  void runCheckersForObjCMessage(ObjCMessageVisitKind visitKind,
                                  ExplodedNodeSet &Dst,
                                  const ExplodedNodeSet &Src,
                                  const ObjCMethodCall &msg, ExprEngine &Eng,
                                  bool wasInlined = false);
 
   /// \brief Run checkers for pre-visiting obj-c messages.
   void runCheckersForPreCall(ExplodedNodeSet &Dst, const ExplodedNodeSet &Src,
                              const CallEvent &Call, ExprEngine &Eng) {
//===----------------------------------------------------------------------===//
   void _registerForPreStmt(CheckStmtFunc checkfn,
                            HandlesStmtFunc isForStmtFn);
   void _registerForPostStmt(CheckStmtFunc checkfn,
                             HandlesStmtFunc isForStmtFn);
 
   void _registerForPreObjCMessage(CheckObjCMessageFunc checkfn);
   void _registerForPostObjCMessage(CheckObjCMessageFunc checkfn);
 
+  void _registerForObjCMessageNil(CheckObjCMessageFunc checkfn);
+
   void _registerForPreCall(CheckCallFunc checkfn);
   void _registerForPostCall(CheckCallFunc checkfn);
 
   void _registerForLocation(CheckLocationFunc checkfn);
 
   void _registerForBind(CheckBindFunc checkfn);
 
   void _registerForEndAnalysis(CheckEndAnalysisFunc checkfn);
private:
 
   typedef SmallVector<CheckStmtFunc, 4> CachedStmtCheckers;
   typedef llvm::DenseMap<unsigned, CachedStmtCheckers> CachedStmtCheckersMapTy;
   CachedStmtCheckersMapTy CachedStmtCheckersMap;
 
   const CachedStmtCheckers &getCachedStmtCheckersFor(const Stmt *S,
                                                      bool isPreVisit);
 
+  /// Returns the checkers that have registered for callbacks of the
+  /// given \p Kind.
+  const std::vector<CheckObjCMessageFunc> &
+  getObjCMessageCheckers(ObjCMessageVisitKind Kind);
+
   std::vector<CheckObjCMessageFunc> PreObjCMessageCheckers;
   std::vector<CheckObjCMessageFunc> PostObjCMessageCheckers;
+  std::vector<CheckObjCMessageFunc> ObjCMessageNilCheckers;
 
   std::vector<CheckCallFunc> PreCallCheckers;
   std::vector<CheckCallFunc> PostCallCheckers;
 
   std::vector<CheckLocationFunc> LocationCheckers;
 
   std::vector<CheckBindFunc> BindCheckers;
 

lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp

struct ChecksFilter {
   CheckName CheckName_CallAndMessageUnInitRefArg;
   CheckName CheckName_CallAndMessageChecker;
 };
 
 class CallAndMessageChecker
   : public Checker< check::PreStmt<CallExpr>,
                     check::PreStmt<CXXDeleteExpr>,
                     check::PreObjCMessage,
+                    check::ObjCMessageNil,
                     check::PreCall > {
   mutable std::unique_ptr<BugType> BT_call_null;
   mutable std::unique_ptr<BugType> BT_call_undef;
   mutable std::unique_ptr<BugType> BT_cxx_call_null;
   mutable std::unique_ptr<BugType> BT_cxx_call_undef;
   mutable std::unique_ptr<BugType> BT_call_arg;
   mutable std::unique_ptr<BugType> BT_cxx_delete_undef;
   mutable std::unique_ptr<BugType> BT_msg_undef;
   mutable std::unique_ptr<BugType> BT_objc_prop_undef;
   mutable std::unique_ptr<BugType> BT_objc_subscript_undef;
   mutable std::unique_ptr<BugType> BT_msg_arg;
   mutable std::unique_ptr<BugType> BT_msg_ret;
   mutable std::unique_ptr<BugType> BT_call_few_args;
 
 public:
   ChecksFilter Filter;
 
   void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;
   void checkPreStmt(const CXXDeleteExpr *DE, CheckerContext &C) const;
   void checkPreObjCMessage(const ObjCMethodCall &msg, CheckerContext &C) const;
+
+  /// Fill in the return value that results from messaging nil based on the
+  /// return type and architecture and diagnose if the return value will be
+  /// garbage.
+  void checkObjCMessageNil(const ObjCMethodCall &msg, CheckerContext &C) const;
+
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
 
 private:
   bool PreVisitProcessArg(CheckerContext &C, SVal V, SourceRange ArgRange,
                           const Expr *ArgEx, bool IsFirstArgument,
                           bool CheckUninitFields, const CallEvent &Call,
                           std::unique_ptr<BugType> &BT,
                           const ParmVarDecl *ParamDecl) const;
if (ExplodedNode *N = C.generateSink()) {
       R->addRange(ME->getReceiverRange());
 
       // FIXME: getTrackNullOrUndefValueVisitor can't handle "super" yet.
       if (const Expr *ReceiverE = ME->getInstanceReceiver())
         bugreporter::trackNullOrUndefValue(N, ReceiverE, *R);
       C.emitReport(std::move(R));
     }
     return;
-  } else {
-    // Bifurcate the state into nil and non-nil ones.
-    DefinedOrUnknownSVal receiverVal = recVal.castAs<DefinedOrUnknownSVal>();
-
-    ProgramStateRef state = C.getState();
-    ProgramStateRef notNilState, nilState;
-    std::tie(notNilState, nilState) = state->assume(receiverVal);
-
-    // Handle receiver must be nil.
-    if (nilState && !notNilState) {
-      HandleNilReceiver(C, state, msg);
-      return;
-    }
-  }
+
+void CallAndMessageChecker::checkObjCMessageNil(const ObjCMethodCall &msg,
+                                                CheckerContext &C) const {
+  HandleNilReceiver(C, C.getState(), msg);
 }
 
 void CallAndMessageChecker::emitNilReceiverBug(CheckerContext &C,
                                                const ObjCMethodCall &msg,
                                                ExplodedNode *N) const {
 
   if (!BT_msg_ret)
     BT_msg_ret.reset(

lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp

 /// as reporting bugs. Most of the callbacks are targeted at path-sensitive
 /// checking.
 ///
 /// \sa CheckerContext
 class CheckerDocumentation : public Checker< check::PreStmt<ReturnStmt>,
                                        check::PostStmt<DeclStmt>,
                                        check::PreObjCMessage,
                                        check::PostObjCMessage,
+                                       check::ObjCMessageNil,
                                        check::PreCall,
                                        check::PostCall,
                                        check::BranchCondition,
                                        check::Location,
                                        check::Bind,
                                        check::DeadSymbols,
                                        check::EndFunction,
                                        check::EndAnalysis,
public:
   void checkPreObjCMessage(const ObjCMethodCall &M, CheckerContext &C) const {}
 
   /// \brief Post-visit the Objective C message.
   /// \sa checkPreObjCMessage()
   ///
   /// check::PostObjCMessage
   void checkPostObjCMessage(const ObjCMethodCall &M, CheckerContext &C) const {}
 
+  /// \brief Visit an Objective-C message whose receiver is nil.
+  ///
+  /// This will be called when the analyzer core processes a method call whose
+  /// receiver is definitely nil. In this case, check{Pre/Post}ObjCMessage and
+  /// check{Pre/Post}Call will not be called.
+  ///
+  /// check::ObjCMessageNil
+  void checkObjCMessageNil(const ObjCMethodCall &M, CheckerContext &C) const {}
+
   /// \brief Pre-visit an abstract "call" event.
   ///
   /// This is used for checkers that want to check arguments or attributed
   /// behavior for functions and methods no matter how they are being invoked.
   ///
   /// Note that this includes ALL cross-body invocations, so if you want to
   /// limit your checks to, say, function calls, you should test for that at the
   /// beginning of your callback function.

lib/StaticAnalyzer/Core/CheckerManager.cpp

void CheckerManager::runCheckersForStmt(bool isPreVisit,
   CheckStmtContext C(isPreVisit, getCachedStmtCheckersFor(S, isPreVisit),
                      S, Eng, WasInlined);
   expandGraphWithCheckers(C, Dst, Src);
 }
 
 namespace {
   struct CheckObjCMessageContext {
     typedef std::vector<CheckerManager::CheckObjCMessageFunc> CheckersTy;
-    bool IsPreVisit, WasInlined;
+
+    ObjCMessageVisitKind Kind;
+    bool WasInlined;
     const CheckersTy &Checkers;
     const ObjCMethodCall &Msg;
     ExprEngine &Eng;
 
     CheckersTy::const_iterator checkers_begin() { return Checkers.begin(); }
     CheckersTy::const_iterator checkers_end() { return Checkers.end(); }
 
-    CheckObjCMessageContext(bool isPreVisit, const CheckersTy &checkers,
+    CheckObjCMessageContext(ObjCMessageVisitKind visitKind,
+                            const CheckersTy &checkers,
                             const ObjCMethodCall &msg, ExprEngine &eng,
                             bool wasInlined)
-      : IsPreVisit(isPreVisit), WasInlined(wasInlined), Checkers(checkers),
+      : Kind(visitKind), WasInlined(wasInlined), Checkers(checkers),
         Msg(msg), Eng(eng) { }
 
     void runChecker(CheckerManager::CheckObjCMessageFunc checkFn,
                     NodeBuilder &Bldr, ExplodedNode *Pred) {
+
+      bool IsPreVisit;
+
+      switch (Kind) {
+        case ObjCMessageVisitKind::Pre:
+        case ObjCMessageVisitKind::MessageNil:
+          IsPreVisit = true;
+          break;
+        case ObjCMessageVisitKind::Post:
+          IsPreVisit = false;
+          break;
+      }
+
       const ProgramPoint &L = Msg.getProgramPoint(IsPreVisit,checkFn.Checker);
       CheckerContext C(Bldr, Eng, Pred, L, WasInlined);
 
       checkFn(*Msg.cloneWithState<ObjCMethodCall>(Pred->getState()), C);
     }
   };
 }
 
 /// \brief Run checkers for visiting obj-c messages.
-void CheckerManager::runCheckersForObjCMessage(bool isPreVisit,
+void CheckerManager::runCheckersForObjCMessage(ObjCMessageVisitKind visitKind,
                                                ExplodedNodeSet &Dst,
                                                const ExplodedNodeSet &Src,
                                                const ObjCMethodCall &msg,
                                                ExprEngine &Eng,
                                                bool WasInlined) {
-  CheckObjCMessageContext C(isPreVisit,
-                            isPreVisit ? PreObjCMessageCheckers
-                                       : PostObjCMessageCheckers,
-                            msg, Eng, WasInlined);
+  auto &checkers = getObjCMessageCheckers(visitKind);
+  CheckObjCMessageContext C(visitKind, checkers, msg, Eng, WasInlined);
   expandGraphWithCheckers(C, Dst, Src);
 }
 
+const std::vector<CheckerManager::CheckObjCMessageFunc> &
+CheckerManager::getObjCMessageCheckers(ObjCMessageVisitKind Kind) {
+  switch (Kind) {
+  case ObjCMessageVisitKind::Pre:
+    return PreObjCMessageCheckers;
+    break;
+  case ObjCMessageVisitKind::Post:
+    return PostObjCMessageCheckers;
+  case ObjCMessageVisitKind::MessageNil:
+    return ObjCMessageNilCheckers;
+  }
+}
 namespace {
   // FIXME: This has all the same signatures as CheckObjCMessageContext.
   // Is there a way we can merge the two?
   struct CheckCallContext {
     typedef std::vector<CheckerManager::CheckCallFunc> CheckersTy;
     bool IsPreVisit, WasInlined;
     const CheckersTy &Checkers;
     const CallEvent &Call;
void CheckerManager::_registerForPostStmt(CheckStmtFunc checkfn,
                                           HandlesStmtFunc isForStmtFn) {
   StmtCheckerInfo info = { checkfn, isForStmtFn, /*IsPreVisit*/false };
   StmtCheckers.push_back(info);
 }
 
 void CheckerManager::_registerForPreObjCMessage(CheckObjCMessageFunc checkfn) {
   PreObjCMessageCheckers.push_back(checkfn);
 }
+
+void CheckerManager::_registerForObjCMessageNil(CheckObjCMessageFunc checkfn) {
+  ObjCMessageNilCheckers.push_back(checkfn);
+}
+
 void CheckerManager::_registerForPostObjCMessage(CheckObjCMessageFunc checkfn) {
   PostObjCMessageCheckers.push_back(checkfn);
 }
 
 void CheckerManager::_registerForPreCall(CheckCallFunc checkfn) {
   PreCallCheckers.push_back(checkfn);
 }
 void CheckerManager::_registerForPostCall(CheckCallFunc checkfn) {

lib/StaticAnalyzer/Core/ExprEngineObjC.cpp

 
 void ExprEngine::VisitObjCMessage(const ObjCMessageExpr *ME,
                                   ExplodedNode *Pred,
                                   ExplodedNodeSet &Dst) {
   CallEventManager &CEMgr = getStateManager().getCallEventManager();
   CallEventRef<ObjCMethodCall> Msg =
     CEMgr.getObjCMethodCall(ME, Pred->getState(), Pred->getLocationContext());
 
+  // There are three cases for the receiver:
+  //   (1) it is definitely nil,
+  //   (2) it is definitely non-nil, and
+  //   (3) we don't know.
+  //
+  // If the receiver is definitely nil, we skip the pre/post callbacks and
+  // instead call the ObjCMessageNil callbacks and return.
+  //
+  // If the receiver is definitely non-nil, we call the pre- callbacks,
+  // evaluate the call, and call the post- callbacks.
+  //
+  // If we don't know, we drop the potential nil flow and instead
+  // continue from the assumed non-nil state as in (2). This approach
+  // intentionally drops coverage in order to prevent false alarms
+  // in the following scenario:
+  //
+  // id result = [o someMethod]
+  // if (result) {
+  //   if (!o) {
+  //     // <-- This program point should be unreachable because if o is nil
+  //     // it must the case that result is nil as well.
+  //   }
+  // }
+  //
+  // We could avoid dropping coverage by performing an explicit case split
+  // on each method call -- but this would get very expensive. An alternative
+  // would be to introduce lazy constraints.
+  // FIXME: This ignores many potential bugs (<rdar://problem/11733396>).
+  // Revisit once we have lazier constraints.
+  if (Msg->isInstanceMessage()) {
+    SVal recVal = Msg->getReceiverSVal();
+    if (!recVal.isUndef()) {
+      // Bifurcate the state into nil and non-nil ones.
+      DefinedOrUnknownSVal receiverVal =
+          recVal.castAs<DefinedOrUnknownSVal>();
+      ProgramStateRef State = Pred->getState();
+
+      ProgramStateRef notNilState, nilState;
+      std::tie(notNilState, nilState) = State->assume(receiverVal);
+
+      // Receiver is definitely nil, so run ObjCMessageNil callbacks and return.
+      if (nilState && !notNilState) {
+        ExplodedNodeSet dstNil;
+        StmtNodeBuilder Bldr(Pred, dstNil, *currBldrCtx);
+
+        Pred = Bldr.generateNode(ME, Pred, nilState);
+        assert(Pred && "Should have cached out already!");
+        getCheckerManager().runCheckersForObjCMessageNil(Dst, dstNil,
+                                                         *Msg, *this);
+        return;
+      }
+
+      ExplodedNodeSet dstNonNil;
+      StmtNodeBuilder Bldr(Pred, dstNonNil, *currBldrCtx);
+      // Generate a transition to non-Nil state, dropping any potential
+      // non-nil flow.
+      if (notNilState != State) {
+        Pred = Bldr.generateNode(ME, Pred, notNilState);
+        assert(Pred && "Should have cached out already!");
+      }
+    }
+  }
+
   // Handle the previsits checks.
   ExplodedNodeSet dstPrevisit;
   getCheckerManager().runCheckersForPreObjCMessage(dstPrevisit, Pred,
                                                    *Msg, *this);
   ExplodedNodeSet dstGenericPrevisit;
   getCheckerManager().runCheckersForPreCall(dstGenericPrevisit, dstPrevisit,
                                             *Msg, *this);
 
   // Proceed with evaluate the message expression.
   ExplodedNodeSet dstEval;
   StmtNodeBuilder Bldr(dstGenericPrevisit, dstEval, *currBldrCtx);
 
   for (ExplodedNodeSet::iterator DI = dstGenericPrevisit.begin(),
        DE = dstGenericPrevisit.end(); DI != DE; ++DI) {
     ExplodedNode *Pred = *DI;
     ProgramStateRef State = Pred->getState();
     CallEventRef<ObjCMethodCall> UpdatedMsg = Msg.cloneWithState(State);
     
     if (UpdatedMsg->isInstanceMessage()) {
       SVal recVal = UpdatedMsg->getReceiverSVal();
       if (!recVal.isUndef()) {
-        // Bifurcate the state into nil and non-nil ones.
-        DefinedOrUnknownSVal receiverVal =
-            recVal.castAs<DefinedOrUnknownSVal>();
-
-        ProgramStateRef notNilState, nilState;
-        std::tie(notNilState, nilState) = State->assume(receiverVal);
-        
-        // There are three cases: can be nil or non-nil, must be nil, must be
-        // non-nil. We ignore must be nil, and merge the rest two into non-nil.
-        // FIXME: This ignores many potential bugs (<rdar://problem/11733396>).
-        // Revisit once we have lazier constraints.
-        if (nilState && !notNilState) {
-          continue;
-        }
-        
-        // Check if the "raise" message was sent.
-        assert(notNilState);
         if (ObjCNoRet.isImplicitNoReturn(ME)) {
           // If we raise an exception, for now treat it as a sink.
           // Eventually we will want to handle exceptions properly.
           Bldr.generateSink(ME, Pred, State);
           continue;
         }
-        
-        // Generate a transition to non-Nil state.
-        if (notNilState != State) {
-          Pred = Bldr.generateNode(ME, Pred, notNilState);
-          assert(Pred && "Should have cached out already!");
-        }
       }
     } else {
       // Check for special class methods that are known to not return
       // and that we should treat as a sink.
       if (ObjCNoRet.isImplicitNoReturn(ME)) {
         // If we raise an exception, for now treat it as a sink.
         // Eventually we will want to handle exceptions properly.
         Bldr.generateSink(ME, Pred, Pred->getState());

test/Analysis/NSContainers.m

 @end
 @protocol NSSecureCoding <NSCoding>
 @required
 + (BOOL)supportsSecureCoding;
 @end
 @interface NSObject <NSObject> {}
 - (id)init;
 + (id)alloc;
+
+- (id)mutableCopy;
 @end
 
 typedef struct {
   unsigned long state;
   id *itemsPtr;
   unsigned long *mutationsPtr;
   unsigned long extra[5];
 } NSFastEnumerationState;

 @interface NSMutableArray (MySafeAdd)
 - (void)addObject:(id)obj safe:(BOOL)safe;
 @end
 
 void testArrayCategory(NSMutableArray *arr) {
   [arr addObject:0 safe:1]; // no-warning
 }
 
+@interface MyView : NSObject
+-(NSArray *)subviews;
+@end
+
+void testNoReportWhenReceiverNil(NSMutableArray *array, int b) {
+  // Don't warn about adding nil to a container when the receiver is also
+  // definitely nil
+  if (array == 0) {
+    [array addObject:0]; // no-warning
+  }
+
+  MyView *view = b ? [[MyView alloc] init] : 0;
+  NSMutableArray *subviews = [[view subviews] mutableCopy];
+  // When view is nil, subviews is also nil so there should be no warning
+  // here either.
+  [subviews addObject:view]; // no-warning
+}

test/Analysis/objc-message.m

+// RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-store=region -verify -Wno-objc-root-class %s
+
+extern void clang_analyzer_warnIfReached();
+void clang_analyzer_eval(int);
+
+@interface SomeClass
+-(id)someMethodWithReturn;
+-(void)someMethod;
+@end
+
+void consistencyOfReturnWithNilReceiver(SomeClass *o) {
+  id result = [o someMethodWithReturn];
+  if (result) {
+    if (!o) {
+      // It is impossible for both o to be nil and result to be non-nil,
+      // so this should not be reached.
+      clang_analyzer_warnIfReached(); // no-warning
+    }
+  }
+}
+
+void maybeNilReceiverIsNotNilAfterMessage(SomeClass *o) {
+  [o someMethod];
+
+  // We intentionally drop the non-nil flow (dropping coverage) after a method
+  // call when the receiver may be nil in order to avoid inconsistencies of
+  // the kind tested for in consistencyOfReturnWithNilReceiver().
+  clang_analyzer_eval(o != 0); // expected-warning{{TRUE}}
+}
+
+void NilReceiverIsStillNilAfterMessage(SomeClass *o) {
+  if (o == 0) {
+    id result = [o someMethodWithReturn];
+
+    // Both the receiver and the result should be nil after a message
+    // sent to a nil receiver returning a value of type id.
+    clang_analyzer_eval(o == 0); // expected-warning{{TRUE}}
+    clang_analyzer_eval(result == 0); // expected-warning{{TRUE}}
+  }
+}