[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG852bafae2bb4: [clangd] Implement cross-file rename. 
(authored by hokein).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Rename.h
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h

Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -38,8 +38,8 @@
 llvm::Expected>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
 
-llvm::Expected>
-runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName);
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, StringRef NewName);
 
 std::string runDumpAST(ClangdServer &Server, PathRef File);
 
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -96,10 +96,9 @@
   return std::move(*Result);
 }
 
-llvm::Expected> runRename(ClangdServer &Server,
-PathRef File, Position Pos,
-llvm::StringRef NewName) {
-  llvm::Optional>> Result;
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, llvm::StringRef NewName) {
+  llvm::Optional> Result;
   Server.rename(File, Pos, NewName, /*WantFormat=*/true, capture(Result));
   return std::move(*Result);
 }
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -9,8 +9,10 @@
 #include "Annotations.h"
 #include "TestFS.h"
 #include "TestTU.h"
+#include "index/Ref.h"
 #include "refactor/Rename.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -18,8 +20,45 @@
 namespace clangd {
 namespace {
 
-MATCHER_P2(RenameRange, Code, Range, "") {
-  return replacementToEdit(Code, arg).range == Range;
+using testing::Eq;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
+// Build a RefSlab from all marked ranges in the annotation. The ranges are
+// assumed to associate with the given SymbolName.
+std::unique_ptr buildRefSlab(const Annotations &Code,
+  llvm::StringRef SymbolName,
+  llvm::StringRef Path) {
+  RefSlab::Builder Builder;
+  TestTU TU;
+  TU.HeaderCode = Code.code();
+  auto Symbols = TU.headerSymbols();
+  const auto &SymbolID = findSymbol(Symbols, SymbolName).ID;
+  for (const auto &Range : Code.ranges()) {
+Ref R;
+R.Kind = RefKind::Reference;
+R.Location.Start.setLine(Range.start.line);
+R.Location.Start.setColumn(Range.start.character);
+R.Location.End.setLine(Range.end.line);
+R.Location.End.setColumn(Range.end.character);
+auto U = URI::create(Path).toString();
+R.Location.FileURI = U.c_str();
+Builder.insert(SymbolID, R);
+  }
+
+  return std::make_unique(std::move(Builder).build());
+}
+
+std::vector<
+std::pair>
+applyEdits(FileEdits FE) {
+  std::vector> Results;
+  for (auto &It : FE)
+Results.emplace_back(
+It.first().str(),
+llvm::cantFail(tooling::applyAllReplacements(
+It.getValue().InitialCode, It.getValue().Replacements)));
+  return Results;
 }
 
 // Generates an expected rename result by replacing all ranges in the given
@@ -363,11 +402,11 @@
 llvm::StringRef NewName = "abcde";
 for (const auto &RenamePos : Code.points()) {
   auto RenameResult =
-  renameWithinFile(AST, testPath(TU.Filename), RenamePos, NewName);
-  ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError() << T;
-  auto ApplyResult = llvm::cantFail(
-  tooling::applyAllReplacements(Code.code(), *RenameResult));
-  EXPECT_EQ(expectedResult(Code, NewName), ApplyResult);
+  rename({RenamePos, NewName, AST, testPath(TU.Filename)});
+  ASSERT_TRUE(bool(RenameResult)) << Renam

[PATCH] D69263: [clangd] Implement cross-file rename.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: pass - 60175 tests passed, 0 failed and 732 were skipped.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 230068.
hokein marked an inline comment as done.
hokein added a comment.

Rebase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Rename.h
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h

Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -38,8 +38,8 @@
 llvm::Expected>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
 
-llvm::Expected>
-runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName);
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, StringRef NewName);
 
 std::string runDumpAST(ClangdServer &Server, PathRef File);
 
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -96,10 +96,9 @@
   return std::move(*Result);
 }
 
-llvm::Expected> runRename(ClangdServer &Server,
-PathRef File, Position Pos,
-llvm::StringRef NewName) {
-  llvm::Optional>> Result;
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, llvm::StringRef NewName) {
+  llvm::Optional> Result;
   Server.rename(File, Pos, NewName, /*WantFormat=*/true, capture(Result));
   return std::move(*Result);
 }
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -9,8 +9,10 @@
 #include "Annotations.h"
 #include "TestFS.h"
 #include "TestTU.h"
+#include "index/Ref.h"
 #include "refactor/Rename.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -18,8 +20,45 @@
 namespace clangd {
 namespace {
 
-MATCHER_P2(RenameRange, Code, Range, "") {
-  return replacementToEdit(Code, arg).range == Range;
+using testing::Eq;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
+// Build a RefSlab from all marked ranges in the annotation. The ranges are
+// assumed to associate with the given SymbolName.
+std::unique_ptr buildRefSlab(const Annotations &Code,
+  llvm::StringRef SymbolName,
+  llvm::StringRef Path) {
+  RefSlab::Builder Builder;
+  TestTU TU;
+  TU.HeaderCode = Code.code();
+  auto Symbols = TU.headerSymbols();
+  const auto &SymbolID = findSymbol(Symbols, SymbolName).ID;
+  for (const auto &Range : Code.ranges()) {
+Ref R;
+R.Kind = RefKind::Reference;
+R.Location.Start.setLine(Range.start.line);
+R.Location.Start.setColumn(Range.start.character);
+R.Location.End.setLine(Range.end.line);
+R.Location.End.setColumn(Range.end.character);
+auto U = URI::create(Path).toString();
+R.Location.FileURI = U.c_str();
+Builder.insert(SymbolID, R);
+  }
+
+  return std::make_unique(std::move(Builder).build());
+}
+
+std::vector<
+std::pair>
+applyEdits(FileEdits FE) {
+  std::vector> Results;
+  for (auto &It : FE)
+Results.emplace_back(
+It.first().str(),
+llvm::cantFail(tooling::applyAllReplacements(
+It.getValue().InitialCode, It.getValue().Replacements)));
+  return Results;
 }
 
 // Generates an expected rename result by replacing all ranges in the given
@@ -363,11 +402,11 @@
 llvm::StringRef NewName = "abcde";
 for (const auto &RenamePos : Code.points()) {
   auto RenameResult =
-  renameWithinFile(AST, testPath(TU.Filename), RenamePos, NewName);
-  ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError() << T;
-  auto ApplyResult = llvm::cantFail(
-  tooling::applyAllReplacements(Code.code(), *RenameResult));
-  EXPECT_EQ(expectedResult(Code, NewName), ApplyResult);
+  rename({RenamePos, NewName, AST, testPath(TU.Filename)});
+  ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
+  ASSERT_EQ(1u, Rename

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment as done.
hokein added a comment.

In D69263#1738289 , @ilya-biryukov 
wrote:

> LGTM.
>
> It's probably worth collecting a list of things we need to fix before 
> enabling cross-file rename and putting it somewhere (a GitHub issue, maybe?)
>  Important things that immediately come to mind:
>
> - add range-patching heuristics, measure how good they are
> - avoid `O(N^2)` when computing edits (converting from positions to offsets)
> - handle implicit references from the index
>
>   There are definitely more.


Thanks, filed at issues#193.




Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:345
+  SourceLocation SourceLocationBeg =
+  SM.getMacroArgExpandedLocation(getBeginningOfIdentifier(
+  RInputs.Pos, SM, AST.getASTContext().getLangOpts()));

ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Why is this different from `prepareRename`, which does not call 
> > > `getMacroArgExpandedLocation`?
> > > 
> > I didn't change it in this patch, but you raise a good point, 
> > `prepareRename` should call `getMacroArgExpandedLocation`.
> Yep, makes sense to change it later. Could you put a FIXME, so that we don't 
> forget about it?
Done, fixed this in this patch, it is trivial.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 229815.
hokein marked an inline comment as done.
hokein added a comment.

rebase and call getMacroArgExpandedLocation in prepareRename.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Rename.h
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h

Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -38,8 +38,8 @@
 llvm::Expected>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
 
-llvm::Expected>
-runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName);
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, StringRef NewName);
 
 std::string runDumpAST(ClangdServer &Server, PathRef File);
 
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -96,10 +96,9 @@
   return std::move(*Result);
 }
 
-llvm::Expected> runRename(ClangdServer &Server,
-PathRef File, Position Pos,
-llvm::StringRef NewName) {
-  llvm::Optional>> Result;
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, llvm::StringRef NewName) {
+  llvm::Optional> Result;
   Server.rename(File, Pos, NewName, /*WantFormat=*/true, capture(Result));
   return std::move(*Result);
 }
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -9,8 +9,10 @@
 #include "Annotations.h"
 #include "TestFS.h"
 #include "TestTU.h"
+#include "index/Ref.h"
 #include "refactor/Rename.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -18,8 +20,45 @@
 namespace clangd {
 namespace {
 
-MATCHER_P2(RenameRange, Code, Range, "") {
-  return replacementToEdit(Code, arg).range == Range;
+using testing::Eq;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
+// Build a RefSlab from all marked ranges in the annotation. The ranges are
+// assumed to associate with the given SymbolName.
+std::unique_ptr buildRefSlab(const Annotations &Code,
+  llvm::StringRef SymbolName,
+  llvm::StringRef Path) {
+  RefSlab::Builder Builder;
+  TestTU TU;
+  TU.HeaderCode = Code.code();
+  auto Symbols = TU.headerSymbols();
+  const auto &SymbolID = findSymbol(Symbols, SymbolName).ID;
+  for (const auto &Range : Code.ranges()) {
+Ref R;
+R.Kind = RefKind::Reference;
+R.Location.Start.setLine(Range.start.line);
+R.Location.Start.setColumn(Range.start.character);
+R.Location.End.setLine(Range.end.line);
+R.Location.End.setColumn(Range.end.character);
+auto U = URI::create(Path).toString();
+R.Location.FileURI = U.c_str();
+Builder.insert(SymbolID, R);
+  }
+
+  return std::make_unique(std::move(Builder).build());
+}
+
+std::vector<
+std::pair>
+applyEdits(FileEdits FE) {
+  std::vector> Results;
+  for (auto &It : FE)
+Results.emplace_back(
+It.first().str(),
+llvm::cantFail(tooling::applyAllReplacements(
+It.getValue().InitialCode, It.getValue().Replacements)));
+  return Results;
 }
 
 // Generates an expected rename result by replacing all ranges in the given
@@ -73,11 +112,11 @@
 auto AST = TU.build();
 llvm::StringRef NewName = "abcde";
 auto RenameResult =
-renameWithinFile(AST, testPath(TU.Filename), Code.point(), NewName);
+rename({Code.point(), NewName, AST, testPath(TU.Filename)});
 ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
-auto ApplyResult = llvm::cantFail(
-tooling::applyAllReplacements(Code.code(), *RenameResult));
-EXPECT_EQ(expectedResult(Code, NewName), ApplyResult);
+ASSERT_EQ(1u, RenameResult->size());
+EXPECT_EQ(applyEdits(std::move(

[PATCH] D69263: [clangd] Implement cross-file rename.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM.

It's probably worth collecting a list of things we need to fix before enabling 
cross-file rename and putting it somewhere (a GitHub issue, maybe?)
Important things that immediately come to mind:

- add range-patching heuristics, measure how good they are
- avoid `O(N^2)` when computing edits (converting from positions to offsets)
- handle implicit references from the index

There are definitely more.




Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:345
+  SourceLocation SourceLocationBeg =
+  SM.getMacroArgExpandedLocation(getBeginningOfIdentifier(
+  RInputs.Pos, SM, AST.getASTContext().getLangOpts()));

hokein wrote:
> ilya-biryukov wrote:
> > Why is this different from `prepareRename`, which does not call 
> > `getMacroArgExpandedLocation`?
> > 
> I didn't change it in this patch, but you raise a good point, `prepareRename` 
> should call `getMacroArgExpandedLocation`.
Yep, makes sense to change it later. Could you put a FIXME, so that we don't 
forget about it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 228294.
hokein marked 13 inline comments as done.
hokein added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Rename.h
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h

Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -38,8 +38,8 @@
 llvm::Expected>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
 
-llvm::Expected>
-runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName);
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, StringRef NewName);
 
 std::string runDumpAST(ClangdServer &Server, PathRef File);
 
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -96,10 +96,9 @@
   return std::move(*Result);
 }
 
-llvm::Expected> runRename(ClangdServer &Server,
-PathRef File, Position Pos,
-llvm::StringRef NewName) {
-  llvm::Optional>> Result;
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, llvm::StringRef NewName) {
+  llvm::Optional> Result;
   Server.rename(File, Pos, NewName, /*WantFormat=*/true, capture(Result));
   return std::move(*Result);
 }
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -9,8 +9,10 @@
 #include "Annotations.h"
 #include "TestFS.h"
 #include "TestTU.h"
+#include "index/Ref.h"
 #include "refactor/Rename.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -18,8 +20,45 @@
 namespace clangd {
 namespace {
 
-MATCHER_P2(RenameRange, Code, Range, "") {
-  return replacementToEdit(Code, arg).range == Range;
+using testing::Eq;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
+// Build a RefSlab from all marked ranges in the annotation. The ranges are
+// assumed to associate with the given SymbolName.
+std::unique_ptr buildRefSlab(const Annotations &Code,
+  llvm::StringRef SymbolName,
+  llvm::StringRef Path) {
+  RefSlab::Builder Builder;
+  TestTU TU;
+  TU.HeaderCode = Code.code();
+  auto Symbols = TU.headerSymbols();
+  const auto &SymbolID = findSymbol(Symbols, SymbolName).ID;
+  for (const auto &Range : Code.ranges()) {
+Ref R;
+R.Kind = RefKind::Reference;
+R.Location.Start.setLine(Range.start.line);
+R.Location.Start.setColumn(Range.start.character);
+R.Location.End.setLine(Range.end.line);
+R.Location.End.setColumn(Range.end.character);
+auto U = URI::create(Path).toString();
+R.Location.FileURI = U.c_str();
+Builder.insert(SymbolID, R);
+  }
+
+  return std::make_unique(std::move(Builder).build());
+}
+
+std::vector<
+std::pair>
+applyEdits(FileEdits FE) {
+  std::vector> Results;
+  for (auto &It : FE)
+Results.emplace_back(
+It.first().str(),
+llvm::cantFail(tooling::applyAllReplacements(
+It.getValue().InitialCode, It.getValue().Replacements)));
+  return Results;
 }
 
 // Generates an expected rename result by replacing all ranges in the given
@@ -73,11 +112,11 @@
 auto AST = TU.build();
 llvm::StringRef NewName = "abcde";
 auto RenameResult =
-renameWithinFile(AST, testPath(TU.Filename), Code.point(), NewName);
+rename({Code.point(), NewName, AST, testPath(TU.Filename)});
 ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
-auto ApplyResult = llvm::cantFail(
-tooling::applyAllReplacements(Code.code(), *RenameResult));
-EXPECT_EQ(expectedResult(Code, NewName), ApplyResult);
+ASSERT_EQ(1u, RenameResult->size());
+EXPECT_EQ(applyEdits(std::move(*RenameResult)).front().second,
+  

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:345
+  SourceLocation SourceLocationBeg =
+  SM.getMacroArgExpandedLocation(getBeginningOfIdentifier(
+  RInputs.Pos, SM, AST.getASTContext().getLangOpts()));

ilya-biryukov wrote:
> Why is this different from `prepareRename`, which does not call 
> `getMacroArgExpandedLocation`?
> 
I didn't change it in this patch, but you raise a good point, `prepareRename` 
should call `getMacroArgExpandedLocation`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:342
+  // A snapshot of all file dirty buffers.
+  llvm::StringMap SnapShot = WorkScheduler.getAllFileContents();
   auto Action = [File = File.str(), NewName = NewName.str(), Pos, WantFormat,

NIT: s/SnapShot/Snapshot



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:371
+}
+CB(std::move(*Edits));
   };

NIT: `return CB(std::move(*Edits));`
to keep all calls to `CB` consistent.



Comment at: clang-tools-extra/clangd/TUScheduler.h:186
+
+  // std::vector
   /// Schedule an async task with no dependencies.

NIT: seems like a leftover from the previous version. Maybe remove?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:48
+  auto ID = getSymbolID(&D);
+  assert(ID);
+  Req.IDs.insert(*ID);

NIT: this assert is redundant, `Optional` asserts it has a value when 
`operator*` is called.
The code could be simplified to the equivalent: `Refs.insert(*getSymbolID(&D))`



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:135
+  // Note: within-file rename does support this through the AST.
+  if (const auto *S = llvm::dyn_cast(&RenameDecl))
+if (S->isVirtual())

nit: add braces to the outer if



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:244
+return EndOffset.takeError();
+  return std::make_pair<>(*StartOffset, *EndOffset);
+};

NIT: remove `<>`. should still work, right?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:297
+  FileEdits Results;
+  std::string OldName = RenameDecl->getNameAsString();
+  for (const auto &FileAndOccurrences : AffectedFiles) {

We don't seem to use it. Remove? Or am I missing the usage?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:345
+  SourceLocation SourceLocationBeg =
+  SM.getMacroArgExpandedLocation(getBeginningOfIdentifier(
+  RInputs.Pos, SM, AST.getASTContext().getLangOpts()));

Why is this different from `prepareRename`, which does not call 
`getMacroArgExpandedLocation`?




Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:379
+return FileEdits(
+{std::make_pair<>(RInputs.MainFilePath,
+  Edit{MainFileCode, 
std::move(*MainFileRenameEdit)})});

NIT: remove `<>`



Comment at: clang-tools-extra/clangd/refactor/Tweak.h:80
 llvm::Optional ShowMessage;
 /// A mapping from file path(the one used for accessing the underlying VFS)
 /// to edits.

The comment is now redundant, since the typedef has the same comment.



Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:27
+
+std::unique_ptr buildRefSlab(const Annotations &Code,
+  llvm::StringRef SymbolName,

Could you document what this function is doing?



Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:52
+std::pair>
+applyRename(FileEdits FE) {
+  std::vector> Results;

NIT: I suggest to call it `applyEdits`, there's nothing rename-specific in this 
function.



Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:115
 ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
-auto ApplyResult = llvm::cantFail(
-tooling::applyAllReplacements(Code.code(), *RenameResult));
-EXPECT_EQ(expectedResult(Code, NewName), ApplyResult);
+EXPECT_THAT(applyRename(std::move(*RenameResult)),
+UnorderedElementsAre(

NIT: maybe simplify to `EXPECT_EQ(applyRename(...).second, expectedResult(Code, 
NewName))`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 228213.
hokein marked 5 inline comments as done.
hokein added a comment.
Herald added a subscriber: javed.absar.

- get dirty buffer in clangdServer layer;
- save a snpatshot for all dirty buffer;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Rename.h
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h

Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -38,8 +38,8 @@
 llvm::Expected>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
 
-llvm::Expected>
-runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName);
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, StringRef NewName);
 
 std::string runDumpAST(ClangdServer &Server, PathRef File);
 
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -96,10 +96,9 @@
   return std::move(*Result);
 }
 
-llvm::Expected> runRename(ClangdServer &Server,
-PathRef File, Position Pos,
-llvm::StringRef NewName) {
-  llvm::Optional>> Result;
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, llvm::StringRef NewName) {
+  llvm::Optional> Result;
   Server.rename(File, Pos, NewName, /*WantFormat=*/true, capture(Result));
   return std::move(*Result);
 }
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -9,8 +9,10 @@
 #include "Annotations.h"
 #include "TestFS.h"
 #include "TestTU.h"
+#include "index/Ref.h"
 #include "refactor/Rename.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -18,8 +20,43 @@
 namespace clangd {
 namespace {
 
-MATCHER_P2(RenameRange, Code, Range, "") {
-  return replacementToEdit(Code, arg).range == Range;
+using testing::Eq;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
+std::unique_ptr buildRefSlab(const Annotations &Code,
+  llvm::StringRef SymbolName,
+  llvm::StringRef Path) {
+  RefSlab::Builder Builder;
+  TestTU TU;
+  TU.HeaderCode = Code.code();
+  auto Symbols = TU.headerSymbols();
+  const auto &SymbolID = findSymbol(Symbols, SymbolName).ID;
+  for (const auto &Range : Code.ranges()) {
+Ref R;
+R.Kind = RefKind::Reference;
+R.Location.Start.setLine(Range.start.line);
+R.Location.Start.setColumn(Range.start.character);
+R.Location.End.setLine(Range.end.line);
+R.Location.End.setColumn(Range.end.character);
+auto U = URI::create(Path).toString();
+R.Location.FileURI = U.c_str();
+Builder.insert(SymbolID, R);
+  }
+
+  return std::make_unique(std::move(Builder).build());
+}
+
+std::vector<
+std::pair>
+applyRename(FileEdits FE) {
+  std::vector> Results;
+  for (auto &It : FE)
+Results.emplace_back(
+It.first().str(),
+llvm::cantFail(tooling::applyAllReplacements(
+It.getValue().InitialCode, It.getValue().Replacements)));
+  return Results;
 }
 
 // Generates an expected rename result by replacing all ranges in the given
@@ -73,11 +110,11 @@
 auto AST = TU.build();
 llvm::StringRef NewName = "abcde";
 auto RenameResult =
-renameWithinFile(AST, testPath(TU.Filename), Code.point(), NewName);
+rename({Code.point(), NewName, AST, testPath(TU.Filename)});
 ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
-auto ApplyResult = llvm::cantFail(
-tooling::applyAllReplacements(Code.code(), *RenameResult));
-EXPECT_EQ(expectedResult(Code, NewName), ApplyResult);
+EXPECT_THAT(applyRename(std::move(*RenameResult)),
+UnorderedElementsAre(
+Pair(testing::_, Eq(expectedR

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:764
+  /*WantFormat=*/true,
+  [this](PathRef File) { return DraftMgr.getDraft(File); },
+  [File, Params, Reply = std::move(Reply),

ilya-biryukov wrote:
> We should probably get a snapshot of all dirty buffers here instead.
> A racy  way (rename is run on a separate thread, new files updates might come 
> in in the meantime) to get contents of the file looks like a bad idea, this 
> will lead to hard-to-debug failures...
> 
> Note that `ClangdServer` also has access to all contents of all the files, 
> they are stored in `TUScheduler`, so we don't need to pass `GetDirtyBuffer` 
> callback up until the final run of `rename`
thanks, I like the idea. This could simplify the code, with this approach, the 
dirty buffer of the main file would be the same as the one used for building 
the AST.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks, this is in a good shape!
The only concern I have is the racy way to get dirty buffers, please see the 
corresponding comment.




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:764
+  /*WantFormat=*/true,
+  [this](PathRef File) { return DraftMgr.getDraft(File); },
+  [File, Params, Reply = std::move(Reply),

We should probably get a snapshot of all dirty buffers here instead.
A racy  way (rename is run on a separate thread, new files updates might come 
in in the meantime) to get contents of the file looks like a bad idea, this 
will lead to hard-to-debug failures...

Note that `ClangdServer` also has access to all contents of all the files, they 
are stored in `TUScheduler`, so we don't need to pass `GetDirtyBuffer` callback 
up until the final run of `rename`



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:317
+if (!Range)
+  // Return null if the "rename" is not valid at the position.
+  return CB(llvm::None);

NIT: comment could be shortened to 
```
/// "rename" is not valid at the position.
```
Or even removed.
Both would allow saving a line (if we put the comment on the same line as 
`return`



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:320
+if (CrossFileRename)
+  return CB(*Range); // FIXME: assueme cross-file rename always succeeds.
+

NIT: s/assueme/assume

also, in FIXME we normally give the final state, so it should probably be 
```
/// FIXME: do not assume cross-file rename always succeeds
```





Comment at: clang-tools-extra/clangd/ClangdServer.cpp:340
 void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName,
-  bool WantFormat, Callback> CB) 
{
+  bool WantFormat, DirtyBufferGetter GetDirtyBuffer,
+  Callback CB) {

`ClangdServer` can obtain the dirty buffers via `TUScheduler::getContents`



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: fail - 59861 tests passed, 21 failed and 763 were skipped.

  failed: lld.ELF/linkerscript/filename-spec.s
  failed: Clang.Index/index-module-with-vfs.m
  failed: Clang.Modules/double-quotes.m
  failed: Clang.Modules/framework-public-includes-private.m
  failed: Clang.VFS/external-names.c
  failed: Clang.VFS/framework-import.m
  failed: Clang.VFS/implicit-include.c
  failed: Clang.VFS/include-mixed-real-and-virtual.c
  failed: Clang.VFS/include-real-from-virtual.c
  failed: Clang.VFS/include-virtual-from-real.c
  failed: Clang.VFS/include.c
  failed: Clang.VFS/incomplete-umbrella.m
  failed: Clang.VFS/module-import.m
  failed: Clang.VFS/module_missing_vfs.m
  failed: Clang.VFS/real-path-found-first.m
  failed: Clang.VFS/relative-path.c
  failed: Clang.VFS/test_nonmodular.c
  failed: Clang.VFS/umbrella-framework-import-skipnonexist.m
  failed: Clang.VFS/vfsroot-include.c
  failed: Clang.VFS/vfsroot-module.m
  failed: Clang.VFS/vfsroot-with-overlay.c

Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 228028.
hokein marked 5 inline comments as done.
hokein added a comment.

- use blacklist
- change to enum scope


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Rename.h
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h

Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -38,8 +38,8 @@
 llvm::Expected>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
 
-llvm::Expected>
-runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName);
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, StringRef NewName);
 
 std::string runDumpAST(ClangdServer &Server, PathRef File);
 
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -96,11 +96,11 @@
   return std::move(*Result);
 }
 
-llvm::Expected> runRename(ClangdServer &Server,
-PathRef File, Position Pos,
-llvm::StringRef NewName) {
-  llvm::Optional>> Result;
-  Server.rename(File, Pos, NewName, /*WantFormat=*/true, capture(Result));
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, llvm::StringRef NewName) {
+  llvm::Optional> Result;
+  Server.rename(File, Pos, NewName, /*WantFormat=*/true,
+/*DirtyBuffer=*/nullptr, capture(Result));
   return std::move(*Result);
 }
 
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -9,8 +9,10 @@
 #include "Annotations.h"
 #include "TestFS.h"
 #include "TestTU.h"
+#include "index/Ref.h"
 #include "refactor/Rename.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -18,9 +20,54 @@
 namespace clangd {
 namespace {
 
+using testing::Eq;
+using testing::Matches;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
 MATCHER_P2(RenameRange, Code, Range, "") {
   return replacementToEdit(Code, arg).range == Range;
 }
+MATCHER_P2(EqualFileEdit, Code, Ranges, "") {
+  using ReplacementMatcher = testing::Matcher;
+  std::vector Expected;
+  for (const auto &R : Ranges)
+Expected.push_back(RenameRange(Code, R));
+  auto Matcher =
+  testing::internal::UnorderedElementsAreArrayMatcher(
+  Expected.begin(), Expected.end());
+  return arg.InitialCode == Code && Matches(Matcher)(arg.Replacements);
+}
+
+std::unique_ptr buildRefSlab(const Annotations &Code,
+  llvm::StringRef SymbolName,
+  llvm::StringRef Path) {
+  RefSlab::Builder Builder;
+  TestTU TU;
+  TU.HeaderCode = Code.code();
+  auto Symbols = TU.headerSymbols();
+  const auto &SymbolID = findSymbol(Symbols, SymbolName).ID;
+  for (const auto &Range : Code.ranges()) {
+Ref R;
+R.Kind = RefKind::Reference;
+R.Location.Start.setLine(Range.start.line);
+R.Location.Start.setColumn(Range.start.character);
+R.Location.End.setLine(Range.end.line);
+R.Location.End.setColumn(Range.end.character);
+auto U = URI::create(Path).toString();
+R.Location.FileURI = U.c_str();
+Builder.insert(SymbolID, R);
+  }
+
+  return std::make_unique(std::move(Builder).build());
+}
+
+std::vector> toVector(FileEdits FE) {
+  std::vector> Results;
+  for (auto &It : FE)
+Results.emplace_back(It.first().str(), std::move(It.getValue()));
+  return Results;
+}
 
 TEST(RenameTest, SingleFile) {
   struct Test {
@@ -80,10 +127,12 @@
 auto TU = TestTU::withCode(Code.code());
 auto AST = TU.build();
 auto RenameResult =
-renameWithinFile(AST, testPath(TU.Filename), Code.point(), "abcde");
+rename({Code.point(), "abcde", AST, testPath(TU.Filename)});
 ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
-auto ApplyResult =
-tooling::applyAllReplacements(Code.code(), *Rename

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:107
+if (!Index)
+  return NoIndexProvided;
+

ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Why isn't this a scope enum in the first place?
> > this is tricky, the reason why I put it here and another copy below is to 
> > avoid regression of local rename (keep existing tests passed), if we move 
> > it in the first place, the error message of local rename may be changed, 
> > e.g. when renaming an external symbol (we know that from AST) without Index 
> > support, we'd like to return "used outside of the main file" rather than 
> > "no index provided".
> > 
> > thinking more about this, the no-index case is making our code complicated 
> > (within-file rename with no-index, cross-file rename with no-index), I 
> > believe the no-index case is not important, and is rare in practice, we 
> > could change this behavior, or assume the index is always provided, this 
> > would help to simplify the code, WDYT?
> Agree, we shouldn't bother about the no-index case too much.
> My comment here was referring to the lack of `ReasonToReject::` qualifier of 
> the name.
> 
> Maybe make `ReasonToReject` a scoped enum, i.e. `enum class ReasonToReject` 
> rather than `enum ReasonToReject`?
ah, I see. Done.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:128
+
+  // A whitelist of renamable symbols.
+  if (llvm::isa(RenameDecl))

ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Why not a blacklist? I'd expect us to blacklist:
> > > - things with custom names (operators, constructors, etc)
> > > - virtual methods
> > > 
> > > Probably some things are missing from the list, but fundamentally most 
> > > that have names are pretty similar, right?
> > I believe our plan is to support major kinds of symbols (class, functions, 
> > enums, typedef, alias) , I feel scary to allow renaming nearly all 
> > `NamedDecl` (in theory, it should work but we're uncertain, for our 
> > experience of local rename, it sometimes leads to very surprising results). 
> > Having a whitelist can lead us to a better state, these symbols are 
> > **officially supported**.
> I disagree here, I don't see how `NamedDecl` is special if its name is an 
> identifier unless we something smart about it.
> My recollection is that local rename issues come from the fact that there are 
> certain kinds of references, which are not handled in the corresponding 
> visitor.
> 
> In any case, I'd suggest blacklisting symbols we know are broken, e.g. IIRC 
> we don't index references to namespaces, so they're definitely broken. And 
> finding the issues with the rest of the symbols and fixing those.
> In practice, this should improve both rename and cross-references - whenever 
> we have broken cross-file rename, there's a good chance that the indexer is 
> also broken.
Re-thinking this, you are right. Also, the indexable symbol kinds have a large 
overlap with the renamable symbol kinds.  Changed to blacklist.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:132
+  if (const auto *S = llvm::dyn_cast(&RenameDecl)) {
+// FIXME: renaming virtual methods requires to rename all overridens in
+// subclasses, which our index doesn't support yet.

ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Isn't the same true for local rename?
> > no, actually the local rename does handle this, it finds all overriden 
> > methods from the AST, and updates all of them when doing rename.
> Do we properly check overriden methods are not used outside the main file, 
> though?
> We might be assuming rename is "local" because there are not usages of the 
> original function, but there may still be usages of the overriden function.
> 
> Worth adding a comment that local rename handles that through the AST.
> Also useful to keep in mind that this is a regression, compared to the local 
> rename (i.e. if we switch to cross-file rename by default, we'll break this 
> use-case)
> Do we properly check overriden methods are not used outside the main file, 
> though?
> We might be assuming rename is "local" because there are not usages of the 
> original function, but there may still be usages of the overriden function.

No, we only check the original function, and we may encounter false positives, 
but we don't see any reports about this.

> Worth adding a comment that local rename handles that through the AST.
> Also useful to keep in mind that this is a regression, compared to the local 
> rename (i.e. if we switch to cross-file rename by default, we'll break this 
> use-case)

exactly,  a way to avoid this regression when turning on cross-file rename by 
default is to first detect whether it is safe&sufficient to perform local 
rename, if yes, we just perform the local-rename.



Comment at: clang-

[PATCH] D69263: [clangd] Implement cross-file rename.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:107
+if (!Index)
+  return NoIndexProvided;
+

hokein wrote:
> ilya-biryukov wrote:
> > Why isn't this a scope enum in the first place?
> this is tricky, the reason why I put it here and another copy below is to 
> avoid regression of local rename (keep existing tests passed), if we move it 
> in the first place, the error message of local rename may be changed, e.g. 
> when renaming an external symbol (we know that from AST) without Index 
> support, we'd like to return "used outside of the main file" rather than "no 
> index provided".
> 
> thinking more about this, the no-index case is making our code complicated 
> (within-file rename with no-index, cross-file rename with no-index), I 
> believe the no-index case is not important, and is rare in practice, we could 
> change this behavior, or assume the index is always provided, this would help 
> to simplify the code, WDYT?
Agree, we shouldn't bother about the no-index case too much.
My comment here was referring to the lack of `ReasonToReject::` qualifier of 
the name.

Maybe make `ReasonToReject` a scoped enum, i.e. `enum class ReasonToReject` 
rather than `enum ReasonToReject`?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:128
+
+  // A whitelist of renamable symbols.
+  if (llvm::isa(RenameDecl))

hokein wrote:
> ilya-biryukov wrote:
> > Why not a blacklist? I'd expect us to blacklist:
> > - things with custom names (operators, constructors, etc)
> > - virtual methods
> > 
> > Probably some things are missing from the list, but fundamentally most that 
> > have names are pretty similar, right?
> I believe our plan is to support major kinds of symbols (class, functions, 
> enums, typedef, alias) , I feel scary to allow renaming nearly all 
> `NamedDecl` (in theory, it should work but we're uncertain, for our 
> experience of local rename, it sometimes leads to very surprising results). 
> Having a whitelist can lead us to a better state, these symbols are 
> **officially supported**.
I disagree here, I don't see how `NamedDecl` is special if its name is an 
identifier unless we something smart about it.
My recollection is that local rename issues come from the fact that there are 
certain kinds of references, which are not handled in the corresponding visitor.

In any case, I'd suggest blacklisting symbols we know are broken, e.g. IIRC we 
don't index references to namespaces, so they're definitely broken. And finding 
the issues with the rest of the symbols and fixing those.
In practice, this should improve both rename and cross-references - whenever we 
have broken cross-file rename, there's a good chance that the indexer is also 
broken.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:132
+  if (const auto *S = llvm::dyn_cast(&RenameDecl)) {
+// FIXME: renaming virtual methods requires to rename all overridens in
+// subclasses, which our index doesn't support yet.

hokein wrote:
> ilya-biryukov wrote:
> > Isn't the same true for local rename?
> no, actually the local rename does handle this, it finds all overriden 
> methods from the AST, and updates all of them when doing rename.
Do we properly check overriden methods are not used outside the main file, 
though?
We might be assuming rename is "local" because there are not usages of the 
original function, but there may still be usages of the overriden function.

Worth adding a comment that local rename handles that through the AST.
Also useful to keep in mind that this is a regression, compared to the local 
rename (i.e. if we switch to cross-file rename by default, we'll break this 
use-case)



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:235
+  });
+  return AffectedFiles;
+}

hokein wrote:
> ilya-biryukov wrote:
> > Again, it looks like the function returns incorrect results and the callers 
> > should actually handle the case where `SymbolID` cannot be obtained in a 
> > custom manner.
> > 
> > Maybe accept a `SymbolID` as a parameter instead of `NamedDecl*` and the 
> > callers handle this case gracefully?
> hmm, is this critical? I think it is safe to assume getSymbolID should always 
> succeed,  I believe we make this assumption in other code parts in clangd as 
> well.
Should we maybe assert it succeeds instead?
If we make this assumption in the code, why not make it explicit?
If not, we should probably add tests where it fails.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:255
+// !positionToOffset is O(N), it is okay at the moment since we only
+// process at most 100 references.
+auto RangeOffset = toRangeOffset(Occurrence, InitialCode);

hokein wrote:
> ilya-biryukov wrote:
> > This is not true anymore, right?
> > We should probably try getting to `O(

[PATCH] D69263: [clangd] Implement cross-file rename.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: fail - 59795 tests passed, 21 failed and 762 were skipped.

  failed: lld.ELF/linkerscript/filename-spec.s
  failed: Clang.Index/index-module-with-vfs.m
  failed: Clang.Modules/double-quotes.m
  failed: Clang.Modules/framework-public-includes-private.m
  failed: Clang.VFS/external-names.c
  failed: Clang.VFS/framework-import.m
  failed: Clang.VFS/implicit-include.c
  failed: Clang.VFS/include-mixed-real-and-virtual.c
  failed: Clang.VFS/include-real-from-virtual.c
  failed: Clang.VFS/include-virtual-from-real.c
  failed: Clang.VFS/include.c
  failed: Clang.VFS/incomplete-umbrella.m
  failed: Clang.VFS/module-import.m
  failed: Clang.VFS/module_missing_vfs.m
  failed: Clang.VFS/real-path-found-first.m
  failed: Clang.VFS/relative-path.c
  failed: Clang.VFS/test_nonmodular.c
  failed: Clang.VFS/umbrella-framework-import-skipnonexist.m
  failed: Clang.VFS/vfsroot-include.c
  failed: Clang.VFS/vfsroot-module.m
  failed: Clang.VFS/vfsroot-with-overlay.c

Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:370
+  for (auto &E : *Edits) {
+if (auto Err = reformatEdit(E.getValue(), Style))
+  elog("Failed to format replacements: {0}", std::move(Err));

ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Maybe use `llvm::joinErrors` to combine multiple failures into a single 
> > > error?
> > > Should simplify the code.
> > I don't see using `llvm::joinErrors` can simplify the code here, 
> > `joinErrors` accepts two `Error` objects, and there is no way to create an 
> > empty Error object, we may end up with more code like:
> > 
> > ```
> > llvm::Optional Err;
> > for (auto &E : *Edits) {
> >if (auto E = reformatEdit(E.getValue(), Style)) {
> >   if (!Err) Err = std::move(E);
> >   else Err = joinErrors(std::move(*Err), std::move(Err));
> >}
> > }
> > ```
> Works nicely if you start with `Error::success`
> ```
> auto Err = Error::success();
> for (...) {
>   Err = llvm::joinErrors(reformatEdit(E.getValue(), Style));
> }
> if (Err)
>   return CB(std::move(Err));
> ```
thanks.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:107
+if (!Index)
+  return NoIndexProvided;
+

ilya-biryukov wrote:
> Why isn't this a scope enum in the first place?
this is tricky, the reason why I put it here and another copy below is to avoid 
regression of local rename (keep existing tests passed), if we move it in the 
first place, the error message of local rename may be changed, e.g. when 
renaming an external symbol (we know that from AST) without Index support, we'd 
like to return "used outside of the main file" rather than "no index provided".

thinking more about this, the no-index case is making our code complicated 
(within-file rename with no-index, cross-file rename with no-index), I believe 
the no-index case is not important, and is rare in practice, we could change 
this behavior, or assume the index is always provided, this would help to 
simplify the code, WDYT?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:128
+
+  // A whitelist of renamable symbols.
+  if (llvm::isa(RenameDecl))

ilya-biryukov wrote:
> Why not a blacklist? I'd expect us to blacklist:
> - things with custom names (operators, constructors, etc)
> - virtual methods
> 
> Probably some things are missing from the list, but fundamentally most that 
> have names are pretty similar, right?
I believe our plan is to support major kinds of symbols (class, functions, 
enums, typedef, alias) , I feel scary to allow renaming nearly all `NamedDecl` 
(in theory, it should work but we're uncertain, for our experience of local 
rename, it sometimes leads to very surprising results). Having a whitelist can 
lead us to a better state, these symbols are **officially supported**.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:132
+  if (const auto *S = llvm::dyn_cast(&RenameDecl)) {
+// FIXME: renaming virtual methods requires to rename all overridens in
+// subclasses, which our index doesn't support yet.

ilya-biryukov wrote:
> Isn't the same true for local rename?
no, actually the local rename does handle this, it finds all overriden methods 
from the AST, and updates all of them when doing rename.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:221
+  if (!Index)
+return {};
+  RefsRequest RQuest;

ilya-biryukov wrote:
> This behavior is very surprising... Obviously, returning empty results is 
> incorrect and the callers have to handle the no-index case in a custom manner.
> 
> Maybe accept only non-null index and handle in the caller?
moved the no-index handling to the caller, the caller code is still a bit fuzzy.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:235
+  });
+  return AffectedFiles;
+}

ilya-biryukov wrote:
> Again, it looks like the function returns incorrect results and the callers 
> should actually handle the case where `SymbolID` cannot be obtained in a 
> custom manner.
> 
> Maybe accept a `SymbolID` as a parameter instead of `NamedDecl*` and the 
> callers handle this case gracefully?
hmm, is this critical? I think it is safe to assume getSymbolID should always 
succeed,  I believe we make this assumption in other code parts in clangd as 
well.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:255
+// !positionToOffset is O(N), it is okay at the moment since we only
+// process at most 100 references.
+auto RangeOffset = toRangeOffset(Occurrence, InitialCode);

ilya-biryukov wrote:
> This is not true anymore, right?
> We should probably try getting to `O(N)` to avoid slowing things down
> 
> Could be a FIXME for now, but have to fix it soon.
yes, I think it is not too bad to leave it n

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 227860.
hokein marked 13 inline comments as done.
hokein added a comment.

address review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Rename.h
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h

Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -38,8 +38,8 @@
 llvm::Expected>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
 
-llvm::Expected>
-runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName);
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, StringRef NewName);
 
 std::string runDumpAST(ClangdServer &Server, PathRef File);
 
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -96,11 +96,11 @@
   return std::move(*Result);
 }
 
-llvm::Expected> runRename(ClangdServer &Server,
-PathRef File, Position Pos,
-llvm::StringRef NewName) {
-  llvm::Optional>> Result;
-  Server.rename(File, Pos, NewName, /*WantFormat=*/true, capture(Result));
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, llvm::StringRef NewName) {
+  llvm::Optional> Result;
+  Server.rename(File, Pos, NewName, /*WantFormat=*/true,
+/*DirtyBuffer=*/nullptr, capture(Result));
   return std::move(*Result);
 }
 
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -9,8 +9,10 @@
 #include "Annotations.h"
 #include "TestFS.h"
 #include "TestTU.h"
+#include "index/Ref.h"
 #include "refactor/Rename.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -18,9 +20,54 @@
 namespace clangd {
 namespace {
 
+using testing::Eq;
+using testing::Matches;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
 MATCHER_P2(RenameRange, Code, Range, "") {
   return replacementToEdit(Code, arg).range == Range;
 }
+MATCHER_P2(EqualFileEdit, Code, Ranges, "") {
+  using ReplacementMatcher = testing::Matcher;
+  std::vector Expected;
+  for (const auto &R : Ranges)
+Expected.push_back(RenameRange(Code, R));
+  auto Matcher =
+  testing::internal::UnorderedElementsAreArrayMatcher(
+  Expected.begin(), Expected.end());
+  return arg.InitialCode == Code && Matches(Matcher)(arg.Replacements);
+}
+
+std::unique_ptr buildRefSlab(const Annotations &Code,
+  llvm::StringRef SymbolName,
+  llvm::StringRef Path) {
+  RefSlab::Builder Builder;
+  TestTU TU;
+  TU.HeaderCode = Code.code();
+  auto Symbols = TU.headerSymbols();
+  const auto &SymbolID = findSymbol(Symbols, SymbolName).ID;
+  for (const auto &Range : Code.ranges()) {
+Ref R;
+R.Kind = RefKind::Reference;
+R.Location.Start.setLine(Range.start.line);
+R.Location.Start.setColumn(Range.start.character);
+R.Location.End.setLine(Range.end.line);
+R.Location.End.setColumn(Range.end.character);
+auto U = URI::create(Path).toString();
+R.Location.FileURI = U.c_str();
+Builder.insert(SymbolID, R);
+  }
+
+  return std::make_unique(std::move(Builder).build());
+}
+
+std::vector> toVector(FileEdits FE) {
+  std::vector> Results;
+  for (auto &It : FE)
+Results.emplace_back(It.first().str(), std::move(It.getValue()));
+  return Results;
+}
 
 TEST(RenameTest, SingleFile) {
   struct Test {
@@ -80,10 +127,12 @@
 auto TU = TestTU::withCode(Code.code());
 auto AST = TU.build();
 auto RenameResult =
-renameWithinFile(AST, testPath(TU.Filename), Code.point(), "abcde");
+rename({Code.point(), "abcde", AST, testPath(TU.Filename)});
 ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
-auto ApplyResult =
-tooling::applyAllReplacements(Code.code(), *RenameResult);
+   

[PATCH] D69263: [clangd] Implement cross-file rename.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:370
+  for (auto &E : *Edits) {
+if (auto Err = reformatEdit(E.getValue(), Style))
+  elog("Failed to format replacements: {0}", std::move(Err));

hokein wrote:
> ilya-biryukov wrote:
> > Maybe use `llvm::joinErrors` to combine multiple failures into a single 
> > error?
> > Should simplify the code.
> I don't see using `llvm::joinErrors` can simplify the code here, `joinErrors` 
> accepts two `Error` objects, and there is no way to create an empty Error 
> object, we may end up with more code like:
> 
> ```
> llvm::Optional Err;
> for (auto &E : *Edits) {
>if (auto E = reformatEdit(E.getValue(), Style)) {
>   if (!Err) Err = std::move(E);
>   else Err = joinErrors(std::move(*Err), std::move(Err));
>}
> }
> ```
Works nicely if you start with `Error::success`
```
auto Err = Error::success();
for (...) {
  Err = llvm::joinErrors(reformatEdit(E.getValue(), Style));
}
if (Err)
  return CB(std::move(Err));
```



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:360
+ GetDirtyBuffer};
+auto Edits = clangd::rename(RInputs);
+if (!Edits)

NIT: inlining `RInputs` should save quite a few lines of code



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:91
+  if (!CrossFile) {
+// Within-file rename.
+auto &ASTCtx = RenameDecl.getASTContext();

NIT: this comment is redundant, previous line says the same thing



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:107
+if (!Index)
+  return NoIndexProvided;
+

Why isn't this a scope enum in the first place?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:128
+
+  // A whitelist of renamable symbols.
+  if (llvm::isa(RenameDecl))

Why not a blacklist? I'd expect us to blacklist:
- things with custom names (operators, constructors, etc)
- virtual methods

Probably some things are missing from the list, but fundamentally most that 
have names are pretty similar, right?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:132
+  if (const auto *S = llvm::dyn_cast(&RenameDecl)) {
+// FIXME: renaming virtual methods requires to rename all overridens in
+// subclasses, which our index doesn't support yet.

Isn't the same true for local rename?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:221
+  if (!Index)
+return {};
+  RefsRequest RQuest;

This behavior is very surprising... Obviously, returning empty results is 
incorrect and the callers have to handle the no-index case in a custom manner.

Maybe accept only non-null index and handle in the caller?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:235
+  });
+  return AffectedFiles;
+}

Again, it looks like the function returns incorrect results and the callers 
should actually handle the case where `SymbolID` cannot be obtained in a custom 
manner.

Maybe accept a `SymbolID` as a parameter instead of `NamedDecl*` and the 
callers handle this case gracefully?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:255
+// !positionToOffset is O(N), it is okay at the moment since we only
+// process at most 100 references.
+auto RangeOffset = toRangeOffset(Occurrence, InitialCode);

This is not true anymore, right?
We should probably try getting to `O(N)` to avoid slowing things down

Could be a FIXME for now, but have to fix it soon.



Comment at: clang-tools-extra/clangd/refactor/Rename.h:33
+  llvm::StringRef MainFilePath;
+  llvm::StringRef MainFileCode;
+

hokein wrote:
> ilya-biryukov wrote:
> > `MainFileCode` can be obtained from the AST. Why not do it?
> `ParsedAST` doesn't provide such API to get the file code, I assume you mean 
> `InputsAndAST`? The `MainFileCode` is from `InputsAndAST`.
`SourceManager` has access to the input buffer.
`StringRef MainFileCode = SM.getBufferData(SM.getMainFileID());`



Comment at: clang-tools-extra/clangd/refactor/Rename.h:40
+  // When set, used by the rename to get file content for all rename-related
+  // files (except the main file).
+  // If there is no corresponding dirty buffer, we will use the file content

hokein wrote:
> ilya-biryukov wrote:
> > Why is `MainFile` special? Can it also be handled with the GetDirtyBuffer?
> I believe we should use the content from `InputsAndAST`, which is 
> corresponding to the main AST (the content from `GetDiryBuffer` may not be 
> reflected to the AST). 
Does that mean we have three states of files that rename should be aware of?
- Dirty buffer
- Main file contents
- Contents on disk

That definitely looks very complicated... 

[PATCH] D69263: [clangd] Implement cross-file rename.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: pass - 59816 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:766
+  [File, Code, Params, Reply = std::move(Reply),
+   this](llvm::Expected Edits) mutable {
+if (!Edits)

ilya-biryukov wrote:
> NIT: is capture of `this` redundant here?
> 
> I could be missing something, though.
it is not redundant, we access the `this->DraftMgr` in the lambda body.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:334
+
 SourceLocation Loc = getBeginningOfIdentifier(
 Pos, AST.getSourceManager(), AST.getASTContext().getLangOpts());

ilya-biryukov wrote:
> Why not do this before running rename? This would allow to:
> 1. return result without doing the actual (expensive) rename if there's no 
> identifier under the cursor
> 2. reduce nesting of the (now large) large piece of code that runs rename:
> ```
> auto Loc = getBeginningOfIdentifier(...);
> auto Range = ...;
> if (!Range)
>   return llvm::None;
> if (!CrossFileRename)
>   return *Range; // FIXME: assume cross-file rename always succeeds
> // do the rename...
> ```
Good point.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:360
+RInputs.Index = Index;
+RInputs.VFS = FSProvider.getFileSystem();
+RInputs.GetDirtyBuffer = GetDirtyBuffer;

ilya-biryukov wrote:
> This can always be obtained from the AST, right?
> Why do we need it separately?
yes, obtained from the AST.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:370
+  for (auto &E : *Edits) {
+if (auto Err = reformatEdit(E.getValue(), Style))
+  elog("Failed to format replacements: {0}", std::move(Err));

ilya-biryukov wrote:
> Maybe use `llvm::joinErrors` to combine multiple failures into a single error?
> Should simplify the code.
I don't see using `llvm::joinErrors` can simplify the code here, `joinErrors` 
accepts two `Error` objects, and there is no way to create an empty Error 
object, we may end up with more code like:

```
llvm::Optional Err;
for (auto &E : *Edits) {
   if (auto E = reformatEdit(E.getValue(), Style)) {
  if (!Err) Err = std::move(E);
  else Err = joinErrors(std::move(*Err), std::move(Err));
   }
}
```



Comment at: clang-tools-extra/clangd/refactor/Rename.h:33
+  llvm::StringRef MainFilePath;
+  llvm::StringRef MainFileCode;
+

ilya-biryukov wrote:
> `MainFileCode` can be obtained from the AST. Why not do it?
`ParsedAST` doesn't provide such API to get the file code, I assume you mean 
`InputsAndAST`? The `MainFileCode` is from `InputsAndAST`.



Comment at: clang-tools-extra/clangd/refactor/Rename.h:40
+  // When set, used by the rename to get file content for all rename-related
+  // files (except the main file).
+  // If there is no corresponding dirty buffer, we will use the file content

ilya-biryukov wrote:
> Why is `MainFile` special? Can it also be handled with the GetDirtyBuffer?
I believe we should use the content from `InputsAndAST`, which is corresponding 
to the main AST (the content from `GetDiryBuffer` may not be reflected to the 
AST). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 227700.
hokein marked 13 inline comments as done.
hokein added a comment.

address comments:

- use VFS from AST;
- simplify the interfaces;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Rename.h
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h

Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -38,8 +38,8 @@
 llvm::Expected>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
 
-llvm::Expected>
-runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName);
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, StringRef NewName);
 
 std::string runDumpAST(ClangdServer &Server, PathRef File);
 
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -96,11 +96,11 @@
   return std::move(*Result);
 }
 
-llvm::Expected> runRename(ClangdServer &Server,
-PathRef File, Position Pos,
-llvm::StringRef NewName) {
-  llvm::Optional>> Result;
-  Server.rename(File, Pos, NewName, /*WantFormat=*/true, capture(Result));
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, llvm::StringRef NewName) {
+  llvm::Optional> Result;
+  Server.rename(File, Pos, NewName, /*WantFormat=*/true,
+/*DirtyBuffer=*/nullptr, capture(Result));
   return std::move(*Result);
 }
 
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -9,8 +9,10 @@
 #include "Annotations.h"
 #include "TestFS.h"
 #include "TestTU.h"
+#include "index/Ref.h"
 #include "refactor/Rename.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -18,9 +20,54 @@
 namespace clangd {
 namespace {
 
+using testing::Eq;
+using testing::Matches;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
 MATCHER_P2(RenameRange, Code, Range, "") {
   return replacementToEdit(Code, arg).range == Range;
 }
+MATCHER_P2(EqualFileEdit, Code, Ranges, "") {
+  using ReplacementMatcher = testing::Matcher;
+  std::vector Expected;
+  for (const auto &R : Ranges)
+Expected.push_back(RenameRange(Code, R));
+  auto Matcher =
+  testing::internal::UnorderedElementsAreArrayMatcher(
+  Expected.begin(), Expected.end());
+  return arg.InitialCode == Code && Matches(Matcher)(arg.Replacements);
+}
+
+std::unique_ptr buildRefSlab(const Annotations &Code,
+  llvm::StringRef SymbolName,
+  llvm::StringRef Path) {
+  RefSlab::Builder Builder;
+  TestTU TU;
+  TU.HeaderCode = Code.code();
+  auto Symbols = TU.headerSymbols();
+  const auto &SymbolID = findSymbol(Symbols, SymbolName).ID;
+  for (const auto &Range : Code.ranges()) {
+Ref R;
+R.Kind = RefKind::Reference;
+R.Location.Start.setLine(Range.start.line);
+R.Location.Start.setColumn(Range.start.character);
+R.Location.End.setLine(Range.end.line);
+R.Location.End.setColumn(Range.end.character);
+auto U = URI::create(Path).toString();
+R.Location.FileURI = U.c_str();
+Builder.insert(SymbolID, R);
+  }
+
+  return std::make_unique(std::move(Builder).build());
+}
+
+std::vector> toVector(FileEdits FE) {
+  std::vector> Results;
+  for (auto &It : FE)
+Results.emplace_back(It.first().str(), std::move(It.getValue()));
+  return Results;
+}
 
 TEST(RenameTest, SingleFile) {
   struct Test {
@@ -80,10 +127,12 @@
 auto TU = TestTU::withCode(Code.code());
 auto AST = TU.build();
 auto RenameResult =
-renameWithinFile(AST, testPath(TU.Filename), Code.point(), "abcde");
+rename({Code.point(), "abcde", AST, testPath(TU.Filename), Code.code()});
 ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
-auto ApplyResult =
-tooling::

[PATCH] D69263: [clangd] Implement cross-file rename.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:766
+  [File, Code, Params, Reply = std::move(Reply),
+   this](llvm::Expected Edits) mutable {
+if (!Edits)

NIT: is capture of `this` redundant here?

I could be missing something, though.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:334
+
 SourceLocation Loc = getBeginningOfIdentifier(
 Pos, AST.getSourceManager(), AST.getASTContext().getLangOpts());

Why not do this before running rename? This would allow to:
1. return result without doing the actual (expensive) rename if there's no 
identifier under the cursor
2. reduce nesting of the (now large) large piece of code that runs rename:
```
auto Loc = getBeginningOfIdentifier(...);
auto Range = ...;
if (!Range)
  return llvm::None;
if (!CrossFileRename)
  return *Range; // FIXME: assume cross-file rename always succeeds
// do the rename...
```



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:353
   return CB(InpAST.takeError());
-auto Changes = renameWithinFile(InpAST->AST, File, Pos, NewName, Index);
-if (!Changes)
-  return CB(Changes.takeError());
+RenameInputs RInputs;
+RInputs.MainFileCode = InpAST->Inputs.Contents;

Could we get a helper function (or constructor) to keep the call sites as 
simple as they used to be?
`RenameInputs{Contents, File, NewName, Pos,...}` should also work just fine and 
I don't think it hurts readability, since all the parameters here are named the 
same as the fields and most of them have different types.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:360
+RInputs.Index = Index;
+RInputs.VFS = FSProvider.getFileSystem();
+RInputs.GetDirtyBuffer = GetDirtyBuffer;

This can always be obtained from the AST, right?
Why do we need it separately?



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:370
+  for (auto &E : *Edits) {
+if (auto Err = reformatEdit(E.getValue(), Style))
+  elog("Failed to format replacements: {0}", std::move(Err));

Maybe use `llvm::joinErrors` to combine multiple failures into a single error?
Should simplify the code.



Comment at: clang-tools-extra/clangd/SourceCode.h:226
 };
+using FileEdits = llvm::StringMap;
 

We should document this typedef, otherwise it does not provide much value.
A doc comment from `ApplyEdits` should fit just nicely here.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:71
+  bool CrossFile) {
+  // Filter out symbols that are unsupported in both rename mode.
   if (llvm::isa(&RenameDecl))

NIT: in both rename **modes**



Comment at: clang-tools-extra/clangd/refactor/Rename.h:23
 
+using DirtyBufferGetter =
+std::function(PathRef Path)>;

ilya-biryukov wrote:
> Could you document what does it mean for this function to return `llvm::None`?
> I assume we read the contents from disk instead.
> 
> Also worth documenting what should be returned for `MainFilePath`? 
> `llvm::None`? same value as `MainFileCode`?
What does it return for `MainFilePath`? `llvm::None`? `MainFileCode`?



Comment at: clang-tools-extra/clangd/refactor/Rename.h:33
+  llvm::StringRef MainFilePath;
+  llvm::StringRef MainFileCode;
+

`MainFileCode` can be obtained from the AST. Why not do it?



Comment at: clang-tools-extra/clangd/refactor/Rename.h:40
+  // When set, used by the rename to get file content for all rename-related
+  // files (except the main file).
+  // If there is no corresponding dirty buffer, we will use the file content

Why is `MainFile` special? Can it also be handled with the GetDirtyBuffer?



Comment at: clang-tools-extra/clangd/refactor/Rename.h:49
+/// in another file (per the index).
+llvm::Expected rename(ParsedAST &AST, const RenameInputs &RInputs);
 

Why isn't `AST` part of the `RenameInputs`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: pass - 59816 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 227663.
hokein marked 4 inline comments as done.
hokein added a comment.

Add comments for DirtyBufferGetter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Rename.h
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h

Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -38,8 +38,8 @@
 llvm::Expected>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
 
-llvm::Expected>
-runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName);
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, StringRef NewName);
 
 std::string runDumpAST(ClangdServer &Server, PathRef File);
 
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -96,11 +96,11 @@
   return std::move(*Result);
 }
 
-llvm::Expected> runRename(ClangdServer &Server,
-PathRef File, Position Pos,
-llvm::StringRef NewName) {
-  llvm::Optional>> Result;
-  Server.rename(File, Pos, NewName, /*WantFormat=*/true, capture(Result));
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, llvm::StringRef NewName) {
+  llvm::Optional> Result;
+  Server.rename(File, Pos, NewName, /*WantFormat=*/true,
+/*DirtyBuffer=*/nullptr, capture(Result));
   return std::move(*Result);
 }
 
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -9,8 +9,10 @@
 #include "Annotations.h"
 #include "TestFS.h"
 #include "TestTU.h"
+#include "index/Ref.h"
 #include "refactor/Rename.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -18,9 +20,68 @@
 namespace clangd {
 namespace {
 
+using testing::Eq;
+using testing::Matches;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
 MATCHER_P2(RenameRange, Code, Range, "") {
   return replacementToEdit(Code, arg).range == Range;
 }
+MATCHER_P2(EqualFileEdit, Code, Ranges, "") {
+  using ReplacementMatcher = testing::Matcher;
+  std::vector Expected;
+  for (const auto &R : Ranges)
+Expected.push_back(RenameRange(Code, R));
+  auto Matcher =
+  testing::internal::UnorderedElementsAreArrayMatcher(
+  Expected.begin(), Expected.end());
+  return arg.InitialCode == Code && Matches(Matcher)(arg.Replacements);
+}
+
+std::unique_ptr buildRefSlab(const Annotations &Code,
+  llvm::StringRef SymbolName,
+  llvm::StringRef Path) {
+  RefSlab::Builder Builder;
+  TestTU TU;
+  TU.HeaderCode = Code.code();
+  auto Symbols = TU.headerSymbols();
+  const auto &SymbolID = findSymbol(Symbols, SymbolName).ID;
+  for (const auto &Range : Code.ranges()) {
+Ref R;
+R.Kind = RefKind::Reference;
+R.Location.Start.setLine(Range.start.line);
+R.Location.Start.setColumn(Range.start.character);
+R.Location.End.setLine(Range.end.line);
+R.Location.End.setColumn(Range.end.character);
+auto U = URI::create(Path).toString();
+R.Location.FileURI = U.c_str();
+Builder.insert(SymbolID, R);
+  }
+
+  return std::make_unique(std::move(Builder).build());
+}
+
+RenameInputs renameInputs(const Annotations &Code, llvm::StringRef NewName,
+  llvm::StringRef Path,
+  const SymbolIndex *Index = nullptr,
+  bool CrossFile = false) {
+  RenameInputs Inputs;
+  Inputs.Pos = Code.point();
+  Inputs.MainFileCode = Code.code();
+  Inputs.MainFilePath = Path;
+  Inputs.NewName = NewName;
+  Inputs.Index = Index;
+  Inputs.AllowCrossFile = CrossFile;
+  return Inputs;
+}
+
+std::vector> toVector(FileEdits FE) {
+  std::vector> Results;
+  for (auto &It : FE)
+Results.emplace_back(It.first().str(), std::move(It.getValue()));
+  return Res

[PATCH] D69263: [clangd] Implement cross-file rename.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:274
+llvm::Expected
+renameOutsideFile(const NamedDecl *RenameDecl, llvm::StringRef MainFilePath,
+  llvm::StringRef NewName, const SymbolIndex *Index,

hokein wrote:
> ilya-biryukov wrote:
> > NIT: Maybe call it `renameWithIndex` instead?
> > It should capture what this function is doing better...
> > 
> > But up to you
> I would keep the current name, as it is symmetrical to the `renameWithinFile`.
Yeah, those go together. Using `renameWithIndex` would mean the other one 
should be `renameWithAST` (or similar).
Feel free to keep as is too.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:216
+// Return all rename occurrences (per the index) outside of the main file,
+// grouped by the file name.
+llvm::StringMap>

NIT: grouped by the **absolute** file name? Or is there a better term than 
"absolute" to describe this?



Comment at: clang-tools-extra/clangd/refactor/Rename.h:23
 
+using DirtyBufferGetter =
+std::function(PathRef Path)>;

Could you document what does it mean for this function to return `llvm::None`?
I assume we read the contents from disk instead.

Also worth documenting what should be returned for `MainFilePath`? 
`llvm::None`? same value as `MainFileCode`?



Comment at: clang-tools-extra/clangd/refactor/Rename.h:24
+using DirtyBufferGetter =
+std::function(PathRef Path)>;
+

NIT: maybe use `function_ref`? We definitely don't store this function anywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: pass - 59816 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:219
+  // us whether the ref results is completed.
+  RQuest.Limit = 100;
+  if (auto ID = getSymbolID(RenameDecl))

ilya-biryukov wrote:
> Why do we need to limit the number of references in the first place?
I was thinking this is for performance, if the index returns too many results 
(the number of affected files >> our limit), we will ends up doing many 
unnecessary work (like resolving URIs).  Removed it now, it seems a premature 
optimization, we can revisit this afterwards.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:274
+llvm::Expected
+renameOutsideFile(const NamedDecl *RenameDecl, llvm::StringRef MainFilePath,
+  llvm::StringRef NewName, const SymbolIndex *Index,

ilya-biryukov wrote:
> NIT: Maybe call it `renameWithIndex` instead?
> It should capture what this function is doing better...
> 
> But up to you
I would keep the current name, as it is symmetrical to the `renameWithinFile`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 227278.
hokein marked 8 inline comments as done.
hokein added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Rename.h
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h

Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -38,8 +38,8 @@
 llvm::Expected>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
 
-llvm::Expected>
-runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName);
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, StringRef NewName);
 
 std::string runDumpAST(ClangdServer &Server, PathRef File);
 
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -96,11 +96,11 @@
   return std::move(*Result);
 }
 
-llvm::Expected> runRename(ClangdServer &Server,
-PathRef File, Position Pos,
-llvm::StringRef NewName) {
-  llvm::Optional>> Result;
-  Server.rename(File, Pos, NewName, /*WantFormat=*/true, capture(Result));
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, llvm::StringRef NewName) {
+  llvm::Optional> Result;
+  Server.rename(File, Pos, NewName, /*WantFormat=*/true,
+/*DirtyBuffer=*/nullptr, capture(Result));
   return std::move(*Result);
 }
 
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -9,8 +9,10 @@
 #include "Annotations.h"
 #include "TestFS.h"
 #include "TestTU.h"
+#include "index/Ref.h"
 #include "refactor/Rename.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -18,9 +20,68 @@
 namespace clangd {
 namespace {
 
+using testing::Eq;
+using testing::Matches;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
 MATCHER_P2(RenameRange, Code, Range, "") {
   return replacementToEdit(Code, arg).range == Range;
 }
+MATCHER_P2(EqualFileEdit, Code, Ranges, "") {
+  using ReplacementMatcher = testing::Matcher;
+  std::vector Expected;
+  for (const auto &R : Ranges)
+Expected.push_back(RenameRange(Code, R));
+  auto Matcher =
+  testing::internal::UnorderedElementsAreArrayMatcher(
+  Expected.begin(), Expected.end());
+  return arg.InitialCode == Code && Matches(Matcher)(arg.Replacements);
+}
+
+std::unique_ptr buildRefSlab(const Annotations &Code,
+  llvm::StringRef SymbolName,
+  llvm::StringRef Path) {
+  RefSlab::Builder Builder;
+  TestTU TU;
+  TU.HeaderCode = Code.code();
+  auto Symbols = TU.headerSymbols();
+  const auto &SymbolID = findSymbol(Symbols, SymbolName).ID;
+  for (const auto &Range : Code.ranges()) {
+Ref R;
+R.Kind = RefKind::Reference;
+R.Location.Start.setLine(Range.start.line);
+R.Location.Start.setColumn(Range.start.character);
+R.Location.End.setLine(Range.end.line);
+R.Location.End.setColumn(Range.end.character);
+auto U = URI::create(Path).toString();
+R.Location.FileURI = U.c_str();
+Builder.insert(SymbolID, R);
+  }
+
+  return std::make_unique(std::move(Builder).build());
+}
+
+RenameInputs renameInputs(const Annotations &Code, llvm::StringRef NewName,
+  llvm::StringRef Path,
+  const SymbolIndex *Index = nullptr,
+  bool CrossFile = false) {
+  RenameInputs Inputs;
+  Inputs.Pos = Code.point();
+  Inputs.MainFileCode = Code.code();
+  Inputs.MainFilePath = Path;
+  Inputs.NewName = NewName;
+  Inputs.Index = Index;
+  Inputs.AllowCrossFile = CrossFile;
+  return Inputs;
+}
+
+std::vector> toVector(FileEdits FE) {
+  std::vector> Results;
+  for (auto &It : FE)
+Results.emplace_back(It.first().str(), std::move(It.getValue()));
+  return Results;
+}
 
 TEST(R

[PATCH] D69263: [clangd] Implement cross-file rename.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks, mostly LG!

The only important comment I have left is about limiting the number of 
references. Others are NITs.




Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:219
+  // us whether the ref results is completed.
+  RQuest.Limit = 100;
+  if (auto ID = getSymbolID(RenameDecl))

Why do we need to limit the number of references in the first place?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:223
+
+  // Helper.
+  auto ToRange = [](const SymbolLocation &L) {

Maybe remove this comment?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:224
+  // Helper.
+  auto ToRange = [](const SymbolLocation &L) {
+Range R;

Why not extract this as a helper function?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:233
+  llvm::StringMap>
+  AffectedFiles; // absolute file path => rename ocurrences in that file.
+  Index->refs(RQuest, [&](const Ref &R) {

NIT: maybe put this comment on the line before the declaration?
Should lead to better formatting


Also, this comment would be most useful on the function declaration. Maybe move 
it there?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:274
+llvm::Expected
+renameOutsideFile(const NamedDecl *RenameDecl, llvm::StringRef MainFilePath,
+  llvm::StringRef NewName, const SymbolIndex *Index,

NIT: Maybe call it `renameWithIndex` instead?
It should capture what this function is doing better...

But up to you



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:289
+
+  // The cross-file rename is purely based on the index, as we don't want to
+  // build all ASTs for affected files, which may cause a performance hit.

Maybe move this comment to the function itself?
It definitely does a great job of capturing what this function is doing and 
what the trade-offs are.



Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:277
+  //   - file on VFS for bar.cc;
+  Annotations MainCode("class  [[Fo^o]] {};");
+  auto MainFilePath = testPath("main.cc");

hokein wrote:
> ilya-biryukov wrote:
> > Thinking whether we can encode these transformations in a nicer way?
> > If we could convert the contents dirty buffer and replacements to something 
> > like:
> > ```
> > class [[Foo |-> newName]] {};
> > ```
> > Just a proposal, maybe there's a better syntax here.
> > 
> > We can easily match strings instead of matching ranges in the tests. This 
> > has the advantage of having textual diffs in case something goes wrong - 
> > much more pleasant than looking at the offsets in the ranges.
> > 
> > WDYT?
> I agree textual diff would give a more friendly results when there are 
> failures in the tests.
> 
> I don't have a better way to encode the transformation in the annotation 
> code, I think a better way maybe to use a hard-coded new name, and applying 
> the actual replacements on the testing code, and verify the the text diffs.
> 
> If we want to do this change, I'd prefer to do it in a followup, since it 
> would change the existing testcases as well. What do you think?
> 
Maybe I'm overthinking it... For rename having just the range is probably 
enough.



Comment at: clang-tools-extra/clangd/unittests/SyncAPI.cpp:103
+  Server.rename(File, Pos, NewName, /*WantFormat=*/true,
+/*DirtyBuffaer*/ nullptr, capture(Result));
   return std::move(*Result);

s/DirtyBuffaer/DirtyBuffer

also use `/*DirtryBuffer=*/` to be consistent with `/*WantFormat=*/` from the 
previous line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: pass - 59701 tests passed, 0 failed and 763 were skipped.
Log files: cmake-log.txt 
, 
ninja_check_all-log.txt 
, 
CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 226641.
hokein marked 3 inline comments as done.
hokein added a comment.

address comments:

- make the command-line flag hidden;
- use the gtesting matcher;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Rename.h
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h

Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -38,8 +38,8 @@
 llvm::Expected>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
 
-llvm::Expected>
-runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName);
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, StringRef NewName);
 
 std::string runDumpAST(ClangdServer &Server, PathRef File);
 
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -96,11 +96,11 @@
   return std::move(*Result);
 }
 
-llvm::Expected> runRename(ClangdServer &Server,
-PathRef File, Position Pos,
-llvm::StringRef NewName) {
-  llvm::Optional>> Result;
-  Server.rename(File, Pos, NewName, /*WantFormat=*/true, capture(Result));
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, llvm::StringRef NewName) {
+  llvm::Optional> Result;
+  Server.rename(File, Pos, NewName, /*WantFormat=*/true,
+/*DirtyBuffaer*/ nullptr, capture(Result));
   return std::move(*Result);
 }
 
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -9,8 +9,10 @@
 #include "Annotations.h"
 #include "TestFS.h"
 #include "TestTU.h"
+#include "index/Ref.h"
 #include "refactor/Rename.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -18,9 +20,68 @@
 namespace clangd {
 namespace {
 
+using testing::Eq;
+using testing::Matches;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
 MATCHER_P2(RenameRange, Code, Range, "") {
   return replacementToEdit(Code, arg).range == Range;
 }
+MATCHER_P2(EqualFileEdit, Code, Ranges, "") {
+  using ReplacementMatcher = testing::Matcher;
+  std::vector Expected;
+  for (const auto &R : Ranges)
+Expected.push_back(RenameRange(Code, R));
+  auto Matcher =
+  testing::internal::UnorderedElementsAreArrayMatcher(
+  Expected.begin(), Expected.end());
+  return arg.InitialCode == Code && Matches(Matcher)(arg.Replacements);
+}
+
+std::unique_ptr buildRefSlab(const Annotations &Code,
+  llvm::StringRef SymbolName,
+  llvm::StringRef Path) {
+  RefSlab::Builder Builder;
+  TestTU TU;
+  TU.HeaderCode = Code.code();
+  auto Symbols = TU.headerSymbols();
+  const auto &SymbolID = findSymbol(Symbols, SymbolName).ID;
+  for (const auto &Range : Code.ranges()) {
+Ref R;
+R.Kind = RefKind::Reference;
+R.Location.Start.setLine(Range.start.line);
+R.Location.Start.setColumn(Range.start.character);
+R.Location.End.setLine(Range.end.line);
+R.Location.End.setColumn(Range.end.character);
+auto U = URI::create(Path).toString();
+R.Location.FileURI = U.c_str();
+Builder.insert(SymbolID, R);
+  }
+
+  return std::make_unique(std::move(Builder).build());
+}
+
+RenameInputs renameInputs(const Annotations &Code, llvm::StringRef NewName,
+  llvm::StringRef Path,
+  const SymbolIndex *Index = nullptr,
+  bool CrossFile = false) {
+  RenameInputs Inputs;
+  Inputs.Pos = Code.point();
+  Inputs.MainFileCode = Code.code();
+  Inputs.MainFilePath = Path;
+  Inputs.NewName = NewName;
+  Inputs.Index = Index;
+  Inputs.AllowCrossFile = CrossFile;
+  return Inputs;
+}
+
+std::vector> toVector(FileEdits FE) {
+  std::vector> Results;
+  for (auto &It : FE)
+Results.emplace_back(It.first(

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:269
+cat(Features),
+desc("Enable cross-file rename feature."),
+init(false),

ilya-biryukov wrote:
> Could you document that the feature is highly experimental and may lead to 
> broken code or incomplete renames for the time being?
> 
> Also, maybe make it hidden for now? 
> At least until we have basic validation of ranges from the index. In the 
> current state we can easily produce edits to unrelated parts of the file...
Done.  yes, we should make it hidden.



Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:277
+  //   - file on VFS for bar.cc;
+  Annotations MainCode("class  [[Fo^o]] {};");
+  auto MainFilePath = testPath("main.cc");

ilya-biryukov wrote:
> Thinking whether we can encode these transformations in a nicer way?
> If we could convert the contents dirty buffer and replacements to something 
> like:
> ```
> class [[Foo |-> newName]] {};
> ```
> Just a proposal, maybe there's a better syntax here.
> 
> We can easily match strings instead of matching ranges in the tests. This has 
> the advantage of having textual diffs in case something goes wrong - much 
> more pleasant than looking at the offsets in the ranges.
> 
> WDYT?
I agree textual diff would give a more friendly results when there are failures 
in the tests.

I don't have a better way to encode the transformation in the annotation code, 
I think a better way maybe to use a hard-coded new name, and applying the 
actual replacements on the testing code, and verify the the text diffs.

If we want to do this change, I'd prefer to do it in a followup, since it would 
change the existing testcases as well. What do you think?




Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:302
+  testing::UnorderedElementsAre(
+  PairMatcher(Eq(FooPath),
+  EqualFileEdit(FooDirtyBuffer, FooCode.ranges())),

ilya-biryukov wrote:
> Instead of defining custom matchers, could we convert to standard types and 
> use existing gtest matchers?
> Something like `std::vector>` should 
> work just fine.
> 
> Huge advantage we'll get is better output in case of errors.
yeah, converting the `llvm::StringMap` to standard types could work, but we 
have to pay the cost of transformation (that was something I tried to avoid).
 I think there is no strong reason to use llvm::StringMap as the `FileEdits`, 
what do you think if we change the `FileEdits` type to 
`std::vector>`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:269
+cat(Features),
+desc("Enable cross-file rename feature."),
+init(false),

Could you document that the feature is highly experimental and may lead to 
broken code or incomplete renames for the time being?

Also, maybe make it hidden for now? 
At least until we have basic validation of ranges from the index. In the 
current state we can easily produce edits to unrelated parts of the file...



Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:277
+  //   - file on VFS for bar.cc;
+  Annotations MainCode("class  [[Fo^o]] {};");
+  auto MainFilePath = testPath("main.cc");

Thinking whether we can encode these transformations in a nicer way?
If we could convert the contents dirty buffer and replacements to something 
like:
```
class [[Foo |-> newName]] {};
```
Just a proposal, maybe there's a better syntax here.

We can easily match strings instead of matching ranges in the tests. This has 
the advantage of having textual diffs in case something goes wrong - much more 
pleasant than looking at the offsets in the ranges.

WDYT?



Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:302
+  testing::UnorderedElementsAre(
+  PairMatcher(Eq(FooPath),
+  EqualFileEdit(FooDirtyBuffer, FooCode.ranges())),

Instead of defining custom matchers, could we convert to standard types and use 
existing gtest matchers?
Something like `std::vector>` should work 
just fine.

Huge advantage we'll get is better output in case of errors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: fail - 59520 tests passed, 1 failed and 763 were skipped.

  failed: Clangd.Clangd/rename.test

Log files: cmake-log.txt 
, 
ninja_check_all-log.txt 
, 
CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D69263#1718525 , @ilya-biryukov 
wrote:

> Not sure that holds. What if the file in question is being rebuilt right now? 
> We do not wait until all ASTs are built (and the dynamic index gets the new 
> results).
>  Open files actually pose an interesting problem: their contents do not 
> correspond to the file contents on disk.
>  We should choose which of those we update on rename: dirty buffers or file 
> contents? (I believe we should do dirty buffers, but I believe @sammccall has 
> the opposite opinion, so we should sync on this)


Based on the offline discussion, we decide to use dirty buffers for opened 
files.




Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:121
+renameableOutsideFile(const NamedDecl &RenameDecl, const SymbolIndex *Index) {
+  if (RenameDecl.getParentFunctionOrMethod())
+return None; // function-local symbols

ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > This function resembles `renamableWithinFile`.
> > > We should probably share implementation and pass an extra flag. Something 
> > > like `isRenamable(..., bool IsCrossFile)`.
> > > 
> > > WDYT?
> > though they look similar,  the logic is quite different, for example, we 
> > query the index in within-file rename to detect a symbol is used outside of 
> > the file which is not needed for cross-file rename, and cross-file rename 
> > allows fewer symbols (as our index doesn't support them), so I don't think 
> > we can share a lot of implementation.
> Can this be handled with a special flag:
> ```
> bool renameable(NamedDecl &Decl, const SymbolIndex *Index, bool CrossFile) {
>   if (CrossFileRename) {
> // check something special...
>   }
> }
> ```
> 
> Sharing implementation in the same function makes the differences between 
> cross-file and single-file case obvious and also ensures any things that need 
> to be updated for both are shared.
Done. I tried my best to unify them, please take a look on the new code.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:317
+  std::string OldName = RenameDecl->getNameAsString();
+  for (const auto &FileAndOccurrences : AffectedFiles) {
+llvm::StringRef FilePath = FileAndOccurrences.first();

ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > I feel this code is fundamental to the trade-offs we made for rename.
> > > Could we move this to a separate function and add unit-tests for it?
> > > 
> > > You probably want to have something that handles just a single file 
> > > rather than all edits in all files, to avoid mocking VFS in tests, etc.
> > > 
> > > 
> > Agree, especially when we start implementing complicated stuff, e.g. range 
> > patching heuristics.
> > 
> >  Moved it out, but note that I don't plan to add more stuff in this initial 
> > patch, so the logic is pretty straightforward, just assembling rename 
> > replacement from occurrences.
> > 
> > but note that I don't plan to add more stuff in this initial patch
> Should we also check whether the replaced text matches the expected 
> identifier and add unit-tests for this?
> Or would you prefer to move all changes to this function to a separate patch?
I prefer to do it afterwards as the patch is relatively big now.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:380
+auto MainFileRenameEdit =
+renameWithinFile(AST, RenameDecl, RInputs.NewName);
+if (!MainFileRenameEdit)

ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Can we rely on the fact that dynamic index should not be stale for the 
> > > current file instead?
> > > Or don't we have this invariant?
> > > 
> > > We end up having two implementations now: index-based and AST-based. It'd 
> > > be nice to have just one.
> > > If that's not possible in the first revision, could you explain why?
> > > 
> > > Note that moving the current-file rename to index-based implementation is 
> > > independent of doing cross-file renames. Maybe we should start with it?
> > I think we can't get rid of the AST-based rename -- mainly for renaming 
> > local symbols (e.g. local variable in function body), as we don't index 
> > these symbols...
> Thanks, I believe you're right... We can't unify these implementations, at 
> least not in the short term.
> 
> So `renameOutsideFile` fails on local symbols and `renameWithinFile` should 
> handle that, right?
> Also worth noting that `renameWithinFile` is AST-based and 
> `renameOutsideFile` is index-based.
> Could we document that?
yes, exactly. I have simplified the code in this function, and added 
documentations.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:357
+// Handle within-file rename.
+auto Reject = renamableWithinFile(*RenameDecl->getCanonicalDecl(),
+  RInputs.MainFilePath, RInpu

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 226414.
hokein marked 5 inline comments as done.
hokein added a comment.

- simplify the code, addressing comments;
- add unittests;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Rename.h
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h

Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -38,8 +38,8 @@
 llvm::Expected>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
 
-llvm::Expected>
-runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName);
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, StringRef NewName);
 
 std::string runDumpAST(ClangdServer &Server, PathRef File);
 
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -96,11 +96,11 @@
   return std::move(*Result);
 }
 
-llvm::Expected> runRename(ClangdServer &Server,
-PathRef File, Position Pos,
-llvm::StringRef NewName) {
-  llvm::Optional>> Result;
-  Server.rename(File, Pos, NewName, /*WantFormat=*/true, capture(Result));
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, llvm::StringRef NewName) {
+  llvm::Optional> Result;
+  Server.rename(File, Pos, NewName, /*WantFormat=*/true,
+/*DirtyBuffaer*/ nullptr, capture(Result));
   return std::move(*Result);
 }
 
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -9,8 +9,10 @@
 #include "Annotations.h"
 #include "TestFS.h"
 #include "TestTU.h"
+#include "index/Ref.h"
 #include "refactor/Rename.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -18,9 +20,61 @@
 namespace clangd {
 namespace {
 
+using testing::Eq;
+using testing::Matches;
+MATCHER_P2(PairMatcher, KeyMatcher, ValueMatcher, "") {
+  return Matches(KeyMatcher)(arg.first()) && Matches(ValueMatcher)(arg.second);
+}
 MATCHER_P2(RenameRange, Code, Range, "") {
   return replacementToEdit(Code, arg).range == Range;
 }
+MATCHER_P2(EqualFileEdit, Code, Ranges, "") {
+  using ReplacementMatcher = testing::Matcher;
+  std::vector Expected;
+  for (const auto &R : Ranges)
+Expected.push_back(RenameRange(Code, R));
+  auto Matcher =
+  testing::internal::UnorderedElementsAreArrayMatcher(
+  Expected.begin(), Expected.end());
+  return arg.InitialCode == Code && Matches(Matcher)(arg.Replacements);
+}
+
+std::unique_ptr buildRefSlab(const Annotations &Code,
+  llvm::StringRef SymbolName,
+  llvm::StringRef Path) {
+  RefSlab::Builder Builder;
+  TestTU TU;
+  TU.HeaderCode = Code.code();
+  auto Symbols = TU.headerSymbols();
+  const auto &SymbolID = findSymbol(Symbols, SymbolName).ID;
+  for (const auto &Range : Code.ranges()) {
+Ref R;
+R.Kind = RefKind::Reference;
+R.Location.Start.setLine(Range.start.line);
+R.Location.Start.setColumn(Range.start.character);
+R.Location.End.setLine(Range.end.line);
+R.Location.End.setColumn(Range.end.character);
+auto U = URI::create(Path).toString();
+R.Location.FileURI = U.c_str();
+Builder.insert(SymbolID, R);
+  }
+
+  return std::make_unique(std::move(Builder).build());
+}
+
+RenameInputs renameInputs(const Annotations &Code, llvm::StringRef NewName,
+  llvm::StringRef Path,
+  const SymbolIndex *Index = nullptr,
+  bool CrossFile = false) {
+  RenameInputs Inputs;
+  Inputs.Pos = Code.point();
+  Inputs.MainFileCode = Code.code();
+  Inputs.MainFilePath = Path;
+  Inputs.NewName = NewName;
+  Inputs.Index = Index;
+  Inputs.AllowCrossFile = CrossFile;
+  return Inputs;
+}
 
 TEST(RenameTes

[PATCH] D69263: [clangd] Implement cross-file rename.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:119
 
+llvm::Optional
+renameableOutsideFile(const NamedDecl &RenameDecl, const SymbolIndex *Index) {

hokein wrote:
> ilya-biryukov wrote:
> > So `llvm::None` means we do not reject?
> > Probably worth documenting that.
> > 
> > Maybe even add an enumerator `ReasonToReject::Accept`, indicating the 
> > symbol should be accepted and get rid of `Optional` altogether.
> > 
> > Otherwise this leads to code like:
> > ```
> > auto R = renameableOutsideFile(N, I);
> > if (!R) { // negative condition.
> >  return doRename(N, I); // But we're running the rename anyway... WAT?
> > }
> > ``
> yeah, returning `None` means that we accept the rename.
> 
> Adding `Accept` is not aligning with the mental model, `Accept` doesn't 
> belong to `ReasonToReject` I think. I agree the above given code is 
> misleading, but looking at the current code in this file, it doesn't seem too 
> bad, I think?
> 
> ```
>  if (auto Reject = renameableOutsideFile(*RenameDecl, RInputs.Index))
> return makeError(*Reject);
> ```
Agreed this aligns with the other functions we have in the file, so let's keep 
as is.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:121
+renameableOutsideFile(const NamedDecl &RenameDecl, const SymbolIndex *Index) {
+  if (RenameDecl.getParentFunctionOrMethod())
+return None; // function-local symbols

hokein wrote:
> ilya-biryukov wrote:
> > This function resembles `renamableWithinFile`.
> > We should probably share implementation and pass an extra flag. Something 
> > like `isRenamable(..., bool IsCrossFile)`.
> > 
> > WDYT?
> though they look similar,  the logic is quite different, for example, we 
> query the index in within-file rename to detect a symbol is used outside of 
> the file which is not needed for cross-file rename, and cross-file rename 
> allows fewer symbols (as our index doesn't support them), so I don't think we 
> can share a lot of implementation.
Can this be handled with a special flag:
```
bool renameable(NamedDecl &Decl, const SymbolIndex *Index, bool CrossFile) {
  if (CrossFileRename) {
// check something special...
  }
}
```

Sharing implementation in the same function makes the differences between 
cross-file and single-file case obvious and also ensures any things that need 
to be updated for both are shared.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:317
+  std::string OldName = RenameDecl->getNameAsString();
+  for (const auto &FileAndOccurrences : AffectedFiles) {
+llvm::StringRef FilePath = FileAndOccurrences.first();

hokein wrote:
> ilya-biryukov wrote:
> > I feel this code is fundamental to the trade-offs we made for rename.
> > Could we move this to a separate function and add unit-tests for it?
> > 
> > You probably want to have something that handles just a single file rather 
> > than all edits in all files, to avoid mocking VFS in tests, etc.
> > 
> > 
> Agree, especially when we start implementing complicated stuff, e.g. range 
> patching heuristics.
> 
>  Moved it out, but note that I don't plan to add more stuff in this initial 
> patch, so the logic is pretty straightforward, just assembling rename 
> replacement from occurrences.
> 
> but note that I don't plan to add more stuff in this initial patch
Should we also check whether the replaced text matches the expected identifier 
and add unit-tests for this?
Or would you prefer to move all changes to this function to a separate patch?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:380
+auto MainFileRenameEdit =
+renameWithinFile(AST, RenameDecl, RInputs.NewName);
+if (!MainFileRenameEdit)

hokein wrote:
> ilya-biryukov wrote:
> > Can we rely on the fact that dynamic index should not be stale for the 
> > current file instead?
> > Or don't we have this invariant?
> > 
> > We end up having two implementations now: index-based and AST-based. It'd 
> > be nice to have just one.
> > If that's not possible in the first revision, could you explain why?
> > 
> > Note that moving the current-file rename to index-based implementation is 
> > independent of doing cross-file renames. Maybe we should start with it?
> I think we can't get rid of the AST-based rename -- mainly for renaming local 
> symbols (e.g. local variable in function body), as we don't index these 
> symbols...
Thanks, I believe you're right... We can't unify these implementations, at 
least not in the short term.

So `renameOutsideFile` fails on local symbols and `renameWithinFile` should 
handle that, right?
Also worth noting that `renameWithinFile` is AST-based and `renameOutsideFile` 
is index-based.
Could we document that?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:357
+// Handle within-file rename.
+auto Reject = renamable

[PATCH] D69263: [clangd] Implement cross-file rename.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: fail - 59518 tests passed, 1 failed and 763 were skipped.

  failed: Clangd.Clangd/rename.test

Log files: cmake-log.txt 
, 
ninja_check_all-log.txt 
, 
CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: fail - 59475 tests passed, 2 failed and 805 were skipped.

  failed: Clangd.Clangd/rename.test
  failed: LLVM.tools/llvm-ar/mri-utf8.test

Log files: cmake-log.txt 
, 
ninja_check_all-log.txt 
, 
CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 226137.
hokein marked 13 inline comments as done.
hokein added a comment.

- address comments;
- add more FIXMEs;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Rename.h
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h

Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -38,8 +38,8 @@
 llvm::Expected>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
 
-llvm::Expected>
-runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName);
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, StringRef NewName);
 
 std::string runDumpAST(ClangdServer &Server, PathRef File);
 
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -96,10 +96,9 @@
   return std::move(*Result);
 }
 
-llvm::Expected> runRename(ClangdServer &Server,
-PathRef File, Position Pos,
-llvm::StringRef NewName) {
-  llvm::Optional>> Result;
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, llvm::StringRef NewName) {
+  llvm::Optional> Result;
   Server.rename(File, Pos, NewName, /*WantFormat=*/true, capture(Result));
   return std::move(*Result);
 }
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -22,6 +22,19 @@
   return replacementToEdit(Code, arg).range == Range;
 }
 
+RenameInputs localRenameInputs(const Annotations &Code, llvm::StringRef NewName,
+   llvm::StringRef Path,
+   const SymbolIndex *Index = nullptr) {
+  RenameInputs Inputs;
+  Inputs.Pos = Code.point();
+  Inputs.MainFileCode = Code.code();
+  Inputs.MainFilePath = Path;
+  Inputs.NewName = NewName;
+  Inputs.Index = Index;
+  Inputs.AllowCrossFile = false;
+  return Inputs;
+}
+
 TEST(RenameTest, SingleFile) {
   struct Test {
 const char* Before;
@@ -80,10 +93,13 @@
 auto TU = TestTU::withCode(Code.code());
 auto AST = TU.build();
 auto RenameResult =
-renameWithinFile(AST, testPath(TU.Filename), Code.point(), "abcde");
+rename(AST, localRenameInputs(Code, "abcde", testPath(TU.Filename)));
+
 ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
-auto ApplyResult =
-tooling::applyAllReplacements(Code.code(), *RenameResult);
+ASSERT_TRUE(RenameResult->size() == 1);
+EXPECT_EQ(Code.code(), RenameResult->begin()->second.InitialCode);
+auto ApplyResult = tooling::applyAllReplacements(
+Code.code(), RenameResult->begin()->second.Replacements);
 ASSERT_TRUE(bool(ApplyResult)) << ApplyResult.takeError();
 
 EXPECT_EQ(T.After, *ApplyResult) << T.Before;
@@ -177,24 +193,27 @@
 }
 auto AST = TU.build();
 
-auto Results = renameWithinFile(AST, testPath(TU.Filename), T.point(),
-"dummyNewName", Case.Index);
+auto Results =
+rename(AST, localRenameInputs(T, "dummyNewName", testPath(TU.Filename),
+  Case.Index));
 bool WantRename = true;
 if (T.ranges().empty())
   WantRename = false;
 if (!WantRename) {
   assert(Case.ErrorMessage && "Error message must be set!");
-  EXPECT_FALSE(Results) << "expected renameWithinFile returned an error: "
-<< T.code();
+  EXPECT_FALSE(Results)
+  << "expected rename returned an error: " << T.code();
   auto ActualMessage = llvm::toString(Results.takeError());
   EXPECT_THAT(ActualMessage, testing::HasSubstr(Case.ErrorMessage));
 } else {
-  EXPECT_TRUE(bool(Results)) << "renameWithinFile returned an error: "
+  EXPECT_TRUE(bool(Results)) << "rename returned an error: "
  << llvm::toString(Results.takeErr

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D69263#1718525 , @ilya-biryukov 
wrote:

> In D69263#1717985 , @hokein wrote:
>
> > Thinking more about this -- we have a dynamic index (for all opened files) 
> > which is overlaid on a static index (which is a background index in 
> > open-source world), so for all affected files, they are either in
> >
> > 1. an open state -- we can rely on the dynamic index, I think it is safe to 
> > assume that index always returns up-to-date results;
>
>
> Not sure that holds. What if the file in question is being rebuilt right now? 
> We do not wait until all ASTs are built (and the dynamic index gets the new 
> results).


I'm not sure this would happen quite often in practice (probably depends on 
users' behavior). but yes, patching ranges would mitigate this issue.

> Open files actually pose an interesting problem: their contents do not 
> correspond to the file contents on disk.
>  We should choose which of those we update on rename: dirty buffers or file 
> contents? (I believe we should do dirty buffers, but I believe @sammccall has 
> the opposite opinion, so we should sync on this)

I have the same feeling of using dirty buffers for open files, let's discuss 
offline.

>> 2. a non-open state -- rely on the background index, however background 
>> index has more chance to be stale (especially we don't detect file-change 
>> events at the moment), we could do a range patch heuristically to mitigate 
>> this stale issue. Failing that, we surface the error to users.
> 
> To summarize, I think files can be stale in both cases and we should patch 
> the ranges in both cases.






Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:268
-  // decide to implement renaming with index support.
-  if ((Roles & static_cast(index::SymbolRole::NameReference)))
-return true;

ilya-biryukov wrote:
> Not sure what was filtered out here.
> I suggest moving this to a separate change, with unit-tests clearly 
> describing the new symbols we do not filter out anymore and an explanation 
> why it's necessary.
> 
> WDYT?
Agreed. split it out.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:119
 
+llvm::Optional
+renameableOutsideFile(const NamedDecl &RenameDecl, const SymbolIndex *Index) {

ilya-biryukov wrote:
> So `llvm::None` means we do not reject?
> Probably worth documenting that.
> 
> Maybe even add an enumerator `ReasonToReject::Accept`, indicating the symbol 
> should be accepted and get rid of `Optional` altogether.
> 
> Otherwise this leads to code like:
> ```
> auto R = renameableOutsideFile(N, I);
> if (!R) { // negative condition.
>  return doRename(N, I); // But we're running the rename anyway... WAT?
> }
> ``
yeah, returning `None` means that we accept the rename.

Adding `Accept` is not aligning with the mental model, `Accept` doesn't belong 
to `ReasonToReject` I think. I agree the above given code is misleading, but 
looking at the current code in this file, it doesn't seem too bad, I think?

```
 if (auto Reject = renameableOutsideFile(*RenameDecl, RInputs.Index))
return makeError(*Reject);
```



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:121
+renameableOutsideFile(const NamedDecl &RenameDecl, const SymbolIndex *Index) {
+  if (RenameDecl.getParentFunctionOrMethod())
+return None; // function-local symbols

ilya-biryukov wrote:
> This function resembles `renamableWithinFile`.
> We should probably share implementation and pass an extra flag. Something 
> like `isRenamable(..., bool IsCrossFile)`.
> 
> WDYT?
though they look similar,  the logic is quite different, for example, we query 
the index in within-file rename to detect a symbol is used outside of the file 
which is not needed for cross-file rename, and cross-file rename allows fewer 
symbols (as our index doesn't support them), so I don't think we can share a 
lot of implementation.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:317
+  std::string OldName = RenameDecl->getNameAsString();
+  for (const auto &FileAndOccurrences : AffectedFiles) {
+llvm::StringRef FilePath = FileAndOccurrences.first();

ilya-biryukov wrote:
> I feel this code is fundamental to the trade-offs we made for rename.
> Could we move this to a separate function and add unit-tests for it?
> 
> You probably want to have something that handles just a single file rather 
> than all edits in all files, to avoid mocking VFS in tests, etc.
> 
> 
Agree, especially when we start implementing complicated stuff, e.g. range 
patching heuristics.

 Moved it out, but note that I don't plan to add more stuff in this initial 
patch, so the logic is pretty straightforward, just assembling rename 
replacement from occurrences.




Comment at: clang-tools-extra/clangd/refactor/Renam

[PATCH] D69263: [clangd] Implement cross-file rename.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D69263#1717985 , @hokein wrote:

> Thinking more about this -- we have a dynamic index (for all opened files) 
> which is overlaid on a static index (which is a background index in 
> open-source world), so for all affected files, they are either in
>
> 1. an open state -- we can rely on the dynamic index, I think it is safe to 
> assume that index always returns up-to-date results;


Not sure that holds. What if the file in question is being rebuilt right now? 
We do not wait until all ASTs are built (and the dynamic index gets the new 
results).
Open files actually pose an interesting problem: their contents do not 
correspond to the file contents on disk.
We should choose which of those we update on rename: dirty buffers or file 
contents? (I believe we should do dirty buffers, but I believe @sammccall has 
the opposite opinion, so we should sync on this)

> 2. a non-open state -- rely on the background index, however background index 
> has more chance to be stale (especially we don't detect file-change events at 
> the moment), we could do a range patch heuristically to mitigate this stale 
> issue. Failing that, we surface the error to users.

To summarize, I think files can be stale in both cases and we should patch the 
ranges in both cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment as done.
hokein added a comment.

Thanks for the comments.

In D69263#1716760 , @ilya-biryukov 
wrote:

> Another important concern is surfacing errors to the users: silently dropping 
> ranges for stale files is definitely not the nicest option, I'm afraid this 
> will lead to non-explainable failure modes and users will be incredibly 
> unhappy...


Agree, I think we should surface this error to users when the index is stale or 
we don't have enough confident to perform the rename.

Thinking more about this -- we have a dynamic index (for all opened files) 
which is overlaid on a static index (which is a background index in open-source 
world), so for all affected files, they are either in

1. an open state -- we can rely on the dynamic index, I think it is safe to 
assume that index always returns up-to-date results;
2. a non-open state -- rely on the background index, however background index 
has more chance to be stale (especially we don't detect file-change events at 
the moment), we could do a range patch heuristically to mitigate this stale 
issue. Failing that, we surface the error to users.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks for implementing this.
I think we could split it into multiple changes to make understanding it 
easier, see inline comments, I've tried to point out the places I find relevant.

Would definitely be nice to have some unit-tests for this.

Another important concern is surfacing errors to the users: silently dropping 
ranges for stale files is definitely not the nicest option, I'm afraid this 
will lead to non-explainable failure modes and users will be incredibly 
unhappy...




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:106
 
 // Makes sure edits in \p E are applicable to latest file contents reported by
 // editor. If not generates an error message containing information about files

NIT: rename to `FE`



Comment at: clang-tools-extra/clangd/ClangdServer.h:333
 
+  // If this is true, enable cross-file rename.
+  bool CrossFileRename = false;

NIT: this comment is redundant



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:268
-  // decide to implement renaming with index support.
-  if ((Roles & static_cast(index::SymbolRole::NameReference)))
-return true;

Not sure what was filtered out here.
I suggest moving this to a separate change, with unit-tests clearly describing 
the new symbols we do not filter out anymore and an explanation why it's 
necessary.

WDYT?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:119
 
+llvm::Optional
+renameableOutsideFile(const NamedDecl &RenameDecl, const SymbolIndex *Index) {

So `llvm::None` means we do not reject?
Probably worth documenting that.

Maybe even add an enumerator `ReasonToReject::Accept`, indicating the symbol 
should be accepted and get rid of `Optional` altogether.

Otherwise this leads to code like:
```
auto R = renameableOutsideFile(N, I);
if (!R) { // negative condition.
 return doRename(N, I); // But we're running the rename anyway... WAT?
}
``



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:121
+renameableOutsideFile(const NamedDecl &RenameDecl, const SymbolIndex *Index) {
+  if (RenameDecl.getParentFunctionOrMethod())
+return None; // function-local symbols

This function resembles `renamableWithinFile`.
We should probably share implementation and pass an extra flag. Something like 
`isRenamable(..., bool IsCrossFile)`.

WDYT?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:152
+return None;
+  else if (const auto *S = llvm::dyn_cast(&RenameDecl)) {
+// FIXME: renaming virtual methods requires to rename all overridens in

NIT: `else` is redundant



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:317
+  std::string OldName = RenameDecl->getNameAsString();
+  for (const auto &FileAndOccurrences : AffectedFiles) {
+llvm::StringRef FilePath = FileAndOccurrences.first();

I feel this code is fundamental to the trade-offs we made for rename.
Could we move this to a separate function and add unit-tests for it?

You probably want to have something that handles just a single file rather than 
all edits in all files, to avoid mocking VFS in tests, etc.





Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:380
+auto MainFileRenameEdit =
+renameWithinFile(AST, RenameDecl, RInputs.NewName);
+if (!MainFileRenameEdit)

Can we rely on the fact that dynamic index should not be stale for the current 
file instead?
Or don't we have this invariant?

We end up having two implementations now: index-based and AST-based. It'd be 
nice to have just one.
If that's not possible in the first revision, could you explain why?

Note that moving the current-file rename to index-based implementation is 
independent of doing cross-file renames. Maybe we should start with it?



Comment at: clang-tools-extra/clangd/refactor/Rename.h:32
+
+  bool AllowCrossFile = false; // true if enable cross-file rename
+};

Have we considered post-filtering instead?
Or are we concerned with performance implications of openning too many files to 
compute the corrected edit ranges?

Probably worth documenting why we need it.



Comment at: clang-tools-extra/clangd/refactor/Rename.h:32
+
+  bool AllowCrossFile = false; // true if enable cross-file rename
+};

ilya-biryukov wrote:
> Have we considered post-filtering instead?
> Or are we concerned with performance implications of openning too many files 
> to compute the corrected edit ranges?
> 
> Probably worth documenting why we need it.
NIT: the comment is redundant



Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:269
+cat(Features),
+desc("Enable cross-file rename fature. By default, "
+ "clangd only allows local rename."),
--

[PATCH] D69263: [clangd] Implement cross-file rename.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: fail - 33608 tests passed, 1 failed and 462 were skipped.

  failed: LLVM.tools/llvm-ar/mri-utf8.test

Log files: cmake-log.txt 
, 
ninja_check_all-log.txt 
, 
CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

tests are missing, but the patch should be sufficient for initial review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added reviewers: ilya-biryukov, sammccall.
Herald added subscribers: usaxena95, kadircet, arphaman, jkorous, MaskRay.
Herald added a project: clang.

This is the initial version. The cross-file rename is purely based on
the index (plus a text-match verification).

It is hidden under a command-line flag, and only available for a small set
of symbols.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69263

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Rename.h
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h
  clang/lib/Tooling/Refactoring/Rename/USRFindingAction.cpp

Index: clang/lib/Tooling/Refactoring/Rename/USRFindingAction.cpp
===
--- clang/lib/Tooling/Refactoring/Rename/USRFindingAction.cpp
+++ clang/lib/Tooling/Refactoring/Rename/USRFindingAction.cpp
@@ -41,9 +41,10 @@
 const NamedDecl *getCanonicalSymbolDeclaration(const NamedDecl *FoundDecl) {
   // If FoundDecl is a constructor or destructor, we want to instead take
   // the Decl of the corresponding class.
-  if (const auto *CtorDecl = dyn_cast(FoundDecl))
+  if (const auto *CtorDecl = dyn_cast_or_null(FoundDecl))
 FoundDecl = CtorDecl->getParent();
-  else if (const auto *DtorDecl = dyn_cast(FoundDecl))
+  else if (const auto *DtorDecl =
+   dyn_cast_or_null(FoundDecl))
 FoundDecl = DtorDecl->getParent();
   // FIXME: (Alex L): Canonicalize implicit template instantions, just like
   // the indexer does it.
Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -38,8 +38,8 @@
 llvm::Expected>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
 
-llvm::Expected>
-runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName);
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, StringRef NewName);
 
 std::string runDumpAST(ClangdServer &Server, PathRef File);
 
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -96,10 +96,9 @@
   return std::move(*Result);
 }
 
-llvm::Expected> runRename(ClangdServer &Server,
-PathRef File, Position Pos,
-llvm::StringRef NewName) {
-  llvm::Optional>> Result;
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, llvm::StringRef NewName) {
+  llvm::Optional> Result;
   Server.rename(File, Pos, NewName, /*WantFormat=*/true, capture(Result));
   return std::move(*Result);
 }
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -22,6 +22,19 @@
   return replacementToEdit(Code, arg).range == Range;
 }
 
+RenameInputs localRenameInputs(const Annotations &Code, llvm::StringRef NewName,
+   llvm::StringRef Path,
+   const SymbolIndex *Index = nullptr) {
+  RenameInputs Inputs;
+  Inputs.Pos = Code.point();
+  Inputs.MainFileCode = Code.code();
+  Inputs.MainFilePath = Path;
+  Inputs.NewName = NewName;
+  Inputs.Index = Index;
+  Inputs.AllowCrossFile = false;
+  return Inputs;
+}
+
 TEST(RenameTest, SingleFile) {
   struct Test {
 const char* Before;
@@ -80,10 +93,13 @@
 auto TU = TestTU::withCode(Code.code());
 auto AST = TU.build();
 auto RenameResult =
-renameWithinFile(AST, testPath(TU.Filename), Code.point(), "abcde");
+rename(AST, localRenameInputs(Code, "abcde", testPath(TU.Filename)));
+
 ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
-auto ApplyResult =
-tooling::applyAllReplacements(Code.code(), *RenameResult);
+ASSERT_TRUE(RenameResult->size() == 1);
+EXPECT_EQ(Code.code(), RenameResult->begin()->second.InitialCode);
+auto ApplyResult = tooling::applyAllReplacements(
+Code.code(), RenameResult->begin()->second.Replacements);
 ASSERT_TRUE(bool(ApplyResult)) << ApplyResult.takeError();
 
 EXPECT_EQ(T.After, *