[refactor] Initial outline of implementation of "extract function" refactoring

[arphaman]

Summary

This patch adds an initial, skeleton outline of the "extract function" refactoring.
The extracted function doesn't capture variables / rewrite code yet, it just basically does a simple copy-paste.
The following initiation rules are specified:

• extraction can only be done for executable code in a function/method/block. This means that you can't extract a global variable initialize into a function right now (should this be allowed though?).
• simple literals and references are not extractable.

This patch also adds support for full source ranges to clang-refactor's test mode.

[ioeric]

Code looks good in general. I see there are a bunch of major features missing; it might make sense to print a warning or document the major missing features in the high-level API.

include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h

74

Could we find a better name for this? It's not clear what the difference between this and SourceRangeSelectionRequirement is from the names.

include/clang/Tooling/Refactoring/RefactoringRuleContext.h

84

Maybe ASTNodeSelection for clarity?

lib/Tooling/Refactoring/ASTSelection.cpp

340

A short explanation of the for-loop would be appreciated :)

lib/Tooling/Refactoring/Extract.cpp

167

An alternative way to get the source code is:

Lexer::getSourceText(
      CharSourceRange::getTokenRange(SM.getSpellingLoc(Start), SM.getSpellingLoc(End)),
      SM, Result.Context->getLangOpts());

tools/clang-refactor/TestSupport.cpp

106

Can we filter the CHECKs with sed in the test? Similar to https://github.com/llvm-mirror/clang-tools-extra/blob/master/test/change-namespace/simple-move.cpp#L1

[arphaman]

Address review comments.

[arphaman]

In D38982#900744, @ioeric wrote:

Code looks good in general. I see there are a bunch of major features missing; it might make sense to print a warning or document the major missing features in the high-level API.

I added a warning in the description of the action.

include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h

55–80

Renamed to CodeRangeASTSelectionRequirement

lib/Tooling/Refactoring/Extract.cpp

167

I will need to get the rewritten source, so I've started using it in the first patch.

tools/clang-refactor/TestSupport.cpp

106

Fixed.

[arphaman]

Fix diff

[ioeric]

Lgtm

include/clang/Basic/DiagnosticRefactoringKinds.td

24

nit: was this newline intended?

tools/clang-refactor/TestSupport.cpp

377

nit: curly braces in else-branch if if-branch has braces.

[arphaman]

include/clang/Basic/DiagnosticRefactoringKinds.td

24

Yeah, it separates the extraction errors from the others.

include/clang/Basic/DiagnosticRefactoringKinds.td

 let Component = "Refactoring" in {
 
 let CategoryName = "Refactoring Invocation Issue" in {
 
 def err_refactor_no_selection : Error<"refactoring action can't be initiated "
   "without a selection">;
 def err_refactor_selection_no_symbol : Error<"there is no symbol at the given "
   "location">;
+def err_refactor_selection_invalid_ast : Error<"the provided selection does "
+  "not overlap with the AST nodes of interest">;
+
+def err_refactor_code_outside_of_function : Error<"the selected code is not a "
+  "part of a function's / method's body">;
+def err_refactor_extract_simple_expression : Error<"the selected expression "
+  "is too simple to extract">;
 
 }
 
 } // end of Refactoring diagnostics

include/clang/Tooling/Refactoring/ASTSelection.h

public:
   const Stmt *operator[](size_t I) const {
     if (!AreChildrenSelected) {
       assert(I == 0 && "Invalid index");
       return SelectedNode.get().Node.get<Stmt>();
     }
     return SelectedNode.get().Children[I].Node.get<Stmt>();
   }
 
+  /// Returns true when a selected code range is in a function-like body
+  /// of code, like a function, method or a block.
+  ///
+  /// This function can be used to test against selected expressions that are
+  /// located outside of a function, e.g. global variable initializers, default
+  /// argument values, or even template arguments.
+  ///
+  /// Use the \c getFunctionLikeNearestParent to get the function-like parent
+  /// declaration.
+  bool isInFunctionLikeBodyOfCode() const;
+
+  /// Returns the nearest function-like parent declaration or null if such
+  /// declaration doesn't exist.
+  const Decl *getFunctionLikeNearestParent() const;
+
   static Optional<CodeRangeASTSelection>
   create(SourceRange SelectionRange, const SelectedASTNode &ASTSelection);
 
 private:
   CodeRangeASTSelection(SelectedASTNode::ReferenceType SelectedNode,
                         ArrayRef<SelectedASTNode::ReferenceType> Parents,
                         bool AreChildrenSelected)
       : SelectedNode(SelectedNode), Parents(Parents.begin(), Parents.end()),

