Re: [PATCH] D22392: [Sema] Fix an invalid nullability warning for binary conditional operators

2016-07-19 Thread Akira Hatanaka via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL276076: [Sema] Compute the nullability of a conditional 
expression based on the (authored by ahatanak).

Changed prior to commit:
  https://reviews.llvm.org/D22392?vs=64557&id=64628#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D22392

Files:
  cfe/trunk/lib/Sema/SemaExpr.cpp
  cfe/trunk/test/Sema/nullability.c
  cfe/trunk/test/SemaCXX/nullability.cpp

Index: cfe/trunk/lib/Sema/SemaExpr.cpp
===
--- cfe/trunk/lib/Sema/SemaExpr.cpp
+++ cfe/trunk/lib/Sema/SemaExpr.cpp
@@ -7002,6 +7002,55 @@
 SourceRange(CondRHS->getLocStart(), RHSExpr->getLocEnd()));
 }
 
+/// Compute the nullability of a conditional expression.
+static QualType computeConditionalNullability(QualType ResTy, bool IsBin,
+  QualType LHSTy, QualType RHSTy,
+  ASTContext &Ctx) {
+  if (!ResTy->isPointerType())
+return ResTy;
+
+  auto GetNullability = [&Ctx](QualType Ty) {
+Optional Kind = Ty->getNullability(Ctx);
+if (Kind)
+  return *Kind;
+return NullabilityKind::Unspecified;
+  };
+
+  auto LHSKind = GetNullability(LHSTy), RHSKind = GetNullability(RHSTy);
+  NullabilityKind MergedKind;
+
+  // Compute nullability of a binary conditional expression.
+  if (IsBin) {
+if (LHSKind == NullabilityKind::NonNull)
+  MergedKind = NullabilityKind::NonNull;
+else
+  MergedKind = RHSKind;
+  // Compute nullability of a normal conditional expression.
+  } else {
+if (LHSKind == NullabilityKind::Nullable ||
+RHSKind == NullabilityKind::Nullable)
+  MergedKind = NullabilityKind::Nullable;
+else if (LHSKind == NullabilityKind::NonNull)
+  MergedKind = RHSKind;
+else if (RHSKind == NullabilityKind::NonNull)
+  MergedKind = LHSKind;
+else
+  MergedKind = NullabilityKind::Unspecified;
+  }
+
+  // Return if ResTy already has the correct nullability.
+  if (GetNullability(ResTy) == MergedKind)
+return ResTy;
+
+  // Strip all nullability from ResTy.
+  while (ResTy->getNullability(Ctx))
+ResTy = ResTy.getSingleStepDesugaredType(Ctx);
+
+  // Create a new AttributedType with the new nullability kind.
+  auto NewAttr = AttributedType::getNullabilityAttrKind(MergedKind);
+  return Ctx.getAttributedType(NewAttr, ResTy, ResTy);
+}
+
 /// ActOnConditionalOp - Parse a ?: operation.  Note that 'LHS' may be null
 /// in the case of a the GNU conditional expr extension.
 ExprResult Sema::ActOnConditionalOp(SourceLocation QuestionLoc,
@@ -7069,6 +7118,7 @@
 LHSExpr = CondExpr = opaqueValue;
   }
 
+  QualType LHSTy = LHSExpr->getType(), RHSTy = RHSExpr->getType();
   ExprValueKind VK = VK_RValue;
   ExprObjectKind OK = OK_Ordinary;
   ExprResult Cond = CondExpr, LHS = LHSExpr, RHS = RHSExpr;
@@ -7083,6 +7133,9 @@
 
   CheckBoolLikeConversion(Cond.get(), QuestionLoc);
 
+  result = computeConditionalNullability(result, commonExpr, LHSTy, RHSTy,
+ Context);
+
   if (!commonExpr)
 return new (Context)
 ConditionalOperator(Cond.get(), QuestionLoc, LHS.get(), ColonLoc,
Index: cfe/trunk/test/SemaCXX/nullability.cpp
===
--- cfe/trunk/test/SemaCXX/nullability.cpp
+++ cfe/trunk/test/SemaCXX/nullability.cpp
@@ -97,3 +97,23 @@
 
   TakeNonnull(ReturnNullable()); //expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull}}
 }
