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

[arphaman]

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.

[hokein]

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

[ioeric]

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

78

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

79

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]

include/clang/Tooling/Refactoring/RefactoringActionRule.h

79

(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]

include/clang/Tooling/Refactoring/RefactoringActionRule.h

79

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]

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]

In D38985#901152, @sammccall wrote:

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]

Avoid using a separate EditorCommand class.

[arphaman]

Fix comment nit.

include/clang/Tooling/Refactoring/RefactoringActionRule.h

79

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]

include/clang/Tooling/Refactoring/RefactoringActionRule.h

80

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.

[sammccall]

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

80

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).

[arphaman]

In D38985#906893, @sammccall wrote:

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]

• 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]

Remove unused function

[arphaman]

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

[sammccall]

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

22

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

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

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

38

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

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

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

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]

include/clang/Tooling/Refactoring/Extract/ExtractFunction.h

22

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

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]

• Use a RefactoringDescriptor struct that's accessible from a static function in an operation class.
• Make createSourceReplacements private.

[sammccall]

This looks great!
Thanks for bearing with me.

include/clang/Tooling/Refactoring/RefactoringActionRule.h

31

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

63

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.

[ioeric]

lg

include/clang/Tooling/Refactoring/RefactoringActionRule.h

59

A comment would be nice here.

lib/Tooling/Refactoring/RefactoringActions.cpp

28

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

[arphaman]

lib/Tooling/Refactoring/RefactoringActions.cpp

28

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.

include/clang/Tooling/Refactoring/Extract/ExtractFunction.h

+//===--- ExtractFunction.h - Clang refactoring library --------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLING_REFACTOR_EXTRACT_EXTRACT_FUNCTION_H
+#define LLVM_CLANG_TOOLING_REFACTOR_EXTRACT_EXTRACT_FUNCTION_H
+
+#include "clang/Tooling/Refactoring/ASTSelection.h"
+#include "clang/Tooling/Refactoring/RefactoringActionRules.h"
+
+namespace clang {
+namespace tooling {
+
+/// An "Extract Function" refactoring moves code into a new function that's
+/// then called from the place where the original code was.
+class ExtractFunction final : public SourceChangeRefactoringRule {
+public:
+  /// Initiates the extract function refactoring operation.
+  ///
+  /// \param Code     The selected set of statements.
+  /// \param DeclName The name name of the extract function. If None,
+  ///                 "extracted" is used.
+  static Expected<ExtractFunction> initiate(RefactoringRuleContext &Context,
+                                            CodeRangeASTSelection Code,
+                                            Optional<std::string> DeclName);
+
+  Expected<AtomicChanges>
+  createSourceReplacements(RefactoringRuleContext &Context) override;
+
+private:
+  ExtractFunction(CodeRangeASTSelection Code, Optional<std::string> DeclName)
+      : Code(std::move(Code)),
+        DeclName(DeclName ? std::move(*DeclName) : "extracted") {}
+
+  CodeRangeASTSelection Code;
+
+  // FIXME: Account for naming collisions:
+  //  - error when name is specified by user.
+  //  - rename to "extractedN" when name is implicit.
+  std::string DeclName;
+};
+
+} // end namespace tooling
+} // end namespace clang
+
+#endif // LLVM_CLANG_TOOLING_REFACTOR_EXTRACT_EXTRACT_FUNCTION_H

include/clang/Tooling/Refactoring/RefactoringActionRegistry.def

-#ifndef REFACTORING_ACTION
-#define REFACTORING_ACTION(Name)
-#endif
-
-REFACTORING_ACTION(LocalRename)
-REFACTORING_ACTION(Extract)
-
-#undef REFACTORING_ACTION

