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

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a subscriber: dergachev.a.
jordan_rose added a comment.

Sorry for the delay. I think this is mostly good, but I do still have a concern about the diagnostics change.



================
Comment at: lib/Sema/SemaExpr.cpp:7117
+    if (E && S.checkNonNullExpr(E))
+      return NullabilityKind::Nullable;
+
----------------
ahatanak wrote:
> jordan_rose wrote:
> > This isn't quite correct, unfortunately. `(_Nonnull id)nil` should be considered non-nullable, since it's the canonical way to avoid all these warnings. It might just be good enough to move this check below the `getNullability` one, though.
> Sema::CheckNonNullExpr checks the nullability of the type of the expression first and returns false if there is a cast to `_Nonnull`.
Hm, okay, I see you have a test. Sorry for the noise.


================
Comment at: lib/Sema/SemaExpr.cpp:7162
+                                              bool IsBin, Expr *LHSExpr,
+                                              Expr *RHSExpr, ASTContext &Ctx) {
   if (!ResTy->isAnyPointerType())
----------------
Nitpick: you could use `const` on the Exprs here.


================
Comment at: lib/Sema/SemaExpr.cpp:11103
 
+  if (const auto *DeclRef = dyn_cast<DeclRefExpr>(LHSExpr))
+    checkNullConstantToNonNull(DeclRef->getType(), RHS.get());
----------------
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.


================
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'}}
 }
----------------
@dergachev.a, what do you think here? Is it okay that the analyzer diagnoses this in addition to the new warning?


================
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'}}
 
----------------
This seems like an unfortunate change to make, since most people do not bother with nullability.


Repository:
  rC Clang

https://reviews.llvm.org/D22391