[clang-tools-extra] [clangd] Find better insertion locations in DefineOutline tweak (PR #128164)
via cfe-commits
cfe-commits at lists.llvm.org
Fri Feb 21 03:25:42 PST 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clangd
Author: Christian Kandeler (ckandeler)
<details>
<summary>Changes</summary>
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
---
Patch is 22.35 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/128164.diff
4 Files Affected:
- (modified) clang-tools-extra/clangd/SourceCode.cpp (+20)
- (modified) clang-tools-extra/clangd/SourceCode.h (+3)
- (modified) clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp (+236-54)
- (modified) clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp (+173)
``````````diff
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...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/128164
More information about the cfe-commits
mailing list