[change-namespace] handle constructor initializer: Derived : Base::Base() {} and added conflict detections

Summary:
namespace nx { namespace ny { class Base { public: Base(i) {}} } }
namespace na {
namespace nb {
class X : public nx::ny {
public:
  X() : Base::Base(1) {}
};
}
}

When changing from na::nb to x::y, "Base::Base" will be changed to "nx::ny::Base" and
 "Base::" in "Base::Base" will be replaced with "nx::ny::Base" too, which causes
conflict. This conflict should've been detected when adding replacements but was hidden by `addOrMergeReplacement`. We now also detect conflict when adding replacements where conflict must not happen.

The namespace lookup is tricky here, we simply replace "Base::Base()" with "nx::ny::Base()" as a workaround, which compiles but not perfect.

Reviewers: hokein

Subscribers: bkramer, cfe-commits

Differential Revision: https://reviews.llvm.org/D26637

llvm-svn: 287118

4 files changed, 152 insertions, 24 deletions

diff --git a/clang-tools-extra/change-namespace/ChangeNamespace.cpp b/clang-tools-extra/change-namespace/ChangeNamespace.cpp
index af29c6d2947..fbbde4397cc 100644
--- a/clang-tools-extra/change-namespace/ChangeNamespace.cpp
+++ b/clang-tools-extra/change-namespace/ChangeNamespace.cpp
@@ -9,6 +9,7 @@
 #include "ChangeNamespace.h"
 #include "clang/Format/Format.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/Support/ErrorHandling.h"
 
 using namespace clang::ast_matchers;
 
@@ -336,14 +337,27 @@ void ChangeNamespaceTool::registerMatchers(ast_matchers::MatchFinder *Finder) {
                          .bind("using_with_shadow"),
                      this);
 
-  // Handle types in nested name specifier.
+  // Handle types in nested name specifier. Specifiers that are in a TypeLoc
+  // matched above are not matched, e.g. "A::" in "A::A" is not matched since
+  // "A::A" would have already been fixed.
   Finder->addMatcher(nestedNameSpecifierLoc(
                          hasAncestor(decl(IsInMovedNs).bind("dc")),
                          loc(nestedNameSpecifier(specifiesType(
-                             hasDeclaration(DeclMatcher.bind("from_decl"))))))
+                             hasDeclaration(DeclMatcher.bind("from_decl"))))),
+                         unless(hasAncestor(typeLoc(loc(qualType(hasDeclaration(
+                             decl(equalsBoundNode("from_decl")))))))))
                          .bind("nested_specifier_loc"),
                      this);
 
+  // Matches base class initializers in constructors. TypeLocs of base class
+  // initializers do not need to be fixed. For example,
+  //    class X : public a::b::Y {
+  //      public:
+  //        X() : Y::Y() {} // Y::Y do not need namespace specifier.
+  //    };
+  Finder->addMatcher(
+      cxxCtorInitializer(isBaseInitializer()).bind("base_initializer"), this);
+
   // Handle function.
   // Only handle functions that are defined in a namespace excluding member
   // function, static methods (qualified by nested specifier), and functions