+
+void ConditionalExpr(bool c) {
+  struct Base {};
+  struct Derived : Base {};
+
+  Base * _Nonnull p;
+  Base * _Nonnull nonnullB;
+  Base * _Nullable nullableB;
+  Derived * _Nonnull nonnullD;
+  Derived * _Nullable nullableD;
+
+  p = c ? nonnullB : nonnullD;
+  p = c ? nonnullB : nullableD; // expected-warning{{implicit conversion from nullable pointer 'Base * _Nullable' to non-nullable pointer type 'Base * _Nonnull}}
+  p = c ? nullableB : nonnullD; // expected-warning{{implicit conversion from nullable pointer 'Base * _Nullable' to non-nullable pointer type 'Base * _Nonnull}}
+  p = c ? nullableB : nullableD; // expected-warning{{implicit conversion from nullable pointer 'Base * _Nullable' to non-nullable pointer type 'Base * _Nonnull}}
+  p = c ? nonnullD : nonnullB;
+  p = c ? nonnullD : nullableB; // expected-warning{{implicit conversion from nullable pointer 'Base * _Nullable' to non-nullable pointer type 'Base * _Nonnull}}
+  p = c ? nullableD : nonnullB; // expected-warning{{implicit conversion from nullable pointer 'Base * _Nullable' to non-nullable pointer type 'Base * _Nonnull}}
+  p = c ? nullableD : nullableB; // expected-warning{{implicit conversion from nullable pointer 'Base * _Nullable' to non-nullable pointer type 'Base * _Nonnull}}
+}
Index: cfe/trunk/test/Sema/nullability.c

Re: [PATCH] D22392: [Sema] Fix an invalid nullability warning for binary conditional operators

2016-07-19 Thread Doug Gregor via cfe-commits 
doug.gregor accepted this revision.
doug.gregor added a comment.
This revision is now accepted and ready to land.

LGTM!


https://reviews.llvm.org/D22392



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22392: [Sema] Fix an invalid nullability warning for binary conditional operators

2016-07-19 Thread Akira Hatanaka via cfe-commits 
ahatanak updated this revision to Diff 64557.
ahatanak added a comment.

Remove the check "if (LHSKind == RHSKind)" in computeConditionalNullability as 
it's not needed.


https://reviews.llvm.org/D22392

Files:
  lib/Sema/SemaExpr.cpp
  test/Sema/nullability.c
  test/SemaCXX/nullability.cpp

Index: test/SemaCXX/nullability.cpp
===
--- test/SemaCXX/nullability.cpp
+++ test/SemaCXX/nullability.cpp
@@ -97,3 +97,23 @@
 
   TakeNonnull(ReturnNullable()); //expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull}}
 }
+
+void ConditionalExpr(bool c) {
+  struct Base {};
+  struct Derived : Base {};
+
+  Base * _Nonnull p;
+  Base * _Nonnull nonnullB;
+  Base * _Nullable nullableB;
+  Derived * _Nonnull nonnullD;
+  Derived * _Nullable nullableD;
+
+  p = c ? nonnullB : nonnullD;
+  p = c ? nonnullB : nullableD; // expected-warning{{implicit conversion from nullable pointer 'Base * _Nullable' to non-nullable pointer type 'Base * _Nonnull}}
+  p = c ? nullableB : nonnullD; // expected-warning{{implicit conversion from nullable pointer 'Base * _Nullable' to non-nullable pointer type 'Base * _Nonnull}}
+  p = c ? nullableB : nullableD; // expected-warning{{implicit conversion from nullable pointer 'Base * _Nullable' to non-nullable pointer type 'Base * _Nonnull}}
+  p = c ? nonnullD : nonnullB;
+  p = c ? nonnullD : nullableB; // expected-warning{{implicit conversion from nullable pointer 'Base * _Nullable' to non-nullable pointer type 'Base * _Nonnull}}
+  p = c ? nullableD : nonnullB; // expected-warning{{implicit conversion from nullable pointer 'Base * _Nullable' to non-nullable pointer type 'Base * _Nonnull}}
+  p = c ? nullableD : nullableB; // expected-warning{{implicit conversion from nullable pointer 'Base * _Nullable' to non-nullable pointer type 'Base * _Nonnull}}
+}
Index: test/Sema/nullability.c
===
--- test/Sema/nullability.c
+++ test/Sema/nullability.c
@@ -128,3 +128,70 @@
 
   accepts_nonnull_1(ptr); // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
 }
