[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

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@264647 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index 31560f2687c678522e47abf3cd74acd44f07308e..35620d33acf1974eab4a93a5f59330b91b60431f 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/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<PreconditionViolated>())
+static bool checkInvariantViolation(ProgramStateRef State, ExplodedNode *N,
+                                    CheckerContext &C) {
+  if (State->get<InvariantViolated>())
     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<PreconditionViolated>(true), N);
+      C.addTransition(State->set<InvariantViolated>(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<PreconditionViolated>(true);
+    OriginalState = OriginalState->set<InvariantViolated>(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<PreconditionViolated>())
+  if (Event.SinkNode->getState()->get<InvariantViolated>())
     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<PreconditionViolated>())
+  if (State->get<InvariantViolated>())
     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<InvariantViolated>(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<PreconditionViolated>())
+  if (State->get<InvariantViolated>())
     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<PreconditionViolated>())
+  if (State->get<InvariantViolated>())
     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<PreconditionViolated>())
+  if (State->get<InvariantViolated>())
     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<PreconditionViolated>())
+  if (State->get<InvariantViolated>())
     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<PreconditionViolated>())
+  if (State->get<InvariantViolated>())
     return;
 
   auto ValDefOrUnknown = V.getAs<DefinedOrUnknownSVal>();
@@ -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/test/Analysis/Inputs/system-header-simulator-for-nullability.h b/test/Analysis/Inputs/system-header-simulator-for-nullability.h
index 9bb2786bd7f1605fd609993494a7b7f5674607bd..8d49f323bc1d6a4a68f1a19913fe17490f48c8da 100644 (file)
--- a/test/Analysis/Inputs/system-header-simulator-for-nullability.h
+++ b/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/test/Analysis/nullability-no-arc.mm b/test/Analysis/nullability-no-arc.mm
index c0e693e90b3c41c14f7a2b1022c702ebd5f1fcbd..37d29b7457afa38b3f88287f7d7f84567d555e64 100644 (file)
--- a/test/Analysis/nullability-no-arc.mm
+++ b/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