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

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC358378: [Lookup] Invisible decls should not be ambiguous 
when renaming. (authored by ioeric, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D60257?vs=194841=195103#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60257/new/

https://reviews.llvm.org/D60257

Files:
  include/clang/Tooling/Core/Lookup.h
  lib/Tooling/Core/Lookup.cpp
  lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
  unittests/Tooling/LookupTest.cpp

Index: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===
--- lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -542,8 +542,8 @@
 if (!llvm::isa(
 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 {
Index: lib/Tooling/Core/Lookup.cpp
===
--- lib/Tooling/Core/Lookup.cpp
+++ lib/Tooling/Core/Lookup.cpp
@@ -14,6 +14,7 @@
 #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;
@@ -123,7 +124,8 @@
 // FIXME: consider using namespaces.
 static std::string disambiguateSpellingInScope(StringRef Spelling,
StringRef QName,
-   const DeclContext ) {
+   const DeclContext ,
+   SourceLocation UseLoc) {
   assert(QName.startswith("::"));
   assert(QName.endswith(Spelling));
   if (Spelling.startswith("::"))
@@ -138,9 +140,10 @@
   getAllNamedNamespaces();
   auto  = UseContext.getParentASTContext();
   StringRef TrimmedQName = QName.substr(2);
+  const auto  = UseContext.getParentASTContext().getSourceManager();
+  UseLoc = SM.getSpellingLoc(UseLoc);
 
-  auto IsAmbiguousSpelling = [, , ](
- 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
@@ -151,7 +154,13 @@
   auto LookupRes = NS->lookup(DeclarationName((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;
   }
 }
@@ -172,6 +181,7 @@
 }
 
 std::string tooling::replaceNestedName(const NestedNameSpecifier *Use,
+   SourceLocation UseLoc,
const DeclContext *UseContext,
const NamedDecl *FromDecl,
StringRef ReplacementString) {
@@ -206,5 +216,6 @@
   StringRef Suggested = getBestNamespaceSubstr(UseContext, ReplacementString,
isFullyQualified(Use));
 
-  return disambiguateSpellingInScope(Suggested, ReplacementString, *UseContext);
+  return disambiguateSpellingInScope(Suggested, ReplacementString, *UseContext,
+ UseLoc);
 }
Index: unittests/Tooling/LookupTest.cpp
===
--- unittests/Tooling/LookupTest.cpp
+++ unittests/Tooling/LookupTest.cpp
@@ -44,8 +44,8 @@
 const auto *Callee = cast(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) {
@@ -181,12 +181,12 @@
 

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

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 194841.
ioeric marked 2 inline comments as done.
ioeric added a comment.

- Add test comment.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60257/new/

https://reviews.llvm.org/D60257

Files:
  include/clang/Tooling/Core/Lookup.h
  lib/Tooling/Core/Lookup.cpp
  lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
  unittests/Tooling/LookupTest.cpp

Index: unittests/Tooling/LookupTest.cpp
===
--- unittests/Tooling/LookupTest.cpp
+++ unittests/Tooling/LookupTest.cpp
@@ -44,8 +44,8 @@
 const auto *Callee = cast(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) {
@@ -181,12 +181,12 @@
 TEST(LookupTest, replaceNestedClassName) {
   GetDeclsVisitor Visitor;
 
-  auto replaceRecordTypeLoc = [&](RecordTypeLoc Loc,
+  auto replaceRecordTypeLoc = [&](RecordTypeLoc TLoc,
   StringRef ReplacementString) {
-const auto *FD = cast(Loc.getDecl());
+const auto *FD = cast(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) {
@@ -211,6 +211,40 @@
   };
   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
Index: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===
--- lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -542,8 +542,8 @@
 if (!llvm::isa(
 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 {
Index: lib/Tooling/Core/Lookup.cpp
===
--- lib/Tooling/Core/Lookup.cpp
+++ lib/Tooling/Core/Lookup.cpp
@@ -14,6 +14,7 @@
 #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;
@@ -123,7 +124,8 @@
 // FIXME: consider using namespaces.
 static std::string disambiguateSpellingInScope(StringRef Spelling,
StringRef QName,
-   const DeclContext ) {
+   const DeclContext ,
+   SourceLocation UseLoc) {
   assert(QName.startswith("::"));
   assert(QName.endswith(Spelling));
   if (Spelling.startswith("::"))
@@ -138,9 +140,10 @@
   getAllNamedNamespaces();
   auto  = UseContext.getParentASTContext();
   StringRef TrimmedQName = QName.substr(2);
+  const auto  = UseContext.getParentASTContext().getSourceManager();
+  UseLoc = SM.getSpellingLoc(UseLoc);
 
-  auto IsAmbiguousSpelling = [, , ](
- const llvm::StringRef CurSpelling) {
+  auto IsAmbiguousSpelling = [&](const llvm::StringRef CurSpelling) {
 if (CurSpelling.startswith("::"))
 

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

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: unittests/Tooling/LookupTest.cpp:215
+
+  // Potentially ambiguous symbols that are not visible at reference location
+  // are not considered ambiguous.

hokein wrote:
> 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?
added comment. PTAL


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60257/new/

https://reviews.llvm.org/D60257



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: unittests/Tooling/LookupTest.cpp:215
+
+  // Potentially ambiguous symbols that are not visible at reference location
+  // are not considered ambiguous.

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?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60257/new/

https://reviews.llvm.org/D60257



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added a reviewer: hokein.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D60257

Files:
  include/clang/Tooling/Core/Lookup.h
  lib/Tooling/Core/Lookup.cpp
  lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
  unittests/Tooling/LookupTest.cpp

Index: unittests/Tooling/LookupTest.cpp
===
--- unittests/Tooling/LookupTest.cpp
+++ unittests/Tooling/LookupTest.cpp
@@ -44,8 +44,8 @@
 const auto *Callee = cast(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) {
@@ -181,12 +181,12 @@
 TEST(LookupTest, replaceNestedClassName) {
   GetDeclsVisitor Visitor;
 
-  auto replaceRecordTypeLoc = [&](RecordTypeLoc Loc,
+  auto replaceRecordTypeLoc = [&](RecordTypeLoc TLoc,
   StringRef ReplacementString) {
-const auto *FD = cast(Loc.getDecl());
+const auto *FD = cast(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) {
@@ -211,6 +211,36 @@
   };
   Visitor.runOver("namespace a { namespace b { class Foo {}; } }\n"
   "namespace c { using a::b::Foo; Foo f();; }\n");
+
+  // Potentially ambiguous symbols that are not visible at reference location
+  // are not considered ambiguous.
+  Visitor.OnRecordTypeLoc = [&](RecordTypeLoc Type) {
+if (Type.getDecl()->getQualifiedNameAsString() == "x::y::Old")
+  EXPECT_EQ("Foo", replaceRecordTypeLoc(Type, "::x::Foo"));
+  };
+  Visitor.runOver(R"(
+// Definitions.
+namespace x {
+ namespace y {
+  class Old {};
+  class Other {};
+ }
+}
+
+// Suppose this is a reference in some header file.
+namespace x {
+ namespace y {
+  Old f;  // This is to be renamed.
+ }
+}
+
+// Suppose this is in a file that includes the above header.
+namespace x {
+namespace y {
+ using Foo = ::x::y::Other;
+}
+}
+)");
 }
 
 } // end anonymous namespace
Index: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===
--- lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -542,8 +542,8 @@
 if (!llvm::isa(
 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 {
Index: lib/Tooling/Core/Lookup.cpp
===
--- lib/Tooling/Core/Lookup.cpp
+++ lib/Tooling/Core/Lookup.cpp
@@ -14,6 +14,7 @@
 #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;
@@ -123,7 +124,8 @@
 // FIXME: consider using namespaces.
 static std::string disambiguateSpellingInScope(StringRef Spelling,
StringRef QName,
-   const DeclContext ) {
+   const DeclContext ,
+   SourceLocation UseLoc) {
   assert(QName.startswith("::"));
   assert(QName.endswith(Spelling));
   if (Spelling.startswith("::"))
@@ -138,9 +140,10 @@
   getAllNamedNamespaces();
   auto  = UseContext.getParentASTContext();
   StringRef TrimmedQName = QName.substr(2);
+  const auto  = UseContext.getParentASTContext().getSourceManager();
+  UseLoc = SM.getSpellingLoc(UseLoc);
 
-  auto IsAmbiguousSpelling = [, , ](
- const llvm::StringRef