+
+// Check nullability of conditional expressions.
+void conditional_expr(int c) {
+  int * _Nonnull p;
+  int * _Nonnull nonnullP;
+  int * _Nullable nullableP;
+  int * _Null_unspecified unspecifiedP;
+  int *noneP;
+
+  p = c ? nonnullP : nonnullP;
+  p = c ? nonnullP : nullableP; // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
+  p = c ? nonnullP : unspecifiedP;
+  p = c ? nonnullP : noneP;
+  p = c ? nullableP : nonnullP; // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
+  p = c ? nullableP : nullableP; // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
+  p = c ? nullableP : unspecifiedP; // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
+  p = c ? nullableP : noneP; // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
+  p = c ? unspecifiedP : nonnullP;
+  p = c ? unspecifiedP : nullableP; // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
+  p = c ? unspecifiedP : unspecifiedP;
+  p = c ? unspecifiedP : noneP;
+  p = c ? noneP : nonnullP;
+  p = c ? noneP : nullableP; // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
+  p = c ? noneP : unspecifiedP;
+  p = c ? noneP : noneP;
+
+  // Check that we don't remove all sugar when creating a new QualType for the
+  // conditional expression.
+  typedef int *IntP;
+  typedef IntP _Nonnull NonnullIntP0;
+  typedef NonnullIntP0 _Nonnull NonnullIntP1;
+  typedef IntP _Nullable NullableIntP0;
+  typedef NullableIntP0 _Nullable NullableIntP1;
+  NonnullIntP1 nonnullP2;
+  NullableIntP1 nullableP2;
+
+  p = c ? nonnullP2 : nonnullP2;
+  p = c ? nonnullP2 : nullableP2; // expected-warning{{implicit conversion from nullable pointer 'IntP _Nullable' (aka 'int *') to non-nullable pointer type 'int * _Nonnull'}}
+  p = c ? nullableP2 : nonnullP2; // expected-warning{{implicit conversion from nullable pointer 'NullableIntP1' (aka 'int *') to non-nullable pointer type 'int * _Nonnull'}}
+  p = c ? nullableP2 : nullableP2; // expected-warning{{implicit conversion from nullable pointer 'NullableIntP1' (aka 'int *') to non-nullable pointer type 'int * _Nonnull'}}
+}
+
+// Check nullability of binary conditional expressions.
+void binary_conditional_expr() {
+  int * _Nonnull p;
+  int * _Nonnull nonnullP;

Re: [PATCH] D22392: [Sema] Fix an invalid nullability warning for binary conditional operators

2016-07-19 Thread Akira Hatanaka via cfe-commits 
ahatanak updated this revision to Diff 64551.
ahatanak added a comment.

Address review comments.

- Rename function to computeConditionalNullability.
- Rewrite the function to compute the nullability of both normal and binary 
conditional expressions.
- Add more test cases.


https://reviews.llvm.org/D22392

Files:
  lib/Sema/SemaExpr.cpp
  test/Sema/nullability.c
  test/SemaCXX/nullability.cpp

Index: test/SemaCXX/nullability.cpp
===
--- test/SemaCXX/nullability.cpp
+++ test/SemaCXX/nullability.cpp
@@ -97,3 +97,23 @@
 
   TakeNonnull(ReturnNullable()); //expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull}}
 }
