diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp --- a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp @@ -6,13 +6,17 @@ // //===----------------------------------------------------------------------===// +#include "AST.h" +#include "FindTarget.h" #include "HeaderSourceSwitch.h" +#include "Logger.h" #include "Path.h" #include "Selection.h" #include "SourceCode.h" #include "refactor/Tweak.h" #include "clang/AST/ASTTypeTraits.h" #include "clang/AST/Decl.h" +#include "clang/AST/DeclBase.h" #include "clang/AST/DeclTemplate.h" #include "clang/AST/Stmt.h" #include "clang/Basic/SourceLocation.h" @@ -22,8 +26,10 @@ #include "clang/Tooling/Core/Replacement.h" #include "llvm/ADT/Optional.h" #include "llvm/ADT/StringRef.h" +#include "llvm/Support/Casting.h" #include "llvm/Support/Error.h" #include +#include namespace clang { namespace clangd { @@ -70,27 +76,89 @@ // Creates a modified version of function definition that can be inserted at a // different location. Contains both function signature and body. -llvm::Optional getFunctionSourceCode(const FunctionDecl *FD) { +llvm::Expected +getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace) { auto &SM = FD->getASTContext().getSourceManager(); - auto CharRange = toHalfOpenFileRange(SM, FD->getASTContext().getLangOpts(), - FD->getSourceRange()); - if (!CharRange) - return llvm::None; - CharRange->setBegin(getBeginLoc(FD)); + const DeclContext *TargetContext = FD->getLexicalDeclContext(); + while (!TargetContext->isTranslationUnit()) { + if (TargetContext->isNamespace()) { + auto *NSD = llvm::dyn_cast(TargetContext); + if (NSD->getNameAsString() == TargetNamespace) + break; + } + TargetContext = TargetContext->getLexicalParent(); + } + + bool HadErrors = false; + tooling::Replacements Replacements; + findExplicitReferences(FD, [&](ReferenceLoc Ref) { + // It is enough to qualify the first qualifier, so skip references with a + // qualifier. Also we can't do much if there are no targets or name is + // inside a macro body. + if (Ref.Qualifier || Ref.Targets.empty() || Ref.NameLoc.isMacroID()) + return; + // Qualify return type + if (Ref.NameLoc != FD->getReturnTypeSourceRange().getBegin()) + return; + + for (const NamedDecl *ND : Ref.Targets) { + if (ND->getDeclContext() != Ref.Targets.front()->getDeclContext()) { + elog("Targets from multiple contexts: {0}, {1}", + printQualifiedName(*Ref.Targets.front()), printQualifiedName(*ND)); + return; + } + } + const NamedDecl *ND = Ref.Targets.front(); + const std::string Qualifier = + getQualification(FD->getASTContext(), TargetContext, + SM.getLocForStartOfFile(SM.getMainFileID()), ND); + if (auto Err = Replacements.add( + tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier))) { + elog("define outline: Failed to add quals: {0}", std::move(Err)); + HadErrors = true; + } + }); + + if (HadErrors) { + return llvm::createStringError( + llvm::inconvertibleErrorCode(), + "define outline: Failed to compute qualifiers see logs for details."); + } + + // Get new begin and end positions for the qualified body. + auto OrigFuncRange = toHalfOpenFileRange( + SM, FD->getASTContext().getLangOpts(), FD->getSourceRange()); + if (!OrigFuncRange) + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "Couldn't get range for function."); + OrigFuncRange->setBegin(getBeginLoc(FD)); + + unsigned FuncBegin = SM.getFileOffset(OrigFuncRange->getBegin()); + unsigned FuncEnd = Replacements.getShiftedCodePosition( + SM.getFileOffset(OrigFuncRange->getEnd())); + + // Trim the result to function body. + auto QualifiedFunc = tooling::applyAllReplacements( + SM.getBufferData(SM.getMainFileID()), Replacements); + if (!QualifiedFunc) + return QualifiedFunc.takeError(); + return QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1); - // FIXME: Qualify return type. // FIXME: Qualify function name depending on the target context. - return toSourceCode(SM, *CharRange); } +struct InsertionPoint { + std::string EnclosingNamespace; + size_t Offset; +}; // 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. -llvm::Expected getInsertionOffset(llvm::StringRef Contents, - llvm::StringRef QualifiedName, - const format::FormatStyle &Style) { +llvm::Expected +getInsertionPoint(llvm::StringRef Contents, llvm::StringRef QualifiedName, + const format::FormatStyle &Style) { auto Region = getEligiblePoints(Contents, QualifiedName, Style); assert(!Region.EligiblePoints.empty()); @@ -98,8 +166,10 @@ // locations for adjacent decls to Source. Unfortunately psudeo parsing in // getEligibleRegions only knows about namespace begin/end events so we // can't match function start/end positions yet. - auto InsertionPoint = Region.EligiblePoints.back(); - return positionToOffset(Contents, InsertionPoint); + auto Offset = positionToOffset(Contents, Region.EligiblePoints.back()); + if (!Offset) + return Offset.takeError(); + return InsertionPoint{Region.EnclosingNamespace, *Offset}; } /// Moves definition of a function/method to an appropriate implementation file. @@ -171,21 +241,22 @@ return llvm::createStringError(Buffer.getError(), Buffer.getError().message()); auto Contents = Buffer->get()->getBuffer(); - auto InsertionOffset = - getInsertionOffset(Contents, Source->getQualifiedNameAsString(), - getFormatStyleForFile(*CCFile, Contents, &FS)); - if (!InsertionOffset) - return InsertionOffset.takeError(); + auto InsertionPoint = + getInsertionPoint(Contents, Source->getQualifiedNameAsString(), + getFormatStyleForFile(*CCFile, Contents, &FS)); + if (!InsertionPoint) + return InsertionPoint.takeError(); - auto FuncDef = getFunctionSourceCode(Source); + auto FuncDef = + getFunctionSourceCode(Source, InsertionPoint->EnclosingNamespace); if (!FuncDef) return llvm::createStringError( llvm::inconvertibleErrorCode(), "Couldn't get full source for function definition."); SourceManagerForFile SMFF(*CCFile, Contents); - const tooling::Replacement InsertFunctionDef(*CCFile, *InsertionOffset, 0, - *FuncDef); + const tooling::Replacement InsertFunctionDef( + *CCFile, InsertionPoint->Offset, 0, *FuncDef); auto Effect = Effect::mainFileEdit( SMFF.get(), tooling::Replacements(InsertFunctionDef)); if (!Effect) diff --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp b/clang-tools-extra/clangd/unittests/TweakTests.cpp --- a/clang-tools-extra/clangd/unittests/TweakTests.cpp +++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp @@ -1775,6 +1775,56 @@ } } +TEST_F(DefineOutlineTest, QualifyReturnValue) { + FileName = "Test.hpp"; + ExtraFiles["Test.cpp"] = ""; + + struct { + llvm::StringRef Test; + llvm::StringRef ExpectedHeader; + llvm::StringRef ExpectedSource; + } Cases[] = { + {R"cpp( + namespace a { class Foo; } + using namespace a; + Foo fo^o() { return; })cpp", + R"cpp( + namespace a { class Foo; } + using namespace a; + Foo foo() ;)cpp", + "a::Foo foo() { return; }"}, + {R"cpp( + namespace a { + class Foo { + class Bar {}; + Bar fo^o() { return {}; } + } + })cpp", + R"cpp( + namespace a { + class Foo { + class Bar {}; + Bar foo() ; + } + })cpp", + "a::Foo::Bar foo() { return {}; }\n"}, + {R"cpp( + class Foo; + Foo fo^o() { return; })cpp", + R"cpp( + class Foo; + Foo foo() ;)cpp", + "Foo foo() { return; }"}, + }; + llvm::StringMap EditedFiles; + for (auto &Case : Cases) { + apply(Case.Test, &EditedFiles); + EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader); + llvm::errs() << EditedFiles.begin()->second << '\n'; + EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents( + testPath("Test.cpp"), Case.ExpectedSource))); + } +} } // namespace } // namespace clangd } // namespace clang