This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Extend the rename API.
ClosedPublic

Authored by hokein on Oct 1 2020, 12:19 AM.

Details

Summary

several changes:

  • return a structure result in rename API;
  • prepareRename now returns more information (main-file occurrences)
  • remove the duplicated detecting-touch-identifier code in prepareRename (which is implemented in rename API);

Diff Detail

Event Timeline

hokein created this revision.Oct 1 2020, 12:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2020, 12:19 AM
hokein requested review of this revision.Oct 1 2020, 12:19 AM
sammccall added inline comments.Oct 1 2020, 6:29 AM
clang-tools-extra/clangd/ClangdServer.cpp
408–418

the empty index is weird here - can we pass nullptr?
Currently nullptr leads to an error in the !crossfile case, but I think we can give rename(Index=nullptr, RenameOpts.CrossFile=true) the behavior we want.

(otherwise, if we need an empty index use MemIndex())

411

mention "the index is used to see if the local rename is complete"?

416

we're now returning the "dummy" string to the caller, so we should document it somewhere (or ideally just make it the empty string and document that)

clang-tools-extra/clangd/ClangdServer.h
277

nit: drop "may be incomplete as it"?

clang-tools-extra/clangd/refactor/Rename.cpp
504–506

nit: I'd find this more readable as a default construction followed by assignments to the fields

508–509

and again here - declare the whole struct first and fill it in?

clang-tools-extra/clangd/refactor/Rename.h
58

nit: I'd suggest RenameResult singular, consistent with CodeCompleteResult and ReferencesResult

60

Give this a real name... Target?

61

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.

hokein updated this revision to Diff 295747.Oct 2 2020, 12:51 AM
hokein marked 6 inline comments as done.

address review comments.

hokein added inline comments.Oct 2 2020, 12:53 AM
clang-tools-extra/clangd/ClangdServer.cpp
416

make it empty string, and added document.

clang-tools-extra/clangd/refactor/Rename.h
61

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.

sammccall accepted this revision.Oct 2 2020, 2:03 AM

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

clang-tools-extra/clangd/ClangdServer.h
277

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)

278

nit: resulslt -> result

clang-tools-extra/clangd/refactor/Rename.h
61

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.

This revision is now accepted and ready to land.Oct 2 2020, 2:03 AM
hokein updated this revision to Diff 295798.Oct 2 2020, 5:03 AM
hokein marked 2 inline comments as done.

address comment: add the LocalChanges field in RenameResult.

clang-tools-extra/clangd/refactor/Rename.h
61

ah, I see your points, agreed. Added the LocalChanges.

sammccall accepted this revision.Oct 2 2020, 5:34 AM

LG, thanks!

clang-tools-extra/clangd/refactor/Rename.h
62

this could just be a vector<Range>, as we don't have a use case for the replacements and it avoids the issue of what to replace with. Up to you though.

hokein updated this revision to Diff 295819.Oct 2 2020, 6:55 AM
hokein marked an inline comment as done.

change LocalChanges type to vector<Range>

This revision was landed with ongoing or failed builds.Oct 2 2020, 7:04 AM
This revision was automatically updated to reflect the committed changes.