diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp index c2806d756983..8785db1a292b 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "AST.h" +#include "FindTarget.h" #include "Logger.h" #include "Selection.h" #include "SourceCode.h" @@ -42,23 +43,42 @@ #include "clang/Tooling/Core/Replacement.h" #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/None.h" #include "llvm/ADT/Optional.h" #include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Casting.h" #include "llvm/Support/Error.h" +#include "llvm/Support/FormatAdapters.h" +#include "llvm/Support/FormatVariadic.h" #include "llvm/Support/Signals.h" #include "llvm/Support/raw_ostream.h" #include #include #include #include +#include #include namespace clang { namespace clangd { namespace { +// Returns semicolon location for the given FD. Since AST doesn't contain that +// information, searches for a semicolon by lexing from end of function decl +// while skipping comments. +llvm::Optional getSemicolonForDecl(const FunctionDecl *FD) { + const SourceManager &SM = FD->getASTContext().getSourceManager(); + const LangOptions &LangOpts = FD->getASTContext().getLangOpts(); + + SourceLocation CurLoc = FD->getEndLoc(); + auto NextTok = Lexer::findNextToken(CurLoc, SM, LangOpts); + if (!NextTok || !NextTok->is(tok::semi)) + return llvm::None; + return NextTok->getLocation(); +} + // Deduces the FunctionDecl from a selection. Requires either the function body // or the function decl to be selected. Returns null if none of the above // criteria is met. @@ -75,7 +95,7 @@ const FunctionDecl *getSelectedFunction(const SelectionTree::Node *SelNode) { } // Checks the decls mentioned in Source are visible in the context of Target. -// Achives that by checking declarations occur before target location in +// Achives that by checking declaraions occur before target location in // translation unit or declared in the same class. bool checkDeclsAreVisible(const llvm::DenseSet &DeclRefs, const FunctionDecl *Target, const SourceManager &SM) { @@ -115,6 +135,97 @@ bool checkDeclsAreVisible(const llvm::DenseSet &DeclRefs, return true; } +// Rewrites body of FD to fully-qualify all of the decls inside. +llvm::Expected qualifyAllDecls(const FunctionDecl *FD) { + // There are three types of spellings that needs to be qualified in a function + // body: + // - Types: Foo -> ns::Foo + // - DeclRefExpr: ns2::foo() -> ns1::ns2::foo(); + // - UsingDecls: + // using ns2::foo -> using ns1::ns2::foo + // using namespace ns2 -> using namespace ns1::ns2 + // using ns3 = ns2 -> using ns3 = ns1::ns2 + // + // Go over all references inside a function body to generate replacements that + // will fully qualify those. So that body can be moved into an arbitrary file. + // We perform the qualification by qualyfying the first type/decl in a + // (un)qualified name. e.g: + // namespace a { namespace b { class Bar{}; void foo(); } } + // b::Bar x; -> a::b::Bar x; + // foo(); -> a::b::foo(); + // FIXME: Instead of fully qualyfying we should try deducing visible scopes at + // target location and generate minimal edits. + + const SourceManager &SM = FD->getASTContext().getSourceManager(); + tooling::Replacements Replacements; + bool HadErrors = false; + findExplicitReferences(FD->getBody(), [&](ReferenceLoc Ref) { + // Since we want to qualify only the first qualifier, skip names with a + // qualifier. + if (Ref.Qualifier) + return; + // There might be no decl in dependent contexts, there's nothing much we can + // do in such cases. + if (Ref.Targets.empty()) + return; + // Do not qualify names introduced by macro expansions. + if (Ref.NameLoc.isMacroID()) + return; + + for (const NamedDecl *ND : Ref.Targets) { + if (ND->getDeclContext() != Ref.Targets.front()->getDeclContext()) { + elog("define inline: Targets from multiple contexts: {0}, {1}", + printQualifiedName(*Ref.Targets.front()), printQualifiedName(*ND)); + HadErrors = true; + return; + } + } + // All Targets are in the same scope, so we can safely chose first one. + const NamedDecl *ND = Ref.Targets.front(); + // Skip anything from a non-namespace scope, these can be: + // - Function or Method scopes, which means decl is local and doesn't need + // qualification. + // - From Class/Struct/Union scope, which again doesn't need any qualifiers, + // rather the left side of it requires qualification, like: + // namespace a { class Bar { public: static int x; } } + // void foo() { Bar::x; } + // ~~~~~ -> we need to qualify Bar not x. + if (!ND->getDeclContext()->isNamespace()) + return; + + std::string Qualifier = printNamespaceScope(*ND->getDeclContext()); + if (auto Err = Replacements.add( + tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier))) { + HadErrors = true; + elog("define inline: Failed to add quals: {0}", std::move(Err)); + } + }); + + if (HadErrors) { + return llvm::createStringError( + llvm::inconvertibleErrorCode(), + "define inline: Failed to compute qualifiers see logs for details."); + } + + // Get new begin and end positions for the qualified body. + auto OrigBodyRange = toHalfOpenFileRange( + SM, FD->getASTContext().getLangOpts(), FD->getBody()->getSourceRange()); + if (!OrigBodyRange) + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "Couldn't get range func body."); + + unsigned BodyBegin = SM.getFileOffset(OrigBodyRange->getBegin()); + unsigned BodyEnd = Replacements.getShiftedCodePosition( + SM.getFileOffset(OrigBodyRange->getEnd())); + + // Trim the result to function body. + auto QualifiedFunc = tooling::applyAllReplacements( + SM.getBufferData(SM.getFileID(OrigBodyRange->getBegin())), Replacements); + if (!QualifiedFunc) + return QualifiedFunc.takeError(); + return QualifiedFunc->substr(BodyBegin, BodyEnd - BodyBegin + 1); +} + // Returns the canonical declaration for the given FunctionDecl. This will // usually be the first declaration in current translation unit with the // exception of template specialization. @@ -136,6 +247,15 @@ const FunctionDecl *findTarget(const FunctionDecl *FD) { return PrevDecl; } +// Returns the begining location for a FunctionDecl. Returns location of +// template keyword for templated functions. +const SourceLocation getBeginLoc(const FunctionDecl *FD) { + // Include template parameter list. + if (auto *FTD = FD->getDescribedFunctionTemplate()) + return FTD->getBeginLoc(); + return FD->getBeginLoc(); +} + /// Moves definition of a function/method to its declaration location. /// Before: /// a.h: @@ -159,7 +279,6 @@ public: std::string title() const override { return "Move function body to declaration"; } - bool hidden() const override { return true; } // Returns true when selection is on a function definition that does not // make use of any internal symbols. @@ -177,6 +296,11 @@ public: if (MD->getParent()->isTemplated()) return false; } + // If function body starts or ends inside a macro, we refuse to move it into + // declaration location. + if (Source->getBody()->getBeginLoc().isMacroID() || + Source->getBody()->getEndLoc().isMacroID()) + return false; Target = findTarget(Source); if (Target == Source) { @@ -198,8 +322,63 @@ public: } Expected apply(const Selection &Sel) override { - return llvm::createStringError(llvm::inconvertibleErrorCode(), - "Not implemented yet"); + const auto &AST = Sel.AST.getASTContext(); + const auto &SM = AST.getSourceManager(); + + auto Semicolon = getSemicolonForDecl(Target); + if (!Semicolon) { + return llvm::createStringError( + llvm::inconvertibleErrorCode(), + "Couldn't find semicolon for target declaration."); + } + + auto QualifiedBody = qualifyAllDecls(Source); + if (!QualifiedBody) + return QualifiedBody.takeError(); + + const tooling::Replacement SemicolonToFuncBody(SM, *Semicolon, 1, + *QualifiedBody); + auto DefRange = toHalfOpenFileRange( + SM, AST.getLangOpts(), + SM.getExpansionRange(CharSourceRange::getCharRange(getBeginLoc(Source), + Source->getEndLoc())) + .getAsRange()); + if (!DefRange) { + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "Couldn't get range for the source."); + } + unsigned int SourceLen = SM.getFileOffset(DefRange->getEnd()) - + SM.getFileOffset(DefRange->getBegin()); + const tooling::Replacement DeleteFuncBody(SM, DefRange->getBegin(), + SourceLen, ""); + + llvm::SmallVector, 2> Edits; + // Edit for Target. + auto FE = Effect::fileEdit(SM, SM.getFileID(*Semicolon), + tooling::Replacements(SemicolonToFuncBody)); + if (!FE) + return FE.takeError(); + Edits.push_back(std::move(*FE)); + + // Edit for Source. + if (!SM.isWrittenInSameFile(DefRange->getBegin(), + SM.getExpansionLoc(Target->getBeginLoc()))) { + // Generate a new edit if the Source and Target are in different files. + auto FE = Effect::fileEdit(SM, SM.getFileID(Sel.Cursor), + tooling::Replacements(DeleteFuncBody)); + if (!FE) + return FE.takeError(); + Edits.push_back(std::move(*FE)); + } else { + // Merge with previous edit if they are in the same file. + if (auto Err = Edits.front().second.Replacements.add(DeleteFuncBody)) + return std::move(Err); + } + + Effect E; + for (auto &Pair : Edits) + E.ApplyEdits.try_emplace(std::move(Pair.first), std::move(Pair.second)); + return E; } private: diff --git a/clang-tools-extra/clangd/unittests/TweakTesting.cpp b/clang-tools-extra/clangd/unittests/TweakTesting.cpp index b7a48a8e9979..4e7c14de6f0a 100644 --- a/clang-tools-extra/clangd/unittests/TweakTesting.cpp +++ b/clang-tools-extra/clangd/unittests/TweakTesting.cpp @@ -10,9 +10,12 @@ #include "Annotations.h" #include "SourceCode.h" +#include "TestFS.h" #include "refactor/Tweak.h" #include "clang/Tooling/Core/Replacement.h" #include "llvm/Support/Error.h" +#include "gtest/gtest.h" +#include namespace clang { namespace clangd { @@ -79,13 +82,15 @@ MATCHER_P4(TweakIsAvailable, TweakID, Ctx, Header, ExtraFiles, } // namespace -std::string TweakTest::apply(llvm::StringRef MarkedCode) const { +std::string TweakTest::apply(llvm::StringRef MarkedCode, + llvm::StringMap *EditedFiles) const { std::string WrappedCode = wrap(Context, MarkedCode); Annotations Input(WrappedCode); auto Selection = rangeOrPoint(Input); TestTU TU; TU.HeaderCode = Header; + TU.AdditionalFiles = std::move(ExtraFiles); TU.Code = Input.code(); TU.ExtraArgs = ExtraArgs; ParsedAST AST = TU.build(); @@ -103,14 +108,24 @@ std::string TweakTest::apply(llvm::StringRef MarkedCode) const { return "message:\n" + *Result->ShowMessage; if (Result->ApplyEdits.empty()) return "no effect"; - if (Result->ApplyEdits.size() > 1) - return "received multi-file edits"; - auto ApplyEdit = Result->ApplyEdits.begin()->second; - if (auto NewText = ApplyEdit.apply()) - return unwrap(Context, *NewText); - else - return "bad edits: " + llvm::toString(NewText.takeError()); + std::string EditedMainFile; + for (auto &It : Result->ApplyEdits) { + auto NewText = It.second.apply(); + if (!NewText) + return "bad edits: " + llvm::toString(NewText.takeError()); + llvm::StringRef Unwrapped = unwrap(Context, *NewText); + if (It.first() == testPath(TU.Filename)) + EditedMainFile = Unwrapped; + else { + if (!EditedFiles) + ADD_FAILURE() << "There were changes to additional files, but client " + "provided a nullptr for EditedFiles."; + else + EditedFiles->try_emplace(It.first(), Unwrapped.str()); + } + } + return EditedMainFile; } ::testing::Matcher TweakTest::isAvailable() const { diff --git a/clang-tools-extra/clangd/unittests/TweakTesting.h b/clang-tools-extra/clangd/unittests/TweakTesting.h index 8b9209d943da..34e355290a60 100644 --- a/clang-tools-extra/clangd/unittests/TweakTesting.h +++ b/clang-tools-extra/clangd/unittests/TweakTesting.h @@ -68,13 +68,19 @@ protected: // Apply the current tweak to the range (or point) in MarkedCode. // MarkedCode will be wrapped according to the Context. - // - if the tweak produces edits, returns the edited code (without markings). + // - if the tweak produces edits, returns the edited code (without markings) + // for the main file. + // Populates \p EditedFiles if there were changes to other files whenever + // it is non-null. It is a mapping from absolute path of the edited file to + // its new contents. Passing a nullptr to \p EditedFiles when there are + // changes, will result in a failure. // The context added to MarkedCode will be stripped away before returning, // unless the tweak edited it. // - if the tweak produces a message, returns "message:\n" // - if prepare() returns false, returns "unavailable" // - if apply() returns an error, returns "fail: " - std::string apply(llvm::StringRef MarkedCode) const; + std::string apply(llvm::StringRef MarkedCode, + llvm::StringMap *EditedFiles = nullptr) const; // Accepts a code snippet with many ranges (or points) marked, and returns a // list of snippets with one range marked each. diff --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp b/clang-tools-extra/clangd/unittests/TweakTests.cpp index d36fd2f34bc5..e1e7f5b63bc8 100644 --- a/clang-tools-extra/clangd/unittests/TweakTests.cpp +++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp @@ -25,6 +25,7 @@ #include "clang/Tooling/Core/Replacement.h" #include "llvm/ADT/IntrusiveRefCntPtr.h" #include "llvm/ADT/StringExtras.h" +#include "llvm/ADT/StringMap.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Error.h" #include "llvm/Support/MemoryBuffer.h" @@ -40,11 +41,16 @@ using ::testing::AllOf; using ::testing::HasSubstr; using ::testing::StartsWith; +using ::testing::ElementsAre; namespace clang { namespace clangd { namespace { +MATCHER_P2(FileWithContents, FileName, Contents, "") { + return arg.first() == FileName && arg.second == Contents; +} + TEST(FileEdits, AbsolutePath) { auto RelPaths = {"a.h", "foo.cpp", "test/test.cpp"}; @@ -1047,6 +1053,512 @@ TEST_F(DefineInlineTest, UsingShadowDecls) { })cpp"); } +TEST_F(DefineInlineTest, TransformNestedNamespaces) { + EXPECT_EQ(apply(R"cpp( + namespace a { + void bar(); + namespace b { + void baz(); + namespace c { + void aux(); + } + } + } + + void foo(); + using namespace a; + using namespace b; + using namespace c; + void f^oo() { + bar(); + a::bar(); + + baz(); + b::baz(); + a::b::baz(); + + aux(); + c::aux(); + b::c::aux(); + a::b::c::aux(); + })cpp"), R"cpp( + namespace a { + void bar(); + namespace b { + void baz(); + namespace c { + void aux(); + } + } + } + + void foo(){ + a::bar(); + a::bar(); + + a::b::baz(); + a::b::baz(); + a::b::baz(); + + a::b::c::aux(); + a::b::c::aux(); + a::b::c::aux(); + a::b::c::aux(); + } + using namespace a; + using namespace b; + using namespace c; + )cpp"); +} + +TEST_F(DefineInlineTest, TransformUsings) { + EXPECT_EQ(apply(R"cpp( + namespace a { namespace b { namespace c { void aux(); } } } + + void foo(); + void f^oo() { + using namespace a; + using namespace b; + using namespace c; + using c::aux; + namespace d = c; + })cpp"), + R"cpp( + namespace a { namespace b { namespace c { void aux(); } } } + + void foo(){ + using namespace a; + using namespace a::b; + using namespace a::b::c; + using a::b::c::aux; + namespace d = a::b::c; + } + )cpp"); +} + +TEST_F(DefineInlineTest, TransformDecls) { + EXPECT_EQ(apply(R"cpp( + void foo(); + void f^oo() { + class Foo { + public: + void foo(); + int x; + static int y; + }; + Foo::y = 0; + + enum En { Zero, One }; + En x = Zero; + + enum class EnClass { Zero, One }; + EnClass y = EnClass::Zero; + })cpp"), + R"cpp( + void foo(){ + class Foo { + public: + void foo(); + int x; + static int y; + }; + Foo::y = 0; + + enum En { Zero, One }; + En x = Zero; + + enum class EnClass { Zero, One }; + EnClass y = EnClass::Zero; + } + )cpp"); +} + +TEST_F(DefineInlineTest, TransformTemplDecls) { + EXPECT_EQ(apply(R"cpp( + namespace a { + template class Bar { + public: + void bar(); + }; + template T bar; + template void aux() {} + } + + void foo(); + + using namespace a; + void f^oo() { + bar>.bar(); + aux>(); + })cpp"), + R"cpp( + namespace a { + template class Bar { + public: + void bar(); + }; + template T bar; + template void aux() {} + } + + void foo(){ + a::bar>.bar(); + a::aux>(); + } + + using namespace a; + )cpp"); +} + +TEST_F(DefineInlineTest, TransformMembers) { + EXPECT_EQ(apply(R"cpp( + class Foo { + void foo(); + }; + + void Foo::f^oo() { + return; + })cpp"), + R"cpp( + class Foo { + void foo(){ + return; + } + }; + + )cpp"); + + ExtraFiles["a.h"] = R"cpp( + class Foo { + void foo(); + };)cpp"; + + llvm::StringMap EditedFiles; + EXPECT_EQ(apply(R"cpp( + #include "a.h" + void Foo::f^oo() { + return; + })cpp", + &EditedFiles), + R"cpp( + #include "a.h" + )cpp"); + EXPECT_THAT(EditedFiles, + ElementsAre(FileWithContents(testPath("a.h"), + R"cpp( + class Foo { + void foo(){ + return; + } + };)cpp"))); +} + +TEST_F(DefineInlineTest, TransformDependentTypes) { + EXPECT_EQ(apply(R"cpp( + namespace a { + template class Bar {}; + } + + template + void foo(); + + using namespace a; + template + void f^oo() { + Bar B; + Bar> q; + })cpp"), + R"cpp( + namespace a { + template class Bar {}; + } + + template + void foo(){ + a::Bar B; + a::Bar> q; + } + + using namespace a; + )cpp"); +} + +TEST_F(DefineInlineTest, TransformFunctionTempls) { + // Check we select correct specialization decl. + EXPECT_EQ(apply(R"cpp( + template + void foo(T p); + + template <> + void foo(int p); + + template <> + void foo(char p); + + template <> + void fo^o(int p) { + return; + })cpp"), + R"cpp( + template + void foo(T p); + + template <> + void foo(int p){ + return; + } + + template <> + void foo(char p); + + )cpp"); + + // Make sure we are not selecting the first specialization all the time. + EXPECT_EQ(apply(R"cpp( + template + void foo(T p); + + template <> + void foo(int p); + + template <> + void foo(char p); + + template <> + void fo^o(char p) { + return; + })cpp"), + R"cpp( + template + void foo(T p); + + template <> + void foo(int p); + + template <> + void foo(char p){ + return; + } + + )cpp"); + + EXPECT_EQ(apply(R"cpp( + template + void foo(T p); + + template <> + void foo(int p); + + template + void fo^o(T p) { + return; + })cpp"), + R"cpp( + template + void foo(T p){ + return; + } + + template <> + void foo(int p); + + )cpp"); +} + +TEST_F(DefineInlineTest, TransformTypeLocs) { + EXPECT_EQ(apply(R"cpp( + namespace a { + template class Bar { + public: + template class Baz {}; + }; + class Foo{}; + } + + void foo(); + + using namespace a; + void f^oo() { + Bar B; + Foo foo; + a::Bar>::Baz> q; + })cpp"), + R"cpp( + namespace a { + template class Bar { + public: + template class Baz {}; + }; + class Foo{}; + } + + void foo(){ + a::Bar B; + a::Foo foo; + a::Bar>::Baz> q; + } + + using namespace a; + )cpp"); +} + +TEST_F(DefineInlineTest, TransformDeclRefs) { + EXPECT_EQ(apply(R"cpp( + namespace a { + template class Bar { + public: + void foo(); + static void bar(); + int x; + static int y; + }; + void bar(); + void test(); + } + + void foo(); + using namespace a; + void f^oo() { + a::Bar B; + B.foo(); + a::bar(); + Bar>::bar(); + a::Bar::bar(); + B.x = Bar::y; + Bar::y = 3; + bar(); + a::test(); + })cpp"), + R"cpp( + namespace a { + template class Bar { + public: + void foo(); + static void bar(); + int x; + static int y; + }; + void bar(); + void test(); + } + + void foo(){ + a::Bar B; + B.foo(); + a::bar(); + a::Bar>::bar(); + a::Bar::bar(); + B.x = a::Bar::y; + a::Bar::y = 3; + a::bar(); + a::test(); + } + using namespace a; + )cpp"); +} + +TEST_F(DefineInlineTest, StaticMembers) { + EXPECT_EQ(apply(R"cpp( + namespace ns { class X { static void foo(); void bar(); }; } + void ns::X::b^ar() { + foo(); + })cpp"), R"cpp( + namespace ns { class X { static void foo(); void bar(){ + foo(); + } }; } + )cpp"); +} + +TEST_F(DefineInlineTest, TransformInlineNamespaces) { + EXPECT_EQ(apply(R"cpp( + namespace a { inline namespace b { namespace { struct Foo{}; } } } + void foo(); + + using namespace a; + void ^foo() {Foo foo;})cpp"), R"cpp( + namespace a { inline namespace b { namespace { struct Foo{}; } } } + void foo(){a::Foo foo;} + + using namespace a; + )cpp"); +} + +TEST_F(DefineInlineTest, TokensBeforeSemicolon) { + EXPECT_EQ(apply(R"cpp( + void foo() /*Comment -_-*/ /*Com 2*/ ; + void fo^o() { return ; })cpp"), + R"cpp( + void foo() /*Comment -_-*/ /*Com 2*/ { return ; } + )cpp"); + + EXPECT_EQ(apply(R"cpp( + void foo(); + void fo^o() { return ; })cpp"), + R"cpp( + void foo(){ return ; } + )cpp"); + + EXPECT_EQ(apply(R"cpp( + #define SEMI ; + void foo() SEMI + void fo^o() { return ; })cpp"), + "fail: Couldn't find semicolon for target declaration."); +} + +TEST_F(DefineInlineTest, HandleMacros) { + EXPECT_UNAVAILABLE(R"cpp( + #define BODY { return; } + void foo(); + void f^oo()BODY)cpp"); + + EXPECT_UNAVAILABLE(R"cpp( + #define BODY void foo(){ return; } + void foo(); + [[BODY]])cpp"); + + // We don't qualify declarations coming from macros. + EXPECT_EQ(apply(R"cpp( + #define BODY Foo + namespace a { class Foo{}; } + void foo(); + using namespace a; + void f^oo(){BODY})cpp"), + R"cpp( + #define BODY Foo + namespace a { class Foo{}; } + void foo(){BODY} + using namespace a; + )cpp"); + + // Macro is not visible at declaration location, but we proceed. + EXPECT_EQ(apply(R"cpp( + void foo(); + #define BODY return; + void f^oo(){BODY})cpp"), + R"cpp( + void foo(){BODY} + #define BODY return; + )cpp"); + + EXPECT_EQ(apply(R"cpp( + #define TARGET void foo() + TARGET; + void f^oo(){ return; })cpp"), + R"cpp( + #define TARGET void foo() + TARGET{ return; } + )cpp"); + + EXPECT_EQ(apply(R"cpp( + #define TARGET foo + void TARGET(); + void f^oo(){ return; })cpp"), + R"cpp( + #define TARGET foo + void TARGET(){ return; } + )cpp"); +} + } // namespace } // namespace clangd } // namespace clang