Page MenuHomePhabricator

[Sema] add warning for comparisons like 'x<=y<=z'
Needs ReviewPublic

Authored by vabridgers on Aug 2 2020, 10:47 AM.

Details

Summary

This changes add a new warning named -Wcompare-op-parentheses that's
part of the -Wparentheses diagnostic group. This diagnostic produces a
warning when a pattern like 'x<=y<=z' is found. When this pattern is not
qualified by parentheses, it's equivalent to '(x<=y ? 1 : 0) <= z',
which is a different interpretation from that of ordinary mathematical
notation.

Diff Detail

Event Timeline

vabridgers created this revision.Aug 2 2020, 10:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2020, 10:47 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
vabridgers requested review of this revision.Aug 2 2020, 10:47 AM

This is an implementation in the CFE, after submitting and getting comments on https://reviews.llvm.org/D84898.

Quuxplusone added inline comments.
clang/docs/DiagnosticsReference.rst
2853

s/-no-/-op-/

9885

s/-no-/-op-/
And what's going on with all these trailing underscores? If they're important, you're missing one.

clang/include/clang/Basic/DiagnosticSemaKinds.td
6135

Why is this x<=y<=z instead of the simpler x<y<z or even x<=y<z (the "half-open range" common case)?
IMHO you should mention the name "chained comparisons" here, since I think that's what people coming from such languages will understand.

clang/test/Sema/warn-compare-op-parentheses.c
2

These are some great test cases, but I think they would benefit from being exhaustive.

int tests(int p1, int p2, int p3) {
    bool b;
    b = (p1 < p2 < p3);  // expected-warning{{comparisons like 'x<y<z' are interpreted}} expected-note{{place parentheses}}
    b = (p1 < p2 <= p3);  // expected-warning{{comparisons like 'x<y<z' are interpreted}} expected-note{{place parentheses}}
    b = (p1 < p2 > p3);  // expected-warning{{comparisons like 'x<y<z' are interpreted}} expected-note{{place parentheses}}
    b = (p1 < p2 >= p3);  // expected-warning{{comparisons like 'x<y<z' are interpreted}} expected-note{{place parentheses}}
    b = (p1 <= p2 < p3);  // expected-warning{{comparisons like 'x<y<z' are interpreted}} expected-note{{place parentheses}}
    b = (p1 <= p2 <= p3);  // expected-warning{{comparisons like 'x<y<z' are interpreted}} expected-note{{place parentheses}}

etc. etc.
I don't see any downside to being systematic here.

130

I would like to see explicit (and preferably exhaustive or at least systematic) test cases for the "no warning intended" case:

if ((p1 < p2) < p3)
if (p1 < (p2 < p3))
if (0 <= (p1 < p2))  // this should already trigger a constant-comparison-result warning, yes?

I would like to see explicit (and preferably exhaustive or at least systematic) test cases for mixed relational and equality comparisons:

if (p1 == p2 < p3)  // maybe intentional, but definitely deserving of a warning
if (p1 == p2 == p3)  // definitely buggy and deserving of a warning
if (p1 != p2 != p3)  // definitely buggy and deserving of a warning
if (p1 < p2 == p3 < p4)  // maybe intentional, but definitely deserving of a warning
if (p1 == p2 < p3 == p4)  // definitely buggy and deserving of a warning

Thank you! I strongly prefer this path forward.

clang/lib/Sema/SemaExpr.cpp
14010

Should we also suggest the fix to rewrite into what user likely intended?
(x op1 y) && (y op2 z)

Btw, this is an awesome patch! I'm looking forward to getting to use it.

Thank you for the comments @lebedev.ri and @Quuxplusone. I'll abandon the tidy approach (https://reviews.llvm.org/D84898) and work towards satisfying these review comments (and any others), driving towards acceptance. Best!

vabridgers marked 2 inline comments as done.

Address simpler issues brought up during review so far.
Tests to be refactored, and a fixit added.

Thanks for the comments. I posted an update for the simpler issues, working on refactoring the test cases and creating a fixit. Thanks for the good and actionable review comments!

clang/docs/DiagnosticsReference.rst
2853

fixed, thank you!

9885

missed that, thank you!

clang/include/clang/Basic/DiagnosticSemaKinds.td
6135

Thanks Arthur. I modeled the warning message after gcc's warning message. We found internally that while gcc detected this, clang did not. See https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#Warning-Options ...
<q>
-Wparentheses
Warn if parentheses are omitted in certain contexts, such as when there is an assignment in a context where a truth value is expected, or when operators are nested whose precedence people often get confused about.

Also warn if a comparison like x<=y<=z appears; this is equivalent to (x<=y ? 1 : 0) <= z, which is a different interpretation from that of ordinary mathematical notation.
...
</q>
While this is the gcc documentation, we can craft whatever message we see fit at this point in time :) I'll add "chained" comparison for this next update, we can tailor the message as we see fit. Thanks!