+
+void ConditionalExpr(bool c) {
+  struct Base {};
+  struct Derived : Base {};
+
+  Base * _Nonnull p;
+  Base * _Nonnull nonnullB;
+  Base * _Nullable nullableB;
+  Derived * _Nonnull nonnullD;
+  Derived * _Nullable nullableD;
+
+  p = c ? nonnullB : nonnullD;
+  p = c ? nonnullB : nullableD; // expected-warning{{implicit conversion from nullable pointer 'Base * _Nullable' to non-nullable pointer type 'Base * _Nonnull}}
+  p = c ? nullableB : nonnullD; // expected-warning{{implicit conversion from nullable pointer 'Base * _Nullable' to non-nullable pointer type 'Base * _Nonnull}}
+  p = c ? nullableB : nullableD; // expected-warning{{implicit conversion from nullable pointer 'Base * _Nullable' to non-nullable pointer type 'Base * _Nonnull}}
+  p = c ? nonnullD : nonnullB;
+  p = c ? nonnullD : nullableB; // expected-warning{{implicit conversion from nullable pointer 'Base * _Nullable' to non-nullable pointer type 'Base * _Nonnull}}
+  p = c ? nullableD : nonnullB; // expected-warning{{implicit conversion from nullable pointer 'Base * _Nullable' to non-nullable pointer type 'Base * _Nonnull}}
+  p = c ? nullableD : nullableB; // expected-warning{{implicit conversion from nullable pointer 'Base * _Nullable' to non-nullable pointer type 'Base * _Nonnull}}
+}
Index: test/Sema/nullability.c
===
--- test/Sema/nullability.c
+++ test/Sema/nullability.c
@@ -128,3 +128,70 @@
 
   accepts_nonnull_1(ptr); // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
 }
+
+// Check nullability of conditional expressions.
+void conditional_expr(int c) {
+  int * _Nonnull p;
+  int * _Nonnull nonnullP;
+  int * _Nullable nullableP;
+  int * _Null_unspecified unspecifiedP;
+  int *noneP;
+
+  p = c ? nonnullP : nonnullP;
+  p = c ? nonnullP : nullableP; // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
+  p = c ? nonnullP : unspecifiedP;
+  p = c ? nonnullP : noneP;
+  p = c ? nullableP : nonnullP; // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
+  p = c ? nullableP : nullableP; // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
+  p = c ? nullableP : unspecifiedP; // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
+  p = c ? nullableP : noneP; // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
+  p = c ? unspecifiedP : nonnullP;
+  p = c ? unspecifiedP : nullableP; // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
+  p = c ? unspecifiedP : unspecifiedP;
+  p = c ? unspecifiedP : noneP;
+  p = c ? noneP : nonnullP;
+  p = c ? noneP : nullableP; // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
+  p = c ? noneP : unspecifiedP;
+  p = c ? noneP : noneP;
+
+  // Check that we don't remove all sugar when creating a new QualType for the
+  // conditional expression.
+  typedef int *IntP;
+  typedef IntP _Nonnull NonnullIntP0;
+  typedef NonnullIntP0 _Nonnull NonnullIntP1;
+  typedef IntP _Nullable NullableIntP0;
+  typedef NullableIntP0 _Nullable NullableIntP1;
+  NonnullIntP1 nonnullP2;
+  NullableIntP1 nullableP2;
+
+  p = c ? nonnullP2 : nonnullP2;
+  p = c ? nonnullP2 : nullableP2; // expected-warning{{implicit conversion from nullable pointer 'IntP _Nullable' (aka 'int *') to non-nullable pointer type 'int * _Nonnull'}}
+  p = c ? nullableP2 : nonnullP2; // expected-warning{{implicit conversion from nullable pointer 'NullableIntP1' (aka 'int *') to non-nullable pointer type 'int * _Nonnull'}}
+  p = c ? nullableP2 : nullableP2; // expected-warning{{implicit conversion from nullable pointer 'NullableIntP1' (aka 'int *') to non-nullable pointer type 'int * _Nonnull'}}
+}
+
+// Check nullability of binary co

Re: [PATCH] D22392: [Sema] Fix an invalid nullability warning for binary conditional operators

2016-07-15 Thread Akira Hatanaka via cfe-commits 
ahatanak added a comment.

I'm thinking about taking the following steps to compute the nullability of 
ternary operators.

The first step is to compute the "merged" nullability of the LHS and RHS.

For normal ternary operators (not the shorthand version):

