[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:

> 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, RInputs.Index);
----------------
ilya-biryukov wrote:
> Can we avoid duplicating calls to `renamebleWithinFile`? It should be possible to only call `renameOutsideFile` when `CrossFileRename` is enabled and always call `renameWithinFile`. Or is there something I'm missing?
I was attempting to keep two branches (within-file rename, cross-file rename) as separately as possible, but I agree with your suggestion, it does simplify the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263