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

[ioeric]

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.

[ioeric]

• Implemented a workaround.

[ioeric]

Test comment to check if cfe-commits is in the email cc list

[hokein]

change-namespace/ChangeNamespace.cpp

343 ↗

(On Diff #77973)

Maybe we can filter it out in run(MatcherResult) function rather than in AST matcher? If we meet a specifier like Base::Base() in CXXCtorInitializer, we just skip them.

[ioeric]

• Don't fix base class initializers at all.

[ioeric]

• Minor fix

[hokein]

change-namespace/ChangeNamespace.cpp

346 ↗

(On Diff #78148)

Maybe pull this unless matcher out and name it to make code more understandable.

650 ↗

(On Diff #78148)

use std::find?

test/change-namespace/lambda-function.cpp

2 ↗

(On Diff #78148)

In practise, we don't use std headers directly in llvm lit test. You need to mock it by yourself...

unittests/change-namespace/ChangeNamespaceTests.cpp

47 ↗

(On Diff #78148)

nit: code indentation.

[bkramer]

change-namespace/ChangeNamespace.cpp

546 ↗

(On Diff #78180)

So is this an error or not? If you can hit this by using the tool it should bail out here. If not use llvm_unreachable instead of assert(false).

637 ↗

(On Diff #78180)

same here.

650 ↗

(On Diff #78180)

llvm::is_contained

707 ↗

(On Diff #78180)

same here.

test/change-namespace/lambda-function.cpp

2 ↗

(On Diff #78180)

You cannot rely on an STL implementation being around in a test. Unless we have a fake <functional> lying around this will fail in some environments. (I know this has been around for a while, just bringing it up now)

[ioeric]

• Addressed comments.

[hokein]

test/change-namespace/lambda-function.cpp

2 ↗

(On Diff #78148)

Hint: here is a simple fake function code, should be compiled.

template<class T> class function;
template<class R, class... ArgTypes> class function<R(ArgTypes...)> {
 public:
  template <typename Functor>
  function(Functor f) {}
};

void B(function<void(int)> t) {};

void g() {
  B([](int x){});
}

[ioeric]

• Add lambda-function.cpp test back.

[hokein]

LGTM once nits get fixed.

change-namespace/ChangeNamespace.cpp

543 ↗

(On Diff #78188)

No {}.

346 ↗

(On Diff #78148)

Looks like you are missing this one.

[ioeric]

• nits

clang-tools-extra/trunk/change-namespace/ChangeNamespace.h

private:
   // inserting it to the new namespace.
   std::map<std::string, std::vector<InsertForwardDeclaration>> InsertFwdDecls;
   // Records all using declarations, which can be used to shorten namespace
   // specifiers.
   llvm::SmallPtrSet<const UsingDecl *, 8> UsingDecls;
   // 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
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CHANGE_NAMESPACE_CHANGENAMESPACE_H

clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp

 //===-- ChangeNamespace.cpp - Change namespace implementation -------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 #include "ChangeNamespace.h"
 #include "clang/Format/Format.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/Support/ErrorHandling.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace change_namespace {
 
 namespace {
 
Finder->addMatcher(
       this);
 
   // Types in `UsingShadowDecl` is not matched by `typeLoc` above, so we need to
   // special case it.
   Finder->addMatcher(usingDecl(IsInMovedNs, hasAnyUsingShadowDecl(decl()))
                          .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
   // defined in the global namespace.
   // Note that the matcher does not exclude calls to out-of-line static method
   // definitions, so we need to exclude them in the callback handler.
   auto FuncMatcher =
       functionDecl(unless(anyOf(cxxMethodDecl(), IsInMovedNs,
moveClassForwardDeclaration(Result, FwdDecl);
                  Result.Nodes.getNodeAs<UsingDecl>("using_with_shadow")) {
     fixUsingShadowDecl(Result, UsingWithShadow);
   } else if (const auto *Specifier =
                  Result.Nodes.getNodeAs<NestedNameSpecifierLoc>(
                      "nested_specifier_loc")) {
     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);
   } else if (const auto *VarRef =
                  Result.Nodes.getNodeAs<DeclRefExpr>("var_ref")) {
     const auto *Var = Result.Nodes.getNodeAs<VarDecl>("var_decl");
     assert(Var);
     if (Var->getCanonicalDecl()->isStaticDataMember())
void ChangeNamespaceTool::moveClassForwardDeclaration(
   SourceLocation AfterSemi = Lexer::findLocationAfterToken(
       End, tok::semi, *Result.SourceManager, Result.Context->getLangOpts(),
       /*SkipTrailingWhitespaceAndNewLine=*/true);
   if (AfterSemi.isValid())
     End = AfterSemi.getLocWithOffset(-1);
   // 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),
           Result.SourceManager->getSpellingLoc(End)),
       *Result.SourceManager, Result.Context->getLangOpts());
   // Insert the forward declaration back into the old namespace after moving the
   // code from old namespace to new namespace.
   // Insertion information is stored in `InsertFwdDecls` and actual
if (isDeclVisibleAtLocation(*Result.SourceManager, Using, DeclCtx, Start)) {
       }
     }
   }
   // If the new nested name in the new namespace is the same as it was in the
   // old namespace, we don't create replacement.
   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
 // `Type` is in `NewNamespace`.
 void ChangeNamespaceTool::fixTypeLoc(
     const ast_matchers::MatchFinder::MatchResult &Result, SourceLocation Start,
     SourceLocation End, TypeLoc Type) {
   // 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
   // a typedef type, we need to use the typedef type instead.
   if (auto *Typedef = Type.getType()->getAs<TypedefType>()) {
     FromDecl = Typedef->getDecl();
     auto IsInMovedNs = [&](const NamedDecl *D) {
       if (!llvm::StringRef(D->getQualifiedNameAsString())
void ChangeNamespaceTool::fixUsingShadowDecl(
   const NamedDecl *TargetDecl =
       UsingDeclaration->shadow_begin()->getTargetDecl();
   std::string TargetDeclName = TargetDecl->getQualifiedNameAsString();
   // FIXME: check if target_decl_name is in moved ns, which doesn't make much
   // sense. If this happens, we need to use name with the new namespace.
   // 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() {
   // Move namespace blocks and insert forward declaration to old namespace.
   for (const auto &FileAndNsMoves : MoveNamespaces) {
     auto &NsMoves = FileAndNsMoves.second;
     if (NsMoves.empty())
       continue;

clang-tools-extra/trunk/test/change-namespace/lambda-function.cpp

+// 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
+}
+}

clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp

std::string runChangeNamespaceOnCode(llvm::StringRef Code) {
 
     std::map<std::string, tooling::Replacements> FileToReplacements;
     change_namespace::ChangeNamespaceTool NamespaceTool(
         OldNamespace, NewNamespace, FilePattern, &FileToReplacements);
     ast_matchers::MatchFinder Finder;
     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));
   }
 
   std::string format(llvm::StringRef Code) {
     tooling::Replacements Replaces = format::reformat(
         format::getLLVMStyle(), Code, {tooling::Range(0, Code.size())});
     auto ChangedCode = tooling::applyAllReplacements(Code, Replaces);
std::string Expected = "namespace na {\n"
                          "void f(na::C_A ca, na::nc::C_C cc) {\n"
                          "  na::C_A ca_1 = ca;\n"
                          "}\n"
                          "} // namespace y\n"
                          "} // namespace x\n";
   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"
                      "namespace na {\n"
                      "namespace nc {\n"
                      "class SAME {};\n"
                      "}\n"
                      "namespace nd {\n"

 }
 
 TEST_F(ChangeNamespaceTest, DoNotFixStaticVariableOfClass) {
   std::string Code = "namespace na {\n"
                      "class A {\n"
                      "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"
                      "} // namespace na\n";
 
   std::string Expected = "namespace na {\n"
                          "class A {\n"
                          "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"
                          "namespace y {\n"
                          "void f() { int a = na::A::A1; int b = na::A::A2; }"
                          "}  // namespace y\n"
                          "}  // namespace x\n";
 
std::string Expected = "namespace nx { namespace ny { class X {}; } }\n"
                          "namespace y {\n"
                          "typedef nx::ny::X Y;\n"
                          "void f() { Y y; }\n"
                          "} // namespace y\n"
                          "} // namespace x\n";
   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