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/4] [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 1d6e38088ad67..f14013b0270a7 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 eb7a9faa65a81..0603d5d5a5b65 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 1bd6a1f864dd3..99f625b0ad734 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 d596ba77efd4a..4a562b1b5f4cd 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 c55fc17ae83e9..4381eaf985f3d 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/4] [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 0603d5d5a5b65..b0bfdb816aeb3 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. >From c97da606081ebec87fc81172a806f38dd61efeb2 Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Thu, 24 Sep 2026 14:05:12 +0200 Subject: [PATCH 3/4] [clangd] Extract to Function: replace ExprMutationAnalyzer with a conservative check ExprMutationAnalyzer's mutation-finding is inherently expensive for this use case: each of its per-expression checks re-scans the whole analyzed statement rather than the candidate expression's local context, so cost scales with references-in-zone times zone-size. For a zone with many statements this is quadratic and clearly user-perceptible (measured ~270ms for a 200-statement/30-variable zone, vs. ~500us with no check at all). Replace it with a purpose-built check folded directly into the existing zone traversal (captureZoneInfo's ExtractionZoneVisitor), so there's no separate pass. Direct mutations (assignment, increment/ decrement, non-const method calls, explicit casts to non-const reference, non-const-reference call arguments, ...) are still recognized precisely, by inspecting each occurrence's immediate syntactic context as it's visited. Anything that aliases a captured variable (a reference bound to it, its address taken, capture by reference in a lambda, a forwarding-reference call argument, a non-const-ref range-for loop variable) is conservatively marked as a possible mutation without tracing whether the alias itself is later touched -- unlike ExprMutationAnalyzer, which for some of these cases searches the rest of the statement to avoid a false positive. This makes the whole analysis linear in zone size, matching the cost of having no check at all (79us / 441us for the same two zones), at the cost of some precision in alias-heavy code that was safe under the old check. Array-typed captures are now never made const, since properly handling array-to-pointer decay and array-element mutation without re-adding non-local searches wasn't worth it for a rare case. Also drop the now-unused clangAnalysis dependency from the three CMakeLists.txt files it was added to for ExprMutationAnalyzer. Assisted-by: Claude --- .../clangd/refactor/tweaks/CMakeLists.txt | 1 - .../refactor/tweaks/ExtractFunction.cpp | 223 +++++++++++++++--- clang-tools-extra/clangd/tool/CMakeLists.txt | 1 - .../clangd/unittests/CMakeLists.txt | 1 - .../unittests/tweaks/ExtractFunctionTests.cpp | 44 ++++ 5 files changed, 230 insertions(+), 40 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt b/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt index f14013b0270a7..1d6e38088ad67 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt +++ b/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt @@ -34,7 +34,6 @@ 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 b0bfdb816aeb3..012149a9fc561 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -60,7 +60,6 @@ #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" @@ -72,7 +71,6 @@ #include "llvm/ADT/StringRef.h" #include "llvm/Support/Casting.h" #include "llvm/Support/Error.h" -#include <memory> #include <optional> namespace clang { @@ -587,7 +585,10 @@ struct CapturedZoneInfo { unsigned DeclIndex; bool IsReferencedInZone = false; bool IsReferencedInPostZone = false; - // FIXME: Capture mutation information + // Conservatively: could this Decl be mutated somewhere in the zone? See + // ExtractionZoneVisitor::markPossiblyMutated() for what "conservatively" + // means here. + bool IsPossiblyMutated = false; DeclInformation(const Decl *TheDecl, ZoneRelative DeclaredIn, unsigned DeclIndex) : TheDecl(TheDecl), DeclaredIn(DeclaredIn), DeclIndex(DeclIndex){}; @@ -644,6 +645,30 @@ bool isLoop(const Stmt *S) { isa<CXXForRangeStmt>(S); } +// Strips E down to the Decl whose storage it ultimately refers to, chaining +// through parens, casts, and member/array-element access (e.g. `a.b[i]` +// resolves to `a`). Mutating any part of such a chain requires the base +// Decl itself to be non-const if the chain is through value members/ +// elements (e.g. `a.b = 1` mutates `a`'s own storage); for a chain through a +// pointer or reference, the base Decl's own binding usually isn't actually +// touched (e.g. `p->b = 1` only mutates `*p`, not `p` itself), but treating +// it as if it were is conservative and safe, just occasionally overcautious. +// Returns null if E isn't ultimately grounded in a variable this way (e.g. +// it's a temporary or a call result). +const Decl *underlyingDecl(const Expr *E) { + E = E->IgnoreParenCasts(); + if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) + return DRE->getDecl(); + if (const auto *ME = dyn_cast<MemberExpr>(E)) + return underlyingDecl(ME->getBase()); + if (const auto *ASE = dyn_cast<ArraySubscriptExpr>(E)) + return underlyingDecl(ASE->getBase()); + if (const auto *BO = dyn_cast<BinaryOperator>(E)) + if (BO->getOpcode() == BO_PtrMemD || BO->getOpcode() == BO_PtrMemI) + return underlyingDecl(BO->getLHS()); + return nullptr; +} + // Captures information from Extraction Zone CapturedZoneInfo captureZoneInfo(const ExtractionZone &ExtZone) { // We use the ASTVisitor instead of using the selection tree since we need to @@ -699,13 +724,162 @@ CapturedZoneInfo captureZoneInfo(const ExtractionZone &ExtZone) { if (!DeclInfo) DeclInfo = Info.createDeclInfo(D, ZoneRelative::OutsideFunc); DeclInfo->markOccurence(CurrentLocation); - // FIXME: check if reference mutates the Decl being referred. + return true; + } + + // Conservatively marks D as possibly mutated: used both for actual + // direct mutations (assignment, increment/decrement, ...) and for + // constructs that alias D in a way we don't want to trace further (a + // reference bound to D, D's address taken, D captured by reference in a + // lambda, ...). We never try to determine whether such an alias is + // itself later mutated -- that would require searching beyond this one + // occurrence, which is exactly the cost this design avoids (and what + // makes ExprMutationAnalyzer prohibitively slow). The price is + // that we sometimes keep a parameter non-const where a full alias + // analysis could prove it safe to const; we never get this wrong in the + // other, unsafe direction. + void markPossiblyMutated(const Decl *D) { + if (!D || CurrentLocation != ZoneRelative::Inside) + return; + if (auto *DeclInfo = Info.getDeclInfoFor(D)) + DeclInfo->IsPossiblyMutated = true; + } + void markPossiblyMutated(const Expr *E) { + markPossiblyMutated(underlyingDecl(E)); + } + + bool VisitBinaryOperator(BinaryOperator *BO) { + if (BO->isAssignmentOp()) + markPossiblyMutated(BO->getLHS()); + return true; + } + + bool VisitUnaryOperator(UnaryOperator *UO) { + if (UO->isIncrementDecrementOp() || UO->getOpcode() == UO_AddrOf) + markPossiblyMutated(UO->getSubExpr()); + return true; + } + + bool VisitExplicitCastExpr(ExplicitCastExpr *ECE) { + // An explicit cast to a non-const reference type allows mutating the + // result as if it were a plain non-const reference. + if (ECE->getType()->isReferenceType() && + !ECE->getType()->getPointeeType().isConstQualified()) + markPossiblyMutated(ECE->getSubExpr()); + return true; + } + + // Marks the object a non-const member function is (or may be) called + // on, whether through `.`, `->`, or an overloaded operator. + void markPossiblyMutatedCallee(const Expr *Object, + const CXXMethodDecl *Method) { + if (Method && !Method->isConst()) + markPossiblyMutated(Object); + } + + bool VisitCXXMemberCallExpr(CXXMemberCallExpr *MCE) { + markPossiblyMutatedCallee(MCE->getImplicitObjectArgument(), + MCE->getMethodDecl()); + return true; + } + + bool VisitCXXOperatorCallExpr(CXXOperatorCallExpr *OCE) { + if (OCE->getNumArgs() >= 1) + markPossiblyMutatedCallee( + OCE->getArg(0), + dyn_cast_or_null<CXXMethodDecl>(OCE->getCalleeDecl())); + return true; + } + + // Marks the arguments of a call that bind to a non-const reference + // parameter of Callee (a FunctionDecl or CXXConstructorDecl). If Callee + // is null (e.g. a call through a function pointer), conservatively marks + // every argument, since we don't know the parameter types. + void markPossiblyMutatedArgs(ArrayRef<const Expr *> Args, + const FunctionDecl *Callee) { + for (unsigned I = 0; I < Args.size(); ++I) { + if (!Callee || I >= Callee->getNumParams() || + (Callee->getParamDecl(I)->getType()->isReferenceType() && + !Callee->getParamDecl(I) + ->getType() + ->getPointeeType() + .isConstQualified())) + markPossiblyMutated(Args[I]); + } + } + + bool VisitCallExpr(CallExpr *CE) { + // Member/operator calls are already handled by their own visitors + // above (the callee there is a method, not a plain function). + if (isa<CXXMemberCallExpr>(CE) || isa<CXXOperatorCallExpr>(CE)) + return true; + markPossiblyMutatedArgs( + llvm::ArrayRef<const Expr *>(CE->getArgs(), CE->getNumArgs()), + CE->getDirectCallee()); + return true; + } + + bool VisitCXXConstructExpr(CXXConstructExpr *CCE) { + markPossiblyMutatedArgs( + llvm::ArrayRef<const Expr *>(CCE->getArgs(), CCE->getNumArgs()), + CCE->getConstructor()); + return true; + } + + bool VisitVarDecl(VarDecl *VD) { + // A non-const reference bound to a captured Decl aliases it: treat any + // such binding as a possible mutation, without checking whether the + // reference itself is later mutated (see markPossiblyMutated). This + // also covers reference structured bindings (DecompositionDecl is a + // VarDecl), since binding `auto &[a, b] = x` conservatively counts as + // aliasing all of `x`. + if (VD->getType()->isReferenceType() && + !VD->getType()->getPointeeType().isConstQualified() && VD->hasInit()) + markPossiblyMutated(VD->getInit()->IgnoreParens()); + return true; + } + + bool VisitLambdaExpr(LambdaExpr *LE) { + // Init-captures (`[r = x]`/`[&r = x]`) are handled by VisitVarDecl + // above, since they introduce a real VarDecl visited independently. + // This only needs to handle plain captures (`[&x]`/`[&]`), which don't. + for (const LambdaCapture &C : LE->captures()) + if (C.capturesVariable() && C.getCaptureKind() == LCK_ByRef) + markPossiblyMutated(C.getCapturedVar()); + return true; + } + + bool VisitCXXForRangeStmt(CXXForRangeStmt *FRS) { + // Conservatively: a non-const reference (or pointer) loop variable + // could mutate the range expression's elements, which -- like a + // member/array-element write -- requires the range expression's own + // Decl to be non-const if it's a value type (e.g. a plain array). + // We don't check whether the loop variable is actually mutated, and we + // don't special-case containers with const-qualified begin()/end(): + // those are visited like any other (possibly non-const) member call. + QualType LoopVarType = FRS->getLoopVariable()->getType(); + if ((LoopVarType->isReferenceType() && + !LoopVarType->getPointeeType().isConstQualified()) || + (LoopVarType->isPointerType() && + !LoopVarType->getPointeeType().isConstQualified())) + if (const Expr *RangeInit = FRS->getRangeInit()) + markPossiblyMutated(RangeInit); return true; } bool VisitReturnStmt(ReturnStmt *Return) { - if (CurrentLocation == ZoneRelative::Inside) + if (CurrentLocation == ZoneRelative::Inside) { Info.HasReturnStmt = true; + // Conservatively treat returning a captured Decl as a possible + // mutation, regardless of whether the return is actually by value + // (safe) or by non-const reference (not safe). Telling these apart + // needs the extracted function's return type, which is only + // decided later in generateReturnProperties(); duplicating its + // logic here would couple two distant functions for little gain, + // since directly returning a capture is rare. + if (const Expr *RV = Return->getRetValue()) + markPossiblyMutated(RV); + } return true; } @@ -739,33 +913,6 @@ CapturedZoneInfo captureZoneInfo(const ExtractionZone &ExtZone) { return Result; } -// 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)); - } - - // 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 // needed. @@ -773,7 +920,7 @@ class ZoneMutationAnalyzer { bool createParameters(NewFunction &ExtractedFunc, const CapturedZoneInfo &CapturedInfo, const ExtractionZone &ExtZone) { - ZoneMutationAnalyzer MutationAnalyzer(ExtZone); + // FIXME: Pass non-mutated parameters of built-in type by value. for (const auto &KeyVal : CapturedInfo.DeclInfoMap) { const auto &DeclInfo = KeyVal.second; // If a Decl was Declared in zone and referenced in post zone, it @@ -795,10 +942,12 @@ bool createParameters(NewFunction &ExtractedFunc, return false; // Parameter qualifiers are same as the Decl's qualifiers. QualType TypeInfo = VD->getType().getNonReferenceType(); - // 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 (!MutationAnalyzer.isMutated(VD)) + // Add const if it's not (conservatively) mutated in the zone: it's + // still passed by reference to avoid a copy, but the reference doesn't + // need to be mutable. Array types are never made const: mutating array + // elements through a non-const-ref loop variable or a decayed pointer + // argument is common and easy to miss conservatively, so we don't try. + if (!DeclInfo.IsPossiblyMutated && !TypeInfo->isArrayType()) TypeInfo.addConst(); bool IsPassedByReference = true; // We use the index of declaration as the ordering priority for parameters. diff --git a/clang-tools-extra/clangd/tool/CMakeLists.txt b/clang-tools-extra/clangd/tool/CMakeLists.txt index 99f625b0ad734..1bd6a1f864dd3 100644 --- a/clang-tools-extra/clangd/tool/CMakeLists.txt +++ b/clang-tools-extra/clangd/tool/CMakeLists.txt @@ -46,7 +46,6 @@ 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 4a562b1b5f4cd..d596ba77efd4a 100644 --- a/clang-tools-extra/clangd/unittests/CMakeLists.txt +++ b/clang-tools-extra/clangd/unittests/CMakeLists.txt @@ -160,7 +160,6 @@ 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 4381eaf985f3d..4f6d62b7ea426 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp @@ -11,6 +11,7 @@ #include "gtest/gtest.h" using ::testing::HasSubstr; +using ::testing::Not; using ::testing::StartsWith; namespace clang { @@ -833,6 +834,49 @@ TEST_F(ExtractFunctionTest, ConstParameters) { HasSubstr("void extracted(const int &x)")); } +TEST_F(ExtractFunctionTest, ConstParametersReferenceAliasing) { + Context = File; + // A non-const reference bound to a captured variable conservatively + // mutates that variable too, without checking whether the reference + // itself is ever actually mutated -- even when the binding and a later + // mutation of the reference are two separate root statements of the same + // zone. Failing to notice this would incorrectly mark `x` const, which + // wouldn't compile (`int &r` can't bind to a `const int`). + EXPECT_THAT(apply(R"cpp( + void f(int x) { + [[int &r = x; + r = 2;]] + } + )cpp"), + HasSubstr("void extracted(int &x)")); +} + +TEST_F(ExtractFunctionTest, ConstParametersConservativeAliasing) { + Context = File; + // Taking the address of a captured variable conservatively mutates it, + // regardless of what's later done with the pointer. + EXPECT_THAT(apply("void f(int x) { [[int *p = &x;]] }"), + HasSubstr("void extracted(int &x)")); + // Explicit cast to a non-const reference type: stays non-const. + EXPECT_THAT(apply("void f(int x) { [[static_cast<int &>(x) = 1;]] }"), + HasSubstr("void extracted(int &x)")); + // Captured by reference in a lambda: stays non-const, without checking + // whether the lambda actually mutates it. + EXPECT_THAT(apply("void f(int x) { [[auto l = [&x]() { int y = x; };]] }"), + HasSubstr("int &x")); + // Captured by value in a lambda: doesn't alias x, so becomes const. + EXPECT_THAT(apply("void f(int x) { [[auto l = [x]() { int y = x; };]] }"), + HasSubstr("const int &x")); + // Returning a captured variable is conservatively treated as a possible + // mutation, regardless of whether the return is actually by value (safe) + // or by non-const reference (not safe) -- telling these apart isn't + // worth the complexity here. + EXPECT_THAT(apply("int f(int x) { [[return x;]] }"), HasSubstr("&x")); + // Array-typed captures are never made const. + EXPECT_THAT(apply("void f() { int arr[5]; [[arr[0] = 1;]] }"), + Not(HasSubstr("const"))); +} + } // namespace } // namespace clangd } // namespace clang >From 482d0dcf29338e92598737f311a2257258fb6f10 Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Mon, 28 Sep 2026 11:05:15 +0200 Subject: [PATCH 4/4] [clangd] Address review: fix member-call arguments and pointer indirection Two correctness fixes and two cleanups in the conservative mutation check: - CXXMemberCallExpr::getImplicitObjectArgument() can return nullptr (e.g. for a call through a member pointer), which underlyingDecl() then dereferenced unconditionally. Add a null check. - VisitCXXMemberCallExpr only ever checked the implicit object, never the call's actual arguments, and VisitCallExpr skipped member/ operator calls entirely on the assumption they were already fully handled. So a non-const-reference argument to a member call (e.g. `s.mayMutate(x)` with `void mayMutate(int&)`) never got marked as mutated. Fix by also checking the explicit arguments of member calls, and give operator calls correct offset-aware handling (a member operator's first argument is the implicit object, not a real parameter; a free-function operator has no implicit object at all). - underlyingDecl() chained through a pointer-typed base the same as a value-typed one (e.g. `ptr->member = 1` and `ptr[i] = 1` both marked `ptr` mutated, even though only the pointee is touched), while a plain dereference (`*ptr = 1`) correctly didn't. Make the member/array-element and pointer-to-member cases stop at a pointer-typed base too, for both correctness-neutral consistency and slightly better precision. - Drop the ExtractionZone parameter from createParameters(), unused since the ExprMutationAnalyzer construction that needed it is gone. Assisted-by: Claude --- .../refactor/tweaks/ExtractFunction.cpp | 102 +++++++++++++----- .../unittests/tweaks/ExtractFunctionTests.cpp | 47 ++++++++ 2 files changed, 120 insertions(+), 29 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp index 012149a9fc561..de5e1a817f548 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -647,24 +647,42 @@ bool isLoop(const Stmt *S) { // Strips E down to the Decl whose storage it ultimately refers to, chaining // through parens, casts, and member/array-element access (e.g. `a.b[i]` -// resolves to `a`). Mutating any part of such a chain requires the base -// Decl itself to be non-const if the chain is through value members/ -// elements (e.g. `a.b = 1` mutates `a`'s own storage); for a chain through a -// pointer or reference, the base Decl's own binding usually isn't actually -// touched (e.g. `p->b = 1` only mutates `*p`, not `p` itself), but treating -// it as if it were is conservative and safe, just occasionally overcautious. +// resolves to `a`), but only when that access reaches through value +// semantics: mutating `a.b` mutates `a`'s own storage, so we keep chaining. +// We deliberately stop at a pointer-typed base (e.g. `p->b`, `p[i]`, +// `p->*pmf`): that only mutates `*p`, never `p`'s own binding, so chaining +// through it would incorrectly require `p` to stay non-const. For the same +// reason, a dereference (`*p = 1`) is intentionally not handled at all: it +// only ever mutates the pointee, never the pointer itself. +// +// A genuine array subscript (`arr[i]` where `arr` is an array, not a +// pointer) also stops here: the base is always wrapped in an +// ArrayToPointerDecay cast, indistinguishable at this point from +// subscripting a real pointer. That's fine because createParameters() +// never makes an array-typed capture const in the first place, regardless +// of what we compute here. +// // Returns null if E isn't ultimately grounded in a variable this way (e.g. -// it's a temporary or a call result). +// it's a temporary, a call result, or reached through a pointer). const Decl *underlyingDecl(const Expr *E) { + if (!E) + return nullptr; E = E->IgnoreParenCasts(); if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) return DRE->getDecl(); - if (const auto *ME = dyn_cast<MemberExpr>(E)) + if (const auto *ME = dyn_cast<MemberExpr>(E)) { + if (ME->getBase()->getType()->isPointerType()) + return nullptr; return underlyingDecl(ME->getBase()); - if (const auto *ASE = dyn_cast<ArraySubscriptExpr>(E)) + } + if (const auto *ASE = dyn_cast<ArraySubscriptExpr>(E)) { + if (ASE->getBase()->getType()->isPointerType()) + return nullptr; return underlyingDecl(ASE->getBase()); + } if (const auto *BO = dyn_cast<BinaryOperator>(E)) - if (BO->getOpcode() == BO_PtrMemD || BO->getOpcode() == BO_PtrMemI) + if ((BO->getOpcode() == BO_PtrMemD || BO->getOpcode() == BO_PtrMemI) && + !BO->getLHS()->getType()->isPointerType()) return underlyingDecl(BO->getLHS()); return nullptr; } @@ -777,20 +795,6 @@ CapturedZoneInfo captureZoneInfo(const ExtractionZone &ExtZone) { markPossiblyMutated(Object); } - bool VisitCXXMemberCallExpr(CXXMemberCallExpr *MCE) { - markPossiblyMutatedCallee(MCE->getImplicitObjectArgument(), - MCE->getMethodDecl()); - return true; - } - - bool VisitCXXOperatorCallExpr(CXXOperatorCallExpr *OCE) { - if (OCE->getNumArgs() >= 1) - markPossiblyMutatedCallee( - OCE->getArg(0), - dyn_cast_or_null<CXXMethodDecl>(OCE->getCalleeDecl())); - return true; - } - // Marks the arguments of a call that bind to a non-const reference // parameter of Callee (a FunctionDecl or CXXConstructorDecl). If Callee // is null (e.g. a call through a function pointer), conservatively marks @@ -808,9 +812,50 @@ CapturedZoneInfo captureZoneInfo(const ExtractionZone &ExtZone) { } } + bool VisitCXXMemberCallExpr(CXXMemberCallExpr *MCE) { + markPossiblyMutatedCallee(MCE->getImplicitObjectArgument(), + MCE->getMethodDecl()); + // getArgs() here is just the explicit argument list, without the + // implicit object handled above, so it aligns directly with the + // method's own parameters. + markPossiblyMutatedArgs( + llvm::ArrayRef<const Expr *>(MCE->getArgs(), MCE->getNumArgs()), + MCE->getMethodDecl()); + return true; + } + + bool VisitCXXOperatorCallExpr(CXXOperatorCallExpr *OCE) { + // Unlike CXXMemberCallExpr, a member operator's implicit object is + // args[0], not split out separately; args[1:] are the real + // parameters. A non-member (free function) operator overload has no + // implicit object at all, so all args align directly with params. + // Every operator has at least one operand, but guard anyway since + // the arithmetic below would underflow on an empty argument list. + if (OCE->getNumArgs() == 0) + return true; + const auto *Method = + dyn_cast_or_null<CXXMethodDecl>(OCE->getCalleeDecl()); + if (Method) { + markPossiblyMutatedCallee(OCE->getArg(0), Method); + markPossiblyMutatedArgs(llvm::ArrayRef<const Expr *>( + OCE->getArgs() + 1, OCE->getNumArgs() - 1), + Method); + } else { + markPossiblyMutatedArgs( + llvm::ArrayRef<const Expr *>(OCE->getArgs(), OCE->getNumArgs()), + OCE->getDirectCallee()); + } + return true; + } + bool VisitCallExpr(CallExpr *CE) { - // Member/operator calls are already handled by their own visitors - // above (the callee there is a method, not a plain function). + // Both are already fully handled by their own visitors above, + // including their arguments: skip CXXMemberCallExpr here to avoid + // redundant work (its getArgs()/getDirectCallee() would otherwise + // align fine on their own), and skip CXXOperatorCallExpr because its + // getArgs() includes the implicit object as args[0], which would + // misalign against a member operator's real parameter list if + // checked here too. if (isa<CXXMemberCallExpr>(CE) || isa<CXXOperatorCallExpr>(CE)) return true; markPossiblyMutatedArgs( @@ -918,8 +963,7 @@ CapturedZoneInfo captureZoneInfo(const ExtractionZone &ExtZone) { // needed. // FIXME: Check if the declaration has a local/anonymous type bool createParameters(NewFunction &ExtractedFunc, - const CapturedZoneInfo &CapturedInfo, - const ExtractionZone &ExtZone) { + const CapturedZoneInfo &CapturedInfo) { // FIXME: Pass non-mutated parameters of built-in type by value. for (const auto &KeyVal : CapturedInfo.DeclInfoMap) { const auto &DeclInfo = KeyVal.second; @@ -1048,7 +1092,7 @@ llvm::Expected<NewFunction> getExtractedFunction(ExtractionZone &ExtZone, ExtractedFunc.DefinitionPoint = ExtZone.getInsertionPoint(); ExtractedFunc.CallerReturnsValue = CapturedInfo.AlwaysReturns; - if (!createParameters(ExtractedFunc, CapturedInfo, ExtZone) || + if (!createParameters(ExtractedFunc, CapturedInfo) || !generateReturnProperties(ExtractedFunc, *ExtZone.EnclosingFunction, CapturedInfo)) return error("Too complex to extract."); diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp index 4f6d62b7ea426..9e816ccfc3b7a 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp @@ -877,6 +877,53 @@ TEST_F(ExtractFunctionTest, ConstParametersConservativeAliasing) { Not(HasSubstr("const"))); } +TEST_F(ExtractFunctionTest, ConstParametersMemberCallArguments) { + Context = File; + // A non-const-ref argument to a member call (as opposed to the implicit + // object, already covered by ConstParameters) stays non-const. The + // method itself is const so it doesn't also mark `s` mutated, isolating + // the argument check from the object check. + EXPECT_THAT(apply(R"cpp( + struct S { void mayMutate(int &) const; }; + void f(S s, int x) { [[s.mayMutate(x);]] } + )cpp"), + HasSubstr("extracted(const S &s, int &x)")); + // Same, but through an overloaded operator: the implicit object is + // args[0], so the real parameter (checked against args[1:]) must be + // found at the right offset. + EXPECT_THAT(apply(R"cpp( + struct S { void operator()(int &) const; }; + void f(S s, int x) { [[s(x);]] } + )cpp"), + HasSubstr("extracted(const S &s, int &x)")); + // A free (non-member) operator overload has no implicit object, so all + // operands align directly with the callee's parameters. + EXPECT_THAT(apply(R"cpp( + struct S {}; + void operator+(const S &, int &); + void f(S s, int x) { [[s + x;]] } + )cpp"), + HasSubstr("extracted(const S &s, int &x)")); +} + +TEST_F(ExtractFunctionTest, ConstParametersPointerIndirection) { + Context = File; + // Mutating a member/element through a pointer only mutates the pointee, + // never the pointer's own binding, so the pointer stays const-eligible -- + // unlike the same access through a value or reference (already covered + // by ConstParameters' `ptr` case, which is mutated directly instead). + EXPECT_THAT(apply(R"cpp( + struct S { int x; }; + void f(S *ptr) { [[ptr->x = 1;]] } + )cpp"), + HasSubstr("extracted(S *const &ptr)")); + EXPECT_THAT(apply("void f(int *p) { [[p[0] = 1;]] }"), + HasSubstr("extracted(int *const &p)")); + // Same for a plain dereference. + EXPECT_THAT(apply("void f(int *p) { [[*p = 1;]] }"), + HasSubstr("extracted(int *const &p)")); +} + } // namespace } // namespace clangd } // namespace clang _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
