[refactor] Use a RefactoringResultConsumer instead of tagged refactoring

rule classes

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

Differential Revision: https://reviews.llvm.org/D37291


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@312316 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/Tooling/Refactoring/RefactoringActionRule.h

@@ -11,52 +11,25 @@
 #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

include/clang/Tooling/Refactoring/RefactoringActionRules.h

@@ -42,15 +42,12 @@ 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,
-      "invalid refactoring result type");
+  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...));
 }
 

include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h

@@ -13,6 +13,7 @@
 #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>
@@ -22,39 +23,20 @@ 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,
+                      llvm::index_sequence_for<RequirementTypes...>());
   }
 
 private:
@@ -95,8 +77,9 @@ private:
   }
 
   template <size_t... Is>
-  Expected<Optional<ResultType>> performImpl(RefactoringRuleContext &Context,
-                                             llvm::index_sequence<Is...>) {
+  void invokeImpl(RefactoringResultConsumer &Consumer,
+                  RefactoringRuleContext &Context,
+                  llvm::index_sequence<Is...>) {
     // Initiate the operation.
     auto Values =
         std::make_tuple(std::get<Is>(Requirements).evaluate(Context)...);
@@ -105,12 +88,19 @@ private:
     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;

include/clang/Tooling/Refactoring/RefactoringResultConsumer.h

@@ -0,0 +1,70 @@
+//===--- 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

unittests/Tooling/RefactoringActionRulesTest.cpp

@@ -32,11 +32,23 @@ protected:
   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) {
@@ -70,11 +82,10 @@ TEST_F(RefactoringActionRulesTest, MyFirstRefactoringRule) {
             .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();
@@ -94,16 +105,20 @@ TEST_F(RefactoringActionRulesTest, MyFirstRefactoringRule) {
                  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");
   }
 }
 
@@ -121,8 +136,7 @@ TEST_F(RefactoringActionRulesTest, ReturnError) {
   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;
@@ -151,8 +165,7 @@ TEST_F(RefactoringActionRulesTest, ReturnInitiationDiagnostic) {
   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;