[PATCH] D88634: [clangd] Extend the rename API.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:61
+  Range R;
+  FileEdits Edits;
+};
----------------
sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > It's slightly awkward to have the half-populated state (may or may not contain cross-file results, can't tell without the query).
> > > 
> > > I'd consider redundantly including `Edit LocalChanges` and `FileEdits GlobalChanges` with the former always set and the latter empty when returned from preparerename.
> > > It's slightly awkward to have the half-populated state (may or may not contain cross-file results, can't tell without the query).
> > 
> > I feel this is not too bad, the query is quite trivial, just `Edits.size() > 1`.
> > 
> > > I'd consider redundantly including Edit LocalChanges and FileEdits GlobalChanges with the former always set and the latter empty when returned from preparerename.
> > 
> > This change seems nice to get changes for main-file, but I think `GlobalChanges` should include LocalChanges (otherwise, client side needs to combine these two when applying the final rename edits)? then we can't leave the later empty while keeping the former set.
> > 
> > Happy to do the change, but it looks like we don't have usage of `LocalChanges` so far. In prepareRename, we want main-file occurrences, `rename` will always return them regardless of single-file or cross-file rename, so I think `Edits` is efficient. 
> > > It's slightly awkward to have the half-populated state (may or may not contain cross-file results, can't tell without the query).
> > 
> > I feel this is not too bad, the query is quite trivial, just `Edits.size() > 1`.
> This isn't sufficient though: if Edits.size() == 1 then the results may be either complete or incomplete. (Sorry my wording above may have been poor, but this is the distinction I was referring to).
> 
> > > I'd consider redundantly including Edit LocalChanges and FileEdits GlobalChanges with the former always set and the latter empty when returned from preparerename.
> > 
> > This change seems nice to get changes for main-file, but I think `GlobalChanges` should include LocalChanges (otherwise, client side needs to combine these two when applying the final rename edits)? then we can't leave the later empty while keeping the former set.
> 
> Sure we can. `// If the full set of changes is unknown, this field is empty.`
> 
> > Happy to do the change, but it looks like we don't have usage of `LocalChanges` so far. In prepareRename, we want main-file occurrences, `rename` will always return them regardless of single-file or cross-file rename, so I think `Edits` is efficient. 
> 
> Yes, it's possible to implement correct behavior on top of the current API. It effectively reuses the same field with different semantics/contracts, and the client has enough information to know which contract is in place (and throw away the key in the expected singleton map).
> However I think this is fragile and harder to understand than providing separate fields for the two contracts - using types to distinguish local vs global results makes the data harder to misuse. Given we expect this to be used by embedders, I think it's worth adding the second field.
ah, I see your points, agreed. Added the LocalChanges.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88634