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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D69263#1718525 <https://reviews.llvm.org/D69263#1718525>, @ilya-biryukov wrote:

> In D69263#1717985 <https://reviews.llvm.org/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<unsigned>(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<ReasonToReject>
+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/Rename.cpp:380
+    auto MainFileRenameEdit =
+        renameWithinFile(AST, RenameDecl, RInputs.NewName);
+    if (!MainFileRenameEdit)
----------------
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...


================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:32
+
+  bool AllowCrossFile = false; // true if enable cross-file rename
+};
----------------
ilya-biryukov wrote:
> 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
Performance is one of the concern. I think the main purpose of this flag is to avoid any regressions in our existing within-file rename while developing the new cross-file rename.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263