[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

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]

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]

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]

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

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]

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.

[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

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

[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

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]

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]

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

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

[ioeric]

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]

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.

cfe/trunk/include/clang/Tooling/Refactoring/Extract/Extract.h

+//===--- Extract.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_H
+#define LLVM_CLANG_TOOLING_REFACTOR_EXTRACT_EXTRACT_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);
+
+  static const RefactoringDescriptor &describe();
+
+private:
+  ExtractFunction(CodeRangeASTSelection Code, Optional<std::string> DeclName)
+      : Code(std::move(Code)),
+        DeclName(DeclName ? std::move(*DeclName) : "extracted") {}
+
+  Expected<AtomicChanges>
+  createSourceReplacements(RefactoringRuleContext &Context) override;
+
+  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_H

cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRegistry.def

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

cfe/trunk/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;
 
+struct RefactoringDescriptor {
+  /// A unique identifier for the specific refactoring.
+  StringRef Name;
+  /// A human readable title for the refactoring.
+  StringRef Title;
+  /// A human readable description of what the refactoring does.
+  StringRef Description;
+};
+
 /// 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;
+
+  /// Returns the structure that describes the refactoring.
+  // static const RefactoringDescriptor &describe() = 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:

cfe/trunk/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,
bool hasSelectionRequirement() override {
                                  RequirementTypes...>::value;
     }
 
     void visitRefactoringOptions(RefactoringOptionVisitor &Visitor) override {
       internal::visitRefactoringOptions(
           Visitor, Requirements,
           llvm::index_sequence_for<RequirementTypes...>());
     }
-
   private:
     std::tuple<RequirementTypes...> Requirements;
   };
 
   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

cfe/trunk/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);
+
+  static const RefactoringDescriptor &describe();
+
+private:
+  RenameOccurrences(const NamedDecl *ND, std::string NewName)
+      : ND(ND), NewName(std::move(NewName)) {}
+
+  Expected<AtomicChanges>
+  createSourceReplacements(RefactoringRuleContext &Context) override;
+
+  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);
 

cfe/trunk/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 * }
 }

cfe/trunk/lib/Tooling/Refactoring/Extract.cpp

 //===--- Extract.cpp - Clang refactoring library --------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// \brief Implements the "extract" refactoring that can pull code into
 /// new functions, methods or declare new variables.
 ///
 //===----------------------------------------------------------------------===//
 
+#include "clang/Tooling/Refactoring/Extract/Extract.h"
 #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"
 
 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();
+}
 
-    // 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);
+} // end anonymous namespace
+
+const RefactoringDescriptor &ExtractFunction::describe() {
+  static const RefactoringDescriptor Descriptor = {
+      "extract-function",
+      "Extract Function",
+      "(WIP action; use with caution!) Extracts code into a new function",
+  };
+  return Descriptor;
+}
+
+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

cfe/trunk/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/Extract.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 {
+
+class DeclNameOption final : public OptionalRefactoringOption<std::string> {
+public:
+  StringRef getName() const { return "name"; }
+  StringRef getDescription() const {
+    return "Name of the extracted declaration";
+  }
+};
+
+// FIXME: Rewrite the Actions to avoid duplication of descriptions/names with
+// rules.
+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>(
+        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: Rewrite the Actions to avoid duplication of descriptions/names with
+// rules.
+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>(
+        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

cfe/trunk/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
+
+const RefactoringDescriptor &RenameOccurrences::describe() {
+  static const RefactoringDescriptor Descriptor = {
+      "local-rename",
+      "Rename",
+      "Finds and renames symbols in code with no indexer support",
+  };
+  return Descriptor;
+}
+
+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() &&

cfe/trunk/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 "ReplacementTest.h"
 #include "RewriterTestContext.h"
 #include "clang/Tooling/Refactoring.h"
-#include "clang/Tooling/Refactoring/RefactoringActionRules.h"
+#include "clang/Tooling/Refactoring/Extract/Extract.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;

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

 
 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);
     }
TEST_F(RefactoringActionRulesTest, ReturnSymbolOccurrences) {
   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) {
+  const RefactoringDescriptor &Descriptor = ExtractFunction::describe();
+  EXPECT_EQ(Descriptor.Name, "extract-function");
+  EXPECT_EQ(
+      Descriptor.Description,
+      "(WIP action; use with caution!) Extracts code into a new function");
+  EXPECT_EQ(Descriptor.Title, "Extract Function");
+}
+
 } // end anonymous namespace