[analyzer] Nullability: Enable analysis of non-inlined nullable objc properties.

[NoQ]

Summary

Patch by Paul Peltz! I'm going to be addressing comments. The overall idea sounds great and the code seems to have all the necessary parts in place but I haven't looked at it very carefully so please comment.

Author's original description:

The NullabilityChecker has a very early policy decision that non-inlined property accesses will be inferred as returning nonnull, despite nullability annotations to the contrary. This decision eliminates false positives related to very common code patterns that look like this:

if (foo.prop) {
    [bar doStuffWithNonnull:foo.prop];
}

While this probably represents a correct nil-check, the analyzer can't determine correctness without gaining visibility into the property implementation.

Unfortunately, inferring nullable properties as nonnull comes at the cost of significantly reduced code coverage. My goal here is to enable detection of many property-related nullability violations without a large increase in false positives.

The approach is to introduce a heuristic: after accessing the value of a property, if the analyzer at any time proves that the property value is nonnull (which would happen in particular due to a nil-check conditional), then subsequent property accesses on that code path will be *inferred* as nonnull. This captures the pattern described above, which I believe to be the dominant source of false positives in real code.

[NoQ]

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

85–86 ↗

(On Diff #451752)

Ooops that wasn't part of the patch I should remove that.

[NoQ]

Get rid of unrelated code (whoops).

[NoQ]

Remove a redundant C.addTransition(State). This caused us to run both the old mode and the new mode by producing a state split on an otherwise linear path.

[NoQ]

Guess I'll commit?

[NoQ]

(thanks @usama54321 for having a look and talking to me about this offline!)

[NoQ]

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

enum class ErrorKind : int {
   NullableReturnedToNonnull,
   NullableDereferenced,
   NullablePassedToNonnull
 };
 
 class NullabilityChecker
     : public Checker<check::Bind, check::PreCall, check::PreStmt<ReturnStmt>,
                      check::PostCall, check::PostStmt<ExplicitCastExpr>,
-                     check::PostObjCMessage, check::DeadSymbols,
+                     check::PostObjCMessage, check::DeadSymbols, eval::Assume,
                      check::Location, check::Event<ImplicitNullDerefEvent>> {
 
 public:
   // If true, the checker will not diagnose nullabilility issues for calls
   // to system headers. This option is motivated by the observation that large
   // projects may have many nullability warnings. These projects may
   // find warnings about nullability annotations that they have explicitly
   // added themselves higher priority to fix than warnings on calls to system
   // libraries.
   bool NoDiagnoseCallsToSystemHeaders = false;
 
   void checkBind(SVal L, SVal V, const Stmt *S, CheckerContext &C) const;
   void checkPostStmt(const ExplicitCastExpr *CE, CheckerContext &C) const;
   void checkPreStmt(const ReturnStmt *S, CheckerContext &C) const;
   void checkPostObjCMessage(const ObjCMethodCall &M, CheckerContext &C) const;
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const;
   void checkEvent(ImplicitNullDerefEvent Event) const;
   void checkLocation(SVal Location, bool IsLoad, const Stmt *S,
                      CheckerContext &C) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+                             bool Assumption) const;
 
   void printState(raw_ostream &Out, ProgramStateRef State, const char *NL,
                   const char *Sep) const override;
 
   enum CheckKind {
     CK_NullPassedToNonnull,
     CK_NullReturnedFromNonnull,
     CK_NullableDereferenced,
if (!BTs[Kind])
       BTs[Kind].reset(new BugType(CheckNames[Kind], "Nullability",
                                   categories::MemoryError));
     return BTs[Kind];
   }
 
   // When set to false no nullability information will be tracked in
   // NullabilityMap. It is possible to catch errors like passing a null pointer
   // to a callee that expects nonnull argument without the information that is
-  // stroed in the NullabilityMap. This is an optimization.
+  // stored in the NullabilityMap. This is an optimization.
   bool NeedTracking = false;
 
 private:
   class NullabilityBugVisitor : public BugReporterVisitor {
   public:
     NullabilityBugVisitor(const MemRegion *M) : Region(M) {}
 
     void Profile(llvm::FoldingSetNodeID &ID) const override {
private:
   const Stmt *Source;
 };
 
 bool operator==(NullabilityState Lhs, NullabilityState Rhs) {
   return Lhs.getValue() == Rhs.getValue() &&
          Lhs.getNullabilitySource() == Rhs.getNullabilitySource();
 }
 
+// For the purpose of tracking historical property accesses, the key for lookup
+// is an object pointer (could be an instance or a class) paired with the unique
+// identifier for the property being invoked on that object.
+using ObjectPropPair = std::pair<const MemRegion *, const IdentifierInfo *>;
+
+// Metadata associated with the return value from a recorded property access.
+struct ConstrainedPropertyVal {
+  // This will reference the conjured return SVal for some call
+  // of the form [object property]
+  DefinedOrUnknownSVal Value;
+
+  // If the SVal has been determined to be nonnull, that is recorded here
+  bool isConstrainedNonnull;
+
+  ConstrainedPropertyVal(DefinedOrUnknownSVal SV)
+      : Value(SV), isConstrainedNonnull(false) {}
+
+  void Profile(llvm::FoldingSetNodeID &ID) const {
+    Value.Profile(ID);
+    ID.AddInteger(isConstrainedNonnull ? 1 : 0);
+  }
+};
+
+bool operator==(const ConstrainedPropertyVal &Lhs,
+                const ConstrainedPropertyVal &Rhs) {
+  return Lhs.Value == Rhs.Value &&
+         Lhs.isConstrainedNonnull == Rhs.isConstrainedNonnull;
+}
+
 } // end anonymous namespace
 
 REGISTER_MAP_WITH_PROGRAMSTATE(NullabilityMap, const MemRegion *,
                                NullabilityState)
+REGISTER_MAP_WITH_PROGRAMSTATE(PropertyAccessesMap, ObjectPropPair,
+                               ConstrainedPropertyVal)
 
 // We say "the nullability type invariant is violated" when a location with a
 // non-null type contains NULL or a function with a non-null return type returns
 // NULL. Violations of the nullability type invariant can be detected either
 // directly (for example, when NULL is passed as an argument to a nonnull
 // parameter) or indirectly (for example, when, inside a function, the
 // programmer defensively checks whether a nonnull parameter contains NULL and
 // finds that it does).
for (NullabilityMapTy::iterator I = Nullabilities.begin(),
                                   E = Nullabilities.end();
        I != E; ++I) {
     const auto *Region = I->first->getAs<SymbolicRegion>();
     assert(Region && "Non-symbolic region is tracked.");
     if (SR.isDead(Region->getSymbol())) {
       State = State->remove<NullabilityMap>(I->first);
     }
   }
+
+  // When an object goes out of scope, we can free the history associated
+  // with any property accesses on that object
+  PropertyAccessesMapTy PropertyAccesses = State->get<PropertyAccessesMap>();
+  for (PropertyAccessesMapTy::iterator I = PropertyAccesses.begin(),
+                                       E = PropertyAccesses.end();
+       I != E; ++I) {
+    const MemRegion *ReceiverRegion = I->first.first;
+    if (!SR.isLiveRegion(ReceiverRegion)) {
+      State = State->remove<PropertyAccessesMap>(I->first);
+    }
+  }
+
   // When one of the nonnull arguments are constrained to be null, nullability
   // preconditions are violated. It is not enough to check this only when we
   // actually report an error, because at that time interesting symbols might be
   // reaped.
   if (checkInvariantViolation(State, C.getPredecessor(), C))
     return;
   C.addTransition(State);
 }