clang/lib/Sema/SemaExpr.cpp
14010

I'll work on this, post in a next update. Thank you!

clang/test/Sema/warn-compare-op-parentheses.c
2

I'll address in a future update. Thank you!

130

Great suggestions, I'll address in a future update. Thank you!

vabridgers added inline comments.Aug 2 2020, 12:57 PM
clang/test/Misc/warning-wall.c
100

oh my, I didn't intend for this to happen. I'll address.

vabridgers updated this revision to Diff 282475.Aug 2 2020, 1:27 PM

back out last unwanted changes from clang-format

vabridgers added inline comments.Aug 2 2020, 3:51 PM
clang/lib/Sema/SemaExpr.cpp
14010

Hi @lebedev.ri , the warning emits a note that says "place parentheses around the '<op>' expression to silence this warning" (see the test cases below). Is this sufficient, or are you looking for something else?

vabridgers updated this revision to Diff 282484.Aug 2 2020, 4:26 PM

refactor test cases per comment from Arthur

I believe I've addressed all comments so far. Looks like Arthur suggested some particular cases that are not currently covered, and are not covered by this change since I think addressing those issues are our of scope of my original intent. If this patch is otherwise acceptable, would the reviewers be ok accepting this patch on the condition of creating a bugzilla report to track those issues?

Quuxplusone added inline comments.Aug 2 2020, 6:40 PM
clang/lib/Sema/SemaExpr.cpp
14010

https://godbolt.org/z/d1dG1o
For the very similar situation (x & 1 == 0), Clang suggests two different fixits, and I believe Roman was suggesting that you should do the same kind of thing here.

<source>:3:16: warning: & has lower precedence than ==; == will be evaluated first [-Wparentheses]
    int y = (x & 1 == 0);
               ^~~~~~~~
<source>:3:16: note: place parentheses around the '==' expression to silence this warning
    int y = (x & 1 == 0);
               ^
                 (     )
<source>:3:16: note: place parentheses around the & expression to evaluate it first
    int y = (x & 1 == 0);
               ^
             (    )

For our situation here it would be something like

<source>:3:16: warning: chained comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z' [-Wcompare-op-parentheses]
    int w = (x < y < z);
               ^~~~~~~~
<source>:3:16: note: place parentheses around the first '<' expression to silence this warning
    int w = (x < y < z);
             ^
             (    )
<source>:3:16: note: separate the expression into two clauses to give it the mathematical meaning
    int y = (x < y < z);
               ^
             (    ) && (y    )

Watch out that you don't suggest a wrong fixit for (0 <= sideeffect() < 10), though. I seem to recall another warning that recently had a lot of trouble phrasing its fixit in a way that wouldn't invoke UB or change the behavior of the code in corner cases, but I forget the details.

Quuxplusone added inline comments.Aug 2 2020, 6:43 PM
clang/lib/Sema/SemaExpr.cpp
14113

Same precedence as what?
I think this should just be called isRelationalOperator, and I would bet money that such a classifier function already exists somewhere in the codebase.

njames93 added inline comments.Aug 3 2020, 2:34 PM
clang/lib/Sema/SemaExpr.cpp
14010

Surely you'd just need to check y for side effects before creating the fix-it.

14113

isRelationalOp and its a member function of BinaryOperator so there is no need to even access the OperatorKind.

vabridgers updated this revision to Diff 282852.Aug 4 2020, 3:16 AM

isRelationalOp

vabridgers updated this revision to Diff 282854.Aug 4 2020, 3:22 AM

fix misc test formatting

vabridgers marked 2 inline comments as done.Aug 4 2020, 3:24 AM
vabridgers added inline comments.
clang/lib/Sema/SemaExpr.cpp
14010

Still working on these, thanks

14010

still working on these, thanks

14113

Fixed, thanks

14113

Fixed, thanks

vabridgers updated this revision to Diff 283447.Aug 5 2020, 5:09 PM

added prototype fixits for review.
added additional RUN test case.
filed https://bugs.llvm.org/show_bug.cgi?id=47010 for other
warnings improvement post landing of this patch, after lgtm

I added prototype fixits per request by Roman, updated the LIT test, and added an additional RUN line (one for -Wparentheses and one for -Wcompare-op-parentheses). Also filed https://bugs.llvm.org/show_bug.cgi?id=47010 to follow up on the FIXME cases at the bottom of the LIT since they are out of scope for this change. Thanks for the feedback and comments so far, I look forward to driving this change to completion.

Also with a name like compare op parenthesis. It sounds like this would consider == and !=

clang/lib/Sema/SemaExpr.cpp
14034

You don't want to insert y but the source code for y