- If both LHS and RHS have the same nullability (one of nonnull, nullable, 
null_unspecified, or none), pick that as the merged nullability.
- Otherwise, if either side is nullable, the merged nullability is nullable too.
- Otherwise, if either side is nonnull, pick the nullability of the other side. 
For example, if (LHS,RHS) == (nonnull, none), the merged nullability is none 
because we can't guarantee that the result is nonnull.
- Otherwise, the nullability is either null_unspecified or none. I think we can 
pick either one in this case.

For binary conditional operators like "p0 ?: p1":

- If the LHS (p0) is nonnull, the merged nullability is nonnull.
- Otherwise, the merged nullability is the nullability of the RHS (p1).

Once we have the merged nullability, the next step is to compare it with the 
result's nullability. If the nullability kinds differ, a new type that has the 
merged nullability is created.

Note that it looks like we might be able to simplify the steps described above 
if we take advantage of the fact that clang only warns when a nullable pointer 
is assigned/passed to a nonnull pointer (I don't think clang issues any 
warnings, for example, when a null_unspecified pointer is assigned to a nonnull 
pointer).


https://reviews.llvm.org/D22392



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22392: [Sema] Fix an invalid nullability warning for binary conditional operators

2016-07-15 Thread Akira Hatanaka via cfe-commits 
ahatanak added inline comments.


Comment at: lib/Sema/SemaExpr.cpp:7007
@@ +7006,3 @@
+/// expression.
+static QualType modifyNullability(QualType ResTy, Expr *RHSExpr,
+  ASTContext &Ctx) {

doug.gregor wrote:
> This name could be improved. You're not really 'modifying' nullability here; 
> in the general case, you're computing the appropriate nullability given the 
> LHS, RHS, and applying that to the result type.
I'm thinking about renaming it to computeConditionalNullability or something, 
but I'm open to suggestions.


Comment at: lib/Sema/SemaExpr.cpp:7024
@@ +7023,3 @@
+
+  NullabilityKind NewKind = GetNullability(RHSExpr->getType());
+

doug.gregor wrote:
> I'm fairly certain that more extensive testing will show that you need to 
> have the LHSExpr as well, to look at the nullability of both.
This patch only tries to correct the case where you have a binary (shorthand 
version) conditional operator "p0 ?: p1" where p1 is nonnulll and p0 is 
nullable, in which case the result type should be nonnull. I think this 
function behaves correctly in this particular case, but it's possible that I 
have overlooked some corner cases.

In any case, I think the focus of this patch was too narrow. I think I should 
fix the nullability of any conditional operators, not just the shorthand 
versions, in my next patch.


Comment at: lib/Sema/SemaExpr.cpp:7030
@@ +7029,3 @@
+  // Create a new AttributedType with the new nullability kind.
+  QualType NewTy = ResTy.getDesugaredType(Ctx);
+  auto NewAttr = AttributedType::getNullabilityAttrKind(NewKind);

doug.gregor wrote:
>  It would be better to only unwrap sugar until we hit the 
> nullability-attributed type, then replace it.
I think we need to unwrap sugar more than once until there are no more 
nullability-attributed types in some cases. For example, it's possible to have 
multiple levels of typedefs having nullability markers:

```
typedef int * IntP;
typedef IntP _Nullable NullableIntP0;
typedef NullableIntP0 _Nullable NullableIntP1;
```


Comment at: test/Sema/nullability.c:137
@@ +136,3 @@
+
+  int * _Nonnull p2 = p0 ?: p1; // no warnings here.
+}

doug.gregor wrote:
> You really need much more testing coverage here, e.g., for ternary operators 
> where the types on the second and third arguments are different types (say, 
> superclass/subclass pointer), the nullability is on either argument, etc. The 
> ternary operator, especially in C++, has a ton of different cases that you 
> need to look at.
Yes, I'll have more test cases in my next patch.


https://reviews.llvm.org/D22392



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22392: [Sema] Fix an invalid nullability warning for binary conditional operators

2016-07-14 Thread Doug Gregor via cfe-commits 
doug.gregor added a comment.

