[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 079a71e017453009ee1a87959a682aa1738472a8..ce6fde28b01287e5909ef2e77775e49e9af061ca 100644 (file)
--- 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 b119e63ffe2b33feb35ed65ad3be848d3f30ed37..cdc798d976b0467ee05bdad5f9436a815f5f91ca 100644 (file)
--- 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