Page MenuHomePhabricator

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...
Needs RevisionPublic

Authored by parallaxe on Aug 6 2016, 2:08 AM.

Details

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.

Diff Detail

Event Timeline

parallaxe updated this revision to Diff 67070.Aug 6 2016, 2:08 AM
parallaxe retitled this revision from to 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 updated this object.

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 edited edge metadata.Aug 8 2016, 1:34 PM
ahatanak added a subscriber: cfe-commits.

+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 added inline comments.Aug 8 2016, 5:26 PM
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 marked 4 inline comments as done.Aug 23 2016, 8:11 AM

+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 updated this revision to Diff 69000.Aug 23 2016, 8:35 AM
parallaxe edited edge metadata.

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

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

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

aaron.ballman accepted this revision.Sep 26 2016, 8:14 AM
aaron.ballman edited edge metadata.

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

This revision is now accepted and ready to land.Sep 26 2016, 8:14 AM

This looks fine to me.

dcoughlin accepted this revision.Sep 26 2016, 8:19 PM
dcoughlin edited edge metadata.

LGTM.

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

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 requested changes to this revision.Sep 30 2016, 2:37 PM
dcoughlin edited edge metadata.

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?

This revision now requires changes to proceed.Sep 30 2016, 2:37 PM

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?

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.

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