[PATCH] D23236: When ARC is enabled, no warning will be generated when a method1. 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...

[Devin Coughlin via cfe-commits]

dcoughlin added a comment.

In https://reviews.llvm.org/D23236#561467, @parallaxe wrote:

> In https://reviews.llvm.org/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.


https://reviews.llvm.org/D23236