[clangd] Rename references to function arguments within the same file

[kbobyrev]

Summary

This is a fix of the problem (rather than a patch - D96247) discussed in
https://github.com/clangd/clangd/issues/685.

[kbobyrev]

Implement constructors handling.

[kbobyrev]

This likely needs some further thought on conflict detection. Assume we have

void foo(int Argument^); // case 1
void foo(int OhWait); // case 2
void foo(int Argument); // case 3

void foo(int AnotherOne) {} // case 4

What should we do when renaming Argument? Just rename all of them? Only rename in places where the name matches the original one (case 3)? Bail out and report an error? Also, there is no easy way to check all of these cases within our existing conflict detection pipeline: IIUC we'd need to walk the whole AST again for that (like we do in this patch within the rename pipeline).

[kbobyrev]

Rebase on top of D96247, check for conflicts in specialization bodies.

[kbobyrev]

@sammccall this is the argument renaming diff I was talking about during the standup.

[kbobyrev]

Ping! Do you think this is a reasonable idea? I think if we just check whether there aren't any references to the function outside of the main file we should be safe enough to rename args.

[kadircet]

I think this is a great use case for pseudo parser actually (unfortunately requires some multi-file interactions). In theory we can just fetch all the references, pseudo-parse the files, and update all the declarations & definitions to have the same parameter name.

My 2 cents for limiting to single-file cases initially;
It sounds like a better option to make the functionality less surprising, but I think we'll still end up confusing the users, so I am not sure if it is worth.

clang-tools-extra/clangd/refactor/Rename.cpp

 //    template <typename T>
 //    bool Foo<T, 0> = true; (2)
 //
 //    template <>
 //    bool Foo<int, 0> = true; (3)
 //
 // Here, both partial (2) and full (3) specializations are canonicalized to (1)
 // which ensures all three of them are renamed.
