[refactor] Initial support for refactoring action rules

[arphaman]

Summary

This patch implements a couple of functions that were described in my RFC from last week that’s available at http://lists.llvm.org/pipermail/cfe-dev/2017-July/054831.html. This patch adds the following pieces: apply function, requiredSelection function, and the selection::SourceSelectionRange constraint.

This code will be used as a base to start moving the functionality and tests for clang-rename over to clang-refactor.

[arphaman]

• Simplify error/diagnostic handling. Use DiagnosticOr instead of DiagOr<Expected>.
• Simplify the code for the selection requirements by removing lambda deducers and instead using special classes for requirements instead of lambdas/functions.
• Rename selectionRequirement to requiredSelection

[arphaman]

+ Introduce refactoring diagnostics.

[arphaman]

Ping.

[ioeric]

This is great work and definitely a lot to digest! ;) Some high-level comments for the first round.

In general, I really appreciate the high-level interfaces; I am just a bit concerned about the implementation which is a bit hard to follow at this point, especially with all the template magics. I left some suggestions in the comments; hopefully they would help.

I would also appreciate more detailed high-level comments on the major classes and their relationships - I found myself constantly going back to the RFC to understand their intentions.

include/clang/Basic/DiagnosticOr.h

1 ↗

(On Diff #110563)

Could you please split changes related to DiagnosticOr into a separate patch and have owners of clang/Basic/ directory review it? ;)

include/clang/Tooling/Refactoring/RefactoringActionRules.h

1 ↗

