https://github.com/ckandeler updated https://github.com/llvm/llvm-project/pull/225666
>From 0431c64cbcac74b9ffe2f1cc0fe7f76c42805deb Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Wed, 23 Sep 2026 11:57:12 +0200 Subject: [PATCH 1/2] [clangd] Extract to Function: mark unmodified captured parameters const Every captured variable was always passed by non-const reference, even when the extracted code never modifies it, resulting in misleading function signatures. Use clang's ExprMutationAnalyzer to check whether each captured variable is ever mutated within the extraction zone, and add const to the parameter's type when it isn't. Add a new clangAnalysis dependency to clangd's tweaks library for ExprMutationAnalyzer. Assisted-by: Claude --- .../clangd/refactor/tweaks/CMakeLists.txt | 1 + .../refactor/tweaks/ExtractFunction.cpp | 30 ++++++--- clang-tools-extra/clangd/tool/CMakeLists.txt | 1 + .../clangd/unittests/CMakeLists.txt | 1 + .../unittests/tweaks/ExtractFunctionTests.cpp | 62 ++++++++++++++++++- 5 files changed, 85 insertions(+), 10 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt b/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt index 1d6e38088ad67a..f14013b0270a72 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt +++ b/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt @@ -34,6 +34,7 @@ add_clang_library(clangDaemonTweaks OBJECT SwapIfBranches.cpp LINK_LIBS + clangAnalysis clangAST clangBasic clangDaemon diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp index eb7a9faa65a81f..0603d5d5a5b651 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -60,6 +60,7 @@ #include "clang/AST/NestedNameSpecifier.h" #include "clang/AST/RecursiveASTVisitor.h" #include "clang/AST/Stmt.h" +#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h" #include "clang/Basic/LangOptions.h" #include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" @@ -737,12 +738,27 @@ CapturedZoneInfo captureZoneInfo(const ExtractionZone &ExtZone) { return Result; } +// Whether VD is mutated anywhere within the extraction zone. If not, the +// corresponding parameter can safely be made const, even though it's still +// passed by reference. +// FIXME: Pass non-mutated parameters of built-in type by value. +bool isCapturedDeclMutated(const ValueDecl *VD, const ExtractionZone &ExtZone) { + ASTContext &Context = ExtZone.EnclosingFunction->getASTContext(); + for (const Stmt *RootStmt : ExtZone.RootStmts) { + ExprMutationAnalyzer Analyzer(*RootStmt, Context); + if (Analyzer.isMutated(VD)) + return true; + } + return false; +} + // Adds parameters to ExtractedFunc. // Returns true if able to find the parameters successfully and no hoisting // needed. // FIXME: Check if the declaration has a local/anonymous type bool createParameters(NewFunction &ExtractedFunc, - const CapturedZoneInfo &CapturedInfo) { + const CapturedZoneInfo &CapturedInfo, + const ExtractionZone &ExtZone) { for (const auto &KeyVal : CapturedInfo.DeclInfoMap) { const auto &DeclInfo = KeyVal.second; // If a Decl was Declared in zone and referenced in post zone, it @@ -764,11 +780,11 @@ bool createParameters(NewFunction &ExtractedFunc, return false; // Parameter qualifiers are same as the Decl's qualifiers. QualType TypeInfo = VD->getType().getNonReferenceType(); - // FIXME: Need better qualifier checks: check mutated status for - // Decl(e.g. was it assigned, passed as nonconst argument, etc) - // FIXME: check if parameter will be a non l-value reference. - // FIXME: We don't want to always pass variables of types like int, - // pointers, etc by reference. + // Add const if it's not mutated in the zone: it's still passed by + // reference to avoid a copy, but the reference doesn't need to be + // mutable. + if (!isCapturedDeclMutated(VD, ExtZone)) + TypeInfo.addConst(); bool IsPassedByReference = true; // We use the index of declaration as the ordering priority for parameters. ExtractedFunc.Parameters.push_back({std::string(VD->getName()), TypeInfo, @@ -868,7 +884,7 @@ llvm::Expected<NewFunction> getExtractedFunction(ExtractionZone &ExtZone, ExtractedFunc.DefinitionPoint = ExtZone.getInsertionPoint(); ExtractedFunc.CallerReturnsValue = CapturedInfo.AlwaysReturns; - if (!createParameters(ExtractedFunc, CapturedInfo) || + if (!createParameters(ExtractedFunc, CapturedInfo, ExtZone) || !generateReturnProperties(ExtractedFunc, *ExtZone.EnclosingFunction, CapturedInfo)) return error("Too complex to extract."); diff --git a/clang-tools-extra/clangd/tool/CMakeLists.txt b/clang-tools-extra/clangd/tool/CMakeLists.txt index 1bd6a1f864dd3b..99f625b0ad7340 100644 --- a/clang-tools-extra/clangd/tool/CMakeLists.txt +++ b/clang-tools-extra/clangd/tool/CMakeLists.txt @@ -46,6 +46,7 @@ target_link_libraries(clangdMain clang_target_link_libraries(clangd PRIVATE + clangAnalysis clangAST clangBasic clangLex diff --git a/clang-tools-extra/clangd/unittests/CMakeLists.txt b/clang-tools-extra/clangd/unittests/CMakeLists.txt index d596ba77efd4ae..4a562b1b5f4cd7 100644 --- a/clang-tools-extra/clangd/unittests/CMakeLists.txt +++ b/clang-tools-extra/clangd/unittests/CMakeLists.txt @@ -160,6 +160,7 @@ target_include_directories(ClangdTests PUBLIC clang_target_link_libraries(ClangdTests PRIVATE + clangAnalysis clangAST clangASTMatchers clangBasic diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp index c55fc17ae83e9a..4381eaf985f3da 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp @@ -61,7 +61,9 @@ TEST_F(ExtractFunctionTest, FunctionTest) { } TEST_F(ExtractFunctionTest, FileTest) { - // Check all parameters are in order + // Check all parameters are in order. `a` and `ptr` are mutated in the + // zone (`+=` and postfix `++` respectively), so stay non-const; `b` and + // `foo` are only read, so become const references. std::string ParameterCheckInput = R"cpp( struct Foo { int x; @@ -77,7 +79,7 @@ void f(int a) { struct Foo { int x; }; -void extracted(int &a, int &b, int * &ptr, Foo &foo) { +void extracted(int &a, const int &b, int * &ptr, const Foo &foo) { a += foo.x + b; *ptr++; } @@ -574,7 +576,7 @@ TEST_F(ExtractFunctionTest, ExistingReturnStatement) { // FIXME: avoid emitting redundant braces const char *After = R"cpp( bool lucky(int N); - int extracted(int &Min, int &Max) { + int extracted(const int &Min, const int &Max) { { for (int I = Min; I <= Max; ++I) if (lucky(I)) @@ -777,6 +779,60 @@ TEST_F(ExtractFunctionTest, VarDeclInitializer) { HasSubstr("extracted")); } +TEST_F(ExtractFunctionTest, ConstParameters) { + Context = File; + // A captured variable that's only read becomes a const reference. + EXPECT_THAT(apply(R"cpp( + void use(int); + void f(int x) { [[use(x);]] } + )cpp"), + HasSubstr("void extracted(const int &x)")); + // Direct assignment: stays non-const. + EXPECT_THAT(apply("void f(int x) { [[x = 1;]] }"), + HasSubstr("void extracted(int &x)")); + // Compound assignment: stays non-const. + EXPECT_THAT(apply("void f(int x) { [[x += 1;]] }"), + HasSubstr("void extracted(int &x)")); + // Increment/decrement: stays non-const. + EXPECT_THAT(apply("void f(int x) { [[++x;]] }"), + HasSubstr("void extracted(int &x)")); + // A non-const method call may mutate the object: stays non-const. + EXPECT_THAT(apply(R"cpp( + struct S { void mutate(); }; + void f(S s) { [[s.mutate();]] } + )cpp"), + HasSubstr("void extracted(S &s)")); + // A const method call cannot mutate the object: becomes const. + EXPECT_THAT(apply(R"cpp( + struct S { void inspect() const; }; + void f(S s) { [[s.inspect();]] } + )cpp"), + HasSubstr("void extracted(const S &s)")); + // Passed to a parameter taking a non-const reference: stays non-const, + // since the callee could mutate it through that reference. + EXPECT_THAT(apply(R"cpp( + void mayMutate(int &); + void f(int x) { [[mayMutate(x);]] } + )cpp"), + HasSubstr("void extracted(int &x)")); + // Passed to a parameter taking a const reference or by value: becomes + // const, since neither can mutate the caller's variable. + EXPECT_THAT(apply(R"cpp( + void readOnly(const int &); + void f(int x) { [[readOnly(x);]] } + )cpp"), + HasSubstr("void extracted(const int &x)")); + EXPECT_THAT(apply(R"cpp( + void byValue(int); + void f(int x) { [[byValue(x);]] } + )cpp"), + HasSubstr("void extracted(const int &x)")); + // A parameter that's already declared const stays as-is (no double + // const). + EXPECT_THAT(apply("void use(int); void f(const int x) { [[use(x);]] }"), + HasSubstr("void extracted(const int &x)")); +} + } // namespace } // namespace clangd } // namespace clang >From 6bc0623ae54fb7844df3511802fe070c0cda3874 Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Wed, 23 Sep 2026 15:05:34 +0200 Subject: [PATCH 2/2] [clangd] Address review: reuse ExprMutationAnalyzer across variables Constructing a fresh ExprMutationAnalyzer for every (captured variable, root statement) pair, inside the loop over captured variables, throws away its memoization cache on every single iteration, giving needlessly repeated work for zones with several captured variables. Build one analyzer per root statement up front in createParameters(), and reuse it across all variables checked against that statement. Assisted-by: Claude --- .../refactor/tweaks/ExtractFunction.cpp | 41 +++++++++++++------ 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp index 0603d5d5a5b651..b0bfdb816aeb36 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -72,6 +72,7 @@ #include "llvm/ADT/StringRef.h" #include "llvm/Support/Casting.h" #include "llvm/Support/Error.h" +#include <memory> #include <optional> namespace clang { @@ -738,19 +739,32 @@ CapturedZoneInfo captureZoneInfo(const ExtractionZone &ExtZone) { return Result; } -// Whether VD is mutated anywhere within the extraction zone. If not, the -// corresponding parameter can safely be made const, even though it's still -// passed by reference. -// FIXME: Pass non-mutated parameters of built-in type by value. -bool isCapturedDeclMutated(const ValueDecl *VD, const ExtractionZone &ExtZone) { - ASTContext &Context = ExtZone.EnclosingFunction->getASTContext(); - for (const Stmt *RootStmt : ExtZone.RootStmts) { - ExprMutationAnalyzer Analyzer(*RootStmt, Context); - if (Analyzer.isMutated(VD)) - return true; +// One ExprMutationAnalyzer per root statement of the extraction zone, built +// once and reused for every captured variable: each analyzer keeps a +// memoization cache keyed by Expr, which is only useful if it's actually +// allowed to persist across the (typically many) isMutated() queries run +// against the same statement. +class ZoneMutationAnalyzer { +public: + ZoneMutationAnalyzer(const ExtractionZone &ExtZone) { + ASTContext &Context = ExtZone.EnclosingFunction->getASTContext(); + for (const Stmt *RootStmt : ExtZone.RootStmts) + Analyzers.push_back( + std::make_unique<ExprMutationAnalyzer>(*RootStmt, Context)); } - return false; -} + + // Whether VD is mutated anywhere within the extraction zone. If not, the + // corresponding parameter can safely be made const, even though it's + // still passed by reference. + // FIXME: Pass non-mutated parameters of built-in type by value. + bool isMutated(const ValueDecl *VD) { + return llvm::any_of( + Analyzers, [VD](auto &Analyzer) { return Analyzer->isMutated(VD); }); + } + +private: + std::vector<std::unique_ptr<ExprMutationAnalyzer>> Analyzers; +}; // Adds parameters to ExtractedFunc. // Returns true if able to find the parameters successfully and no hoisting @@ -759,6 +773,7 @@ bool isCapturedDeclMutated(const ValueDecl *VD, const ExtractionZone &ExtZone) { bool createParameters(NewFunction &ExtractedFunc, const CapturedZoneInfo &CapturedInfo, const ExtractionZone &ExtZone) { + ZoneMutationAnalyzer MutationAnalyzer(ExtZone); for (const auto &KeyVal : CapturedInfo.DeclInfoMap) { const auto &DeclInfo = KeyVal.second; // If a Decl was Declared in zone and referenced in post zone, it @@ -783,7 +798,7 @@ bool createParameters(NewFunction &ExtractedFunc, // Add const if it's not mutated in the zone: it's still passed by // reference to avoid a copy, but the reference doesn't need to be // mutable. - if (!isCapturedDeclMutated(VD, ExtZone)) + if (!MutationAnalyzer.isMutated(VD)) TypeInfo.addConst(); bool IsPassedByReference = true; // We use the index of declaration as the ordering priority for parameters. _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
