r258461 - [analyzer] Suppress nullability warning for defensive super initializer idiom.

[Devin Coughlin via cfe-commits]

Author: dcoughlin
Date: Thu Jan 21 19:01:11 2016
New Revision: 258461

URL: http://llvm.org/viewvc/llvm-project?rev=258461&view=rev
Log:
[analyzer] Suppress nullability warning for defensive super initializer idiom.

A common idiom in Objective-C initializers is for a defensive nil-check on the
result of a call to a super initializer:
  if (self = [super init]) {
     ...
  }
  return self;

To avoid warning on this idiom, the nullability checker now suppress diagnostics
for returns of nil on syntactic 'return self' even in initializers with non-null
return types.

Modified:
    cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
    cfe/trunk/test/Analysis/nullability.mm

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp?rev=258461&r1=258460&r2=258461&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp Thu Jan 21 19:01:11 2016
@@ -470,6 +470,22 @@ static const Expr *lookThroughImplicitCa
   return E;
 }
 
+/// Returns true when the return statement is a syntactic 'return self' in
+/// Objective-C.
+static bool isReturnSelf(const ReturnStmt *RS, CheckerContext &C) {
+  const ImplicitParamDecl *SelfDecl =
+    C.getCurrentAnalysisDeclContext()->getSelfDecl();
+  if (!SelfDecl)
+    return false;
+
+  const Expr *ReturnExpr = lookThroughImplicitCasts(RS->getRetValue());
+  auto *RefExpr = dyn_cast<DeclRefExpr>(ReturnExpr);
+  if (!RefExpr)
+    return false;
+
+  return RefExpr->getDecl() == SelfDecl;
+}
+
 /// This method check when nullable pointer or null value is returned from a
 /// function that has nonnull return type.
 ///
@@ -494,16 +510,28 @@ void NullabilityChecker::checkPreStmt(co
   if (!RetSVal)
     return;
 
+  bool IsReturnSelfInObjCInit = false;
+
   QualType RequiredRetType;
   AnalysisDeclContext *DeclCtxt =
       C.getLocationContext()->getAnalysisDeclContext();
   const Decl *D = DeclCtxt->getDecl();
-  if (auto *MD = dyn_cast<ObjCMethodDecl>(D))
+  if (auto *MD = dyn_cast<ObjCMethodDecl>(D)) {
     RequiredRetType = MD->getReturnType();
-  else if (auto *FD = dyn_cast<FunctionDecl>(D))
+    // Suppress diagnostics for returns of nil that are syntactic returns of
+    // self in ObjC initializers. This avoids warning under the common idiom of
+    // a defensive check of the result of a call to super:
+    //   if (self = [super init]) {
+    //     ...
+    //   }
+    //   return self; // no-warning
+    IsReturnSelfInObjCInit = (MD->getMethodFamily() == OMF_init) &&
+                              isReturnSelf(S, C);
+  } else if (auto *FD = dyn_cast<FunctionDecl>(D)) {
     RequiredRetType = FD->getReturnType();
-  else
+  } else {
     return;
+  }
 
   NullConstraint Nullness = getNullConstraint(*RetSVal, State);
 
@@ -520,7 +548,8 @@ void NullabilityChecker::checkPreStmt(co
   if (Filter.CheckNullReturnedFromNonnull &&
       Nullness == NullConstraint::IsNull &&
       RetExprTypeLevelNullability != Nullability::Nonnull &&
-      RequiredNullability == Nullability::Nonnull) {
+      RequiredNullability == Nullability::Nonnull &&
+      !IsReturnSelfInObjCInit) {
     static CheckerProgramPointTag Tag(this, "NullReturnedFromNonnull");
     ExplodedNode *N = C.generateErrorNode(State, &Tag);
     if (!N)

Modified: cfe/trunk/test/Analysis/nullability.mm
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/nullability.mm?rev=258461&r1=258460&r2=258461&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/nullability.mm (original)
+++ cfe/trunk/test/Analysis/nullability.mm Thu Jan 21 19:01:11 2016
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fobjc-arc -analyze -analyzer-checker=core,nullability -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,nullability -verify %s
 
 #define nil 0
 #define BOOL int
@@ -279,10 +280,21 @@ Dummy *_Nonnull testDefensiveInlineCheck
   return p;
 }
 
- at interface SomeClass : NSObject
+
+ at interface SomeClass : NSObject {
+  int instanceVar;
+}
 @end
 
 @implementation SomeClass (MethodReturn)
+- (id)initWithSomething:(int)i {
+  if (self = [super init]) {
+    instanceVar = i;
+  }
+
+  return self;
+}
+
 - (TestObject * _Nonnull)testReturnsNullableInNonnullIndirectly {
   TestObject *local = getNullableTestObject();
   return local; // expected-warning {{Nullable pointer is returned from a function that is expected to return a non-null value}}
@@ -301,3 +313,41 @@ Dummy *_Nonnull testDefensiveInlineCheck
     return p; // no-warning
 }
 @end
+
+ at interface ClassWithInitializers : NSObject
+ at end
+
+ at implementation ClassWithInitializers
+- (instancetype _Nonnull)initWithNonnullReturnAndSelfCheckingIdiom {
+  // This defensive check is a common-enough idiom that we filter 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;
+  // Still warn when when not returning via self.
+  return other; // expected-warning {{Null is returned from a function that is expected to return a non-null value}}
+}
+ at end
+
+ at interface SubClassWithInitializers : ClassWithInitializers
+ at end
+
+ at implementation SubClassWithInitializers
+// Note: Because this is overridding
+// -[ClassWithInitializers initWithNonnullReturnAndSelfCheckingIdiom],
+// the return type of this method becomes implicitly id _Nonnull.
+- (id)initWithNonnullReturnAndSelfCheckingIdiom {
+  if (self = [super initWithNonnullReturnAndSelfCheckingIdiom]) {
+  }
+
+  return self; // no-warning
+}
+ at end