[clang] 6ab01d4 - [analyzer] Nullability: Enable analysis of non-inlined nullable objc properties.
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 12 14:19:33 PST 2022
Author: Paul Pelzl
Date: 2022-12-12T14:19:26-08:00
New Revision: 6ab01d4a5cbd46f521de89b167571c0754e6c557
URL: https://github.com/llvm/llvm-project/commit/6ab01d4a5cbd46f521de89b167571c0754e6c557
DIFF: https://github.com/llvm/llvm-project/commit/6ab01d4a5cbd46f521de89b167571c0754e6c557.diff
LOG: [analyzer] Nullability: Enable analysis of non-inlined nullable objc properties.
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.
https://reviews.llvm.org/D131655
Added:
Modified:
clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
clang/test/Analysis/nullability.mm
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index a016eba29a0d9..da8529f4ea813 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -80,7 +80,7 @@ enum class ErrorKind : int {
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:
@@ -102,6 +102,8 @@ class NullabilityChecker
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 @@ class NullabilityChecker
// 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 @@ bool operator==(NullabilityState Lhs, NullabilityState Rhs) {
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
@@ -467,6 +500,19 @@ void NullabilityChecker::checkDeadSymbols(SymbolReaper &SR,
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
@@ -854,6 +900,32 @@ static Nullability getReceiverNullability(const ObjCMethodCall &M,
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.
@@ -950,12 +1022,53 @@ void NullabilityChecker::checkPostObjCMessage(const ObjCMethodCall &M,
// 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<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) {
diff --git a/clang/test/Analysis/nullability.mm b/clang/test/Analysis/nullability.mm
index c3f27e4d22a4e..f9b3fc60c5a02 100644
--- a/clang/test/Analysis/nullability.mm
+++ b/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)takesNonnull:(int *_Nonnull)p;
- (void)takesNullable:(int *_Nullable)p;
- (void)takesUnspecified:(int *)p;
@property(readonly, strong) NSString *stuff;
+ at property(readonly, nonnull) int *propReturnsNonnull;
+ at property(readonly, nullable) int *propReturnsNullable;
+ at property(readonly) int *propReturnsUnspecified;
@end
TestObject * getUnspecifiedTestObject();
@@ -182,6 +192,53 @@ void testObjCMessageResultNullability() {
}
}
+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
More information about the cfe-commits
mailing list