[change-namespace] fix name qualifiers in UsingShadowDecl and NestedNameSpecifier.

[hokein]

Looks almost good. A few comments.

change-namespace/ChangeNamespace.cpp

467 ↗

(On Diff #71983)

It is not always accurate to use the first using-decls. It's fine now since don't have better solution for this. Add a FIXME.

unittests/change-namespace/ChangeNamespaceTests.cpp

243 ↗

(On Diff #71983)

How about the case void f2(SAME Same){}? Is it changed to ::na::nc::SAME too? Make sense to add it to the test.

BTW, does the patch cover using alias ("using A = x::y::z;") case? If yes, also add to the test.

[ioeric]

• Addressed review comments.

[hokein]

LGTM.

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

private:
   void replaceQualifiedSymbolInDeclContext(
       const ast_matchers::MatchFinder::MatchResult &Result,
       const Decl *DeclContext, SourceLocation Start, SourceLocation End,
       llvm::StringRef DeclName);
 
   void fixTypeLoc(const ast_matchers::MatchFinder::MatchResult &Result,
                   SourceLocation Start, SourceLocation End, TypeLoc Type);
 
+  void fixUsingShadowDecl(const ast_matchers::MatchFinder::MatchResult &Result,
+                          const UsingDecl *UsingDeclaration);
+
   // Information about moving an old namespace.
   struct MoveNamespace {
     // The start offset of the namespace block being moved in the original
     // code.
     unsigned Offset;
     // The length of the namespace block in the original code.
     unsigned Length;
     // The offset at which the new namespace block will be inserted in the

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

while (!OldNsSplitted.empty() && !NewNsSplitted.empty() &&
     OldNsSplitted.erase(OldNsSplitted.begin());
     NewNsSplitted.erase(NewNsSplitted.begin());
   }
   DiffOldNamespace = joinNamespaces(OldNsSplitted);
   DiffNewNamespace = joinNamespaces(NewNsSplitted);
 }
 
 // FIXME: handle the following symbols:
-//   - Types in `UsingShadowDecl` (e.g. `using a::b::c;`) which are not matched
-//   by `typeLoc`.
-//   - Types in nested name specifier, e.g. "na::X" in "na::X::Nested".
 //   - Function references.
 //   - Variable references.
 void ChangeNamespaceTool::registerMatchers(ast_matchers::MatchFinder *Finder) {
   // Match old namespace blocks.
   std::string FullOldNs = "::" + OldNamespace;
   Finder->addMatcher(
       namespaceDecl(hasName(FullOldNs), isExpansionInFileMatching(FilePattern))
           .bind("old_ns"),
auto DeclMatcher = namedDecl(
       hasAncestor(namespaceDecl()),
       unless(anyOf(
           hasAncestor(namespaceDecl(isAnonymous())),
           hasAncestor(cxxRecordDecl()),
           allOf(IsInMovedNs, unless(cxxRecordDecl(unless(isDefinition())))))));
   // Match TypeLocs on the declaration. Carefully match only the outermost
   // TypeLoc that's directly linked to the old class and don't handle nested
   // name specifier locs.
-  // FIXME: match and handle nested name specifier locs.
   Finder->addMatcher(
       typeLoc(IsInMovedNs,
               loc(qualType(hasDeclaration(DeclMatcher.bind("from_decl")))),
               unless(anyOf(hasParent(typeLoc(
                                loc(qualType(hasDeclaration(DeclMatcher))))),
                            hasParent(nestedNameSpecifierLoc()))),
               hasAncestor(decl().bind("dc")))
           .bind("type"),
       this);
+  // Types in `UsingShadowDecl` is not matched by `typeLoc` above, so we need to
+  // special case it.
+  Finder->addMatcher(
+      usingDecl(hasAnyUsingShadowDecl(IsInMovedNs)).bind("using_decl"), this);
+  // Handle types in nested name specifier.
+  Finder->addMatcher(nestedNameSpecifierLoc(
+                         hasAncestor(decl(IsInMovedNs).bind("dc")),
+                         loc(nestedNameSpecifier(specifiesType(
+                             hasDeclaration(DeclMatcher.bind("from_decl"))))))
+                         .bind("nested_specifier_loc"),
+                     this);
 }
 
 void ChangeNamespaceTool::run(
     const ast_matchers::MatchFinder::MatchResult &Result) {
   if (const auto *NsDecl = Result.Nodes.getNodeAs<NamespaceDecl>("old_ns")) {
     moveOldNamespace(Result, NsDecl);
   } else if (const auto *FwdDecl =
                  Result.Nodes.getNodeAs<CXXRecordDecl>("fwd_decl")) {
     moveClassForwardDeclaration(Result, FwdDecl);
+  } else if (const auto *UsingDeclaration =
+                 Result.Nodes.getNodeAs<UsingDecl>("using_decl")) {
+    fixUsingShadowDecl(Result, UsingDeclaration);
+  } 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 {
     const auto *TLoc = Result.Nodes.getNodeAs<TypeLoc>("type");
     assert(TLoc != nullptr && "Expecting callback for TypeLoc");
     fixTypeLoc(Result, startLocationForType(*TLoc), EndLocationForType(*TLoc),
                *TLoc);
   }
 }
 
if (auto *Typedef = Type.getType()->getAs<TypedefType>())
     FromDecl = Typedef->getDecl();
 
   const Decl *DeclCtx = Result.Nodes.getNodeAs<Decl>("dc");
   assert(DeclCtx && "Empty decl context.");
   replaceQualifiedSymbolInDeclContext(Result, DeclCtx, Start, End,
                                       FromDecl->getQualifiedNameAsString());
 }
 
+void ChangeNamespaceTool::fixUsingShadowDecl(
+    const ast_matchers::MatchFinder::MatchResult &Result,
+    const UsingDecl *UsingDeclaration) {
+  SourceLocation Start = UsingDeclaration->getLocStart();
+  SourceLocation End = UsingDeclaration->getLocEnd();
+  if (Start.isInvalid() || End.isInvalid()) return;
+
+  assert(UsingDeclaration->shadow_size() > 0);
+  // FIXME: it might not be always accurate to use the first using-decl.
+  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()]);
+}
+
 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;
     const std::string &FilePath = FileAndNsMoves.first;
     auto &Replaces = FileToReplacements[FilePath];

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

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, FixUsingShadowDecl) {
+  std::string Code = "namespace na {\n"
+                     "namespace nc {\n"
+                     "class SAME {};\n"
+                     "}\n"
+                     "namespace nd {\n"
+                     "class SAME {};\n"
+                     "}\n"
+                     "namespace nb {\n"
+                     "using nc::SAME;\n"
+                     "using YO = nc::SAME;\n"
+                     "typedef nc::SAME IDENTICAL;\n"
+                     "void f(nd::SAME Same) {}\n"
+                     "} // namespace nb\n"
+                     "} // namespace na\n";
+
+  std::string Expected = "namespace na {\n"
+                         "namespace nc {\n"
+                         "class SAME {};\n"
+                         "}\n"
+                         "namespace nd {\n"
+                         "class SAME {};\n"
+                         "}\n"
+                         "\n"
+                         "} // namespace na\n"
+                         "namespace x {\n"
+                         "namespace y {\n"
+                         "using ::na::nc::SAME;\n"
+                         "using YO = na::nc::SAME;\n"
+                         "typedef na::nc::SAME IDENTICAL;\n"
+                         "void f(na::nd::SAME Same) {}\n"
+                         "} // namespace y\n"
+                         "} // namespace x\n";
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, TypeInNestedNameSpecifier) {
+  std::string Code =
+      "namespace na {\n"
+      "class C_A {\n"
+      "public:\n"
+      "  class Nested {\n"
+      "    public:\n"
+      "      static int NestedX;\n"
+      "      static void nestedFunc() {}\n"
+      "  };\n"
+      "};\n"
+      "namespace nb {\n"
+      "class C_X {\n"
+      "  C_A na;\n"
+      "  C_A::Nested nested;\n"
+      "  void f() {\n"
+      "    C_A::Nested::nestedFunc();\n"
+      "    int X = C_A::Nested::NestedX;\n"
+      "  }\n"
+      "};\n"
+      "}  // namespace nb\n"
+      "}  // namespace na\n";
+  std::string Expected =
+      "namespace na {\n"
+      "class C_A {\n"
+      "public:\n"
+      "  class Nested {\n"
+      "    public:\n"
+      "      static int NestedX;\n"
+      "      static void nestedFunc() {}\n"
+      "  };\n"
+      "};\n"
+      "\n"
+      "}  // namespace na\n"
+      "namespace x {\n"
+      "namespace y {\n"
+      "class C_X {\n"
+      "  na::C_A na;\n"
+      "  na::C_A::Nested nested;\n"
+      "  void f() {\n"
+      "    na::C_A::Nested::nestedFunc();\n"
+      "    int X = na::C_A::Nested::NestedX;\n"
+      "  }\n"
+      "};\n"
+      "}  // namespace y\n"
+      "}  // namespace x\n";
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
 } // anonymous namespace
 } // namespace change_namespace
 } // namespace clang