When ARC is enabled, no warning will be generated when a method 1. Returns 'nil' in a method that is attributed to return a 'nonnull' 2. The return-statement is a ConditionalOperator, where the lhs is nil and rhs an objC-method-call (or the other...

[parallaxe]

Summary

...way round)

You may have a look at the added test-cases in nullability.mm, when my description is too confusing.

Bug-tracing:
When ARC is enabled, clang builds an ImplictCastExpr around the ConditionalOperator. The type that is assigned to the ImplicitCastExpr is derived from the scoping method / function. As the method / function is marked with _Nonnull, clang compares the nullability-attributes of the the ReturnStmt with the nullability-attributes of the method / function, and as they are the same, it generates no warning.

Conclusion:
I tried to built a non-intrusive patch. When the type that is assigned to the ImplicitCastExpr comes from the scoping method / function, i remove the nullability-attributes of the type.

Side effects:
Some tests in test/SemaObjC/nullability.m generated different warnings than before. Though, i think that the new ones are better than the old ones, so i would call this a nice little side-improvement.

[aaron.ballman]

Adding a few more reviewers, plus some nit comments.

lib/Sema/SemaStmt.cpp

3346

Nit: comments should start with a capital letter.

3349

Nit: can use const auto * because the type is spelled out in the initializer.

test/Analysis/nullability.mm

3 ↗

