Revert "[clang analysis] ExprMutationAnalyzer avoid infinite recursion for recursive forwarding reference"

[fmayer]

Reverts #87954

Broke sanitizer bots, e.g. https://lab.llvm.org/buildbot/#/builders/239/builds/6587/steps/10/logs/stdio

[llvmbot]

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

@llvm/pr-subscribers-clang-tools-extra

Author: Florian Mayer (fmayer)

Changes

Reverts llvm/llvm-project#87954

Broke sanitizer bots, e.g. https://lab.llvm.org/buildbot/#/builders/239/builds/6587/steps/10/logs/stdio

---

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

5 Files Affected:

• (modified) clang-tools-extra/docs/ReleaseNotes.rst (-4)
• (modified) clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp (-15)
• (modified) clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h (+7-21)
• (modified) clang/lib/Analysis/ExprMutationAnalyzer.cpp (+8-14)
• (modified) clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp (-30)

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 7095c564444fe6..4dfbd8ca49ab9b 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -221,10 +221,6 @@ Changes in existing checks
   <clang-tidy/checks/llvm/header-guard>` check by replacing the local
   option `HeaderFileExtensions` by the global option of the same name.
 
-- Improved :doc:`misc-const-correctness
-  <clang-tidy/checks/misc/const-correctness>` check by avoiding infinite recursion
-  for recursive forwarding reference.
-
 - Improved :doc:`misc-definitions-in-headers
   <clang-tidy/checks/misc/definitions-in-headers>` check by replacing the local
   option `HeaderFileExtensions` by the global option of the same name.
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 248374a71dd40b..9da468128743e9 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
@@ -58,18 +58,3 @@ void concatenate3(Args... args)
     (..., (stream << args));
 }
 } // namespace gh70323
-
-namespace gh60895 {
-
-template <class T> void f1(T &&a);
-template <class T> void f2(T &&a);
-template <class T> void f1(T &&a) { f2<T>(a); }
-template <class T> void f2(T &&a) { f1<T>(a); }
-void f() {
-  int x = 0;
-  // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'x' of type 'int' can be declared 'const'
-  // CHECK-FIXES: int const x = 0;
-  f1(x);
-}
-
-} // namespace gh60895
diff --git a/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h b/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
index c4e5d0badb8e58..1ceef944fbc34e 100644
--- a/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
+++ b/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
@@ -8,10 +8,11 @@
 #ifndef LLVM_CLANG_ANALYSIS_ANALYSES_EXPRMUTATIONANALYZER_H
 #define LLVM_CLANG_ANALYSIS_ANALYSES_EXPRMUTATIONANALYZER_H
 
+#include <type_traits>
+
 #include "clang/AST/AST.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "llvm/ADT/DenseMap.h"
-#include <variant>
 
 namespace clang {
 
@@ -21,15 +22,8 @@ class FunctionParmMutationAnalyzer;
 /// a given statement.
 class ExprMutationAnalyzer {
 public:
-  friend class FunctionParmMutationAnalyzer;
-  struct Cache {
-    llvm::SmallDenseMap<const FunctionDecl *,
-                        std::unique_ptr<FunctionParmMutationAnalyzer>>
-        FuncParmAnalyzer;
-  };
-
   ExprMutationAnalyzer(const Stmt &Stm, ASTContext &Context)
-      : ExprMutationAnalyzer(Stm, Context, std::make_shared<Cache>()) {}
+      : Stm(Stm), Context(Context) {}
 
   bool isMutated(const Expr *Exp) { return findMutation(Exp) != nullptr; }
   bool isMutated(const Decl *Dec) { return findMutation(Dec) != nullptr; }
@@ -51,11 +45,6 @@ class ExprMutationAnalyzer {
   using MutationFinder = const Stmt *(ExprMutationAnalyzer::*)(const Expr *);
   using ResultMap = llvm::DenseMap<const Expr *, const Stmt *>;
 
-  ExprMutationAnalyzer(const Stmt &Stm, ASTContext &Context,
-                       std::shared_ptr<Cache> CrossAnalysisCache)
-      : Stm(Stm), Context(Context),
-        CrossAnalysisCache(std::move(CrossAnalysisCache)) {}
-
   const Stmt *findMutationMemoized(const Expr *Exp,
                                    llvm::ArrayRef<MutationFinder> Finders,
                                    ResultMap &MemoizedResults);
@@ -80,7 +69,9 @@ class ExprMutationAnalyzer {
 
   const Stmt &Stm;
   ASTContext &Context;
-  std::shared_ptr<Cache> CrossAnalysisCache;
+  llvm::DenseMap<const FunctionDecl *,
+                 std::unique_ptr<FunctionParmMutationAnalyzer>>
+      FuncParmAnalyzer;
   ResultMap Results;
   ResultMap PointeeResults;
 };
@@ -89,12 +80,7 @@ class ExprMutationAnalyzer {
 // params.
 class FunctionParmMutationAnalyzer {
 public:
-  FunctionParmMutationAnalyzer(const FunctionDecl &Func, ASTContext &Context)
-      : FunctionParmMutationAnalyzer(
-            Func, Context, std::make_shared<ExprMutationAnalyzer::Cache>()) {}
-  FunctionParmMutationAnalyzer(
-      const FunctionDecl &Func, ASTContext &Context,
-      std::shared_ptr<ExprMutationAnalyzer::Cache> CrossAnalysisCache);
+  FunctionParmMutationAnalyzer(const FunctionDecl &Func, ASTContext &Context);
 
   bool isMutated(const ParmVarDecl *Parm) {
     return findMutation(Parm) != nullptr;
diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
index 503ca4bea99e50..bb042760d297a7 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -638,10 +638,9 @@ const Stmt *ExprMutationAnalyzer::findFunctionArgMutation(const Expr *Exp) {
       if (!RefType->getPointeeType().getQualifiers() &&
           RefType->getPointeeType()->getAs<TemplateTypeParmType>()) {
         std::unique_ptr<FunctionParmMutationAnalyzer> &Analyzer =
-            CrossAnalysisCache->FuncParmAnalyzer[Func];
+            FuncParmAnalyzer[Func];
         if (!Analyzer)
-          Analyzer.reset(new FunctionParmMutationAnalyzer(*Func, Context,
-                                                          CrossAnalysisCache));
+          Analyzer.reset(new FunctionParmMutationAnalyzer(*Func, Context));
         if (Analyzer->findMutation(Parm))
           return Exp;
         continue;
@@ -654,15 +653,13 @@ const Stmt *ExprMutationAnalyzer::findFunctionArgMutation(const Expr *Exp) {
 }
 
 FunctionParmMutationAnalyzer::FunctionParmMutationAnalyzer(
-    const FunctionDecl &Func, ASTContext &Context,
-    std::shared_ptr<ExprMutationAnalyzer::Cache> CrossAnalysisCache)
-    : BodyAnalyzer(*Func.getBody(), Context, CrossAnalysisCache) {
+    const FunctionDecl &Func, ASTContext &Context)
+    : BodyAnalyzer(*Func.getBody(), Context) {
   if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(&Func)) {
     // CXXCtorInitializer might also mutate Param but they're not part of
     // function body, check them eagerly here since they're typically trivial.
     for (const CXXCtorInitializer *Init : Ctor->inits()) {
-      ExprMutationAnalyzer InitAnalyzer(*Init->getInit(), Context,
-                                        CrossAnalysisCache);
+      ExprMutationAnalyzer InitAnalyzer(*Init->getInit(), Context);
       for (const ParmVarDecl *Parm : Ctor->parameters()) {
         if (Results.contains(Parm))
           continue;
@@ -678,14 +675,11 @@ FunctionParmMutationAnalyzer::findMutation(const ParmVarDecl *Parm) {
   const auto Memoized = Results.find(Parm);
   if (Memoized != Results.end())
     return Memoized->second;
-  // To handle call A -> call B -> call A. Assume parameters of A is not mutated
-  // before analyzing parameters of A. Then when analyzing the second "call A",
-  // FunctionParmMutationAnalyzer can use this memoized value to avoid infinite
-  // recursion.
-  Results[Parm] = nullptr;
+
   if (const Stmt *S = BodyAnalyzer.findMutation(Parm))
     return Results[Parm] = S;
-  return Results[Parm];
+
+  return Results[Parm] = nullptr;
 }
 
 } // namespace clang
diff --git a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
index 9c1dc1a76db63d..f58ce4aebcbfc8 100644
--- a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -977,36 +977,6 @@ TEST(ExprMutationAnalyzerTest, FollowFuncArgModified) {
                          "void f() { int x; g(x); }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("g(x)"));
-
-  AST = buildASTFromCode(
-      StdRemoveReference + StdForward +
-      "template <class T> void f1(T &&a);"
-      "template <class T> void f2(T &&a);"
-      "template <class T> void f1(T &&a) { f2<T>(std::forward<T>(a)); }"
-      "template <class T> void f2(T &&a) { f1<T>(std::forward<T>(a)); }"
-      "void f() { int x; f1(x); }");
-  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
-
-  AST = buildASTFromCode(
-      StdRemoveReference + StdForward +
-      "template <class T> void f1(T &&a);"
-      "template <class T> void f2(T &&a);"
-      "template <class T> void f1(T &&a) { f2<T>(std::forward<T>(a)); }"
-      "template <class T> void f2(T &&a) { f1<T>(std::forward<T>(a)); a++; }"
-      "void f() { int x; f1(x); }");
-  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("f1(x)"));
-
-  AST = buildASTFromCode(
-      StdRemoveReference + StdForward +
-      "template <class T> void f1(T &&a);"
-      "template <class T> void f2(T &&a);"
-      "template <class T> void f1(T &&a) { f2<T>(std::forward<T>(a)); a++; }"
-      "template <class T> void f2(T &&a) { f1<T>(std::forward<T>(a)); }"
-      "void f() { int x; f1(x); }");
-  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("f1(x)"));
 }
 
 TEST(ExprMutationAnalyzerTest, FollowFuncArgNotModified) {

[PiotrZSL]

Looks like cyclic-dependency between shared_ptrs... Thx.

[HerrCai0907]

Sorry, I will re-land it after fixing this bug.