[Clang] omit parentheses in fold expressions with a single expansion

[a-tarasyuk]

Fixes #101863

[llvmbot]

@llvm/pr-subscribers-clang

Author: Oleksandr T. (a-tarasyuk)

Changes

Fixes #101863

---

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

4 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/lib/Sema/TreeTransform.h (+8-1)
• (modified) clang/test/SemaCXX/warn-assignment-condition.cpp (+24-15)
• (modified) clang/test/SemaTemplate/instantiate-requires-expr.cpp (+1-1)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index b7d287fdf4cc61..850a857e11abff 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -378,6 +378,8 @@ Improvements to Clang's diagnostics
 
 - Clang now emits a diagnostic note at the class declaration when the method definition does not match any declaration (#GH110638).
 
+- Clang now omits warnings for extra parentheses in fold expressions with single expansion. (#GH101863)
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 04308b2b57d8a0..6a78ecc1229b3a 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -15566,6 +15566,14 @@ TreeTransform<Derived>::TransformCXXFoldExpr(CXXFoldExpr *E) {
       return true;
   }
 
+  // When there's only one expansion, the parentheses can be safely eliminated
+  // to avoid any extra redundancy that may result in incorrect checks
+  // For example, transforming (((args == 0) || ...)) into (args == 0)
+  // allows the omission of parentheses while ensuring precise representation
+  // and avoiding warnings regarding redundant parentheses.
+  if (*NumExpansions == 1)
+    Pattern = Pattern->IgnoreParens();
+
   for (unsigned I = 0; I != *NumExpansions; ++I) {
     Sema::ArgumentPackSubstitutionIndexRAII SubstIndex(
         getSema(), LeftFold ? I : *NumExpansions - I - 1);
@@ -15623,7 +15631,6 @@ TreeTransform<Derived>::TransformCXXFoldExpr(CXXFoldExpr *E) {
   if (Result.isUnset())
     return getDerived().RebuildEmptyCXXFoldExpr(E->getEllipsisLoc(),
                                                 E->getOperator());
-
   return Result;
 }
 
diff --git a/clang/test/SemaCXX/warn-assignment-condition.cpp b/clang/test/SemaCXX/warn-assignment-condition.cpp
index 09084e36bb4916..ea169e8f9fa49c 100644
--- a/clang/test/SemaCXX/warn-assignment-condition.cpp
+++ b/clang/test/SemaCXX/warn-assignment-condition.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -Wparentheses -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wparentheses -std=c++2a -verify %s
 
 struct A {
   int foo();
@@ -122,25 +122,34 @@ void test() {
 #undef EQ
 }
 
-void (*fn)();
+namespace GH101863 {
+void t1(auto... args) {
+  if (((args == 0) or ...)) { }
+}
+
+template <typename... Args>
+void t2(Args... args) {
+    if (((args == 0) or ...)) { }
+}
+
+void t3(auto... args) {
+  if ((... && (args == 0))) { }
+}
 
-void test2() {
-    if ((fn == test2)) {} // expected-warning {{equality comparison with extraneous parentheses}} \
-                          // expected-note {{use '=' to turn this equality comparison into an assignment}} \
-                          // expected-note {{remove extraneous parentheses around the comparison to silence this warning}}
-    if ((test2 == fn)) {}
+void t4(auto... a, auto... b) {
+  if (((a == 0) or ...) && ((b == 0) or ...)) { }
 }
 
-namespace rdar9027658 {
-template <typename T>
-void f(T t) {
-    if ((t.g == 3)) { } // expected-warning {{equality comparison with extraneous parentheses}} \
-                         // expected-note {{use '=' to turn this equality comparison into an assignment}} \
-                         // expected-note {{remove extraneous parentheses around the comparison to silence this warning}}
+void t5(auto... args) {
+  if ((((args == 0) or ...))) { }
 }
 
-struct S { int g; };
 void test() {
-  f(S()); // expected-note {{in instantiation}}
+  t1(0, 1);
+  t2<>();
+  t3(1, 2, 3);
+  t3(0, 1);
+  t4(0, 1);
+  t5(0, 1);
 }
 }
diff --git a/clang/test/SemaTemplate/instantiate-requires-expr.cpp b/clang/test/SemaTemplate/instantiate-requires-expr.cpp
index 20a19d731ae169..ce2c060a176045 100644
--- a/clang/test/SemaTemplate/instantiate-requires-expr.cpp
+++ b/clang/test/SemaTemplate/instantiate-requires-expr.cpp
@@ -36,7 +36,7 @@ using r1i2 = r1<char>; // expected-error {{constraints not satisfied for class t
 template<typename... Ts> requires
 false_v<requires (Ts... ts) {requires ((sizeof(ts) == 2) && ...);}>
 // expected-note@-1 {{because 'false_v<requires (short ts, unsigned short ts) { requires (sizeof (ts) == 2) && (sizeof (ts) == 2); }>'}}
-// expected-note@-2 {{because 'false_v<requires (short ts) { requires (sizeof (ts) == 2); }>' evaluated to false}}
+// expected-note@-2 {{because 'false_v<requires (short ts) { requires sizeof (ts) == 2; }>' evaluated to false}}
 struct r2 {};
 
 using r2i1 = r2<short, unsigned short>; // expected-error {{constraints not satisfied for class template 'r2' [with Ts = <short, unsigned short>]}}

[erichkeane]

The test changes are weird, we want to leave all the existing tests in place, because we want this diagnostic to happen.

Also, I'd love it if someone else (@cor3ntin ?) had a think on this as well.

[cor3ntin]

I am happy with the direction, but I think we need changes to ASTStmtReader::VisitParenExpr/ASTStmtWriter::VisitParenExpr

(also, I want to make sure Erich is happy too)