From 0beca4d1ecd6b20abd8be8ee4f46783128a009b4 Mon Sep 17 00:00:00 2001 From: Alex Lorenz Date: Fri, 27 Oct 2017 18:19:11 +0000 Subject: [PATCH] [refactor] Describe refactorings in the operation classes This commit changes the way that the refactoring operation classes are structured: - Users have to call `initiate` instead of constructing an instance of the class. The `initiate` is now supposed to have custom initiation logic, and you don't need to subclass the builtin requirements. - A new `describe` function returns a structure with the id, title and the description of the refactoring operation. The refactoring action classes are now placed into one common place in RefactoringActions.cpp instead of being separate. Differential Revision: https://reviews.llvm.org/D38985 llvm-svn: 316780 --- .../Tooling/Refactoring/Extract/Extract.h | 53 ++++++++ .../Refactoring/RefactoringActionRegistry.def | 8 -- .../Refactoring/RefactoringActionRule.h | 14 +++ .../RefactoringActionRulesInternal.h | 7 +- .../Refactoring/Rename/RenamingAction.h | 22 +++- clang/include/clang/module.modulemap | 2 - clang/lib/Tooling/Refactoring/Extract.cpp | 115 +++++------------- .../Refactoring/RefactoringActions.cpp | 71 ++++++++++- .../Refactoring/Rename/RenamingAction.cpp | 88 +++++--------- .../Tooling/RefactoringActionRulesTest.cpp | 28 ++++- 10 files changed, 243 insertions(+), 165 deletions(-) create mode 100644 clang/include/clang/Tooling/Refactoring/Extract/Extract.h delete mode 100644 clang/include/clang/Tooling/Refactoring/RefactoringActionRegistry.def diff --git a/clang/include/clang/Tooling/Refactoring/Extract/Extract.h b/clang/include/clang/Tooling/Refactoring/Extract/Extract.h new file mode 100644 index 000000000000..2fd76d252c62 --- /dev/null +++ b/clang/include/clang/Tooling/Refactoring/Extract/Extract.h @@ -0,0 +1,53 @@ +//===--- Extract.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_EXTRACT_EXTRACT_H +#define LLVM_CLANG_TOOLING_REFACTOR_EXTRACT_EXTRACT_H + +#include "clang/Tooling/Refactoring/ASTSelection.h" +#include "clang/Tooling/Refactoring/RefactoringActionRules.h" + +namespace clang { +namespace tooling { + +/// An "Extract Function" refactoring moves code into a new function that's +/// then called from the place where the original code was. +class ExtractFunction final : public SourceChangeRefactoringRule { +public: + /// Initiates the extract function refactoring operation. + /// + /// \param Code The selected set of statements. + /// \param DeclName The name name of the extract function. If None, + /// "extracted" is used. + static Expected initiate(RefactoringRuleContext &Context, + CodeRangeASTSelection Code, + Optional DeclName); + + static const RefactoringDescriptor &describe(); + +private: + ExtractFunction(CodeRangeASTSelection Code, Optional DeclName) + : Code(std::move(Code)), + DeclName(DeclName ? std::move(*DeclName) : "extracted") {} + + Expected + createSourceReplacements(RefactoringRuleContext &Context) override; + + CodeRangeASTSelection Code; + + // FIXME: Account for naming collisions: + // - error when name is specified by user. + // - rename to "extractedN" when name is implicit. + std::string DeclName; +}; + +} // end namespace tooling +} // end namespace clang + +#endif // LLVM_CLANG_TOOLING_REFACTOR_EXTRACT_EXTRACT_H diff --git a/clang/include/clang/Tooling/Refactoring/RefactoringActionRegistry.def b/clang/include/clang/Tooling/Refactoring/RefactoringActionRegistry.def deleted file mode 100644 index 9aeead41489f..000000000000 --- a/clang/include/clang/Tooling/Refactoring/RefactoringActionRegistry.def +++ /dev/null @@ -1,8 +0,0 @@ -#ifndef REFACTORING_ACTION -#define REFACTORING_ACTION(Name) -#endif - -REFACTORING_ACTION(LocalRename) -REFACTORING_ACTION(Extract) - -#undef REFACTORING_ACTION diff --git a/clang/include/clang/Tooling/Refactoring/RefactoringActionRule.h b/clang/include/clang/Tooling/Refactoring/RefactoringActionRule.h index 294ccc381b5c..4130e29d8dea 100644 --- a/clang/include/clang/Tooling/Refactoring/RefactoringActionRule.h +++ b/clang/include/clang/Tooling/Refactoring/RefactoringActionRule.h @@ -11,6 +11,8 @@ #define LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_ACTION_RULE_H #include "clang/Basic/LLVM.h" +#include "llvm/ADT/Optional.h" +#include "llvm/ADT/StringRef.h" #include namespace clang { @@ -20,6 +22,15 @@ class RefactoringOptionVisitor; class RefactoringResultConsumer; class RefactoringRuleContext; +struct RefactoringDescriptor { + /// A unique identifier for the specific refactoring. + StringRef Name; + /// A human readable title for the refactoring. + StringRef Title; + /// A human readable description of what the refactoring does. + StringRef Description; +}; + /// A common refactoring action rule interface that defines the 'invoke' /// function that performs the refactoring operation (either fully or /// partially). @@ -33,6 +44,9 @@ public: /// consumer to propagate the result of the refactoring action. virtual void invoke(RefactoringResultConsumer &Consumer, RefactoringRuleContext &Context) = 0; + + /// Returns the structure that describes the refactoring. + // static const RefactoringDescriptor &describe() = 0; }; /// A refactoring action rule is a wrapper class around a specific refactoring diff --git a/clang/include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h b/clang/include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h index 443c7f86df81..75b6c8f70d17 100644 --- a/clang/include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h +++ b/clang/include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h @@ -57,7 +57,11 @@ 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::move((*std::get(Values)))...).invoke(Consumer, Context); + auto Rule = + RuleType::initiate(Context, std::move((*std::get(Values)))...); + if (!Rule) + return Consumer.handleError(Rule.takeError()); + Rule->invoke(Consumer, Context); } inline void visitRefactoringOptionsImpl(RefactoringOptionVisitor &) {} @@ -141,7 +145,6 @@ createRefactoringActionRule(const RequirementTypes &... Requirements) { Visitor, Requirements, llvm::index_sequence_for()); } - private: std::tuple Requirements; }; diff --git a/clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h b/clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h index f2d9a7bb4d9a..d9ed7d3a1f3c 100644 --- a/clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h +++ b/clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h @@ -17,6 +17,7 @@ #include "clang/Tooling/Refactoring.h" #include "clang/Tooling/Refactoring/AtomicChange.h" +#include "clang/Tooling/Refactoring/RefactoringActionRules.h" #include "clang/Tooling/Refactoring/RefactoringOptions.h" #include "clang/Tooling/Refactoring/Rename/SymbolOccurrences.h" #include "llvm/Support/Error.h" @@ -46,12 +47,23 @@ private: bool PrintLocations; }; -class NewNameOption : public RequiredRefactoringOption { +class RenameOccurrences final : public SourceChangeRefactoringRule { public: - StringRef getName() const override { return "new-name"; } - StringRef getDescription() const override { - return "The new name to change the symbol to"; - } + static Expected initiate(RefactoringRuleContext &Context, + SourceRange SelectionRange, + std::string NewName); + + static const RefactoringDescriptor &describe(); + +private: + RenameOccurrences(const NamedDecl *ND, std::string NewName) + : ND(ND), NewName(std::move(NewName)) {} + + Expected + createSourceReplacements(RefactoringRuleContext &Context) override; + + const NamedDecl *ND; + std::string NewName; }; /// Returns source replacements that correspond to the rename of the given diff --git a/clang/include/clang/module.modulemap b/clang/include/clang/module.modulemap index 1e3262188f99..41cf73d910d8 100644 --- a/clang/include/clang/module.modulemap +++ b/clang/include/clang/module.modulemap @@ -146,8 +146,6 @@ module Clang_Tooling { // importing the AST matchers library gives a link dependency on the AST // matchers (and thus the AST), which clang-format should not have. exclude header "Tooling/RefactoringCallbacks.h" - - textual header "Tooling/Refactoring/RefactoringActionRegistry.def" } module Clang_ToolingCore { diff --git a/clang/lib/Tooling/Refactoring/Extract.cpp b/clang/lib/Tooling/Refactoring/Extract.cpp index 616900c1819e..b1000b60ee00 100644 --- a/clang/lib/Tooling/Refactoring/Extract.cpp +++ b/clang/lib/Tooling/Refactoring/Extract.cpp @@ -13,12 +13,10 @@ /// //===----------------------------------------------------------------------===// +#include "clang/Tooling/Refactoring/Extract/Extract.h" #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 { @@ -44,58 +42,43 @@ bool isSimpleExpression(const Expr *E) { } } -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(); } +} // end anonymous namespace + +const RefactoringDescriptor &ExtractFunction::describe() { + static const RefactoringDescriptor Descriptor = { + "extract-function", + "Extract Function", + "(WIP action; use with caution!) Extracts code into a new function", + }; + return Descriptor; +} + +Expected +ExtractFunction::initiate(RefactoringRuleContext &Context, + CodeRangeASTSelection Code, + Optional DeclName) { + // 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 ExtractFunction(std::move(Code), DeclName); +} + // FIXME: Support C++ method extraction. // FIXME: Support Objective-C method extraction. Expected @@ -194,39 +177,5 @@ ExtractFunction::createSourceReplacements(RefactoringRuleContext &Context) { 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/clang/lib/Tooling/Refactoring/RefactoringActions.cpp b/clang/lib/Tooling/Refactoring/RefactoringActions.cpp index 25f055b7270b..73a311839620 100644 --- a/clang/lib/Tooling/Refactoring/RefactoringActions.cpp +++ b/clang/lib/Tooling/Refactoring/RefactoringActions.cpp @@ -7,21 +7,80 @@ // //===----------------------------------------------------------------------===// +#include "clang/Tooling/Refactoring/Extract/Extract.h" #include "clang/Tooling/Refactoring/RefactoringAction.h" +#include "clang/Tooling/Refactoring/RefactoringOptions.h" +#include "clang/Tooling/Refactoring/Rename/RenamingAction.h" namespace clang { namespace tooling { -// Forward declare the individual create*Action functions. -#define REFACTORING_ACTION(Name) \ - std::unique_ptr create##Name##Action(); -#include "clang/Tooling/Refactoring/RefactoringActionRegistry.def" +namespace { + +class DeclNameOption final : public OptionalRefactoringOption { +public: + StringRef getName() const { return "name"; } + StringRef getDescription() const { + return "Name of the extracted declaration"; + } +}; + +// FIXME: Rewrite the Actions to avoid duplication of descriptions/names with +// rules. +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"; + } + + /// Returns a set of refactoring actions rules that are defined by this + /// action. + RefactoringActionRules createActionRules() const override { + RefactoringActionRules Rules; + Rules.push_back(createRefactoringActionRule( + CodeRangeASTSelectionRequirement(), + OptionRequirement())); + return Rules; + } +}; + +class NewNameOption : public RequiredRefactoringOption { +public: + StringRef getName() const override { return "new-name"; } + StringRef getDescription() const override { + return "The new name to change the symbol to"; + } +}; + +// FIXME: Rewrite the Actions to avoid duplication of descriptions/names with +// rules. +class LocalRename final : public RefactoringAction { +public: + StringRef getCommand() const override { return "local-rename"; } + + StringRef getDescription() const override { + return "Finds and renames symbols in code with no indexer support"; + } + + /// Returns a set of refactoring actions rules that are defined by this + /// action. + RefactoringActionRules createActionRules() const override { + RefactoringActionRules Rules; + Rules.push_back(createRefactoringActionRule( + SourceRangeSelectionRequirement(), OptionRequirement())); + return Rules; + } +}; + +} // end anonymous namespace std::vector> createRefactoringActions() { std::vector> Actions; -#define REFACTORING_ACTION(Name) Actions.push_back(create##Name##Action()); -#include "clang/Tooling/Refactoring/RefactoringActionRegistry.def" + Actions.push_back(llvm::make_unique()); + Actions.push_back(llvm::make_unique()); return Actions; } diff --git a/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp b/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp index 28912c3e139f..210b45b79ef2 100644 --- a/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp +++ b/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp @@ -41,22 +41,6 @@ namespace tooling { namespace { -class SymbolSelectionRequirement : public SourceRangeSelectionRequirement { -public: - Expected evaluate(RefactoringRuleContext &Context) const { - Expected Selection = - SourceRangeSelectionRequirement::evaluate(Context); - if (!Selection) - return Selection.takeError(); - const NamedDecl *ND = - getNamedDeclAt(Context.getASTContext(), Selection->getBegin()); - if (!ND) - return Context.createDiagnosticError( - Selection->getBegin(), diag::err_refactor_selection_no_symbol); - return getCanonicalSymbolDeclaration(ND); - } -}; - class OccurrenceFinder final : public FindSymbolOccurrencesRefactoringRule { public: OccurrenceFinder(const NamedDecl *ND) : ND(ND) {} @@ -74,50 +58,38 @@ private: const NamedDecl *ND; }; -class RenameOccurrences final : public SourceChangeRefactoringRule { -public: - RenameOccurrences(const NamedDecl *ND, std::string NewName) - : Finder(ND), NewName(std::move(NewName)) {} - - Expected - createSourceReplacements(RefactoringRuleContext &Context) override { - Expected Occurrences = - Finder.findSymbolOccurrences(Context); - if (!Occurrences) - return Occurrences.takeError(); - // FIXME: Verify that the new name is valid. - SymbolName Name(NewName); - return createRenameReplacements( - *Occurrences, Context.getASTContext().getSourceManager(), Name); - } - -private: - OccurrenceFinder Finder; - std::string NewName; -}; - -class LocalRename final : public RefactoringAction { -public: - StringRef getCommand() const override { return "local-rename"; } - - StringRef getDescription() const override { - return "Finds and renames symbols in code with no indexer support"; - } - - /// Returns a set of refactoring actions rules that are defined by this - /// action. - RefactoringActionRules createActionRules() const override { - RefactoringActionRules Rules; - Rules.push_back(createRefactoringActionRule( - SymbolSelectionRequirement(), OptionRequirement())); - return Rules; - } -}; - } // end anonymous namespace -std::unique_ptr createLocalRenameAction() { - return llvm::make_unique(); +const RefactoringDescriptor &RenameOccurrences::describe() { + static const RefactoringDescriptor Descriptor = { + "local-rename", + "Rename", + "Finds and renames symbols in code with no indexer support", + }; + return Descriptor; +} + +Expected +RenameOccurrences::initiate(RefactoringRuleContext &Context, + SourceRange SelectionRange, std::string NewName) { + const NamedDecl *ND = + getNamedDeclAt(Context.getASTContext(), SelectionRange.getBegin()); + if (!ND) + return Context.createDiagnosticError( + SelectionRange.getBegin(), diag::err_refactor_selection_no_symbol); + return RenameOccurrences(getCanonicalSymbolDeclaration(ND), NewName); +} + +Expected +RenameOccurrences::createSourceReplacements(RefactoringRuleContext &Context) { + Expected Occurrences = + OccurrenceFinder(ND).findSymbolOccurrences(Context); + if (!Occurrences) + return Occurrences.takeError(); + // FIXME: Verify that the new name is valid. + SymbolName Name(NewName); + return createRenameReplacements( + *Occurrences, Context.getASTContext().getSourceManager(), Name); } Expected> diff --git a/clang/unittests/Tooling/RefactoringActionRulesTest.cpp b/clang/unittests/Tooling/RefactoringActionRulesTest.cpp index 132a3a449635..f0b6466fec46 100644 --- a/clang/unittests/Tooling/RefactoringActionRulesTest.cpp +++ b/clang/unittests/Tooling/RefactoringActionRulesTest.cpp @@ -10,7 +10,8 @@ #include "ReplacementTest.h" #include "RewriterTestContext.h" #include "clang/Tooling/Refactoring.h" -#include "clang/Tooling/Refactoring/RefactoringActionRules.h" +#include "clang/Tooling/Refactoring/Extract/Extract.h" +#include "clang/Tooling/Refactoring/RefactoringAction.h" #include "clang/Tooling/Refactoring/RefactoringDiagnostic.h" #include "clang/Tooling/Refactoring/Rename/SymbolName.h" #include "clang/Tooling/Tooling.h" @@ -63,6 +64,12 @@ TEST_F(RefactoringActionRulesTest, MyFirstRefactoringRule) { ReplaceAWithB(std::pair Selection) : Selection(Selection) {} + static Expected + initiate(RefactoringRuleContext &Cotnext, + std::pair Selection) { + return ReplaceAWithB(Selection); + } + Expected createSourceReplacements(RefactoringRuleContext &Context) { const SourceManager &SM = Context.getSources(); @@ -141,6 +148,11 @@ TEST_F(RefactoringActionRulesTest, MyFirstRefactoringRule) { TEST_F(RefactoringActionRulesTest, ReturnError) { class ErrorRule : public SourceChangeRefactoringRule { public: + static Expected initiate(RefactoringRuleContext &, + SourceRange R) { + return ErrorRule(R); + } + ErrorRule(SourceRange R) {} Expected createSourceReplacements(RefactoringRuleContext &) { return llvm::make_error( @@ -191,6 +203,11 @@ TEST_F(RefactoringActionRulesTest, ReturnSymbolOccurrences) { public: FindOccurrences(SourceRange Selection) : Selection(Selection) {} + static Expected initiate(RefactoringRuleContext &, + SourceRange Selection) { + return FindOccurrences(Selection); + } + Expected findSymbolOccurrences(RefactoringRuleContext &) override { SymbolOccurrences Occurrences; @@ -219,4 +236,13 @@ TEST_F(RefactoringActionRulesTest, ReturnSymbolOccurrences) { SourceRange(Cursor, Cursor.getLocWithOffset(strlen("test")))); } +TEST_F(RefactoringActionRulesTest, EditorCommandBinding) { + const RefactoringDescriptor &Descriptor = ExtractFunction::describe(); + EXPECT_EQ(Descriptor.Name, "extract-function"); + EXPECT_EQ( + Descriptor.Description, + "(WIP action; use with caution!) Extracts code into a new function"); + EXPECT_EQ(Descriptor.Title, "Extract Function"); +} + } // end anonymous namespace