Page MenuHomePhabricator

[clangd] Add rename support.
ClosedPublic

Authored by hokein on Nov 6 2017, 1:32 AM.

Details

Summary

Make clangd handle "textDocument/rename" request. The rename
functionality comes from the "local-rename" sub-tool of clang-refactor.

Currently clangd only supports local rename (only symbol occurrences in
the main file will be renamed).

Diff Detail

Repository
rL LLVM

Event Timeline

hokein created this revision.Nov 6 2017, 1:32 AM
sammccall edited edge metadata.Nov 6 2017, 10:32 AM

Impl LG apart from error handling :-)

clangd/ClangdServer.cpp
338 ↗(On Diff #121696)

I think you can split out a private method:

Expected<vector<Replacement>> ClangdServer::applyRefactoring(RefactoringActionRuleBase&);

This would make this function easier to read by separating out the rename-specific stuff from (some of) the boilerplate.
(It also seems likely to be reusable if we add direct support for other refactorings, but that's not the main reason I think)

345 ↗(On Diff #121696)

don't you want to do something here?

347 ↗(On Diff #121696)

I don't think you need to override this, assuming you don't expect any of these

369 ↗(On Diff #121696)

else?

377 ↗(On Diff #121696)

Add to the comment what you actually want to fix :-)
Things we may want:

  • rename in any included header
  • rename only in the "main" header
  • provide an error if there are decls we won't rename (e.g. if you rename std::vector)
  • rename globally in project (this may be slow or surprising, so I'd make that a more explicit action)
  • rename in open files (though I'm leery about side-effects of open files)
clangd/ClangdUnit.cpp
1399 ↗(On Diff #121696)

nit: the rest of this file defines functions outside their namespace.

TBH I prefer the style you're using here, but we should be consistent within a file.

test/clangd/rename.test
1 ↗(On Diff #121696)

For new tests after rL317486, please add -pretty and FileCheck -strict-whitespace

(See that patch for how the CHECKs should look)

14 ↗(On Diff #121696)

uber-nit: the # line previous to this one is for spacing, no need for another blank line (or any of them, as far as I'm concerned)

25 ↗(On Diff #121696)

No need to assert the response from shutdown (I removed most of these recently)

Side-comment: will want to let the user know about the fact that global symbols aren't really renamed yet until global rename works?

ilya-biryukov edited edge metadata.Nov 8 2017, 2:09 AM

Just a few minor code style comments.

clangd/ClangdServer.cpp
367 ↗(On Diff #121696)

NIT: local vars are UpperCamelCase

clangd/ClangdUnit.cpp
1399 ↗(On Diff #121696)

+1. Let's document and enforce the preferred style in new commits.
But better done in a separate commit for all of clangd and let's stick to the original style in this file for now.

hokein updated this revision to Diff 122060.Nov 8 2017, 4:20 AM
hokein marked 10 inline comments as done.
  • address review comments.
  • add error handling.
hokein added inline comments.Nov 8 2017, 4:21 AM
clangd/ClangdServer.cpp
338 ↗(On Diff #121696)

I'd keep the code in Rename currently, as most of the code is rename-specific, we can refactor it out when the need arises.

347 ↗(On Diff #121696)

We have to override this method, the default implementation (generating an "unsupported result" error) is not sensible for clangd.
Added a comment for it.

clangd/ClangdUnit.cpp
1399 ↗(On Diff #121696)

Reserved to the original style. Yeah, +1 on change the style for this huge file. Currently, it is not clear for read.

sammccall added inline comments.Nov 8 2017, 4:37 AM
clangd/ClangdServer.cpp
338 ↗(On Diff #121696)

OK, then please find some other way to simplify/break up this method - it's too long and complicated.

347 ↗(On Diff #121696)

I don't understand.

My reading was that we expect handle(SymbolOccurrences) to never be called. If it was called, this is an internal problem and reporting an error seems reasonable.

Under what circumstances would it be called *and* the right response would be to ignore it?

359 ↗(On Diff #122060)

the current refactoring engine doesn't call handle() multiple times, nor both handleError() and handle(), but I don't see that guaranteed anywhere.

At least we should assert that we're not overwriting anything.

hokein updated this revision to Diff 122074.Nov 8 2017, 5:50 AM
hokein marked 2 inline comments as done.

Simplify the code.

hokein added inline comments.Nov 8 2017, 5:52 AM
clangd/ClangdServer.cpp
347 ↗(On Diff #121696)

Oops, you are right. I misread the code of RenameOccurrences: I thought the rename workflow is to find the occurrences, pass to occurrences to the ResultConsumer, generate the atomic change and pass it to ResultConsumer. But the it turns out it just uses the occurrences to generate atomic change directly.

359 ↗(On Diff #122060)

I think the engine should do it (the current refactoring rule implementations imply it). If the refactoring succeeds, call "handle(AtomicChanges)" to pass the result; If any error happened during refactoring, call "handle(Error)".

But adding assert is safer.

sammccall accepted this revision.Nov 8 2017, 6:35 AM
sammccall added inline comments.
clangd/ClangdServer.cpp
67 ↗(On Diff #122074)

why is this needed? shouldn't inheriting the base method be enough if it's only ever called through the base?

This revision is now accepted and ready to land.Nov 8 2017, 6:35 AM
hokein marked an inline comment as done.Nov 8 2017, 6:51 AM
hokein added inline comments.
clangd/ClangdServer.cpp
67 ↗(On Diff #122074)

We need this to make compiler happy, otherwise the compiler will emit "overloaded-virtual" error (the handle(SymbolOccurrences) will be hidden in this class).

This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.