[refactor] Use a RefactoringResultConsumer instead of tagged refactoring rule classes

[arphaman]

Summary

This patch changes the way that the refactoring results are produced. Instead of using different RefactoringActionRule subclasses for each result type, we now use a single RefactoringResultConsumer. This was suggested by Manuel in https://reviews.llvm.org/D36075.

[arphaman]

Fixed a comment

[arphaman]

Use std::move

[ioeric]

include/clang/Tooling/Refactoring/RefactoringResultConsumer.h

39 ↗

(On Diff #113223)

I think this interface is specific to some refactoring rules and should be pushed down to derived classes.

unittests/Tooling/RefactoringActionRulesTest.cpp

39 ↗

(On Diff #113223)

Can we probably have default error handling in the base class so that we don't need to re-implement these for every derived consumer. I would expect the error handling for initiation and invocation to be similar in different consumers.

[arphaman]

include/clang/Tooling/Refactoring/RefactoringResultConsumer.h

39 ↗

(On Diff #113223)

Are you talking about derived classes of RefactoringResultConsumer? So something like

class RefactoringResultConsumer {
  virtual void handleInvocationError(llvm::Error Err) = 0;
};

class RefactoringResultSourceReplacementConsumer: RefactoringResultConsumer {
 virtual void handle(AtomicChanges SourceReplacements) = 0;
};

If yes, can you please clarify how the rule can call handle if it's in a subclass of RefactoringResultConsumer?

[ioeric]

include/clang/Tooling/Refactoring/RefactoringResultConsumer.h

39 ↗

(On Diff #113223)

Sorry, I thought the handle interface was dispatched by template.

Maybe we can have default implementation of rule-specific handlers, e.g. generate errors, in the base class? Derived classes can implement handlers that they care about and ignore others.

[arphaman]

include/clang/Tooling/Refactoring/RefactoringResultConsumer.h

39 ↗

(On Diff #113223)

Sure, I will add default implementations for handle(...).

I'm not sure about handle...Error(Error) though because llvm::Error forces some form of handling, and I don't want to use a generic stub that will silence the llvm::Error as it won't be that useful. The errors will be handled differently by different clients anyway, like clang-refactor might output the error to stderr, and clangd might either try to notify the user or silently swallow the error.

[arphaman]

• Add default implementation for handle()
• Merge two error handling functions into one

unittests/Tooling/RefactoringActionRulesTest.cpp

39 ↗

(On Diff #113223)

I've merge the two error functions into one, but I'm reluctant to add default implementation for them because of the reasons that I've mentioned in my previous inline comment.

[ioeric]

LGTM.

@klimek Manuel, would you like to take a look? This addresses your comment https://reviews.llvm.org/D36075#854075

include/clang/Tooling/Refactoring/RefactoringResultConsumer.h

30 ↗

(On Diff #113397)

Just wondering if it is possible to provide more information about the initiation failure to the handler?

unittests/Tooling/RefactoringActionRulesTest.cpp

39 ↗

(On Diff #113223)

Ok. Makes sense.

[klimek]

Thanks! I think this makes the code easier to understand. Now my remaining question is why the ResultType is templates vs. also being an interface (sorry for being late, was out on vacation the past 2 weeks :)

include/clang/Tooling/Refactoring/RefactoringResultConsumer.h

30 ↗

(On Diff #113397)

Also, why is initiationFailure not simply handled via handleError?

[arphaman]

include/clang/Tooling/Refactoring/RefactoringResultConsumer.h

30 ↗

(On Diff #113397)

Initiation failure is not really an error as such. It corresponds to a selection in an editor that doesn't overlap with any interesting AST at all. In the editor, the rule will fail to be initiated without a concrete error when we request it, or will be skipped when the editor is querying available actions for some selection. Therefore it doesn't really have any other information.

However, your comments made me reconsider this particular design point. We should emit an error for this kind of initiation failure as well, and clang-refactor will benefit by default as it will consume it and display it. The editor clients will be able to avoid this kind of error if they want by inspecting the selection constraint prior to invocation.

I will remove this method and use just handleError when committing.

[arphaman]

In D37291#857727, @klimek wrote:

Thanks! I think this makes the code easier to understand. Now my remaining question is why the ResultType is templates vs. also being an interface (sorry for being late, was out on vacation the past 2 weeks :)

ResultType is typically a vector of AtomicChange or SymbolOccurrences which we can't subclass from an interface. Or are you wondering if AtomicChange and SymbolOccurrence should subclass from a common interface? It could potentially be useful, but I'm not sure if there could be that many methods that would go into the interface.

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 "clang/Tooling/Refactoring/AtomicChange.h"
-#include "llvm/Support/Error.h"
 #include <vector>
 
 namespace clang {
 namespace tooling {
 
+class RefactoringResultConsumer;
 class RefactoringRuleContext;
 
 /// A common refactoring action rule interface.
 class RefactoringActionRule {
 public:
-  enum RuleKind { SourceChangeRefactoringRuleKind };
-
-  RuleKind getRuleKind() const { return Kind; }
-
   virtual ~RefactoringActionRule() {}
 
-protected:
-  RefactoringActionRule(RuleKind Kind) : Kind(Kind) {}
-
-private:
-  RuleKind Kind;
-};
-
-/// A type of refactoring action rule that produces source replacements in the
-/// form of atomic changes.
-///
-/// This action rule is typically used for local refactorings that replace
-/// source in a single AST unit.
-class SourceChangeRefactoringRule : public RefactoringActionRule {
-public:
-  SourceChangeRefactoringRule()
-      : RefactoringActionRule(SourceChangeRefactoringRuleKind) {}
-
-  /// Initiates and performs a refactoring action that modifies the sources.
+  /// Initiates and performs a specific refactoring action.
   ///
-  /// The specific rule must return an llvm::Error with a DiagnosticError
-  /// payload or None when the refactoring action couldn't be initiated/
-  /// performed, or \c AtomicChanges when the action was performed successfully.
-  virtual Expected<Optional<AtomicChanges>>
-  createSourceReplacements(RefactoringRuleContext &Context) = 0;
-
-  static bool classof(const RefactoringActionRule *Rule) {
-    return Rule->getRuleKind() == SourceChangeRefactoringRuleKind;
-  }
+  /// 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 set of refactoring action rules that should have unique initiation
 /// requirements.
 using RefactoringActionRules =
     std::vector<std::unique_ptr<RefactoringActionRule>>;
 
 } // end namespace tooling
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_ACTION_RULE_H

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

 ///
 ///  - requiredSelection: The refactoring function won't be invoked unless the
 ///                       given selection requirement is satisfied.
 template <typename ResultType, typename... RequirementTypes>
 std::unique_ptr<RefactoringActionRule>
 createRefactoringRule(Expected<ResultType> (*RefactoringFunction)(
                           typename RequirementTypes::OutputType...),
                       const RequirementTypes &... Requirements) {
-  static_assert(
-      std::is_base_of<
-          RefactoringActionRule,
-          internal::SpecificRefactoringRuleAdapter<ResultType>>::value,
+  static_assert(tooling::traits::IsValidRefactoringResult<ResultType>::value,
                 "invalid refactoring result type");
   static_assert(traits::IsRequirement<RequirementTypes...>::value,
                 "invalid refactoring action rule requirement");
   return llvm::make_unique<internal::PlainFunctionRule<
-      ResultType, decltype(RefactoringFunction), RequirementTypes...>>(
+      decltype(RefactoringFunction), RequirementTypes...>>(
       RefactoringFunction, std::make_tuple(Requirements...));
 }
 
 template <
     typename Callable, typename... RequirementTypes,
     typename Fn = decltype(&Callable::operator()),
     typename ResultType = typename internal::LambdaDeducer<Fn>::ReturnType,
     bool IsNonCapturingLambda = std::is_convertible<

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

 //===--- RefactoringActionRulesInternal.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_RULES_INTERNAL_H
 #define LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_ACTION_RULES_INTERNAL_H
 
 #include "clang/Basic/LLVM.h"
 #include "clang/Tooling/Refactoring/RefactoringActionRule.h"
 #include "clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h"
+#include "clang/Tooling/Refactoring/RefactoringResultConsumer.h"
 #include "clang/Tooling/Refactoring/RefactoringRuleContext.h"
 #include "llvm/Support/Error.h"
 #include <type_traits>
 
 namespace clang {
 namespace tooling {
 namespace refactoring_action_rules {
 namespace internal {
 
-/// A wrapper around a specific refactoring action rule that calls a generic
-/// 'perform' method from the specific refactoring method.
-template <typename T> struct SpecificRefactoringRuleAdapter {};
-
-template <>
-class SpecificRefactoringRuleAdapter<AtomicChanges>
-    : public SourceChangeRefactoringRule {
-public:
-  virtual Expected<Optional<AtomicChanges>>
-  perform(RefactoringRuleContext &Context) = 0;
-
-  Expected<Optional<AtomicChanges>>
-  createSourceReplacements(RefactoringRuleContext &Context) final override {
-    return perform(Context);
-  }
-};
-
 /// A specialized refactoring action rule that calls the stored function once
 /// all the of the requirements are fullfilled. The values produced during the
 /// evaluation of requirements are passed to the stored function.
-template <typename ResultType, typename FunctionType,
-          typename... RequirementTypes>
-class PlainFunctionRule final
-    : public SpecificRefactoringRuleAdapter<ResultType> {
+template <typename FunctionType, typename... RequirementTypes>
+class PlainFunctionRule final : public RefactoringActionRule {
 public:
   PlainFunctionRule(FunctionType Function,
                     std::tuple<RequirementTypes...> &&Requirements)
       : Function(Function), Requirements(std::move(Requirements)) {}
 
-  Expected<Optional<ResultType>>
-  perform(RefactoringRuleContext &Context) override {
-    return performImpl(Context,
-                       llvm::index_sequence_for<RequirementTypes...>());
+  void invoke(RefactoringResultConsumer &Consumer,
+              RefactoringRuleContext &Context) override {
+    return invokeImpl(Consumer, Context,
   }
 
 private:
   /// Returns \c T when given \c Expected<Optional<T>>, or \c T otherwise.
   template <typename T>
   static T &&unwrapRequirementResult(Expected<Optional<T>> &&X) {
     assert(X && "unexpected diagnostic!");
     return std::move(**X);
static Optional<llvm::Error> takeErrorNone(Expected<Optional<T>> &Diag) {
     if (!Diag)
       return std::move(Diag.takeError());
     if (!*Diag)
       return llvm::Error::success(); // Initiation failed without a diagnostic.
     return None;
   }
 
   template <size_t... Is>
-  Expected<Optional<ResultType>> performImpl(RefactoringRuleContext &Context,
-                                             llvm::index_sequence<Is...>) {
+  void invokeImpl(RefactoringResultConsumer &Consumer,
+                  RefactoringRuleContext &Context,
     // Initiate the operation.
     auto Values =
         std::make_tuple(std::get<Is>(Requirements).evaluate(Context)...);
     Optional<llvm::Error> InitiationFailure =
         findErrorNone(std::get<Is>(Values)...);
     if (InitiationFailure) {
       llvm::Error Error = std::move(*InitiationFailure);
       if (!Error)
-        return None;
-      return std::move(Error);
+        // FIXME: Use a diagnostic.
+        return Consumer.handleError(llvm::make_error<llvm::StringError>(
+            "refactoring action can't be initiated with the specified "
+            "selection range",
+            llvm::inconvertibleErrorCode()));
+      return Consumer.handleError(std::move(Error));
     }
     // Perform the operation.
-    return Function(
-        unwrapRequirementResult(std::move(std::get<Is>(Values)))...);
+    auto Result =
+        Function(unwrapRequirementResult(std::move(std::get<Is>(Values)))...);
+    if (!Result)
+      return Consumer.handleError(Result.takeError());
+    Consumer.handle(std::move(*Result));
   }
 
   FunctionType Function;
   std::tuple<RequirementTypes...> Requirements;
 };
 
 /// Used to deduce the refactoring result type for the lambda that passed into
 /// createRefactoringRule.

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

+//===--- RefactoringResultConsumer.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_RESULT_CONSUMER_H
+#define LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_RESULT_CONSUMER_H
+
+#include "clang/Basic/LLVM.h"
+#include "clang/Tooling/Refactoring/AtomicChange.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace tooling {
+
+/// An abstract interface that consumes the various refactoring results that can
+/// be produced by refactoring actions.
+///
+/// A valid refactoring result must be handled by a \c handle method.
+class RefactoringResultConsumer {
+public:
+  virtual ~RefactoringResultConsumer() {}
+
+  /// Handles an initation or an invication error. An initiation error typically
+  /// has a \c DiagnosticError payload that describes why initation failed.
+  virtual void handleError(llvm::Error Err) = 0;
+
+  /// Handles the source replacements that are produced by a refactoring action.
+  virtual void handle(AtomicChanges SourceReplacements) {
+    defaultResultHandler();
+  }
+
+private:
+  void defaultResultHandler() {
+    handleError(llvm::make_error<llvm::StringError>(
+        "unsupported refactoring result", llvm::inconvertibleErrorCode()));
+  }
+};
+
+namespace traits {
+namespace internal {
+
+template <typename T> struct HasHandle {
+private:
+  template <typename ClassT>
+  static auto check(ClassT *) -> typename std::is_same<
+      decltype(std::declval<ClassT>().handle(std::declval<T>())), void>::type;
+
+  template <typename> static std::false_type check(...);
+
+public:
+  using Type = decltype(check<RefactoringResultConsumer>(nullptr));
+};
+
+} // end namespace internal
+
+/// A type trait that returns true iff the given type is a valid refactoring
+/// result.
+template <typename T>
+struct IsValidRefactoringResult : internal::HasHandle<T>::Type {};
+
+} // end namespace traits
+} // end namespace tooling
+} // end namespace clang
+
+#endif // LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_RESULT_CONSUMER_H

cfe/trunk/unittests/Tooling/RefactoringActionRulesTest.cpp

void SetUp() override {
     Context.Sources.setMainFileID(
         Context.createInMemoryFile("input.cpp", DefaultCode));
   }
 
   RewriterTestContext Context;
   std::string DefaultCode = std::string(100, 'a');
 };
 
-Expected<Optional<AtomicChanges>>
+Expected<AtomicChanges>
 createReplacements(const std::unique_ptr<RefactoringActionRule> &Rule,
                    RefactoringRuleContext &Context) {
-  return cast<SourceChangeRefactoringRule>(*Rule).createSourceReplacements(
-      Context);
+  class Consumer final : public RefactoringResultConsumer {
+    void handleError(llvm::Error Err) override { Result = std::move(Err); }
+
+    void handle(AtomicChanges SourceReplacements) override {
+      Result = std::move(SourceReplacements);
+    }
+
+  public:
+    Optional<Expected<AtomicChanges>> Result;
+  };
+
+  Consumer C;
+  Rule->invoke(C, Context);
+  return std::move(*C.Result);
 }
 
 TEST_F(RefactoringActionRulesTest, MyFirstRefactoringRule) {
   auto ReplaceAWithB =
       [](std::pair<selection::SourceSelectionRange, int> Selection)
       -> Expected<AtomicChanges> {
     const SourceManager &SM = Selection.first.getSources();
     SourceLocation Loc = Selection.first.getRange().getBegin().getLocWithOffset(
TEST_F(RefactoringActionRulesTest, MyFirstRefactoringRule) {
   // 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});
 
-    Expected<Optional<AtomicChanges>> ErrorOrResult =
+    Expected<AtomicChanges> ErrorOrResult =
         createReplacements(Rule, RefContext);
     ASSERT_FALSE(!ErrorOrResult);
-    ASSERT_FALSE(!*ErrorOrResult);
-    AtomicChanges Result = std::move(**ErrorOrResult);
+    AtomicChanges Result = std::move(*ErrorOrResult);
     ASSERT_EQ(Result.size(), 1u);
     std::string YAMLString =
         const_cast<AtomicChange &>(Result[0]).toYAMLString();
 
     ASSERT_STREQ("---\n"
                  "Key:             'input.cpp:30'\n"
                  "FilePath:        input.cpp\n"
                  "Error:           ''\n"
                  "InsertedHeaders: \n"
                  "RemovedHeaders:  \n"
                  "Replacements:    \n" // Extra whitespace here!
                  "  - FilePath:        input.cpp\n"
                  "    Offset:          30\n"
                  "    Length:          1\n"
                  "    ReplacementText: b\n"
                  "...\n",
                  YAMLString.c_str());
   }
 
-  // When one of the requirements is not satisfied, perform should return either
-  // None or a valid diagnostic.
+  // When one of the requirements is not satisfied, invoke should return a
+  // valid error.
   {
     RefactoringRuleContext RefContext(Context.Sources);
-    Expected<Optional<AtomicChanges>> ErrorOrResult =
+    Expected<AtomicChanges> ErrorOrResult =
         createReplacements(Rule, RefContext);
 
-    ASSERT_FALSE(!ErrorOrResult);
-    Optional<AtomicChanges> Value = std::move(*ErrorOrResult);
-    EXPECT_TRUE(!Value);
+    ASSERT_TRUE(!ErrorOrResult);
+    std::string Message;
+    llvm::handleAllErrors(
+        ErrorOrResult.takeError(),
+        [&](llvm::StringError &Error) { Message = Error.getMessage(); });
+    EXPECT_EQ(Message, "refactoring action can't be initiated with the "
+                       "specified selection range");
   }
 }
 
 TEST_F(RefactoringActionRulesTest, ReturnError) {
   Expected<AtomicChanges> (*Func)(selection::SourceSelectionRange) =
       [](selection::SourceSelectionRange) -> Expected<AtomicChanges> {
     return llvm::make_error<llvm::StringError>(
         "Error", llvm::make_error_code(llvm::errc::invalid_argument));
   };
   auto Rule = createRefactoringRule(
       Func, requiredSelection(
                 selection::identity<selection::SourceSelectionRange>()));
 
   RefactoringRuleContext RefContext(Context.Sources);
   SourceLocation Cursor =
       Context.Sources.getLocForStartOfFile(Context.Sources.getMainFileID());
   RefContext.setSelectionRange({Cursor, Cursor});
-  Expected<Optional<AtomicChanges>> Result =
-      createReplacements(Rule, RefContext);
+  Expected<AtomicChanges> Result = createReplacements(Rule, RefContext);
 
   ASSERT_TRUE(!Result);
   std::string Message;
   llvm::handleAllErrors(Result.takeError(), [&](llvm::StringError &Error) {
     Message = Error.getMessage();
   });
   EXPECT_EQ(Message, "Error");
 }
auto Rule = createRefactoringRule(
       [](int) -> Expected<AtomicChanges> {
         llvm::report_fatal_error("Should not run!");
       },
       requiredSelection(SelectionRequirement()));
 
   SourceLocation Cursor =
       Context.Sources.getLocForStartOfFile(Context.Sources.getMainFileID());
   RefContext.setSelectionRange({Cursor, Cursor});
-  Expected<Optional<AtomicChanges>> Result =
-      createReplacements(Rule, RefContext);
+  Expected<AtomicChanges> Result = createReplacements(Rule, RefContext);
 
   ASSERT_TRUE(!Result);
   std::string Message;
   llvm::handleAllErrors(Result.takeError(), [&](llvm::StringError &Error) {
     Message = Error.getMessage();
   });
   EXPECT_EQ(Message, "bad selection");
 }
 
 } // end anonymous namespace