-const NamedDecl *canonicalRenameDecl(const NamedDecl *D) {
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D,
+                                     bool CanonicalizeCtorToType = true) {
   if (const auto *VarTemplate = dyn_cast<VarTemplateSpecializationDecl>(D))
     return canonicalRenameDecl(
         VarTemplate->getSpecializedTemplate()->getTemplatedDecl());
   if (const auto *Template = dyn_cast<TemplateDecl>(D))
     if (const NamedDecl *TemplatedDecl = Template->getTemplatedDecl())
       return canonicalRenameDecl(TemplatedDecl);
   if (const auto *ClassTemplateSpecialization =
           dyn_cast<ClassTemplateSpecializationDecl>(D))
     return canonicalRenameDecl(
         ClassTemplateSpecialization->getSpecializedTemplate()
             ->getTemplatedDecl());
   if (const auto *Method = dyn_cast<CXXMethodDecl>(D)) {
-    if (Method->getDeclKind() == Decl::Kind::CXXConstructor ||
+    // By default ctors and dtors are canonicalized to the returned type but
+    // when CanonicalizeCtorToType is not set, we should still retrieve the
+    // canonical method.
+    if ((Method->getDeclKind() == Decl::Kind::CXXConstructor &&
+         CanonicalizeCtorToType) ||
         Method->getDeclKind() == Decl::Kind::CXXDestructor)
       return canonicalRenameDecl(Method->getParent());
+    if (const FunctionTemplateDecl *Template = Method->getPrimaryTemplate())
+      if (const auto *TemplatedDecl =
+              dyn_cast<CXXMethodDecl>(Template->getTemplatedDecl()))
+        Method = TemplatedDecl;
     if (const FunctionDecl *InstantiatedMethod =
             Method->getInstantiatedFromMemberFunction())
       Method = cast<CXXMethodDecl>(InstantiatedMethod);
     // FIXME(kirillbobyrev): For virtual methods with
     // size_overridden_methods() > 1, this will not rename all functions it
     // overrides, because this code assumes there is a single canonical
     // declaration.
     while (Method->isVirtual() && Method->size_overridden_methods())
if (const auto *Field = dyn_cast<FieldDecl>(D)) {
     for (const FieldDecl *Candidate : FieldParent->fields())
       if (Field->getDeclName() == Candidate->getDeclName())
         return Candidate->getCanonicalDecl();
     elog("FieldParent should have field with the same name as Field.");
   }
   if (const auto *VD = dyn_cast<VarDecl>(D)) {
     if (const VarDecl *OriginalVD = VD->getInstantiatedFromStaticDataMember())
       VD = OriginalVD;
+    // Arguments are canonicalized to their canonical function's arguments.
+    if (const auto *Argument = dyn_cast<ParmVarDecl>(VD)) {
+      if (const auto *Function =
+              dyn_cast<FunctionDecl>(Argument->getDeclContext())) {
+        const auto *Canonical = dyn_cast<FunctionDecl>(
+            canonicalRenameDecl(Function, /*CanonicalizeCtorToType=*/false));
+        assert(Canonical &&
+               "canonicalRenameDecl should return the declaration "
+               "of the same type with CanonicalizeCtorToType=true.");
+        return Canonical->getParamDecl(Argument->getFunctionScopeIndex())
+            ->getCanonicalDecl();
+      }
+    }
     return VD->getCanonicalDecl();
   }
   return dyn_cast<NamedDecl>(D->getCanonicalDecl());
 }
 
 llvm::DenseSet<const NamedDecl *> locateDeclAt(ParsedAST &AST,
                                                SourceLocation TokenStartLoc) {
   unsigned Offset =
if (const auto *Result = CheckDeclStmt(
       return Result;
     return CheckCompoundStmt(EnclosingFor->getBody(), NewName);
   }
   if (const auto *EnclosingFunction = Parent->get<FunctionDecl>()) {
     // Check for conflicts with other arguments.
     for (const auto *Parameter : EnclosingFunction->parameters())
       if (Parameter != &RenamedDecl && Parameter->getName() == NewName)
         return Parameter;
-    // FIXME: We don't modify all references to function parameters when
-    // renaming from forward declaration now, so using a name colliding with
-    // something in the definition's body is a valid transformation.
-    if (!EnclosingFunction->doesThisDeclarationHaveABody())
-      return nullptr;
+    EnclosingFunction = dyn_cast<FunctionDecl>(canonicalRenameDecl(
+        EnclosingFunction, /*CanonicalizeCtorToType=*/false));
+    assert(EnclosingFunction &&
+           "canonicalRenameDecl should return the declaration "
+           "of the same type with CanonicalizeCtorToType=false.");
+    // Because we will be renaming parameter in all specializations, check
+    // their bodies for conflict, too.
+    if (const auto *Template =
+            EnclosingFunction->getDescribedFunctionTemplate())
+      for (const auto *Spec : Template->specializations())
+        if (const auto *Result = CheckCompoundStmt(Spec->getBody(), NewName))
+          return Result;
     return CheckCompoundStmt(EnclosingFunction->getBody(), NewName);
   }
 
   return nullptr;
 }
 
 // Lookup the declarations (if any) with the given Name in the context of
 // RenameDecl.

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

llvm::StringRef Tests[] = {
             Baz = Foo;
             Foo = A::[[F^oo]] + Baz;
             A::[[Fo^o]] = b.Foo;
           }
           Foo = b.Foo;
         }
       )cpp",
 
+      // Function argument.
+      R"cpp(
+        // Main.cpp
+        void foo(int [[Bar^]]);
+
+        void foo(int [[Bar^]]) {}
+
+        // Forward declarations somewhere else.
+        void foo(int [[Bar^]]);
+        void foo(int /*Bar*/);
+      )cpp",
+
+      // Template function argument.
+      R"cpp(
+        template <typename T>
+        void foo(T [[Bar^]]);
+
+        template <typename T>
+        void foo(T [[Bar^]]) {}
+      )cpp",
+      R"cpp(
+        template <typename T>
+        void foo(T [[Bar^]]);
+
+        template <typename T>
+        void foo(T [[Bar^]]) {}
+
+        template <>
+        void foo(int [[Bar^]]) {}
+      )cpp",
+
+      // Constructor argument.
+      R"cpp(
+        class Foo {
+          Foo(int [[Bar^]]);
+        };
+        Foo::Foo(int [[Bar^]]) {}
+      )cpp",
+      R"cpp(
+        class Foo {
+          template <typename T, typename U>
+          Foo(T Something, U [[Bar^]]);
+
+          Foo();
+        };
+
+        template <typename T, typename U>
+        Foo::Foo(T Something, U [[Bar^]]) {}
+
+        template <>
+        Foo::Foo(int Something, int [[Bar^]]) {}
+        template <>
+        Foo::Foo(bool Something, bool [[Bar^]]) {}
+      )cpp",
+
       // Namespace alias.
       R"cpp(
         namespace a { namespace b { void foo(); } }
         namespace [[^x]] = a::b;
         void bar() {
           [[x^]]::foo();
         }
       )cpp",
Case Cases[] = {
         void func(int Var);
 
         void func(int V^ar) {
           bool Conflict;
         }
       )cpp",
        "conflict", !HeaderFile, nullptr, "Conflict"},
 
-      {R"cpp(// No conflict: only forward declaration's argument is renamed.
-        void func(int [[V^ar]]);
+      {R"cpp(
+        template <typename T>
+        void func(T Var);
+
+        template <>
+        void func(bool V^ar) {}
 
+        template <>
         void func(int Var) {
-          bool Conflict;
+          // We are renaming from the primary template but the parameter name
+          // will also be changed here introducing a new conflict.
+          int Conflict;
         }
       )cpp",
-       nullptr, !HeaderFile, nullptr, "Conflict"},
+       "conflict", !HeaderFile, nullptr, "Conflict"},
 
       {R"cpp(
         void func(int V^ar, int Conflict) {
         }
       )cpp",
        "conflict", !HeaderFile, nullptr, "Conflict"},
 
       {R"cpp(// Trying to rename into the same name, SameName == SameName.