https://github.com/ckandeler updated https://github.com/llvm/llvm-project/pull/236113
>From c6a2f08b4f0a3396a62fe5780f9e6ebe04f5bc25 Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Tue, 27 Oct 2026 13:26:54 +0100 Subject: [PATCH] [clangd] Extract to function: Reject code that uses a function-local type The extracted function is always placed before the enclosing function, so a type declared inside that function (e.g. a local struct, or a local enum/typedef) isn't visible there. Previously this wasn't checked at all: extracting e.g. void f() { struct S { int A; }; S X = S{1}; } produced `void extracted() { S X = S{1}; }` placed before `f`, where `S` doesn't exist -- broken code. Detected two ways, reusing the existing capture-analysis visitor (CapturedZoneInfo) rather than adding a separate pass: - An explicit type mention anywhere inside the zone (a declarator, a cast, sizeof(T), etc.) is caught directly via VisitTypeLoc. - A captured variable's own type can be function-local without any explicit mention of it appearing inside the zone at all (its declaration is outside); this is instead caught by checking the extracted function's actual generated parameter and return types after they're computed. Deliberately does *not* check every expression's own type as a third mechanism: that would over-reject a local lambda's closure type (whose DeclContext is the enclosing function too), even though an `auto`-declared lambda variable never needs to spell that type out anywhere. The type-unwrapping helper (referencesFunctionLocalType) looks through pointers, references, arrays, template arguments, member-pointer types (both the pointee and the pointed-into class), and function types' return/parameter types, to catch a local type nested arbitrarily deeply in a signature. Assisted-by: Claude Code --- .../refactor/tweaks/ExtractFunction.cpp | 93 ++++++++++++++++++- .../unittests/tweaks/ExtractFunctionTests.cpp | 92 ++++++++++++++++++ 2 files changed, 183 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp index 7b60b95bd61f23..3bb49e249df649 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -846,7 +846,7 @@ struct CapturedZoneInfo { bool HasUnsafeTypeQueryUseInZone = false; DeclInformation(const Decl *TheDecl, ZoneRelative DeclaredIn, unsigned DeclIndex) - : TheDecl(TheDecl), DeclaredIn(DeclaredIn), DeclIndex(DeclIndex){}; + : TheDecl(TheDecl), DeclaredIn(DeclaredIn), DeclIndex(DeclIndex) {}; // Marks the occurence of a reference for this declaration void markOccurence(ZoneRelative ReferenceLoc); }; @@ -857,8 +857,14 @@ struct CapturedZoneInfo { // Control flow is broken if we are extracting a break/continue without a // corresponding parent loop/switch bool BrokenControlFlow = false; + // Whether an explicit type mention inside the zone (a declarator, a + // cast, sizeof(T), etc.) refers to a struct/class/union, enum, or + // typedef declared inside the enclosing function. Such a type isn't + // visible at the extracted function's insertion point, which is + // always before the enclosing function -- see + // referencesFunctionLocalType. + bool ReferencesFunctionLocalType = false; // FIXME: capture TypeAliasDecl and UsingDirectiveDecl - // FIXME: Capture type information as well. DeclInformation *createDeclInfo(const Decl *D, ZoneRelative RelativeLoc); DeclInformation *getDeclInfoFor(const Decl *D); }; @@ -962,6 +968,55 @@ void collectUnderlyingDecls(const Expr *E, } } +// Whether T refers, anywhere within it (looking through pointers, +// references, arrays, and template arguments), to a struct/class/union, +// enum, or typedef declared inside some function. Such a type isn't visible +// outside that function, so it can't be used in the extracted +// function's signature or body, which always end up placed before the +// enclosing function. +// +bool referencesFunctionLocalType(QualType T) { + if (T.isNull()) + return false; + if (const TagDecl *TD = T->getAsTagDecl()) + if (TD->getDeclContext()->isFunctionOrMethod()) + return true; + if (const auto *PT = T->getAs<PointerType>()) + return referencesFunctionLocalType(PT->getPointeeType()); + if (const auto *RT = T->getAs<ReferenceType>()) + return referencesFunctionLocalType(RT->getPointeeType()); + if (const auto *AT = T->getAsArrayTypeUnsafe()) + return referencesFunctionLocalType(AT->getElementType()); + if (const auto *TST = T->getAs<TemplateSpecializationType>()) + for (const TemplateArgument &Arg : TST->template_arguments()) + if (Arg.getKind() == TemplateArgument::Type && + referencesFunctionLocalType(Arg.getAsType())) + return true; + // A pointer-to-member's own class (`S::*`, for either a data or a + // member-function member) must be nameable too, same as the member's + // own type, which getPointeeType() below covers uniformly for both + // kinds (a plain type for a data member, a function type for a + // member-function one). + if (const auto *MPT = T->getAs<MemberPointerType>()) { + if (const CXXRecordDecl *RD = MPT->getMostRecentCXXRecordDecl()) + if (RD->getDeclContext()->isFunctionOrMethod()) + return true; + return referencesFunctionLocalType(MPT->getPointeeType()); + } + // A function type itself (as opposed to a function *pointer*, whose + // pointee is handled above): its return type, and -- if the parameter + // list isn't just `()`/unprototyped -- every parameter's type. + if (const auto *FT = T->getAs<FunctionType>()) { + if (referencesFunctionLocalType(FT->getReturnType())) + return true; + if (const auto *FPT = dyn_cast<FunctionProtoType>(FT)) + for (QualType ParamTy : FPT->getParamTypes()) + if (referencesFunctionLocalType(ParamTy)) + return true; + } + return false; +} + // Captures information from Extraction Zone CapturedZoneInfo captureZoneInfo(const ExtractionZone &ExtZone) { // We use the ASTVisitor instead of using the selection tree since we need to @@ -1009,6 +1064,23 @@ CapturedZoneInfo captureZoneInfo(const ExtractionZone &ExtZone) { return true; } + // Catches a local type named explicitly -- a declarator, a cast, or + // sizeof(T) -- anywhere inside the zone. Deliberately doesn't also + // check every Expr's own type: that would over-trigger on e.g. a + // local lambda's closure type (its DeclContext is the enclosing + // function too), even though an `auto`-declared lambda variable + // never needs to spell that type out anywhere. A *captured* + // variable (declared outside the zone) of a local type is instead + // caught separately, in getExtractedFunction, by checking the + // actual generated parameter/return types: that's the only place + // such a type would actually need to be spelled out. + bool VisitTypeLoc(TypeLoc TL) { + if (CurrentLocation == ZoneRelative::Inside && + referencesFunctionLocalType(TL.getType())) + Info.ReferencesFunctionLocalType = true; + return true; + } + // Set by VisitMemberExpr right before traversing into a non-arrow // MemberExpr's base, when that base is (possibly parenthesized) // exactly a DeclRefExpr: since that base is always traversed @@ -1484,6 +1556,12 @@ llvm::Expected<NewFunction> getExtractedFunction(ExtractionZone &ExtZone, if (CapturedInfo.BrokenControlFlow) return error("Cannot extract break/continue without corresponding " "loop/switch statement."); + // The extracted function is always placed before the enclosing + // function, where a type declared inside it (e.g. a local struct) + // isn't visible. + if (CapturedInfo.ReferencesFunctionLocalType) + return error("Cannot extract code that uses a type local to the " + "enclosing function."); NewFunction ExtractedFunc(getSemicolonPolicy(ExtZone, SM, LangOpts), &LangOpts); @@ -1529,6 +1607,17 @@ llvm::Expected<NewFunction> getExtractedFunction(ExtractionZone &ExtZone, !generateReturnProperties(ExtractedFunc, *ExtZone.EnclosingFunction, CapturedInfo)) return error("Too complex to extract."); + // A captured variable's own type can be function-local (e.g. a local + // struct) without any explicit mention of that type ever appearing + // inside the zone itself (checked above): the type still has to be + // spelled out here, in the parameter it becomes. + if (referencesFunctionLocalType(ExtractedFunc.ReturnType) || + llvm::any_of(ExtractedFunc.Parameters, + [](const NewFunction::Parameter &P) { + return referencesFunctionLocalType(P.TypeInfo); + })) + return error("Cannot extract code that uses a type local to the " + "enclosing function."); return ExtractedFunc; } diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp index 981373b8933474..00cda892cbd869 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp @@ -1618,6 +1618,98 @@ TEST_F(ExtractFunctionTest, RejectPureForwardingStructuredBinding) { "unavailable"); } +TEST_F(ExtractFunctionTest, RejectLocalTypeConstructedInline) { + Context = File; + // The extracted function always ends up placed before the enclosing + // function, where a type declared inside it isn't visible -- so this + // can't be extracted at all, not even as a pure rename. + EXPECT_EQ(apply(R"cpp( + void f() { + struct S { int A; }; + [[S X = S{1};]] + })cpp"), + "fail: Cannot extract code that uses a type local to the " + "enclosing function."); +} + +TEST_F(ExtractFunctionTest, RejectLocalTypeThroughCapturedVariable) { + Context = File; + // Same hazard, but reached through a captured variable's type rather + // than a type written out directly in the zone: `S` never appears as + // its own TypeLoc within the selection at all. Caught by checking the + // extracted function's own generated parameter type instead (`X` + // becomes a parameter of type `S`), in getExtractedFunction. + EXPECT_EQ(apply(R"cpp( + void foo(int); + void f() { + struct S { int A; }; + S X; + [[foo(X.A);]] + })cpp"), + "fail: Cannot extract code that uses a type local to the " + "enclosing function."); +} + +TEST_F(ExtractFunctionTest, RejectLocalTypeNamedOnly) { + Context = File; + // sizeof(S)'s operand is a bare type name, not an expression: caught + // by VisitTypeLoc, same as the declarator in + // RejectLocalTypeConstructedInline. + EXPECT_EQ(apply(R"cpp( + void foo(unsigned long); + void f() { + struct S { int A; }; + [[foo(sizeof(S));]] + })cpp"), + "fail: Cannot extract code that uses a type local to the " + "enclosing function."); +} + +TEST_F(ExtractFunctionTest, OfferDespiteEnclosingClassType) { + Context = File; + // A method's own enclosing class isn't function-local (its + // DeclContext is the namespace/TU, not a function), so referencing it + // -- unlike a type declared inside the method's own body -- is fine. + EXPECT_THAT(apply(R"cpp( + struct S { + int A; + void f() { [[S X = S{1};]] } + }; + )cpp"), + HasSubstr("extracted")); +} + +TEST_F(ExtractFunctionTest, RejectLocalTypeInFunctionPointerSignature) { + Context = File; + // S never appears as its own TypeLoc anywhere in the zone -- it's only + // reachable through captured fp's own declared type (a pointer to a + // function returning S). The literal argument keeps this from being + // suppressed as a pure forwarding wrapper instead (see + // RejectPureForwarding* above), which would otherwise mask this check. + EXPECT_EQ(apply(R"cpp( + void f() { + struct S { int A; }; + S (*fp)(int); + [[fp(1);]] + })cpp"), + "fail: Cannot extract code that uses a type local to the " + "enclosing function."); +} + +TEST_F(ExtractFunctionTest, RejectLocalTypeThroughMemberPointer) { + Context = File; + EXPECT_EQ(apply(R"cpp( + void use(int); + void f() { + struct S { int A; }; + S s; + int S::*pm = &S::A; + [[use(s.*pm);]] + })cpp"), + "fail: Cannot extract code that uses a type local to the " + "enclosing function."); +} + } // namespace } // namespace clangd } // namespace clang _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
