This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Use raw rename functions to implement the rename.
ClosedPublic

Authored by hokein on Aug 8 2019, 3:52 AM.

Details

Summary

The API provided by refactoring lib doesn't provide enough flexibility
to get clangd's rename to behave as we expect. Instead, we replace it
with the low-level rename functions, which give us more control.

Bonus:

  • performance, previously we visit the TU to find all occurrences, now we just visit top-level decls from main file;
  • fix a bug where we wrongly filter out the main file replacement due to the different relative/absolute file path;

Event Timeline

hokein created this revision.Aug 8 2019, 3:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2019, 3:52 AM
sammccall added inline comments.Aug 8 2019, 3:59 AM
clang-tools-extra/clangd/refactor/Rename.cpp
141

nit: most of the new uses of auto in this patch don't have a type that's obvious from the RHS, nor one that's hard to read, and should probably be expanded

175

is this a regression in this patch, or did the limitation already exist?

178

why can't this fail?

hokein updated this revision to Diff 214112.Aug 8 2019, 4:41 AM
hokein marked 4 inline comments as done.

Address comments.

hokein added inline comments.Aug 8 2019, 4:42 AM
clang-tools-extra/clangd/refactor/Rename.cpp
175

this is not a regression, this is a limitation in clangd.

178

I just kept the previous behavior. Made the error emit to the client rather than crashing here.

sammccall added inline comments.Aug 8 2019, 5:49 AM
clang-tools-extra/clangd/refactor/Rename.cpp
175

can you point me to where this behavior (bailing out when there's more than one name range) occurred in the old version of the code?

(by regression I would mean e.g. if objective-c selector renames worked before this change, but don't work afterwards. But either way, it'd be useful to understand why)

178

I'm not sure what you meant by "kept the previous behavior".
Yes, before this change there was a call cantFail, but the implementation was different, so it's not clear the set of errors it is handling are the same.

Previously, I guess it couldn't fail because we assumed that the underlying layer (which returned a set of edits) wouldn't produce conflicting ones.

Here, we're producing the set of edits ourselves, so any global property (e.g. "no pair of these conflict") needs to be established by us. So we should document whether/under what circumstances it can fail.

182

for actual error handling behavior (if this can actually fail): is this a deliberate choice to refuse to perform any of the edits if there are conflicts? Is this better than applying some non-conflicting set?

Unlike most error-propagation, this is a nontrivial policy choice and needs a comment.

hokein marked 3 inline comments as done.Aug 8 2019, 6:48 AM
hokein added inline comments.
clang-tools-extra/clangd/refactor/Rename.cpp
175

There is a FIXME states that.

can you point me to where this behavior (bailing out when there's more than one name range) occurred in the old version of the code?

I'm pretty sure that the old version of the code will trigger the assertion when doing multiple-piece rename, as the name pieces of the NewName is always 1 according to the code.

178

Previously, I guess it couldn't fail because we assumed that the underlying layer (which returned a set of edits) wouldn't produce conflicting ones.

This is true, at this point we shouldn't see conflicts, because if we have conflicts, we have caught an error (in the RefactoringResultConsumer::handleError) when invoking the RenameOccurrences.

To sum up, in the previous version, we will report the error and refuse to apply any edits if there are conflicts, which is the same behavior we keep in current patch.
The only difference I see is that we traversal main file decls (rather than TUDecl), if we have bugs in the main file decls, we may generate conflicting edits, but I wouldn't too worry about this (as top level decls are a small subset of decls inside TUDecl).

182

I think if there are conflicts, it is a signal that indicates we have bugs in the code (either in clangd, or rename library), applying parts of them may be not valuable (like generating uncompilable code)

I'd prefer to refuse to perform renaming if we have conflicts, WDYT?

sammccall accepted this revision.Aug 9 2019, 1:46 AM
sammccall added inline comments.
clang-tools-extra/clangd/refactor/Rename.cpp
182

That sounds OK to me, can you add a comment?

This revision is now accepted and ready to land.Aug 9 2019, 1:46 AM
hokein updated this revision to Diff 214349.Aug 9 2019, 3:54 AM
hokein marked an inline comment as done.

Add a comment.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2019, 3:56 AM