if (ValueRegionSVal) {
     const NullabilityState *TrackedSelfNullability =
         State->get<NullabilityMap>(SelfRegion);
     if (TrackedSelfNullability)
       return TrackedSelfNullability->getValue();
   }
   return Nullability::Unspecified;
 }
 
+// The return value of a property access is typically a temporary value which
+// will not be tracked in a persistent manner by the analyzer.  We use
+// evalAssume() in order to immediately record constraints on those temporaries
+// at the time they are imposed (e.g. by a nil-check conditional).
+ProgramStateRef NullabilityChecker::evalAssume(ProgramStateRef State, SVal Cond,
+                                               bool Assumption) const {
+  PropertyAccessesMapTy PropertyAccesses = State->get<PropertyAccessesMap>();
+  for (PropertyAccessesMapTy::iterator I = PropertyAccesses.begin(),
+                                       E = PropertyAccesses.end();
+       I != E; ++I) {
+    if (!I->second.isConstrainedNonnull) {
+      ConditionTruthVal IsNonNull = State->isNonNull(I->second.Value);
+      if (IsNonNull.isConstrainedTrue()) {
+        ConstrainedPropertyVal Replacement = I->second;
+        Replacement.isConstrainedNonnull = true;
+        State = State->set<PropertyAccessesMap>(I->first, Replacement);
+      } else if (IsNonNull.isConstrainedFalse()) {
+        // Space optimization: no point in tracking constrained-null cases
+        State = State->remove<PropertyAccessesMap>(I->first);
+      }
+    }
+  }
+
+  return State;
+}
+
 /// Calculate the nullability of the result of a message expr based on the
 /// nullability of the receiver, the nullability of the return value, and the
 /// constraints.
 void NullabilityChecker::checkPostObjCMessage(const ObjCMethodCall &M,
                                               CheckerContext &C) const {
   auto Decl = M.getDecl();
   if (!Decl)
     return;
if (ComputedNullab != RetValTracked &&
       C.addTransition(State);
     }
     return;
   }
 
   // No tracked information. Use static type information for return value.
   Nullability RetNullability = getNullabilityAnnotation(RetType);
 
