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

[Devin Coughlin via cfe-commits]

Author: dcoughlin
Date: Thu Jan 28 16:23:34 2016
New Revision: 259099

URL: http://llvm.org/viewvc/llvm-project?rev=259099&view=rev
Log:
[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

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=259099&r1=259098&r2=259099&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp Thu Jan 28 16:23:34 2016
@@ -510,23 +510,22 @@ void NullabilityChecker::checkPreStmt(co
   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(co
       Nullness == NullConstraint::IsNull &&
       RetExprTypeLevelNullability != Nullability::Nonnull &&
       RequiredNullability == Nullability::Nonnull &&
-      !IsReturnSelfInObjCInit) {
+      !InSuppressedMethodFamily) {
     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=259099&r1=259098&r2=259099&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/nullability.mm (original)
+++ cfe/trunk/test/Analysis/nullability.mm Thu Jan 28 16:23:34 2016
@@ -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
 
+ at protocol NSMutableCopying
+- (id)mutableCopyWithZone:(nullable NSZone *)zone;
+ at end
+NS_ASSUME_NONNULL_END
+
 __attribute__((objc_root_class))
 @interface
 NSObject<NSObject>
@@ -332,8 +345,9 @@ Dummy *_Nonnull testDefensiveInlineCheck
   // 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 testDefensiveInlineCheck
 
   return self; // no-warning
 }
+
+- (id _Nonnull)initWithNonnullReturnAndSelfCheckingIdiomV2; {
+  // Another common return-checking idiom
+  self = [super initWithNonnullReturnAndSelfCheckingIdiom];
+  if (!self) {
+    return nil; // no-warning
+  }
+
+  return self;
+}
+ at end
+
+ at interface ClassWithCopyWithZone : NSObject<NSCopying,NSMutableCopying> {
+  id i;
+}
+
+ at end
+
+ at 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