include/clang/Tooling/Refactoring/RefactoringActionRegistry.def

 #ifndef REFACTORING_ACTION
 #define REFACTORING_ACTION(Name)
 #endif
 
 REFACTORING_ACTION(LocalRename)
+REFACTORING_ACTION(Extract)
 
 #undef REFACTORING_ACTION

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/Basic/LLVM.h"
+#include "clang/Tooling/Refactoring/ASTSelection.h"
 #include "clang/Tooling/Refactoring/RefactoringDiagnostic.h"
 #include "clang/Tooling/Refactoring/RefactoringOption.h"
 #include "clang/Tooling/Refactoring/RefactoringRuleContext.h"
 #include "llvm/Support/Error.h"
 #include <type_traits>
 
 namespace clang {
 namespace tooling {

 class SourceRangeSelectionRequirement : public SourceSelectionRequirement {
 public:
   Expected<SourceRange> evaluate(RefactoringRuleContext &Context) const {
     if (Context.getSelectionRange().isValid())
       return Context.getSelectionRange();
     return Context.createDiagnosticError(diag::err_refactor_no_selection);
   }
 };
 
+/// An AST selection requirement is satisfied when any portion of the AST
+/// overlaps with the selection range.
+///
+/// The requirement will be evaluated only once during the initiation and
+/// search of matching refactoring action rules.
+class ASTSelectionRequirement : public SourceRangeSelectionRequirement {
+public:
+  Expected<SelectedASTNode> evaluate(RefactoringRuleContext &Context) const;
+};
+
+/// A selection requirement that is satisfied when a portion of code in a
+/// declaration like function is selected using a non-empty source selection
+/// (i.e. cursors won't be accepted).
+///
+/// The requirement will be evaluated only once during the initiation and search
+/// of matching refactoring action rules.
+///
+/// \see CodeRangeASTSelection
+class CodeRangeSelectionRequirement : public ASTSelectionRequirement {
+public:
+  Expected<CodeRangeASTSelection>
+  evaluate(RefactoringRuleContext &Context) const;
+};
+
 /// A base class for any requirement that requires some refactoring options.
 class RefactoringOptionsRequirement : public RefactoringActionRuleRequirement {
 public:
   virtual ~RefactoringOptionsRequirement() {}
 
   /// Returns the set of refactoring options that are used when evaluating this
   /// requirement.
   virtual ArrayRef<std::shared_ptr<RefactoringOption>>
   getRefactoringOptions() const = 0;

include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h

void invokeRuleAfterValidatingRequirements(
   // Check if the requirements we're interested in can be evaluated.
   auto Values =
       std::make_tuple(std::get<Is>(Requirements).evaluate(Context)...);
   auto Err = findError(std::get<Is>(Values)...);
   if (Err)
     return Consumer.handleError(std::move(Err));
   // Construct the target action rule by extracting the evaluated
   // requirements from Expected<> wrappers and then run it.
-  RuleType((*std::get<Is>(Values))...).invoke(Consumer, Context);
+  RuleType(std::move((*std::get<Is>(Values)))...).invoke(Consumer, Context);
 }
 
 inline void visitRefactoringOptionsImpl(RefactoringOptionVisitor &) {}
 
 /// Scans the list of requirements in a rule and visits all the refactoring
 /// options that are used by all the requirements.
 template <typename FirstT, typename... RestT>
 void visitRefactoringOptionsImpl(RefactoringOptionVisitor &Visitor,

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/DiagnosticError.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Refactoring/ASTSelection.h"
 
 namespace clang {
 
 class ASTContext;
 
 namespace tooling {
 
 /// The refactoring rule context stores all of the inputs that might be needed
public:
   llvm::Error createDiagnosticError(SourceLocation Loc, unsigned DiagID) {
     return DiagnosticError::create(Loc, PartialDiagnostic(DiagID, DiagStorage));
   }
 
   llvm::Error createDiagnosticError(unsigned DiagID) {
     return createDiagnosticError(SourceLocation(), DiagID);
   }
 
+  void setASTSelection(std::unique_ptr<SelectedASTNode> Node) {
+    ASTSelection = std::move(Node);
+  }
+
 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;
   /// An optional AST for the translation unit on which a refactoring action
   /// might operate on.
   ASTContext *AST = nullptr;
   /// The allocator for diagnostics.
   PartialDiagnostic::StorageAllocator DiagStorage;
+
+  // FIXME: Remove when memoized.
+  std::unique_ptr<SelectedASTNode> ASTSelection;
 };
 
 } // end namespace tooling
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_RULE_CONTEXT_H

lib/Tooling/Refactoring/ASTSelection.cpp

CodeRangeASTSelection::create(SourceRange SelectionRange,
   if (!Selected.Node.get().Node.get<Stmt>())
     return None;
   const Stmt *CodeRangeStmt = Selected.Node.get().Node.get<Stmt>();
   if (!isa<CompoundStmt>(CodeRangeStmt)) {
     // FIXME (Alex L): Canonicalize.
     return CodeRangeASTSelection(Selected.Node, Selected.Parents,
                                  /*AreChildrenSelected=*/false);
   }
+  // FIXME (Alex L): First selected SwitchCase means that first case statement.
+  // is selected actually
+  // (See https://github.com/apple/swift-clang & CompoundStmtRange).
+
   // FIXME (Alex L): Tweak selection rules for compound statements, see:
   // https://github.com/apple/swift-clang/blob/swift-4.1-branch/lib/Tooling/
   // Refactor/ASTSlice.cpp#L513
   // The user selected multiple statements in a compound statement.
   Selected.Parents.push_back(Selected.Node);
   return CodeRangeASTSelection(Selected.Node, Selected.Parents,
                                /*AreChildrenSelected=*/true);
 }
+
+bool CodeRangeASTSelection::isInFunctionLikeBodyOfCode() const {
+  bool IsPrevCompound = false;
+  for (const auto &Parent : llvm::reverse(Parents)) {
+    const DynTypedNode &Node = Parent.get().Node;
+    if (const auto *D = Node.get<Decl>()) {
+      // FIXME (Alex L): Test for BlockDecl && ObjCMethodDecl.
+      if (isa<FunctionDecl>(D))
+        return IsPrevCompound;
+      // FIXME (Alex L): We should return false on top-level decls in functions
+      // e.g. we don't want to extract:
+      // function foo() { struct X {
+      //   int m = /*selection:*/ 1 + 2 /*selection end*/; }; };
+    }
+    IsPrevCompound = Node.get<CompoundStmt>() != nullptr;
+  }
+  return false;
+}
+
+const Decl *CodeRangeASTSelection::getFunctionLikeNearestParent() const {
+  for (const auto &Parent : llvm::reverse(Parents)) {
+    const DynTypedNode &Node = Parent.get().Node;
+    if (const auto *D = Node.get<Decl>()) {
+      // FIXME (Alex L): Test for BlockDecl && ObjCMethodDecl.
+      if (isa<FunctionDecl>(D))
+        return D;
+    }
+  }
+  return nullptr;
+}

lib/Tooling/Refactoring/ASTSelectionRequirements.cpp

+//===--- ASTSelectionRequirements.cpp - Clang refactoring library ---------===//
+//
+//                     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/RefactoringActionRuleRequirements.h"
+
+using namespace clang;
+using namespace tooling;
+
+Expected<SelectedASTNode>
+ASTSelectionRequirement::evaluate(RefactoringRuleContext &Context) const {
+  // FIXME: Memoize so that selection is evaluated only once.
+  Expected<SourceRange> Range =
+      SourceRangeSelectionRequirement::evaluate(Context);
+  if (!Range)
+    return Range.takeError();
+
+  Optional<SelectedASTNode> Selection =
+      findSelectedASTNodes(Context.getASTContext(), *Range);
+  if (!Selection)
+    return Context.createDiagnosticError(
+        Range->getBegin(), diag::err_refactor_selection_invalid_ast);
+  return std::move(*Selection);
+}
+
+Expected<CodeRangeASTSelection>
+CodeRangeSelectionRequirement::evaluate(RefactoringRuleContext &Context) const {
+  // FIXME: Memoize so that selection is evaluated only once.
+  Expected<SelectedASTNode> ASTSelection =
+      ASTSelectionRequirement::evaluate(Context);
+  if (!ASTSelection)
+    return ASTSelection.takeError();
+  std::unique_ptr<SelectedASTNode> StoredSelection =
+      llvm::make_unique<SelectedASTNode>(std::move(*ASTSelection));
+  Optional<CodeRangeASTSelection> CodeRange = CodeRangeASTSelection::create(
+      Context.getSelectionRange(), *StoredSelection);
+  if (!CodeRange)
+    return Context.createDiagnosticError(
+        Context.getSelectionRange().getBegin(),
+        diag::err_refactor_selection_invalid_ast);
+  Context.setASTSelection(std::move(StoredSelection));
+  return std::move(*CodeRange);
+}

lib/Tooling/Refactoring/CMakeLists.txt

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

lib/Tooling/Refactoring/Extract.cpp

+//===--- Extract.cpp - Clang refactoring library --------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// \brief Implements the "extract" refactoring that can pull code into
+/// new functions, methods or declare new variables.
+///
+//===----------------------------------------------------------------------===//
+
+#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.
+bool isSimpleExpression(const Expr *E) {
+  if (!E)
+    return false;
+  switch (E->IgnoreParenCasts()->getStmtClass()) {
+  case Stmt::DeclRefExprClass:
+  case Stmt::PredefinedExprClass:
+  case Stmt::IntegerLiteralClass:
+  case Stmt::FloatingLiteralClass:
+  case Stmt::ImaginaryLiteralClass:
+  case Stmt::CharacterLiteralClass:
+  case Stmt::StringLiteralClass:
+    return true;
+  default:
+    return false;
+  }
+}
+
+class ExtractableCodeSelectionRequirement final
+    : public CodeRangeSelectionRequirement {
+public:
+  Expected<CodeRangeASTSelection>
+  evaluate(RefactoringRuleContext &Context) const {
+    Expected<CodeRangeASTSelection> Selection =
+        CodeRangeSelectionRequirement::evaluate(Context);
+    if (!Selection)
+      return Selection.takeError();
+    CodeRangeASTSelection &Code = *Selection;
+
+    // We would like to extract code out of functions/methods/blocks.
+    // Prohibit extraction from things like global variable / field
+    // initializers and other top-level expressions.
+    if (!Code.isInFunctionLikeBodyOfCode())
+      return Context.createDiagnosticError(
+          diag::err_refactor_code_outside_of_function);
+
+    // Avoid extraction of simple literals and references.
+    if (Code.size() == 1 && isSimpleExpression(dyn_cast<Expr>(Code[0])))
+      return Context.createDiagnosticError(
+          diag::err_refactor_extract_simple_expression);
+
+    // FIXME (Alex L): Prohibit extraction of Objective-C property setters.
+    return Selection;
+  }
+};
+
+class ExtractFunction final : public SourceChangeRefactoringRule {
+public:
+  ExtractFunction(CodeRangeASTSelection Code, Optional<std::string> DeclName)
+      : Code(std::move(Code)),
+        DeclName(DeclName ? std::move(*DeclName) : "extracted") {}
+
+  Expected<AtomicChanges>
+  createSourceReplacements(RefactoringRuleContext &Context) override;
+
+private:
+  CodeRangeASTSelection Code;
+
+  // FIXME: Account for naming collisions:
+  //  - error when name is specified by user.
+  //  - rename to "extractedN" when name is implicit.
+  std::string DeclName;
+};
+
+SourceLocation computeFunctionExtractionLocation(const Decl *D) {
+  // FIXME (Alex L): Method -> function extraction should place function before
+  // C++ record if the method is defined inside the record.
+  return D->getLocStart();
+}
+
+// FIXME: Support C++ method extraction.
+// FIXME: Support Objective-C method extraction.
+Expected<AtomicChanges>
+ExtractFunction::createSourceReplacements(RefactoringRuleContext &Context) {
+  const Decl *ParentDecl = Code.getFunctionLikeNearestParent();
+  assert(ParentDecl && "missing parent");
+
+  // Compute the source range of the code that should be extracted.
+  SourceRange ExtractedRange(Code[0]->getLocStart(),
+                             Code[Code.size() - 1]->getLocEnd());
+  // FIXME (Alex L): Add code that accounts for macro locations.
+
+  ASTContext &AST = Context.getASTContext();
+  SourceManager &SM = AST.getSourceManager();
+  const LangOptions &LangOpts = AST.getLangOpts();
+  Rewriter ExtractedCodeRewriter(SM, LangOpts);
+
+  // FIXME: Capture used variables.
+
+  // Compute the return type.
+  QualType ReturnType = AST.VoidTy;
+  // FIXME (Alex L): Account for the return statement in extracted code.
+  // FIXME (Alex L): Check for lexical expression instead.
+  bool IsExpr = Code.size() == 1 && isa<Expr>(Code[0]);
+  if (IsExpr) {
+    // FIXME (Alex L): Get a more user-friendly type if needed.
+    ReturnType = cast<Expr>(Code[0])->getType();
+  }
+
+  // FIXME: Rewrite the extracted code performing any required adjustments.
+
+  // FIXME: Capture any field if necessary (method -> function extraction).
+
+  // FIXME: Sort captured variables by name.
+
+  // FIXME: Capture 'this' / 'self' if necessary.
+
+  // FIXME: Compute the actual parameter types.
+
+  // Compute the location of the extracted declaration.
+  SourceLocation ExtractedDeclLocation =
+      computeFunctionExtractionLocation(ParentDecl);
+  // FIXME: Adjust the location to account for any preceding comments.
+
+  // FIXME: Adjust with PP awareness like in Sema to get correct 'bool'
+  // treatment.
+  PrintingPolicy PP = AST.getPrintingPolicy();
+  // FIXME: PP.UseStdFunctionForLambda = true;
+  PP.SuppressStrongLifetime = true;
+  PP.SuppressLifetimeQualifiers = true;
+  PP.SuppressUnwrittenScope = true;
+
+  AtomicChange Change(SM, ExtractedDeclLocation);
+  // Create the replacement for the extracted declaration.
+  {
+    std::string ExtractedCode;
+    llvm::raw_string_ostream OS(ExtractedCode);
+    // FIXME: Use 'inline' in header.
+    OS << "static ";
+    ReturnType.print(OS, PP, DeclName);
+    OS << '(';
+    // FIXME: Arguments.
+    OS << ')';
+
+    // Function body.
+    OS << " {\n";
+    if (IsExpr && !ReturnType->isVoidType())
+      OS << "return ";
+    OS << ExtractedCodeRewriter.getRewrittenText(ExtractedRange);
+    // FIXME: Compute the correct semicolon policy.
+    OS << ';';
+    OS << "\n}\n\n";
+    auto Err = Change.insert(SM, ExtractedDeclLocation, OS.str());
+    if (Err)
+      return std::move(Err);
+  }
+
+  // Create the replacement for the call to the extracted declaration.
+  {
+    std::string ReplacedCode;
+    llvm::raw_string_ostream OS(ReplacedCode);
+
+    OS << DeclName << '(';
+    // FIXME: Forward arguments.
+    OS << ')';
+    // FIXME: Add semicolon if needed.
+
+    auto Err = Change.replace(
+        SM, CharSourceRange::getTokenRange(ExtractedRange), OS.str());
+    if (Err)
+      return std::move(Err);
+  }
+
+  // FIXME: Add support for assocciated symbol location to AtomicChange to mark
+  // the ranges of the name of the extracted declaration.
+  return AtomicChanges{std::move(Change)};
+}
+
+class DeclNameOption final : public OptionalRefactoringOption<std::string> {
+public:
+  StringRef getName() const { return "name"; }
+  StringRef getDescription() const {
+    return "Name of the extracted declaration";
+  }
+};
+
+class ExtractRefactoring final : public RefactoringAction {
+public:
+  StringRef getCommand() const override { return "extract"; }
+
+  StringRef getDescription() const override {
+    return "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>(
+        ExtractableCodeSelectionRequirement(),
+        OptionRequirement<DeclNameOption>()));
+    return Rules;
+  }
+};
+
+} // end anonymous namespace
+
+std::unique_ptr<RefactoringAction> createExtractAction() {
+  return llvm::make_unique<ExtractRefactoring>();
+}
+
+} // end namespace tooling
+} // end namespace clang

