From 71aa3f7b7e43bf7d2a8a38324b1f9f7b12bbbdfc Mon Sep 17 00:00:00 2001 From: Kadir Cetinkaya Date: Mon, 14 Oct 2019 12:02:24 +0200 Subject: [PATCH] [clangd] Add parameter renaming to define-inline code action Summary: When moving a function definition to declaration location we also need to handle renaming of the both function and template parameters. This patch achives that by making sure every parameter name and dependent type in destination is renamed to their respective name in the source. Reviewers: ilya-biryukov Subscribers: MaskRay, jkorous, arphaman, usaxena95, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D68937 --- .../clangd/refactor/tweaks/DefineInline.cpp | 110 +++++++++++++++++- .../clangd/unittests/TweakTests.cpp | 74 +++++++++++- 2 files changed, 177 insertions(+), 7 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp index 6bce81bae306..f6966f619ade 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp @@ -226,6 +226,107 @@ llvm::Expected qualifyAllDecls(const FunctionDecl *FD) { return QualifiedFunc->substr(BodyBegin, BodyEnd - BodyBegin + 1); } +/// Generates Replacements for changing template and function parameter names in +/// \p Dest to be the same as in \p Source. +llvm::Expected +renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) { + llvm::DenseMap ParamToNewName; + llvm::DenseMap> RefLocs; + auto HandleParam = [&](const NamedDecl *DestParam, + const NamedDecl *SourceParam) { + // No need to rename if parameters already have the same name. + if (DestParam->getName() == SourceParam->getName()) + return; + std::string NewName; + // Unnamed parameters won't be visited in findExplicitReferences. So add + // them here. + if (DestParam->getName().empty()) { + RefLocs[DestParam].push_back(DestParam->getLocation()); + // If decl is unnamed in destination we pad the new name to avoid gluing + // with previous token, e.g. foo(int^) shouldn't turn into foo(intx). + NewName = " "; + } + NewName.append(SourceParam->getName()); + ParamToNewName[DestParam->getCanonicalDecl()] = std::move(NewName); + }; + + // Populate mapping for template parameters. + auto *DestTempl = Dest->getDescribedFunctionTemplate(); + auto *SourceTempl = Source->getDescribedFunctionTemplate(); + assert(bool(DestTempl) == bool(SourceTempl)); + if (DestTempl) { + 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) + HandleParam(DestTPL->getParam(I), SourceTPL->getParam(I)); + } + + // Populate mapping for function params. + assert(Dest->param_size() == Source->param_size()); + for (size_t I = 0, E = Dest->param_size(); I != E; ++I) + HandleParam(Dest->getParamDecl(I), Source->getParamDecl(I)); + + const SourceManager &SM = Dest->getASTContext().getSourceManager(); + const LangOptions &LangOpts = Dest->getASTContext().getLangOpts(); + // Collect other references in function signature, i.e parameter types and + // default arguments. + findExplicitReferences( + // Use function template in case of templated functions to visit template + // parameters. + DestTempl ? llvm::dyn_cast(DestTempl) : llvm::dyn_cast(Dest), + [&](ReferenceLoc Ref) { + if (Ref.Targets.size() != 1) + return; + const auto *Target = + llvm::cast(Ref.Targets.front()->getCanonicalDecl()); + auto It = ParamToNewName.find(Target); + if (It == ParamToNewName.end()) + return; + RefLocs[Target].push_back(Ref.NameLoc); + }); + + // Now try to generate edits for all the refs. + tooling::Replacements Replacements; + for (auto &Entry : RefLocs) { + const auto *OldDecl = Entry.first; + llvm::StringRef OldName = OldDecl->getName(); + llvm::StringRef NewName = ParamToNewName[OldDecl]; + for (SourceLocation RefLoc : Entry.second) { + CharSourceRange ReplaceRange; + // In case of unnamed parameters, we have an empty char range, whereas we + // have a tokenrange at RefLoc with named parameters. + if (OldName.empty()) + ReplaceRange = CharSourceRange::getCharRange(RefLoc, RefLoc); + else + ReplaceRange = CharSourceRange::getTokenRange(RefLoc, RefLoc); + // If occurence is coming from a macro expansion, try to get back to the + // file range. + if (RefLoc.isMacroID()) { + ReplaceRange = Lexer::makeFileCharRange(ReplaceRange, SM, LangOpts); + // Bail out if we need to replace macro bodies. + if (ReplaceRange.isInvalid()) { + auto Err = llvm::createStringError( + llvm::inconvertibleErrorCode(), + "Cant rename parameter inside macro body."); + elog("define inline: {0}", Err); + return std::move(Err); + } + } + + if (auto Err = Replacements.add( + tooling::Replacement(SM, ReplaceRange, NewName))) { + elog("define inline: Couldn't replace parameter name for {0} to {1}: " + "{2}", + OldName, NewName, Err); + 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. @@ -332,6 +433,10 @@ public: "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(); @@ -354,8 +459,9 @@ public: llvm::SmallVector, 2> Edits; // Edit for Target. - auto FE = Effect::fileEdit(SM, SM.getFileID(*Semicolon), - tooling::Replacements(SemicolonToFuncBody)); + auto FE = Effect::fileEdit( + SM, SM.getFileID(*Semicolon), + tooling::Replacements(SemicolonToFuncBody).merge(*ParamReplacements)); if (!FE) return FE.takeError(); Edits.push_back(std::move(*FE)); diff --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp b/clang-tools-extra/clangd/unittests/TweakTests.cpp index f14383b24081..126aab61ada5 100644 --- a/clang-tools-extra/clangd/unittests/TweakTests.cpp +++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp @@ -40,9 +40,9 @@ #include using ::testing::AllOf; +using ::testing::ElementsAre; using ::testing::HasSubstr; using ::testing::StartsWith; -using ::testing::ElementsAre; namespace clang { namespace clangd { @@ -1386,7 +1386,7 @@ TEST_F(DefineInlineTest, TransformFunctionTempls) { // Template body is not parsed until instantiation time on windows, which // results in arbitrary failures as function body becomes NULL. ExtraArgs.push_back("-fno-delayed-template-parsing"); - for(const auto &Case : Cases) + for (const auto &Case : Cases) EXPECT_EQ(apply(Case.first), Case.second) << Case.first; } @@ -1429,7 +1429,7 @@ TEST_F(DefineInlineTest, TransformTypeLocs) { } TEST_F(DefineInlineTest, TransformDeclRefs) { - auto Test =R"cpp( + auto Test = R"cpp( namespace a { template class Bar { public: @@ -1498,6 +1498,70 @@ TEST_F(DefineInlineTest, StaticMembers) { EXPECT_EQ(apply(Test), Expected); } +TEST_F(DefineInlineTest, TransformParamNames) { + std::pair Cases[] = { + {R"cpp( + void foo(int, bool b, int T\ +est); + void ^foo(int f, bool x, int z) {})cpp", + R"cpp( + void foo(int f, bool x, int z){} + )cpp"}, + {R"cpp( + #define PARAM int Z + void foo(PARAM); + + void ^foo(int X) {})cpp", + "fail: Cant rename parameter inside macro body."}, + {R"cpp( + #define TYPE int + #define PARAM TYPE Z + #define BODY(x) 5 * (x) + 2 + template + void foo(PARAM, TYPE Q, TYPE, TYPE W = BODY(P)); + template + void ^foo(int Z, int b, int c, int d) {})cpp", + R"cpp( + #define TYPE int + #define PARAM TYPE Z + #define BODY(x) 5 * (x) + 2 + template + void foo(PARAM, TYPE b, TYPE c, TYPE d = BODY(x)){} + )cpp"}, + }; + for (const auto &Case : Cases) + EXPECT_EQ(apply(Case.first), Case.second) << Case.first; +} + +TEST_F(DefineInlineTest, TransformTemplParamNames) { + auto Test = R"cpp( + struct Foo { + struct Bar { + template class, template class Y, + int, int Z> + void foo(X, Y, int W = 5 * Z + 2); + }; + }; + + template class V, template class W, + int X, int Y> + void Foo::Bar::f^oo(U, W, int Q) {})cpp"; + auto Expected = R"cpp( + struct Foo { + struct Bar { + template class V, template class W, + int X, int Y> + void foo(U, W, int Q = 5 * Y + 2){} + }; + }; + + )cpp"; + EXPECT_EQ(apply(Test), Expected); +} + TEST_F(DefineInlineTest, TransformInlineNamespaces) { auto Test = R"cpp( namespace a { inline namespace b { namespace { struct Foo{}; } } } @@ -1536,7 +1600,7 @@ TEST_F(DefineInlineTest, TokensBeforeSemicolon) { void fo^o() { return ; })cpp", "fail: Couldn't find semicolon for target declaration."}, }; - for(const auto& Case: Cases) + for (const auto &Case : Cases) EXPECT_EQ(apply(Case.first), Case.second) << Case.first; } @@ -1594,7 +1658,7 @@ TEST_F(DefineInlineTest, HandleMacros) { void TARGET(){ return; } )cpp"}, }; - for(const auto& Case: Cases) + for (const auto &Case : Cases) EXPECT_EQ(apply(Case.first), Case.second) << Case.first; }