https://github.com/max-winkler created https://github.com/llvm/llvm-project/pull/237866
Clangd's OverridePureVirtuals tweak auto-generated overrides of pure virtual functions but omitted the nested name specifiers required for the code to complile. The reimplementation of `generateOverrideString` now generates the override string using non-default configuration of a PrintingPolicy. The nested name specifiers for parameters and return value, that are not enclosing the derived class, are printed correctly, while name specifiers enclosing the derived class are omitted. New printing functionality makes the `removePureVirtualSyntax` which removes `virtual` and `= 0` obsolete, so it is removed. Tweak triggered an assertion for pure virtuals without name (e.g. operator[]), which is also fixed. Add new unittests for nested name specifiers and methods without names (operator[], etc.) in OverridePureVirtualsTest.cpp. Fixes clangd/clangd#2706 >From f15e83eb1e31059fec2ea02ac8f653f1dbbda1ae Mon Sep 17 00:00:00 2001 From: Max Winkler <[email protected]> Date: Wed, 28 Oct 2026 16:57:38 +0100 Subject: [PATCH] [clangd] OverridePureVirtuals with correct nested name specifiers Clangd's OverridePureVirtuals tweak auto-generated overrides of pure virtual functions but omitted the nested name specifiers required for the code to complile. The reimplementation of `generateOverrideString` now generates the override string using non-default configuration of a PrintingPolicy. The nested name specifiers for parameters and return value, that are not enclosing the derived class, are printed correctly, while name specifiers enclosing the derived class are omitted. New printing functionality makes the `removePureVirtualSyntax` which removes `virtual` and `= 0` obsolete, so it is removed. Tweak triggered an assertion for pure virtuals without name (e.g. operator[]), which is also fixed. Add new unittests for nested name specifiers and methods without names (operator[], etc.) in OverridePureVirtualsTest.cpp. Fixes clangd/clangd#2706 --- .../refactor/tweaks/OverridePureVirtuals.cpp | 137 +++++++---- .../tweaks/OverridePureVirtualsTests.cpp | 225 +++++++++++++++++- 2 files changed, 315 insertions(+), 47 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp index b557066d979f5..4d2f544c3a4ab 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/OverridePureVirtuals.cpp @@ -35,8 +35,10 @@ // - Each generated stub includes the `override` keyword, a `// TODO:` // comment, and a `static_assert(false, ...)` to force a compile-time // error if the method remains unimplemented. -// - The base method's signature is adjusted (e.g., `virtual` and `= 0` -// are removed for the override). +// - The base method's signature is adjusted for the override (e.g., +// `virtual` and `= 0` are removed, and all nested name specifiers +// that do not enclose the derived class are added to the types of +// parameters and return value). // // 4. Code Action Provided: // - A single code action titled "Override pure virtual methods" is offered. @@ -92,41 +94,6 @@ namespace clang { namespace clangd { namespace { -// This function removes the "virtual" and the "= 0" at the end; -// e.g.: -// "virtual void foo(int var = 0) = 0" // input. -// "void foo(int var = 0)" // output. -std::string removePureVirtualSyntax(const std::string &MethodDecl, - const LangOptions &LangOpts) { - assert(!MethodDecl.empty()); - - TokenStream TS = lex(MethodDecl, LangOpts); - - std::string DeclString; - for (const clangd::Token &Tk : TS.tokens()) { - if (Tk.Kind == clang::tok::raw_identifier && Tk.text() == "virtual") - continue; - - // If the ending two tokens are "= 0", we break here and we already have the - // method's string without the pure virtual syntax. - const auto &Next = Tk.next(); - if (Next.next().Kind == tok::eof && Tk.Kind == clang::tok::equal && - Next.text() == "0") - break; - - DeclString += Tk.text(); - if (Tk.Kind != tok::l_paren && Next.Kind != tok::comma && - Next.Kind != tok::r_paren && Next.Kind != tok::l_paren && - Tk.Kind != tok::coloncolon && Next.Kind != tok::coloncolon) - DeclString += ' '; - } - // Trim the last whitespace. - if (DeclString.back() == ' ') - DeclString.pop_back(); - - return DeclString; -} - class OverridePureVirtuals final : public Tweak { public: const char *id() const final; // defined by REGISTER_TWEAK. @@ -264,30 +231,109 @@ void OverridePureVirtuals::collectMissingPureVirtuals() { } } -std::string generateOverrideString(const CXXMethodDecl *Method, - const LangOptions &LangOpts) { +std::string generateOverrideString(const clang::CXXRecordDecl *CurrentDeclDef, + const CXXMethodDecl *Method) { std::string MethodDecl; auto OS = llvm::raw_string_ostream(MethodDecl); - Method->print(OS); + + // Create policy that prints all nested name specifiers that do not enclose + // the class to be modified. + PrintingPolicy PP = Method->getASTContext().getPrintingPolicy(); + PP.FullyQualifiedName = true; + + class PrintCB : public PrintingCallbacks { + public: + PrintCB(const DeclContext *CurContext) : CurContext(CurContext) {} + virtual ~PrintCB() {} + bool isScopeVisible(const DeclContext *DC) const override { + return DC->Encloses(CurContext); + } + + private: + const DeclContext *CurContext; + }; + + PrintCB PCB(CurrentDeclDef); + PP.Callbacks = &PCB; + + // Get function prototype + QualType Ty = Method->getType(); + const FunctionProtoType *FT = Ty->getAs<FunctionProtoType>(); + + // Relevant specifiers + if (Method->isInlineSpecified()) + OS << "inline "; + if (Method->isConstexprSpecified()) + OS << "constexpr "; + if (Method->isConsteval()) + OS << "consteval "; + + // Return type + if (FT->hasTrailingReturn()) + OS << "auto "; + else { + FT->getReturnType().print(OS, PP); + OS << " "; + } + + // Unqualified method name + OS << Method->getNameAsString(); + + // Parameter list + OS << "("; + for (unsigned i = 0; i < Method->getNumParams(); ++i) { + if (i > 0) + OS << ", "; + Method->getParamDecl(i)->print(OS, PP); + } + OS << ")"; + + // CV Qualifiers + if (FT->isConst()) + OS << " const"; + if (FT->isVolatile()) + OS << " volatile"; + + // Reference Qualifiers + switch (FT->getRefQualifier()) { + case RQ_None: + break; + case RQ_LValue: + OS << " &"; + break; + case RQ_RValue: + OS << " &&"; + break; + } + + // Exception specifier + if (isNoexceptExceptionSpec(FT->getExceptionSpecType())) + OS << " noexcept"; + + // Trailing return type + if (FT->hasTrailingReturn()) { + OS << " -> "; + FT->getReturnType().print(OS, PP); + } return llvm::formatv( "\n {0} override {{\n" " // TODO: Implement this pure virtual method.\n" " static_assert(false, \"Method `{1}` is not implemented.\");\n" " }", - removePureVirtualSyntax(MethodDecl, LangOpts), Method->getName()) + MethodDecl, Method->getNameAsString()) .str(); } // Free function to generate the string for a group of method overrides. std::string generateOverridesStringForGroup( - llvm::SmallVector<const CXXMethodDecl *> Methods, - const LangOptions &LangOpts) { + const clang::CXXRecordDecl *CurrentDeclDef, + llvm::SmallVector<const CXXMethodDecl *> Methods) { llvm::SmallVector<std::string> MethodsString; MethodsString.reserve(Methods.size()); for (const CXXMethodDecl *Method : Methods) { - MethodsString.emplace_back(generateOverrideString(Method, LangOpts)); + MethodsString.emplace_back(generateOverrideString(CurrentDeclDef, Method)); } return llvm::join(MethodsString, "\n") + '\n'; @@ -302,7 +348,6 @@ Expected<Tweak::Effect> OverridePureVirtuals::apply(const Selection &Sel) { assert(!MissingMethodsByAccess.empty()); const auto &SM = Sel.AST->getSourceManager(); - const auto &LangOpts = Sel.AST->getLangOpts(); tooling::Replacements EditReplacements; // Stores text for new access specifier sections that are not already present @@ -316,7 +361,7 @@ Expected<Tweak::Effect> OverridePureVirtuals::apply(const Selection &Sel) { assert(!Methods.empty()); std::string MethodsGroupString = - generateOverridesStringForGroup(Methods, LangOpts); + generateOverridesStringForGroup(CurrentDeclDef, Methods); auto *ExistingSpecLocIter = AccessSpecifierLocations.find(AS); bool ASExists = ExistingSpecLocIter != AccessSpecifierLocations.end(); diff --git a/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp index 72095ab2f5982..3a64c5f6ba1ad 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/OverridePureVirtualsTests.cpp @@ -102,7 +102,7 @@ class Derived : public Base { static_assert(false, "Method `F1` is not implemented."); } - void F2(int P1, const int & P2) override { + void F2(int P1, const int &P2) override { // TODO: Implement this pure virtual method. static_assert(false, "Method `F2` is not implemented."); } @@ -754,6 +754,229 @@ class D : public B { EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied; } +TEST_F(OverridePureVirtualsTests, NestedQualifiedNames) { + constexpr auto Before = R"cpp( +namespace A { +namespace B { +class S {}; +class Base { +public: + class T {}; + virtual S bar(S &s) = 0; + virtual T foo(T &t) = 0; +}; +} + +namespace C { +class D^eriv : B::Base {}; +} +} +)cpp"; + + constexpr auto Expected = R"cpp( +namespace A { +namespace B { +class S {}; +class Base { +public: + class T {}; + virtual S bar(S &s) = 0; + virtual T foo(T &t) = 0; +}; +} + +namespace C { +class Deriv : B::Base { +public: + B::S bar(B::S &s) override { + // TODO: Implement this pure virtual method. + static_assert(false, "Method `bar` is not implemented."); + } + + B::Base::T foo(B::Base::T &t) override { + // TODO: Implement this pure virtual method. + static_assert(false, "Method `foo` is not implemented."); + } +}; +} +} +)cpp"; + auto Applied = apply(Before); + EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied; +} + +TEST_F(OverridePureVirtualsTests, NestedQualifiedNamesWithTrailingReturnTypes) { + constexpr auto Before = R"cpp( +namespace A { +namespace B { +class S {}; +class Base { +public: + class T {}; + virtual auto bar(S &s) -> S = 0; + virtual auto foo(T &t) -> T = 0; +}; +} + +namespace C { +class D^eriv : B::Base {}; +} +} +)cpp"; + + constexpr auto Expected = R"cpp( +namespace A { +namespace B { +class S {}; +class Base { +public: + class T {}; + virtual auto bar(S &s) -> S = 0; + virtual auto foo(T &t) -> T = 0; +}; +} + +namespace C { +class Deriv : B::Base { +public: + auto bar(B::S &s) -> B::S override { + // TODO: Implement this pure virtual method. + static_assert(false, "Method `bar` is not implemented."); + } + + auto foo(B::Base::T &t) -> B::Base::T override { + // TODO: Implement this pure virtual method. + static_assert(false, "Method `foo` is not implemented."); + } +}; +} +} +)cpp"; + auto Applied = apply(Before); + EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied; +} + +TEST_F(OverridePureVirtualsTests, Operators) { + constexpr auto Before = R"cpp( +class B { +public: + virtual ~B() = default; + virtual float & operator()(int i) = 0; + virtual const float & operator()(int i) const = 0; + virtual float & operator[](int i) = 0; + virtual bool operator!() = 0; + virtual float operator-() = 0; + virtual B & operator++() = 0; + virtual B operator++(int) = 0; + virtual B operator+(const B &o) const = 0; + virtual B & operator=(const B &o) = 0; + virtual B & operator+=(const B &o) = 0; + virtual B & operator=(B &&o) noexcept = 0; + virtual bool operator==(const B &o) const = 0; + virtual B* operator->() const = 0; + virtual B & operator*() = 0; +}; + +class ^D : public B {}; +)cpp"; + + constexpr auto Expected = R"cpp( +class B { +public: + virtual ~B() = default; + virtual float & operator()(int i) = 0; + virtual const float & operator()(int i) const = 0; + virtual float & operator[](int i) = 0; + virtual bool operator!() = 0; + virtual float operator-() = 0; + virtual B & operator++() = 0; + virtual B operator++(int) = 0; + virtual B operator+(const B &o) const = 0; + virtual B & operator=(const B &o) = 0; + virtual B & operator+=(const B &o) = 0; + virtual B & operator=(B &&o) noexcept = 0; + virtual bool operator==(const B &o) const = 0; + virtual B* operator->() const = 0; + virtual B & operator*() = 0; +}; + +class D : public B { +public: + float & operator()(int i) override { + // TODO: Implement this pure virtual method. + static_assert(false, "Method `operator()` is not implemented."); + } + + const float & operator()(int i) const override { + // TODO: Implement this pure virtual method. + static_assert(false, "Method `operator()` is not implemented."); + } + + float & operator[](int i) override { + // TODO: Implement this pure virtual method. + static_assert(false, "Method `operator[]` is not implemented."); + } + + bool operator!() override { + // TODO: Implement this pure virtual method. + static_assert(false, "Method `operator!` is not implemented."); + } + + float operator-() override { + // TODO: Implement this pure virtual method. + static_assert(false, "Method `operator-` is not implemented."); + } + + B & operator++() override { + // TODO: Implement this pure virtual method. + static_assert(false, "Method `operator++` is not implemented."); + } + + B operator++(int) override { + // TODO: Implement this pure virtual method. + static_assert(false, "Method `operator++` is not implemented."); + } + + B operator+(const B &o) const override { + // TODO: Implement this pure virtual method. + static_assert(false, "Method `operator+` is not implemented."); + } + + B & operator=(const B &o) override { + // TODO: Implement this pure virtual method. + static_assert(false, "Method `operator=` is not implemented."); + } + + B & operator+=(const B &o) override { + // TODO: Implement this pure virtual method. + static_assert(false, "Method `operator+=` is not implemented."); + } + + B & operator=(B &&o) noexcept override { + // TODO: Implement this pure virtual method. + static_assert(false, "Method `operator=` is not implemented."); + } + + bool operator==(const B &o) const override { + // TODO: Implement this pure virtual method. + static_assert(false, "Method `operator==` is not implemented."); + } + + B * operator->() const override { + // TODO: Implement this pure virtual method. + static_assert(false, "Method `operator->` is not implemented."); + } + + B & operator*() override { + // TODO: Implement this pure virtual method. + static_assert(false, "Method `operator*` is not implemented."); + } +}; +)cpp"; + auto Applied = apply(Before); + EXPECT_EQ(Expected, Applied) << "Applied result:\n" << Applied; +} + } // namespace } // namespace clangd } // namespace clang _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
