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

[kadircet]

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.

[ilya-biryukov]

An alternative approach I'm thinking of:
After D68977 lands, we could try using findExplicitReferences to produce all of these edits:

1. we collect locations of all references and declaration of relevant parameters and template parameters.
2. find the ones where the name differs and produce the changes.

That would handle not only references in the default argument, but also all references in the body and should be less code overall.
IMO it's worth exploring this approach right away, e.g. you could layer your patch on top of the current version of D68977

clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp

229

We only need name from the Source. Why not accept it as a StringRef Name directly?
Would allow to call this function even if Source is not a NamedDecl

[kadircet]

• Accept StringRef for Source in rewriteParameterName instead of a NamedDecl

[kadircet]

In D68937#1710634, @ilya-biryukov wrote:

An alternative approach I'm thinking of:
After D68977 lands, we could try using findExplicitReferences to produce all of these edits:

1. we collect locations of all references and declaration of relevant parameters and template parameters.
2. find the ones where the name differs and produce the changes.

That would handle not only references in the default argument, but also all references in the body and should be less code overall.
IMO it's worth exploring this approach right away, e.g. you could layer your patch on top of the current version of D68977

I totally agree that the solution you proposed would also work, but don't think it would be any less code. Since one needs to correlate
parameters between two different declarations, and findExplicitReferences doesn't really provide a nice way to achieve that. One
would have to rely on SourceLocation ordering or in the order callback was issued(which implicitly relies on AST traversal order).

It would be nice to unify the rewrite parameter name/type/defaultarg logics, but not sure if it is worth.

[kuhnel]

Build failed as patch failed to apply:

00:00:06.098 Checking patch clang-tools-extra/trunk/clangd/FindTarget.h...
00:00:06.100 error: clang-tools-extra/trunk/clangd/FindTarget.h: does not exist in index
00:00:06.100 Checking patch clang-tools-extra/trunk/clangd/FindTarget.cpp...
00:00:06.100 error: clang-tools-extra/trunk/clangd/FindTarget.cpp: does not exist in index
00:00:06.101 
00:00:06.101  Patch Failed! 
00:00:06.101 Usage Exception: Unable to apply patch!

[kadircet]

• Rebase

[ilya-biryukov]

In D68937#1710915, @kadircet wrote:

I totally agree that the solution you proposed would also work, but don't think it would be any less code. Since one needs to correlate
parameters between two different declarations, and findExplicitReferences doesn't really provide a nice way to achieve that. One
would have to rely on SourceLocation ordering or in the order callback was issued(which implicitly relies on AST traversal order).

It would be nice to unify the rewrite parameter name/type/defaultarg logics, but not sure if it is worth.

I believe it should be less and much simpler code. In particular, I propose something like this:

DenseSet<const NameDecl*> ParametersToRename;
// Fill ParametersToRename with template and function parameters.

DenseMap<const NamedDecl*, std::vector<SourceLocation>> References;
findExplicitReferences(Func, [&](ReferenceLoc R) {
  if (!ParametersToRename.count(R.Target))
    return;
  References[R.Target].push_back(R.NameLoc);
});

for (auto [Param, Locations] : References) {
  auto NewName = NewNameForParam(Param);
  for (auto L : Locations)
    Replacements.add(replaceToken(L, NewName));
}

That should also handle various interesting cases like try-catch blocks and constructor initializer lists.

clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp

229

NIT: that renames \p Dest to \p SourceName

232

NIT: call it NewName?
Source and Target are very define-inline-specific. This function actually does a more general thing and using the define-inline terminology hurt the redability a little.

239

Why do we need to insert a space here? could you add an example?
I guess it's

void foo(int^) // <-- avoid gluing 'int' and the new name here

241

NIT: could you move this expression and a comment to a separate variable?
should simplify the if condition and improve readability

[ilya-biryukov]

Ah, we're actually trying to rename parameters in the declaration to match the ones in the definition... So the try-catch blocks are not a problem, actually

[kadircet]

• Use findExplicitReferences for decl traversals.

[merge_guards_bot]

Build result: fail - 59631 tests passed, 1 failed and 805 were skipped.

failed: LLVM.tools/llvm-ar/mri-utf8.test

Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

[kadircet]

• Don't rename if parameters have the same name

[merge_guards_bot]

Build result: pass - 59695 tests passed, 0 failed and 763 were skipped.
Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

[ilya-biryukov]

clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp

278

why not llvm::cast? We'd rather fail early (during the cast) than postpone it until we dereference Target a few lines further

285

Could we measure the token length instead?
There are various bizzare cases when the source text is different and we definitely don't want to accidentally crash on those, e.g.

int a = N\
A\
ME;

295

Wow, TIL I learned something new. Thanks.

[kadircet]

• Address comments

[ilya-biryukov]

D69511 broke the anonymous parameters. Sorry about that, I hope that's for the best in the long run :-)
We'll need some code to update this patch. Other than that, I think this patch is good to go!

[ilya-biryukov]

clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp

233

NIT: instead of returning whether Dest is a template, we could instead:

1. accept a TemplateDecl* Dest and TemplateDecl *Source,
2. check whether functions are template at the call site and only call fillTemplateParameterMapping when they're template.

