[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

58

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

59

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

59

(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

59

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

59

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

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

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

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.

59

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

21

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

59

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

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

39

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.

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;
 
 /// 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 name and the title of an editor command that was
+  /// associated with this rule.
+  virtual Optional<std::pair<StringRef, StringRef>> getEditorCommandInfo() {
+    return None;
+  }
 };
 
 } // end namespace tooling
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_ACTION_RULE_H

include/clang/Tooling/Refactoring/RefactoringActionRules.h

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

lib/Tooling/Refactoring/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS Support)
 
 add_clang_library(clangToolingRefactor
   ASTSelection.cpp
   ASTSelectionRequirements.cpp
   AtomicChange.cpp
+  EditorClient.cpp
   Extract.cpp
   RefactoringActions.cpp
   Rename/RenamingAction.cpp
   Rename/SymbolOccurrences.cpp
   Rename/USRFinder.cpp
   Rename/USRFindingAction.cpp
   Rename/USRLocFinder.cpp
 

lib/Tooling/Refactoring/EditorClient.cpp

+//===--- EditorClient.cpp - Refactoring editor integration support --------===//
+//
+//                     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"
+
+namespace clang {
+namespace tooling {
+
+std::unique_ptr<RefactoringActionRule>
+createEditorBinding(std::unique_ptr<RefactoringActionRule> Rule, StringRef Name,
+                    StringRef Title) {
+  class BoundEditorRefactoringActionRule : public RefactoringActionRule {
+  public:
+    BoundEditorRefactoringActionRule(
+        std::unique_ptr<RefactoringActionRule> Rule, StringRef Name,
+        StringRef Title)
+        : Rule(std::move(Rule)), Name(Name), Title(Title) {}
+
+    void invoke(RefactoringResultConsumer &Consumer,
+                RefactoringRuleContext &Context) override {
+      Rule->invoke(Consumer, Context);
+    }
+
+    bool hasSelectionRequirement() override {
+      return Rule->hasSelectionRequirement();
+    }
+
+    void visitRefactoringOptions(RefactoringOptionVisitor &Visitor) override {
+      return Rule->visitRefactoringOptions(Visitor);
+    }
+
+    Optional<std::pair<StringRef, StringRef>> getEditorCommandInfo() override {
+      return std::make_pair(Name, Title);
+    }
+
+  private:
+    std::unique_ptr<RefactoringActionRule> Rule;
+    StringRef Name;
+    StringRef Title;
+  };
+  return llvm::make_unique<BoundEditorRefactoringActionRule>(std::move(Rule),
+                                                             Name, Title);
+}
+
+} // end namespace tooling
+} // end namespace clang

lib/Tooling/Refactoring/Extract.cpp

 ///
 //===----------------------------------------------------------------------===//
 
 #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.
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>(
+    Rules.push_back(
+        createEditorBinding(createRefactoringActionRule<ExtractFunction>(
                                 ExtractableCodeSelectionRequirement(),
-        OptionRequirement<DeclNameOption>()));
+                                OptionRequirement<DeclNameOption>()),
+                            "extract-function", "Extract Function"));
     return Rules;
   }
 };
 
 } // end anonymous namespace
 
 std::unique_ptr<RefactoringAction> createExtractAction() {
   return llvm::make_unique<ExtractRefactoring>();
 }
 
 } // end namespace tooling
 } // end namespace clang

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/RefactoringAction.h"
 #include "clang/Tooling/Refactoring/RefactoringActionRules.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;
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) {
+  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());
+      Optional<std::pair<StringRef, StringRef>> Cmd =
+          Rules[0]->getEditorCommandInfo();
+      ASSERT_TRUE(Cmd);
+      EXPECT_EQ(Cmd->first, "extract-function");
+      EXPECT_EQ(Cmd->second, "Extract Function");
+      return;
+    }
+  }
+  // Never found 'extract'?
+  ASSERT_TRUE(false);
+}
+
 } // end anonymous namespace