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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.


================
Comment at: lib/Sema/SemaExpr.cpp:11103
 
+  if (const auto *DeclRef = dyn_cast<DeclRefExpr>(LHSExpr))
+    checkNullConstantToNonNull(DeclRef->getType(), RHS.get());
----------------
jordan_rose wrote:
> This could come later, but what about struct members or ObjC properties or ObjC subscripts? Seems like you could just check the type of the LHS.
I removed the check for DeclRef so that it warns on struct members (see test case in null_constant_to_nonnull.c). 

CheckNonNullArgument already checks null arguments passed to ObjC properties and subscripts setter methods. I added a new test case to test/SemaObjC/nullability.m.


================
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'}}
 }
----------------
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.


================
Comment at: test/Sema/conditional-expr.c:20
   vp = 0 ? (double *)0 : (void *)0;
-  ip = 0 ? (double *)0 : (void *)0; // expected-warning {{incompatible pointer types assigning to 'int *' from 'double *'}}
+  ip = 0 ? (double *)0 : (void *)0; // expected-warning {{incompatible pointer types assigning to 'int *' from 'double * _Nullable'}}
 
----------------
jordan_rose wrote:
> This seems like an unfortunate change to make, since most people do not bother with nullability.
Yes, this is unfortunate, but I'm not sure what's the right way to avoid printing nullability specifiers in the diagnostic message. Do you have any suggestions?


Repository:
  rC Clang

https://reviews.llvm.org/D22391