Workaround ASTMatcher crashes. Added some more test cases.

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.

Reviewers: klimek, sbenza, aaron.ballman, hokein

Subscribers: aaron.ballman, cfe-commits

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

llvm-svn: 282486

diff --git a/clang-tools-extra/change-namespace/ChangeNamespace.cpp b/clang-tools-extra/change-namespace/ChangeNamespace.cpp
index b63bc94..072fc1d 100644 (file)
--- a/clang-tools-extra/change-namespace/ChangeNamespace.cpp
+++ b/clang-tools-extra/change-namespace/ChangeNamespace.cpp
@@ -256,9 +256,10 @@ void ChangeNamespaceTool::registerMatchers(ast_matchers::MatchFinder *Finder) {
   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.
@@ -271,10 +272,13 @@ void ChangeNamespaceTool::registerMatchers(ast_matchers::MatchFinder *Finder) {
               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")),
@@ -284,18 +288,19 @@ void ChangeNamespaceTool::registerMatchers(ast_matchers::MatchFinder *Finder) {
                      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);
 }
@@ -432,6 +437,8 @@ void ChangeNamespaceTool::moveClassForwardDeclaration(
 
 // 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) {

diff --git a/clang-tools-extra/unittests/change-namespace/ChangeNamespaceTests.cpp b/clang-tools-extra/unittests/change-namespace/ChangeNamespaceTests.cpp
index f14d51d..636097c 100644 (file)
--- a/clang-tools-extra/unittests/change-namespace/ChangeNamespaceTests.cpp
+++ b/clang-tools-extra/unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -229,8 +229,23 @@ 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 = "namespace na {\n"
+  std::string Code = "class GLOB {};\n"
+                     "using BLOG = GLOB;\n"
+                     "namespace na {\n"
                      "namespace nc {\n"
                      "class SAME {};\n"
                      "}\n"
@@ -245,7 +260,9 @@ TEST_F(ChangeNamespaceTest, FixUsingShadowDecl) {
                      "} // 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"
@@ -265,6 +282,85 @@ TEST_F(ChangeNamespaceTest, FixUsingShadowDecl) {
   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"