[clang-tools-extra] [clangd] Extract to Function: mark unmodified captured parameters const (PR #225666)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 23 03:13:02 PDT 2026
llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tools-extra
Author: Christian Kandeler (ckandeler)
<details>
<summary>Changes</summary>
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
---
Full diff: https://github.com/llvm/llvm-project/pull/225666.diff
5 Files Affected:
- (modified) clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt (+1)
- (modified) clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp (+23-7)
- (modified) clang-tools-extra/clangd/tool/CMakeLists.txt (+1)
- (modified) clang-tools-extra/clangd/unittests/CMakeLists.txt (+1)
- (modified) clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp (+59-3)
``````````diff
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
``````````
</details>
https://github.com/llvm/llvm-project/pull/225666
More information about the cfe-commits
mailing list