From 77942db0b8ae4799fa9cbd2f4b9872397b94bd43 Mon Sep 17 00:00:00 2001 From: Devin Coughlin Date: Mon, 28 Mar 2016 20:30:25 +0000 Subject: [PATCH] [analyzer] Nullability: Don't warn along paths where null returned from non-null. Change the nullability checker to not warn along paths where null is returned from a method with a non-null return type, even when the diagnostic for this return has been suppressed. This prevents warning from methods with non-null return types that inline methods that themselves return nil but that suppressed the diagnostic. Also change the PreconditionViolated state component to be called "InvariantViolated" because it is set when a post-condition is violated, as well. rdar://problem/25393539 llvm-svn: 264647 --- .../Checkers/NullabilityChecker.cpp | 134 +++++++++++------- .../system-header-simulator-for-nullability.h | 5 +- clang/test/Analysis/nullability-no-arc.mm | 55 +++++++ 3 files changed, 137 insertions(+), 57 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp index 31560f2687c6..35620d33acf1 100644 --- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp @@ -168,11 +168,11 @@ private: /// /// When \p SuppressPath is set to true, no more bugs will be reported on this /// path by this checker. - void reportBugIfPreconditionHolds(StringRef Msg, ErrorKind Error, - ExplodedNode *N, const MemRegion *Region, - CheckerContext &C, - const Stmt *ValueExpr = nullptr, - bool SuppressPath = false) const; + void reportBugIfInvariantHolds(StringRef Msg, ErrorKind Error, + ExplodedNode *N, const MemRegion *Region, + CheckerContext &C, + const Stmt *ValueExpr = nullptr, + bool SuppressPath = false) const; void reportBug(StringRef Msg, ErrorKind Error, ExplodedNode *N, const MemRegion *Region, BugReporter &BR, @@ -247,12 +247,31 @@ bool operator==(NullabilityState Lhs, NullabilityState Rhs) { REGISTER_MAP_WITH_PROGRAMSTATE(NullabilityMap, const MemRegion *, NullabilityState) -// If the nullability precondition of a function is violated, we should not -// report nullability related issues on that path. For this reason once a -// precondition is not met on a path, this checker will be esentially turned off -// for the rest of the analysis. We do not want to generate a sink node however, -// so this checker would not lead to reduced coverage. -REGISTER_TRAIT_WITH_PROGRAMSTATE(PreconditionViolated, bool) +// 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). +// +// As a matter of policy, the nullability checker typically warns on direct +// violations of the nullability invariant (although it uses various +// heuristics to suppress warnings in some cases) but will not warn if the +// invariant has already been violated along the path (either directly or +// indirectly). As a practical matter, this prevents the analyzer from +// (1) warning on defensive code paths where a nullability precondition is +// determined to have been violated, (2) warning additional times after an +// initial direct violation has been discovered, and (3) warning after a direct +// violation that has been implicitly or explicitly suppressed (for +// example, with a cast of NULL to _Nonnull). In essence, once an invariant +// violation is detected on a path, this checker will be esentially turned off +// for the rest of the analysis +// +// The analyzer takes this approach (rather than generating a sink node) to +// ensure coverage of defensive paths, which may be important for backwards +// compatibility in codebases that were developed without nullability in mind. +REGISTER_TRAIT_WITH_PROGRAMSTATE(InvariantViolated, bool) enum class NullConstraint { IsNull, IsNotNull, Unknown }; @@ -366,9 +385,9 @@ checkParamsForPreconditionViolation(const ParamVarDeclRange &Params, return false; } -static bool checkPreconditionViolation(ProgramStateRef State, ExplodedNode *N, - CheckerContext &C) { - if (State->get()) +static bool checkInvariantViolation(ProgramStateRef State, ExplodedNode *N, + CheckerContext &C) { + if (State->get()) return true; const LocationContext *LocCtxt = C.getLocationContext(); @@ -388,21 +407,21 @@ static bool checkPreconditionViolation(ProgramStateRef State, ExplodedNode *N, if (checkParamsForPreconditionViolation(Params, State, LocCtxt)) { if (!N->isSink()) - C.addTransition(State->set(true), N); + C.addTransition(State->set(true), N); return true; } return false; } -void NullabilityChecker::reportBugIfPreconditionHolds(StringRef Msg, +void NullabilityChecker::reportBugIfInvariantHolds(StringRef Msg, ErrorKind Error, ExplodedNode *N, const MemRegion *Region, CheckerContext &C, const Stmt *ValueExpr, bool SuppressPath) const { ProgramStateRef OriginalState = N->getState(); - if (checkPreconditionViolation(OriginalState, N, C)) + if (checkInvariantViolation(OriginalState, N, C)) return; if (SuppressPath) { - OriginalState = OriginalState->set(true); + OriginalState = OriginalState->set(true); N = C.addTransition(OriginalState, N); } @@ -430,7 +449,7 @@ void NullabilityChecker::checkDeadSymbols(SymbolReaper &SR, // 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 (checkPreconditionViolation(State, C.getPredecessor(), C)) + if (checkInvariantViolation(State, C.getPredecessor(), C)) return; C.addTransition(State); } @@ -439,7 +458,7 @@ void NullabilityChecker::checkDeadSymbols(SymbolReaper &SR, /// not know anything about the value of that pointer. When that pointer is /// nullable, this code emits a warning. void NullabilityChecker::checkEvent(ImplicitNullDerefEvent Event) const { - if (Event.SinkNode->getState()->get()) + if (Event.SinkNode->getState()->get()) return; const MemRegion *Region = @@ -486,10 +505,6 @@ static const Expr *lookThroughImplicitCasts(const Expr *E) { /// This method check when nullable pointer or null value is returned from a /// function that has nonnull return type. -/// -/// TODO: when nullability preconditons are violated, it is ok to violate the -/// nullability postconditons (i.e.: when one of the nonnull parameters are null -/// this check should not report any nullability related issue). void NullabilityChecker::checkPreStmt(const ReturnStmt *S, CheckerContext &C) const { auto RetExpr = S->getRetValue(); @@ -500,7 +515,7 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S, return; ProgramStateRef State = C.getState(); - if (State->get()) + if (State->get()) return; auto RetSVal = @@ -542,10 +557,11 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S, Nullability RetExprTypeLevelNullability = getNullabilityAnnotation(lookThroughImplicitCasts(RetExpr)->getType()); + bool NullReturnedFromNonNull = (RequiredNullability == Nullability::Nonnull && + Nullness == NullConstraint::IsNull); if (Filter.CheckNullReturnedFromNonnull && - Nullness == NullConstraint::IsNull && + NullReturnedFromNonNull && RetExprTypeLevelNullability != Nullability::Nonnull && - RequiredNullability == Nullability::Nonnull && !InSuppressedMethodFamily) { static CheckerProgramPointTag Tag(this, "NullReturnedFromNonnull"); ExplodedNode *N = C.generateErrorNode(State, &Tag); @@ -557,9 +573,17 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S, OS << "Null is returned from a " << C.getDeclDescription(D) << " that is expected to return a non-null value"; - reportBugIfPreconditionHolds(OS.str(), - ErrorKind::NilReturnedToNonnull, N, nullptr, C, - RetExpr); + reportBugIfInvariantHolds(OS.str(), + ErrorKind::NilReturnedToNonnull, N, nullptr, C, + RetExpr); + return; + } + + // If null was returned from a non-null function, mark the nullability + // invariant as violated even if the diagnostic was suppressed. + if (NullReturnedFromNonNull) { + State = State->set(true); + C.addTransition(State); return; } @@ -583,9 +607,9 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S, OS << "Nullable pointer is returned from a " << C.getDeclDescription(D) << " that is expected to return a non-null value"; - reportBugIfPreconditionHolds(OS.str(), - ErrorKind::NullableReturnedToNonnull, N, - Region, C); + reportBugIfInvariantHolds(OS.str(), + ErrorKind::NullableReturnedToNonnull, N, + Region, C); } return; } @@ -605,7 +629,7 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call, return; ProgramStateRef State = C.getState(); - if (State->get()) + if (State->get()) return; ProgramStateRef OrigState = State; @@ -646,9 +670,9 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call, llvm::raw_svector_ostream OS(SBuf); OS << "Null passed to a callee that requires a non-null " << ParamIdx << llvm::getOrdinalSuffix(ParamIdx) << " parameter"; - reportBugIfPreconditionHolds(OS.str(), ErrorKind::NilPassedToNonnull, N, - nullptr, C, - ArgExpr, /*SuppressPath=*/false); + reportBugIfInvariantHolds(OS.str(), ErrorKind::NilPassedToNonnull, N, + nullptr, C, + ArgExpr, /*SuppressPath=*/false); return; } @@ -672,17 +696,17 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call, llvm::raw_svector_ostream OS(SBuf); OS << "Nullable pointer is passed to a callee that requires a non-null " << ParamIdx << llvm::getOrdinalSuffix(ParamIdx) << " parameter"; - reportBugIfPreconditionHolds(OS.str(), - ErrorKind::NullablePassedToNonnull, N, - Region, C, ArgExpr, /*SuppressPath=*/true); + reportBugIfInvariantHolds(OS.str(), + ErrorKind::NullablePassedToNonnull, N, + Region, C, ArgExpr, /*SuppressPath=*/true); return; } if (Filter.CheckNullableDereferenced && Param->getType()->isReferenceType()) { ExplodedNode *N = C.addTransition(State); - reportBugIfPreconditionHolds("Nullable pointer is dereferenced", - ErrorKind::NullableDereferenced, N, Region, - C, ArgExpr, /*SuppressPath=*/true); + reportBugIfInvariantHolds("Nullable pointer is dereferenced", + ErrorKind::NullableDereferenced, N, Region, + C, ArgExpr, /*SuppressPath=*/true); return; } continue; @@ -713,7 +737,7 @@ void NullabilityChecker::checkPostCall(const CallEvent &Call, if (!ReturnType->isAnyPointerType()) return; ProgramStateRef State = C.getState(); - if (State->get()) + if (State->get()) return; const MemRegion *Region = getTrackRegion(Call.getReturnValue()); @@ -782,7 +806,7 @@ void NullabilityChecker::checkPostObjCMessage(const ObjCMethodCall &M, return; ProgramStateRef State = C.getState(); - if (State->get()) + if (State->get()) return; const MemRegion *ReturnRegion = getTrackRegion(M.getReturnValue()); @@ -897,7 +921,7 @@ void NullabilityChecker::checkPostStmt(const ExplicitCastExpr *CE, return; ProgramStateRef State = C.getState(); - if (State->get()) + if (State->get()) return; Nullability DestNullability = getNullabilityAnnotation(DestType); @@ -1022,7 +1046,7 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S, return; ProgramStateRef State = C.getState(); - if (State->get()) + if (State->get()) return; auto ValDefOrUnknown = V.getAs(); @@ -1050,10 +1074,10 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S, if (!ValueExpr) ValueExpr = S; - reportBugIfPreconditionHolds("Null is assigned to a pointer which is " - "expected to have non-null value", - ErrorKind::NilAssignedToNonnull, N, nullptr, C, - ValueExpr); + reportBugIfInvariantHolds("Null is assigned to a pointer which is " + "expected to have non-null value", + ErrorKind::NilAssignedToNonnull, N, nullptr, C, + ValueExpr); return; } // Intentionally missing case: '0' is bound to a reference. It is handled by @@ -1074,10 +1098,10 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S, LocNullability == Nullability::Nonnull) { static CheckerProgramPointTag Tag(this, "NullablePassedToNonnull"); ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag); - reportBugIfPreconditionHolds("Nullable pointer is assigned to a pointer " - "which is expected to have non-null value", - ErrorKind::NullableAssignedToNonnull, N, - ValueRegion, C); + reportBugIfInvariantHolds("Nullable pointer is assigned to a pointer " + "which is expected to have non-null value", + ErrorKind::NullableAssignedToNonnull, N, + ValueRegion, C); } return; } diff --git a/clang/test/Analysis/Inputs/system-header-simulator-for-nullability.h b/clang/test/Analysis/Inputs/system-header-simulator-for-nullability.h index 9bb2786bd7f1..8d49f323bc1d 100644 --- a/clang/test/Analysis/Inputs/system-header-simulator-for-nullability.h +++ b/clang/test/Analysis/Inputs/system-header-simulator-for-nullability.h @@ -11,8 +11,9 @@ NS_ASSUME_NONNULL_BEGIN typedef struct _NSZone NSZone; @protocol NSObject -+ (id)alloc; -- (id)init; ++ (instancetype)alloc; +- (instancetype)init; +- (instancetype)autorelease; @end @protocol NSCopying diff --git a/clang/test/Analysis/nullability-no-arc.mm b/clang/test/Analysis/nullability-no-arc.mm index c0e693e90b3c..37d29b7457af 100644 --- a/clang/test/Analysis/nullability-no-arc.mm +++ b/clang/test/Analysis/nullability-no-arc.mm @@ -5,6 +5,8 @@ @protocol NSObject + (id)alloc; - (id)init; +- (instancetype)autorelease; +- (void)release; @end __attribute__((objc_root_class)) @@ -43,3 +45,56 @@ void testObjCNonARCNoInitialization(TestObject * _Nonnull p) { void testObjCNonARCExplicitZeroInitialization() { TestObject * _Nonnull explicitlyZeroInitialized = nil; // expected-warning {{Null is assigned to a pointer which is expected to have non-null value}} } + +@interface ClassWithInitializers : NSObject +@end + +@implementation ClassWithInitializers +- (instancetype _Nonnull)initWithNonnullReturnAndSelfCheckingIdiom { + // This defensive check is a common-enough idiom that we don't want + // to issue a diagnostic for it. + if (self = [super init]) { + } + + return self; // no-warning +} + +- (instancetype _Nonnull)initWithNonnullReturnAndNilReturnViaLocal { + self = [super init]; + // This leaks, but we're not checking for that here. + + ClassWithInitializers *other = nil; + // False negative. Once we have more subtle suppression of defensive checks in + // initializers we should warn here. + return other; +} + +- (instancetype _Nonnull)initWithPreconditionViolation:(int)p { + self = [super init]; + if (p < 0) { + [self release]; + return (ClassWithInitializers * _Nonnull)nil; + } + return self; +} + ++ (instancetype _Nonnull)factoryCallingInitWithNonnullReturnAndSelfCheckingIdiom { + return [[[self alloc] initWithNonnullReturnAndSelfCheckingIdiom] autorelease]; // no-warning +} + ++ (instancetype _Nonnull)factoryCallingInitWithNonnullReturnAndNilReturnViaLocal { + return [[[self alloc] initWithNonnullReturnAndNilReturnViaLocal] autorelease]; // no-warning +} + ++ (instancetype _Nonnull)initWithPreconditionViolation:(int) p { + return [[[self alloc] initWithPreconditionViolation:p] autorelease]; // no-warning +} + +- (TestObject * _Nonnull) returnsNil { + return (TestObject * _Nonnull)nil; +} +- (TestObject * _Nonnull) inlineOfReturnsNilObjCInstanceDirectlyWithSuppressingCast { + TestObject *o = [self returnsNil]; + return o; +} +@end