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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG, introducing a separate field for locally affected ranges is up to you.



================
Comment at: clang-tools-extra/clangd/ClangdServer.h:277
+  ///
+  /// It is equal to invoke rename with an empty NewName, except that the
+  /// returned resuslt only contains main-file occurrences (for performance).
----------------
This seems a little like spelling out too many implementation details, and "equal to invoke" isn't quite right grammatically. Maybe just...

"The returned result describes edits in the current file only (all occurrences of the target name are simply deleted)".

(Note that if we split out the field for file ranges it could just be vector<Range>, avoiding the need to explain the replacement with empty string)


================
Comment at: clang-tools-extra/clangd/ClangdServer.h:278
+  /// It is equal to invoke rename with an empty NewName, except that the
+  /// returned resuslt only contains main-file occurrences (for performance).
   void prepareRename(PathRef File, Position Pos,
----------------
nit: resulslt -> result


================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:61
+  Range R;
+  FileEdits Edits;
+};
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88634