[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<ReasonToReject>
+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 = renamableWithinFile(*RenameDecl->getCanonicalDecl(),
+                                      RInputs.MainFilePath, RInputs.Index);
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263