[Clang][diagnostics] Improve the diagnostics for chained comparisons #129285

AmrDeveloper · 2025-02-28T18:45:20Z

Improve the diagnostics for chained comparisons to report actual expressions and operators

Fixes #129069

llvmbot · 2025-02-28T18:45:54Z

@llvm/pr-subscribers-clang

Author: Amr Hesham (AmrDeveloper)

Changes

Improve the diagnostics for chained comparisons to report actual expressions and operators

Fixes #129069

Full diff: https://github.com/llvm/llvm-project/pull/129285.diff

9 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+2)
(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+1-1)
(modified) clang/lib/Sema/SemaExpr.cpp (+2-1)
(modified) clang/test/Sema/bool-compare.c (+2-2)
(modified) clang/test/Sema/parentheses.cpp (+4-4)
(modified) clang/test/SemaCXX/bool-compare.cpp (+2-2)
(modified) clang/test/SemaCXX/cxx2a-adl-only-template-id.cpp (+1-1)
(modified) clang/test/SemaTemplate/typo-dependent-name.cpp (+1-1)
(modified) clang/test/SemaTemplate/typo-template-name.cpp (+1-1)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 2b72143482943..a98200f9e931a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -214,6 +214,8 @@ Improvements to Clang's diagnostics
   :doc:`ThreadSafetyAnalysis` still does not perform alias analysis. The
   feature will be default-enabled with ``-Wthread-safety`` in a future release.
 
+- Improve the diagnostics for chained comparisons to report actual expressions and operators.
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index d094c075ecee2..b247a4a6e6dfe 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7157,7 +7157,7 @@ def note_precedence_conditional_first : Note<
   "place parentheses around the '?:' expression to evaluate it first">;
 
 def warn_consecutive_comparison : Warning<