(On Diff #110563)

Code in this file is a bit hard to follow, especially with so many classes and template magics :P

Is it possible to split them into smaller modules, and ideally with more detailed documentation for each?

It seems to me that the following logics could live in their own headers:

• RefactoringActionRule interface.
• Base classes for requirements.
• SourceSlection-related requirements.
• Derived/specialized rules.

227 ↗

(On Diff #110563)

Why is this called apply? I feel something like createAction or generateAction would be more intuitive.

include/clang/Tooling/Refactoring/RefactoringOperationController.h

19 ↗

(On Diff #110563)

What are all the possible states, for example?

22 ↗

(On Diff #110563)

Maybe I am missing too much context, but I found it hard to understand the comment and couldn't relate the comment to the public interfaces. Could you elaborate more?

It is also unclear to me if this is specific to the source selection.

40 ↗

(On Diff #110563)

It's unclear to me what the intentions of these members are. Could you add some comments for these members?

include/clang/Tooling/Refactoring/RefactoringResult.h

35 ↗

(On Diff #110563)

Do we expect the result changes to be modified? Why?

include/clang/Tooling/Refactoring/SourceSelectionConstraints.h

21 ↗

(On Diff #110563)

nit: redundant be used.

35 ↗

(On Diff #110563)

It might worth explaining the relationship between this and the RequirementBase.

[alexander-shaposhnikov]

include/clang/Basic/DiagnosticOr.h

40 ↗

(On Diff #110563)

explicit ?

49 ↗

(On Diff #110563)

but probably it would be a bit cleaner to enable SFINAE via a template parameter (http://en.cppreference.com/w/cpp/types/enable_if , option #4) rather than via extra argument.

include/clang/Tooling/Refactoring/RefactoringResult.h

27 ↗

(On Diff #110563)

explicit ?

[arphaman]

Extracted DiagnosticOr to a separate patch at https://reviews.llvm.org/D36969.

I will update this patch tomorrow.

[arphaman]

• Split the header
• Remove DiagnosticOr in favour of Expected that will use a DiagnosticError that was proposed in the other patch.
• Address the other review comments

include/clang/Tooling/Refactoring/RefactoringOperationController.h

19 ↗

(On Diff #110563)

Bad comment. I think inputs is more descriptive.

include/clang/Tooling/Refactoring/RefactoringResult.h

27 ↗

(On Diff #110563)

Nah, it's more convenient to be able to return a single AtomicChanges without an explicit initializer I think.

35 ↗

(On Diff #110563)

No, that was a workaround for a non-const member use. I'll use a const cast instead.

include/clang/Tooling/Refactoring/SourceSelectionConstraints.h

35 ↗

(On Diff #110563)

This class is not related to RequirementBase though. Were you talking about another class?

[ioeric]

Thanks for the changes! The code is much clearer.

I am wondering if the current design could be extended to support tools (or rules) that use AST matchers? Or is the selection expected to be powerful enough to replace AST matchers?

We have a few tools in clang-tools-extra (e.g. clang-move and change-namespace) and many internal tools that are based on AST matchers, but we would really like to move them into clang-refactor in the future :)

include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h

26 ↗

(On Diff #112195)

Could you help me understand this class?

This seems to be a selection-specific requirement and should live in selection. It is also used in BaseSpecializedRule which seems to be a higher level of abstraction.

include/clang/Tooling/Refactoring/RefactoringResult.h

21 ↗

(On Diff #112195)

I'm a bit unsure about the abstraction of the refactoring result. I would expected refactoring results to be source changes always. Do you have any refactoring tool that outputs occurrences in mind?

27 ↗

(On Diff #110563)

+1 to explicit which could prevent unintentional conversion. RefactoringResult(Change); isn't too bad IMO.

include/clang/Tooling/Refactoring/SourceSelectionConstraints.h

55 ↗

(On Diff #112195)

Does detail mean internal implementation? Maybe use internal which is more commonly used for this?

[arphaman]

include/clang/Tooling/Refactoring/RefactoringResult.h

21 ↗

(On Diff #112195)

In Xcode we require rename to return symbol occurrences because the IDE is responsible for figuring out:

1. The new name. Thus we can't produce replacements when renaming because we don't know what the new name is when gathering the occurrences.
2. Whether these occurrences should be actually applied. Thus we can't produce replacements because it's up to the user to decide if they want to rename some occurrence in a comment for example.

In general 2) can be applied to tools like clang-refactor that could allow users to select occurrences that don't guarantee a direct semantic match (comments, etc.) in an interactive manner.

I think clangd has a rather naive rename support, so these points may not apply, but we may want to extend clangd's support for rename in the future as well.

[arphaman]

In D36075#851278, @ioeric wrote:

Thanks for the changes! The code is much clearer.

I am wondering if the current design could be extended to support tools (or rules) that use AST matchers? Or is the selection expected to be powerful enough to replace AST matchers?

We have a few tools in clang-tools-extra (e.g. clang-move and change-namespace) and many internal tools that are based on AST matchers, but we would really like to move them into clang-refactor in the future :)

The selection requirement is supposed to be orthogonal to AST matchers, not a replacement. It should be used when working with source selection in editors.

I did mess around with moving over clang-reorder-fields using this kind of model and didn't see any issues when using AST matchers. Essentially I used the requiredOption requirements and mapped them to run my matching code instead of using the selection requirement. Thus this requirement was satisfied only when the AST matchers were successful. It might be possible to simplify that pattern even further to make it simpler.

In D36075#851278, @ioeric wrote:

Thanks for the changes! The code is much clearer.

I am wondering if the current design could be extended to support tools (or rules) that use AST matchers? Or is the selection expected to be powerful enough to replace AST matchers?

We have a few tools in clang-tools-extra (e.g. clang-move and change-namespace) and many internal tools that are based on AST matchers, but we would really like to move them into clang-refactor in the future :)

[arphaman]

include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h

26 ↗

(On Diff #112195)

It's just a container class that stores all information about the requiredSelection requirement. I agree about BaseSpecializedRule, that connection should be chopped. I will move the evaluation code into the requirement itself when I update the patch.

I would tend to disagree about moving it though, as SourceSelectionRequirement is a requirement first and I think that's why it should live with other requirements. Yes, it's related to selections, but it uses them to implement the requirement. I think it's better to keep requirements together, as opposed to having option requirements close to options, selection requirements close to selection, and so on. WDYT?

[arphaman]

• Rename detail to internal.
• Remove the BaseSpecializedRule.
• Simplify selection requirement and constraint evaluation.

[ioeric]

include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h

26 ↗

(On Diff #112195)

Thanks!

Makes sense. We might want to put individual requirements into their own headers so that this doesn't grow into a huge file when more requirements are supported.

include/clang/Tooling/Refactoring/RefactoringActionRuleRequirementsInternal.h

42 ↗

(On Diff #112566)

It might worth having a comment explaining why and how Expected<Optional> is wrapped and unwrapped during the evaluation.

include/clang/Tooling/Refactoring/RefactoringActionRules.h

33 ↗

(On Diff #112566)

We might want to document supported requirements somewhere else so that we don't need to update this file every time a new requirement is added.

include/clang/Tooling/Refactoring/RefactoringOperation.h

1 ↗

(On Diff #112566)

s/RefactoringOperationController.h/RefactoringOperation.h/ :)

29 ↗

(On Diff #112566)

I found the name a bit confusing - RefactoringOperation sounds a bit like RefactoringAction.

Would it make sense to call this RefactoringContext or RefactoringRuleContext, if this stores states of a refactoring rule?

include/clang/Tooling/Refactoring/RefactoringResult.h

21 ↗

(On Diff #112195)

I feel occurrences are more of an intermediate state of a refactoring action than a result. I'm wondering if it makes sense to introduce a separate class to represent such intermediate states? I am a bit nervous to fuse multiple classes into one; the interfaces can get pretty ugly when more result kinds are added.

lib/Tooling/Refactoring/SourceSelectionConstraints.cpp

13 ↗

(On Diff #112566)

We are tempted to avoid using namespace if possible.

[arphaman]

include/clang/Tooling/Refactoring/RefactoringResult.h

21 ↗

(On Diff #112195)

Good point. I agree.

I think it would be better to differentiate between RefactoringActionRules then. Ordinary rules return a set of AtomicChanges instead of RefactoringResult. But then we could also have "interactive" rules that return "partial" results like symbol occurrences.

I think I'll try the following approach:

class RefactoringActionRule {
  virtual ~RefactoringActionRule() {}
};

class RefactoringActionSourceChangeRule: public RefactoringActionRule {
public:
  virtual Expected<Optional<AtomicChanges>>
  createSourceReplacements(RefactoringOperation &Operation) = 0;
};

class RefactoringActionSymbolOccurrencesRule: public RefactoringActionRule {
public:
  virtual Expected<Optional<SymbolOccurrences>>
  findSymbolOccurrences(RefactoringOperation &Operation) = 0;
};

[arphaman]

lib/Tooling/Refactoring/SourceSelectionConstraints.cpp

13 ↗

(On Diff #112566)

Why? It's not in a header. using namespace clang is the common practice across all of Clang's sources.

[alexander-shaposhnikov]

@arphaman

The selection requirement is supposed to be orthogonal to AST matchers, not a replacement. It should be used when working with source selection in editors. 
I did mess around with moving over clang-reorder-fields using this kind of model and didn't see any issues when using AST matchers. Essentially I used the  requiredOption requirements and mapped them to run my 
matching code instead of using the selection requirement. Thus this requirement was satisfied only when the AST matchers were successful. 
It might be possible to simplify that pattern even further to make it simpler.

that's great, i'm interested in this too and would be happy to see clang-reorder-fields moving to clang-refactor (pls, let me know if i can help make this happen)

[arphaman]

include/clang/Tooling/Refactoring/RefactoringActionRules.h

33 ↗

(On Diff #112566)

Do you think it should be in Clang's documentation? I can start on a new document there but I'd prefer to do it in a separate patch. WDYT?

[arphaman]

• Get rid of RefactoringResult in favour of different rule types.
• Rename RefactoringOperation to RefactoringRuleContext.
• Address a couple more comments.

[ioeric]

Thanks for the changes! Lgtm with a few nits.

include/clang/Tooling/Refactoring/RefactoringActionRules.h

33 ↗

(On Diff #112566)

Sure, this is fine for now. It would be nice to have proper documentation in the future when pieces get into places.

include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h

33 ↗

(On Diff #112678)

Why isn't this a interface in SpecificRefactoringRuleAdapter with return type Expected<Optional<T>>?

unittests/Tooling/RefactoringActionRulesTest.cpp

67 ↗

(On Diff #112678)

It would be nice to also rename the variable from Operation to Context.

[arphaman]

Thanks,

I'll start working on the documentation patch and will split follow-up clang-refactor patch into one or two parts today.

include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h

33 ↗

(On Diff #112678)

A method declaration in SpecificRefactoringRuleAdapter won't work since it won't be available in either the template specialisation or the deriving class as the classes won't be directly related. I could use a separate parent class independent of SpecificRefactoringRuleAdapter that declares a generic interface though.

[klimek]

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

23–37

Sorry for being late, was out on vacation.
Generally, why do we need this tag-based abstraction here instead of using the more typical OO double-dispatch where necessary?
(We do this in the AST a lot, but the AST is special, because there we want to implement a lot of different algorithms that rely on the node type, while I don't see how that applies here)

[arphaman]

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

23–37

Generally the clients will have to somehow distinguish between the types of results that are produced by rules to figure out what to do (e.g. AtomicChanges -> apply, SymbolOccurrences -> ask user, Continuation -> look for more ASTs). So far I've thought that the LLVM based dynamic casts will work well for this, e.g.

if (auto *Action = dyn_cast<SourceChangeRefactoringRule>()) {
  Expected<Optional<AtomicChanges>> Changes = Action->createSourceReplacements();
  applyChanges(Changes);
} else if (...) {
  ...
} else (...) {
   ...
}

But you're probably right, there might be a better way to do this rather than the tag based approach. Something like a consumer class that clients can implement that provides consumer functions that take in the specific results. I reckon a single consumer will actually work better in the long-run when we might Continuations that both return changes in the first TU and information for searches in other TUs. I'll see if I can get a patch out that removes this tag and uses the consumer approach.

[klimek]

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

23–37

Cool, looking forward to it :)

cfe/trunk/include/clang/Basic/LLVM.h

namespace llvm {
   class Twine;
   template<typename T> class ArrayRef;
   template<typename T> class MutableArrayRef;
   template<typename T> class OwningArrayRef;
   template<unsigned InternalLen> class SmallString;
   template<typename T, unsigned N> class SmallVector;
   template<typename T> class SmallVectorImpl;
   template<typename T> class Optional;
+  template <class T> class Expected;
 
   template<typename T>
   struct SaveAndRestore;
 
   // Reference counting.
   template <typename T> class IntrusiveRefCntPtr;
   template <typename T> struct IntrusiveRefCntPtrInfo;
   template <class Derived> class RefCountedBase;
namespace clang {
   using llvm::ArrayRef;
   using llvm::MutableArrayRef;
   using llvm::OwningArrayRef;
   using llvm::SmallString;
   using llvm::SmallVector;
   using llvm::SmallVectorImpl;
   using llvm::SaveAndRestore;
 
+  // Error handling.
+  using llvm::Expected;
+
   // Reference counting.
   using llvm::IntrusiveRefCntPtr;
   using llvm::IntrusiveRefCntPtrInfo;
   using llvm::RefCountedBase;
 
   using llvm::raw_ostream;
   using llvm::raw_pwrite_stream;
 } // end namespace clang.
 
 #endif

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

public:
   /// \p KeyPosition should be the location of the key syntactical element that
   /// is being changed, e.g. the call to a refactored method.
   AtomicChange(const SourceManager &SM, SourceLocation KeyPosition);
 
   /// \brief Creates an atomic change for \p FilePath with a customized key.
   AtomicChange(llvm::StringRef FilePath, llvm::StringRef Key)
       : Key(Key), FilePath(FilePath) {}
 
+  AtomicChange(AtomicChange &&) = default;
+  AtomicChange(const AtomicChange &) = default;
+
+  AtomicChange &operator=(AtomicChange &&) = default;
+  AtomicChange &operator=(const AtomicChange &) = default;
+
   /// \brief Returns the atomic change as a YAML string.
   std::string toYAMLString();
 
   /// \brief Converts a YAML-encoded automic change to AtomicChange.
   static AtomicChange convertFromYAML(llvm::StringRef YAMLContent);
 
   /// \brief Returns the key of this change, which is a concatenation of the
   /// file name and offset of the key position.
private:
   std::string Key;
   std::string FilePath;
   std::string Error;
   std::vector<std::string> InsertedHeaders;
   std::vector<std::string> RemovedHeaders;
   tooling::Replacements Replaces;
 };
 
+using AtomicChanges = std::vector<AtomicChange>;
+
 // Defines specs for applying changes.
 struct ApplyChangesSpec {
   // If true, cleans up redundant/erroneous code around changed code with
   // clang-format's cleanup functionality, e.g. redundant commas around deleted
   // parameter or empty namespaces introduced by deletions.
   bool Cleanup = true;
 
   format::FormatStyle Style = format::getNoStyle();

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 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.
+  ///
+  /// 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;
+  }
+};
+
+/// 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/RefactoringActionRuleRequirements.h

+//===--- RefactoringActionRuleRequirements.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_REQUIREMENTS_H
+#define LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_ACTION_RULE_REQUIREMENTS_H
+
+#include "clang/Tooling/Refactoring/RefactoringActionRuleRequirementsInternal.h"
+#include "llvm/Support/Error.h"
+#include <type_traits>
+
+namespace clang {
+namespace tooling {
+namespace refactoring_action_rules {
+
+/// Creates a selection requirement from the given requirement.
+///
+/// Requirements must subclass \c selection::Requirement and implement
+/// evaluateSelection member function.
+template <typename T>
+internal::SourceSelectionRequirement<
+    typename selection::internal::EvaluateSelectionChecker<
+        decltype(&T::evaluateSelection)>::ArgType,
+    typename selection::internal::EvaluateSelectionChecker<
+        decltype(&T::evaluateSelection)>::ReturnType,
+    T>
+requiredSelection(
+    const T &Requirement,
+    typename std::enable_if<selection::traits::IsRequirement<T>::value>::type
+        * = nullptr) {
+  return internal::SourceSelectionRequirement<
+      typename selection::internal::EvaluateSelectionChecker<decltype(
+          &T::evaluateSelection)>::ArgType,
+      typename selection::internal::EvaluateSelectionChecker<decltype(
+          &T::evaluateSelection)>::ReturnType,
+      T>(Requirement);
+}
+
+template <typename T>
+void requiredSelection(
+    const T &,
+    typename std::enable_if<
+        !std::is_base_of<selection::Requirement, T>::value>::type * = nullptr) {
+  static_assert(
+      sizeof(T) && false,
+      "selection requirement must be a class derived from Requirement");
+}
+
+} // end namespace refactoring_action_rules
+} // end namespace tooling
+} // end namespace clang
+
+#endif // LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_ACTION_RULE_REQUIREMENTS_H

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

+//===--- RefactoringActionRuleRequirementsInternal.h - --------------------===//
+//
+//                     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_REQUIREMENTS_INTERNAL_H
+#define LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_ACTION_REQUIREMENTS_INTERNAL_H
+
+#include "clang/Basic/LLVM.h"
+#include "clang/Tooling/Refactoring/RefactoringRuleContext.h"
+#include "clang/Tooling/Refactoring/SourceSelectionConstraints.h"
+#include <type_traits>
+
+namespace clang {
+namespace tooling {
+namespace refactoring_action_rules {
+namespace internal {
+
+/// A base class for any requirement. Used by the \c IsRequirement trait to
+/// determine if a class is a valid requirement.
+struct RequirementBase {};
+
+/// Defines a type alias of type \T when given \c Expected<Optional<T>>, or
+/// \c T otherwise.
+template <typename T> struct DropExpectedOptional { using Type = T; };
+
+template <typename T> struct DropExpectedOptional<Expected<Optional<T>>> {
+  using Type = T;
+};
+
+/// The \c requiredSelection refactoring action requirement is represented
+/// using this type.
+template <typename InputT, typename OutputT, typename RequirementT>
+struct SourceSelectionRequirement
+    : std::enable_if<selection::traits::IsConstraint<InputT>::value &&
+                         selection::traits::IsRequirement<RequirementT>::value,
+                     RequirementBase>::type {
+  using OutputType = typename DropExpectedOptional<OutputT>::Type;
+
+  SourceSelectionRequirement(const RequirementT &Requirement)
+      : Requirement(Requirement) {}
+
+  /// Evaluates the action rule requirement by ensuring that both the selection
+  /// constraint and the selection requirement can be evaluated with the given
+  /// context.
+  ///
+  /// \returns None if the selection constraint is not evaluated successfully,
+  /// Error if the selection requirement is not evaluated successfully or
+  /// an OutputT if the selection requirement was successfully. The OutpuT
+  /// value is wrapped in Expected<Optional<>> which is then unwrapped by the
+  /// refactoring action rule before passing the value to the refactoring
+  /// function.
+  Expected<Optional<OutputType>> evaluate(RefactoringRuleContext &Context) {
+    Optional<InputT> Value = InputT::evaluate(Context);
+    if (!Value)
+      return None;
+    return std::move(Requirement.evaluateSelection(*Value));
+  }
+
+private:
+  const RequirementT Requirement;
+};
+
+} // end namespace internal
+
+namespace traits {
+
+/// A type trait that returns true iff the given type is a valid rule
+/// requirement.
+template <typename First, typename... Rest>
+struct IsRequirement : std::conditional<IsRequirement<First>::value &&
+                                            IsRequirement<Rest...>::value,
+                                        std::true_type, std::false_type>::type {
+};
+
+template <typename T>
+struct IsRequirement<T>
+    : std::conditional<std::is_base_of<internal::RequirementBase, T>::value,
+                       std::true_type, std::false_type>::type {};
+
+} // end namespace traits
+} // end namespace refactoring_action_rules
+} // end namespace tooling
+} // end namespace clang
+
+#endif // LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_ACTION_REQUIREMENTS_INTERNAL_H

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

+//===--- RefactoringActionRules.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_H
+#define LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_ACTION_RULES_H
+
+#include "clang/Tooling/Refactoring/RefactoringActionRule.h"
+#include "clang/Tooling/Refactoring/RefactoringActionRulesInternal.h"
+
+namespace clang {
+namespace tooling {
+namespace refactoring_action_rules {
+
+/// Creates a new refactoring action rule that invokes the given function once
+/// all of the requirements are satisfied. The values produced during the
+/// evaluation of requirements are passed to the given function (in the order of
+/// requirements).
+///
+/// \param RefactoringFunction the function that will perform the refactoring
+/// once the requirements are satisfied. The function must return a valid
+/// refactoring result type wrapped in an \c Expected type. The following result
+/// types are currently supported:
+///
+///  - AtomicChanges: the refactoring function will be used to create source
+///                   replacements.
+///
+/// \param Requirements a set of rule requirements that have to be satisfied.
+/// Each requirement must be a valid requirement, i.e. the value of
+/// \c traits::IsRequirement<T> must be true. The following requirements are
+/// currently supported:
+///
+///  - 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,
+      "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...>>(
+      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<
+        Callable,
+        ResultType (*)(typename RequirementTypes::OutputType...)>::value,
+    typename = typename std::enable_if<IsNonCapturingLambda>::type>
+std::unique_ptr<RefactoringActionRule>
+createRefactoringRule(const Callable &C,
+                      const RequirementTypes &... Requirements) {
+  ResultType (*Func)(typename RequirementTypes::OutputType...) = C;
+  return createRefactoringRule(Func, Requirements...);
+}
+
+} // end namespace refactoring_action_rules
+} // end namespace tooling
+} // end namespace clang
+
+#endif // LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_ACTION_RULES_H

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/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> {
+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...>());
+  }
+
+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);
+  }
+  template <typename T> static T &&unwrapRequirementResult(T &&X) {
+    return std::move(X);
+  }
+
+  /// Scans the tuple and returns a \c PartialDiagnosticAt
+  /// from the first invalid \c DiagnosticOr value. Returns \c None if all
+  /// values are valid.
+  template <typename FirstT, typename... RestT>
+  static Optional<llvm::Error> findErrorNone(FirstT &First, RestT &... Rest) {
+    Optional<llvm::Error> Result = takeErrorNone(First);
+    if (Result)
+      return Result;
+    return findErrorNone(Rest...);
+  }
+
+  static Optional<llvm::Error> findErrorNone() { return None; }
+
+  template <typename T> static Optional<llvm::Error> takeErrorNone(T &) {
+    return None;
+  }
+
+  template <typename T>
+  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...>) {
+    // 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);
+    }
+    // Perform the operation.
+    return Function(
+        unwrapRequirementResult(std::move(std::get<Is>(Values)))...);
+  }
+
+  FunctionType Function;
+  std::tuple<RequirementTypes...> Requirements;
+};
+
+/// Used to deduce the refactoring result type for the lambda that passed into
+/// createRefactoringRule.
+template <typename T> struct LambdaDeducer;
+template <typename T, typename R, typename... Args>
+struct LambdaDeducer<R (T::*)(Args...) const> {
+  using ReturnType = R;
+};
+
+} // end namespace internal
+} // end namespace refactoring_action_rules
+} // end namespace tooling
+} // end namespace clang
+
+#endif // LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_ACTION_RULES_INTERNAL_H

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

+//===--- RefactoringRuleContext.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_RULE_CONTEXT_H
+#define LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_RULE_CONTEXT_H
+
+#include "clang/Basic/SourceManager.h"
+
+namespace clang {
+namespace tooling {
+
+/// The refactoring rule context stores all of the inputs that might be needed
+/// by a refactoring action rule. It can create the specialized
+/// \c ASTRefactoringOperation or \c PreprocessorRefactoringOperation values
+/// that can be used by the refactoring action rules.
+///
+/// The following inputs are stored by the operation:
+///
+///   - SourceManager: a reference to a valid source manager.
+///
+///   - SelectionRange: an optional source selection ranges that can be used
+///     to represent a selection in an editor.
+class RefactoringRuleContext {
+public:
+  RefactoringRuleContext(const SourceManager &SM) : SM(SM) {}
+
+  const SourceManager &getSources() const { return SM; }
+
+  /// Returns the current source selection range as set by the
+  /// refactoring engine. Can be invalid.
+  SourceRange getSelectionRange() const { return SelectionRange; }
+
+  void setSelectionRange(SourceRange R) { SelectionRange = R; }
+
+private:
+  /// The source manager for the translation unit / file on which a refactoring
+  /// action might operate on.
+  const SourceManager &SM;
+  /// An optional source selection range that's commonly used to represent
+  /// a selection in an editor.
+  SourceRange SelectionRange;
+};
+
+} // end namespace tooling
+} // end namespace clang
+
+#endif // LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_RULE_CONTEXT_H

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

+//===--- SourceSelectionConstraints.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_SOURCE_SELECTION_CONSTRAINTS_H
+#define LLVM_CLANG_TOOLING_REFACTOR_SOURCE_SELECTION_CONSTRAINTS_H
+
+#include "clang/Basic/LLVM.h"
+#include "clang/Basic/SourceLocation.h"
+#include <type_traits>
+
+namespace clang {
+namespace tooling {
+
+class RefactoringRuleContext;
+
+namespace selection {
+
+/// This constraint is satisfied when any portion of the source text is
+/// selected. It can be used to obtain the raw source selection range.
+struct SourceSelectionRange {
+  SourceSelectionRange(const SourceManager &SM, SourceRange Range)
+      : SM(SM), Range(Range) {}
+
+  const SourceManager &getSources() const { return SM; }
+  SourceRange getRange() const { return Range; }
+
+  static Optional<SourceSelectionRange>
+  evaluate(RefactoringRuleContext &Context);
+
+private:
+  const SourceManager &SM;
+  SourceRange Range;
+};
+
+/// A custom selection requirement.
+class Requirement {
+  /// Subclasses must implement 'T evaluateSelection(SelectionConstraint) const'
+  /// member function. \c T is used to determine the return type that is
+  /// passed to the refactoring rule's function.
+  /// If T is \c DiagnosticOr<S> , then \c S is passed to the rule's function
+  /// using move semantics.
+  /// Otherwise, T is passed to the function directly using move semantics.
+  ///
+  /// The different return type rules allow refactoring actions to fail
+  /// initiation when the relevant portions of AST aren't selected.
+};
+
+namespace traits {
+
+/// A type trait that returns true iff the given type is a valid selection
+/// constraint.
+template <typename T> struct IsConstraint : public std::false_type {};
+
+} // end namespace traits
+
+namespace internal {
+
+template <typename T> struct EvaluateSelectionChecker : std::false_type {};
+
+template <typename T, typename R, typename A>
+struct EvaluateSelectionChecker<R (T::*)(A) const> : std::true_type {
+  using ReturnType = R;
+  using ArgType = A;
+};
+
+template <typename T> class Identity : public Requirement {
+public:
+  T evaluateSelection(T Value) const { return std::move(Value); }
+};
+
+} // end namespace internal
+
+/// A identity function that returns the given selection constraint is provided
+/// for convenience, as it can be passed to \c requiredSelection directly.
+template <typename T> internal::Identity<T> identity() {
+  static_assert(
+      traits::IsConstraint<T>::value,
+      "selection::identity can be used with selection constraints only");
+  return internal::Identity<T>();
+}
+
+namespace traits {
+
+template <>
+struct IsConstraint<SourceSelectionRange> : public std::true_type {};
+
+/// A type trait that returns true iff \c T is a valid selection requirement.
+template <typename T>
+struct IsRequirement
+    : std::conditional<
+          std::is_base_of<Requirement, T>::value &&
+              internal::EvaluateSelectionChecker<decltype(
+                  &T::evaluateSelection)>::value &&
+              IsConstraint<typename internal::EvaluateSelectionChecker<decltype(
+                  &T::evaluateSelection)>::ArgType>::value,
+          std::true_type, std::false_type>::type {};
+
+} // end namespace traits
+} // end namespace selection
+} // end namespace tooling
+} // end namespace clang
+
+#endif // LLVM_CLANG_TOOLING_REFACTOR_SOURCE_SELECTION_CONSTRAINTS_H

cfe/trunk/lib/Tooling/Refactoring/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS Support)
 
 add_clang_library(clangToolingRefactor
   ASTSelection.cpp
   AtomicChange.cpp
   Rename/RenamingAction.cpp
   Rename/SymbolOccurrences.cpp
   Rename/USRFinder.cpp
   Rename/USRFindingAction.cpp
   Rename/USRLocFinder.cpp
+  SourceSelectionConstraints.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangFormat
   clangIndex
   clangLex
   clangToolingCore
   )

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

+//===--- SourceSelectionConstraints.cpp - Evaluate selection constraints --===//
+//
+//                     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/SourceSelectionConstraints.h"
+#include "clang/Tooling/Refactoring/RefactoringRuleContext.h"
+
+using namespace clang;
+using namespace tooling;
+using namespace selection;
+
+Optional<SourceSelectionRange>
+SourceSelectionRange::evaluate(RefactoringRuleContext &Context) {
+  SourceRange R = Context.getSelectionRange();
+  if (R.isInvalid())
+    return None;
+  return SourceSelectionRange(Context.getSources(), R);
+}

cfe/trunk/unittests/Tooling/CMakeLists.txt

add_clang_unittest(ToolingTests
   LexicallyOrderedRecursiveASTVisitorTest.cpp
   LookupTest.cpp
   QualTypeNamesTest.cpp
   RecursiveASTVisitorTest.cpp
   RecursiveASTVisitorTestCallVisitor.cpp
   RecursiveASTVisitorTestDeclVisitor.cpp
   RecursiveASTVisitorTestExprVisitor.cpp
   RecursiveASTVisitorTestTypeLocVisitor.cpp
+  RefactoringActionRulesTest.cpp
   RefactoringCallbacksTest.cpp
   RefactoringTest.cpp
   ReplacementsYamlTest.cpp
   RewriterTest.cpp
   ToolingTest.cpp
   )
 
 target_link_libraries(ToolingTests

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/Tooling.h"
+#include "llvm/Support/Errc.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace tooling;
+using namespace refactoring_action_rules;
+
+namespace {
+
+class RefactoringActionRulesTest : public ::testing::Test {
+protected:
+  void SetUp() override {
+    Context.Sources.setMainFileID(
+        Context.createInMemoryFile("input.cpp", DefaultCode));
+  }
+
+  RewriterTestContext Context;
+  std::string DefaultCode = std::string(100, 'a');
+};
+
+Expected<Optional<AtomicChanges>>
+createReplacements(const std::unique_ptr<RefactoringActionRule> &Rule,
+                   RefactoringRuleContext &Context) {
+  return cast<SourceChangeRefactoringRule>(*Rule).createSourceReplacements(
+      Context);
+}
+
+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(
+        Selection.second);
+    AtomicChange Change(SM, Loc);
+    llvm::Error E = Change.replace(SM, Loc, 1, "b");
+    if (E)
+      return std::move(E);
+    return AtomicChanges{Change};
+  };
+  class SelectionRequirement : public selection::Requirement {
+  public:
+    std::pair<selection::SourceSelectionRange, int>
+    evaluateSelection(selection::SourceSelectionRange Selection) const {
+      return std::make_pair(Selection, 20);
+    }
+  };
+  auto Rule = createRefactoringRule(ReplaceAWithB,
+                                    requiredSelection(SelectionRequirement()));
+
+  // 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 =
+        createReplacements(Rule, RefContext);
+    ASSERT_FALSE(!ErrorOrResult);
+    ASSERT_FALSE(!*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.
+  {
+    RefactoringRuleContext RefContext(Context.Sources);
+    Expected<Optional<AtomicChanges>> ErrorOrResult =
+        createReplacements(Rule, RefContext);
+
+    ASSERT_FALSE(!ErrorOrResult);
+    Optional<AtomicChanges> Value = std::move(*ErrorOrResult);
+    EXPECT_TRUE(!Value);
+  }
+}
+
+TEST_F(RefactoringActionRulesTest, ReturnError) {
+  Expected<AtomicChanges> (*Func)(selection::SourceSelectionRange) =
+      [](selection::SourceSelectionRange) -> Expected<AtomicChanges> {
+    return llvm::make_error<llvm::StringError>(
+        "Error", std::make_error_code(std::errc::bad_message));
+  };
+  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);
+
+  ASSERT_TRUE(!Result);
+  std::string Message;
+  llvm::handleAllErrors(Result.takeError(), [&](llvm::StringError &Error) {
+    Message = Error.getMessage();
+  });
+  EXPECT_EQ(Message, "Error");
+}
+
+TEST_F(RefactoringActionRulesTest, ReturnInitiationDiagnostic) {
+  RefactoringRuleContext RefContext(Context.Sources);
+  class SelectionRequirement : public selection::Requirement {
+  public:
+    Expected<Optional<int>>
+    evaluateSelection(selection::SourceSelectionRange Selection) const {
+      return llvm::make_error<llvm::StringError>(
+          "bad selection", std::make_error_code(std::errc::bad_message));
+    }
+  };
+  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);
+
+  ASSERT_TRUE(!Result);
+  std::string Message;
+  llvm::handleAllErrors(Result.takeError(), [&](llvm::StringError &Error) {
+    Message = Error.getMessage();
+  });
+  EXPECT_EQ(Message, "bad selection");
+}
+
+} // end anonymous namespace