A bunch of comments above. This needs much more extensive testing, because 
there are numerous paths through the ternary operator code and the results need 
to be symmetric.



Comment at: lib/Sema/SemaExpr.cpp:7007
@@ +7006,3 @@
+/// expression.
+static QualType modifyNullability(QualType ResTy, Expr *RHSExpr,
+  ASTContext &Ctx) {

This name could be improved. You're not really 'modifying' nullability here; in 
the general case, you're computing the appropriate nullability given the LHS, 
RHS, and applying that to the result type.


Comment at: lib/Sema/SemaExpr.cpp:7024
@@ +7023,3 @@
+
+  NullabilityKind NewKind = GetNullability(RHSExpr->getType());
+

I'm fairly certain that more extensive testing will show that you need to have 
the LHSExpr as well, to look at the nullability of both.


Comment at: lib/Sema/SemaExpr.cpp:7030
@@ +7029,3 @@
+  // Create a new AttributedType with the new nullability kind.
+  QualType NewTy = ResTy.getDesugaredType(Ctx);
+  auto NewAttr = AttributedType::getNullabilityAttrKind(NewKind);

 It would be better to only unwrap sugar until we hit the 
nullability-attributed type, then replace it.


Comment at: test/Sema/nullability.c:137
@@ +136,3 @@
+
+  int * _Nonnull p2 = p0 ?: p1; // no warnings here.
+}

You really need much more testing coverage here, e.g., for ternary operators 
where the types on the second and third arguments are different types (say, 
superclass/subclass pointer), the nullability is on either argument, etc. The 
ternary operator, especially in C++, has a ton of different cases that you need 
to look at.


https://reviews.llvm.org/D22392



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22392: [Sema] Fix an invalid nullability warning for binary conditional operators

2016-07-14 Thread Akira Hatanaka via cfe-commits 
ahatanak updated this revision to Diff 64077.
ahatanak added a comment.

Remove unused variable.


https://reviews.llvm.org/D22392

Files:
  lib/Sema/SemaExpr.cpp
  test/Sema/nullability.c

Index: test/Sema/nullability.c
===
--- test/Sema/nullability.c
+++ test/Sema/nullability.c
@@ -128,3 +128,11 @@
 
   accepts_nonnull_1(ptr); // expected-warning{{implicit conversion from 
nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * 
_Nonnull'}}
 }
+
+// Check nullability of binary conditional expressions.
+void modify_nullability(int c) {
+  int * _Nullable p0;
+  int * _Nonnull p1;
+
+  int * _Nonnull p2 = p0 ?: p1; // no warnings here.
+}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -7002,6 +7002,36 @@
 SourceRange(CondRHS->getLocStart(), RHSExpr->getLocEnd()));
 }
 
+/// Modify the nullability kind of the result type of a binary conditional
+/// expression.
+static QualType modifyNullability(QualType ResTy, Expr *RHSExpr,
+  ASTContext &Ctx) {
+  if (!ResTy->isPointerType())
+return ResTy;
+
+  auto GetNullability = [&Ctx](QualType Ty) {
+Optional Kind = Ty->getNullability(Ctx);
+if (Kind)
+  return *Kind;
+return NullabilityKind::Unspecified;
+  };
+
+  // Change the result type only if the conditional expression is nullable and
+  // the RHS is nonnull.
+  if (GetNullability(ResTy) != NullabilityKind::Nullable)
+return ResTy;
+
+  NullabilityKind NewKind = GetNullability(RHSExpr->getType());
+
+  if (NewKind != NullabilityKind::NonNull)
+return ResTy;
+
+  // Create a new AttributedType with the new nullability kind.
+  QualType NewTy = ResTy.getDesugaredType(Ctx);
+  auto NewAttr = AttributedType::getNullabilityAttrKind(NewKind);
+  return Ctx.getAttributedType(NewAttr, NewTy, NewTy);
+}
+
 /// ActOnConditionalOp - Parse a ?: operation.  Note that 'LHS' may be null
 /// in the case of a the GNU conditional expr extension.
 ExprResult Sema::ActOnConditionalOp(SourceLocation QuestionLoc,
@@ -7088,6 +7118,7 @@
 ConditionalOperator(Cond.get(), QuestionLoc, LHS.get(), ColonLoc,
 RHS.get(), result, VK, OK);
 
+  result = modifyNullability(result, RHSExpr, Context);
   return new (Context) BinaryConditionalOperator(
   commonExpr, opaqueValue, Cond.get(), LHS.get(), RHS.get(), QuestionLoc,
   ColonLoc, result, VK, OK);


Index: test/Sema/nullability.c
===
--- test/Sema/nullability.c
+++ test/Sema/nullability.c
@@ -128,3 +128,11 @@
 
   accepts_nonnull_1(ptr); // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
 }