test/Refactor/Extract/ExtractExprIntoFunction.cpp

+// RUN: clang-refactor extract -selection=test:%s %s -- -std=c++11 2>&1 | FileCheck %s
+
+
+void simpleExtractNoCaptures() {
+  int i = /*range=->+0:33*/1 + 2;
+}
+
+// CHECK: 1 '' results:
+// CHECK:      static int extracted() {
+// CHECK-NEXT: return 1 + 2;{{$}}
+// CHECK-NEXT: }{{[[:space:]].*}}
+// CHECK-NEXT: void simpleExtractNoCaptures() {
+// CHECK-NEXT:   int i = /*range=->+0:33*/extracted();{{$}}
+// CHECK-NEXT: }
+
+void simpleExtractStmtNoCaptures() {
+  /*range astatement=->+1:13*/int a = 1;
+  int b = 2;
+}
+// CHECK: 1 'astatement' results:
+// CHECK:      static void extracted() {
+// CHECK-NEXT: int a = 1;
+// CHECK-NEXT: int b = 2;;{{$}}
+// CHECK-NEXT: }{{[[:space:]].*}}
+// CHECK-NEXT: void simpleExtractStmtNoCaptures() {
+// CHECK-NEXT:   /*range astatement=->+1:13*/extracted(){{$}}
+// CHECK-NEXT: }
+
+
+void blankRangeNoExtraction() {
+  int i = /*range blank=*/1 + 2;
+}
+
+// CHECK: 1 'blank' results:
+// CHECK-NEXT: the provided selection does not overlap with the AST nodes of interest
+
+int outOfBodyCodeNoExtraction = /*range out_of_body_expr=->+0:72*/1 + 2;
+
+struct OutOfBodyStuff {
+  int FieldInit = /*range out_of_body_expr=->+0:58*/1 + 2;
+
+  void foo(int x =/*range out_of_body_expr=->+0:58*/1 + 2);
+};
+
+// CHECK: 3 'out_of_body_expr' results:
+// CHECK: the selected code is not a part of a function's / method's body
+
+void simpleExpressionNoExtraction() {
+  int i = /*range simple_expr=->+0:41*/1 + /*range simple_expr=->+0:76*/(2);
+  (void) /*range simple_expr=->+0:40*/i;
+  (void)/*range simple_expr=->+0:47*/"literal";
+  (void)/*range simple_expr=->+0:41*/'c';
+}
+
+// CHECK: 5 'simple_expr' results:
+// CHECK-NEXT: the selected expression is too simple to extract

