Workaround ASTMatcher crashes. Added some more test cases.

[ioeric]

Summary

• UsingDecl matcher crashed when UsingShadowDecl has no parent map. Workaround by moving parent check into UsingDecl.
• FunctionDecl matcher crashed when there is a lambda defined in parameter list (also due to no parent map). Workaround by putting unless(cxxMethodDecl()) before parent check.

[ioeric]

• Update comments

[aaron.ballman]

Should this perhaps be fixed in the AST matchers rather than in the check itself?

[ioeric]

In D24862#550615, @aaron.ballman wrote:

Should this perhaps be fixed in the AST matchers rather than in the check itself?

I'll file bugs for the crashes, but the fix in this patch is not that "dirty" - it simply changes the order of matchers to workaround the crashes. So I guess this doesn't need to wait for the fix in matcher library.

[aaron.ballman]

In D24862#550628, @ioeric wrote:

In D24862#550615, @aaron.ballman wrote:

Should this perhaps be fixed in the AST matchers rather than in the check itself?

I'll file bugs for the crashes, but the fix in this patch is not that "dirty" - it simply changes the order of matchers to workaround the crashes. So I guess this doesn't need to wait for the fix in matcher library.

Whenever possible, we should be fixing the underlying bugs, not working around them.

[ioeric]

• Update comments.

[ioeric]

In D24862#550637, @aaron.ballman wrote:

In D24862#550628, @ioeric wrote:

In D24862#550615, @aaron.ballman wrote:

Should this perhaps be fixed in the AST matchers rather than in the check itself?

I'll file bugs for the crashes, but the fix in this patch is not that "dirty" - it simply changes the order of matchers to workaround the crashes. So I guess this doesn't need to wait for the fix in matcher library.

Whenever possible, we should be fixing the underlying bugs, not working around them.

Acked, and I totally agree with you :) It's just that the change in this patch would still be valid after the underlying bugs are fixed, so I thought it was fine to fix those bugs after this.

[aaron.ballman]

In D24862#550646, @ioeric wrote:

Acked, and I totally agree with you :) It's just that the change in this patch would still be valid after the underlying bugs are fixed, so I thought it was fine to fix those bugs after this.

I might be confused then (this happens from time to time) -- are the changes in this patch still necessary after the underlying bugs are fixed?

[ioeric]

• Update comment.

[ioeric]

In D24862#550654, @aaron.ballman wrote:

In D24862#550646, @ioeric wrote:

Acked, and I totally agree with you :) It's just that the change in this patch would still be valid after the underlying bugs are fixed, so I thought it was fine to fix those bugs after this.

I might be confused then (this happens from time to time) -- are the changes in this patch still necessary after the underlying bugs are fixed?

Sorry for the confusion. I guess I should've been clearer in the comments and patch summary... The changes would've been what we wanted even without the underlying bugs and would not be reverted after the bugs are fixed.

[aaron.ballman]

In D24862#550665, @ioeric wrote:

Sorry for the confusion. I guess I should've been clearer in the comments and patch summary... The changes would've been what we wanted even without the underlying bugs and would not be reverted after the bugs are fixed.

Ah, thank you for clarifying! Now it makes more sense to me. :-)

Actual review comments are inline.

change-namespace/ChangeNamespace.cpp

279–281

Why is this change an improvement even after the underlying bug is fixed?

296–300

Comment is fine, but it should be reworded once the underlying bug is fixed, which makes me wonder if the comment is really adding value. Might make more sense to not talk in terms of the crash, but just why you need to filter this way.

change-namespace/tool/ClangChangeNamespace.cpp

73 ↗

