[PATCH] D22391: [Sema] Add warning for implicitly casting a null constant to a non null pointer type

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: test/Analysis/nullability_nullonly.mm:103
 void testObjCARCExplicitZeroInitialization() {
-  TestObject * _Nonnull explicitlyZeroInitialized = nil; // expected-warning {{nil assigned to a pointer which is expected to have non-null value}}
+  TestObject * _Nonnull explicitlyZeroInitialized = nil; // expected-warning {{nil assigned to a pointer which is expected to have non-null value}} expected-warning{{implicitly casting a null constant to non-nullable pointer type 'TestObject * _Nonnull __strong'}}
 }
----------------
ahatanak wrote:
> NoQ wrote:
> > jordan_rose wrote:
> > > @dergachev.a, what do you think here? Is it okay that the analyzer diagnoses this in addition to the new warning?
> > We're usually trying to avoid this when we notice it, but there are many cases where we didn't notice it because both warnings and the analyzer are becoming better independently. I guess you could just give us a heads up with a bug report if you don't want to bother with this.
> > 
> > In this case i think it should be easy to fix though, because the analyzer already has `static isARCNilInitializedLocal()` that suppresses implicit null initializers, we could teach it to suppress all null initializers instead.
> I think the warning in nullability-no-arc.mm should be suppressed too? I made changes to isARCNilInitializedLocal so that it returns true when the initializer is explicit or when it is not ARC. I'm not sure whether this is the right way to fix it, but it doesn't cause any regression tests to fail.
Yep, that makes sense, thank you!

It seems that we also need to check that the initializer is a null/nil literal, in a manner similar to how the warning checks it, otherwise we lose an analyzer positive on
```
int *foo() {
  int *x = 0;
  int *_Nonnull y = x; // expected-warning{{Null assigned to a pointer which is expected to have non-null value}}
  return y;
}
```
(which wasn't there in the test suite, so i had to make it up)

And i guess you'll need to rename the function and update comments, and parameter `C` also becomes unused (not sure it'll remain this way when the aforementioned test is fixed).


Repository:
  rC Clang

https://reviews.llvm.org/D22391