include/clang/Tooling/Refactoring/RefactoringActionRule.h

 //===--- RefactoringActionRule.h - Clang refactoring library -------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_ACTION_RULE_H
 #define LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_ACTION_RULE_H
 
 #include "clang/Basic/LLVM.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
 #include <vector>
 
 namespace clang {
 namespace tooling {
 
 class RefactoringOptionVisitor;
 class RefactoringResultConsumer;
 class RefactoringRuleContext;
 
+/// Describes a refactoring rule.
+class RefactoringDescriptorInterface {
+public:
+  virtual ~RefactoringDescriptorInterface() {}
+
+  /// Returns the name of the refactoring.
+  virtual StringRef getName() const = 0;
+
+  /// Returns the human readable description of what the refactoring does.
+  virtual StringRef getDescription() const = 0;
+
+  /// Returns the title that should be used in an editor for this refactoring.
+  ///
+  /// When the refactoring has no title, it won't be available in an editor
+  /// unless the editor client explicitly supports the refactoring through
+  /// a specialized interface (like a specialized protocol entry in LSP in
+  /// clangd).
+  virtual Optional<StringRef> getEditorTitle() const { return None; }
+};
+
 /// A common refactoring action rule interface that defines the 'invoke'
 /// function that performs the refactoring operation (either fully or
 /// partially).
 class RefactoringActionRuleBase {
 public:
   virtual ~RefactoringActionRuleBase() {}
 
   /// Initiates and performs a specific refactoring action.
   ///
   /// The specific rule will invoke an appropriate \c handle method on a
   /// consumer to propagate the result of the refactoring action.
   virtual void invoke(RefactoringResultConsumer &Consumer,
                       RefactoringRuleContext &Context) = 0;
 };
 
 /// A refactoring action rule is a wrapper class around a specific refactoring
 /// action rule (SourceChangeRefactoringRule, etc) that, in addition to invoking
 /// the action, describes the requirements that determine when the action can be
 /// initiated.
 class RefactoringActionRule : public RefactoringActionRuleBase {
 public:
   /// Returns true when the rule has a source selection requirement that has
   /// to be fullfilled before refactoring can be performed.
   virtual bool hasSelectionRequirement() = 0;
 
   /// Traverses each refactoring option used by the rule and invokes the
   /// \c visit callback in the consumer for each option.
   ///
   /// Options are visited in the order of use, e.g. if a rule has two
   /// requirements that use options, the options from the first requirement
   /// are visited before the options in the second requirement.
   virtual void visitRefactoringOptions(RefactoringOptionVisitor &Visitor) = 0;
+
+  /// Returns the descriptor for this refactoring rule.
+  virtual const RefactoringDescriptorInterface &getDescriptor() = 0;
 };
 
 } // end namespace tooling
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_ACTION_RULE_H

include/clang/Tooling/Refactoring/RefactoringActionRules.h

 /// immediately after construction.
 ///
 /// The separation of requirements, their evaluation and the invocation of the
 /// refactoring action rule allows the refactoring clients to:
 ///   - Disable refactoring action rules whose requirements are not supported.
 ///   - Gather the set of options and define a command-line / visual interface
 ///     that allows users to input these options without ever invoking the
 ///     action.
-template <typename RuleType, typename... RequirementTypes>
+template <typename RuleType, typename DescriptorType,
+          typename... RequirementTypes>
 std::unique_ptr<RefactoringActionRule>
 createRefactoringActionRule(const RequirementTypes &... Requirements);
 
+/// Creates a refactoring action rule that's accessible in an editor by
+/// wrapping around the given rule.
+///
+/// \param Name the name of the editor command.
+/// \param Title the title (display string) of the editor command.
+std::unique_ptr<RefactoringActionRule>
+createEditorBinding(std::unique_ptr<RefactoringActionRule> Rule, StringRef Name,
+                    StringRef Title);
+
 /// A set of refactoring action rules that should have unique initiation
 /// requirements.
 using RefactoringActionRules =
     std::vector<std::unique_ptr<RefactoringActionRule>>;
 
 /// A type of refactoring action rule that produces source replacements in the
 /// form of atomic changes.
 ///

include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h

void invokeRuleAfterValidatingRequirements(
   // Check if the requirements we're interested in can be evaluated.
   auto Values =
       std::make_tuple(std::get<Is>(Requirements).evaluate(Context)...);
   auto Err = findError(std::get<Is>(Values)...);
   if (Err)
     return Consumer.handleError(std::move(Err));
   // Construct the target action rule by extracting the evaluated
   // requirements from Expected<> wrappers and then run it.
-  RuleType(std::move((*std::get<Is>(Values)))...).invoke(Consumer, Context);
+  auto Rule =
+      RuleType::initiate(Context, std::move((*std::get<Is>(Values)))...);
+  if (!Rule)
+    return Consumer.handleError(Rule.takeError());
+  Rule->invoke(Consumer, Context);
 }
 
 inline void visitRefactoringOptionsImpl(RefactoringOptionVisitor &) {}
 
 /// Scans the list of requirements in a rule and visits all the refactoring
 /// options that are used by all the requirements.
 template <typename FirstT, typename... RestT>
 void visitRefactoringOptionsImpl(RefactoringOptionVisitor &Visitor,
struct AreBaseOf : std::conditional<AreBaseOf<Base, First>::value &&
                                         AreBaseOf<Base, Rest...>::value,
                                     std::true_type, std::false_type>::type {};
 
 template <typename Base, typename T>
 struct AreBaseOf<Base, T> : std::is_base_of<Base, T> {};
 
 } // end namespace internal
 
-template <typename RuleType, typename... RequirementTypes>
+template <typename RuleType, typename DescriptorType,
+          typename... RequirementTypes>
 std::unique_ptr<RefactoringActionRule>
 createRefactoringActionRule(const RequirementTypes &... Requirements) {
   static_assert(std::is_base_of<RefactoringActionRuleBase, RuleType>::value,
                 "Expected a refactoring action rule type");
   static_assert(internal::AreBaseOf<RefactoringActionRuleRequirement,
                                     RequirementTypes...>::value,
                 "Expected a list of refactoring action rules");
 
public:
     }
 
     void visitRefactoringOptions(RefactoringOptionVisitor &Visitor) override {
       internal::visitRefactoringOptions(
           Visitor, Requirements,
           llvm::index_sequence_for<RequirementTypes...>());
     }
 
+    const RefactoringDescriptorInterface &getDescriptor() override {
+      if (!Descriptor)
+        Descriptor = llvm::make_unique<DescriptorType>();
+      return *Descriptor;
+    }
+
   private:
     std::tuple<RequirementTypes...> Requirements;
+    std::unique_ptr<DescriptorType> Descriptor;
   };
 
   return llvm::make_unique<Rule>(std::make_tuple(Requirements...));
 }
 
 } // end namespace tooling
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_ACTION_RULES_INTERNAL_H