test/Refactor/LocalRename/Field.cpp

 // RUN: clang-refactor local-rename -selection=test:%s -new-name=Bar %s -- | FileCheck %s
 
 class Baz {
-  int /*range=*/Foo; // CHECK: int /*range=*/Bar;
+  int /*range=*/Foo;
+  // CHECK: int /*range=*/Bar;
 public:
   Baz();
 };
 
-Baz::Baz() : /*range=*/Foo(0) {}  // CHECK: Baz::Baz() : /*range=*/Bar(0) {};
+Baz::Baz() : /*range=*/Foo(0) {}
+// CHECK: Baz::Baz() : /*range=*/Bar(0) {}

test/Refactor/tool-test-support.c

 // RUN: clang-refactor local-rename -selection=test:%s -new-name=test -v %s -- | FileCheck %s
 
 /*range=*/int test;
 
 /*range named=*/int test2;
 
 /*range= +1*/int test3;
 
 /* range = +100 */int test4;
 
 /*range named =+0*/int test5;
 
+/*range =->+0:22*/int test6;
+
 // CHECK: Test selection group '':
 // CHECK-NEXT:   105-105
 // CHECK-NEXT:   158-158
 // CHECK-NEXT:   197-197
+// CHECK-NEXT:   248-251
 // CHECK-NEXT: Test selection group 'named':
 // CHECK-NEXT:   132-132
 // CHECK-NEXT:   218-218
 
 // The following invocations are in the default group:
 
 // CHECK: invoking action 'local-rename':
 // CHECK-NEXT: -selection={{.*}}tool-test-support.c:3:11
 
 // CHECK: invoking action 'local-rename':
 // CHECK-NEXT: -selection={{.*}}tool-test-support.c:7:15
 
 // CHECK: invoking action 'local-rename':
 // CHECK-NEXT: -selection={{.*}}tool-test-support.c:9:29
 
