https://github.com/ckandeler created https://github.com/llvm/llvm-project/pull/128164
If possible, put the definition next to the definition of an adjacent declaration. For example: struct S { void f^oo1() {} void foo2(); void f^oo3() {} }; // S::foo1() goes here void S::foo2() {} // S::foo3() goes here >From 7285b4a42d87681801dc3c950614d9f9febfe8a1 Mon Sep 17 00:00:00 2001 From: Christian Kandeler <christian.kande...@qt.io> Date: Mon, 25 Nov 2024 17:39:21 +0100 Subject: [PATCH] [clangd] Find better insertion locations in DefineOutline tweak If possible, put the definition next to the definition of an adjacent declaration. For example: struct S { void f^oo1() {} void foo2(); void f^oo3() {} }; // S::foo1() goes here void S::foo2() {} // S::foo3() goes here --- clang-tools-extra/clangd/SourceCode.cpp | 20 ++ clang-tools-extra/clangd/SourceCode.h | 3 + .../clangd/refactor/tweaks/DefineOutline.cpp | 290 ++++++++++++++---- .../unittests/tweaks/DefineOutlineTests.cpp | 173 +++++++++++ 4 files changed, 432 insertions(+), 54 deletions(-) diff --git a/clang-tools-extra/clangd/SourceCode.cpp b/clang-tools-extra/clangd/SourceCode.cpp index 780aaa471dc8b..043d5c4f68c5e 100644 --- a/clang-tools-extra/clangd/SourceCode.cpp +++ b/clang-tools-extra/clangd/SourceCode.cpp @@ -1217,6 +1217,26 @@ EligibleRegion getEligiblePoints(llvm::StringRef Code, return ER; } +std::string getNamespaceAtPosition(StringRef Code, const Position &Pos, + const LangOptions &LangOpts) { + std::vector<std::string> Enclosing = {""}; + parseNamespaceEvents(Code, LangOpts, [&](NamespaceEvent Event) { + if (Pos < Event.Pos) + return; + if (Event.Trigger == NamespaceEvent::UsingDirective) + return; + if (!Event.Payload.empty()) + Event.Payload.append("::"); + if (Event.Trigger == NamespaceEvent::BeginNamespace) { + Enclosing.emplace_back(std::move(Event.Payload)); + } else { + Enclosing.pop_back(); + assert(Enclosing.back() == Event.Payload); + } + }); + return Enclosing.back(); +} + bool isHeaderFile(llvm::StringRef FileName, std::optional<LangOptions> LangOpts) { // Respect the langOpts, for non-file-extension cases, e.g. standard library diff --git a/clang-tools-extra/clangd/SourceCode.h b/clang-tools-extra/clangd/SourceCode.h index 028549f659d60..78ebc7a7d3b22 100644 --- a/clang-tools-extra/clangd/SourceCode.h +++ b/clang-tools-extra/clangd/SourceCode.h @@ -309,6 +309,9 @@ EligibleRegion getEligiblePoints(llvm::StringRef Code, llvm::StringRef FullyQualifiedName, const LangOptions &LangOpts); +std::string getNamespaceAtPosition(llvm::StringRef Code, const Position &Pos, + const LangOptions &LangOpts); + struct DefinedMacro { llvm::StringRef Name; const MacroInfo *Info; diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp index e4eef228b6b99..5ff64aa946004 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "AST.h" +#include "FindSymbols.h" #include "FindTarget.h" #include "HeaderSourceSwitch.h" #include "ParsedAST.h" @@ -489,8 +490,16 @@ class DefineOutline : public Tweak { Expected<Effect> apply(const Selection &Sel) override { const SourceManager &SM = Sel.AST->getSourceManager(); - auto CCFile = SameFile ? Sel.AST->tuPath().str() - : getSourceFile(Sel.AST->tuPath(), Sel); + std::optional<Path> CCFile; + std::optional<std::pair<Symbol, RelativeInsertPos>> Anchor = + getDefinitionOfAdjacentDecl(Sel); + if (Anchor) { + CCFile = Anchor->first.Definition.FileURI; + CCFile->erase(0, 7); // "file://" + } else { + CCFile = SameFile ? Sel.AST->tuPath().str() + : getSourceFile(Sel.AST->tuPath(), Sel); + } if (!CCFile) return error("Couldn't find a suitable implementation file."); assert(Sel.FS && "FS Must be set in apply"); @@ -499,21 +508,62 @@ class DefineOutline : public Tweak { // doesn't exist? if (!Buffer) return llvm::errorCodeToError(Buffer.getError()); + + std::optional<Position> InsertionPos; + if (Anchor) { + if (auto P = getInsertionPointFromExistingDefinition( + **Buffer, Anchor->first, Anchor->second, Sel.AST)) { + InsertionPos = *P; + } + } + auto Contents = Buffer->get()->getBuffer(); - auto InsertionPoint = getInsertionPoint(Contents, Sel); - if (!InsertionPoint) - return InsertionPoint.takeError(); + + std::size_t Offset = -1; + const DeclContext *EnclosingNamespace = nullptr; + std::string EnclosingNamespaceName; + + if (InsertionPos) { + EnclosingNamespaceName = getNamespaceAtPosition(Contents, *InsertionPos, + Sel.AST->getLangOpts()); + } else if (SameFile) { + auto P = getInsertionPointInMainFile(Sel.AST); + if (!P) + return P.takeError(); + Offset = P->Offset; + EnclosingNamespace = P->EnclosingNamespace; + } else { + auto Region = getEligiblePoints( + Contents, Source->getQualifiedNameAsString(), Sel.AST->getLangOpts()); + assert(!Region.EligiblePoints.empty()); + EnclosingNamespaceName = Region.EnclosingNamespace; + InsertionPos = Region.EligiblePoints.back(); + } + + if (InsertionPos) { + auto O = positionToOffset(Contents, *InsertionPos); + if (!O) + return O.takeError(); + Offset = *O; + auto TargetContext = + findContextForNS(EnclosingNamespaceName, Source->getDeclContext()); + if (!TargetContext) + return error("define outline: couldn't find a context for target"); + EnclosingNamespace = *TargetContext; + } + + assert(Offset >= 0); + assert(EnclosingNamespace); auto FuncDef = getFunctionSourceCode( - Source, InsertionPoint->EnclosingNamespace, Sel.AST->getTokens(), + Source, EnclosingNamespace, Sel.AST->getTokens(), Sel.AST->getHeuristicResolver(), SameFile && isHeaderFile(Sel.AST->tuPath(), Sel.AST->getLangOpts())); if (!FuncDef) return FuncDef.takeError(); SourceManagerForFile SMFF(*CCFile, Contents); - const tooling::Replacement InsertFunctionDef( - *CCFile, InsertionPoint->Offset, 0, *FuncDef); + const tooling::Replacement InsertFunctionDef(*CCFile, Offset, 0, *FuncDef); auto Effect = Effect::mainFileEdit( SMFF.get(), tooling::Replacements(InsertFunctionDef)); if (!Effect) @@ -548,59 +598,191 @@ class DefineOutline : public Tweak { return std::move(*Effect); } - // Returns the most natural insertion point for \p QualifiedName in \p - // Contents. This currently cares about only the namespace proximity, but in - // feature it should also try to follow ordering of declarations. For example, - // if decls come in order `foo, bar, baz` then this function should return - // some point between foo and baz for inserting bar. - // FIXME: The selection can be made smarter by looking at the definition - // locations for adjacent decls to Source. Unfortunately pseudo parsing in - // getEligibleRegions only knows about namespace begin/end events so we - // can't match function start/end positions yet. - llvm::Expected<InsertionPoint> getInsertionPoint(llvm::StringRef Contents, - const Selection &Sel) { - // If the definition goes to the same file and there is a namespace, - // we should (and, in the case of anonymous namespaces, need to) - // put the definition into the original namespace block. - if (SameFile) { - auto *Klass = Source->getDeclContext()->getOuterLexicalRecordContext(); - if (!Klass) - return error("moving to same file not supported for free functions"); - const SourceLocation EndLoc = Klass->getBraceRange().getEnd(); - const auto &TokBuf = Sel.AST->getTokens(); - auto Tokens = TokBuf.expandedTokens(); - auto It = llvm::lower_bound( - Tokens, EndLoc, [](const syntax::Token &Tok, SourceLocation EndLoc) { - return Tok.location() < EndLoc; - }); - while (It != Tokens.end()) { - if (It->kind() != tok::semi) { - ++It; - continue; + enum class RelativeInsertPos { Before, After }; + std::optional<std::pair<Symbol, RelativeInsertPos>> + getDefinitionOfAdjacentDecl(const Selection &Sel) { + if (!Sel.Index) + return {}; + std::optional<Symbol> Anchor; + Path PrefixedTuPath = "file://" + Sel.AST->tuPath().str(); + auto CheckCandidate = [&](Decl *Candidate) { + assert(Candidate != Source); + if (auto Func = llvm::dyn_cast_or_null<FunctionDecl>(Candidate); + !Func || Func->isThisDeclarationADefinition()) { + return; + } + std::optional<Symbol> CandidateSymbol; + Sel.Index->lookup({{getSymbolID(Candidate)}}, [&](const Symbol &S) { + if (S.Definition) { + CandidateSymbol = S; } - unsigned Offset = Sel.AST->getSourceManager() - .getDecomposedLoc(It->endLocation()) - .second; - return InsertionPoint{Klass->getEnclosingNamespaceContext(), Offset}; + }); + if (!CandidateSymbol) + return; + + // If our definition is constrained to the same file, ignore + // definitions that are not located there. + // If our definition is not constrained to the same file, but + // our anchor definition is in the same file, then we also put our + // definition there, because that appears to be the user preference. + // Exception: If the existing definition is a template, then the + // location is likely due to technical necessity rather than preference, + // so ignore that definition. + bool CandidateSameFile = + PrefixedTuPath == CandidateSymbol->Definition.FileURI; + if (SameFile && !CandidateSameFile) + return; + if (!SameFile && CandidateSameFile) { + if (Candidate->isTemplateDecl()) + return; + SameFile = true; } - return error( - "failed to determine insertion location: no end of class found"); + Anchor = *CandidateSymbol; + }; + + // Try to find adjacent function declarations. + // Determine the closest one by alternatingly going "up" and "down" + // from our function in increasing steps. + const DeclContext *ParentContext = Source->getParent(); + const auto SourceIt = llvm::find_if( + ParentContext->decls(), [this](const Decl *D) { return D == Source; }); + if (SourceIt == ParentContext->decls_end()) + return {}; + const int Preceding = std::distance(ParentContext->decls_begin(), SourceIt); + const int Following = + std::distance(SourceIt, ParentContext->decls_end()) - 1; + for (int Offset = 1; Offset <= Preceding || Offset <= Following; ++Offset) { + if (Offset <= Preceding) + CheckCandidate( + *std::next(ParentContext->decls_begin(), Preceding - Offset)); + if (Anchor) + return std::make_pair(*Anchor, RelativeInsertPos::After); + if (Offset <= Following) + CheckCandidate(*std::next(SourceIt, Offset)); + if (Anchor) + return std::make_pair(*Anchor, RelativeInsertPos::Before); } + return {}; + } - auto Region = getEligiblePoints( - Contents, Source->getQualifiedNameAsString(), Sel.AST->getLangOpts()); + // We don't know the actual start or end of the definition, only the position + // of the name. Therefore, we heuristically try to locate the last token + // before or in this function, respectively. Adapt as required by user code. + llvm::Expected<Position> getInsertionPointFromExistingDefinition( + const llvm::MemoryBuffer &Buffer, const Symbol &S, + RelativeInsertPos RelInsertPos, ParsedAST *AST) { + auto LspPos = indexToLSPLocation(S.Definition, AST->tuPath()); + if (!LspPos) + return LspPos.takeError(); + auto StartOffset = + positionToOffset(Buffer.getBuffer(), LspPos->range.start); + if (!StartOffset) + return LspPos.takeError(); + SourceLocation InsertionLoc; + SourceManager &SM = AST->getSourceManager(); + FileID F = Buffer.getBufferIdentifier() == AST->tuPath() + ? SM.getMainFileID() + : SM.createFileID(Buffer); + + auto InsertBefore = [&] { + // Go backwards until we encounter one of the following: + // - An opening brace (of a namespace). + // - A closing brace (of a function definition). + // - A semicolon (of a declaration). + // If no such token was found, then the first token in the file starts the + // definition. + auto Tokens = syntax::tokenize(syntax::FileRange(F, 0, *StartOffset), SM, + AST->getLangOpts()); + if (Tokens.empty()) + return; + for (auto I = std::rbegin(Tokens); + InsertionLoc.isInvalid() && I != std::rend(Tokens); ++I) { + switch (I->kind()) { + case tok::l_brace: + case tok::r_brace: + case tok::semi: + if (I != std::rbegin(Tokens)) + InsertionLoc = std::prev(I)->location(); + else + InsertionLoc = I->endLocation(); + break; + default: + break; + } + } + if (InsertionLoc.isInvalid()) + InsertionLoc = Tokens.front().location(); + }; - assert(!Region.EligiblePoints.empty()); - auto Offset = positionToOffset(Contents, Region.EligiblePoints.back()); - if (!Offset) - return Offset.takeError(); + if (RelInsertPos == RelativeInsertPos::Before) { + InsertBefore(); + } else { + // Skip over one top-level pair of parentheses (for the parameter list) + // and one pair of curly braces (for the code block). + // If that fails, insert before the function instead. + auto Tokens = syntax::tokenize( + syntax::FileRange(F, *StartOffset, Buffer.getBuffer().size()), SM, + AST->getLangOpts()); + bool SkippedParams = false; + int OpenParens = 0; + int OpenBraces = 0; + std::optional<syntax::Token> Tok; + for (const auto &T : Tokens) { + tok::TokenKind StartKind = SkippedParams ? tok::l_brace : tok::l_paren; + tok::TokenKind EndKind = SkippedParams ? tok::r_brace : tok::r_paren; + int &Count = SkippedParams ? OpenBraces : OpenParens; + if (T.kind() == StartKind) { + ++Count; + } else if (T.kind() == EndKind) { + if (--Count == 0) { + if (SkippedParams) { + Tok = T; + break; + } + SkippedParams = true; + } else if (Count < 0) { + break; + } + } + } + if (Tok) + InsertionLoc = Tok->endLocation(); + else + InsertBefore(); + } - auto TargetContext = - findContextForNS(Region.EnclosingNamespace, Source->getDeclContext()); - if (!TargetContext) - return error("define outline: couldn't find a context for target"); + return InsertionLoc.isValid() ? sourceLocToPosition(SM, InsertionLoc) + : Position(); + } - return InsertionPoint{*TargetContext, *Offset}; + // Returns the most natural insertion point in this file. + // This is a fallback for when we failed to find an existing definition to + // place the new one next to. It only considers namespace proximity. + llvm::Expected<InsertionPoint> getInsertionPointInMainFile(ParsedAST *AST) { + // If the definition goes to the same file and there is a namespace, + // we should (and, in the case of anonymous namespaces, need to) + // put the definition into the original namespace block. + auto *Klass = Source->getDeclContext()->getOuterLexicalRecordContext(); + if (!Klass) + return error("moving to same file not supported for free functions"); + const SourceLocation EndLoc = Klass->getBraceRange().getEnd(); + const auto &TokBuf = AST->getTokens(); + auto Tokens = TokBuf.expandedTokens(); + auto It = llvm::lower_bound( + Tokens, EndLoc, [](const syntax::Token &Tok, SourceLocation EndLoc) { + return Tok.location() < EndLoc; + }); + while (It != Tokens.end()) { + if (It->kind() != tok::semi) { + ++It; + continue; + } + unsigned Offset = + AST->getSourceManager().getDecomposedLoc(It->endLocation()).second; + return InsertionPoint{Klass->getEnclosingNamespaceContext(), Offset}; + } + return error( + "failed to determine insertion location: no end of class found"); } private: diff --git a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp index b5f09f9b14604..9340255543421 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp @@ -7,7 +7,9 @@ //===----------------------------------------------------------------------===// #include "TestFS.h" +#include "TestIndex.h" #include "TweakTesting.h" +#include "index/MemIndex.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -447,9 +449,27 @@ inline T Foo<T>::bar(const T& t, const U& u) { return {}; } TEST_F(DefineOutlineTest, InCppFile) { FileName = "Test.cpp"; + const auto MakePos = [](uint32_t Line, uint32_t Col) { + SymbolLocation::Position Pos; + Pos.setLine(Line); + Pos.setColumn(Col); + return Pos; + }; + struct SymbolSpec { + StringRef NamespaceName; + StringRef ClassName; + StringRef FuncName; + SymbolLocation::Position DeclStart; + SymbolLocation::Position DeclEnd; + SymbolLocation::Position DefStart; + SymbolLocation::Position DefEnd; + }; + using SymbolSpecs = std::vector<SymbolSpec>; + struct { llvm::StringRef Test; llvm::StringRef ExpectedSource; + SymbolSpecs Definitions; } Cases[] = { { R"cpp( @@ -470,10 +490,163 @@ TEST_F(DefineOutlineTest, InCppFile) { } } )cpp"}, + + // Criterion 1: Distance + { + R"cpp( + struct Foo { + void ignored1(); // Too far away + void ignored2(); // No definition + void ignored3() {} // Defined inline + void fo^o() {} + void neighbor(); + }; + void Foo::ignored1() {} + void Foo::neighbor() {} + )cpp", + R"cpp( + struct Foo { + void ignored1(); // Too far away + void ignored2(); // No definition + void ignored3() {} // Defined inline + void foo() ; + void neighbor(); + }; + void Foo::ignored1() {} + void Foo::foo() {} +void Foo::neighbor() {} + )cpp", + SymbolSpecs{{"", "Foo", "ignored3", MakePos(4, 21), MakePos(4, 29), + MakePos(4, 21), MakePos(4, 29)}, + {"", "Foo", "ignored1", MakePos(2, 21), MakePos(2, 29), + MakePos(8, 22), MakePos(8, 30)}, + {"", "Foo", "neighbor", MakePos(6, 21), MakePos(6, 29), + MakePos(9, 22), MakePos(9, 30)}}}, + + // Criterion 2: Prefer preceding + { + R"cpp( + struct Foo { + void neighbor(); + void fo^o() {} + void ignored(); + }; + void Foo::neighbor() {} + void Foo::ignored() {} + )cpp", + R"cpp( + struct Foo { + void neighbor(); + void foo() ; + void ignored(); + }; + void Foo::neighbor() {}void Foo::foo() {} + + void Foo::ignored() {} + )cpp", + SymbolSpecs{{"", "Foo", "ignored", MakePos(4, 21), MakePos(4, 28), + MakePos(7, 22), MakePos(7, 29)}, + {"", "Foo", "neighbor", MakePos(2, 22), MakePos(2, 29), + MakePos(6, 22), MakePos(6, 30)}}}, + + // Like above, but with a namespace + { + R"cpp( + namespace NS { + struct Foo { + void neighbor(); + void fo^o() {} + void ignored(); + }; + void Foo::neighbor() {} + void Foo::ignored() {} + } + )cpp", + R"cpp( + namespace NS { + struct Foo { + void neighbor(); + void foo() ; + void ignored(); + }; + void Foo::neighbor() {}void Foo::foo() {} + + void Foo::ignored() {} + } + )cpp", + SymbolSpecs{{"NS", "Foo", "ignored", MakePos(5, 21), MakePos(5, 29), + MakePos(8, 22), MakePos(8, 30)}, + {"NS", "Foo", "neighbor", MakePos(3, 21), MakePos(3, 29), + MakePos(7, 22), MakePos(7, 30)}}}, + + // Like above, but there is no namespace at the definition site + { + R"cpp( + namespace NS { + struct Foo { + void neighbor(); + void fo^o() {} + void ignored(); + }; + } + void NS::Foo::neighbor() {} + void NS::Foo::ignored() {} + )cpp", + R"cpp( + namespace NS { + struct Foo { + void neighbor(); + void foo() ; + void ignored(); + }; + } + void NS::Foo::neighbor() {}void NS::Foo::foo() {} + + void NS::Foo::ignored() {} + )cpp", + SymbolSpecs{{"NS", "Foo", "ignored", MakePos(5, 21), MakePos(5, 29), + MakePos(9, 26), MakePos(9, 33)}, + {"NS", "Foo", "neighbor", MakePos(3, 21), MakePos(3, 29), + MakePos(8, 26), MakePos(8, 34)}}}, + }; + + const std::string URI = +#ifdef _WIN32 + "file://C:\\clangd-test/" +#else + "file:///clangd-test/" +#endif + + FileName.str(); + std::string FullNamespace; + + const auto BuildIndex = [&](const SymbolSpecs &Specs) { + SymbolSlab::Builder Slab; + for (const auto &S : Specs) { + std::string USRFormat; + if (!S.NamespaceName.empty()) + USRFormat += "@N@" + S.NamespaceName.str(); + if (!S.ClassName.empty()) + USRFormat += "@S@" + S.ClassName.str(); + USRFormat += "@F@\\0#"; + Symbol Sym = sym(S.FuncName, index::SymbolKind::Function, USRFormat); + Sym.CanonicalDeclaration.FileURI = URI.data(); + Sym.CanonicalDeclaration.Start = S.DeclStart; + Sym.CanonicalDeclaration.End = S.DeclEnd; + Sym.Definition.FileURI = Sym.CanonicalDeclaration.FileURI; + Sym.Definition.Start = S.DefStart; + Sym.Definition.End = S.DefEnd; + if (!S.NamespaceName.empty()) { + FullNamespace = S.NamespaceName.str() + "::"; + Sym.Scope = FullNamespace; + } + Slab.insert(Sym); + } + return MemIndex::build(std::move(Slab).build(), RefSlab(), RelationSlab()); }; for (const auto &Case : Cases) { SCOPED_TRACE(Case.Test); + Index = BuildIndex(Case.Definitions); EXPECT_EQ(apply(Case.Test, nullptr), Case.ExpectedSource); } } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits