[Lookup] Invisible decls should not be ambiguous when renaming.

[ioeric]

Summary

For example, a renamed type in a header file can conflict with declaration in
a random file that includes the header, but we should not consider the decl ambiguous if
it's not visible at the rename location. This improves consistency of generated replacements
when header file is included in different TUs.

[hokein]

unittests/Tooling/LookupTest.cpp

215

The test seems hard to understand what it actually does, could you explain it? which symbol is ambiguous here? What's the behavior of the test before vs after the fix?

[ioeric]

• Add test comment.

unittests/Tooling/LookupTest.cpp

215

added comment. PTAL

include/clang/Tooling/Core/Lookup.h

 //===--- Lookup.h - Framework for clang refactoring tools --*- C++ -*------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines helper methods for clang tools performing name lookup.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLING_CORE_LOOKUP_H
 #define LLVM_CLANG_TOOLING_CORE_LOOKUP_H
 
 #include "clang/Basic/LLVM.h"
+#include "clang/Basic/SourceLocation.h"
 #include <string>
 
 namespace clang {
 
 class DeclContext;
 class NamedDecl;
 class NestedNameSpecifier;
 
 namespace tooling {
 
 /// Emulate a lookup to replace one nested name specifier with another using as
 /// few additional namespace qualifications as possible.
 ///
 /// This does not perform a full C++ lookup so ADL will not work.
 ///
 /// \param Use The nested name to be replaced.
+/// \param UseLoc The location of name to be replaced.
 /// \param UseContext The context in which the nested name is contained. This
 ///                   will be used to minimize namespace qualifications.
 /// \param FromDecl The declaration to which the nested name points.
 /// \param ReplacementString The replacement nested name. Must be fully
 ///                          qualified including a leading "::".
 /// \returns The new name to be inserted in place of the current nested name.
 std::string replaceNestedName(const NestedNameSpecifier *Use,
+                              SourceLocation UseLoc,
                               const DeclContext *UseContext,
                               const NamedDecl *FromDecl,
                               StringRef ReplacementString);
 
 } // end namespace tooling
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLING_CORE_LOOKUP_H

lib/Tooling/Core/Lookup.cpp

 //===--- Lookup.cpp - Framework for clang refactoring tools ---------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  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"
+#include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/SmallVector.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 ( ... ) } }`,

 // Adds more scope specifier to the spelled name until the spelling is not
 // ambiguous. A spelling is ambiguous if the resolution of the symbol is
 // ambiguous. For example, if QName is "::y::bar", the spelling is "y::bar", and
 // context contains a nested namespace "a::y", then "y::bar" can be resolved to
 // ::a::y::bar in the context, which can cause compile error.
 // FIXME: consider using namespaces.
 static std::string disambiguateSpellingInScope(StringRef Spelling,
                                                StringRef QName,
-                                               const DeclContext &UseContext) {
+                                               const DeclContext &UseContext,
+                                               SourceLocation UseLoc) {
   assert(QName.startswith("::"));
   assert(QName.endswith(Spelling));
   if (Spelling.startswith("::"))
     return Spelling;
 
   auto UnspelledSpecifier = QName.drop_back(Spelling.size());
   llvm::SmallVector<llvm::StringRef, 2> UnspelledScopes;
   UnspelledSpecifier.split(UnspelledScopes, "::", /*MaxSplit=*/-1,
                            /*KeepEmpty=*/false);
 
   llvm::SmallVector<const NamespaceDecl *, 4> EnclosingNamespaces =
       getAllNamedNamespaces(&UseContext);
   auto &AST = UseContext.getParentASTContext();
   StringRef TrimmedQName = QName.substr(2);
+  const auto &SM = UseContext.getParentASTContext().getSourceManager();
+  UseLoc = SM.getSpellingLoc(UseLoc);
 
-  auto IsAmbiguousSpelling = [&EnclosingNamespaces, &AST, &TrimmedQName](
-                                 const llvm::StringRef CurSpelling) {
+  auto IsAmbiguousSpelling = [&](const llvm::StringRef CurSpelling) {
     if (CurSpelling.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 = CurSpelling.split("::").first;
     for (const auto *NS : EnclosingNamespaces) {
       auto LookupRes = NS->lookup(DeclarationName(&AST.Idents.get(Head)));
       if (!LookupRes.empty()) {
         for (const NamedDecl *Res : LookupRes)
-          if (!TrimmedQName.startswith(Res->getQualifiedNameAsString()))
+          // If `Res` is not visible in `UseLoc`, we don't consider it
+          // ambiguous. For example, a reference in a header file should not be
+          // affected by a potentially ambiguous name in some file that includes
+          // the header.
+          if (!TrimmedQName.startswith(Res->getQualifiedNameAsString()) &&
+              SM.isBeforeInTranslationUnit(
+                  SM.getSpellingLoc(Res->getLocation()), UseLoc))
             return true;
       }
     }
     return false;
   };
 
   // Add more qualifiers until the spelling is not ambiguous.
   std::string Disambiguated = Spelling;
   while (IsAmbiguousSpelling(Disambiguated)) {
     if (UnspelledScopes.empty()) {
       Disambiguated = "::" + Disambiguated;
     } else {
       Disambiguated = (UnspelledScopes.back() + "::" + Disambiguated).str();
       UnspelledScopes.pop_back();
     }
   }
   return Disambiguated;
 }
 
 std::string tooling::replaceNestedName(const NestedNameSpecifier *Use,
+                                       SourceLocation UseLoc,
                                        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
   // the original class/function and it is not in the global namespace.  The
std::string tooling::replaceNestedName(const NestedNameSpecifier *Use,
   }
   // 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).
   StringRef Suggested = getBestNamespaceSubstr(UseContext, ReplacementString,
                                                isFullyQualified(Use));
 
-  return disambiguateSpellingInScope(Suggested, ReplacementString, *UseContext);
+  return disambiguateSpellingInScope(Suggested, ReplacementString, *UseContext,
+                                     UseLoc);
 }

lib/Tooling/Refactoring/Rename/USRLocFinder.cpp

if (RenameInfo.IgnorePrefixQualifers) {
       size_t LastColonPos = NewName.find_last_of(':');
       if (LastColonPos != std::string::npos)
         ReplacedName = NewName.substr(LastColonPos + 1);
     } else {
       if (RenameInfo.FromDecl && RenameInfo.Context) {
         if (!llvm::isa<clang::TranslationUnitDecl>(
                 RenameInfo.Context->getDeclContext())) {
           ReplacedName = tooling::replaceNestedName(
-              RenameInfo.Specifier, RenameInfo.Context->getDeclContext(),
-              RenameInfo.FromDecl,
+              RenameInfo.Specifier, RenameInfo.Begin,
+              RenameInfo.Context->getDeclContext(), RenameInfo.FromDecl,
               NewName.startswith("::") ? NewName.str()
                                        : ("::" + NewName).str());
         } else {
           // This fixes the case where type `T` is a parameter inside a function
           // type (e.g. `std::function<void(T)>`) and the DeclContext of `T`
           // becomes the translation unit. As a workaround, we simply use
           // fully-qualified name here for all references whose `DeclContext` is
           // the translation unit and ignore the possible existence of

unittests/Tooling/LookupTest.cpp

 TEST(LookupTest, replaceNestedFunctionName) {
   GetDeclsVisitor Visitor;
 
   auto replaceCallExpr = [&](const CallExpr *Expr,
                              StringRef ReplacementString) {
     const auto *Callee = cast<DeclRefExpr>(Expr->getCallee()->IgnoreImplicit());
     const ValueDecl *FD = Callee->getDecl();
     return tooling::replaceNestedName(
-        Callee->getQualifier(), Visitor.DeclStack.back()->getDeclContext(), FD,
-        ReplacementString);
+        Callee->getQualifier(), Callee->getLocation(),
+        Visitor.DeclStack.back()->getDeclContext(), FD, ReplacementString);
   };
 
   Visitor.OnCall = [&](CallExpr *Expr) {
     EXPECT_EQ("bar", replaceCallExpr(Expr, "::bar"));
   };
   Visitor.runOver("namespace a { void foo(); }\n"
                   "namespace a { void f() { foo(); } }\n");
 
Visitor.runOver(R"(
 
     void f() { a::b::x::foo(); }
     )");
 }
 
 TEST(LookupTest, replaceNestedClassName) {
   GetDeclsVisitor Visitor;
 
-  auto replaceRecordTypeLoc = [&](RecordTypeLoc Loc,
+  auto replaceRecordTypeLoc = [&](RecordTypeLoc TLoc,
                                   StringRef ReplacementString) {
-    const auto *FD = cast<CXXRecordDecl>(Loc.getDecl());
+    const auto *FD = cast<CXXRecordDecl>(TLoc.getDecl());
     return tooling::replaceNestedName(
-        nullptr, Visitor.DeclStack.back()->getDeclContext(), FD,
-        ReplacementString);
+        nullptr, TLoc.getBeginLoc(), Visitor.DeclStack.back()->getDeclContext(),
+        FD, ReplacementString);
   };
 
   Visitor.OnRecordTypeLoc = [&](RecordTypeLoc Type) {
     // Filter Types by name since there are other `RecordTypeLoc` in the test
     // file.
     if (Type.getDecl()->getQualifiedNameAsString() == "a::b::Foo") {
       EXPECT_EQ("x::Bar", replaceRecordTypeLoc(Type, "::a::x::Bar"));
     }
   };
   Visitor.runOver("namespace a { namespace b {\n"
                   "class Foo;\n"
                   "namespace c { Foo f();; }\n"
                   "} }\n");
 
   Visitor.OnRecordTypeLoc = [&](RecordTypeLoc Type) {
     // Filter Types by name since there are other `RecordTypeLoc` in the test
     // file.
     // `a::b::Foo` in using shadow decl is not `TypeLoc`.
     if (Type.getDecl()->getQualifiedNameAsString() == "a::b::Foo") {
       EXPECT_EQ("Bar", replaceRecordTypeLoc(Type, "::a::x::Bar"));
     }
   };
   Visitor.runOver("namespace a { namespace b { class Foo {}; } }\n"
                   "namespace c { using a::b::Foo; Foo f();; }\n");
+
+  // Rename TypeLoc `x::y::Old` to new name `x::Foo` at [0] and check that the
+  // type is replaced with "Foo" instead of "x::Foo". Although there is a symbol
+  // `x::y::Foo` in c.cc [1], it should not make "Foo" at [0] ambiguous because
+  // it's not visible at [0].
+  Visitor.OnRecordTypeLoc = [&](RecordTypeLoc Type) {
+    if (Type.getDecl()->getQualifiedNameAsString() == "x::y::Old")
+      EXPECT_EQ("Foo", replaceRecordTypeLoc(Type, "::x::Foo"));
+  };
+  Visitor.runOver(R"(
+    // a.h
+    namespace x {
+     namespace y {
+      class Old {};
+      class Other {};
+     }
+    }
+
+    // b.h
+    namespace x {
+     namespace y {
+      // This is to be renamed to x::Foo
+      // The expected replacement is "Foo".
+      Old f;  // [0].
+     }
+    }
+
+    // c.cc
+    namespace x {
+    namespace y {
+     using Foo = ::x::y::Other; // [1]
+    }
+    }
+    )");
 }
 
 } // end anonymous namespace