+// CHECK: invoking action 'local-rename':
+// CHECK-NEXT: -selection={{.*}}tool-test-support.c:13:19 -> {{.*}}tool-test-support.c:13:22
 
 // The following invocations are in the 'named' group, and they follow
 // the default invocation even if some of their ranges occur prior to the
 // ranges from the default group because the groups are tested one-by-one:
 
 // CHECK: invoking action 'local-rename':
 // CHECK-NEXT: -selection={{.*}}tool-test-support.c:5:17
 
 // CHECK: invoking action 'local-rename':
 // CHECK-NEXT: -selection={{.*}}tool-test-support.c:11:20

tools/clang-refactor/TestSupport.cpp

 
 #include "TestSupport.h"
 #include "clang/Basic/DiagnosticError.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/LineIterator.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
 
 namespace clang {
 namespace refactor {
if (!BufferErr) {
       return true;
     }
     auto Result = tooling::applyAtomicChanges(File, (*BufferErr)->getBuffer(),
                                               Changes, Spec);
     if (!Result) {
       llvm::errs() << toString(Result.takeError());
       return true;
     }
-    OS << *Result;
+    auto Buf = llvm::MemoryBuffer::getMemBuffer(Result->c_str());
+    for (llvm::line_iterator It(*Buf, /*SkipBlanks=*/false); !It.is_at_end();
+         ++It) {
+      // Drop the 'CHECK' lines alltogether to avoid erroneous matches.
+      if (It->contains("CHECK"))
+        continue;
+      OS << *It << "\n";
+    }
   }
   return false;
 }
 
 class TestRefactoringResultConsumer final
     : public ClangRefactorToolConsumerInterface {
 public:
   TestRefactoringResultConsumer(const TestSelectionRangesInFile &TestRanges)
for (auto &I : llvm::enumerate(Group.value())) {
         llvm::errs() << "  Produced:\n";
         dumpChanges(*Result, llvm::errs());
       }
     }
 
     // Dump the results:
     const auto &TestGroup = TestRanges.GroupedRanges[Group.index()];
     if (!CanonicalResult) {
-      llvm::errs() << TestGroup.Ranges.size() << " '" << TestGroup.Name
+      llvm::outs() << TestGroup.Ranges.size() << " '" << TestGroup.Name
                    << "' results:\n";
-      llvm::errs() << *CanonicalErrorMessage << "\n";
+      llvm::outs() << *CanonicalErrorMessage << "\n";
     } else {
       llvm::outs() << TestGroup.Ranges.size() << " '" << TestGroup.Name
                    << "' results:\n";
       if (printRewrittenSources(*CanonicalResult, llvm::outs()))
         return true;
     }
   }
   return Failed;
