From: Devin Coughlin <dcoughlin@apple.com>
Date: Thu, 28 Jan 2016 22:23:34 +0000 (+0000)
Subject: [analyzer] Suppress nullability warnings in copy, mutableCopy, and init families.
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=2cd93241422ef7586c71235559dafeec05043a50;p=clang

[analyzer] Suppress nullability warnings in copy, mutableCopy, and init families.

There are multiple, common idioms of defensive nil-checks in copy,
mutableCopy, and init methods in ObjC. The analyzer doesn't currently have the
capability to distinguish these idioms from true positives, so suppress all
warnings about returns in those families. This is a pretty blunt suppression
that we should improve later.

rdar://problem/24395811

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

diff --git a/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index 079a71e017..ce6fde28b0 100644
--- a/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -510,23 +510,22 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
   if (!RetSVal)
     return;
 
-  bool IsReturnSelfInObjCInit = false;
+  bool InSuppressedMethodFamily = false;
 
   QualType RequiredRetType;
   AnalysisDeclContext *DeclCtxt =
       C.getLocationContext()->getAnalysisDeclContext();
   const Decl *D = DeclCtxt->getDecl();
   if (auto *MD = dyn_cast<ObjCMethodDecl>(D)) {
+    // HACK: This is a big hammer to avoid warning when there are defensive
+    // nil checks in -init and -copy methods. We should add more sophisticated
+    // logic here to suppress on common defensive idioms but still
+    // warn when there is a likely problem.
+    ObjCMethodFamily Family = MD->getMethodFamily();
+    if (OMF_init == Family || OMF_copy == Family || OMF_mutableCopy == Family)
+      InSuppressedMethodFamily = true;
+
     RequiredRetType = MD->getReturnType();
-    // 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 {
@@ -549,7 +548,7 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
       Nullness == NullConstraint::IsNull &&
       RetExprTypeLevelNullability != Nullability::Nonnull &&
       RequiredNullability == Nullability::Nonnull &&
-      !IsReturnSelfInObjCInit) {
+      !InSuppressedMethodFamily) {
     static CheckerProgramPointTag Tag(this, "NullReturnedFromNonnull");
     ExplodedNode *N = C.generateErrorNode(State, &Tag);
     if (!N)
diff --git a/test/Analysis/nullability.mm b/test/Analysis/nullability.mm
index b119e63ffe..cdc798d976 100644
--- a/test/Analysis/nullability.mm
+++ b/test/Analysis/nullability.mm
@@ -4,14 +4,27 @@
 #define nil 0
 #define BOOL int
 
+#define NS_ASSUME_NONNULL_BEGIN _Pragma("clang assume_nonnull begin")
+#define NS_ASSUME_NONNULL_END   _Pragma("clang assume_nonnull end")
+
+typedef struct _NSZone NSZone;
+
 @protocol NSObject
 + (id)alloc;
 - (id)init;
 @end
 
+NS_ASSUME_NONNULL_BEGIN
 @protocol NSCopying
+- (id)copyWithZone:(nullable NSZone *)zone;
+
 @end
 
+@protocol NSMutableCopying
+- (id)mutableCopyWithZone:(nullable NSZone *)zone;
+@end
+NS_ASSUME_NONNULL_END
+
 __attribute__((objc_root_class))
 @interface
 NSObject<NSObject>
@@ -332,8 +345,9 @@ Dummy *_Nonnull testDefensiveInlineChecks(Dummy * p) {
   // 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}}
+  // False negative. Once we have more subtle suppression of defensive checks in
+  // initializers we should warn here.
+  return other;
 }
 @end
 
@@ -350,4 +364,40 @@ Dummy *_Nonnull testDefensiveInlineChecks(Dummy * p) {
 
   return self; // no-warning
 }
+
+- (id _Nonnull)initWithNonnullReturnAndSelfCheckingIdiomV2; {
+  // Another common return-checking idiom
+  self = [super initWithNonnullReturnAndSelfCheckingIdiom];
+  if (!self) {
+    return nil; // no-warning
+  }
+
+  return self;
+}
+@end
+
+@interface ClassWithCopyWithZone : NSObject<NSCopying,NSMutableCopying> {
+  id i;
+}
+
+@end
+
+@implementation ClassWithCopyWithZone
+-(id)copyWithZone:(NSZone *)zone {
+  ClassWithCopyWithZone *newInstance = [[ClassWithCopyWithZone alloc] init];
+  if (!newInstance)
+    return nil;
+
+  newInstance->i = i;
+  return newInstance;
+}
+
+-(id)mutableCopyWithZone:(NSZone *)zone {
+  ClassWithCopyWithZone *newInstance = [[ClassWithCopyWithZone alloc] init];
+  if (newInstance) {
+    newInstance->i = i;
+  }
+
+  return newInstance;
+}
 @end