[clangd] Add parameter renaming to define-inline code action

kadircet · kadircet · commit 71aa3f7b7e43 · 2019-10-31T09:23:09.000+01:00
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
diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
@@ -226,6 +226,107 @@ llvm::Expected<std::string> 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<tooling::Replacements>
+renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) {
+  llvm::DenseMap<const Decl *, std::string> ParamToNewName;
+  llvm::DenseMap<const NamedDecl *, std::vector<SourceLocation>> 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<Decl>(DestTempl) : llvm::dyn_cast<Decl>(Dest),
+      [&](ReferenceLoc Ref) {
+        if (Ref.Targets.size() != 1)
+          return;
+        const auto *Target =
+            llvm::cast<NamedDecl>(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 @@ class DefineInline : public Tweak {
           "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 @@ class DefineInline : public Tweak {
 
     llvm::SmallVector<std::pair<std::string, Edit>, 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
@@ -40,9 +40,9 @@
 #include <vector>
 
 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 <typename T> class Bar {
       public:
@@ -1498,6 +1498,70 @@ TEST_F(DefineInlineTest, StaticMembers) {
   EXPECT_EQ(apply(Test), Expected);
 }
 
+TEST_F(DefineInlineTest, TransformParamNames) {
+  std::pair<llvm::StringRef, llvm::StringRef> 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 <int P>
+        void foo(PARAM, TYPE Q, TYPE, TYPE W = BODY(P));
+        template <int x>
+        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 <int x>
+        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, class X,
+                  template<typename> class, template<typename> class Y,
+                  int, int Z>
+        void foo(X, Y<X>, int W = 5 * Z + 2);
+      };
+    };
+
+    template <class T, class U,
+              template<typename> class V, template<typename> class W,
+              int X, int Y>
+    void Foo::Bar::f^oo(U, W<U>, int Q) {})cpp";
+  auto Expected = 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(U, W<U>, 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;
 }
 
