Page MenuHomePhabricator

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

Authored by ahatanak on Jul 14 2016, 6:42 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak updated this revision to Diff 64073.Jul 14 2016, 6:42 PM
ahatanak retitled this revision from to [Sema] Fix an invalid nullability warning for binary conditional operators.
ahatanak updated this object.
ahatanak added a reviewer: doug.gregor.
ahatanak added a subscriber: cfe-commits.
ahatanak updated this revision to Diff 64077.Jul 14 2016, 7:16 PM

Remove unused variable.

doug.gregor edited edge metadata.Jul 14 2016, 9:26 PM

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 added inline comments.Jul 15 2016, 2:00 PM
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.

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 updated this revision to Diff 64551.Jul 19 2016, 1:19 PM
ahatanak edited edge metadata.

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 updated this revision to Diff 64557.Jul 19 2016, 1:39 PM

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

doug.gregor accepted this revision.Jul 19 2016, 3:50 PM
doug.gregor edited edge metadata.

LGTM!

This revision is now accepted and ready to land.Jul 19 2016, 3:50 PM
This revision was automatically updated to reflect the committed changes.