(On Diff #67070)

Is DNOSYSTEMHEADERS=0 required for your new test?

[ahatanak]

+cfe-commits

If this patch is applied, does clang issue a warning if a method marked "nonnull" returns a null value? I see a warning is issued for conditional expressions in the test case you've added, but I don't see a test case for a function returning just nil or 0.

I was wondering whether the change made in this patch contradicts what's stated in r240146's commit log:

"Note that we don't warn about nil returns from Objective-C methods,

because it's common for Objective-C methods to mimic the nil-swallowing
behavior of the receiver by checking ostensibly non-null parameters
and returning nil from otherwise non-null methods in that
case."

[dcoughlin]

test/Analysis/nullability.mm

114 ↗

(On Diff #67070)

I think a better file for these two analyzer tests is Analysis/nullability_nullonly.mm because they check for flows of nil to _Nonnull and not _Nullable to _Nonnull. Also, you shouldn't need to add an extra RUN line there.

Long-term these analyzer nullability test files need to be merged, but for this change I think sticking the two added tests in nullability_nullonly.mm is the best option.

[parallaxe]

In D23236#509013, @ahatanak wrote:

+cfe-commits

If this patch is applied, does clang issue a warning if a method marked "nonnull" returns a null value? I see a warning is issued for conditional expressions in the test case you've added, but I don't see a test case for a function returning just nil or 0.

I was wondering whether the change made in this patch contradicts what's stated in r240146's commit log:

"Note that we don't warn about nil returns from Objective-C methods,

because it's common for Objective-C methods to mimic the nil-swallowing
behavior of the receiver by checking ostensibly non-null parameters
and returning nil from otherwise non-null methods in that
case."

I'm not sure how this is meant. As far as I understand the commit-message you quoted, it is related to methods that return nil when a precondition fails. A test for this would be doNotWarnWhenPreconditionIsViolatedInTopFunc in nullability_nullonly.mm, which is not affected by my patch.
On the other side, according to the test testReturnsNilInNonnull in nullability_nullonly.mm

- (SomeClass * _Nonnull)testReturnsNilInNonnull {
  SomeClass *local = nil;
  return local; // expected-warning {{Null is returned from a method that is expected to return a non-null value}}
}

it is expected to produce a warning, when a method marked as nonnull returns nil while no precondition is violated.
So I would expect that the existing tests already proof that my patch didn't produce unwanted side-effects.

Sorry in case that I'm missing your point here, maybe you can elaborate it a bit more?

[parallaxe]

Moved the tests into nullability_nullonly.mm according to @dcoughlin.
Applied the changes according to @aaron.ballman.

[ahatanak]

In D23236#523173, @parallaxe wrote:

In D23236#509013, @ahatanak wrote:

+cfe-commits

If this patch is applied, does clang issue a warning if a method marked "nonnull" returns a null value? I see a warning is issued for conditional expressions in the test case you've added, but I don't see a test case for a function returning just nil or 0.

I was wondering whether the change made in this patch contradicts what's stated in r240146's commit log:

"Note that we don't warn about nil returns from Objective-C methods,

because it's common for Objective-C methods to mimic the nil-swallowing
behavior of the receiver by checking ostensibly non-null parameters
and returning nil from otherwise non-null methods in that
case."

I'm not sure how this is meant. As far as I understand the commit-message you quoted, it is related to methods that return nil when a precondition fails. A test for this would be doNotWarnWhenPreconditionIsViolatedInTopFunc in nullability_nullonly.mm, which is not affected by my patch.
On the other side, according to the test testReturnsNilInNonnull in nullability_nullonly.mm

- (SomeClass * _Nonnull)testReturnsNilInNonnull {
  SomeClass *local = nil;
  return local; // expected-warning {{Null is returned from a method that is expected to return a non-null value}}
}

it is expected to produce a warning, when a method marked as nonnull returns nil while no precondition is violated.
So I would expect that the existing tests already proof that my patch didn't produce unwanted side-effects.

Sorry in case that I'm missing your point here, maybe you can elaborate it a bit more?

Thank you for the explanation, I think I understand the comment now. I was wondering whether the analyzer was being overzealous since r240153's commit log clearly states that Sema never warns about nonnull methods returning nil, but I agree with you that probably it was just trying to avoid false positive warnings for functions like doNotWarnWhenPreconditionIsViolatedInTopFunc.

[parallaxe]

Uhm, is there something missing from my side that i have overseen to go forward with this patch?

[aaron.ballman]

LGTM, but please wait for @dcoughlin or @ahatanak to chime in as well.

[ahatanak]

This looks fine to me.

[dcoughlin]

LGTM.

[dcoughlin]

@parallaxe Do you need someone to commit this for you?

[parallaxe]

I tried to commit by using arc, though it didn't work as I was prompted to login into the subversion-repository, which I can't. At this point I'm not sure if this should have worked or not, but I assumed that arc + phabricator would handle it in a way that i wouldn't need write-access to the repository itself.
So, yes, it would be nice when someone can actually commit it for me.

[dcoughlin]

Upon reflection, I don't think this is the right approach.

Desugaring any AttributedType in the return type seems like a really, really big hammer and could be an unexpected surprise for future attributed types where this is not the right behavior. I think a better approach for the nullability issue would be to update lookThroughImplicitCasts() in NullabilityChecker.cpp to look though ExprWithCleanups expressions just like it currently does for implicit casts.

What do you think?

[parallaxe]

In D23236#557898, @dcoughlin wrote:

Upon reflection, I don't think this is the right approach.

Desugaring any AttributedType in the return type seems like a really, really big hammer and could be an unexpected surprise for future attributed types where this is not the right behavior. I think a better approach for the nullability issue would be to update lookThroughImplicitCasts() in NullabilityChecker.cpp to look though ExprWithCleanups expressions just like it currently does for implicit casts.

What do you think?

As far as I understand the code, I would leave my change as it is.
The part of the code I have changed tries to guess the return-type for the expression inside the return-stmt. At this point, no type could be computed (as RelatedRetType.isNull() is true). Assuming that any annotation that the return-type of the function has, will also apply to the return-type of the expression, might be misleading. The improvements of the warnings in nullability.m are a good sign of this (which would be lost when I would just make a change in the NullabilityChecker).

Does that sound sensible?

[dcoughlin]

In D23236#561467, @parallaxe wrote:

In D23236#557898, @dcoughlin wrote:

Upon reflection, I don't think this is the right approach.

Desugaring any AttributedType in the return type seems like a really, really big hammer and could be an unexpected surprise for future attributed types where this is not the right behavior. I think a better approach for the nullability issue would be to update lookThroughImplicitCasts() in NullabilityChecker.cpp to look though ExprWithCleanups expressions just like it currently does for implicit casts.

What do you think?

As far as I understand the code, I would leave my change as it is.

You definitely can't just desugar the return type before passing it to InitializedEntity::InitializeResult(). Doing so would place a potential landmine for someone to trip over in the future for cases where the type qualifier has a real semantic meaning. For nullability qualifiers desugaring seems mostly harmless (except for the diagnostic regressions) but for an arbitrary type qualifier I don't think this is acceptable.

The part of the code I have changed tries to guess the return-type for the expression inside the return-stmt. At this point, no type could be computed (as RelatedRetType.isNull() is true). Assuming that any annotation that the return-type of the function has, will also apply to the return-type of the expression, might be misleading.

This code doesn't assume that the the return type of the function is the return type of the expression. Instead, it uses the return type of the function to drive semantic analysis of the expression and apply any conversions needed from the type of the returned expression to the return type of the function. (In this case a 'NullToPointer' conversion). These conversions are often represented as implicit casts, added by Sema, in the AST. See InitializationSequence::Perform() for all the kinds of implicit conversions that can go on here. It is important to record these casts so that any codegen needed to perform the conversions/casts can be emitted at the right place. If such a conversion is not allowed, a diagnostic will be emitted.

The improvements of the warnings in nullability.m are a good sign of this (which would be lost when I would just make a change in the NullabilityChecker).

The diagnostics changes to Sema/nullability.m in this patch would be a regression. Those diagnostic tests are there explicitly to test the functionality implemented in mergeInterfaceMethodToImpl() in SemaDeclObjC.cpp. This code takes a nullability annotation on a return type from a declaration and makes sure that the return type in the definition has the same nullability even if the programmer did not explicitly write it. This means that, for example, the return type of -[NSMerge methodA:] in the @implementation is supposed to be 'NSFoo * _Nonnull' since in the @interface it is annotated with 'nonnull'. You can see the merged return type for the method declaration by passing -Xclang -ast-dump to the driver.

[dcoughlin]

I left out the example I was describing above for 'InitializationSequence::Perform()':

@interface Bar
- (nonnull Bar *)method;
@end

@implementation Bar
- (Bar *)method {
  return 0;
}
@end

lib/Sema/SemaStmt.cpp

if (FnRetType->isVoidType()) {
     else
       Diag(ReturnLoc, DiagID) << getCurMethodDecl()->getDeclName() << 1/*meth*/;
 
     Result = new (Context) ReturnStmt(ReturnLoc);
   } else {
     assert(RetValExp || HasDependentReturnType);
     const VarDecl *NRVOCandidate = nullptr;
 
-    QualType RetType = RelatedRetType.isNull() ? FnRetType : RelatedRetType;
+    QualType RetType;
+    if (RelatedRetType.isNull()) {
+      RetType = FnRetType;
+      // The nullability-attributes of the return-type of a function/method
+      // is not necessarily conforming to the nullability-attributes of
+      // the expression itself, so we remove the nullability-attributes.
+      if(const auto *AttrType = RetType->getAs<AttributedType>())
+        RetType = AttrType->desugar();
+    } else {
+      RetType = RelatedRetType;
+    }
 
     // C99 6.8.6.4p3(136): The return statement is not an assignment. The
     // overlap restriction of subclause 6.5.16.1 does not apply to the case of
     // function return.
 
     // In C++ the return statement is handled via a copy initialization,
     // the C version of which boils down to CheckSingleAssignmentConstraints.
     if (RetValExp)

test/Analysis/nullability_nullonly.mm

if (getRandom())
       return ret; // no warning
     else
       return p; // no warning
   } else {
     return p;
   }
 }
 
+NSString *_Nonnull testNullReturnInTernaryOperator(int x) {
+  return x > 3 ? nil : [@"" stringByAppendingString:@""]; // expected-warning {{Null is returned from a function that is expected to return a non-null value}}
+}
+
 void testPreconditionViolationInInlinedFunction(Dummy *p) {
   doNotWarnWhenPreconditionIsViolated(p);
 }
 
 void inlinedNullable(Dummy *_Nullable p) {
   if (p) return;
 }
 void inlinedNonnull(Dummy *_Nonnull p) {

 
 - (SomeClass * _Nonnull)testReturnsNilInNonnullWhenPreconditionViolated:(SomeClass * _Nonnull) p {
   SomeClass *local = nil;
   if (!p) // Pre-condition violated here.
     return local; // no-warning
   else
     return p; // no-warning
 }
+
+- (NSString * _Nonnull)testNullReturnInTernaryOperator:(int)x {
+  return x > 3 ? nil : [@"" stringByAppendingString:@""]; // expected-warning {{Null is returned from a method that is expected to return a non-null value}}
+}
 @end
 
 
 void callFunctionInSystemHeader() {
   NSString *s;
   s = nil;
 
   NSSystemFunctionTakingNonnull(s);

test/SemaObjC/nullability.m

- (NSFoo **)invalidMethod1 {
   return 0; 
 }
 
 - (NSFoo *)conflictingMethod1 { 
   return 0; // no warning
 }
 - (NSFoo *)redundantMethod1 {
   int *ip = 0;
-  return ip; // expected-warning{{result type 'NSFoo * _Nonnull'}}
+  return ip; // expected-warning{{incompatible pointer types returning 'int *' from a function with result type 'NSFoo *'}}
 }
 @end
 
 __attribute__((objc_root_class))
 @interface NSMerge
 - (nonnull NSFoo *)methodA:(nonnull NSFoo*)foo;
 - (nonnull NSFoo *)methodB:(nonnull NSFoo*)foo;
 - (NSFoo *)methodC:(NSFoo*)foo;
 @end
 
 @implementation NSMerge
 - (NSFoo *)methodA:(NSFoo*)foo {
   int *ptr = foo; // expected-warning{{incompatible pointer types initializing 'int *' with an expression of type 'NSFoo * _Nonnull'}}
-  return ptr; // expected-warning{{result type 'NSFoo * _Nonnull'}}
+  return ptr; // expected-warning{{incompatible pointer types returning 'int *' from a function with result type 'NSFoo *'}}
 }
 
 - (nullable NSFoo *)methodB:(null_unspecified NSFoo*)foo { // expected-error{{nullability specifier 'nullable' conflicts with existing specifier 'nonnull'}} \
   // expected-error{{nullability specifier 'null_unspecified' conflicts with existing specifier 'nonnull'}}
   return 0;
 }
 
 - (nonnull NSFoo *)methodC:(nullable NSFoo*)foo {
   int *ip = 0;
-  return ip; // expected-warning{{result type 'NSFoo * _Nonnull'}}
+  return ip; // expected-warning{{incompatible pointer types returning 'int *' from a function with result type 'NSFoo *'}}
 }
 @end
 
 // Checking merging of nullability when sending a message.
 @interface NSMergeReceiver
 - (id)returnsNone;
 - (nonnull id)returnsNonNull;
 - (nullable id)returnsNullable;

 void test_null_resettable(NSResettable *r, int *ip) {
   [r setResettable1:ip]; // expected-warning{{incompatible pointer types sending 'int *' to parameter of type 'NSResettable * _Nullable'}}
   r.resettable1 = ip; // expected-warning{{incompatible pointer types assigning to 'NSResettable * _Nullable' from 'int *'}}
 }
 
 @implementation NSResettable // expected-warning{{synthesized setter 'setResettable4:' for null_resettable property 'resettable4' does not handle nil}}
 - (NSResettable *)resettable1 {
   int *ip = 0;
-  return ip; // expected-warning{{result type 'NSResettable * _Nonnull'}}
+  return ip; // expected-warning{{incompatible pointer types returning 'int *' from a function with result type 'NSResettable *'}}
 }
 
 - (void)setResettable1:(NSResettable *)param {
 }
 
 @synthesize resettable2; // no warning; not synthesized
 @synthesize resettable3; // expected-warning{{synthesized setter 'setResettable3:' for null_resettable property 'resettable3' does not handle nil}}