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

[ahatanak]

Summary

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.

[ahatanak]

Remove unused variable.

[doug.gregor]

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.

lib/Sema/SemaExpr.cpp

7007 ↗

(On Diff #64077)

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.

7024 ↗

(On Diff #64077)

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.

7030 ↗

(On Diff #64077)

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

test/Sema/nullability.c

137 ↗

(On Diff #64077)

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.

[ahatanak]

lib/Sema/SemaExpr.cpp

7007 ↗

(On Diff #64077)

I'm thinking about renaming it to computeConditionalNullability or something, but I'm open to suggestions.

7024 ↗

(On Diff #64077)

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.

7030 ↗

(On Diff #64077)

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;

test/Sema/nullability.c

137 ↗

(On Diff #64077)

Yes, I'll have more test cases in my next patch.

[ahatanak]

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

[ahatanak]

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.

[ahatanak]

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

[doug.gregor]

LGTM!

cfe/trunk/lib/Sema/SemaExpr.cpp

Self.PDiag(diag::note_precedence_silence)
       << BinaryOperator::getOpcodeStr(CondOpcode),
     SourceRange(Condition->getLocStart(), Condition->getLocEnd()));
 
   SuggestParentheses(Self, OpLoc,
     Self.PDiag(diag::note_precedence_conditional_first),
     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<NullabilityKind> 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,
                                     SourceLocation ColonLoc,
                                     Expr *CondExpr, Expr *LHSExpr,
                                     Expr *RHSExpr) {
   if (!getLangOpts().CPlusPlus) {
     // C cannot handle TypoExpr nodes in the condition because it
if (!LHSExpr) {
     opaqueValue = new (Context) OpaqueValueExpr(commonExpr->getExprLoc(),
                                                 commonExpr->getType(),
                                                 commonExpr->getValueKind(),
                                                 commonExpr->getObjectKind(),
                                                 commonExpr);
     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;
   QualType result = CheckConditionalOperands(Cond, LHS, RHS, 
                                              VK, OK, QuestionLoc);
   if (result.isNull() || Cond.isInvalid() || LHS.isInvalid() ||
       RHS.isInvalid())
     return ExprError();
 
   DiagnoseConditionalPrecedence(*this, QuestionLoc, Cond.get(), LHS.get(),
                                 RHS.get());
 
   CheckBoolLikeConversion(Cond.get(), QuestionLoc);
 
+  result = computeConditionalNullability(result, commonExpr, LHSTy, RHSTy,
+                                         Context);
+
   if (!commonExpr)
     return new (Context)
         ConditionalOperator(Cond.get(), QuestionLoc, LHS.get(), ColonLoc,
                             RHS.get(), result, VK, OK);
 
   return new (Context) BinaryConditionalOperator(
       commonExpr, opaqueValue, Cond.get(), LHS.get(), RHS.get(), QuestionLoc,
       ColonLoc, result, VK, OK);

cfe/trunk/test/Sema/nullability.c

 // Check nullable-to-nonnull conversions.
 void nullable_to_nonnull(_Nullable int *ptr) {
   int *a = ptr; // okay
   _Nonnull int *b = ptr; // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
   b = ptr; // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
 
   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;
+  int * _Nullable nullableP;
+  int * _Null_unspecified unspecifiedP;
+  int *noneP;
+
+  p = nonnullP ?: nonnullP;
+  p = nonnullP ?: nullableP;
+  p = nonnullP ?: unspecifiedP;
+  p = nonnullP ?: noneP;
+  p = nullableP ?: nonnullP;
+  p = nullableP ?: nullableP; // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
+  p = nullableP ?: unspecifiedP;
+  p = nullableP ?: noneP;
+  p = unspecifiedP ?: nonnullP;
+  p = unspecifiedP ?: nullableP; // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
+  p = unspecifiedP ?: unspecifiedP;
+  p = unspecifiedP ?: noneP;
+  p = noneP ?: nonnullP;
+  p = noneP ?: nullableP; // expected-warning{{implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull'}}
+  p = noneP ?: unspecifiedP;
+  p = noneP ?: noneP;
+}

cfe/trunk/test/SemaCXX/nullability.cpp

void AssignAndInitNonNullFromFn() {
   void *_Nonnull p3 = {ReturnNullable()}; // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}
   void *_Nonnull p4 = ReturnNullable(); // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}
   void *_Nonnull nonnull;
   nonnull = ReturnNullable(); // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}
   nonnull = {ReturnNullable()}; // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}
 
   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}}
+}