include/clang/Tooling/Refactoring/Rename/RenamingAction.h

 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLING_REFACTOR_RENAME_RENAMING_ACTION_H
 #define LLVM_CLANG_TOOLING_REFACTOR_RENAME_RENAMING_ACTION_H
 
 #include "clang/Tooling/Refactoring.h"
 #include "clang/Tooling/Refactoring/AtomicChange.h"
+#include "clang/Tooling/Refactoring/RefactoringActionRules.h"
 #include "clang/Tooling/Refactoring/RefactoringOptions.h"
 #include "clang/Tooling/Refactoring/Rename/SymbolOccurrences.h"
 #include "llvm/Support/Error.h"
 
 namespace clang {
 class ASTConsumer;
 class CompilerInstance;
 

 
 private:
   const std::vector<std::string> &NewNames, &PrevNames;
   const std::vector<std::vector<std::string>> &USRList;
   std::map<std::string, tooling::Replacements> &FileToReplaces;
   bool PrintLocations;
 };
 
-class NewNameOption : public RequiredRefactoringOption<std::string> {
+class RenameOccurrences final : public SourceChangeRefactoringRule {
 public:
-  StringRef getName() const override { return "new-name"; }
-  StringRef getDescription() const override {
-    return "The new name to change the symbol to";
-  }
+  static Expected<RenameOccurrences> initiate(RefactoringRuleContext &Context,
+                                              SourceRange SelectionRange,
+                                              std::string NewName);
+
+  Expected<AtomicChanges>
+  createSourceReplacements(RefactoringRuleContext &Context) override;
+
+private:
+  RenameOccurrences(const NamedDecl *ND, std::string NewName)
+      : ND(ND), NewName(std::move(NewName)) {}
+
+  const NamedDecl *ND;
+  std::string NewName;
 };
 
 /// Returns source replacements that correspond to the rename of the given
 /// symbol occurrences.
 llvm::Expected<std::vector<AtomicChange>>
 createRenameReplacements(const SymbolOccurrences &Occurrences,
                          const SourceManager &SM, const SymbolName &NewName);
 

include/clang/module.modulemap

 
 module Clang_Tooling {
   requires cplusplus umbrella "Tooling" module * { export * }
   // FIXME: Exclude these headers to avoid pulling all of the AST matchers
   // library into clang-format. Due to inline key functions in the headers,
   // importing the AST matchers library gives a link dependency on the AST
   // matchers (and thus the AST), which clang-format should not have.
   exclude header "Tooling/RefactoringCallbacks.h"
-
-  textual header "Tooling/Refactoring/RefactoringActionRegistry.def"
 }
 
 module Clang_ToolingCore {
   requires cplusplus
   umbrella "Tooling/Core" module * { export * }
 }

lib/Tooling/Refactoring/Extract.cpp

 /// \brief Implements the "extract" refactoring that can pull code into
 /// new functions, methods or declare new variables.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Expr.h"
 #include "clang/Rewrite/Core/Rewriter.h"
-#include "clang/Tooling/Refactoring/RefactoringAction.h"
-#include "clang/Tooling/Refactoring/RefactoringActionRules.h"
-#include "clang/Tooling/Refactoring/RefactoringOptions.h"
+#include "clang/Tooling/Refactoring/Extract/ExtractFunction.h"
 
 namespace clang {
 namespace tooling {
 
 namespace {
 
 /// Returns true if \c E is a simple literal or a reference expression that
 /// should not be extracted.
bool isSimpleExpression(const Expr *E) {
   case Stmt::CharacterLiteralClass:
   case Stmt::StringLiteralClass:
     return true;
   default:
     return false;
   }
 }
 
-class ExtractableCodeSelectionRequirement final
-    : public CodeRangeASTSelectionRequirement {
-public:
-  Expected<CodeRangeASTSelection>
-  evaluate(RefactoringRuleContext &Context) const {
-    Expected<CodeRangeASTSelection> Selection =
-        CodeRangeASTSelectionRequirement::evaluate(Context);
-    if (!Selection)
-      return Selection.takeError();
-    CodeRangeASTSelection &Code = *Selection;
+SourceLocation computeFunctionExtractionLocation(const Decl *D) {
+  // FIXME (Alex L): Method -> function extraction should place function before
+  // C++ record if the method is defined inside the record.
+  return D->getLocStart();
+}
+
+} // end anonymous namespace
 
-    // We would like to extract code out of functions/methods/blocks.
-    // Prohibit extraction from things like global variable / field
-    // initializers and other top-level expressions.
-    if (!Code.isInFunctionLikeBodyOfCode())
-      return Context.createDiagnosticError(
-          diag::err_refactor_code_outside_of_function);
+Expected<ExtractFunction>
+ExtractFunction::initiate(RefactoringRuleContext &Context,
+                          CodeRangeASTSelection Code,
+                          Optional<std::string> DeclName) {
 
-    // Avoid extraction of simple literals and references.
-    if (Code.size() == 1 && isSimpleExpression(dyn_cast<Expr>(Code[0])))
-      return Context.createDiagnosticError(
-          diag::err_refactor_extract_simple_expression);
 
-    // FIXME (Alex L): Prohibit extraction of Objective-C property setters.
-    return Selection;
-  }
-};
-
-class ExtractFunction final : public SourceChangeRefactoringRule {
-public:
-  ExtractFunction(CodeRangeASTSelection Code, Optional<std::string> DeclName)
-      : Code(std::move(Code)),
-        DeclName(DeclName ? std::move(*DeclName) : "extracted") {}
-
-  Expected<AtomicChanges>
-  createSourceReplacements(RefactoringRuleContext &Context) override;
-
-private:
-  CodeRangeASTSelection Code;
-
-  // FIXME: Account for naming collisions:
-  //  - error when name is specified by user.
-  //  - rename to "extractedN" when name is implicit.
-  std::string DeclName;
-};
-
-SourceLocation computeFunctionExtractionLocation(const Decl *D) {
-  // FIXME (Alex L): Method -> function extraction should place function before
-  // C++ record if the method is defined inside the record.
-  return D->getLocStart();
+  return ExtractFunction(std::move(Code), DeclName);
 }
 
 // FIXME: Support C++ method extraction.
 // FIXME: Support Objective-C method extraction.
 Expected<AtomicChanges>
 ExtractFunction::createSourceReplacements(RefactoringRuleContext &Context) {
   const Decl *ParentDecl = Code.getFunctionLikeNearestParent();
   assert(ParentDecl && "missing parent");
if (Err)
       return std::move(Err);
   }
 
   // FIXME: Add support for assocciated symbol location to AtomicChange to mark
   // the ranges of the name of the extracted declaration.
   return AtomicChanges{std::move(Change)};
 }
 
-class DeclNameOption final : public OptionalRefactoringOption<std::string> {
-public:
-  StringRef getName() const { return "name"; }
-  StringRef getDescription() const {
-    return "Name of the extracted declaration";
-  }
-};
-
-class ExtractRefactoring final : public RefactoringAction {
-public:
-  StringRef getCommand() const override { return "extract"; }
-
-  StringRef getDescription() const override {
-    return "(WIP action; use with caution!) Extracts code into a new function "
-           "/ method / variable";
-  }
-
-  /// Returns a set of refactoring actions rules that are defined by this
-  /// action.
-  RefactoringActionRules createActionRules() const override {
-    RefactoringActionRules Rules;
-    Rules.push_back(createRefactoringActionRule<ExtractFunction>(
-        ExtractableCodeSelectionRequirement(),
-        OptionRequirement<DeclNameOption>()));
-    return Rules;
-  }
-};
-
-} // end anonymous namespace
-
-std::unique_ptr<RefactoringAction> createExtractAction() {
-  return llvm::make_unique<ExtractRefactoring>();
-}
-
 } // end namespace tooling
 } // end namespace clang

