[PATCH] D85097: [Sema] add warning for comparisons like 'x<=y<=z'

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

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.



================
Comment at: clang/docs/DiagnosticsReference.rst:1-5
 ..
   -------------------------------------------------------------------
   NOTE: This file is automatically generated by running clang-tblgen
   -gen-diag-docs. Do not edit this file by hand!!
   -------------------------------------------------------------------
----------------
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.)


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6137
+def note_compare_with_parens : Note<
+  "place parentheses around the '%0' comparison to silence this warning">;
+def note_compare_seperate_sides : Note<
----------------
If you're going to quote the operator here, you should prepend "first" or similar if both operators are the same.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6139
+def note_compare_seperate_sides : Note<
+  "seperate the expression into two clauses to give it the intended mathematical meaning">;
+
----------------
Quuxplusone wrote:
> 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.
Don't say "intended". We don't know the intent.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6135
+  "comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'">,
+  InGroup<CompareOpParentheses>, DefaultIgnore;
+
----------------
vabridgers wrote:
> Quuxplusone wrote:
> > 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.
> 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!
> 
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)
      ^
      (     )
```


================
Comment at: clang/lib/Sema/SemaExpr.cpp:14008-14010
+// Accepts (x ['<'|'<='|'>'|'>='] y ['<'|'<='|'>'|'>='] z), suggests:
+// 1) ((x ['<'|'<='|'>'|'>='] y) ['<'|'<='|'>'|'>='] z), or
+// 2) (x ['<'|'<='|'>'|'>='] (y ['<'|'<='|'>'|'>='] z)). or
----------------
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.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:14031
+
+  Self.Diag(LHSExpr->getBeginLoc(), diag::note_compare_seperate_sides)
+      << FixItHint::CreateInsertion(LHSExpr->getBeginLoc(), "(")
----------------
I think we should consider omitting the fixit if the `y` expression is long or has side-effects.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:14032-14035
+      << FixItHint::CreateInsertion(LHSExpr->getBeginLoc(), "(")
+      << FixItHint::CreateInsertion(LHSExpr->getEndLoc(), ") && ")
+      << FixItHint::CreateInsertion(LHSBO->getRHS()->getBeginLoc(), "(y ")
+      << FixItHint::CreateInsertion(RHSExpr->getEndLoc(), ")");
----------------
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 `&&`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85097/new/

https://reviews.llvm.org/D85097