Page MenuHomePhabricator

[refactor] Add support for editor commands that connect IDEs/editors to the refactoring actions
ClosedPublic

Authored by arphaman on Oct 16 2017, 5:23 PM.

Details

Summary

This patch adds support for editor commands that allow refactoring to be used in editor clients like libclang or clangd.
An editor command can be bound to an refactoring action rule. Once it is bound, it's available in editors that use the supported editor clients.

I plan on sending out a follow-up patch for clangd support tomorrow.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Oct 16 2017, 5:23 PM
hokein edited edge metadata.Oct 17 2017, 3:09 AM

+sammccall who is currently working on clangD, he might have ideas on the clangD/refactor integration.

ioeric added inline comments.Oct 17 2017, 3:11 AM
include/clang/Tooling/Refactoring/EditorCommandRegistry.def
5 ↗(On Diff #119237)

Isn't rename also supported?

include/clang/Tooling/Refactoring/EditorCommands.h
23 ↗(On Diff #119237)

Could you elaborate a bit on the editor/clangd integration? For example, if clangd wants to extract a function, how would clangd discover the rule to use and pass options into the engine? Would editor/clangd be responsible for applying the changes?

include/clang/Tooling/Refactoring/RefactoringActionRule.h
57 ↗(On Diff #119237)

nit: s/that's was/that was/

58 ↗(On Diff #119237)

I'm still not quite sure about the intention of EditorCommand (is it supposed to be used as a mapping from name to rule, or the other way around?), but I'm a bit concerned about mixing editor logic with the refactoring rules this way. Also to enable a editor command, it seems to me that we would need to touch both the registry and the rule creation code, which seems a bit cumbersome.

I think we should either 1) separate out the command logic cleanly without touching action/rule interfaces in the refactoring engine or 2) simply bake the logic into the refactoring engine.

It is unclear to me if 1) is possible, but for 2), I think we could introduce a RefactoringEngine class which carries all refactoring actions as well as a map to serve the purpose of EditorCommand. And I think by doing this, we could also make the interfaces of RefactoringEngine more explicit.

