Page MenuHomePhabricator

[clang-refactor] Introduce a new rename rule for qualified symbols
ClosedPublic

Authored by hokein on Oct 26 2017, 7:10 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

hokein created this revision.Oct 26 2017, 7:10 AM
hokein updated this revision to Diff 120555.Oct 27 2017, 1:51 AM

Add test.

hokein retitled this revision from [clang-refactor] Introduce "local-qualified-rename" action (For discussion). to [clang-refactor] Introduce "local-qualified-rename" action..Oct 27 2017, 1:57 AM
hokein edited the summary of this revision. (Show Details)
hokein edited the summary of this revision. (Show Details)Oct 27 2017, 2:03 AM
hokein added reviewers: arphaman, ioeric, sammccall.
hokein added a subscriber: cfe-commits.
sammccall edited edge metadata.Oct 27 2017, 3:25 AM

Implementation LG.

My questions are mostly framework-level:

  • should this be an Action, or just a Rule under the existing rename Action?
  • {qname vs selection} x {local vs global} x ... seems like a combinatorial explosion in the number of rules. Shouldn't concerns like format of the input be taken care of before we get to the Rule?

@arphaman

lib/Tooling/Refactoring/Rename/RenamingAction.cpp
143 ↗(On Diff #120555)

if this is a local refactor, and there are no USRs (symbol doesn't exist), is this an error that needs to be signaled somehow?

154 ↗(On Diff #120555)

As discussed offline, it's not clear why this is a separate Action, rather than a different Rule that's part of the same Action.

@arphaman how does the framework answer this question?

hokein added inline comments.Oct 27 2017, 3:55 AM
lib/Tooling/Refactoring/Rename/RenamingAction.cpp
143 ↗(On Diff #120555)

This case should not be happened IMO. If we find the ND, we will rename ND at least.

154 ↗(On Diff #120555)

There is a document describing it, but still ambiguous.

We also had some questions about local-rename from the discussion, need @arphaman's input:

  • OccurrenceFinder is not exposed now, it is merely used in RenameOccurrences. We think there should be a public interface to the clients, like for implementing interactive mode in IDE?
  • Currently the rules defined in the same action must have mutual command-line options, otherwise clang-refactor would complain the command-line option are being registered more than once. It might be very strict for some cases. For example, -new-name is most likely being used by many rules in local-rename action.
sammccall added inline comments.Oct 27 2017, 4:10 AM
lib/Tooling/Refactoring/Rename/RenamingAction.cpp
143 ↗(On Diff #120555)

Oops, of course. Maybe an assert?

hokein updated this revision to Diff 120575.Oct 27 2017, 5:28 AM

Add an assert for non-empty USRs.

arphaman edited edge metadata.Oct 27 2017, 10:56 AM

I've committed https://reviews.llvm.org/D38985, so you'd have to rebase unfortunately. Things are still somewhat unstable :)

lib/Tooling/Refactoring/Rename/RenamingAction.cpp
154 ↗(On Diff #120555)

I think that this should be just a rule in local-rename.

So you'd be able to call:

clang-refactor local-rename -selection=X -new-name=foo
clang-refactor local-rename -old-qualified-name=bar -new-name=foo.

ioeric added inline comments.Oct 27 2017, 11:35 AM
lib/Tooling/Refactoring/Rename/RenamingAction.cpp
154 ↗(On Diff #120555)

We need your help to understand how exactly local-rename is intended to be used.

From the current code and previous conversations we had, it seems to me that the action would support the use case where a user selects an identifier in the editor (say, with cursor) and initiates a local-rename action but without providing the new name in the beginning. The rename rule finds and returns all occurrences (including token ranges) to the editor, and users can then start typing in the new name, and in the same time, the editor performs text replacements according to ranges of occurrences and the new name typed in. Is this how RenameOccurrences is intended to be used in the future?

If this is how local-rename is expected to be used, it would be hard to merge qualified rename into it, because both qualified old name and new name are required in order to calculate the range of a symbol reference, and this doesn't fit with the above workflow. But if my understanding is simply wrong (e.g. the editor would invoke local-rename again to perform the actual refactoring), then I think it makes a lot of sense to merge qualified rename into the current local-rename action.

ioeric added inline comments.Oct 27 2017, 11:38 AM
lib/Tooling/Refactoring/Rename/RenamingAction.cpp
154 ↗(On Diff #120555)

Sorry, by "your help", I was referring to Alex ;) @arphaman

arphaman added inline comments.Oct 27 2017, 1:57 PM
lib/Tooling/Refactoring/Rename/RenamingAction.cpp
154 ↗(On Diff #120555)

You're right that rename should deal with occurrences conceptually, but I believe that's more of requirement imposed onto the editor clients. Rename in particular is basically impossible to map to all clients using just one generic model, so I think it's fine if RenameOccurrences class returns source replacements that local-rename in clang-refactor consumes. I don't think this will change in the future, if anything we will lift OccurrenceFinderclass into the header so that the editor clients can use it.
So I think in terms of the tool it should be ok to have immediate local-rename action that behaves similarly to clang-rename and deals with source changes and not replacements.

ioeric added inline comments.Oct 27 2017, 2:24 PM
lib/Tooling/Refactoring/Rename/RenamingAction.cpp
154 ↗(On Diff #120555)

Thanks for the clarification! In that case, I agree that qualified rename should be a rule in the local-rename action.

hokein updated this revision to Diff 120792.Oct 30 2017, 3:50 AM
  • rebase to master
  • move the implementation to a refactoring rule in local-rename
  • support non-selection based refactoring action invocation call path
hokein marked an inline comment as done.Oct 30 2017, 3:51 AM
hokein added inline comments.
lib/Tooling/Refactoring/Rename/RenamingAction.cpp
154 ↗(On Diff #120555)

Thanks for the explanation, that makes sense. Moved to local-rename action.

arphaman added inline comments.Oct 30 2017, 11:39 AM
lib/Tooling/Refactoring/Rename/RenamingAction.cpp
101 ↗(On Diff #120792)

It might be better to find the declaration (and report error if needed) during in initiation, and then pass the ND to the class. Maybe then both RenameOccurrences and QualifiedRenameRule could subclass from one base class that actually does just this:

auto USRs = getUSRsForDeclaration(ND, Context.getASTContext());
  assert(!USRs.empty());
  return tooling::createRenameAtomicChanges(
      USRs, NewQualifiedName, Context.getASTContext().getTranslationUnitDecl());
jklaehn added a subscriber: jklaehn.Nov 2 2017, 2:17 AM

I spotted two typos. :) Also, the commit message needs to be updated.

lib/Tooling/Refactoring/RefactoringActions.cpp
61 ↗(On Diff #120792)

There is a typo here → "qualified name to change"

lib/Tooling/Refactoring/Rename/RenamingAction.cpp
105 ↗(On Diff #120792)

missing ::? → ::describe()

hokein updated this revision to Diff 121266.Nov 2 2017, 3:06 AM
hokein marked 3 inline comments as done.
  • add USRRenameRule interface.
  • fix typos.
hokein retitled this revision from [clang-refactor] Introduce "local-qualified-rename" action. to [clang-refactor] Introduce a new rename rule for qualified symbols.Nov 2 2017, 3:09 AM
hokein edited the summary of this revision. (Show Details)
hokein added inline comments.
lib/Tooling/Refactoring/Rename/RenamingAction.cpp
101 ↗(On Diff #120792)

Done. Introduced a common interface USRRenameRule.

105 ↗(On Diff #120792)

Good Catch!

ioeric added inline comments.Nov 2 2017, 9:24 AM
lib/Tooling/Refactoring/Rename/RenamingAction.cpp
101 ↗(On Diff #120792)

USRRenameRule doesn't seem to be a very useful abstraction. I think what Alex proposed is a base class that implements createSourceReplacements which could be shared by both RenameOccurrences and QualifiedRenameRule. Intuitively, un-qualified rename is a special case of qualified rename (maybe?), where namespace is not changed.

115 ↗(On Diff #121266)

We should also document the behavior when namespace is changed. What would happen to symbol definitions and symbol references?

arphaman added inline comments.Nov 2 2017, 11:21 AM
lib/Tooling/Refactoring/Rename/RenamingAction.cpp
101 ↗(On Diff #120792)

Yep, I meant that indeed.

hokein updated this revision to Diff 121456.Nov 3 2017, 3:35 AM
hokein marked an inline comment as done.

Document the qualified rename.

hokein added inline comments.Nov 3 2017, 3:37 AM
lib/Tooling/Refactoring/Rename/RenamingAction.cpp
101 ↗(On Diff #120792)

Ah, sorry for the misunderstanding.

Unfortunately, RenameOccurrences and QualifiedRenameRule could not share the same createSourceReplacements implementation,

  • Their supported use cases are different. QualifiedRenameRule only supports renaming class, function, enums, alias and class members, which doesn't cover all the cases of RenameOccurrences (like renaming local variable in function body).
  • we have separated implementations in the code base(createRenameAtomicChanges for qualified rename, createRenameReplacements for un-qualified rename). The implementation of qualified rename does more things, including calculating the shortest prefix qualifiers, getting correct range of replacement.
115 ↗(On Diff #121266)

Sound good! Done.

sammccall added inline comments.Nov 6 2017, 9:27 AM
lib/Tooling/Refactoring/Rename/RenamingAction.cpp
101 ↗(On Diff #120792)

That makes sense (I think), but then we should remove USRRenameRule again if we're not actually reusing anything.

(And ideally we can hide any such future reuse as functions in the cc file, rather than a class hierarchy exposed in the header)

arphaman added inline comments.Nov 6 2017, 2:06 PM
lib/Tooling/Refactoring/Rename/RenamingAction.cpp
101 ↗(On Diff #120792)

ok, that's fine for now then.

104 ↗(On Diff #121456)

You can use a refactoring diagnostic here (see RenameOccurrences::initiate).

hokein updated this revision to Diff 121941.Nov 7 2017, 11:24 AM
  • remove USRRenameRule
  • rebase to master
hokein marked an inline comment as done.Nov 7 2017, 11:27 AM
hokein added inline comments.
lib/Tooling/Refactoring/Rename/RenamingAction.cpp
101 ↗(On Diff #120792)

Done. Reverted it back.

104 ↗(On Diff #121456)

I think using StringError is sufficient here -- didn't see much value using diagnostic:

  1. we need to add a new diagnostic type clang_refactor_no_symbol.
  2. we can't include the detailed information (OldQualifiedName) in the diagnostic, and we don't have SourceLocation in the code for the diagnostic.
arphaman accepted this revision.Nov 7 2017, 2:58 PM

lgtm

This revision is now accepted and ready to land.Nov 7 2017, 2:58 PM
sammccall accepted this revision.Nov 7 2017, 11:42 PM
sammccall added inline comments.
lib/Tooling/Refactoring/Rename/RenamingAction.cpp
116 ↗(On Diff #121941)

"with no indexer support" isn't meaningful to users I think - "within a translation unit" or "within a source file"?

132 ↗(On Diff #121941)

fter -> after

hokein updated this revision to Diff 122038.Nov 8 2017, 12:44 AM
hokein marked 3 inline comments as done.

Fix remaining nits.

This revision was automatically updated to reflect the committed changes.