+
+// Check nullability of binary conditional expressions.
+void modify_nullability(int c) {
+  int * _Nullable p0;
+  int * _Nonnull p1;
+
+  int * _Nonnull p2 = p0 ?: p1; // no warnings here.
+}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -7002,6 +7002,36 @@
 SourceRange(CondRHS->getLocStart(), RHSExpr->getLocEnd()));
 }
 
+/// Modify the nullability kind of the result type of a binary conditional
+/// expression.
+static QualType modifyNullability(QualType ResTy, Expr *RHSExpr,
+  ASTContext &Ctx) {
+  if (!ResTy->isPointerType())
+return ResTy;
+
+  auto GetNullability = [&Ctx](QualType Ty) {
+Optional Kind = Ty->getNullability(Ctx);
+if (Kind)
+  return *Kind;
+return NullabilityKind::Unspecified;
+  };
+
+  // Change the result type only if the conditional expression is nullable and
+  // the RHS is nonnull.
+  if (GetNullability(ResTy) != NullabilityKind::Nullable)
+return ResTy;
+
+  NullabilityKind NewKind = GetNullability(RHSExpr->getType());
+
+  if (NewKind != NullabilityKind::NonNull)
+return ResTy;
+
+  // Create a new AttributedType with the new nullability kind.
+  QualType NewTy = ResTy.getDesugaredType(Ctx);
+  auto NewAttr = AttributedType::getNullabilityAttrKind(NewKind);
+  return Ctx.getAttributedType(NewAttr, NewTy, NewTy);
+}
+
 /// ActOnConditionalOp - Parse a ?: operation.  Note that 'LHS' may be null
 /// in the case of a the GNU conditional expr extension.
 ExprResult Sema::ActOnConditionalOp(SourceLocation QuestionLoc,
@@ -7088,6 +7118,7 @@
 ConditionalOperator(Cond.get(), QuestionLoc, LHS.get(), ColonLoc,
 RHS.get(), result, VK, OK);
 
+  result = modifyNullability(result, RHSExpr, Context);
   return new (Context) BinaryConditionalOperator(
   commonExpr, opaqueValue, Cond.get(), LHS.get(), RHS.get(), QuestionLoc,
   ColonLoc, result, VK, OK);
___
cfe-commits mailing list
cfe-commits@list

[PATCH] D22392: [Sema] Fix an invalid nullability warning for binary conditional operators

2016-07-14 Thread Akira Hatanaka via cfe-commits 
ahatanak created this revision.
ahatanak added a reviewer: doug.gregor.
ahatanak added a subscriber: cfe-commits.

Currently clang issues a warning when the following code using a shorthand form 
of the conditional operator is compiled:

$ cat test1.c
```
void foo() {
  int * _Nullable p0;
  int * _Nonnull p1;
  int * _Nonnull p2 = p0 ?: p1;
}
```
test.c:4:23: warning: implicit conversion from nullable pointer 'int * 
_Nullable' to non-nullable pointer
  type 'int * _Nonnull' [-Wnullable-to-nonnull-conversion]
  int * _Nonnull p2 = p0 ?: p1; 

This happens because sema uses the type of the first operand (p0's type, which 
is "int * _Nullable") for the type of the binary condition expression and then 
complains because a nullable pointer is being assigned to a nonnull pointer. 
The warning is not valid since the expression "p0 ?: p1" cannot be null if p1 
is nonnull.

This patch fixes the bug by propagating the nullability of the last operand to 
the binary conditional expression.

https://reviews.llvm.org/D22392

Files:
  lib/Sema/SemaExpr.cpp
  test/Sema/nullability.c

Index: test/Sema/nullability.c
===
--- test/Sema/nullability.c
+++ test/Sema/nullability.c
@@ -128,3 +128,11 @@
 
   accepts_nonnull_1(ptr); // expected-warning{{implicit conversion from 
nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * 
_Nonnull'}}
 }
