Fully qualify the renamed symbol if the shortened name is ambiguous.

[ioeric]

Summary

For example, when renaming a::b::x::foo to y::foo below, replacing
x::foo() with y::foo() can cause ambiguity. In such cases, we simply fully
qualify the name with leading ::.

namespace a {
namespace b {
namespace x { void foo() {} }
namespace y { void foo() {} }
}
}

namespace a {
namespace b {
void f() { x::foo(); }
}
}

[ioeric]

• minor update to comment.

[ilya-biryukov]

lib/Tooling/Core/Lookup.cpp

137 ↗

(On Diff #158761)

maybe use range-based-for here? I.e. for (const NamespaceDecl* NS : UseNamespaces) { ... }

139 ↗

(On Diff #158761)

This will not take using namespaces into account, right? Do we care about those or is this a known limitation of the tools?

[ioeric]

• address review comments.

[ioeric]

lib/Tooling/Core/Lookup.cpp

139 ↗

(On Diff #158761)

Right, I think we don't handle using namespaces in this library due to limitation/importance etc. Added a FIXME.

[ilya-biryukov]

LGTM

cfe/trunk/lib/Tooling/Core/Lookup.cpp

 //===--- Lookup.cpp - Framework for clang refactoring tools ---------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines helper methods for clang tools performing name lookup.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Tooling/Core/Lookup.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclarationName.h"
 using namespace clang;
 using namespace clang::tooling;
 
 // Gets all namespaces that \p Context is in as a vector (ignoring anonymous
 // namespaces). The inner namespaces come before outer namespaces in the vector.
 // For example, if the context is in the following namespace:
 //    `namespace a { namespace b { namespace c ( ... ) } }`,
 // the vector will be `{c, b, a}`.
static bool isFullyQualified(const NestedNameSpecifier *NNS) {
   while (NNS) {
     if (NNS->getKind() == NestedNameSpecifier::Global)
       return true;
     NNS = NNS->getPrefix();
   }
   return false;
 }
 
+// Returns true if spelling symbol \p QName as \p Spelling in \p UseContext is
+// ambiguous. For example, if QName is "::y::bar" and the spelling is "y::bar"
+// in `UseContext` "a" that contains a nested namespace "a::y", then "y::bar"
+// can be resolved to ::a::y::bar, which can cause compile error.
+// FIXME: consider using namespaces.
+static bool isAmbiguousNameInScope(StringRef Spelling, StringRef QName,
+                                   const DeclContext &UseContext) {
+  assert(QName.startswith("::"));
+  if (Spelling.startswith("::"))
+    return false;
+
+  // Lookup the first component of Spelling in all enclosing namespaces and
+  // check if there is any existing symbols with the same name but in different
+  // scope.
+  StringRef Head = Spelling.split("::").first;
+
+  llvm::SmallVector<const NamespaceDecl *, 4> UseNamespaces =
+      getAllNamedNamespaces(&UseContext);
+  auto &AST = UseContext.getParentASTContext();
+  StringRef TrimmedQName = QName.substr(2);
+  for (const auto *NS : UseNamespaces) {
+    auto LookupRes = NS->lookup(DeclarationName(&AST.Idents.get(Head)));
+    if (!LookupRes.empty()) {
+      for (const NamedDecl *Res : LookupRes)
+        if (!TrimmedQName.startswith(Res->getQualifiedNameAsString()))
+          return true;
+    }
+  }
+  return false;
+}
+
 std::string tooling::replaceNestedName(const NestedNameSpecifier *Use,
                                        const DeclContext *UseContext,
                                        const NamedDecl *FromDecl,
                                        StringRef ReplacementString) {
   assert(ReplacementString.startswith("::") &&
          "Expected fully-qualified name!");
 
   // We can do a raw name replacement when we are not inside the namespace for
if (class_name_only && !in_global_namespace && !is_class_forward_decl &&
     auto Pos = ReplacementString.rfind("::");
     return Pos != StringRef::npos ? ReplacementString.substr(Pos + 2)
                                   : ReplacementString;
   }
   // We did not match this because of a using statement, so we will need to
   // figure out how good a namespace match we have with our destination type.
   // We work backwards (from most specific possible namespace to least
   // specific).
-  return getBestNamespaceSubstr(UseContext, ReplacementString,
+  StringRef Suggested = getBestNamespaceSubstr(UseContext, ReplacementString,
                                                isFullyQualified(Use));
+  // Use the fully qualified name if the suggested name is ambiguous.
+  // FIXME: consider re-shortening the name until the name is not ambiguous. We
+  // are not doing this because ambiguity is pretty bad and we should not try to
+  // be clever in handling such cases. Making this noticeable to users seems to
+  // be a better option.
+  return isAmbiguousNameInScope(Suggested, ReplacementString, *UseContext)
+             ? ReplacementString
+             : Suggested;
 }

cfe/trunk/unittests/Tooling/LookupTest.cpp

TEST(LookupTest, replaceNestedFunctionName) {
 
   Visitor.OnCall = [&](CallExpr *Expr) {
     EXPECT_EQ("a::bar", replaceCallExpr(Expr, "::a::bar"));
   };
   Visitor.runOver("namespace a { void foo(); }\n"
                   "namespace b { void f() { a::foo(); } }\n");
 
   Visitor.OnCall = [&](CallExpr *Expr) {
-    EXPECT_EQ("a::bar", replaceCallExpr(Expr, "::a::bar"));
+    EXPECT_EQ("::a::bar", replaceCallExpr(Expr, "::a::bar"));
   };
   Visitor.runOver("namespace a { void foo(); }\n"
                   "namespace b { namespace a { void foo(); }\n"
                   "void f() { a::foo(); } }\n");
 
   Visitor.OnCall = [&](CallExpr *Expr) {
     EXPECT_EQ("c::bar", replaceCallExpr(Expr, "::a::c::bar"));
   };
TEST(LookupTest, replaceNestedFunctionName) {
 
   Visitor.OnCall = [&](CallExpr *Expr) {
     EXPECT_EQ("x::bar", replaceCallExpr(Expr, "::a::x::bar"));
   };
   Visitor.runOver("namespace a { namespace b { void foo(); } }\n"
                   "namespace a { namespace b { namespace c {"
                   "void f() { foo(); }"
                   "} } }\n");
+
+  // If the shortest name is ambiguous, we need to add more qualifiers.
+  Visitor.OnCall = [&](CallExpr *Expr) {
+    EXPECT_EQ("::a::y::bar", replaceCallExpr(Expr, "::a::y::bar"));
+  };
+  Visitor.runOver(R"(
+    namespace a {
+    namespace b {
+    namespace x { void foo() {} }
+    namespace y { void foo() {} }
+    }
+    }
+
+    namespace a {
+    namespace b {
+    void f() { x::foo(); }
+    }
+    })");
+
+  Visitor.OnCall = [&](CallExpr *Expr) {
+    EXPECT_EQ("y::bar", replaceCallExpr(Expr, "::y::bar"));
+  };
+  Visitor.runOver(R"(
+    namespace a {
+    namespace b {
+    namespace x { void foo() {} }
+    namespace y { void foo() {} }
+    }
+    }
+
+    void f() { a::b::x::foo(); }
+    )");
 }
 
 TEST(LookupTest, replaceNestedClassName) {
   GetDeclsVisitor Visitor;
 
   auto replaceRecordTypeLoc = [&](RecordTypeLoc Loc,
                                   StringRef ReplacementString) {
     const auto *FD = cast<CXXRecordDecl>(Loc.getDecl());