@@ -393,6 +407,11 @@ void ChangeNamespaceTool::run(
     SourceLocation Start = Specifier->getBeginLoc();
     SourceLocation End = EndLocationForType(Specifier->getTypeLoc());
     fixTypeLoc(Result, Start, End, Specifier->getTypeLoc());
+  } else if (const auto *BaseInitializer =
+                 Result.Nodes.getNodeAs<CXXCtorInitializer>(
+                     "base_initializer")) {
+    BaseCtorInitializerTypeLocs.push_back(
+        BaseInitializer->getTypeSourceInfo()->getTypeLoc());
   } else if (const auto *TLoc = Result.Nodes.getNodeAs<TypeLoc>("type")) {
     fixTypeLoc(Result, startLocationForType(*TLoc), EndLocationForType(*TLoc),
                *TLoc);
@@ -520,7 +539,9 @@ void ChangeNamespaceTool::moveClassForwardDeclaration(
   // Delete the forward declaration from the code to be moved.
   const auto Deletion =
       createReplacement(Start, End, "", *Result.SourceManager);
-  addOrMergeReplacement(Deletion, &FileToReplacements[Deletion.getFilePath()]);
+  auto Err = FileToReplacements[Deletion.getFilePath()].add(Deletion);
+  if (Err)
+    llvm_unreachable(llvm::toString(std::move(Err)).c_str());
   llvm::StringRef Code = Lexer::getSourceText(
       CharSourceRange::getTokenRange(
           Result.SourceManager->getSpellingLoc(Start),
@@ -606,7 +627,9 @@ void ChangeNamespaceTool::replaceQualifiedSymbolInDeclContext(
   if (NestedName == ReplaceName)
     return;
   auto R = createReplacement(Start, End, ReplaceName, *Result.SourceManager);
-  addOrMergeReplacement(R, &FileToReplacements[R.getFilePath()]);
+  auto Err = FileToReplacements[R.getFilePath()].add(R);
+  if (Err)
+    llvm_unreachable(llvm::toString(std::move(Err)).c_str());
 }
 
 // Replace the [Start, End] of `Type` with the shortest qualified name when the
@@ -617,6 +640,9 @@ void ChangeNamespaceTool::fixTypeLoc(
   // FIXME: do not rename template parameter.
   if (Start.isInvalid() || End.isInvalid())
     return;
+  // Types of CXXCtorInitializers do not need to be fixed.
+  if (llvm::is_contained(BaseCtorInitializerTypeLocs, Type))
+    return;
   // The declaration which this TypeLoc refers to.
   const auto *FromDecl = Result.Nodes.getNodeAs<NamedDecl>("from_decl");
   // `hasDeclaration` gives underlying declaration, but if the type is
@@ -667,7 +693,9 @@ void ChangeNamespaceTool::fixUsingShadowDecl(
   // Use fully qualified name in UsingDecl for now.
   auto R = createReplacement(Start, End, "using ::" + TargetDeclName,
                              *Result.SourceManager);
-  addOrMergeReplacement(R, &FileToReplacements[R.getFilePath()]);
+  auto Err = FileToReplacements[R.getFilePath()].add(R);
+  if (Err)
+    llvm_unreachable(llvm::toString(std::move(Err)).c_str());
 }
 
 void ChangeNamespaceTool::onEndOfTranslationUnit() {
diff --git a/clang-tools-extra/change-namespace/ChangeNamespace.h b/clang-tools-extra/change-namespace/ChangeNamespace.h
index b8be9acc31f..4073f6fe39c 100644
--- a/clang-tools-extra/change-namespace/ChangeNamespace.h
+++ b/clang-tools-extra/change-namespace/ChangeNamespace.h
@@ -145,6 +145,9 @@ private:
   // Records all using namespace declarations, which can be used to shorten
   // namespace specifiers.
   llvm::SmallPtrSet<const UsingDirectiveDecl *, 8> UsingNamespaceDecls;
+  // TypeLocs of CXXCtorInitializer. Types of CXXCtorInitializers do not need to
+  // be fixed.
+  llvm::SmallVector<TypeLoc, 8> BaseCtorInitializerTypeLocs;
 };
 
 } // namespace change_namespace
diff --git a/clang-tools-extra/test/change-namespace/lambda-function.cpp b/clang-tools-extra/test/change-namespace/lambda-function.cpp
new file mode 100644
index 00000000000..484989ef41a
--- /dev/null
+++ b/clang-tools-extra/test/change-namespace/lambda-function.cpp
@@ -0,0 +1,22 @@
+// RUN: clang-change-namespace -old_namespace "na::nb" -new_namespace "x::y" --file_pattern ".*" %s -- -std=c++11 | sed 's,// CHECK.*,,' | FileCheck %s
+
+template <class T>
+class function;
+template <class R, class... ArgTypes>
+class function<R(ArgTypes...)> {
+public:
+  template <typename Functor>
+  function(Functor f) {}
+  R operator()(ArgTypes...) const {}
+};
+
+// CHECK: namespace x {
+// CHECK-NEXT: namespace y {
+namespace na {
+namespace nb {
+void f(function<void(int)> func, int param) { func(param); }
+void g() { f([](int x) {}, 1); }
+// CHECK: } // namespace y
+// CHECK-NEXT: } // namespace x
+}
+}
diff --git a/clang-tools-extra/unittests/change-namespace/ChangeNamespaceTests.cpp b/clang-tools-extra/unittests/change-namespace/ChangeNamespaceTests.cpp
index 87698ff7704..17d4500bb69 100644
--- a/clang-tools-extra/unittests/change-namespace/ChangeNamespaceTests.cpp
+++ b/clang-tools-extra/unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -43,8 +43,9 @@ public:
     NamespaceTool.registerMatchers(&Finder);
     std::unique_ptr<tooling::FrontendActionFactory> Factory =
         tooling::newFrontendActionFactory(&Finder);
-    tooling::runToolOnCodeWithArgs(Factory->create(), Code, {"-std=c++11"},
-                                   FileName);
+    if (!tooling::runToolOnCodeWithArgs(Factory->create(), Code, {"-std=c++11"},
+                                        FileName))
+      return "";
     formatAndApplyAllReplacements(FileToReplacements, Context.Rewrite);
     return format(Context.getRewrittenText(ID));
   }
@@ -330,19 +331,6 @@ TEST_F(ChangeNamespaceTest, MoveFunctions) {
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
 }
 
-TEST_F(ChangeNamespaceTest, DoNotCrashWithLambdaAsParameter) {
-  std::string Code =
-      "#include <functional>\n"
-      "void f(std::function<void(int)> func, int param) { func(param); } "
-      "void g() { f([](int x) {}, 1); }";
-
-  std::string Expected =
-      "#include <functional>\n"
-      "void f(std::function<void(int)> func, int param) { func(param); } "
-      "void g() { f([](int x) {}, 1); }";
-  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
-}
-
 TEST_F(ChangeNamespaceTest, FixUsingShadowDecl) {
   std::string Code = "class GLOB {};\n"
                      "using BLOG = GLOB;\n"
@@ -510,8 +498,8 @@ TEST_F(ChangeNamespaceTest, DoNotFixStaticVariableOfClass) {
                      "public:\n"
                      "static int A1;\n"
                      "static int A2;\n"
-                     "}\n"
-                     "static int A::A1 = 0;\n"
+                     "};\n"
+                     "int A::A1 = 0;\n"
                      "namespace nb {\n"
                      "void f() { int a = A::A1; int b = A::A2; }"
                      "} // namespace nb\n"
@@ -522,8 +510,8 @@ TEST_F(ChangeNamespaceTest, DoNotFixStaticVariableOfClass) {
                          "public:\n"
                          "static int A1;\n"
                          "static int A2;\n"
-                         "}\n"
-                         "static int A::A1 = 0;\n"
+                         "};\n"
+                         "int A::A1 = 0;\n"
                          "\n"
                          "} // namespace na\n"
                          "namespace x {\n"
@@ -1005,6 +993,93 @@ TEST_F(ChangeNamespaceTest, TypedefAliasDecl) {
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
 }
 
+TEST_F(ChangeNamespaceTest, DerivedClassWithConstructors) {
+  std::string Code =
+      "namespace nx { namespace ny { class X { public: X(int i) {} }; } }\n"
+      "namespace na {\n"
+      "namespace nb {\n"
+      "class A : public nx::ny::X {\n"
+      "public:\n"
+      "  A() : X(0) {}\n"
+      "  A(int i);\n"
+      "};\n"
+      "A::A(int i) : X(i) {}\n"
+      "} // namespace nb\n"
+      "} // namespace na\n";
+  std::string Expected =
+      "namespace nx { namespace ny { class X { public: X(int i) {} }; } }\n"
+      "\n\n"
+      "namespace x {\n"
+      "namespace y {\n"
+      "class A : public nx::ny::X {\n"
+      "public:\n"
+      "  A() : X(0) {}\n"
+      "  A(int i);\n"
+      "};\n"
+      "A::A(int i) : X(i) {}\n"
+      "} // namespace y\n"
+      "} // namespace x\n";
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, DerivedClassWithQualifiedConstructors) {
+  std::string Code =
+      "namespace nx { namespace ny { class X { public: X(int i) {} }; } }\n"
+      "namespace na {\n"
+      "namespace nb {\n"
+      "class A : public nx::ny::X {\n"
+      "public:\n"
+      "  A() : X::X(0) {}\n"
+      "  A(int i);\n"
+      "};\n"
+      "A::A(int i) : X::X(i) {}\n"
+      "} // namespace nb\n"
+      "} // namespace na\n";
+  std::string Expected =
+      "namespace nx { namespace ny { class X { public: X(int i) {} }; } }\n"
+      "\n\n"
+      "namespace x {\n"
+      "namespace y {\n"
+      "class A : public nx::ny::X {\n"
+      "public:\n"
+      "  A() : X::X(0) {}\n"
+      "  A(int i);\n"
+      "};\n"
+      "A::A(int i) : X::X(i) {}\n"
+      "} // namespace y\n"
+      "} // namespace x\n";
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, DerivedClassWithConstructorsAndTypeRefs) {
+  std::string Code =
+      "namespace nx { namespace ny { class X { public: X(int i) {} }; } }\n"
+      "namespace na {\n"
+      "namespace nb {\n"
+      "class A : public nx::ny::X {\n"
+      "public:\n"
+      "  A() : X(0) {}\n"
+      "  A(int i);\n"
+      "};\n"
+      "A::A(int i) : X(i) { X x(1);}\n"
+      "} // namespace nb\n"
+      "} // namespace na\n";
+  std::string Expected =
+      "namespace nx { namespace ny { class X { public: X(int i) {} }; } }\n"
+      "\n\n"
+      "namespace x {\n"
+      "namespace y {\n"
+      "class A : public nx::ny::X {\n"
+      "public:\n"
+      "  A() : X(0) {}\n"
+      "  A(int i);\n"
+      "};\n"
+      "A::A(int i) : X(i) { nx::ny::X x(1);}\n"
+      "} // namespace y\n"
+      "} // namespace x\n";
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
 } // anonymous namespace
 } // namespace change_namespace
 } // namespace clang