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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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 `Requirement`s 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.



================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRule.h:60
+  /// associated with this rule.
+  virtual Optional<std::pair<StringRef, StringRef>> getEditorCommandInfo() {
+    return None;
----------------
ioeric wrote:
> 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. 
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).



Repository:
  rL LLVM

https://reviews.llvm.org/D38985