[Clang] Warning as error for fold expressions over comparison operators #136836

cor3ntin · 2025-04-23T09:54:37Z

We made chained comparisons an error.
Fold-expressions over a comparison operator produce chained comparisons, so we should be consistent there too.

We only emit the warning when instantiating the fold expression so as not to warn on types with user-defined comparisons.

Partially addresses #129570

We made chained comparisons an error. Fold exprerssions over a comparison operators produce chained comparison, so we should be consistent there too. We only emit the warning when instantiating the fold expression as to not warn on types with user-defined comparisons. Partially addresses llvm#129570

llvmbot · 2025-04-23T09:55:11Z

@llvm/pr-subscribers-clang

Author: cor3ntin (cor3ntin)

Changes

We made chained comparisons an error.
Fold-expressions over a comparison operator produce chained comparisons, so we should be consistent there too.

We only emit the warning when instantiating the fold expression so as not to warn on types with user-defined comparisons.

Partially addresses #129570

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

5 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+1)
(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+5)
(modified) clang/lib/Sema/TreeTransform.h (+8)
(modified) clang/test/Parser/cxx1z-fold-expressions.cpp (+3-1)
(modified) clang/test/SemaTemplate/cxx1z-fold-expressions.cpp (+23)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index f5cd1fbeabcfe..3905aef111394 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -318,6 +318,7 @@ Improvements to Clang's diagnostics
   under the subgroup ``-Wunsafe-buffer-usage-in-libc-call``.
 - Diagnostics on chained comparisons (``a < b < c``) are now an error by default. This can be disabled with
   ``-Wno-error=parentheses``.
+- Similarly, fold expressions over a comparison operator are now an error by default.
 - Clang now better preserves the sugared types of pointers to member.
 - Clang now better preserves the presence of the template keyword with dependent
   prefixes.
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c29a3422acd26..b1d261193763a 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7138,6 +7138,11 @@ def warn_consecutive_comparison : Warning<
   "chained comparison 'X %0 Y %1 Z' does not behave the same as a mathematical expression">,
   InGroup<Parentheses>, DefaultError;
 
+def warn_comparison_in_fold_expression : Warning<
+  "comparison in a fold expression would evaluate to '(X %0 Y) %0 Z' "
+  "which does not behave the same as a mathematical expression">,
+  InGroup<Parentheses>, DefaultError;
+
 def warn_enum_constant_in_bool_context : Warning<
   "converting the enum constant to a boolean">,
   InGroup<IntInBoolContext>, DefaultIgnore;
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 2469991bf2ce8..ba107da82d683 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -16411,6 +16411,7 @@ TreeTransform<Derived>::TransformCXXFoldExpr(CXXFoldExpr *E) {
       return true;
   }
 
+  bool WarnedOnComparison = false;
   for (unsigned I = 0; I != *NumExpansions; ++I) {
     Sema::ArgPackSubstIndexRAII SubstIndex(
         getSema(), LeftFold ? I : *NumExpansions - I - 1);
@@ -16439,6 +16440,13 @@ TreeTransform<Derived>::TransformCXXFoldExpr(CXXFoldExpr *E) {
       } else {
         Result = getDerived().RebuildBinaryOperator(E->getEllipsisLoc(),
                                                     E->getOperator(), LHS, RHS);
+        if(!WarnedOnComparison && Result.isUsable()) {
+            if(auto * BO = dyn_cast<BinaryOperator>(Result.get()); BO && BO->isComparisonOp()) {
+                WarnedOnComparison = true;
+                SemaRef.Diag(BO->getBeginLoc(), diag::warn_comparison_in_fold_expression)
+                        << BO->getOpcodeStr();
+            }
+        }
       }
     } else
       Result = Out;
diff --git a/clang/test/Parser/cxx1z-fold-expressions.cpp b/clang/test/Parser/cxx1z-fold-expressions.cpp
index ac27111697737..3ba9a0932ccf1 100644
--- a/clang/test/Parser/cxx1z-fold-expressions.cpp
+++ b/clang/test/Parser/cxx1z-fold-expressions.cpp
@@ -52,9 +52,11 @@ template<typename ...T> void as_operand_of_cast(int a, T ...t) {
 
 // fold-operator can be '>' or '>>'.
 template <int... N> constexpr bool greaterThan() { return (N > ...); }
+// expected-error@-1 {{comparison in a fold expression}}
+
 template <int... N> constexpr int rightShift() { return (N >> ...); }
 
-static_assert(greaterThan<2, 1>());
+static_assert(greaterThan<2, 1>()); // expected-note {{in instantiation}}
 static_assert(rightShift<10, 1>() == 5);
 
 template <auto V> constexpr auto Identity = V;
diff --git a/clang/test/SemaTemplate/cxx1z-fold-expressions.cpp b/clang/test/SemaTemplate/cxx1z-fold-expressions.cpp
index 47a252eb335f6..7dc67fe8d6bf7 100644
--- a/clang/test/SemaTemplate/cxx1z-fold-expressions.cpp
+++ b/clang/test/SemaTemplate/cxx1z-fold-expressions.cpp
@@ -132,3 +132,26 @@ bool f();
 template <typename... T>
 void g(bool = (f<T>() || ...));
 }