-  // Properties might be computed. For this reason the static analyzer creates a
-  // new symbol each time an unknown property  is read. To avoid false pozitives
-  // do not treat unknown properties as nullable, even when they explicitly
-  // marked nullable.
-  if (M.getMessageKind() == OCM_PropertyAccess && !C.wasInlined)
+  // Properties might be computed, which means the property value could
+  // theoretically change between calls even in commonly-observed cases like
+  // this:
+  //
+  //     if (foo.prop) {    // ok, it's nonnull here...
+  //         [bar doStuffWithNonnullVal:foo.prop];     // ...but what about
+  //         here?
+  //     }
+  //
+  // If the property is nullable-annotated, a naive analysis would lead to many
+  // false positives despite the presence of probably-correct nil-checks.  To
+  // reduce the false positive rate, we maintain a history of the most recently
+  // observed property value.  For each property access, if the prior value has
+  // been constrained to be not nil then we will conservatively assume that the
+  // next access can be inferred as nonnull.
+  if (RetNullability != Nullability::Nonnull &&
+      M.getMessageKind() == OCM_PropertyAccess && !C.wasInlined) {
+    bool LookupResolved = false;
+    if (const MemRegion *ReceiverRegion = getTrackRegion(M.getReceiverSVal())) {
+      if (IdentifierInfo *Ident = M.getSelector().getIdentifierInfoForSlot(0)) {
+        LookupResolved = true;
+        ObjectPropPair Key = std::make_pair(ReceiverRegion, Ident);
+        const ConstrainedPropertyVal *PrevPropVal =
+            State->get<PropertyAccessesMap>(Key);
+        if (PrevPropVal && PrevPropVal->isConstrainedNonnull) {
+          RetNullability = Nullability::Nonnull;
+        } else {
+          // If a previous property access was constrained as nonnull, we hold
+          // on to that constraint (effectively inferring that all subsequent
+          // accesses on that code path can be inferred as nonnull).  If the
+          // previous property access was *not* constrained as nonnull, then
+          // let's throw it away in favor of keeping the SVal associated with
+          // this more recent access.
+          if (auto ReturnSVal =
+                  M.getReturnValue().getAs<DefinedOrUnknownSVal>()) {
+            State = State->set<PropertyAccessesMap>(
+                Key, ConstrainedPropertyVal(*ReturnSVal));
+          }
+        }
+      }
+    }
+
+    if (!LookupResolved) {
+      // Fallback: err on the side of suppressing the false positive.
       RetNullability = Nullability::Nonnull;
+    }
+  }
 
   Nullability ComputedNullab = getMostNullable(RetNullability, SelfNullability);
   if (ComputedNullab == Nullability::Nullable) {
     const Stmt *NullabilitySource = ComputedNullab == RetNullability
                                         ? Message
                                         : Message->getInstanceReceiver();
     State = State->set<NullabilityMap>(
         ReturnRegion, NullabilityState(ComputedNullab, NullabilitySource));

clang/test/Analysis/nullability.mm

 // RUN:   -analyzer-checker=nullability.NullablePassedToNonnull\
 // RUN:   -analyzer-checker=nullability.NullableReturnedFromNonnull\
 // RUN:   -analyzer-checker=nullability.NullableDereferenced\
 // RUN:   -DNOSYSTEMHEADERS=1 -fobjc-arc\
 // RUN:   -analyzer-config nullability:NoDiagnoseCallsToSystemHeaders=true
 
 #include "Inputs/system-header-simulator-for-nullability.h"
 
+extern void __assert_fail(__const char *__assertion, __const char *__file,
+                          unsigned int __line, __const char *__function)
+    __attribute__((__noreturn__));
+
+#define assert(expr) \
+  ((expr) ? (void)(0) : __assert_fail(#expr, __FILE__, __LINE__, __func__))
+
 @interface TestObject : NSObject
 - (int *_Nonnull)returnsNonnull;
 - (int *_Nullable)returnsNullable;
 - (int *)returnsUnspecified;
 - (void)takesNonnull:(int *_Nonnull)p;
 - (void)takesNullable:(int *_Nullable)p;
 - (void)takesUnspecified:(int *)p;
 @property(readonly, strong) NSString *stuff;
+@property(readonly, nonnull) int *propReturnsNonnull;
+@property(readonly, nullable) int *propReturnsNullable;
+@property(readonly) int *propReturnsUnspecified;
 @end
 
 TestObject * getUnspecifiedTestObject();
 TestObject *_Nonnull getNonnullTestObject();
 TestObject *_Nullable getNullableTestObject();
 
 int getRandom();
 
case 6:
     break;
   case 7: {
     int *shouldBeNonnull = [eraseNullab(getNonnullTestObject()) returnsNonnull];
     [o takesNonnull:shouldBeNonnull];
   } break;
   }
 }
 
+void testObjCPropertyReadNullability() {
+  TestObject *o = getNonnullTestObject();
+  switch (getRandom()) {
+  case 0:
+    [o takesNonnull:o.propReturnsNonnull]; // no-warning
+    break;
+  case 1:
+    [o takesNonnull:o.propReturnsUnspecified]; // no-warning
+    break;
+  case 2:
+    [o takesNonnull:o.propReturnsNullable]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+    break;
+  case 3:
+    // If a property is constrained nonnull, assume it remains nonnull
+    if (o.propReturnsNullable) {
+      [o takesNonnull:o.propReturnsNullable]; // no-warning
+      [o takesNonnull:o.propReturnsNullable]; // no-warning
+    }
+    break;
+  case 4:
+    if (!o.propReturnsNullable) {
+      [o takesNonnull:o.propReturnsNullable]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+    }
+    break;
+  case 5:
+    if (!o.propReturnsNullable) {
+      if (o.propReturnsNullable) {
+        // Nonnull constraint from the more recent call wins
+        [o takesNonnull:o.propReturnsNullable]; // no-warning
+      }
+    }
+    break;
+  case 6:
+    // Constraints on property return values are receiver-qualified
+    if (o.propReturnsNullable) {
+      [o takesNonnull:o.propReturnsNullable];                      // no-warning
+      [o takesNonnull:getNonnullTestObject().propReturnsNullable]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+    }
+    break;
+  case 7:
+    // Assertions must be effective at suppressing warnings
+    assert(o.propReturnsNullable);
+    [o takesNonnull:o.propReturnsNullable]; // no-warning
+    break;
+  }
+}
+
 Dummy * _Nonnull testDirectCastNullableToNonnull() {
   Dummy *p = returnsNullable();
   takesNonnull((Dummy * _Nonnull)p);  // no-warning
   return (Dummy * _Nonnull)p;         // no-warning
 }
 
 Dummy * _Nonnull testIndirectCastNullableToNonnull() {
   Dummy *p = (Dummy * _Nonnull)returnsNullable();