diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp index 079a71e01745..ce6fde28b012 100644 --- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp +++ b/clang/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(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(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/clang/test/Analysis/nullability.mm b/clang/test/Analysis/nullability.mm index b119e63ffe2b..cdc798d976b0 100644 --- a/clang/test/Analysis/nullability.mm +++ b/clang/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 @@ -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 { + 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