[clang-tidy] fix misc-const-correctnes false-positive for fold expressions

[5chmidti]

The check no longer emits a diagnostic for non-parameter-pack
variables in C++17 fold expressions.
The operator used is type-dependent because of the parameter pack
and can therefore not be guaranteed to not mutate the variable.

Fixes: #70323

[llvmbot]

@llvm/pr-subscribers-clang-analysis
@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang

Author: Julian Schmidt (5chmidti)

Changes

The check no longer emits a diagnostic for variables used as the
initializer of C++17 fold expressions.
The operator used is type-dependent because of the parameter pack
and can therefore not be guaranteed to not mutate the variable.

Fixes: #70323

---

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

4 Files Affected:

• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+3-7)
• (modified) clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp (+17)
• (modified) clang/lib/Analysis/ExprMutationAnalyzer.cpp (+4)
• (modified) clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp (+15)

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index d77267588db9158..c975b99c5786bef 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -382,7 +382,8 @@ Changes in existing checks
   using pointer to member function. Additionally, the check no longer emits
   a diagnostic when a variable that is not type-dependent is an operand of a
   type-dependent binary operator. Improved performance of the check through
-  optimizations.
+  optimizations. The check no longer emits a diagnostic for
+  variables used as the initializer of C++17 fold expressions.
 
 - Improved :doc:`misc-include-cleaner
   <clang-tidy/checks/misc/include-cleaner>` check by adding option
@@ -502,12 +503,7 @@ Changes in existing checks
   <clang-tidy/checks/readability/implicit-bool-conversion>` check to take
   do-while loops into account for the `AllowIntegerConditions` and
   `AllowPointerConditions` options. It also now provides more consistent
-  suggestions when parentheses are added to the return value or expressions. 
-  It also ignores false-positives for comparison containing bool bitfield.
-
-- Improved :doc:`readability-misleading-indentation
-  <clang-tidy/checks/readability/misleading-indentation>` check to ignore
-  false-positives for line started with empty macro.
+  suggestions when parentheses are added to the return value.
 
 - Improved :doc:`readability-non-const-parameter
   <clang-tidy/checks/readability/non-const-parameter>` check to ignore
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp
index 794578ceeeba8fc..a4be41d20eae131 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp
@@ -30,3 +30,20 @@ namespace gh57297{
 struct Stream { };
 template <typename T> void f() { T t; Stream x; x << t; }
 } // namespace gh57297
+
+namespace gh70323{
+// A fold expression may contain the checked variable as it's initializer.
+// We don't know if the operator modifies that variable because the
+// operator is type dependent due to the parameter pack.
+
+struct Stream {};
+template <typename T>
+Stream& operator<<(Stream&, T);
+template <typename... Args>
+void concatenate(Args... args)
+{
+    Stream stream;
+    (stream << ... << args);
+    (args << ... << stream);
+}
+} // namespace gh70323
diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
index 9af818be0415f39..bb042760d297a78 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -343,6 +343,10 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
       // in different instantiations of the template.
       binaryOperator(isTypeDependent(),
                      hasEitherOperand(ignoringImpCasts(canResolveToExpr(Exp)))),
+      // A fold expression may contain `Exp` as it's initializer.
+      // We don't know if the operator modifies `Exp` because the
+      // operator is type dependent due to the parameter pack.
+      cxxFoldExpr(hasFoldInit(ignoringImpCasts(canResolveToExpr(Exp)))),
       // Within class templates and member functions the member expression might
       // not be resolved. In that case, the `callExpr` is considered to be a
       // modification.
diff --git a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
index a94f857720b0357..50d0399ed4b0159 100644
--- a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -359,6 +359,21 @@ TEST(ExprMutationAnalyzerTest, DependentOperatorWithNonDependentOperand) {
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x << t"));
 }
 
+TEST(ExprMutationAnalyzerTest, FoldExpression) {
+  // gh70323
+  // A fold expression may contain `Exp` as it's initializer.
+  // We don't know if the operator modifies `Exp` because the
+  // operator is type dependent due to the parameter pack.
+  const auto AST = buildASTFromCode(
+      "struct Stream {};"
+      "template <typename T> Stream& operator<<(Stream&, T); "
+      "template <typename... Args> void concatenate(Args... args) "
+      "{ Stream x; (x << ... << args); }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("(x << ... << args)"));
+}
+
 // Section: expression as call argument
 
 TEST(ExprMutationAnalyzerTest, ByValueArgument) {

[PiotrZSL]

LGTM, but i'm not 100% sure about ExprMutationAnalyzer part (if thats a right place).
Leave it open for few days, so others could comment if they have objections.

[HerrCai0907]

I am not sure "initializer of C++17 fold expressions" is the correct description for InitExpr in fold expression since I don't find it in https://eel.is/c++draft/expr.prim.fold and https://en.cppreference.com/w/cpp/language/fold.

[5chmidti]

You are right, what do you think about The check no longer emits a diagnostic for non-parameter-pack variables in C++17 fold expressions.?

[5chmidti]

Done

[5chmidti]

I fixed the clang-tidy test case by separating them and I have added a test case to check that using a variable in the pack side of the fold expression would be detected (that did not change with this patch though, just as a sanity check).

[5chmidti]

@PiotrZSL is this a reconfirmation of your previous approval (given the changed tests and release note), or is this an approval for everything?
Specifically

LGTM, but i'm not 100% sure about ExprMutationAnalyzer part (if thats a right place).
Leave it open for few days, so others could comment if they have objections.

(I think it is the right place, just under the binaryOperator which is incredibly similar to the case detected by the cxxFoldExpr matcher)

[PiotrZSL]

@5chmidti I would say: fell free to merge it. It's not that it would break too much. In worst case we got around ~1 month to fix it on a branch (if someone would complain).