diff --git a/clang-tools-extra/clangd/SourceCode.cpp b/clang-tools-extra/clangd/SourceCode.cpp index 1e1f7c68350ea..21c078fd2cdb9 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 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 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..274099f1f4d33 100644 --- a/clang-tools-extra/clangd/SourceCode.h +++ b/clang-tools-extra/clangd/SourceCode.h @@ -309,6 +309,11 @@ EligibleRegion getEligiblePoints(llvm::StringRef Code, llvm::StringRef FullyQualifiedName, const LangOptions &LangOpts); +/// Returns the fully qualified name of the namespace at \p Pos in the \p Code. +/// Employs pseudo-parsing to determine the start and end of namespaces. +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..45e7adeeefcd9 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" @@ -34,6 +35,7 @@ #include #include #include +#include namespace clang { namespace clangd { @@ -362,6 +364,12 @@ struct InsertionPoint { size_t Offset; }; +enum class RelativeInsertPos { Before, After }; +struct InsertionAnchor { + Location Loc; + RelativeInsertPos RelInsertPos = RelativeInsertPos::Before; +}; + // Returns the range that should be deleted from declaration, which always // contains function body. In addition to that it might contain constructor // initializers. @@ -489,8 +497,14 @@ class DefineOutline : public Tweak { Expected 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 CCFile; + auto Anchor = getDefinitionOfAdjacentDecl(Sel); + if (Anchor) { + CCFile = Anchor->Loc.uri.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 +513,62 @@ class DefineOutline : public Tweak { // doesn't exist? if (!Buffer) return llvm::errorCodeToError(Buffer.getError()); + auto Contents = Buffer->get()->getBuffer(); - auto InsertionPoint = getInsertionPoint(Contents, Sel); - if (!InsertionPoint) - return InsertionPoint.takeError(); + SourceManagerForFile SMFF(*CCFile, Contents); + + std::optional InsertionPos; + if (Anchor) { + if (auto P = getInsertionPointFromExistingDefinition( + SMFF, **Buffer, Anchor->Loc, Anchor->RelInsertPos, Sel.AST)) { + InsertionPos = *P; + } + } + + std::optional Offset; + 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); + 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 +603,188 @@ 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 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; + std::optional + getDefinitionOfAdjacentDecl(const Selection &Sel) { + if (!Sel.Index) + return {}; + std::optional Anchor; + std::string TuURI = URI::createFile(Sel.AST->tuPath()).toString(); + auto CheckCandidate = [&](Decl *Candidate) { + assert(Candidate != Source); + if (auto Func = llvm::dyn_cast_or_null(Candidate); + !Func || Func->isThisDeclarationADefinition()) { + return; + } + std::optional CandidateLoc; + Sel.Index->lookup({{getSymbolID(Candidate)}}, [&](const Symbol &S) { + if (S.Definition) { + if (auto Loc = indexToLSPLocation(S.Definition, Sel.AST->tuPath())) + CandidateLoc = *Loc; + else + log("getDefinitionOfAdjacentDecl: {0}", Loc.takeError()); } - unsigned Offset = Sel.AST->getSourceManager() - .getDecomposedLoc(It->endLocation()) - .second; - return InsertionPoint{Klass->getEnclosingNamespaceContext(), Offset}; + }); + if (!CandidateLoc) + 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 = TuURI == CandidateLoc->uri.uri(); + 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 = *CandidateLoc; + }; + + // 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 InsertionAnchor{*Anchor, RelativeInsertPos::After}; + if (Offset <= Following) + CheckCandidate(*std::next(SourceIt, Offset)); + if (Anchor) + return InsertionAnchor{*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. + std::optional getInsertionPointFromExistingDefinition( + SourceManagerForFile &SMFF, const llvm::MemoryBuffer &Buffer, + const Location &Loc, RelativeInsertPos RelInsertPos, ParsedAST *AST) { + auto StartOffset = positionToOffset(Buffer.getBuffer(), Loc.range.start); + if (!StartOffset) + return {}; + SourceLocation InsertionLoc; + SourceManager &SM = SMFF.get(); + + 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(SM.getMainFileID(), 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(SM.getMainFileID(), *StartOffset, + Buffer.getBuffer().size()), + SM, AST->getLangOpts()); + bool SkippedParams = false; + int OpenParens = 0; + int OpenBraces = 0; + std::optional 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"); + if (!InsertionLoc.isValid()) + return {}; + return sourceLocToPosition(SM, InsertionLoc); + } - 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 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 02133ec5d77a3..7689bf3181a5f 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp @@ -448,7 +448,6 @@ inline T Foo::bar(const T& t, const U& u) { return {}; } TEST_F(DefineOutlineTest, InCppFile) { FileName = "Test.cpp"; - struct { llvm::StringRef Test; llvm::StringRef ExpectedSource; @@ -773,8 +772,8 @@ class A { )cpp"; std::string SourceAfter = R"cpp( #include "a.hpp" -void A::bar(){} -void A::foo(){} +void A::bar(){}void A::foo(){} + )cpp"; Workspace.addSource("a.hpp", HeaderBefore.code()); Workspace.addMainFile("a.cpp", SourceBefore); @@ -786,6 +785,195 @@ void A::foo(){} FileWithContents(testPath("a.cpp"), SourceAfter))))); } +// Test that the definition is inserted in a sensible location +// under various circumstances. +// Note that the formatting looks a little off here and there, +// which is because in contrast to the actual tweak, the test procedure +// does not run clang-format on the resulting code. +TEST_F(DefineOutlineWorkspaceTest, SensibleInsertionLocations) { + const struct { + llvm::StringRef HeaderBefore; + llvm::StringRef SourceBefore; + llvm::StringRef HeaderAfter; + llvm::StringRef SourceAfter; + } Cases[] = { + // 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(); +}; +)cpp", + R"cpp( +#include "a.hpp" +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(); +}; +)cpp", + R"cpp( +#include "a.hpp" +void Foo::ignored1() {} +void Foo::foo() {} +void Foo::neighbor() {} +)cpp"}, + + // Criterion 2: Prefer preceding + { + R"cpp( +struct Foo { + void neighbor(); + void fo^o() {} + void ignored(); +}; +)cpp", + R"cpp( +#include "a.hpp" +void Foo::neighbor() {} +void Foo::ignored() {} +)cpp", + R"cpp( +struct Foo { + void neighbor(); + void foo() ; + void ignored(); +}; +)cpp", + R"cpp( +#include "a.hpp" +void Foo::neighbor() {}void Foo::foo() {} + +void Foo::ignored() {} +)cpp"}, + + // Like above, but with a namespace + { + R"cpp( +namespace NS { +struct Foo { + void neighbor(); + void fo^o() {} + void ignored(); +}; +} +)cpp", + R"cpp( +#include "a.hpp" +namespace NS { +void Foo::neighbor() {} +void Foo::ignored() {} +} +)cpp", + R"cpp( +namespace NS { +struct Foo { + void neighbor(); + void foo() ; + void ignored(); +}; +} +)cpp", + R"cpp( +#include "a.hpp" +namespace NS { +void Foo::neighbor() {}void Foo::foo() {} + +void Foo::ignored() {} +} +)cpp"}, + + // Like above, but there is no namespace at the definition site + { + R"cpp( +namespace NS { +struct Foo { + void neighbor(); + void fo^o() {} + void ignored(); +}; +} +)cpp", + R"cpp( +#include "a.hpp" +void NS::Foo::neighbor() {} +void NS::Foo::ignored() {} +)cpp", + R"cpp( +namespace NS { +struct Foo { + void neighbor(); + void foo() ; + void ignored(); +}; +} +)cpp", + R"cpp( +#include "a.hpp" +void NS::Foo::neighbor() {}void NS::Foo::foo() {} + +void NS::Foo::ignored() {} +)cpp"}, + + // Neighbor's definition is in header + { + R"cpp( +struct Foo { + void fo^o() {} + void neighbor(); + void ignored(); +}; +inline void Foo::neighbor() {} +)cpp", + R"cpp( +#include "a.hpp" +void Foo::ignored() {} +)cpp", + R"cpp( +struct Foo { + void foo() ; + void neighbor(); + void ignored(); +}; +inline void Foo::foo() {} +inline void Foo::neighbor() {} +)cpp", + {}}, + + }; + + for (const auto &Case : Cases) { + Workspace = {}; + llvm::Annotations Hdr(Case.HeaderBefore); + Workspace.addSource("a.hpp", Hdr.code()); + Workspace.addMainFile("a.cpp", Case.SourceBefore); + auto Result = apply("a.hpp", {Hdr.point(), Hdr.point()}); + if (Case.SourceAfter.empty()) { + EXPECT_THAT(Result, + AllOf(withStatus("success"), + editedFiles(UnorderedElementsAre(FileWithContents( + testPath("a.hpp"), Case.HeaderAfter))))); + + } else { + EXPECT_THAT( + Result, + AllOf(withStatus("success"), + editedFiles(UnorderedElementsAre( + FileWithContents(testPath("a.hpp"), Case.HeaderAfter), + FileWithContents(testPath("a.cpp"), Case.SourceAfter))))); + } + } +} } // namespace } // namespace clangd } // namespace clang