From 58538f115416ae2ce6cc4cfa794d243bade5458d Mon Sep 17 00:00:00 2001 From: Alex Lorenz Date: Tue, 24 Oct 2017 17:18:45 +0000 Subject: [PATCH] [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 --- .../clang/Basic/DiagnosticRefactoringKinds.td | 7 + .../clang/Tooling/Refactoring/ASTSelection.h | 15 ++ .../Refactoring/RefactoringActionRegistry.def | 1 + .../RefactoringActionRuleRequirements.h | 26 ++ .../RefactoringActionRulesInternal.h | 2 +- .../Refactoring/RefactoringRuleContext.h | 8 + lib/Tooling/Refactoring/ASTSelection.cpp | 37 +++ .../Refactoring/ASTSelectionRequirements.cpp | 48 ++++ lib/Tooling/Refactoring/CMakeLists.txt | 2 + lib/Tooling/Refactoring/Extract.cpp | 232 ++++++++++++++++++ .../Extract/ExtractExprIntoFunction.cpp | 56 +++++ test/Refactor/LocalRename/Field.cpp | 8 +- .../LocalRename/NoSymbolSelectedError.cpp | 4 +- test/Refactor/tool-test-support.c | 5 + tools/clang-refactor/TestSupport.cpp | 65 ++++- 15 files changed, 499 insertions(+), 17 deletions(-) create mode 100644 lib/Tooling/Refactoring/ASTSelectionRequirements.cpp create mode 100644 lib/Tooling/Refactoring/Extract.cpp create mode 100644 test/Refactor/Extract/ExtractExprIntoFunction.cpp diff --git a/include/clang/Basic/DiagnosticRefactoringKinds.td b/include/clang/Basic/DiagnosticRefactoringKinds.td index ba9c830d60..b54b9301a7 100644 --- a/include/clang/Basic/DiagnosticRefactoringKinds.td +++ b/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">; } diff --git a/include/clang/Tooling/Refactoring/ASTSelection.h b/include/clang/Tooling/Refactoring/ASTSelection.h index 17a97d2aaf..aa02a6899e 100644 --- a/include/clang/Tooling/Refactoring/ASTSelection.h +++ b/include/clang/Tooling/Refactoring/ASTSelection.h @@ -115,6 +115,21 @@ public: return SelectedNode.get().Children[I].Node.get(); } + /// 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 create(SourceRange SelectionRange, const SelectedASTNode &ASTSelection); diff --git a/include/clang/Tooling/Refactoring/RefactoringActionRegistry.def b/include/clang/Tooling/Refactoring/RefactoringActionRegistry.def index a1191f1e50..9aeead4148 100644 --- a/include/clang/Tooling/Refactoring/RefactoringActionRegistry.def +++ b/include/clang/Tooling/Refactoring/RefactoringActionRegistry.def @@ -3,5 +3,6 @@ #endif REFACTORING_ACTION(LocalRename) +REFACTORING_ACTION(Extract) #undef REFACTORING_ACTION diff --git a/include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h b/include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h index 36c982b063..355a6a55f2 100644 --- a/include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h +++ b/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 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 + evaluate(RefactoringRuleContext &Context) const; +}; + /// A base class for any requirement that requires some refactoring options. class RefactoringOptionsRequirement : public RefactoringActionRuleRequirement { public: diff --git a/include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h b/include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h index cf6147c0ba..443c7f86df 100644 --- a/include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h +++ b/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(Values))...).invoke(Consumer, Context); + RuleType(std::move((*std::get(Values)))...).invoke(Consumer, Context); } inline void visitRefactoringOptionsImpl(RefactoringOptionVisitor &) {} diff --git a/include/clang/Tooling/Refactoring/RefactoringRuleContext.h b/include/clang/Tooling/Refactoring/RefactoringRuleContext.h index 54ea06d861..882ab824b6 100644 --- a/include/clang/Tooling/Refactoring/RefactoringRuleContext.h +++ b/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 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 ASTNodeSelection; }; } // end namespace tooling diff --git a/lib/Tooling/Refactoring/ASTSelection.cpp b/lib/Tooling/Refactoring/ASTSelection.cpp index 2c9c42bfcb..9d0683a285 100644 --- a/lib/Tooling/Refactoring/ASTSelection.cpp +++ b/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()) { + // FIXME (Alex L): Test for BlockDecl && ObjCMethodDecl. + if (isa(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() != 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()) { + // FIXME (Alex L): Test for BlockDecl && ObjCMethodDecl. + if (isa(D)) + return D; + } + } + return nullptr; +} diff --git a/lib/Tooling/Refactoring/ASTSelectionRequirements.cpp b/lib/Tooling/Refactoring/ASTSelectionRequirements.cpp new file mode 100644 index 0000000000..c0232c5da4 --- /dev/null +++ b/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 +ASTSelectionRequirement::evaluate(RefactoringRuleContext &Context) const { + // FIXME: Memoize so that selection is evaluated only once. + Expected Range = + SourceRangeSelectionRequirement::evaluate(Context); + if (!Range) + return Range.takeError(); + + Optional Selection = + findSelectedASTNodes(Context.getASTContext(), *Range); + if (!Selection) + return Context.createDiagnosticError( + Range->getBegin(), diag::err_refactor_selection_invalid_ast); + return std::move(*Selection); +} + +Expected CodeRangeASTSelectionRequirement::evaluate( + RefactoringRuleContext &Context) const { + // FIXME: Memoize so that selection is evaluated only once. + Expected ASTSelection = + ASTSelectionRequirement::evaluate(Context); + if (!ASTSelection) + return ASTSelection.takeError(); + std::unique_ptr StoredSelection = + llvm::make_unique(std::move(*ASTSelection)); + Optional 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); +} diff --git a/lib/Tooling/Refactoring/CMakeLists.txt b/lib/Tooling/Refactoring/CMakeLists.txt index 43ea1d9c54..f30c679a92 100644 --- a/lib/Tooling/Refactoring/CMakeLists.txt +++ b/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 diff --git a/lib/Tooling/Refactoring/Extract.cpp b/lib/Tooling/Refactoring/Extract.cpp new file mode 100644 index 0000000000..616900c181 --- /dev/null +++ b/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 + evaluate(RefactoringRuleContext &Context) const { + Expected 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(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 DeclName) + : Code(std::move(Code)), + DeclName(DeclName ? std::move(*DeclName) : "extracted") {} + + Expected + 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 +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(Code[0]); + if (IsExpr) { + // FIXME (Alex L): Get a more user-friendly type if needed. + ReturnType = cast(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 { +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( + ExtractableCodeSelectionRequirement(), + OptionRequirement())); + return Rules; + } +}; + +} // end anonymous namespace + +std::unique_ptr createExtractAction() { + return llvm::make_unique(); +} + +} // end namespace tooling +} // end namespace clang diff --git a/test/Refactor/Extract/ExtractExprIntoFunction.cpp b/test/Refactor/Extract/ExtractExprIntoFunction.cpp new file mode 100644 index 0000000000..be610fc303 --- /dev/null +++ b/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 diff --git a/test/Refactor/LocalRename/Field.cpp b/test/Refactor/LocalRename/Field.cpp index 83ca2e5748..e674401b96 100644 --- a/test/Refactor/LocalRename/Field.cpp +++ b/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) {} diff --git a/test/Refactor/LocalRename/NoSymbolSelectedError.cpp b/test/Refactor/LocalRename/NoSymbolSelectedError.cpp index b6e96e11b7..98de61a45f 100644 --- a/test/Refactor/LocalRename/NoSymbolSelectedError.cpp +++ b/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: clang-refactor local-rename -selection=test:%s -new-name=Bar %s -- 2>&1 | FileCheck --check-prefix=TESTCHECK %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 | grep -v CHECK | FileCheck --check-prefix=TESTCHECK %s class Baz { // CHECK: [[@LINE]]:1: error: there is no symbol at the given location }; diff --git a/test/Refactor/tool-test-support.c b/test/Refactor/tool-test-support.c index a20825518c..0e073f6e1c 100644 --- a/test/Refactor/tool-test-support.c +++ b/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 diff --git a/tools/clang-refactor/TestSupport.cpp b/tools/clang-refactor/TestSupport.cpp index 2f127bd7f6..9331dfd92e 100644 --- a/tools/clang-refactor/TestSupport.cpp +++ b/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 findTestSelectionRanges(StringRef Filename) { ErrorOr> 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> GroupedRanges; @@ -304,18 +324,22 @@ findTestSelectionRanges(StringRef Filename) { StringRef Comment = Source.substr(Tok.getLocation().getRawEncoding(), Tok.getLength()); SmallVector 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. + // 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(); @@ -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 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{Range})); if (!It.second) -- 2.40.0