vabridgers added inline comments.Aug 6 2020, 2:46 AM
clang/lib/Sema/SemaExpr.cpp
14034

Gotcha, I'll address.

Quuxplusone added inline comments.Aug 6 2020, 12:08 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
6139

Both in the string and in the identifier, s/seperate/separate/.
I would also s/sides/clauses/ or s/sides/expression/ just to avoid giving too many different names to the same entity.

clang/lib/Sema/SemaExpr.cpp
14010

My understanding is that option (2) (x <= (y <= z)) is never suggested. It's reasonable not to suggest it, because it is neither "what the compiler sees, elucidated for the human reader" nor "what the programmer meant, corrected for the compiler." However, as it's not suggested, it shouldn't be documented in this comment.

14034

Please make sure to add at least one regression test to catch this (i.e. some test that tests the wording of the fixit and uses a name for the middle term that is not y). I don't know how to test a fixit, but there must be a way.

14261

FWIW, I would s/loosely/such as/.

clang/test/Misc/warning-wall.c
86

My impression is that you (Vincent) are tending toward just adding x == y == z and x <= y == z to this same -Wcompare-op-parentheses. That's also my preference (except that I wish it could be done all together in one big pull request instead of being split up and perhaps deferred). Have we conclusively established that everyone's on board for this?

An alternative (but IMHO a poor alternative) would be to have two or three more granular warnings, e.g. -Wrelational-op-parentheses and -Wequality-op-parentheses (and -Wmixed-relational-equality-op-parentheses?!). If we were to foresee that happening, then -Wcompare-op-parentheses would be the wrong name for this warning option today.

rsmith added a subscriber: rsmith.Aug 6 2020, 12:48 PM

The diagnostic we get for the case of three or more comparison operators here doesn't seem ideal. Perhaps we could do this check as part of the SemaChecking pass over the completed expression rather than doing it as we form the individual comparisons? That way we'll have the contextual information necessary to find the complete chained comparison; we'd also be able to detect the special case where the operator sequence is the operand of an &/|/^ so that we need parentheses in the fixit.

clang/docs/DiagnosticsReference.rst
1–5

Please note this :)

You can revert the changes to this file; it's auto-generated. (We only check in a version because the clang website infrastructure isn't yet able to generate it on-demand.)

clang/include/clang/Basic/DiagnosticSemaKinds.td
6135

Per our diagnostic best practices (http://clang.llvm.org/docs/InternalsManual.html#the-format-string), don't repeat the code in the diagnostic (or even an analogy of it such as 'x<=y<=z'); instead, consider underlining the chained comparison operators in the snippet. Also keep in mind that the developer will see the diagnostic and the notes together; you can use the notes to help clarify the meaning of the diagnostic:

error: chained comparison does not have its mathematical meaning
  if (a <= b < c)
         ^~   ~
note: perform two separate comparisons to compare the same operand twice
  if (a <= b < c)
             ^
             && b
note: place parentheses around the first comparison to compare against its 'bool' result and silence this warning
  if (a <= b < c)
      ^
      (     )
6137

If you're going to quote the operator here, you should prepend "first" or similar if both operators are the same.

6139

Don't say "intended". We don't know the intent.

clang/lib/Sema/SemaExpr.cpp
14008–14010

Why not also == and != here? a < b == c < d is a perfectly reasonable mathematical equation (modulo the spelling of ==), and it would seem sensible to catch that with the same diagnostic.

14031

I think we should consider omitting the fixit if the y expression is long or has side-effects.

14032–14035

The parentheses here clutter and distract from the point of the fixit (adding the && y). We usually don't need them, and in the cases where we do need them, the parens you added here aren't correct.

For example, cond1 && a + b < c < d + e && cond2 doesn't need any parens, and a | b < c < d needs parens around the entire rewritten b < c && c < d expression, not around the individual terms.

If you want the rewrite to always be correct, you should add a ( at the start, an ) at the end, and a && y before the second operator (or a y && after the first operator). If you're OK with it being wrong in the weird case of an & / | / ^ operator adjacent to the chained comparison, then no parens are needed at all. But there's no need to parenthesize the individual operands of the &&.

vabridgers updated this revision to Diff 283780.Aug 6 2020, 5:59 PM

use source from expression in fixit
s/seperate/separate/
address some chained comparison ambiguities outside of original scope of changes

Thanks for the recent comments. I just pushed a few improvements over the last patch that didn't comprehend latest comments from @rsmith and @Quuxplusone. I'll read through those carefully and address those.

Thanks for the recent comments. I just pushed a few improvements over the last patch that didn't comprehend latest comments from @rsmith and @Quuxplusone. I'll read through those carefully and address those.

Reverse-ping, thanks. I think this is a worthwhile warning.

xbolva00 added inline comments.
clang/test/Sema/warn-compare-op-parentheses.c
49

Not very user friendly warning text..