arphaman added inline comments.Oct 18 2017, 7:26 AM
include/clang/Tooling/Refactoring/RefactoringActionRule.h
58 ↗(On Diff #119237)

(Quick comment before the devmeeting:)
Mapping from name to rule.
You're right though, It might be better to explore an alternative solution, but I don't quite follow your proposal yet. I'll be in the tooling hacker's lab at the dev meeting today if you want to discuss this in person.

ioeric added inline comments.Oct 18 2017, 7:58 AM
include/clang/Tooling/Refactoring/RefactoringActionRule.h
58 ↗(On Diff #119237)

Currently, we have createRefactoringActions as the API of the refactoring engine/library. I think we could introduce a RefactoringEngine class that exposes all user-facing interfaces from the engine, including creating/accessing actions and providing the mapping of editor command. And the command registration could be handled inside the class, which I assume would be simpler. This would also allow us to expose more interfaces in the future via this class.

(Sorry, I didn't make it to the dev meeting this year... I hope I could get a chance to meet you in the next dev meeting or the European dev meeting early next year :)

sammccall edited edge metadata.Oct 18 2017, 1:58 PM

Hi Alex! I'm working on clangd, but am pretty new to the project, so forgive some naive questions.

I'm a bit unclear at a high level what the new abstractions in this patch add, in terms of wiring refactorings up to clangd and other tools.
My understanding is: we have a hierarchy of refactorings consisting of actions (roughly: user intent?) and rules (roughly: implementations).

Clangd needs to be able to:

  • enumerate the actions and their associated rules: possible via createRefactoringActions()
  • find names for these (machine readable and human readable): possible at the Action level, not Rule yet
  • determine whether rules can be applied and what configuration is needed: possible via the Rule API
  • invoke rules: possible via the Rule API

So AFAICT, clangd could be hooked up to the existing Refactor API, without EditorCommand or EditorClient. What's the gain?

arphaman added a comment.EditedOct 24 2017, 11:22 AM

Hi Alex! I'm working on clangd, but am pretty new to the project, so forgive some naive questions.

I'm a bit unclear at a high level what the new abstractions in this patch add, in terms of wiring refactorings up to clangd and other tools.
My understanding is: we have a hierarchy of refactorings consisting of actions (roughly: user intent?) and rules (roughly: implementations).

Clangd needs to be able to:

  • enumerate the actions and their associated rules: possible via createRefactoringActions()
  • find names for these (machine readable and human readable): possible at the Action level, not Rule yet
  • determine whether rules can be applied and what configuration is needed: possible via the Rule API
  • invoke rules: possible via the Rule API

Right.

So AFAICT, clangd could be hooked up to the existing Refactor API, without EditorCommand or EditorClient. What's the gain?

  • EditorCommand is not a necessity, but an abstraction to keep the rule simple. I thought it'd be good to have them in a registry for some libclang uses (particularly mapping to C API enums), but on a second though I think that this can be avoided. I'll avoid the class altogether then.
  • EditorClient simplifies the editor operation and will be reused in libclang.
arphaman updated this revision to Diff 120128.Oct 24 2017, 1:38 PM

Avoid using a separate EditorCommand class.

arphaman updated this revision to Diff 120129.Oct 24 2017, 1:40 PM
arphaman marked an inline comment as done.

Fix comment nit.

include/clang/Tooling/Refactoring/RefactoringActionRule.h
58 ↗(On Diff #119237)

I think that's a good idea.

I will post a follow up patch that moves the rule creation to the engine. This one still creates the editor binding in the file.

ioeric added inline comments.Oct 25 2017, 8:30 AM
include/clang/Tooling/Refactoring/RefactoringActionRule.h
60 ↗(On Diff #120129)

I think getEditorCommandInfo might be a wrong name here.

IMO, all end rules (i.e. editor-facing rules) should have name information. It might make sense to introduce a structure that holds all metadata about a rule as well as an interface that returns such a structure. With that, we also don't need to update the API when more rule information is added in the future.

I also think the interface should be pure virtual, and all end rules should implement this interface since they should have names or metadata of some sort.

Thanks, this looks a bunch simpler.

I have a question about the direction here: AFAICS there's various efforts in the refactoring infrastructure to allow consumers of the Refactor libs (e.g. clang-refactor, clangd, XCode?) to operate without knowing the details of the refactorings.

One example is the treating the identifying Name of a rule as data, instead of referring to a subclass directly. Another is the way options are communicated via OptionsVisitor and Requirements etc, where they could be more directly expressed as members of a Rule-specific struct or parameters to a function.

An interface that truly decouples N refactorings from M consumers has scalability advantages, but there are some things that cut against it:

  • It tends to add complexity/indirection, which can slow down contributors, mask bugs, and makes certain features (ones that don't fit into the existing framework) hard to add.
  • If the generic interfaces aren't enough we pierce them, resulting in both coupling *and* complexity. e.g. Clangd really needs control over how refactorings are exposed: which ones are visible under what names, how the JSON-RPC messages are structured. (Because its protocol isn't really an implementation detail under our control, we need to be able to adapt to/influence editors and evolution of the LSP standard).

What's the biggest value we get out of the generic interface? Whose life would get harder if refactorings had this strawman interface/concept?

template<typename Options, typename Result>
class Refactoring {
   bool available(RefactoringContext&, const Options&) = 0;
   Error invoke(RefactoringContext&, const Options&, RefactoringConsumer&) = 0;
}

@klimek may have thoughts here.

include/clang/Tooling/Refactoring/RefactoringActionRule.h
60 ↗(On Diff #120129)

I like the idea of to moving metadata like Title to the RefactoringActionRule through a method like getEditorCommandInfo(), though as Eric says a struct would be clearer.

Is there any reason to make it optional and rebindable? Surely we can come up with a reasonable name for each rule, and if the editor wants to use a different name for some purpose, it can do so without help from the framework (e.g. put the RefactoringActionRule in a struct).

Thanks, this looks a bunch simpler.

I have a question about the direction here: AFAICS there's various efforts in the refactoring infrastructure to allow consumers of the Refactor libs (e.g. clang-refactor, clangd, XCode?) to operate without knowing the details of the refactorings.

One example is the treating the identifying Name of a rule as data, instead of referring to a subclass directly. Another is the way options are communicated via OptionsVisitor and Requirements etc, where they could be more directly expressed as members of a Rule-specific struct or parameters to a function.

An interface that truly decouples N refactorings from M consumers has scalability advantages, but there are some things that cut against it:

  • It tends to add complexity/indirection, which can slow down contributors, mask bugs, and makes certain features (ones that don't fit into the existing framework) hard to add.
  • If the generic interfaces aren't enough we pierce them, resulting in both coupling *and* complexity. e.g. Clangd really needs control over how refactorings are exposed: which ones are visible under what names, how the JSON-RPC messages are structured. (Because its protocol isn't really an implementation detail under our control, we need to be able to adapt to/influence editors and evolution of the LSP standard).

Good point. I agree, it would be good to have a regular refactoring-specific interface that can be used by clangd and provide a more generic abstraction at another layer. However, I think that Clangd will still benefit from a generic editor interface because not all refactorings will have special protocol support in LSP.

I've updated the patch and tried an approach that's somewhat similar to the proposed one.
Please take a look!

What's the biggest value we get out of the generic interface? Whose life would get harder if refactorings had this strawman interface/concept?

template<typename Options, typename Result>
class Refactoring {
   bool available(RefactoringContext&, const Options&) = 0;
   Error invoke(RefactoringContext&, const Options&, RefactoringConsumer&) = 0;
}

@klimek may have thoughts here.

arphaman updated this revision to Diff 120335.Oct 25 2017, 4:09 PM
  • Refactoring operations like ExtractFunction should be declared in headers and should contain the initiation checks.
  • Refactoring actions can now be all created in one "engine" file.
  • A new descriptor interface describes a rule (name, description + optional editor title).
arphaman updated this revision to Diff 120336.Oct 25 2017, 4:11 PM

Remove unused function

The updated patch introduces some redundancy between the RefactoringAction and the descriptor interface. I will address this in a follow-up patch.

Thanks, I think this is looking much better.
A couple of bits of the interface could be yet simpler :-)

include/clang/Tooling/Refactoring/Extract/ExtractFunction.h
21 ↗(On Diff #120336)

The interface given here looks great, much simpler to use directly!

The inheritance gives me some pause. A couple of related things are going on here:

  • we're inheriting the RefactoringActionRuleBase interface, which allows this refactoring to be used generically
  • we're inheriting SourceChangeRefactoringRule, which adapts the generic interface to the one we actually implement

As consequences:

  • we're bound to use a fairly generic name as our entry point here (but I actually think it's a good name, so no worries)
  • we advertise *two* public entry point, that do the same thing (but it's not obvious from the signature)

The thing is, I don't think people who want the "generic" interface will be using this class directly - they'll be going through the Engine, right?
So ExtractFunction -> RefactoringActionRuleBase adapter doesn't need to be here, I think it can be private in the Engine.
That seems like a clearer separation of concerns: ExtractFunction only cares about getting its work done, and RefactoringEngine owns exposing a generic interface to all the rules.

include/clang/Tooling/Refactoring/RefactoringActionRule.h
26 ↗(On Diff #120336)

Does this need to be an interface + implementation + template parameter - can it just be a struct? I think it's static data per instance.

30 ↗(On Diff #120336)

nit: the comment could be more specific: "unique identifier" would make it clearer this isn't primarily human-readable.

38 ↗(On Diff #120336)

I think this is conflating two concepts:

  • what's the best short human-readable name for the refactoring
  • do we want the refactoring turned on by "automatically" for editors

I'm not sure this library is the right place to decide what set of features an editor should present - at least for clangd I'd prefer to have the editor own a whitelist.
Either way, it'd be great to separate out the two fields here.

79 ↗(On Diff #120336)

Did you intend to put this in RefactoringActionRuleBase? It'd be nice to have it for all types of rules.

Also, it's an instance method, but don't you want to be able to get at the information even if you can't apply the refactoring in the given context (so initiate fails).

Maybe this should be static (and RefactoringEngine should expose it as part of its generic interface)

lib/Tooling/Refactoring/Extract.cpp
19 ↗(On Diff #120336)

nit: The cpp file is "Extract" but the header is "Extract/ExtractFunction.h" - are we expecting lots more extractions?

lib/Tooling/Refactoring/RefactoringActions.cpp
24 ↗(On Diff #120336)

It'd be nice if the actual names remained near the rules themselves I think.
Could be quite simple, like

 ExtractFunction {
  ...
  static const RefactoringDescriptor& describe() {
    static RefactoringDescriptor D{
      "extract-function",
      "Extract Function",
      "Extracts code into a new function",
     };
    return D;
  }
}
arphaman marked 5 inline comments as done.Oct 26 2017, 11:48 AM
arphaman added inline comments.
include/clang/Tooling/Refactoring/Extract/ExtractFunction.h
21 ↗(On Diff #120336)

You're right about the dual adaption, but this kind of class should not be invoked directly for the following reason:
While it's easy to call createSourceReplacements or invoke for local refactorings, the cross-TU actions will need a different invocation harness around them. Clangd should avoid using this kind of class directly and use another harness that can run refactorings across multiple-TUs if needed.

I think it's best if it stays right now, as it will help to drive cross-TU refactorings. We can reconsider this in the future though.
I made the createSourceReplacements here private though.

include/clang/Tooling/Refactoring/RefactoringActionRule.h
79 ↗(On Diff #120336)

Classes like ExtractFunction could be reused by multiple rules, so I put it there before to keep the "is this refactoring available in editor" associated with a rule. But now with a more generic descriptor that doesn't really associate title with "is this refactoring available in an editor" I can put the descriptor into the ExtractFunction class itself.

arphaman updated this revision to Diff 120466.Oct 26 2017, 11:50 AM
  • Use a RefactoringDescriptor struct that's accessible from a static function in an operation class.
  • Make createSourceReplacements private.
sammccall accepted this revision.Oct 27 2017, 3:36 AM

This looks great!
Thanks for bearing with me.

include/clang/Tooling/Refactoring/RefactoringActionRule.h
31 ↗(On Diff #120466)

nit: I think the RefactoringDescriptor literals are easier to understand if Title comes before Description, so they're terse --> verbose.

But totally up to you.

lib/Tooling/Refactoring/Rename/RenamingAction.cpp
66 ↗(On Diff #120466)

nit: please wrap before the title.

You can hint clang-format to wrap one-per-line by including a trailing comma in the init-list.

This revision is now accepted and ready to land.Oct 27 2017, 3:36 AM
ioeric accepted this revision.Oct 27 2017, 4:23 AM

lg

include/clang/Tooling/Refactoring/RefactoringActionRule.h
48 ↗(On Diff #120466)

A comment would be nice here.

lib/Tooling/Refactoring/RefactoringActions.cpp
28 ↗(On Diff #120466)

Maybe I'm missing the context here... Why do we want to remove the actions?

arphaman added inline comments.Oct 27 2017, 9:49 AM
lib/Tooling/Refactoring/RefactoringActions.cpp
28 ↗(On Diff #120466)

There's now some redundancy between the command/description in action and the actual refactoring classes like ExtractFunction.
Remove might be a bad term here, something like rewrite would be better. I will change this in the commit.

This revision was automatically updated to reflect the committed changes.
arphaman marked 3 inline comments as done.
arphaman marked an inline comment as done.Oct 27 2017, 11:19 AM