Page MenuHomePhabricator

[clangd] Narrow rename to local symbols.
ClosedPublic

Authored by hokein on Jun 17 2019, 6:01 AM.

Details

Summary

Previously, we perform rename for all kinds of symbols (local, global).

This patch narrows the scope by only renaming symbols not being used
outside of the main file (with index asisitance). Renaming global
symbols is not supported at the moment (return an error).

Event Timeline

hokein created this revision.Jun 17 2019, 6:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2019, 6:01 AM
sammccall added inline comments.Jun 18 2019, 1:41 AM
clang-tools-extra/clangd/refactor/Rename.cpp
105

pull out a function for this?

109

it's unfortunate that our "what's under the cursor" logic here has to duplicate what's done by RenameOccurrences::initiate().

Can we have RenameOccurrences expose the NamedDecl instead?

109

does this not work for macros?

111

We're effectively doing this test twice (see if (!Rename) below) and emitting different error mesages

maybe we should just skip the rest of the check in this case?
(Exposing the ND from the RenameOccurrences would help make this duplication more obvious)

hokein updated this revision to Diff 205355.Jun 18 2019, 8:00 AM
hokein marked 5 inline comments as done.

Address review comments

hokein added inline comments.Jun 18 2019, 8:00 AM
clang-tools-extra/clangd/refactor/Rename.cpp
109

unfortunately, the rename library doesn't rename marcos.

111

The duplication doesn't exist if we expose the ND from the RenameOccurrence.

Thanks for exposing the ND, cleaner.
Found another problem :-( not in your code but about this whole approach to using the index.
Let's talk tomorrow

clang-tools-extra/clangd/refactor/Rename.cpp
81

What about cases where the symbol has a USR but is not indexed? Non-toplevel, template bits we ignore, etc?

Not sure what we should do honestly. The "real" solution is probably to use QNames instead of symbol id here, and index then all. Silently allowing the rename seems pretty bad and needs at least to be explicit

113

This would be fixed in the helper function, move the fixme there

hokein updated this revision to Diff 205540.Jun 19 2019, 3:58 AM
hokein marked an inline comment as done.

Switch to a hybrid solution (language semantic + index) to detect "symbol local".

hokein updated this revision to Diff 205541.Jun 19 2019, 4:00 AM

Remove the unrelated changes.

Use the hybrid approach based on our discussion, the implementation should be fine to review, I'm still working on adding more tests.

hokein updated this revision to Diff 205963.Jun 21 2019, 4:01 AM

Improve the tests.

sammccall accepted this revision.Jun 24 2019, 2:59 AM
sammccall added inline comments.
clang-tools-extra/clangd/refactor/Rename.cpp
69

...to find some other file...

98

Can we move these steps next to the associated code? The separate lists can be hard to maintain.

e.g.

// If the symbol is in the main file (which is not a header), we can rename it
if (DeclaredInMainFile && !MainFileIsHeader)
  ...
99

(which is not a header)

107

renamableWithinFile

110

this isn't the right check - LangOpts::IsHeaderFile

115

can we avoid the double negative here? maybe just inline into the if

126

this is a user-visible error message, right?

  • "reject to rename" doesn't have any obvious meaning to me. Maybe "Cannot rename symbol" ?
  • "no index is provided" -> "symbol may be used in other files (no index available)"
  • "the symbol is not indexable" -> "symbol may be used in other files (not eligible for indexing)"
129

I can't parse "not support for now" - not sure this comment is necessary, the whole point of this function (with its current signature) is to reject this.

clang-tools-extra/clangd/refactor/Rename.h
21–24

It's uncler what "only support" means here.
"Renaming a symbol that's used in another file (per the index) returns an error"

This revision is now accepted and ready to land.Jun 24 2019, 2:59 AM
hokein updated this revision to Diff 206184.Jun 24 2019, 4:06 AM
hokein marked 11 inline comments as done.

Address review comments.

hokein updated this revision to Diff 206185.Jun 24 2019, 4:07 AM

More cleanups.

Harbormaster completed remote builds in B33774: Diff 206185.
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2019, 1:52 AM