(On Diff #72275)

I don't think these changes are necessary as part of this patch.

unittests/change-namespace/ChangeNamespaceTests.cpp

325

May want to duplicate this FIXME to where we are generating the change?

350

Same here as above.

[ioeric]

• Addressed review comments.

[ioeric]

Thanks for the comments!

change-namespace/ChangeNamespace.cpp

279–281

I think IsInMovedNs makes more sense to be a filter for using decl, and it is also more restrictive than hasAnyUsingShadowDecl, which would make the failure matching stop earlier IMO.

296–300

Fair enough. I think I'll just get rid of the comment regarding crash and let the unittest handle it.

change-namespace/tool/ClangChangeNamespace.cpp

73 ↗

(On Diff #72275)

Removed.

[aaron.ballman]

LGTM, thank you!

change-namespace/ChangeNamespace.cpp

279–281

I had originally thought that this was changing behavior, but I am starting to think this is actually fine.

[hokein]

LGTM.

change-namespace/ChangeNamespace.cpp

Finder->addMatcher(
       this);
 
   // Match references to types that are not defined in the old namespace.
   // Forward-declarations in the old namespace are also matched since they will
   // be moved back to the old namespace.
   auto DeclMatcher = namedDecl(
       hasAncestor(namespaceDecl()),
       unless(anyOf(
-          hasAncestor(namespaceDecl(isAnonymous())),
+          isImplicit(), 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.
   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);
+      usingDecl(IsInMovedNs, hasAnyUsingShadowDecl(decl())).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);
 
   // Handle function.
-  // Only handle functions that are defined in a namespace excluding static
-  // methods (qualified by nested specifier) and functions defined in the global
-  // namespace.
+  // 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(
-      hasParent(namespaceDecl()),
-      unless(anyOf(IsInMovedNs, hasAncestor(namespaceDecl(isAnonymous())),
-                   hasAncestor(cxxRecordDecl()))));
+  auto FuncMatcher =
+      functionDecl(unless(anyOf(cxxMethodDecl(), IsInMovedNs,
+                                hasAncestor(namespaceDecl(isAnonymous())),
+                                hasAncestor(cxxRecordDecl()))),
+                   hasParent(namespaceDecl()));
   Finder->addMatcher(
       decl(forEachDescendant(callExpr(callee(FuncMatcher)).bind("call")),
-           IsInMovedNs)
+           IsInMovedNs, unless(isImplicit()))
           .bind("dc"),
       this);
 }
 
 void ChangeNamespaceTool::run(
     const ast_matchers::MatchFinder::MatchResult &Result) {
   if (const auto *NsDecl = Result.Nodes.getNodeAs<NamespaceDecl>("old_ns")) {
     moveOldNamespace(Result, NsDecl);
void ChangeNamespaceTool::moveClassForwardDeclaration(
   InsertForwardDeclaration InsertFwd;
   InsertFwd.InsertionOffset = Insertion.getOffset();
   InsertFwd.ForwardDeclText = Insertion.getReplacementText().str();
   InsertFwdDecls[Insertion.getFilePath()].push_back(InsertFwd);
 }
 
 // Replaces a qualified symbol that refers to a declaration `DeclName` with the
 // shortest qualified name possible when the reference is in `NewNamespace`.
+// FIXME: don't need to add redundant namespace qualifier when there is
+// UsingShadowDecl or using namespace decl.
 void ChangeNamespaceTool::replaceQualifiedSymbolInDeclContext(
     const ast_matchers::MatchFinder::MatchResult &Result, const Decl *DeclCtx,
     SourceLocation Start, SourceLocation End, llvm::StringRef DeclName) {
   const auto *NsDeclContext =
       DeclCtx->getDeclContext()->getEnclosingNamespaceContext();
   const auto *NsDecl = llvm::dyn_cast<NamespaceDecl>(NsDeclContext);
   // Calculate the name of the `NsDecl` after it is moved to new namespace.
   std::string OldNs = NsDecl->getQualifiedNameAsString();

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, 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 = "namespace na {\n"
+  std::string Code = "class GLOB {};\n"
+                     "using BLOG = GLOB;\n"
+                     "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"
+  std::string Expected = "class GLOB {};\n"
+                         "using BLOG = GLOB;\n"
+                         "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, UsingShadowDeclInFunction) {
+  std::string Code = "namespace glob {\n"
+                     "class Glob {};\n"
+                     "}\n"
+                     "namespace na {\n"
+                     "namespace nb {\n"
+                     "void f() {\n"
+                     "  using glob::Glob;\n"
+                     "  Glob g;\n"
+                     "}\n"
+                     "} // namespace nb\n"
+                     "} // namespace na\n";
+
+  // FIXME: don't add namespace qualifier when there is UsingShadowDecl.
+  std::string Expected = "namespace glob {\n"
+                         "class Glob {};\n"
+                         "}\n"
+                         "\n"
+                         "namespace x {\n"
+                         "namespace y {\n"
+                         "void f() {\n"
+                         "  using ::glob::Glob;\n"
+                         "  glob::Glob g;\n"
+                         "}\n"
+                         "} // namespace y\n"
+                         "} // namespace x\n";
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, UsingShadowDeclInGlobal) {
+  std::string Code = "namespace glob {\n"
+                     "class Glob {};\n"
+                     "}\n"
+                     "using glob::Glob;\n"
+                     "namespace na {\n"
+                     "namespace nb {\n"
+                     "void f() { Glob g; }\n"
+                     "} // namespace nb\n"
+                     "} // namespace na\n";
+
+  // FIXME: don't add namespace qualifier when there is UsingShadowDecl.
+  std::string Expected = "namespace glob {\n"
+                         "class Glob {};\n"
+                         "}\n"
+                         "using glob::Glob;\n"
+                         "\n"
+                         "namespace x {\n"
+                         "namespace y {\n"
+                         "void f() { glob::Glob g; }\n"
+                         "} // namespace y\n"
+                         "} // namespace x\n";
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, UsingNamespace) {
+  std::string Code = "namespace glob {\n"
+                     "class Glob {};\n"
+                     "}\n"
+                     "using namespace glob;\n"
+                     "namespace na {\n"
+                     "namespace nb {\n"
+                     "void f() { Glob g; }\n"
+                     "} // namespace nb\n"
+                     "} // namespace na\n";
+
+  // FIXME: don't add namespace qualifier when there is "using namespace" decl.
+  std::string Expected = "namespace glob {\n"
+                         "class Glob {};\n"
+                         "}\n"
+                         "using namespace glob;\n"
+                         "\n"
+                         "namespace x {\n"
+                         "namespace y {\n"
+                         "void f() { glob::Glob g; }\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"