static unsigned addColumnOffset(StringRef Source, unsigned Offset,
   if (!ColumnOffset)
     return Offset;
   StringRef Substr = Source.drop_front(Offset).take_front(ColumnOffset);
   size_t NewlinePos = Substr.find_first_of("\r\n");
   return Offset +
          (NewlinePos == StringRef::npos ? ColumnOffset : (unsigned)NewlinePos);
 }
 
+static unsigned addEndLineOffsetAndEndColumn(StringRef Source, unsigned Offset,
+                                             unsigned LineNumberOffset,
+                                             unsigned Column) {
+  StringRef Line = Source.drop_front(Offset);
+  unsigned LineOffset = 0;
+  for (; LineNumberOffset != 0; --LineNumberOffset) {
+    size_t NewlinePos = Line.find_first_of("\r\n");
+    // Line offset goes out of bounds.
+    if (NewlinePos == StringRef::npos)
+      break;
+    LineOffset += NewlinePos + 1;
+    Line = Line.drop_front(NewlinePos + 1);
+  }
+  // Source now points to the line at +lineOffset;
+  size_t LineStart = Source.find_last_of("\r\n", /*From=*/Offset + LineOffset);
+  return addColumnOffset(
+      Source, LineStart == StringRef::npos ? 0 : LineStart + 1, Column - 1);
+}
+
 Optional<TestSelectionRangesInFile>
 findTestSelectionRanges(StringRef Filename) {
   ErrorOr<std::unique_ptr<MemoryBuffer>> ErrOrFile =
       MemoryBuffer::getFile(Filename);
   if (!ErrOrFile) {
     llvm::errs() << "error: -selection=test:" << Filename
                  << " : could not open the given file";
     return None;
   }
   StringRef Source = ErrOrFile.get()->getBuffer();
 
-  // FIXME (Alex L): 3rd capture groups for +line:column.
   // See the doc comment for this function for the explanation of this
   // syntax.
   static Regex RangeRegex("range[[:blank:]]*([[:alpha:]_]*)?[[:blank:]]*=[[:"
-                          "blank:]]*(\\+[[:digit:]]+)?");
+                          "blank:]]*(\\+[[:digit:]]+)?[[:blank:]]*(->[[:blank:]"
+                          "]*[\\+\\:[:digit:]]+)?");
 
   std::map<std::string, SmallVector<TestSelectionRange, 8>> GroupedRanges;
 
   LangOptions LangOpts;
   LangOpts.CPlusPlus = 1;
   LangOpts.CPlusPlus11 = 1;
   Lexer Lex(SourceLocation::getFromRawEncoding(0), LangOpts, Source.begin(),
             Source.begin(), Source.end());
   Lex.SetCommentRetentionState(true);
   Token Tok;
   for (Lex.LexFromRawLexer(Tok); Tok.isNot(tok::eof);
        Lex.LexFromRawLexer(Tok)) {
     if (Tok.isNot(tok::comment))
       continue;
     StringRef Comment =
         Source.substr(Tok.getLocation().getRawEncoding(), Tok.getLength());
     SmallVector<StringRef, 4> Matches;
-    // Allow CHECK: comments to contain range= commands.
-    if (!RangeRegex.match(Comment, &Matches) || Comment.contains("CHECK")) {
-      // Try to detect mistyped 'range:' comments to ensure tests don't miss
-      // anything.
+    auto DetectMistypedCommand = [&]() -> bool {
       if (Comment.contains_lower("range") && Comment.contains("=") &&
           !Comment.contains_lower("run") && !Comment.contains("CHECK")) {
         llvm::errs() << "error: suspicious comment '" << Comment
                      << "' that "
                         "resembles the range command found\n";
         llvm::errs() << "note: please reword if this isn't a range command\n";
-        return None;
       }
+      return false;
+    };
+    // Allow CHECK: comments to contain range= commands.
+    if (!RangeRegex.match(Comment, &Matches) || Comment.contains("CHECK")) {
+      if (DetectMistypedCommand())
+        return None;
       continue;
     }
     unsigned Offset = Tok.getEndLoc().getRawEncoding();
     unsigned ColumnOffset = 0;
     if (!Matches[2].empty()) {
       // Don't forget to drop the '+'!
       if (Matches[2].drop_front().getAsInteger(10, ColumnOffset))
         assert(false && "regex should have produced a number");
     }
-    // FIXME (Alex L): Support true ranges.
     Offset = addColumnOffset(Source, Offset, ColumnOffset);
-    TestSelectionRange Range = {Offset, Offset};
+    unsigned EndOffset;
+
+    if (!Matches[3].empty()) {
+      static Regex EndLocRegex(
+          "->[[:blank:]]*(\\+[[:digit:]]+):([[:digit:]]+)");
+      SmallVector<StringRef, 4> EndLocMatches;
+      if (!EndLocRegex.match(Matches[3], &EndLocMatches)) {
+        if (DetectMistypedCommand())
+          return None;
+        continue;
+      }
+      unsigned EndLineOffset = 0, EndColumn = 0;
+      if (EndLocMatches[1].drop_front().getAsInteger(10, EndLineOffset) ||
+          EndLocMatches[2].getAsInteger(10, EndColumn))
+        assert(false && "regex should have produced a number");
+      EndOffset = addEndLineOffsetAndEndColumn(Source, Offset, EndLineOffset,
+                                               EndColumn);
+    } else
+      EndOffset = Offset;
+    TestSelectionRange Range = {Offset, EndOffset};
     auto It = GroupedRanges.insert(std::make_pair(
         Matches[1].str(), SmallVector<TestSelectionRange, 8>{Range}));
     if (!It.second)
       It.first->second.push_back(Range);
   }
   if (GroupedRanges.empty()) {
     llvm::errs() << "error: -selection=test:" << Filename
                  << ": no 'range' commands";