+
+
+namespace comparison_warning {
+  struct S {
+    bool operator<(const S&) const;
+    bool operator==(const S&) const;
+  };
+
+  template <typename...T>
+  void f(T... ts) {
+    (void)(ts == ...);
+    // expected-error@-1{{comparison in a fold expression would evaluate to '(X == Y) == Z'}}
+    (void)(ts < ...);
+    // expected-error@-1{{comparison in a fold expression would evaluate to '(X < Y) < Z'}}
+  }
+
+  void test() {
+    f(0, 1, 2); // expected-note{{in instantiation}}
+    f(S{}, S{});
+    f(0);
+  }
+
+};

github-actions · 2025-04-23T09:58:18Z

✅ With the latest revision this PR passed the C/C++ code formatter.

AaronBallman

LGTM with a minor rewording and reformatting of the diagnostic message.

AaronBallman · 2025-04-23T11:57:21Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

+def warn_comparison_in_fold_expression
+    : Warning<
+          "comparison in a fold expression would evaluate to '(X %0 Y) %0 Z' "
+          "which does not behave the same as a mathematical expression">,
+      InGroup<Parentheses>,
+      DefaultError;


Suggested change

def warn_comparison_in_fold_expression

: Warning<

"comparison in a fold expression would evaluate to '(X %0 Y) %0 Z' "

"which does not behave the same as a mathematical expression">,

InGroup<Parentheses>,

DefaultError;

def warn_comparison_in_fold_expression : Warning<

"comparison in fold expression would evaluate to '(X %0 Y) %0 Z' "

"which does not behave the same as a mathematical expression">,

InGroup<Parentheses>, DefaultError;

zyn0217

thanks

zyn0217 · 2025-04-23T12:36:09Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

  InGroup<Parentheses>, DefaultError;

+def warn_comparison_in_fold_expression : Warning<
+  "comparison in fold expression would evaluate to '(X %0 Y) %0 Z' "


It might be better to print out the expression than X, Y, Z

And I'm thinking if there's a case where it actually evaluates to X %0 (Y %0 Z)

We added that discussion for the previous PR, we concluded it was really not useful!

zyn0217 · 2025-04-23T12:39:48Z

clang/test/SemaTemplate/cxx1z-fold-expressions.cpp

+  void test() {
+    f(0, 1, 2); // expected-note{{in instantiation}}
+    f(S{}, S{});
+    f(0);


can you test f(0, 0)?

cor3ntin · 2025-04-23T15:09:10Z

https://compiler-explorer.com/z/zoze5br4d it turns out that we already produce a warning for right fold... hum, this will require more work

zyn0217 · 2025-05-01T10:22:33Z

clang/test/SemaTemplate/cxx1z-fold-expressions.cpp

+
+  void test() {
+    f(0, 1, 2); // expected-note{{in instantiation}}
+    f(0, 1); // expected-note{{in instantiation}}


Hmmm. It looks strange to me if we warn for '0 < 1'

This is intended -, ideally, we would even warn when n <= 1 (ie, the size of the pack should not affect the warning) - but that would be annoying because there could be user-defined operators, and we only discover that for n >=2.

zyn0217 · 2025-05-01T12:02:11Z

clang/lib/Sema/SemaExpr.cpp


    if (const auto *BI = dyn_cast<BinaryOperator>(LHSExpr);
-        BI && BI->isComparisonOp())
+        !ForFoldExpression && BI && BI->isComparisonOp())


The right fold cases are handled by this branch, right? So shall we rename ForFoldExpression to ForLeftFold or something?

That's just a side effect of looking at the LHS first - I don't think the distinction matters here

zyn0217

Okay, let's go ahead as is. Though I feel it's unfortunate that the warnings for left fold and right fold are scattered - maybe we could clean them up in future

Thanks for working on this.

cor3ntin · 2025-05-01T14:59:58Z

Okay, let's go ahead as is. Though I feel it's unfortunate that the warnings for left fold and right fold are scattered - maybe we could clean them up in future

This is not the case, there is a warning for the fold case (regardless of direction) in TreeTransform, and a different warning for the non-fold case in CreateBinOp

…rs (llvm#136836) We made chained comparisons an error. Fold-expressions over a comparison operator produce chained comparisons, so we should be consistent there too. We only emit the warning when instantiating the fold expression so as not to warn on types with user-defined comparisons. Partially addresses llvm#129570

cor3ntin requested a review from Sirraide April 23, 2025 09:54

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

format

7e53392

AaronBallman approved these changes Apr 23, 2025

View reviewed changes

Address aaron's feedback

7c61d91

zyn0217 reviewed Apr 23, 2025

View reviewed changes

add tests

5a2297b

add dpplicasted wordings for right folds

2b8e3b0

zyn0217 reviewed May 1, 2025

View reviewed changes

zyn0217 approved these changes May 1, 2025

View reviewed changes

cor3ntin merged commit 7ffaaf4 into llvm:main May 1, 2025
12 checks passed

cor3ntin deleted the fold_expression_comparison branch May 1, 2025 15:00

[Clang] Warning as error for fold expressions over comparison operators #136836

[Clang] Warning as error for fold expressions over comparison operators #136836

Uh oh!

Conversation

cor3ntin commented Apr 23, 2025

Uh oh!

llvmbot commented Apr 23, 2025

Uh oh!

github-actions bot commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zyn0217 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cor3ntin commented Apr 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zyn0217 left a comment

Choose a reason for hiding this comment

Uh oh!

cor3ntin commented May 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Apr 23, 2025 •

edited

Loading