From 4a330201ffc2d45c316b89e35c90412fd8f26cbb Mon Sep 17 00:00:00 2001
From: Devin Coughlin <dcoughlin@apple.com>
Date: Fri, 22 Jan 2016 01:01:11 +0000
Subject: [PATCH] [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.

llvm-svn: 258461
---
 .../Checkers/NullabilityChecker.cpp           | 37 +++++++++++--
 clang/test/Analysis/nullability.mm            | 52 ++++++++++++++++++-
 2 files changed, 84 insertions(+), 5 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index b532deb55336..079a71e01745 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -470,6 +470,22 @@ static const Expr *lookThroughImplicitCasts(const Expr *E) {
   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(const ReturnStmt *S,
   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(const ReturnStmt *S,
   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)
diff --git a/clang/test/Analysis/nullability.mm b/clang/test/Analysis/nullability.mm
index aa909fbb8c42..b119e63ffe2b 100644
--- a/clang/test/Analysis/nullability.mm
+++ b/clang/test/Analysis/nullability.mm
@@ -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 testDefensiveInlineChecks(Dummy * p) {
   return p;
 }
 
-@interface SomeClass : NSObject
+
+@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 testDefensiveInlineChecks(Dummy * p) {
     return p; // no-warning
 }
 @end
+
+@interface ClassWithInitializers : NSObject
+@end
+
+@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}}
+}
+@end
+
+@interface SubClassWithInitializers : ClassWithInitializers
+@end
+
+@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
+}
+@end