+
+// Check nullability of binary conditional expressions.
+void modify_nullability(int c) {
+  int * _Nullable p0;
+  int * _Nonnull p1;
+
+  int * _Nonnull p2 = p0 ?: p1; // no warnings here.
+}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -7002,6 +7002,38 @@
 SourceRange(CondRHS->getLocStart(), RHSExpr->getLocEnd()));
 }
 
+/// Modify the nullability kind of the result type of a binary conditional
+/// expression.
+static QualType modifyNullability(QualType ResTy, Expr *RHSExpr,
+  ASTContext &Ctx) {
+  if (!ResTy->isPointerType())
+return ResTy;
+
+  auto GetNullability = [&Ctx](QualType Ty) {
+Optional Kind = Ty->getNullability(Ctx);
+if (Kind)
+  return *Kind;
+return NullabilityKind::Unspecified;
+  };
+
+  // Change the result type only if the conditional expression is nullable and
+  // the RHS is nonnull.
+  NullabilityKind ResKind = GetNullability(ResTy);
+
+  if (GetNullability(ResTy) != NullabilityKind::Nullable)
+return ResTy;
+
+  NullabilityKind NewKind = GetNullability(RHSExpr->getType());
+
+  if (NewKind != NullabilityKind::NonNull)
+return ResTy;
+
+  // Create a new AttributedType with the new nullability kind.
+  QualType NewTy = ResTy.getDesugaredType(Ctx);
+  auto NewAttr = AttributedType::getNullabilityAttrKind(NewKind);
+  return Ctx.getAttributedType(NewAttr, NewTy, NewTy);
+}
+
 /// ActOnConditionalOp - Parse a ?: operation.  Note that 'LHS' may be null
 /// in the case of a the GNU conditional expr extension.
 ExprResult Sema::ActOnConditionalOp(SourceLocation QuestionLoc,
@@ -7088,6 +7120,7 @@
 ConditionalOperator(Cond.get(), QuestionLoc, LHS.get(), ColonLoc,
 RHS.get(), result, VK, OK);
 
+  result = modifyNullability(result, RHSExpr, Context);
   return new (Context) BinaryConditionalOperator(
   commonExpr, opaqueValue, Cond.get(), LHS.get(), RHS.get(), QuestionLoc,
   ColonLoc, result, VK, OK);


Index: test/Sema/nullability.c
===
--- test/Sema/nullability.c
+++ test/Sema/nullability.c
@@ -128,3 +128,11 @@
 
   accepts_nonnull_1(ptr); // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
 }
+
+// Check nullability of binary conditional expressions.
+void modify_nullability(int c) {
+  int * _Nullable p0;
+  int * _Nonnull p1;
+
+  int * _Nonnull p2 = p0 ?: p1; // no warnings here.
+}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -7002,6 +7002,38 @@
 SourceRange(CondRHS->getLocStart(), RHSExpr->getLocEnd()));
 }
 
+/// Modify the nullability kind of the result type of a binary conditional
+/// expression.
+static QualType modifyNullability(QualType ResTy, Expr *RHSExpr,
+  ASTContext &Ctx) {
+  if (!ResTy->isPointerType())
+return ResTy;
+
+  auto GetNullability = [&Ctx](QualType Ty) {
+Optional Kind = Ty->getNullability(Ctx);
+if (Kind)
+  return *Kind;
+return NullabilityKind::Unspecified;
+  };
+
+  // Change the result type only if the conditional expression is nullable and
+  // the RHS is nonnull.
+  NullabilityKind ResKind = GetNullability(ResTy);
+
+  if (GetNullability(ResTy) != NullabilityKind::Nullable)
+return ResTy;
+
+  NullabilityKind NewKind = Ge