-  "comparisons like 'X<=Y<=Z' don't have their mathematical meaning">,
+  "comparisons like '%0 %1 %2' don't have their mathematical meaning">,
   InGroup<Parentheses>, DefaultError;
 
 def warn_enum_constant_in_bool_context : Warning<
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 1738663327453..e417fc669314d 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -14924,7 +14924,8 @@ ExprResult Sema::CreateBuiltinBinOp(SourceLocation OpLoc,
 
     if (const auto *BI = dyn_cast<BinaryOperator>(LHSExpr);
         BI && BI->isComparisonOp())
-      Diag(OpLoc, diag::warn_consecutive_comparison);
+      Diag(OpLoc, diag::warn_consecutive_comparison)
+          << LHS.get() << BinaryOperator::getOpcodeStr(Opc) << RHS.get();
 
     break;
   case BO_EQ:
diff --git a/clang/test/Sema/bool-compare.c b/clang/test/Sema/bool-compare.c
index 861f47864ddd9..4949da1cb501f 100644
--- a/clang/test/Sema/bool-compare.c
+++ b/clang/test/Sema/bool-compare.c
@@ -85,7 +85,7 @@ void f(int x, int y, int z) {
   if ((a<y) != -1) {}// expected-warning {{comparison of constant -1 with boolean expression is always true}}
 
   if ((a<y) == z) {} // no warning
-  if (a>y<z)      {} // expected-error {{comparisons like 'X<=Y<=Z' don't have their mathematical meaning}}
+  if (a>y<z)      {} // expected-error {{comparisons like 'a > y < z' don't have their mathematical meaning}}
   if ((a<y) > z)  {} // no warning
   if((a<y)>(z<y)) {} // no warning
   if((a<y)==(z<y)){} // no warning
@@ -145,7 +145,7 @@ void f(int x, int y, int z) {
   if (-1 !=(a<y)) {} // expected-warning {{comparison of constant -1 with boolean expression is always true}}
 
   if (z ==(a<y))  {}    // no warning
-  if (z<a>y)      {} // expected-error {{comparisons like 'X<=Y<=Z' don't have their mathematical meaning}}
+  if (z<a>y)      {} // expected-error {{comparisons like 'z < a > y' don't have their mathematical meaning}}
   if (z > (a<y))  {}    // no warning
   if((z<y)>(a<y)) {}   // no warning
   if((z<y)==(a<y)){}  // no warning
diff --git a/clang/test/Sema/parentheses.cpp b/clang/test/Sema/parentheses.cpp
index 5424704ea01d5..beb10a4c281ab 100644
--- a/clang/test/Sema/parentheses.cpp
+++ b/clang/test/Sema/parentheses.cpp
@@ -217,10 +217,10 @@ namespace PR20735 {
 }
 
 void consecutive_builtin_compare(int x, int y, int z) {
-  (void)(x < y < z);  // expected-warning {{comparisons like 'X<=Y<=Z' don't have their mathematical meaning}}
-  (void)(x < y > z);  // expected-warning {{comparisons like 'X<=Y<=Z' don't have their mathematical meaning}}
-  (void)(x < y <= z); // expected-warning {{comparisons like 'X<=Y<=Z' don't have their mathematical meaning}}
-  (void)(x <= y > z); // expected-warning {{comparisons like 'X<=Y<=Z' don't have their mathematical meaning}}
+  (void)(x < y < z);  // expected-warning {{comparisons like 'x < y < z' don't have their mathematical meaning}}
+  (void)(x < y > z);  // expected-warning {{comparisons like 'x < y > z' don't have their mathematical meaning}}
+  (void)(x < y <= z); // expected-warning {{comparisons like 'x < y <= z' don't have their mathematical meaning}}
+  (void)(x <= y > z); // expected-warning {{comparisons like 'x <= y > z' don't have their mathematical meaning}}
   (void)((x < y) < z);  // no-warning
   (void)((x < y) >= z); // no-warning
 
diff --git a/clang/test/SemaCXX/bool-compare.cpp b/clang/test/SemaCXX/bool-compare.cpp
index 077d55ff9367d..0de8b3a994508 100644
--- a/clang/test/SemaCXX/bool-compare.cpp
+++ b/clang/test/SemaCXX/bool-compare.cpp
@@ -98,7 +98,7 @@ void f(int x, int y, int z) {
   if ((a<y) != -1) {}// expected-warning {{comparison of constant -1 with expression of type 'bool' is always true}}
 
   if ((a<y) == z) {} // no warning
-  if (a>y<z)      {} // expected-error {{comparisons like 'X<=Y<=Z' don't have their mathematical meaning}}
+  if (a>y<z)      {} // expected-error {{comparisons like 'a > y < z' don't have their mathematical meaning}}
   if ((a<y) > z)  {} // no warning
   if((a<y)>(z<y)) {} // no warning
   if((a<y)==(z<y)){} // no warning
@@ -159,7 +159,7 @@ void f(int x, int y, int z) {
   if (-1 !=(a<y)) {} // expected-warning {{comparison of constant -1 with expression of type 'bool' is always true}}
 
   if (z ==(a<y))  {} // no warning
-  if (z<a>y)      {} // expected-error {{comparisons like 'X<=Y<=Z' don't have their mathematical meaning}}
+  if (z<a>y)      {} // expected-error {{comparisons like 'z < a > y' don't have their mathematical meaning}}
   if (z > (a<y))  {} // no warning
   if((z<y)>(a<y)) {} // no warning
   if((z<y)==(a<y)){} // no warning
diff --git a/clang/test/SemaCXX/cxx2a-adl-only-template-id.cpp b/clang/test/SemaCXX/cxx2a-adl-only-template-id.cpp
index fb840e1036707..3b7ea10cdf922 100644
--- a/clang/test/SemaCXX/cxx2a-adl-only-template-id.cpp
+++ b/clang/test/SemaCXX/cxx2a-adl-only-template-id.cpp
@@ -27,7 +27,7 @@ int e = h<0>(q); // ok, found by unqualified lookup
 void fn() {
   f<0>(q);
   int f;
-  f<0>(q); // expected-error {{invalid operands to binary expression}} // expected-error {{comparisons like 'X<=Y<=Z' don't have their mathematical meaning}}
+  f<0>(q); // expected-error {{invalid operands to binary expression}} // expected-error {{comparisons like 'f < 0 > (q)' don't have their mathematical meaning}}
 }
 
 void disambig() {
diff --git a/clang/test/SemaTemplate/typo-dependent-name.cpp b/clang/test/SemaTemplate/typo-dependent-name.cpp
index 1812525ec5d7b..3d94245af8388 100644
--- a/clang/test/SemaTemplate/typo-dependent-name.cpp
+++ b/clang/test/SemaTemplate/typo-dependent-name.cpp
@@ -20,7 +20,7 @@ struct X : Base<T> {
   bool f(T other) {
     // A pair of comparisons; 'inner' is a dependent name so can't be assumed
     // to be a template.
-    return this->inner < other > ::z; // expected-error {{comparisons like 'X<=Y<=Z' don't have their mathematical meaning}}
+    return this->inner < other > ::z; // expected-error {{comparisons like 'this->inner < other > ::z' don't have their mathematical meaning}}
   }
 };
 
diff --git a/clang/test/SemaTemplate/typo-template-name.cpp b/clang/test/SemaTemplate/typo-template-name.cpp
index c1d8aa7c8e27d..d03649736ee24 100644
--- a/clang/test/SemaTemplate/typo-template-name.cpp
+++ b/clang/test/SemaTemplate/typo-template-name.cpp
@@ -36,7 +36,7 @@ namespace InExpr {
 
     // These are valid expressions.
     foo<foo; // expected-warning {{self-comparison}}
-    foo<int()>(0); // expected-error {{comparisons like 'X<=Y<=Z' don't have their mathematical meaning}}
+    foo<int()>(0); // expected-error {{comparisons like 'foo < int() > (0)' don't have their mathematical meaning}}
     foo<int(), true>(false);
     foo<Base{}.n;
   }

erichkeane · 2025-02-28T18:54:55Z

clang/test/Sema/bool-compare.c

@@ -85,7 +85,7 @@ void f(int x, int y, int z) {
  if ((a<y) != -1) {}// expected-warning {{comparison of constant -1 with boolean expression is always true}}

  if ((a<y) == z) {} // no warning
-  if (a>y<z)      {} // expected-error {{comparisons like 'X<=Y<=Z' don't have their mathematical meaning}}
+  if (a>y<z)      {} // expected-error {{comparisons like 'a > y < z' don't have their mathematical meaning}}


Hmm.... @AaronBallman will probably have to comment here, but I'm not sure this is better? The point of comparisons like here was to simplify the diagnostics to something that is like what is there, but not actually.

This patch ends up giving us really awful warning/diagnostics in the case where these are more than simple variables, as its just dumping the entire expression. So I don't know if I like this patch.

Yes i think the word like should be removed here, but I am thinking should we prefer the previous diagnostics or the one with actual expression

I don't like the one with the actual expression in it, as that is going to get arbitrarily large. That said, I DO wonder if there is value in using the actual operators instead of just <= for both sides (so not printing the expression, just the operators, and still X, Y, Z). That would require keeping like though.

Yes, I got your point also it will not be great with large expressions :D, I am thinking should we change the message to be different or just the old one with actual operators 🤔

I feel like making it so it prints the actual operators is simple enough to where we can just do that. I agree that we probably don’t want to print the entire expression because that can get rather messy.

Further thought: I haven't seen the 'change' you're proposing to the message, but am willing to keep an open mind. But I also think just changing to the actual operators is a distinct improvement.

…to report actual expressions and operators

cor3ntin

LGTM once @erichkeane is happy

AaronBallman

Thank you for working on this! I think we can make the diagnostic message a bit more clear while we're at it, WDYT?

AaronBallman · 2025-03-05T13:00:39Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -7157,7 +7157,7 @@ def note_precedence_conditional_first : Note<
  "place parentheses around the '?:' expression to evaluate it first">;

 def warn_consecutive_comparison : Warning<
-  "comparisons like 'X<=Y<=Z' don't have their mathematical meaning">,
+  "comparisons like 'X %0 Y %1 Z' don't have their mathematical meaning">,


Suggested change

"comparisons like 'X %0 Y %1 Z' don't have their mathematical meaning">,

"chained comparison 'X %0 Y %1 Z' does not behave the same as a mathematical expression">,

AaronBallman

LGTM!

…lvm#129285) Improve the diagnostics for chained comparisons to report actual expressions and operators Fixes llvm#129069

AmrDeveloper requested review from cor3ntin, shafik and erichkeane February 28, 2025 18:45

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 28, 2025

AmrDeveloper added the clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer label Feb 28, 2025

erichkeane reviewed Feb 28, 2025

View reviewed changes

AmrDeveloper requested a review from AaronBallman February 28, 2025 19:09

AmrDeveloper added 2 commits March 3, 2025 20:26

[Clang][diagnostics] Improve the diagnostics for chained comparisons …

7a61f41

…to report actual expressions and operators

Add issue number to release note

99197b6

AmrDeveloper force-pushed the fix_diagnostics branch from 66cd32d to 99197b6 Compare March 3, 2025 19:28

cor3ntin approved these changes Mar 5, 2025

View reviewed changes

AaronBallman reviewed Mar 5, 2025

View reviewed changes

Improve the error message

b0de9a9

AaronBallman approved these changes Mar 5, 2025

View reviewed changes

AmrDeveloper merged commit 9ecb0f5 into llvm:main Mar 6, 2025
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang][diagnostics] Improve the diagnostics for chained comparisons #129285

[Clang][diagnostics] Improve the diagnostics for chained comparisons #129285

Uh oh!

AmrDeveloper commented Feb 28, 2025

Uh oh!

llvmbot commented Feb 28, 2025

Uh oh!

erichkeane Feb 28, 2025

Uh oh!

AmrDeveloper Feb 28, 2025

Uh oh!

erichkeane Feb 28, 2025

Uh oh!

AmrDeveloper Mar 1, 2025

Uh oh!

Sirraide Mar 3, 2025

Uh oh!

erichkeane Mar 3, 2025

Uh oh!

cor3ntin left a comment •

edited

Loading

Uh oh!

AaronBallman left a comment

Uh oh!

AaronBallman Mar 5, 2025

Uh oh!

AaronBallman left a comment

Uh oh!

Uh oh!

Uh oh!

	"comparisons like 'X %0 Y %1 Z' don't have their mathematical meaning">,
	"chained comparison 'X %0 Y %1 Z' does not behave the same as a mathematical expression">,

[Clang][diagnostics] Improve the diagnostics for chained comparisons #129285

[Clang][diagnostics] Improve the diagnostics for chained comparisons #129285

Uh oh!

Conversation

AmrDeveloper commented Feb 28, 2025

Uh oh!

llvmbot commented Feb 28, 2025

Uh oh!

erichkeane Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

AmrDeveloper Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

erichkeane Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

AmrDeveloper Mar 1, 2025

Choose a reason for hiding this comment

Uh oh!

Sirraide Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

erichkeane Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

cor3ntin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

AaronBallman Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cor3ntin left a comment •

edited

Loading