I believe that would make both functions simpler and more straight-forward. But up to you.

[merge_guards_bot]

Build result: fail - 59657 tests passed, 2 failed and 805 were skipped.

failed: libc++.libcxx/thread/thread_threads/thread_thread_this/sleep_for.pass.cpp
failed: LLVM.tools/llvm-ar/mri-utf8.test

Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

[kadircet]

• Handle unnamed parameters explicitly

[merge_guards_bot]

Build result: pass - 59704 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt, CMakeCache.txt

[ilya-biryukov]

Just a few final NITs from my side.
And would also be nice to get tests with macros.

clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp

251

NIT: maybe use assignment syntax?
NewName += SourceParam->getName()

270

NIT: also add assert(Source->param_size() == Dest->param_size())?

275

s/signautre/signature

clang-tools-extra/clangd/unittests/TweakTests.cpp

1501

Could you add tests with some parameter references inside macro expansions?
To make sure we don't crash and don't produce invalid edits? Failing is obviously ok

[kadircet]

• Handle macros in parameter names.

[ilya-biryukov]

clang-tools-extra/clangd/unittests/TweakTests.cpp

1526

We fail to rename the parameter in that case, right?
Should the action not be available or maybe show an error message?

Also ok with keeping the current behavior if we add a FIXME mentioning it would be nice to fix this.

[merge_guards_bot]

Build result: pass - 59704 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt, CMakeCache.txt

[kadircet]

• Use Lexer::makeFileCharRange

[kadircet]

clang-tools-extra/clangd/unittests/TweakTests.cpp

1526

somehow forgot my stashed changes ...

yes this was supposed to bail out on failure, but leave the macro occurrence if parameter names are the same.

[merge_guards_bot]

Build result: pass - 59704 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt, CMakeCache.txt

[kadircet]

• Add comments
• Early return when generating edits fails

[merge_guards_bot]

Build result: pass - 59704 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt, CMakeCache.txt

[ilya-biryukov]

LGTM, many thanks!

clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp

293

NIT: Maybe move declaration of Replacements here? To make it closer to the use-site.

[kadircet]

• Address comments
• Get rid of raw string literals inside macro calls, to not break stage1 compilers.

[merge_guards_bot]

Build result: pass - 59704 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt, CMakeCache.txt

[thakis]

This is failing on Windows: http://45.33.8.238/win/1481/step_7.txt

[thakis]

clang-tools-extra/clangd/unittests/TweakTests.cpp

1388

You might need something like this in some of the new tests.

[thakis]

Tests are happy again after 1c66d09 , thanks!

clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp

llvm::Expected<std::string> qualifyAllDecls(const FunctionDecl *FD) {
   // 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);
 }
 
+/// 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.
 // For those we return first declaration different than the canonical one.
 // Because canonical declaration points to template decl instead of
 // specialization.
 const FunctionDecl *findTarget(const FunctionDecl *FD) {
   auto CanonDecl = FD->getCanonicalDecl();
Expected<Effect> apply(const Selection &Sel) override {
 
     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);
     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<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));
 
     // 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.

clang-tools-extra/clangd/unittests/TweakTests.cpp

 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <cassert>
 #include <string>
 #include <utility>
 #include <vector>
 
 using ::testing::AllOf;
+using ::testing::ElementsAre;
 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;
 }
std::pair<llvm::StringRef, llvm::StringRef> Cases[] = {
 
           template <>
           void foo<int>(int p);
 
           )cpp"},
   };
   // 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;
 }
 
 TEST_F(DefineInlineTest, TransformTypeLocs) {
   auto Test = R"cpp(
     namespace a {
       template <typename T> class Bar {
       public:
auto Expected = R"cpp(
     }
 
     using namespace a;
     )cpp";
   EXPECT_EQ(apply(Test), Expected);
 }
 
 TEST_F(DefineInlineTest, TransformDeclRefs) {
-  auto Test =R"cpp(
+  auto Test = R"cpp(
     namespace a {
       template <typename T> class Bar {
       public:
         void foo();
         static void bar();
         int x;
         static int y;
       };
TEST_F(DefineInlineTest, StaticMembers) {
   auto Expected = R"cpp(
     namespace ns { class X { static void foo(); void bar(){
       foo();
     } }; }
     )cpp";
   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{}; } } }
     void foo();
 
     using namespace a;
     void ^foo() {Foo foo;})cpp";
   auto Expected = R"cpp(
std::pair<llvm::StringRef, llvm::StringRef> Cases[] = {
           )cpp"},
 
       {R"cpp(
           #define SEMI ;
           void foo() SEMI
           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;
 }
 
 TEST_F(DefineInlineTest, HandleMacros) {
   EXPECT_UNAVAILABLE(R"cpp(
     #define BODY { return; }
     void foo();
     void f^oo()BODY)cpp");
std::pair<llvm::StringRef, llvm::StringRef> Cases[] = {
           #define TARGET foo
           void TARGET();
           void f^oo(){ return; })cpp",
        R"cpp(
           #define TARGET foo
           void TARGET(){ return; }
           )cpp"},
   };
-  for(const auto& Case: Cases)
+  for (const auto &Case : Cases)
     EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang