[clangd] Add support for within-file rename of complicated fields

[kbobyrev]

Summary

This was originally a part of D71880 but is separated for simplicity and ease
of reviewing.

Fixes: https://github.com/clangd/clangd/issues/582

[hokein]

I assume this fixes https://github.com/clangd/clangd/issues/582?

can you check the static members in template classes etc? I think the AST is different.

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

131

yeah, this is a bit unfortunate, I don't have a better solution (extending the AST is one option, but it may be not easy). I think we can live with the heuristic.

137

nit: I think we can simplify the code a bit more:

const auto* Template = Field->getParent()->getTemplateInstantiationPattern();
if (!Template)
  return...;

142

I think we could also check the equality of their names.

143

this assertion seems too strong to me -- this is a heuristics, it may have false positives. I think we can remove it.

149

use elog?

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

545

I think this is a normal rename-field case, it should already work prior to the patch.

575

Another seems to be not needed. I'd suggest removing it to make the testcase as tense as possible.

[kbobyrev]

Resolve most actionable comments.

[kbobyrev]

template<typename T>
struct Foo {
  static T [[Var^iable]];
};

template <>
int Foo<int>::[[Variable^]] = 42;

template <>
bool Foo<bool>::[[Variable^]] = true;

void foo() {
  int LocalInt = Foo<int>::[[Variable^]];
  bool LocalBool = Foo<bool>::[[Variable^]];
}

This doesn't work yet, need to investigate.

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

131

Yes, I thought about that but realized it might be too much effort, maybe I should put a FIXME here.

142

Yes, that is what @sammccall proposed too, but names not simple unsigned numbers, checking for that is just more expensive.

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

575

In this case there is only one field and I'd be happy to check that the correct field gets renamed in case there are two of them (this is when the index comparison is checked). Otherwise this would pass without the index checking (there is one field, we don't check if it's actually the one we need and it gets renamed regardless).

[kbobyrev]

Added support for static templated fields. Ready for the review again.

[hokein]

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

134

sorry, I thought Field->getParent() returns a CXXRecordDecl.

I think this is dangerous for C, getParent returns a RecordDecl, then dyn_cast returns null, and we will access a nullptr.

142

I think I meant to use Field->getDeclName() == Candidate->getDeclName(), DeclarationName comparison is cheap, just comparing with the underlying pointer -- clang will unique all identifiers in the IdentifierTable.

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

575

I see, for index comparison. this makes sense.

613

can you add a template partial testcase? something like

template <typename T, typename U> struct Foo { static T Variable; };

template <typename T> struct Foo<T, bool> {
  static T Variable;
};

void test() {
  Foo<int, bool>::Variable = 5;
}

[kbobyrev]

Resolve remaining comments.

[hokein]

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

132

not sure the FIXME is useful here, we may never do that, I'd just drop it.

138

nit: this if branch is not necessary, because the if below already handles that.

150

nit: we can inline the OriginalVD declaration into the if.

[kbobyrev]

Resolve another round of comments.

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

if (const auto *Method = dyn_cast<CXXMethodDecl>(D)) {
     // declaration.
     while (Method->isVirtual() && Method->size_overridden_methods())
       Method = *Method->overridden_methods().begin();
     return dyn_cast<NamedDecl>(Method->getCanonicalDecl());
   }
   if (const auto *Function = dyn_cast<FunctionDecl>(D))
     if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
       return canonicalRenameDecl(Template);
+  if (const auto *Field = dyn_cast<FieldDecl>(D)) {
+    // This is a hacky way to do something like
+    // CXXMethodDecl::getInstantiatedFromMemberFunction for the field because
+    // Clang AST does not store relevant information about the field that is
+    // instantiated.
+    // FIXME: Maybe a proper way of fixing this would be enhancing Clang AST
+    // although it might be a bigger effort.
+    const auto *FieldParent = dyn_cast<CXXRecordDecl>(Field->getParent());
+    if (!FieldParent)
+      return Field->getCanonicalDecl();
+    FieldParent = FieldParent->getTemplateInstantiationPattern();
+    if (!FieldParent)
+      return Field->getCanonicalDecl();
+    // Field is not instantiation.
+    if (!FieldParent || Field->getParent() == FieldParent)
+      return Field->getCanonicalDecl();
+    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)) {
+    const VarDecl *OriginalVD = VD->getInstantiatedFromStaticDataMember();
+    if (OriginalVD)
+      VD = OriginalVD;
+    return VD->getCanonicalDecl();
+  }
   return dyn_cast<NamedDecl>(D->getCanonicalDecl());
 }
 
 llvm::DenseSet<const NamedDecl *> locateDeclAt(ParsedAST &AST,
                                                SourceLocation TokenStartLoc) {
   unsigned Offset =
       AST.getSourceManager().getDecomposedSpellingLoc(TokenStartLoc).second;
 

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

llvm::StringRef Tests[] = {
         int main() {
           Baz baz;
           baz.[[F^oo]] = 1;
           MACRO(baz.[[F^oo]]);
           int y = baz.[[F^oo]];
         }
       )cpp",
 
+      // Fields in classes & partial and full specialiations.
+      R"cpp(
+        template<typename T>
+        struct Foo {
+          T [[Vari^able]] = 42;
+        };
+
+        void foo() {
+          Foo<int> f;
+          f.[[Varia^ble]] = 9000;
+        }
+      )cpp",
+      R"cpp(
+        template<typename T, typename U>
+        struct Foo {
+          T Variable[42];
+          U Another;
+
+          void bar() {}
+        };
+
+        template<typename T>
+        struct Foo<T, bool> {
+          T [[Var^iable]];
+          void bar() { ++[[Var^iable]]; }
+        };
+
+        void foo() {
+          Foo<unsigned, bool> f;
+          f.[[Var^iable]] = 9000;
+        }
+      )cpp",
+      R"cpp(
+        template<typename T, typename U>
+        struct Foo {
+          T Variable[42];
+          U Another;
+
+          void bar() {}
+        };
+
+        template<typename T>
+        struct Foo<T, bool> {
+          T Variable;
+          void bar() { ++Variable; }
+        };
+
+        template<>
+        struct Foo<unsigned, bool> {
+          unsigned [[Var^iable]];
+          void bar() { ++[[Var^iable]]; }
+        };
+
+        void foo() {
+          Foo<unsigned, bool> f;
+          f.[[Var^iable]] = 9000;
+        }
+      )cpp",
+      // Static fields.
+      R"cpp(
+        struct Foo {
+          static int [[Var^iable]];
+        };
+
+        int Foo::[[Var^iable]] = 42;
+
+        void foo() {
+          int LocalInt = Foo::[[Var^iable]];
+        }
+      )cpp",
+      R"cpp(
+        template<typename T>
+        struct Foo {
+          static T [[Var^iable]];
+        };
+
+        template <>
+        int Foo<int>::[[Var^iable]] = 42;
+
+        template <>
+        bool Foo<bool>::[[Var^iable]] = true;
+
+        void foo() {
+          int LocalInt = Foo<int>::[[Var^iable]];
+          bool LocalBool = Foo<bool>::[[Var^iable]];
+        }
+      )cpp",
+
       // Template parameters.
       R"cpp(
         template <typename [[^T]]>
         class Foo {
           [[T^]] foo([[T^]] arg, [[T^]]& ref, [[^T]]* ptr) {
             [[T]] value;
             int number = 42;
             value = ([[T^]])number;