Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp @@ -80,7 +80,7 @@ class NullabilityChecker : public Checker, check::PostCall, check::PostStmt, - check::PostObjCMessage, check::DeadSymbols, + check::PostObjCMessage, check::DeadSymbols, eval::Assume, check::Location, check::Event> { public: @@ -102,6 +102,8 @@ 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; @@ -129,7 +131,7 @@ // 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: @@ -230,10 +232,41 @@ 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; + +// 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 @@ -464,6 +497,19 @@ State = State->remove(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(); + 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(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 @@ -851,6 +897,32 @@ 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(); + 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(I->first, Replacement); + } else if (IsNonNull.isConstrainedFalse()) { + // Space optimization: no point in tracking constrained-null cases + State = State->remove(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. @@ -947,12 +1019,53 @@ // 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) - RetNullability = Nullability::Nonnull; + // 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(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()) { + State = State->set( + 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) { Index: clang/test/Analysis/nullability.mm =================================================================== --- clang/test/Analysis/nullability.mm +++ clang/test/Analysis/nullability.mm @@ -34,6 +34,13 @@ #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; @@ -42,6 +49,9 @@ - (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(); @@ -182,6 +192,53 @@ } } +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