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

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Oct 21 08:51:48 PDT 2019


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<unsigned>(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<ReasonToReject>
+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<CXXMethodDecl>(&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."),
----------------
s/fature/feature


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:270
+    desc("Enable cross-file rename fature. By default, "
+         "clangd only allows local rename."),
+    init(false),
----------------
Indicating the default is probably redundant, LLVM options-parsing library will indicate the default on command-line anyway.


================
Comment at: clang/lib/Tooling/Refactoring/Rename/USRFindingAction.cpp:44
   // the Decl of the corresponding class.
-  if (const auto *CtorDecl = dyn_cast<CXXConstructorDecl>(FoundDecl))
+  if (const auto *CtorDecl = dyn_cast_or_null<CXXConstructorDecl>(FoundDecl))
     FoundDecl = CtorDecl->getParent();
----------------
Could you move this to a separate change and add corresponding unit-tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263





More information about the cfe-commits mailing list