kadircet updated this revision to Diff 222180. kadircet added a comment. - Add renaming of template and function parameters
Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66647/new/ https://reviews.llvm.org/D66647 Files: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp clang-tools-extra/clangd/unittests/TweakTesting.cpp clang-tools-extra/clangd/unittests/TweakTesting.h clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/TweakTests.cpp +++ 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" @@ -103,7 +104,7 @@ EXPECT_UNAVAILABLE(R"cpp(R"(multi )" ^"token " u8"str\ning")cpp"); // nonascii EXPECT_UNAVAILABLE(R"cpp(^R^"^(^multi )" "token " "str\ning")cpp"); // raw EXPECT_UNAVAILABLE(R"cpp(^"token\n" __FILE__)cpp"); // chunk is macro - EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp"); // forbidden escape char + EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp"); // forbidden escape char const char *Input = R"cpp(R"(multi token)" "\nst^ring\n" "literal")cpp"; @@ -815,6 +816,497 @@ })cpp"); } +TEST_F(DefineInlineTest, TransformUsings) { + EXPECT_EQ(apply(R"cpp( + namespace a { + void bar(); + namespace b { + void baz(); + namespace c { + void aux(); + } + } + } + + void foo(); + void f^oo() { + using namespace a; + + using namespace b; + using namespace a::b; + + using namespace c; + using namespace b::c; + using namespace a::b::c; + + using a::bar; + + using b::baz; + using a::b::baz; + + using c::aux; + using b::c::aux; + using a::b::c::aux; + + namespace d = c; + namespace d = b::c; + } + )cpp"), + R"cpp( + namespace a { + void bar(); + namespace b { + void baz(); + namespace c { + void aux(); + } + } + } + + void foo(){ + using namespace a; + + using namespace a::b; + using namespace a::b; + + using namespace a::b::c; + using namespace a::b::c; + using namespace a::b::c; + + using a::bar; + + using a::b::baz; + using a::b::baz; + + using a::b::c::aux; + using a::b::c::aux; + using a::b::c::aux; + + namespace d = a::b::c; + namespace d = a::b::c; + } + + )cpp"); +} + +TEST_F(DefineInlineTest, TransformDecls) { + EXPECT_EQ(apply(R"cpp( + void foo() /*Comment -_-*/ ; + + 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; + + template <typename T> class Bar {}; + Bar<int> z; + } + )cpp"), + R"cpp( + void foo() /*Comment -_-*/ { + 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; + + template <typename T> class Bar {}; + Bar<int> z; + } + + + )cpp"); +} + +TEST_F(DefineInlineTest, TransformTemplDecls) { + EXPECT_EQ(apply(R"cpp( + namespace a { + template <typename T> class Bar { + public: + void bar(); + }; + template <typename T> T bar; + template <typename T> void aux() {} + } + + void foo() /*Comment -_-*/ ; + + void f^oo() { + using namespace a; + bar<Bar<int>>.bar(); + aux<Bar<int>>(); + } + )cpp"), + R"cpp( + namespace a { + template <typename T> class Bar { + public: + void bar(); + }; + template <typename T> T bar; + template <typename T> void aux() {} + } + + void foo() /*Comment -_-*/ { + using namespace a; + a::bar<a::Bar<int>>.bar(); + a::aux<a::Bar<int>>(); + } + + + )cpp"); +} + +MATCHER_P2(FileWithContents, FileName, Contents, "") { + return arg.first() == FileName && arg.second == Contents; +} + +TEST_F(DefineInlineTest, TransformMembers) { + EXPECT_EQ(apply(R"cpp( + class Foo { + void foo() /*Comment -_-*/ ; + }; + + void Foo::f^oo() { + return; + } + )cpp"), + R"cpp( + class Foo { + void foo() /*Comment -_-*/ { + return; + } + }; + + + )cpp"); + + ExtraFiles["a.h"] = R"cpp( + class Foo { + void foo() /*Comment -_-*/ ; + };)cpp"; + EXPECT_EQ(apply(R"cpp( + #include "a.h" + void Foo::f^oo() { + return; + })cpp"), + R"cpp( + #include "a.h" + )cpp"); + EXPECT_THAT(EditedFiles, + testing::ElementsAre(FileWithContents(testPath("a.h"), + R"cpp( + class Foo { + void foo() /*Comment -_-*/ { + return; + } + };)cpp"))); +} + +TEST_F(DefineInlineTest, TransformDependentTypes) { + EXPECT_EQ(apply(R"cpp( + namespace a { + template <typename T> class Bar {}; + } + using namespace a; + + template <typename T> + void foo() /*Comment -_-*/ ; + + template <typename T> + void f^oo() { + Bar<T> B; + Bar<Bar<T>> q; + } + )cpp"), + R"cpp( + namespace a { + template <typename T> class Bar {}; + } + using namespace a; + + template <typename T> + void foo() /*Comment -_-*/ { + a::Bar<T> B; + a::Bar<a::Bar<T>> q; + } + + + )cpp"); +} + +TEST_F(DefineInlineTest, TransformFunctionTempls) { + EXPECT_EQ(apply(R"cpp( + template <typename T> + void foo(T p); + + template <> + void foo<int>(int p); + + template <> + void foo<char>(char p); + + template <> + void fo^o<int>(int p) { + return; + } + )cpp"), + R"cpp( + template <typename T> + void foo(T p); + + template <> + void foo<int>(int p){ + return; + } + + template <> + void foo<char>(char p); + + + )cpp"); + + EXPECT_EQ(apply(R"cpp( + template <typename T> + void foo(T p); + + template <> + void foo<int>(int p); + + template <> + void foo<char>(char p); + + template <> + void fo^o<char>(char p) { + return; + } + )cpp"), + R"cpp( + template <typename T> + void foo(T p); + + template <> + void foo<int>(int p); + + template <> + void foo<char>(char p){ + return; + } + + + )cpp"); + + EXPECT_EQ(apply(R"cpp( + template <typename T> + void foo(T p); + + template <> + void foo<int>(int p); + + template <typename T> + void fo^o(T p) { + return; + } + )cpp"), + R"cpp( + template <typename T> + void foo(T p){ + return; + } + + template <> + void foo<int>(int p); + + + )cpp"); +} + +TEST_F(DefineInlineTest, TransformTypeLocs) { + EXPECT_EQ(apply(R"cpp( + namespace a { + template <typename T> class Bar { + public: + template <typename Q> class Baz {}; + }; + namespace b{ + class Foo{}; + }; + } + using namespace a; + using namespace b; + using namespace c; + + void foo() /*Comment -_-*/ ; + + void f^oo() { + Bar<int> B; + b::Foo foo; + a::Bar<Bar<int>>::Baz<Bar<int>> q; + } + )cpp"), + R"cpp( + namespace a { + template <typename T> class Bar { + public: + template <typename Q> class Baz {}; + }; + namespace b{ + class Foo{}; + }; + } + using namespace a; + using namespace b; + using namespace c; + + void foo() /*Comment -_-*/ { + a::Bar<int> B; + a::b::Foo foo; + a::Bar<a::Bar<int>>::Baz<a::Bar<int>> q; + } + + + )cpp"); +} + +TEST_F(DefineInlineTest, TransformDeclRefs) { + EXPECT_EQ(apply(R"cpp( + namespace a { + template <typename T> class Bar { + public: + void foo(); + static void bar(); + int x; + static int y; + }; + void bar(); + namespace b { + class Foo{}; + namespace c { + void test(); + } + } + } + using namespace a; + using namespace b; + using namespace c; + + void foo() /*Comment -_-*/ ; + + void f^oo() { + a::Bar<int> B; + B.foo(); + a::bar(); + Bar<Bar<int>>::bar(); + a::Bar<int>::bar(); + B.x = Bar<int>::y; + Bar<int>::y = 3; + bar(); + c::test(); + } + )cpp"), + R"cpp( + namespace a { + template <typename T> class Bar { + public: + void foo(); + static void bar(); + int x; + static int y; + }; + void bar(); + namespace b { + class Foo{}; + namespace c { + void test(); + } + } + } + using namespace a; + using namespace b; + using namespace c; + + void foo() /*Comment -_-*/ { + a::Bar<int> B; + B.foo(); + a::bar(); + a::Bar<a::Bar<int>>::bar(); + a::Bar<int>::bar(); + B.x = a::Bar<int>::y; + a::Bar<int>::y = 3; + a::bar(); + a::b::c::test(); + } + + + )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, TransformParamNames) { + EXPECT_EQ(apply(R"cpp( + void foo(int, bool b); + + void ^foo(int f, bool x) {})cpp"), R"cpp( + void foo(int f, bool x){} + + )cpp"); +} + +TEST_F(DefineInlineTest, TransformTemplParamNames) { + EXPECT_EQ(apply(R"cpp( + struct Foo { + struct Bar { + template <class, class X, + template<typename> class, template<typename> class Y, + int, int Z> + void foo(); + }; + }; + + template <class T, class U, + template<typename> class V, template<typename> class W, + int X, int Y> + void Foo::Bar::f^oo() {})cpp"), R"cpp( + struct Foo { + struct Bar { + template <class T, class U, + template<typename> class V, template<typename> class W, + int X, int Y> + void foo(){} + }; + }; + + )cpp"); +} } // namespace } // namespace clangd } // namespace clang Index: clang-tools-extra/clangd/unittests/TweakTesting.h =================================================================== --- clang-tools-extra/clangd/unittests/TweakTesting.h +++ clang-tools-extra/clangd/unittests/TweakTesting.h @@ -51,6 +51,7 @@ // Mapping from file name to contents. llvm::StringMap<std::string> ExtraFiles; + llvm::StringMap<std::string> EditedFiles; protected: TweakTest(const char *TweakID) : TweakID(TweakID) {} @@ -65,13 +66,15 @@ // 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 any other + // changes. // 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<message>" // - if prepare() returns false, returns "unavailable" // - if apply() returns an error, returns "fail: <message>" - std::string apply(llvm::StringRef MarkedCode) const; + std::string apply(llvm::StringRef MarkedCode); // Accepts a code snippet with many ranges (or points) marked, and returns a // list of snippets with one range marked each. Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp =================================================================== --- clang-tools-extra/clangd/unittests/TweakTesting.cpp +++ clang-tools-extra/clangd/unittests/TweakTesting.cpp @@ -10,9 +10,11 @@ #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 <string> namespace clang { namespace clangd { @@ -79,12 +81,13 @@ } // namespace -std::string TweakTest::apply(llvm::StringRef MarkedCode) const { +std::string TweakTest::apply(llvm::StringRef MarkedCode) { 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(); ParsedAST AST = TU.build(); Tweak::Selection S(AST, Selection.first, Selection.second); @@ -101,14 +104,19 @@ 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 + EditedFiles.try_emplace(It.first(), Unwrapped.str()); + } + return EditedMainFile; } ::testing::Matcher<llvm::StringRef> TweakTest::isAvailable() const { Index: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp =================================================================== --- clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp +++ 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,48 @@ #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 <cstddef> #include <set> #include <string> #include <unordered_map> +#include <utility> #include <vector> namespace clang { namespace clangd { namespace { +// Lexes from end of \p FD until it finds a semicolon. +llvm::Optional<SourceLocation> getSemiColonForDecl(const FunctionDecl *FD) { + const SourceManager &SM = FD->getASTContext().getSourceManager(); + const LangOptions &LangOpts = FD->getASTContext().getLangOpts(); + + SourceLocation SemiColon = + Lexer::getLocForEndOfToken(FD->getEndLoc(), 0, SM, LangOpts); + llvm::StringRef BufData = SM.getBufferData(SM.getFileID(SemiColon)); + Lexer RawLexer(SM.getLocForStartOfFile(SM.getFileID(SemiColon)), LangOpts, + BufData.begin(), SM.getCharacterData(SemiColon), + BufData.end()); + Token CurToken; + while (!CurToken.is(tok::semi)) { + if (RawLexer.LexFromRawLexer(CurToken)) + return llvm::None; + } + SemiColon = CurToken.getLocation(); + return SemiColon; +} + // 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. @@ -115,6 +141,132 @@ return true; } +// Rewrites body of FD to fully-qualify all of the decls inside. +llvm::Expected<std::string> 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; + std::string Failure; + findExplicitReferences(FD->getBody(), [&](ReferenceLoc Ref) { + if (!Failure.empty()) + return; + // Since we want to qualify only the first qualifier, skip names with a + // qualifier. + if (Ref.Qualifier.hasQualifier()) + return; + // There might be no decl in dependent contexts, there's nothing much we can + // do in such cases. + if (Ref.Targets.empty()) + return; + + // All Targets should be in the same nested name scope, so we can safely + // chose any one of them. + 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))) { + Failure = llvm::formatv("Failed to add qualifier: {0}", + llvm::fmt_consume(std::move(Err))); + elog(Failure.c_str()); + } + }); + + if (!Failure.empty()) + return llvm::createStringError(llvm::inconvertibleErrorCode(), Failure); + + auto QualifiedFunc = tooling::applyAllReplacements( + SM.getBufferData(SM.getFileID(FD->getLocation())), Replacements); + if (!QualifiedFunc) + return QualifiedFunc.takeError(); + + // Get new begin and end positions for the qualified body. + SourceRange OrigFuncRange = FD->getBody()->getSourceRange(); + unsigned BodyBegin = SM.getFileOffset(OrigFuncRange.getBegin()); + unsigned BodyEnd = Replacements.getShiftedCodePosition( + SM.getFileOffset(OrigFuncRange.getEnd())); + + // Trim the result to function body. + return QualifiedFunc->substr(BodyBegin, BodyEnd - BodyBegin + 1); +} + +/// Generates Replacements for changing parameter names in \p Dest to be the +/// same as in \p Source. +llvm::Expected<tooling::Replacements> +renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) { + const SourceManager &SM = Dest->getASTContext().getSourceManager(); + tooling::Replacements Replacements; + auto GenerateReplacements = [&](const NamedDecl *Dest, + const NamedDecl *Source) -> llvm::Error { + llvm::StringRef DestName = Dest->getName(); + llvm::StringRef SourceName = Source->getName(); + if (DestName == SourceName) + return llvm::Error::success(); + if (auto Err = Replacements.add(tooling::Replacement( + SM, Dest->getLocation(), DestName.size(), + ((DestName.empty() ? " " : "") + SourceName).str()))) { + return Err; + } + return llvm::Error::success(); + }; + + // Fix function parameter names. + assert(Dest->param_size() == Source->param_size()); + for (size_t I = 0, E = Dest->param_size(); I != E; ++I) { + if (auto Err = GenerateReplacements(Dest->getParamDecl(I), + Source->getParamDecl(I))) + return std::move(Err); + } + + // Fix function template parameter list. + auto *DestTempl = Dest->getDescribedFunctionTemplate(); + auto *SourceTempl = Source->getDescribedFunctionTemplate(); + assert(bool(DestTempl) == bool(SourceTempl)); + + // Nothing to do if function is not a template. + if (!DestTempl) + return Replacements; + + const auto *DestTPL = DestTempl->getTemplateParameters(); + const auto *SourceTPL = SourceTempl->getTemplateParameters(); + assert(DestTPL->size() == SourceTPL->size()); + + for (size_t I = 0, EP = DestTPL->size(); I != EP; ++I) { + if (auto Err = + GenerateReplacements(DestTPL->getParam(I), SourceTPL->getParam(I))) + return std::move(Err); + } + return Replacements; +} + // 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 +288,15 @@ 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 +320,6 @@ 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. @@ -198,8 +358,58 @@ } Expected<Effect> apply(const Selection &Sel) override { - return llvm::createStringError(llvm::inconvertibleErrorCode(), - "Not implemented yet"); + const ASTContext &AST = Sel.AST.getASTContext(); + const SourceManager &SM = AST.getSourceManager(); + + auto SemiColon = getSemiColonForDecl(Target); + if (!SemiColon) { + return llvm::createStringError( + llvm::inconvertibleErrorCode(), + "Couldn't find semicolon for target declaration"); + } + + auto ParamReplacements = renameParameters(Target, Source); + if (!ParamReplacements) + return ParamReplacements.takeError(); + + auto QualifiedBody = qualifyAllDecls(Source); + if (!QualifiedBody) + return QualifiedBody.takeError(); + + const tooling::Replacement SemiColonToFuncBody(SM, *SemiColon, 1, + *QualifiedBody); + SourceLocation BeginLoc = getBeginLoc(Source); + unsigned int SourceLen = + SM.getFileOffset(Source->getEndLoc()) - SM.getFileOffset(BeginLoc) + 1; + const tooling::Replacement DeleteFuncBody(SM, BeginLoc, SourceLen, ""); + + llvm::SmallVector<std::pair<std::string, Edit>, 2> Edits; + // Edit for Target. + auto FE = Effect::fileEdit( + SM, SM.getFileID(*SemiColon), + tooling::Replacements(SemiColonToFuncBody).merge(*ParamReplacements)); + if (!FE) + return FE.takeError(); + Edits.push_back(std::move(*FE)); + + // Edit for Source. + if (!SM.isWrittenInSameFile(Source->getBeginLoc(), 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:
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits