[clang-rename] Rename enum.

[hokein]

Summary

• Add unit tests for renaming enum.
• Support unscoped enum constants in expressions.

[ioeric]

lib/Tooling/Refactoring/Rename/USRLocFinder.cpp

212 ↗

(On Diff #119263)

Why do we ignore this case? What if Color is moved to a different namespace? We would also need to qualify Green with a new namespace.

218 ↗

(On Diff #119263)

nit: no braces

228 ↗

(On Diff #119263)

I'm wondering why the old EndLoc points to G instead of n in Green.

[hokein]

address review comments, and add FIXME

[hokein]

lib/Tooling/Refactoring/Rename/USRLocFinder.cpp

212 ↗

(On Diff #119263)

Thanks for pointing it out (we missed such test case before).

Yeah, this should be a FIXME, and added the case to the unittest.

228 ↗

(On Diff #119263)

This is the behavior of Expr->getLocEnd() or Expr->getSourceRange().getEnd(), which returns the start location of the last token.

[ioeric]

lib/Tooling/Refactoring/Rename/USRLocFinder.cpp

212 ↗

(On Diff #119263)

It would be nice if this could also be fixed in this patch; I think it might affect the current code structure. This doesn't seem to be a very hard case to fix after all?

228 ↗

(On Diff #119263)

I'm a bit nervous about the implementation - it feels a bit too hacky...

Would it be possible to get the range for the qualifier ns1::ns2:: from the DeclRefExpr?

[hokein]

Use getQualifierLoc.

lib/Tooling/Refactoring/Rename/USRLocFinder.cpp

212 ↗

(On Diff #119263)

To fix this case , we have to insert the prefix qualifier at the BeginLoc, which is not supported by the current code.
I prefer to do it in a follow-up patch.

228 ↗

(On Diff #119263)

Switched to use getQualifierLoc, we still need to exclude the last :: by moving 1 character backward.

[ioeric]

Lg

lib/Tooling/Refactoring/Rename/USRLocFinder.cpp

212 ↗

(On Diff #119263)

Sure. Up to you.

228 ↗

(On Diff #119263)

It might also make sense to double check that the old end loc is pointing at "::".

cfe/trunk/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp

bool VisitNamedDecl(const NamedDecl *Decl) {
     }
     return true;
   }
 
   bool VisitDeclRefExpr(const DeclRefExpr *Expr) {
     const NamedDecl *Decl = Expr->getFoundDecl();
     // Get the underlying declaration of the shadow declaration introduced by a
     // using declaration.
-    if (auto* UsingShadow = llvm::dyn_cast<UsingShadowDecl>(Decl)) {
+    if (auto *UsingShadow = llvm::dyn_cast<UsingShadowDecl>(Decl)) {
       Decl = UsingShadow->getTargetDecl();
     }
 
+    auto BeginLoc = Expr->getLocStart();
+    auto EndLoc = Expr->getLocEnd();
+    // In case of renaming an enum declaration, we have to explicitly handle
+    // unscoped enum constants referenced in expressions (e.g.
+    // "auto r = ns1::ns2::Green" where Green is an enum constant of an unscoped
+    // enum decl "ns1::ns2::Color") as these enum constants cannot be caught by
+    // TypeLoc.
+    if (const auto *T = llvm::dyn_cast<EnumConstantDecl>(Decl)) {
+      // FIXME: Handle the enum constant without prefix qualifiers (`a = Green`)
+      // when renaming an unscoped enum declaration with a new namespace.
+      if (!Expr->hasQualifier())
+        return true;
+
+      if (const auto *ED =
+              llvm::dyn_cast_or_null<EnumDecl>(getClosestAncestorDecl(*T))) {
+        if (ED->isScoped())
+          return true;
+        Decl = ED;
+      }
+      // The current fix would qualify "ns1::ns2::Green" as
+      // "ns1::ns2::Color::Green".
+      //
+      // Get the EndLoc of the replacement by moving 1 character backward (
+      // to exclude the last '::').
+      //
+      //    ns1::ns2::Green;
+      //    ^      ^^
+      // BeginLoc  |EndLoc of the qualifier
+      //           new EndLoc
+      EndLoc = Expr->getQualifierLoc().getEndLoc().getLocWithOffset(-1);
+      assert(EndLoc.isValid() &&
+             "The enum constant should have prefix qualifers.");
+    }
     if (isInUSRSet(Decl)) {
-      RenameInfo Info = {Expr->getSourceRange().getBegin(),
-                         Expr->getSourceRange().getEnd(),
+      RenameInfo Info = {BeginLoc,
+                         EndLoc,
                          Decl,
                          getClosestAncestorDecl(*Expr),
                          Expr->getQualifier(),
                          /*IgnorePrefixQualifers=*/false};
       RenameInfos.push_back(Info);
     }
 
     return true;
while (!TL.isNull()) {
       TL = TL.getNextTypeLoc();
     }
     return false;
   }
 
   // Get the supported declaration from a given typeLoc. If the declaration type
   // is not supported, returns nullptr.
   //
-  // FIXME: support more types, e.g. enum, type alias.
+  // FIXME: support more types, e.g. type alias.
   const NamedDecl *getSupportedDeclFromTypeLoc(TypeLoc Loc) {
     if (const auto *RD = Loc.getType()->getAsCXXRecordDecl())
       return RD;
+    if (const auto *ED =
+            llvm::dyn_cast_or_null<EnumDecl>(Loc.getType()->getAsTagDecl()))
+      return ED;
     return nullptr;
   }
 
   // Get the closest ancester which is a declaration of a given AST node.
   template <typename ASTNodeType>
   const Decl *getClosestAncestorDecl(const ASTNodeType &Node) {
     auto Parents = Context.getParents(Node);
     // FIXME: figure out how to handle it when there are multiple parents.

cfe/trunk/unittests/Rename/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   support
   )
 
 # We'd like clang/unittests/Tooling/RewriterTestContext.h in the test.
 include_directories(${CLANG_SOURCE_DIR})
 
 add_clang_unittest(ClangRenameTests
   RenameClassTest.cpp
+  RenameEnumTest.cpp
   RenameFunctionTest.cpp
   )
 
 target_link_libraries(ClangRenameTests
   clangAST
   clangASTMatchers
   clangBasic
   clangFormat
   clangFrontend
   clangRewrite
   clangTooling
   clangToolingCore
   clangToolingRefactor
   )

cfe/trunk/unittests/Rename/RenameEnumTest.cpp

+#include "ClangRenameTest.h"
+
+namespace clang {
+namespace clang_rename {
+namespace test {
+namespace {
+
+class RenameEnumTest : public ClangRenameTest {
+public:
+  RenameEnumTest() {
+    AppendToHeader(R"(
+        #define MACRO(x) x
+        namespace a {
+        enum A1 { Red };
+        enum class A2 { Blue };
+        struct C {
+         enum NestedEnum { White };
+         enum class NestedScopedEnum { Black };
+        };
+        namespace d {
+        enum A3 { Orange };
+        } // namespace d
+        enum A4 { Pink };
+        } // namespace a
+        enum A5 { Green };)");
+  }
+};
+
+INSTANTIATE_TEST_CASE_P(
+    RenameEnumTests, RenameEnumTest,
+    testing::ValuesIn(std::vector<Case>({
+        {"void f(a::A2 arg) { a::A2 t = a::A2::Blue; }",
+         "void f(b::B2 arg) { b::B2 t = b::B2::Blue; }", "a::A2", "b::B2"},
+        {"void f() { a::A1* t1; }", "void f() { b::B1* t1; }", "a::A1",
+         "b::B1"},
+        {"void f() { a::A2* t1; }", "void f() { b::B2* t1; }", "a::A2",
+         "b::B2"},
+        {"void f() { enum a::A2 t = a::A2::Blue; }",
+         "void f() { enum b::B2 t = b::B2::Blue; }", "a::A2", "b::B2"},
+        {"void f() { enum a::A2 t = a::A2::Blue; }",
+         "void f() { enum b::B2 t = b::B2::Blue; }", "a::A2", "b::B2"},
+
+        {"void f() { a::A1 t = a::Red; }", "void f() { b::B1 t = b::B1::Red; }",
+         "a::A1", "b::B1"},
+        {"void f() { a::A1 t = a::A1::Red; }",
+         "void f() { b::B1 t = b::B1::Red; }", "a::A1", "b::B1"},
+        {"void f() { auto t = a::Red; }", "void f() { auto t = b::B1::Red; }",
+         "a::A1", "b::B1"},
+        {"namespace b { void f() { a::A1 t = a::Red; } }",
+         "namespace b { void f() { B1 t = B1::Red; } }", "a::A1", "b::B1"},
+        {"void f() { a::d::A3 t = a::d::Orange; }",
+         "void f() { a::b::B3 t = a::b::B3::Orange; }", "a::d::A3", "a::b::B3"},
+        {"namespace a { void f() { a::d::A3 t = a::d::Orange; } }",
+         "namespace a { void f() { b::B3 t = b::B3::Orange; } }", "a::d::A3",
+         "a::b::B3"},
+        {"void f() { A5 t = Green; }", "void f() { B5 t = Green; }", "A5",
+         "B5"},
+        // FIXME: the new namespace qualifier should be added to the unscoped
+        // enum constant.
+        {"namespace a { void f() { auto t = Green; } }",
+         "namespace a { void f() { auto t = Green; } }", "a::A1", "b::B1"},
+
+        // namespace qualifiers
+        {"namespace a { void f(A1 a1) {} }",
+         "namespace a { void f(b::B1 a1) {} }", "a::A1", "b::B1"},
+        {"namespace a { void f(A2 a2) {} }",
+         "namespace a { void f(b::B2 a2) {} }", "a::A2", "b::B2"},
+        {"namespace b { void f(a::A1 a1) {} }",
+         "namespace b { void f(B1 a1) {} }", "a::A1", "b::B1"},
+        {"namespace b { void f(a::A2 a2) {} }",
+         "namespace b { void f(B2 a2) {} }", "a::A2", "b::B2"},
+
+        // nested enums
+        {"void f() { a::C::NestedEnum t = a::C::White; }",
+         "void f() { a::C::NewNestedEnum t = a::C::NewNestedEnum::White; }",
+         "a::C::NestedEnum", "a::C::NewNestedEnum"},
+        {"void f() { a::C::NestedScopedEnum t = a::C::NestedScopedEnum::Black; "
+         "}",
+         "void f() { a::C::NewNestedScopedEnum t = "
+         "a::C::NewNestedScopedEnum::Black; }",
+         "a::C::NestedScopedEnum", "a::C::NewNestedScopedEnum"},
+
+        // macros
+        {"void f(MACRO(a::A1) a1) {}", "void f(MACRO(b::B1) a1) {}", "a::A1",
+         "b::B1"},
+        {"void f(MACRO(a::A2) a2) {}", "void f(MACRO(b::B2) a2) {}", "a::A2",
+         "b::B2"},
+        {"#define FOO(T, t) T t\nvoid f() { FOO(a::A1, a1); }",
+         "#define FOO(T, t) T t\nvoid f() { FOO(b::B1, a1); }", "a::A1",
+         "b::B1"},
+        {"#define FOO(T, t) T t\nvoid f() { FOO(a::A2, a2); }",
+         "#define FOO(T, t) T t\nvoid f() { FOO(b::B2, a2); }", "a::A2",
+         "b::B2"},
+        {"#define FOO(n) a::A1 n\nvoid f() { FOO(a1); FOO(a2); }",
+         "#define FOO(n) b::B1 n\nvoid f() { FOO(a1); FOO(a2); }", "a::A1",
+         "b::B1"},
+
+        // using and type alias
+        {"using a::A1; A1 gA;", "using b::B1; b::B1 gA;", "a::A1", "b::B1"},
+        {"using a::A2; A2 gA;", "using b::B2; b::B2 gA;", "a::A2", "b::B2"},
+        {"struct S { using T = a::A1; T a_; };",
+         "struct S { using T = b::B1; T a_; };", "a::A1", "b::B1"},
+        {"using T = a::A1; T gA;", "using T = b::B1; T gA;", "a::A1", "b::B1"},
+        {"using T = a::A2; T gA;", "using T = b::B2; T gA;", "a::A2", "b::B2"},
+        {"typedef a::A1 T; T gA;", "typedef b::B1 T; T gA;", "a::A1", "b::B1"},
+        {"typedef a::A2 T; T gA;", "typedef b::B2 T; T gA;", "a::A2", "b::B2"},
+        {"typedef MACRO(a::A1) T; T gA;", "typedef MACRO(b::B1) T; T gA;",
+         "a::A1", "b::B1"},
+
+        // templates
+        {"template<typename T> struct Foo { T t; }; void f() { Foo<a::A1> "
+         "foo1; }",
+         "template<typename T> struct Foo { T t; }; void f() { Foo<b::B1> "
+         "foo1; }",
+         "a::A1", "b::B1"},
+        {"template<typename T> struct Foo { T t; }; void f() { Foo<a::A2> "
+         "foo2; }",
+         "template<typename T> struct Foo { T t; }; void f() { Foo<b::B2> "
+         "foo2; }",
+         "a::A2", "b::B2"},
+        {"template<typename T> struct Foo { a::A1 a1; };",
+         "template<typename T> struct Foo { b::B1 a1; };", "a::A1", "b::B1"},
+        {"template<typename T> struct Foo { a::A2 a2; };",
+         "template<typename T> struct Foo { b::B2 a2; };", "a::A2", "b::B2"},
+        {"template<typename T> int f() { return 1; } template<> int f<a::A1>() "
+         "{ return 2; } int g() { return f<a::A1>(); }",
+         "template<typename T> int f() { return 1; } template<> int f<b::B1>() "
+         "{ return 2; } int g() { return f<b::B1>(); }",
+         "a::A1", "b::B1"},
+        {"template<typename T> int f() { return 1; } template<> int f<a::A2>() "
+         "{ return 2; } int g() { return f<a::A2>(); }",
+         "template<typename T> int f() { return 1; } template<> int f<b::B2>() "
+         "{ return 2; } int g() { return f<b::B2>(); }",
+         "a::A2", "b::B2"},
+        {"struct Foo { template <typename T> T foo(); }; void g() { Foo f;  "
+         "f.foo<a::A1>(); }",
+         "struct Foo { template <typename T> T foo(); }; void g() { Foo f;  "
+         "f.foo<b::B1>(); }",
+         "a::A1", "b::B1"},
+        {"struct Foo { template <typename T> T foo(); }; void g() { Foo f;  "
+         "f.foo<a::A2>(); }",
+         "struct Foo { template <typename T> T foo(); }; void g() { Foo f;  "
+         "f.foo<b::B2>(); }",
+         "a::A2", "b::B2"},
+    })), );
+
+TEST_P(RenameEnumTest, RenameEnums) {
+  auto Param = GetParam();
+  assert(!Param.OldName.empty());
+  assert(!Param.NewName.empty());
+  std::string Actual =
+      runClangRenameOnCode(Param.Before, Param.OldName, Param.NewName);
+  CompareSnippets(Param.After, Actual);
+}
+
+TEST_F(RenameEnumTest, RenameEnumDecl) {
+  std::string Before = R"(
+      namespace ns {
+      enum Old1 { Blue };
+      }
+  )";
+  std::string Expected = R"(
+      namespace ns {
+      enum New1 { Blue };
+      }
+  )";
+  std::string After = runClangRenameOnCode(Before, "ns::Old1", "ns::New1");
+  CompareSnippets(Expected, After);
+}
+
+TEST_F(RenameEnumTest, RenameScopedEnumDecl) {
+  std::string Before = R"(
+      namespace ns {
+      enum class Old1 { Blue };
+      }
+  )";
+  std::string Expected = R"(
+      namespace ns {
+      enum class New1 { Blue };
+      }
+  )";
+  std::string After = runClangRenameOnCode(Before, "ns::Old1", "ns::New1");
+  CompareSnippets(Expected, After);
+}
+
+} // anonymous namespace
+} // namespace test
+} // namespace clang_rename
+} // namesdpace clang