[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...

[Akira Hatanaka via cfe-commits]

ahatanak added a comment.

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

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


https://reviews.llvm.org/D23236