diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 1405fb0df1f8d..de0a7b79bb2cf 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -221,6 +221,10 @@ Changes in existing checks ` check by replacing the local option `HeaderFileExtensions` by the global option of the same name. +- Improved :doc:`misc-const-correctness + ` check by avoiding infinite recursion + for recursive forwarding reference. + - Improved :doc:`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 9da468128743e..248374a71dd40 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,3 +58,18 @@ void concatenate3(Args... args) (..., (stream << args)); } } // namespace gh70323 + +namespace gh60895 { + +template void f1(T &&a); +template void f2(T &&a); +template void f1(T &&a) { f2(a); } +template void f2(T &&a) { f1(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 1ceef944fbc34..c4e5d0badb8e5 100644 --- a/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h +++ b/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h @@ -8,11 +8,10 @@ #ifndef LLVM_CLANG_ANALYSIS_ANALYSES_EXPRMUTATIONANALYZER_H #define LLVM_CLANG_ANALYSIS_ANALYSES_EXPRMUTATIONANALYZER_H -#include - #include "clang/AST/AST.h" #include "clang/ASTMatchers/ASTMatchers.h" #include "llvm/ADT/DenseMap.h" +#include namespace clang { @@ -22,8 +21,15 @@ class FunctionParmMutationAnalyzer; /// a given statement. class ExprMutationAnalyzer { public: + friend class FunctionParmMutationAnalyzer; + struct Cache { + llvm::SmallDenseMap> + FuncParmAnalyzer; + }; + ExprMutationAnalyzer(const Stmt &Stm, ASTContext &Context) - : Stm(Stm), Context(Context) {} + : ExprMutationAnalyzer(Stm, Context, std::make_shared()) {} bool isMutated(const Expr *Exp) { return findMutation(Exp) != nullptr; } bool isMutated(const Decl *Dec) { return findMutation(Dec) != nullptr; } @@ -45,6 +51,11 @@ class ExprMutationAnalyzer { using MutationFinder = const Stmt *(ExprMutationAnalyzer::*)(const Expr *); using ResultMap = llvm::DenseMap; + ExprMutationAnalyzer(const Stmt &Stm, ASTContext &Context, + std::shared_ptr CrossAnalysisCache) + : Stm(Stm), Context(Context), + CrossAnalysisCache(std::move(CrossAnalysisCache)) {} + const Stmt *findMutationMemoized(const Expr *Exp, llvm::ArrayRef Finders, ResultMap &MemoizedResults); @@ -69,9 +80,7 @@ class ExprMutationAnalyzer { const Stmt &Stm; ASTContext &Context; - llvm::DenseMap> - FuncParmAnalyzer; + std::shared_ptr CrossAnalysisCache; ResultMap Results; ResultMap PointeeResults; }; @@ -80,7 +89,12 @@ class ExprMutationAnalyzer { // params. class FunctionParmMutationAnalyzer { public: - FunctionParmMutationAnalyzer(const FunctionDecl &Func, ASTContext &Context); + FunctionParmMutationAnalyzer(const FunctionDecl &Func, ASTContext &Context) + : FunctionParmMutationAnalyzer( + Func, Context, std::make_shared()) {} + FunctionParmMutationAnalyzer( + const FunctionDecl &Func, ASTContext &Context, + std::shared_ptr CrossAnalysisCache); bool isMutated(const ParmVarDecl *Parm) { return findMutation(Parm) != nullptr; diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp index bb042760d297a..503ca4bea99e5 100644 --- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp +++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp @@ -638,9 +638,10 @@ const Stmt *ExprMutationAnalyzer::findFunctionArgMutation(const Expr *Exp) { if (!RefType->getPointeeType().getQualifiers() && RefType->getPointeeType()->getAs()) { std::unique_ptr &Analyzer = - FuncParmAnalyzer[Func]; + CrossAnalysisCache->FuncParmAnalyzer[Func]; if (!Analyzer) - Analyzer.reset(new FunctionParmMutationAnalyzer(*Func, Context)); + Analyzer.reset(new FunctionParmMutationAnalyzer(*Func, Context, + CrossAnalysisCache)); if (Analyzer->findMutation(Parm)) return Exp; continue; @@ -653,13 +654,15 @@ const Stmt *ExprMutationAnalyzer::findFunctionArgMutation(const Expr *Exp) { } FunctionParmMutationAnalyzer::FunctionParmMutationAnalyzer( - const FunctionDecl &Func, ASTContext &Context) - : BodyAnalyzer(*Func.getBody(), Context) { + const FunctionDecl &Func, ASTContext &Context, + std::shared_ptr CrossAnalysisCache) + : BodyAnalyzer(*Func.getBody(), Context, CrossAnalysisCache) { if (const auto *Ctor = dyn_cast(&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); + ExprMutationAnalyzer InitAnalyzer(*Init->getInit(), Context, + CrossAnalysisCache); for (const ParmVarDecl *Parm : Ctor->parameters()) { if (Results.contains(Parm)) continue; @@ -675,11 +678,14 @@ 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] = nullptr; + return Results[Parm]; } } // namespace clang diff --git a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp index f58ce4aebcbfc..9c1dc1a76db63 100644 --- a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp +++ b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp @@ -977,6 +977,36 @@ 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 void f1(T &&a);" + "template void f2(T &&a);" + "template void f1(T &&a) { f2(std::forward(a)); }" + "template void f2(T &&a) { f1(std::forward(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 void f1(T &&a);" + "template void f2(T &&a);" + "template void f1(T &&a) { f2(std::forward(a)); }" + "template void f2(T &&a) { f1(std::forward(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 void f1(T &&a);" + "template void f2(T &&a);" + "template void f1(T &&a) { f2(std::forward(a)); a++; }" + "template void f2(T &&a) { f1(std::forward(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) {