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

This commit 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.
- simple literals and references are not extractable.

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

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


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

include/clang/Basic/DiagnosticRefactoringKinds.td

@@ -19,6 +19,13 @@ 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">;
 
 }
 

include/clang/Tooling/Refactoring/ASTSelection.h

@@ -115,6 +115,21 @@ public:
     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);
 

include/clang/Tooling/Refactoring/RefactoringActionRegistry.def

@@ -3,5 +3,6 @@
 #endif
 
 REFACTORING_ACTION(LocalRename)
+REFACTORING_ACTION(Extract)
 
 #undef REFACTORING_ACTION

include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h

@@ -11,6 +11,7 @@
 #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"
@@ -52,6 +53,31 @@ public:
   }
 };
 
+/// 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 the selection range overlaps
+/// with a number of neighbouring statements in the AST. The statemenst must be
+/// contained in declaration like a function. The selection range must be 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 CodeRangeASTSelectionRequirement : 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:

include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h

@@ -57,7 +57,7 @@ void invokeRuleAfterValidatingRequirements(
     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 &) {}

include/clang/Tooling/Refactoring/RefactoringRuleContext.h

@@ -12,6 +12,7 @@
 
 #include "clang/Basic/DiagnosticError.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Refactoring/ASTSelection.h"
 
 namespace clang {
 
@@ -62,6 +63,10 @@ public:
     return createDiagnosticError(SourceLocation(), DiagID);
   }
 
+  void setASTSelection(std::unique_ptr<SelectedASTNode> Node) {
+    ASTNodeSelection = std::move(Node);
+  }
+
 private:
   /// The source manager for the translation unit / file on which a refactoring
   /// action might operate on.
@@ -74,6 +79,9 @@ private:
   ASTContext *AST = nullptr;
   /// The allocator for diagnostics.
   PartialDiagnostic::StorageAllocator DiagStorage;
+
+  // FIXME: Remove when memoized.
+  std::unique_ptr<SelectedASTNode> ASTNodeSelection;
 };
 
 } // end namespace tooling

lib/Tooling/Refactoring/ASTSelection.cpp

@@ -322,6 +322,10 @@ CodeRangeASTSelection::create(SourceRange SelectionRange,
     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
@@ -330,3 +334,36 @@ CodeRangeASTSelection::create(SourceRange SelectionRange,
   return CodeRangeASTSelection(Selected.Node, Selected.Parents,
                                /*AreChildrenSelected=*/true);
 }
+
+bool CodeRangeASTSelection::isInFunctionLikeBodyOfCode() const {
+  bool IsPrevCompound = false;
+  // Scan through the parents (bottom-to-top) and check if the selection is
+  // contained in a compound statement that's a body of a function/method
+  // declaration.
+  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

@@ -0,0 +1,48 @@
+//===--- 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> CodeRangeASTSelectionRequirement::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

@@ -2,7 +2,9 @@ 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

lib/Tooling/Refactoring/Extract.cpp

@@ -0,0 +1,232 @@
+//===--- 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 CodeRangeASTSelectionRequirement {
+public:
+  Expected<CodeRangeASTSelection>
+  evaluate(RefactoringRuleContext &Context) const {
+    Expected<CodeRangeASTSelection> Selection =
+        CodeRangeASTSelectionRequirement::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 "(WIP action; use with caution!) 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

@@ -0,0 +1,56 @@
+// RUN: clang-refactor extract -selection=test:%s %s -- -std=c++11 2>&1 | grep -v CHECK | 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

@@ -1,9 +1,11 @@
-// RUN: clang-refactor local-rename -selection=test:%s -new-name=Bar %s -- | FileCheck %s
+// RUN: clang-refactor local-rename -selection=test:%s -new-name=Bar %s -- | grep -v CHECK | 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/LocalRename/NoSymbolSelectedError.cpp

@@ -1,5 +1,5 @@
-// RUN: not clang-refactor local-rename -selection=%s:4:1 -new-name=Bar %s -- 2>&1 | FileCheck %s
+// RUN: not clang-refactor local-rename -selection=%s:4:1 -new-name=Bar %s -- 2>&1 | grep -v CHECK | FileCheck %s
-// RUN: clang-refactor local-rename -selection=test:%s -new-name=Bar %s -- 2>&1 | FileCheck --check-prefix=TESTCHECK %s
+// RUN: clang-refactor local-rename -selection=test:%s -new-name=Bar %s -- 2>&1 | grep -v CHECK | FileCheck --check-prefix=TESTCHECK %s
 
 class Baz { // CHECK: [[@LINE]]:1: error: there is no symbol at the given location
 };

test/Refactor/tool-test-support.c

@@ -10,10 +10,13 @@
 
 /*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
@@ -29,6 +32,8 @@
 // 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

tools/clang-refactor/TestSupport.cpp

@@ -20,6 +20,7 @@
 #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"
@@ -241,9 +242,9 @@ bool TestRefactoringResultConsumer::handleAllResults() {
     // 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";
@@ -271,6 +272,25 @@ static unsigned addColumnOffset(StringRef Source, unsigned 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 =
@@ -282,11 +302,11 @@ findTestSelectionRanges(StringRef Filename) {
   }
   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;
 
@@ -304,18 +324,22 @@ findTestSelectionRanges(StringRef Filename) {
     StringRef Comment =
         Source.substr(Tok.getLocation().getRawEncoding(), Tok.getLength());
     SmallVector<StringRef, 4> Matches;
-    // Allow CHECK: comments to contain range= commands.
+    // Try to detect mistyped 'range:' comments to ensure tests don't miss
-    if (!RangeRegex.match(Comment, &Matches) || Comment.contains("CHECK")) {
+    // anything.
-      // Try to detect mistyped 'range:' comments to ensure tests don't miss
+    auto DetectMistypedCommand = [&]() -> bool {
-      // anything.
       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();
@@ -325,9 +349,28 @@ findTestSelectionRanges(StringRef Filename) {
       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)