https://github.com/ckandeler created https://github.com/llvm/llvm-project/pull/225666
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` (already used by several clang-tidy checks for the same purpose, e.g. `misc-const-correctness`) 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. Still passed by reference either way, to avoid a copy. Add a new `clangAnalysis` dependency to clangd's tweaks library for `ExprMutationAnalyzer`. Assisted-by: Claude >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] [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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