lib/Tooling/Refactoring/RefactoringActions.cpp

 //===--- RefactoringActions.cpp - Constructs refactoring actions ----------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/Tooling/Refactoring/Extract/ExtractFunction.h"
 #include "clang/Tooling/Refactoring/RefactoringAction.h"
+#include "clang/Tooling/Refactoring/RefactoringOptions.h"
+#include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
 
 namespace clang {
 namespace tooling {
 
-// Forward declare the individual create*Action functions.
-#define REFACTORING_ACTION(Name)                                               \
-  std::unique_ptr<RefactoringAction> create##Name##Action();
-#include "clang/Tooling/Refactoring/RefactoringActionRegistry.def"
+namespace {
+
+template <typename T> class RefactoringDescriptor;
+
+// The generic descriptors for all of the refactorings.
+
+template <>
+class RefactoringDescriptor<ExtractFunction>
+    : public RefactoringDescriptorInterface {
+public:
+  StringRef getName() const override { return "extract-function"; }
+
+  StringRef getDescription() const override {
+    return "(WIP action; use with caution!) Extracts code into a new function";
+  }
+
+  Optional<StringRef> getEditorTitle() const override {
+    return StringRef("Extract Function");
+  }
+};
+
+template <>
+class RefactoringDescriptor<RenameOccurrences>
+    : public RefactoringDescriptorInterface {
+public:
+  StringRef getName() const override { return "local-rename"; }
+
+  StringRef getDescription() const override {
+    return "Finds and renames symbols in code with no indexer support";
+  }
+};
+
+class DeclNameOption final : public OptionalRefactoringOption<std::string> {
+public:
+  StringRef getName() const { return "name"; }
+  StringRef getDescription() const {
+    return "Name of the extracted declaration";
+  }
+};
+
+// FIXME: Remove the Actions alltogether.
+class ExtractRefactoring final : public RefactoringAction {
+public:
+  StringRef getCommand() const override { return "extract"; }
+
+  StringRef getDescription() const override {
+    return "(WIP action; use with caution!) Extracts code into a new function";
+  }
+
+  /// Returns a set of refactoring actions rules that are defined by this
+  /// action.
+  RefactoringActionRules createActionRules() const override {
+    RefactoringActionRules Rules;
+    Rules.push_back(
+        createRefactoringActionRule<ExtractFunction,
+                                    RefactoringDescriptor<ExtractFunction>>(
+            CodeRangeASTSelectionRequirement(),
+            OptionRequirement<DeclNameOption>()));
+    return Rules;
+  }
+};
+
+class NewNameOption : public RequiredRefactoringOption<std::string> {
+public:
+  StringRef getName() const override { return "new-name"; }
+  StringRef getDescription() const override {
+    return "The new name to change the symbol to";
+  }
+};
+
+// FIXME: Remove the Actions alltogether.
+class LocalRename final : public RefactoringAction {
+public:
+  StringRef getCommand() const override { return "local-rename"; }
+
+  StringRef getDescription() const override {
+    return "Finds and renames symbols in code with no indexer support";
+  }
+
+  /// Returns a set of refactoring actions rules that are defined by this
+  /// action.
+  RefactoringActionRules createActionRules() const override {
+    RefactoringActionRules Rules;
+    Rules.push_back(
+        createRefactoringActionRule<RenameOccurrences,
+                                    RefactoringDescriptor<RenameOccurrences>>(
+            SourceRangeSelectionRequirement(),
+            OptionRequirement<NewNameOption>()));
+    return Rules;
+  }
+};
+
+} // end anonymous namespace
 
 std::vector<std::unique_ptr<RefactoringAction>> createRefactoringActions() {
   std::vector<std::unique_ptr<RefactoringAction>> Actions;
 
-#define REFACTORING_ACTION(Name) Actions.push_back(create##Name##Action());
-#include "clang/Tooling/Refactoring/RefactoringActionRegistry.def"
+  Actions.push_back(llvm::make_unique<LocalRename>());
+  Actions.push_back(llvm::make_unique<ExtractRefactoring>());
 
   return Actions;
 }
 
 RefactoringActionRules RefactoringAction::createActiveActionRules() {
   // FIXME: Filter out rules that are not supported by a particular client.
   return createActionRules();
 }
 
 } // end namespace tooling
 } // end namespace clang

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

 
 using namespace llvm;
 
 namespace clang {
 namespace tooling {
 
 namespace {
 
-class SymbolSelectionRequirement : public SourceRangeSelectionRequirement {
-public:
-  Expected<const NamedDecl *> evaluate(RefactoringRuleContext &Context) const {
-    Expected<SourceRange> Selection =
-        SourceRangeSelectionRequirement::evaluate(Context);
-    if (!Selection)
-      return Selection.takeError();
-    const NamedDecl *ND =
-        getNamedDeclAt(Context.getASTContext(), Selection->getBegin());
-    if (!ND)
-      return Context.createDiagnosticError(
-          Selection->getBegin(), diag::err_refactor_selection_no_symbol);
-    return getCanonicalSymbolDeclaration(ND);
-  }
-};
-
 class OccurrenceFinder final : public FindSymbolOccurrencesRefactoringRule {
 public:
   OccurrenceFinder(const NamedDecl *ND) : ND(ND) {}
 
   Expected<SymbolOccurrences>
   findSymbolOccurrences(RefactoringRuleContext &Context) override {
     std::vector<std::string> USRs =
         getUSRsForDeclaration(ND, Context.getASTContext());
     std::string PrevName = ND->getNameAsString();
     return getOccurrencesOfUSRs(
         USRs, PrevName, Context.getASTContext().getTranslationUnitDecl());
   }
 
 private:
   const NamedDecl *ND;
 };
 
-class RenameOccurrences final : public SourceChangeRefactoringRule {
-public:
-  RenameOccurrences(const NamedDecl *ND, std::string NewName)
-      : Finder(ND), NewName(std::move(NewName)) {}
+} // end anonymous namespace
+
+Expected<RenameOccurrences>
+RenameOccurrences::initiate(RefactoringRuleContext &Context,
+                            SourceRange SelectionRange, std::string NewName) {
+  const NamedDecl *ND =
+      getNamedDeclAt(Context.getASTContext(), SelectionRange.getBegin());
+  if (!ND)
+    return Context.createDiagnosticError(
+        SelectionRange.getBegin(), diag::err_refactor_selection_no_symbol);
+  return RenameOccurrences(getCanonicalSymbolDeclaration(ND), NewName);
+}
 
-  Expected<AtomicChanges>
-  createSourceReplacements(RefactoringRuleContext &Context) override {
-    Expected<SymbolOccurrences> Occurrences =
-        Finder.findSymbolOccurrences(Context);
-    if (!Occurrences)
-      return Occurrences.takeError();
-    // FIXME: Verify that the new name is valid.
-    SymbolName Name(NewName);
-    return createRenameReplacements(
-        *Occurrences, Context.getASTContext().getSourceManager(), Name);
-  }
+RenameOccurrences::createSourceReplacements(RefactoringRuleContext &Context) {
+      OccurrenceFinder(ND).findSymbolOccurrences(Context);
 
-private:
-  OccurrenceFinder Finder;
-  std::string NewName;
-};
-
-class LocalRename final : public RefactoringAction {
-public:
-  StringRef getCommand() const override { return "local-rename"; }
-
-  StringRef getDescription() const override {
-    return "Finds and renames symbols in code with no indexer support";
-  }
-
-  /// Returns a set of refactoring actions rules that are defined by this
-  /// action.
-  RefactoringActionRules createActionRules() const override {
-    RefactoringActionRules Rules;
-    Rules.push_back(createRefactoringActionRule<RenameOccurrences>(
-        SymbolSelectionRequirement(), OptionRequirement<NewNameOption>()));
-    return Rules;
-  }
-};
-
-} // end anonymous namespace
-
-std::unique_ptr<RefactoringAction> createLocalRenameAction() {
-  return llvm::make_unique<LocalRename>();
-}
-
 Expected<std::vector<AtomicChange>>
 createRenameReplacements(const SymbolOccurrences &Occurrences,
                          const SourceManager &SM, const SymbolName &NewName) {
   // FIXME: A true local rename can use just one AtomicChange.
   std::vector<AtomicChange> Changes;
   for (const auto &Occurrence : Occurrences) {
     ArrayRef<SourceRange> Ranges = Occurrence.getNameRanges();
     assert(NewName.getNamePieces().size() == Ranges.size() &&

unittests/Tooling/RefactoringActionRulesTest.cpp

 //===- unittest/Tooling/RefactoringTestActionRulesTest.cpp ----------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/Tooling/Refactoring/RefactoringActionRules.h"
 #include "ReplacementTest.h"
 #include "RewriterTestContext.h"
 #include "clang/Tooling/Refactoring.h"
-#include "clang/Tooling/Refactoring/RefactoringActionRules.h"
+#include "clang/Tooling/Refactoring/RefactoringAction.h"
 #include "clang/Tooling/Refactoring/RefactoringDiagnostic.h"
 #include "clang/Tooling/Refactoring/Rename/SymbolName.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/Errc.h"
 #include "gtest/gtest.h"
 
 using namespace clang;
 using namespace tooling;
 
 namespace {
 
+class TestDescriptor : public RefactoringDescriptorInterface {
+public:
+  virtual StringRef getName() const { return "test"; }
+  virtual StringRef getDescription() const { return ""; }
+};
+
 class RefactoringActionRulesTest : public ::testing::Test {
 protected:
   void SetUp() override {
     Context.Sources.setMainFileID(
         Context.createInMemoryFile("input.cpp", DefaultCode));
   }
 
   RewriterTestContext Context;

 TEST_F(RefactoringActionRulesTest, MyFirstRefactoringRule) {
   class ReplaceAWithB : public SourceChangeRefactoringRule {
     std::pair<SourceRange, int> Selection;
 
   public:
     ReplaceAWithB(std::pair<SourceRange, int> Selection)
         : Selection(Selection) {}
 
+    static Expected<ReplaceAWithB>
+    initiate(RefactoringRuleContext &Cotnext,
+             std::pair<SourceRange, int> Selection) {
+      return ReplaceAWithB(Selection);
+    }
+
     Expected<AtomicChanges>
     createSourceReplacements(RefactoringRuleContext &Context) {
       const SourceManager &SM = Context.getSources();
       SourceLocation Loc =
           Selection.first.getBegin().getLocWithOffset(Selection.second);
       AtomicChange Change(SM, Loc);
       llvm::Error E = Change.replace(SM, Loc, 1, "b");
       if (E)
         return std::move(E);
       return AtomicChanges{Change};
     }
   };
 
   class SelectionRequirement : public SourceRangeSelectionRequirement {
   public:
     Expected<std::pair<SourceRange, int>>
     evaluate(RefactoringRuleContext &Context) const {
       Expected<SourceRange> R =
           SourceRangeSelectionRequirement::evaluate(Context);
       if (!R)
         return R.takeError();
       return std::make_pair(*R, 20);
     }
   };
-  auto Rule =
-      createRefactoringActionRule<ReplaceAWithB>(SelectionRequirement());
+  auto Rule = createRefactoringActionRule<ReplaceAWithB, TestDescriptor>(
+      SelectionRequirement());
 
   // When the requirements are satisifed, the rule's function must be invoked.
   {
     RefactoringRuleContext RefContext(Context.Sources);
     SourceLocation Cursor =
         Context.Sources.getLocForStartOfFile(Context.Sources.getMainFileID())
             .getLocWithOffset(10);
     RefContext.setSelectionRange({Cursor, Cursor});
llvm::handleAllErrors(ErrorOrResult.takeError(),
                           });
     EXPECT_EQ(DiagID, diag::err_refactor_no_selection);
   }
 }
 
 TEST_F(RefactoringActionRulesTest, ReturnError) {
   class ErrorRule : public SourceChangeRefactoringRule {
   public:
+    static Expected<ErrorRule> initiate(RefactoringRuleContext &,
+                                        SourceRange R) {
+      return ErrorRule(R);
+    }
+
     ErrorRule(SourceRange R) {}
     Expected<AtomicChanges> createSourceReplacements(RefactoringRuleContext &) {
       return llvm::make_error<llvm::StringError>(
           "Error", llvm::make_error_code(llvm::errc::invalid_argument));
     }
   };
 
-  auto Rule =
-      createRefactoringActionRule<ErrorRule>(SourceRangeSelectionRequirement());
+  auto Rule = createRefactoringActionRule<ErrorRule, TestDescriptor>(
+      SourceRangeSelectionRequirement());
   RefactoringRuleContext RefContext(Context.Sources);
   SourceLocation Cursor =
       Context.Sources.getLocForStartOfFile(Context.Sources.getMainFileID());
   RefContext.setSelectionRange({Cursor, Cursor});
   Expected<AtomicChanges> Result = createReplacements(Rule, RefContext);
 
   ASSERT_TRUE(!Result);
   std::string Message;

 
 TEST_F(RefactoringActionRulesTest, ReturnSymbolOccurrences) {
   class FindOccurrences : public FindSymbolOccurrencesRefactoringRule {
     SourceRange Selection;
 
   public:
     FindOccurrences(SourceRange Selection) : Selection(Selection) {}
 
+    static Expected<FindOccurrences> initiate(RefactoringRuleContext &,
+                                              SourceRange Selection) {
+      return FindOccurrences(Selection);
+    }
+
     Expected<SymbolOccurrences>
     findSymbolOccurrences(RefactoringRuleContext &) override {
       SymbolOccurrences Occurrences;
       Occurrences.push_back(SymbolOccurrence(SymbolName("test"),
                                              SymbolOccurrence::MatchingSymbol,
                                              Selection.getBegin()));
       return std::move(Occurrences);
     }
   };
 
-  auto Rule = createRefactoringActionRule<FindOccurrences>(
+  auto Rule = createRefactoringActionRule<FindOccurrences, TestDescriptor>(
       SourceRangeSelectionRequirement());
 
   RefactoringRuleContext RefContext(Context.Sources);
   SourceLocation Cursor =
       Context.Sources.getLocForStartOfFile(Context.Sources.getMainFileID());
   RefContext.setSelectionRange({Cursor, Cursor});
   Optional<SymbolOccurrences> Result = findOccurrences(*Rule, RefContext);
 
   ASSERT_FALSE(!Result);
   SymbolOccurrences Occurrences = std::move(*Result);
   EXPECT_EQ(Occurrences.size(), 1u);
   EXPECT_EQ(Occurrences[0].getKind(), SymbolOccurrence::MatchingSymbol);
   EXPECT_EQ(Occurrences[0].getNameRanges().size(), 1u);
   EXPECT_EQ(Occurrences[0].getNameRanges()[0],
             SourceRange(Cursor, Cursor.getLocWithOffset(strlen("test"))));
 }
 
+TEST_F(RefactoringActionRulesTest, EditorCommandBinding) {
+  std::vector<std::unique_ptr<RefactoringAction>> Actions =
+      createRefactoringActions();
+  for (auto &Action : Actions) {
+    if (Action->getCommand() == "extract") {
+      std::vector<std::unique_ptr<RefactoringActionRule>> Rules =
+          Action->createActiveActionRules();
+      ASSERT_FALSE(Rules.empty());
+      const RefactoringDescriptorInterface &Descriptor =
+          Rules[0]->getDescriptor();
+      EXPECT_EQ(Descriptor.getName(), "extract-function");
+      EXPECT_EQ(
+          Descriptor.getDescription(),
+          "(WIP action; use with caution!) Extracts code into a new function");
+      EXPECT_EQ(Descriptor.getEditorTitle(),
+                Optional<StringRef>("Extract Function"));
+      return;
+    }
+  }
+  // Never found 'extract'?
+  ASSERT_TRUE(false);
+}
+
 } // end anonymous namespace