[clang-tools-extra] r344897 - [change-namespace] Enhance detection of conflicting namespaces.

[Eric Liu via cfe-commits]

Author: ioeric
Date: Mon Oct 22 05:48:49 2018
New Revision: 344897

URL: http://llvm.org/viewvc/llvm-project?rev=344897&view=rev
Log:
[change-namespace] Enhance detection of conflicting namespaces.

Summary:
For example:
```
namespace util { class Base; }

namespace new {
namespace util { class Internal; }
}

namespace old {
util::Base b1;
}
```

When changing `old::` to `new::`, `util::` in namespace "new::" will conflict
with "new::util::" unless a leading "::" is added.

Reviewers: hokein

Subscribers: cfe-commits

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

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

Modified: clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp?rev=344897&r1=344896&r2=344897&view=diff
==============================================================================
--- clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp (original)
+++ clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp Mon Oct 22 05:48:49 2018
@@ -7,8 +7,10 @@
 //
 //===----------------------------------------------------------------------===//
 #include "ChangeNamespace.h"
+#include "clang/AST/ASTContext.h"
 #include "clang/Format/Format.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
 
 using namespace clang::ast_matchers;
@@ -283,23 +285,48 @@ bool isDeclVisibleAtLocation(const Sourc
 }
 
 // Given a qualified symbol name, returns true if the symbol will be
-// incorrectly qualified without leading "::".
-bool conflictInNamespace(llvm::StringRef QualifiedSymbol,
+// incorrectly qualified without leading "::". For example, a symbol
+// "nx::ny::Foo" in namespace "na::nx::ny" without leading "::"; a symbol
+// "util::X" in namespace "na" can potentially conflict with "na::util" (if this
+// exists).
+bool conflictInNamespace(const ASTContext &AST, llvm::StringRef QualifiedSymbol,
                          llvm::StringRef Namespace) {
   auto SymbolSplitted = splitSymbolName(QualifiedSymbol.trim(":"));
   assert(!SymbolSplitted.empty());
   SymbolSplitted.pop_back();  // We are only interested in namespaces.
 
-  if (SymbolSplitted.size() > 1 && !Namespace.empty()) {
+  if (SymbolSplitted.size() >= 1 && !Namespace.empty()) {
+    auto SymbolTopNs = SymbolSplitted.front();
     auto NsSplitted = splitSymbolName(Namespace.trim(":"));
     assert(!NsSplitted.empty());
-    // We do not check the outermost namespace since it would not be a conflict
-    // if it equals to the symbol's outermost namespace and the symbol name
-    // would have been shortened.
+
+    auto LookupDecl = [&AST](const Decl &Scope,
+                             llvm::StringRef Name) -> const NamedDecl * {
+      const auto *DC = llvm::dyn_cast<DeclContext>(&Scope);
+      if (!DC)
+        return nullptr;
+      auto LookupRes = DC->lookup(DeclarationName(&AST.Idents.get(Name)));
+      if (LookupRes.empty())
+        return nullptr;
+      return LookupRes.front();
+    };
+    // We do not check the outermost namespace since it would not be a
+    // conflict if it equals to the symbol's outermost namespace and the
+    // symbol name would have been shortened.
+    const NamedDecl *Scope =
+        LookupDecl(*AST.getTranslationUnitDecl(), NsSplitted.front());
     for (auto I = NsSplitted.begin() + 1, E = NsSplitted.end(); I != E; ++I) {
-      if (*I == SymbolSplitted.front())
+      if (*I == SymbolTopNs) // Handles "::ny" in "::nx::ny" case.
         return true;
+      // Handles "::util" and "::nx::util" conflicts.
+      if (Scope) {
+        if (LookupDecl(*Scope, SymbolTopNs))
+          return true;
+        Scope = LookupDecl(*Scope, *I);
+      }
     }
+    if (Scope && LookupDecl(*Scope, SymbolTopNs))
+      return true;
   }
   return false;
 }
@@ -844,15 +871,16 @@ void ChangeNamespaceTool::replaceQualifi
       }
     }
   }
+  bool Conflict = conflictInNamespace(DeclCtx->getParentASTContext(),
+                                      ReplaceName, NewNamespace);
   // 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 ||
+  // old namespace, we don't create replacement unless there can be ambiguity.
+  if ((NestedName == ReplaceName && !Conflict) ||
       (NestedName.startswith("::") && NestedName.drop_front(2) == ReplaceName))
     return;
   // If the reference need to be fully-qualified, add a leading "::" unless
   // NewNamespace is the global namespace.
-  if (ReplaceName == FromDeclName && !NewNamespace.empty() &&
-      conflictInNamespace(ReplaceName, NewNamespace))
+  if (ReplaceName == FromDeclName && !NewNamespace.empty() && Conflict)
     ReplaceName = "::" + ReplaceName;
   addReplacementOrDie(Start, End, ReplaceName, *Result.SourceManager,
                       &FileToReplacements);

Modified: clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp?rev=344897&r1=344896&r2=344897&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp Mon Oct 22 05:48:49 2018
@@ -2244,6 +2244,39 @@ TEST_F(ChangeNamespaceTest, InjectedClas
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
 }
 
+TEST_F(ChangeNamespaceTest, FullyQualifyConflictNamespace) {
+  std::string Code =
+      "namespace x { namespace util { class Some {}; } }\n"
+      "namespace x { namespace y {namespace base { class Base {}; } } }\n"
+      "namespace util { class Status {}; }\n"
+      "namespace base { class Base {}; }\n"
+      "namespace na {\n"
+      "namespace nb {\n"
+      "void f() {\n"
+      "  util::Status s1; x::util::Some s2;\n"
+      "  base::Base b1; x::y::base::Base b2;\n"
+      "}\n"
+      "} // namespace nb\n"
+      "} // namespace na\n";
+
+  std::string Expected =
+      "namespace x { namespace util { class Some {}; } }\n"
+      "namespace x { namespace y {namespace base { class Base {}; } } }\n"
+      "namespace util { class Status {}; }\n"
+      "namespace base { class Base {}; }\n"
+      "\n"
+      "namespace x {\n"
+      "namespace y {\n"
+      "void f() {\n"
+      "  ::util::Status s1; util::Some s2;\n"
+      "  ::base::Base b1; base::Base b2;\n"
+      "}\n"
+      "} // namespace y\n"
+      "} // namespace x\n";
+
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
 } // anonymous namespace
 } // namespace change_namespace
 } // namespace clang