https://github.com/ckandeler updated https://github.com/llvm/llvm-project/pull/230046
>From 60e6beac9a68d0f8d839fc85fc968797597b5f51 Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Wed, 7 Oct 2026 16:14:15 +0200 Subject: [PATCH 1/6] [clangd] Extract to function: Fix parameter declarator printing A parameter's type and name were rendered by printing the type as a standalone string and appending the name after it. That's wrong for any declarator syntax that doesn't simply put the name after the type: a function pointer parameter rendered as the uncompilable `void (*)(int) F` instead of `void (*F)(int)`, and a parameter that's a reference to an array rendered as the equally invalid `int[5] &arr` instead of `int (&arr)[5]`. Fixed by passing the name as printType()'s existing declarator placeholder argument instead, letting clang's own type printer place it correctly. For the reference case, this requires building the actual reference QualType rather than just prepending "&" to the placeholder string, since only the real type lets the printer parenthesize the array case correctly. Assisted-by: Claude Code --- .../refactor/tweaks/ExtractFunction.cpp | 16 +++++++++-- .../unittests/tweaks/ExtractFunctionTests.cpp | 28 +++++++++++++------ 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp index 1ac049b4a4f0ef..73bbef76206669 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -642,8 +642,20 @@ NewFunction::getFuncBody(const SourceManager &SM) const { } std::string NewFunction::Parameter::render(const DeclContext *Context) const { - return printType(TypeInfo, *Context) + - (Kind == ParamPassKind::Reference ? " &" : " ") + Name; + // Passing Name as printType()'s declarator placeholder (rather than just + // appending it after the printed type) lets clang's own type printer + // place it correctly for declarator syntax that doesn't simply put the + // name after the type, e.g. a function pointer (`void (*name)(int)`, not + // `void (*)(int) name`) or an array (`int name[5]`, not `int[5] name`). + // For a reference, building the actual reference QualType (rather than + // just prepending "&" to the placeholder string) is what lets the + // printer correctly parenthesize a reference-to-array + // (`int (&name)[5]`, not the invalid `int &name[5]`). + QualType RenderedType = + Kind == ParamPassKind::Reference + ? Context->getParentASTContext().getLValueReferenceType(TypeInfo) + : TypeInfo; + return printType(RenderedType, *Context, Name); } // Stores captured information about Extraction Zone. diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp index b432f6b090c96a..43611ac9068653 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp @@ -79,7 +79,7 @@ void f(int a) { struct Foo { int x; }; -void extracted(int &a, int b, int * &ptr, const Foo &foo) { +void extracted(int &a, int b, int *&ptr, const Foo &foo) { a += foo.x + b; *ptr++; } @@ -927,12 +927,12 @@ TEST_F(ExtractFunctionTest, ConstParametersPointerIndirection) { struct S { int x; }; void f(S *ptr) { [[ptr->x = 1;]] } )cpp"), - HasSubstr("extracted(S * ptr)")); + HasSubstr("extracted(S *ptr)")); EXPECT_THAT(apply("void f(int *p) { [[p[0] = 1;]] }"), - HasSubstr("extracted(int * p)")); + HasSubstr("extracted(int *p)")); // Same for a plain dereference. EXPECT_THAT(apply("void f(int *p) { [[*p = 1;]] }"), - HasSubstr("extracted(int * p)")); + HasSubstr("extracted(int *p)")); } TEST_F(ExtractFunctionTest, ConstParametersConditionalReferenceBinding) { @@ -975,11 +975,21 @@ TEST_F(ExtractFunctionTest, ConstParametersStaticOperatorCall) { HasSubstr("extracted(const S &s, int x)")); } +TEST_F(ExtractFunctionTest, ConstParametersFunctionPointer) { + Context = File; + // A function pointer's declarator syntax puts the name inside the + // parens around the "*", not after the whole type: printType()'s + // placeholder must be threaded through for this to render as + // `void (*F)(int)` rather than the uncompilable `void (*)(int) F`. + EXPECT_THAT(apply("void f(void (*F)(int)) { [[F(1);]] }"), + HasSubstr("extracted(void (*F)(int))")); +} + TEST_F(ExtractFunctionTest, ConstParametersScalarsByValue) { Context = File; // An unmutated pointer is a scalar too: passed by value. EXPECT_THAT(apply("void use(int *); void f(int *p) { [[use(p);]] }"), - HasSubstr("extracted(int * p)")); + HasSubstr("extracted(int *p)")); // An unmutated enum: passed by value. EXPECT_THAT(apply(R"cpp( enum E { A, B }; @@ -998,7 +1008,7 @@ TEST_F(ExtractFunctionTest, ConstParametersScalarsByValue) { // An unmutated array is not a scalar (even though its element type is): // stays a non-const reference, per the existing array carve-out. EXPECT_THAT(apply("void f() { int arr[5]; [[int x = arr[0];]] }"), - HasSubstr("extracted(int[5] &arr)")); + HasSubstr("extracted(int (&arr)[5])")); } // Variables of reference type, const or non-const, must stay references, @@ -1065,7 +1075,7 @@ TEST_F(ExtractFunctionTest, CFileModifiedScalarBecomesPointer) { int j; [[j = 0;]] })cpp"), - AllOf(HasSubstr("extracted(int * j)"), HasSubstr("(*j) = 0;"), + AllOf(HasSubstr("extracted(int *j)"), HasSubstr("(*j) = 0;"), HasSubstr("extracted(&j)"))); } @@ -1082,7 +1092,7 @@ TEST_F(ExtractFunctionTest, CFileUnmodifiedStructBecomesConstPointer) { p.v1 = 0; [[i = p.v1;]] })cpp"), - AllOf(HasSubstr("extracted(const struct pair * p)"), + AllOf(HasSubstr("extracted(const struct pair *p)"), HasSubstr("i = p->v1;"), HasSubstr("extracted(&p)"))); } @@ -1134,7 +1144,7 @@ TEST_F(ExtractFunctionTest, CFileModifiedArrayStaysPlainPointer) { int arr[5]; [[arr[0] = 1;]] })cpp"), - AllOf(HasSubstr("extracted(int * arr)"), HasSubstr("arr[0] = 1;"), + AllOf(HasSubstr("extracted(int *arr)"), HasSubstr("arr[0] = 1;"), HasSubstr("extracted(arr)"), Not(HasSubstr("&arr")))); } >From 601fdf0f7ca430cd1bbb9b697132b58beb8aed7f Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Fri, 9 Oct 2026 10:22:19 +0200 Subject: [PATCH 2/6] [clangd] Extract to function: Fix function-reference const and reject VLAs Adding const to an unmutated captured reference applied to a captured function reference too, producing unparseable output: a (non-pointer) function type can't be cv-qualified at all, so `void (&F)(int)` became the invalid `void (const &F)(int)`. Excluded function types from that branch, same as arrays already were for a different reason. In C, decaying a captured array to a pointer parameter also needs to reject variable-length arrays: a VLA's bound (at any nesting depth, e.g. the inner dimension of `int A[2][N]`) is an arbitrary expression, typically another variable's name, that isn't necessarily a parameter of the extracted function at all, and even when it is, the printed array-pointer parameter type can't correctly account for it. Assisted-by: Claude Code --- .../refactor/tweaks/ExtractFunction.cpp | 18 ++++++++++-- .../unittests/tweaks/ExtractFunctionTests.cpp | 28 +++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp index 73bbef76206669..672a6395c3a3a4 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -1185,12 +1185,15 @@ bool createParameters(NewFunction &ExtractedFunc, !FullTypeInfo->isReferenceType() && Context.getTypeSizeInChars(TypeInfo) <= 2 * WordSize) { Kind = ParamPassKind::Value; - } else if (!TypeInfo->isArrayType()) { + } else if (!TypeInfo->isArrayType() && !TypeInfo->isFunctionType()) { // 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. + // conservatively, so we don't try. A (non-pointer) function type + // can't be cv-qualified at all -- doing so anyway produces + // unparseable output, e.g. `void (const &F)(int)` for a + // captured function reference. TypeInfo.addConst(); } } @@ -1212,6 +1215,17 @@ bool createParameters(NewFunction &ExtractedFunc, // sizeof/alignof/typeof (or similar) on it. if (DeclInfo.HasUnsafeTypeQueryUseInZone) return false; + // A variable-length array's bound (at any nesting depth, e.g. + // the inner dimension of `int A[2][N]`) is an arbitrary + // expression that's typically just some other variable's name + // -- but that variable isn't necessarily (and often isn't) also + // a parameter of the extracted function, and even if it is, a + // mutated one would itself have been turned into a pointer, + // which the bound expression can't account for either way. + // Bail out rather than risk printing a parameter type that + // references a name meaningless (or absent) in the new scope. + if (TypeInfo->isVariablyModifiedType()) + return false; TypeInfo = Context.getArrayDecayedType(TypeInfo); Kind = ParamPassKind::Value; } else { diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp index 43611ac9068653..c0e2de9c7b95fa 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp @@ -985,6 +985,16 @@ TEST_F(ExtractFunctionTest, ConstParametersFunctionPointer) { HasSubstr("extracted(void (*F)(int))")); } +TEST_F(ExtractFunctionTest, ConstParametersFunctionReference) { + Context = File; + // A (non-pointer) function type can't be cv-qualified at all, unlike + // every other non-scalar, non-array type this conservative-const + // logic otherwise applies to: adding const here would produce + // unparseable output (`void (const &F)(int)`). + EXPECT_THAT(apply("void f(void (&F)(int)) { [[F(0);]] }"), + HasSubstr("extracted(void (&F)(int))")); +} + TEST_F(ExtractFunctionTest, ConstParametersScalarsByValue) { Context = File; // An unmutated pointer is a scalar too: passed by value. @@ -1221,6 +1231,24 @@ TEST_F(ExtractFunctionTest, CFileRejectArrayTypeof) { "fail: Too complex to extract."); } +TEST_F(ExtractFunctionTest, CFileRejectVariableLengthArray) { + // A VLA's bound is an arbitrary expression -- typically another + // variable's name -- that isn't necessarily (and here isn't at all) + // itself a parameter of the extracted function, and even printing it + // via the usual array-decay path would reference a name meaningless + // in the new scope. Here the VLA bound isn't even at the top level + // (it's the inner dimension of a 2D array), exercising that + // `isVariablyModifiedType()` catches it regardless of nesting depth. + FileName = "a.c"; + Context = File; + EXPECT_EQ(apply(R"cpp( + void foo(int N) { + int A[2][N]; + [[A[0][0] = 1;]] + })cpp"), + "fail: Too complex to extract."); +} + } // namespace } // namespace clangd } // namespace clang >From bc4113bf2dabf86d3f2582e5511bcb387b1a4e65 Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Fri, 9 Oct 2026 12:58:04 +0200 Subject: [PATCH 3/6] [clangd] Extract to function: Don't over-reject variable-length arrays Only an array whose decayed pointer type still has a variable dimension is actually a problem (e.g. `int A[2][N]` decays to `int (*A)[N]`, which can't correctly account for `N`). A single- dimension VLA decays to a plain pointer with no bound to print at all (`int A[N]` to `int *A`), so it was being rejected unnecessarily. Fixed by checking the decayed type's pointee for variability instead of the original array type. Also added a test confirming C++ already handles VLA captures correctly as-is: an unmutated array there always stays a reference, never decaying to a pointer, so there's no separate parameter that needs to agree with the array's own bound. Assisted-by: Claude Code --- .../refactor/tweaks/ExtractFunction.cpp | 27 ++++++----- .../unittests/tweaks/ExtractFunctionTests.cpp | 45 +++++++++++++++---- 2 files changed, 53 insertions(+), 19 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp index 672a6395c3a3a4..3077fc67ea3db5 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -1215,18 +1215,23 @@ bool createParameters(NewFunction &ExtractedFunc, // sizeof/alignof/typeof (or similar) on it. if (DeclInfo.HasUnsafeTypeQueryUseInZone) return false; - // A variable-length array's bound (at any nesting depth, e.g. - // the inner dimension of `int A[2][N]`) is an arbitrary - // expression that's typically just some other variable's name - // -- but that variable isn't necessarily (and often isn't) also - // a parameter of the extracted function, and even if it is, a - // mutated one would itself have been turned into a pointer, - // which the bound expression can't account for either way. - // Bail out rather than risk printing a parameter type that - // references a name meaningless (or absent) in the new scope. - if (TypeInfo->isVariablyModifiedType()) + QualType Decayed = Context.getArrayDecayedType(TypeInfo); + // The array's own outermost dimension is never a problem, VLA + // or not: once decayed, it no longer appears in the type at + // all (`int A[N]` decays to plain `int *A`). An inner + // dimension that's still variable-length (e.g. the `N` in + // `int A[2][N]`, which decays to `int (*A)[N]`) is the actual + // hazard: its bound is an arbitrary expression, typically just + // some other variable's name, that isn't necessarily (and + // often isn't) also a parameter of the extracted function, and + // even if it is, a mutated one would itself have been turned + // into a pointer, which the bound expression can't account for + // either way. Bail out rather than risk printing a parameter + // type that references a name meaningless (or absent) in the + // new scope. + if (Decayed->getPointeeType()->isVariablyModifiedType()) return false; - TypeInfo = Context.getArrayDecayedType(TypeInfo); + TypeInfo = Decayed; Kind = ParamPassKind::Value; } else { // Bail out rather than rewrite a use whose location (or, for a diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp index c0e2de9c7b95fa..9857b8e64b1f99 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp @@ -1231,14 +1231,28 @@ TEST_F(ExtractFunctionTest, CFileRejectArrayTypeof) { "fail: Too complex to extract."); } -TEST_F(ExtractFunctionTest, CFileRejectVariableLengthArray) { - // A VLA's bound is an arbitrary expression -- typically another - // variable's name -- that isn't necessarily (and here isn't at all) - // itself a parameter of the extracted function, and even printing it - // via the usual array-decay path would reference a name meaningless - // in the new scope. Here the VLA bound isn't even at the top level - // (it's the inner dimension of a 2D array), exercising that - // `isVariablyModifiedType()` catches it regardless of nesting depth. +TEST_F(ExtractFunctionTest, CFileAllowSingleDimensionVariableLengthArray) { + // A VLA's own outermost dimension is never a problem: once decayed, + // it no longer appears in the parameter type at all (`int A[N]` + // decays to plain `int *A`, not something that needs to mention `N`). + FileName = "a.c"; + Context = File; + EXPECT_THAT(apply(R"cpp( + void foo(int N) { + int A[N]; + [[A[0] = 1;]] + })cpp"), + HasSubstr("extracted(int *A)")); +} + +TEST_F(ExtractFunctionTest, CFileRejectMultiDimensionVariableLengthArray) { + // Unlike the single-dimension case, an inner dimension that's still + // variable-length after decaying the outer one (`int A[2][N]` decays + // to `int (*A)[N]`) is a real hazard: `N` is an arbitrary expression, + // typically another variable's name, that isn't necessarily (and + // here isn't at all) itself a parameter of the extracted function, + // and even printing it would reference a name meaningless in the new + // scope. FileName = "a.c"; Context = File; EXPECT_EQ(apply(R"cpp( @@ -1249,6 +1263,21 @@ TEST_F(ExtractFunctionTest, CFileRejectVariableLengthArray) { "fail: Too complex to extract."); } +TEST_F(ExtractFunctionTest, CXXVariableLengthArrayByReference) { + // In C++ (where clang also supports VLAs as a GNU extension), this + // isn't a problem at all: an unmutated array always stays a + // reference there (see ConstParametersScalarsByValue's array case), + // never decaying to a pointer, so there's no separate parameter that + // needs to agree with the array's own bound. + Context = File; + EXPECT_THAT(apply(R"cpp( + void foo(int N) { + int A[2][N]; + [[A[0][0] = 1;]] + })cpp"), + HasSubstr("extracted(int (&A)[2][N])")); +} + } // namespace } // namespace clangd } // namespace clang >From f8bb22ba1b3b9e9b5772e87e519532b13d839d5d Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Tue, 13 Oct 2026 12:52:57 +0200 Subject: [PATCH 4/6] [clangd] Extract to function: Reject C++ variable-length arrays too A captured array stays by reference in C++, never decaying to a pointer -- but a reference's declared type spells out every dimension's bound, including the outermost one. For a variable-length array, that bound is an arbitrary expression (typically another variable's name) that isn't in scope in the extracted function's declaration, and making it an explicit preceding parameter doesn't help either: the two don't line up at the call site, since a reference-to-array parameter can't be bound by just matching the spelling. The previous VLA rejection only applied in C; this applies it unconditionally. Also: pass DeclContext by reference instead of by pointer in Parameter::render(), avoiding a double dereference, and corrected a couple of comments that overclaimed printType()'s declarator placeholder handles a bare (non-reference, non-pointer) array type correctly -- confirmed empirically that it doesn't, which is why the C path still needs to decay explicitly. Assisted-by: Claude Code --- .../refactor/tweaks/ExtractFunction.cpp | 49 ++++++++++++++----- .../unittests/tweaks/ExtractFunctionTests.cpp | 36 +++++++++++--- 2 files changed, 64 insertions(+), 21 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp index 3077fc67ea3db5..10ed822518401a 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -438,7 +438,7 @@ struct NewFunction { QualType TypeInfo; ParamPassKind Kind; unsigned OrderPriority; // Lower value parameters are preferred first. - std::string render(const DeclContext *Context) const; + std::string render(const DeclContext &Context) const; bool operator<(const Parameter &Other) const { return OrderPriority < Other.OrderPriority; } @@ -510,7 +510,7 @@ std::string NewFunction::renderParametersForDeclaration( if (NeedCommaBefore) Result += ", "; NeedCommaBefore = true; - Result += P.render(&Enclosing); + Result += P.render(Enclosing); } return Result; } @@ -641,21 +641,27 @@ NewFunction::getFuncBody(const SourceManager &SM) const { return *NewBody + (SemicolonPolicy.isNeededInExtractedFunction() ? ";" : ""); } -std::string NewFunction::Parameter::render(const DeclContext *Context) const { +std::string NewFunction::Parameter::render(const DeclContext &Context) const { // Passing Name as printType()'s declarator placeholder (rather than just // appending it after the printed type) lets clang's own type printer // place it correctly for declarator syntax that doesn't simply put the // name after the type, e.g. a function pointer (`void (*name)(int)`, not - // `void (*)(int) name`) or an array (`int name[5]`, not `int[5] name`). - // For a reference, building the actual reference QualType (rather than - // just prepending "&" to the placeholder string) is what lets the - // printer correctly parenthesize a reference-to-array - // (`int (&name)[5]`, not the invalid `int &name[5]`). + // `void (*)(int) name`). This only threads correctly through an outer + // pointer or reference, though, not through a bare array type on its + // own (confirmed empirically: it prints as `int[5] name`, not the + // intended `int name[5]`) -- harmless here, since createParameters + // never leaves TypeInfo as a bare array: it's always either decayed + // to a pointer (in C) or wrapped in the reference built just below + // (in C++). For that reference case, building the actual reference + // QualType (rather than just prepending "&" to the placeholder + // string) is what lets the printer correctly parenthesize a + // reference-to-array (`int (&name)[5]`, not the invalid + // `int &name[5]`). QualType RenderedType = Kind == ParamPassKind::Reference - ? Context->getParentASTContext().getLValueReferenceType(TypeInfo) + ? Context.getParentASTContext().getLValueReferenceType(TypeInfo) : TypeInfo; - return printType(RenderedType, *Context, Name); + return printType(RenderedType, Context, Name); } // Stores captured information about Extraction Zone. @@ -1198,6 +1204,22 @@ bool createParameters(NewFunction &ExtractedFunc, } } + // A reference-to-array parameter's declared type always spells out + // every dimension's bound, including the outermost one (unlike a + // decayed C pointer, which drops it). For a variable-length array, + // that bound is an arbitrary expression -- typically another + // variable's name -- that isn't in scope in the extracted + // function's declaration, and even making it an explicit preceding + // parameter doesn't help: the two don't line up at the call site + // (the callee's declared bound and the caller's concrete array type + // don't reference-bind just because both happen to be named the + // same). So this is rejected unconditionally, in both languages -- + // C still gets a chance to extract it below, by decaying to a + // pointer instead of keeping a reference. + if (Kind == ParamPassKind::Reference && TypeInfo->isArrayType() && + TypeInfo->isVariablyModifiedType() && LangOpts.CPlusPlus) + return false; + // C has no references. A parameter that would otherwise need one // becomes a real pointer instead: the call site takes the address of // the original variable explicitly, and every use of it in the @@ -1205,9 +1227,10 @@ bool createParameters(NewFunction &ExtractedFunc, // NewFunction::getFuncBody). Array types are the exception: they decay // to a pointer on their own wherever they're used, so no rewriting or // address-of is needed for them at all -- except that TypeInfo itself - // must be decayed too (NewFunction::Parameter::render() has no special - // case for array declarator syntax, so leaving it as an array type - // would print as the uncompilable `int[5] name`). + // must still be decayed explicitly (confirmed empirically): unlike a + // pointer or reference, printType()'s declarator placeholder doesn't + // thread correctly through a *bare* array type on its own, printing + // the uncompilable `int[5] name` instead of `int name[5]`. if (Kind == ParamPassKind::Reference && !LangOpts.CPlusPlus) { if (TypeInfo->isArrayType()) { // A decayed pointer no longer reports the array's own size, diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp index 9857b8e64b1f99..a5e2e604400ed4 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp @@ -1263,19 +1263,39 @@ TEST_F(ExtractFunctionTest, CFileRejectMultiDimensionVariableLengthArray) { "fail: Too complex to extract."); } -TEST_F(ExtractFunctionTest, CXXVariableLengthArrayByReference) { - // In C++ (where clang also supports VLAs as a GNU extension), this - // isn't a problem at all: an unmutated array always stays a - // reference there (see ConstParametersScalarsByValue's array case), - // never decaying to a pointer, so there's no separate parameter that - // needs to agree with the array's own bound. +TEST_F(ExtractFunctionTest, CXXRejectVariableLengthArray) { + // Clang also supports VLAs in C++ as a GNU extension. An unmutated + // array capture is passed by reference there (see + // ConstParametersScalarsByValue's plain, fixed-size array case, + // which that's still fine for) rather than decaying it to a pointer + // the way C does -- but a reference's declared type spells out every + // dimension's bound, including the outermost one: + // `void extracted(int (&A)[2][N])` looks plausible, and even + // compiles, but a call to it doesn't -- `N` in the parameter list + // isn't the same `N` as at the call site, and a reference-to-array + // parameter can't be bound by just matching the spelling. Context = File; - EXPECT_THAT(apply(R"cpp( + EXPECT_EQ(apply(R"cpp( void foo(int N) { int A[2][N]; [[A[0][0] = 1;]] })cpp"), - HasSubstr("extracted(int (&A)[2][N])")); + "fail: Too complex to extract."); +} + +TEST_F(ExtractFunctionTest, CXXRejectSingleDimensionVariableLengthArray) { + // Unlike in C (where a single-dimension VLA decays to a pointer that + // no longer mentions the bound at all, see + // CFileAllowSingleDimensionVariableLengthArray), a C++ reference's + // declared type keeps the bound regardless of dimension count, so + // even this simpler case has the same hazard as the 2D one above. + Context = File; + EXPECT_EQ(apply(R"cpp( + void foo(int N) { + int A[N]; + [[A[0] = 1;]] + })cpp"), + "fail: Too complex to extract."); } } // namespace >From 513bb7a54479d50a238025e425a0793fc0288ec1 Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Wed, 14 Oct 2026 13:00:41 +0200 Subject: [PATCH 5/6] [clangd] Extract to function: Reject pointers to variable-length arrays A pointer to a VLA (`int (*)[N]`) isn't an array type itself, so the existing array-specific VLA checks don't apply to it. But it has the same hazard as a captured VLA directly: the pointee's bound is baked into the type at every level, so it keeps mentioning a name that isn't in scope in the extracted function's declaration regardless of how the pointer itself ends up passed -- by value, by reference, or, in C, by a further pointer indirection if mutated. Unlike a plain array, there's no decay step that removes the bound here, so this is rejected unconditionally rather than narrowed to a specific dimension. Assisted-by: Claude Code --- .../refactor/tweaks/ExtractFunction.cpp | 13 +++++++ .../unittests/tweaks/ExtractFunctionTests.cpp | 34 +++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp index 10ed822518401a..aeaf094809ae4d 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -1181,6 +1181,19 @@ bool createParameters(NewFunction &ExtractedFunc, // Parameter qualifiers are same as the Decl's qualifiers. QualType FullTypeInfo = VD->getType(); QualType TypeInfo = FullTypeInfo.getNonReferenceType(); + + // A variably-modified type that isn't itself a plain array (e.g. a + // pointer to a VLA, `int (*)[N]`) has no path below that handles it + // safely: its bound is baked into the type at every level, so it + // keeps mentioning `N` whether passed by value, by reference, or + // (in C) by pointer -- const-qualifying or wrapping it in another + // pointer/reference doesn't remove the bound from the type the way + // decaying an array's own outer dimension does. A plain array gets + // a narrower, more precise check further down instead, since its + // outer dimension specifically isn't a problem once decayed. + if (TypeInfo->isVariablyModifiedType() && !TypeInfo->isArrayType()) + return false; + // FIXME: check if parameter will be a non l-value reference. ParamPassKind Kind = ParamPassKind::Reference; if (!DeclInfo.IsPossiblyMutated) { diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp index a5e2e604400ed4..fd2472655af378 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp @@ -1298,6 +1298,40 @@ TEST_F(ExtractFunctionTest, CXXRejectSingleDimensionVariableLengthArray) { "fail: Too complex to extract."); } +TEST_F(ExtractFunctionTest, CXXRejectPointerToVariableLengthArray) { + // A pointer to a VLA (`int (*)[N]`) isn't an array type itself, so + // the array-specific VLA checks above don't apply to it -- but it + // has the exact same hazard: its pointee's bound is baked into the + // type regardless of how the pointer itself ends up passed (by + // value, by reference, or, in C, by a further pointer indirection), + // so it can't be printed without mentioning a name that isn't in + // scope. + Context = File; + EXPECT_EQ(apply(R"cpp( + void foo(int N) { + int (*A)[N]; + [[(*A)[0] = 1;]] + })cpp"), + "fail: Too complex to extract."); +} + +TEST_F(ExtractFunctionTest, CFileRejectMutatedPointerToVariableLengthArray) { + // Same hazard as above, but in C, and with the pointer additionally + // mutated (so it would otherwise go through the C pointer-adapter + // path instead of being passed by value): the bound is baked into + // the pointee type regardless, so `int (**A)[N]` is just as broken + // as the unmutated `int (*A)[N]` would have been. + FileName = "a.c"; + Context = File; + EXPECT_EQ(apply(R"cpp( + void foo(int N) { + int (*A)[N]; + A = 0; + [[(*A)[0] = 1;]] + })cpp"), + "fail: Too complex to extract."); +} + } // namespace } // namespace clangd } // namespace clang >From da372bf2374ce697312f16da6df1a1c056b081f4 Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Wed, 14 Oct 2026 13:29:42 +0200 Subject: [PATCH 6/6] [clangd] Extract to function: Combine the two variably-modified-type checks The pointer-to-VLA rejection and the C++ array-VLA rejection were two separate checks for what's really one condition: reject a variably- modified type unless it's a plain array being extracted in C, which gets a narrower, more precise check (based on the decayed type) a bit further down instead. Assisted-by: Claude Code --- .../refactor/tweaks/ExtractFunction.cpp | 37 ++++++------------- 1 file changed, 11 insertions(+), 26 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp index aeaf094809ae4d..de9a5fa45b9c06 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -1182,16 +1182,17 @@ bool createParameters(NewFunction &ExtractedFunc, QualType FullTypeInfo = VD->getType(); QualType TypeInfo = FullTypeInfo.getNonReferenceType(); - // A variably-modified type that isn't itself a plain array (e.g. a - // pointer to a VLA, `int (*)[N]`) has no path below that handles it - // safely: its bound is baked into the type at every level, so it - // keeps mentioning `N` whether passed by value, by reference, or - // (in C) by pointer -- const-qualifying or wrapping it in another - // pointer/reference doesn't remove the bound from the type the way - // decaying an array's own outer dimension does. A plain array gets - // a narrower, more precise check further down instead, since its - // outer dimension specifically isn't a problem once decayed. - if (TypeInfo->isVariablyModifiedType() && !TypeInfo->isArrayType()) + // A variably-modified type is rejected outright unless it's a plain + // array being extracted in C: a reference's (or any other wrapping, + // e.g. a further pointer indirection's) declared type still spells + // out the bound at every level, so it keeps mentioning a name + // (typically some other variable's) that isn't in scope in the + // extracted function's declaration. A plain array in C is the one + // case this doesn't apply to as-is, since its outer dimension is + // specifically not a problem once decayed to a pointer -- that + // gets a narrower, more precise check further down instead. + if (TypeInfo->isVariablyModifiedType() && + (LangOpts.CPlusPlus || !TypeInfo->isArrayType())) return false; // FIXME: check if parameter will be a non l-value reference. @@ -1217,22 +1218,6 @@ bool createParameters(NewFunction &ExtractedFunc, } } - // A reference-to-array parameter's declared type always spells out - // every dimension's bound, including the outermost one (unlike a - // decayed C pointer, which drops it). For a variable-length array, - // that bound is an arbitrary expression -- typically another - // variable's name -- that isn't in scope in the extracted - // function's declaration, and even making it an explicit preceding - // parameter doesn't help: the two don't line up at the call site - // (the callee's declared bound and the caller's concrete array type - // don't reference-bind just because both happen to be named the - // same). So this is rejected unconditionally, in both languages -- - // C still gets a chance to extract it below, by decaying to a - // pointer instead of keeping a reference. - if (Kind == ParamPassKind::Reference && TypeInfo->isArrayType() && - TypeInfo->isVariablyModifiedType() && LangOpts.CPlusPlus) - return false; - // C has no references. A parameter that would otherwise need one // becomes a real pointer instead: the call site takes the address of // the original variable explicitly, and every use of it in the _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
