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 --- .../clang/Tooling/Refactoring/Extract/Extract.h | 53 ++++++++++ .../Refactoring/RefactoringActionRegistry.def | 8 -- .../Tooling/Refactoring/RefactoringActionRule.h | 14 +++ .../Refactoring/RefactoringActionRulesInternal.h | 7 +- .../Tooling/Refactoring/Rename/RenamingAction.h | 22 +++- clang/include/clang/module.modulemap | 2 - clang/lib/Tooling/Refactoring/Extract.cpp | 115 ++++++--------------- .../lib/Tooling/Refactoring/RefactoringActions.cpp | 71 +++++++++++-- .../Tooling/Refactoring/Rename/RenamingAction.cpp | 86 ++++++--------- .../Tooling/RefactoringActionRulesTest.cpp | 28 ++++- 10 files changed, 242 insertions(+), 164 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 0000000..2fd76d2 --- /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 9aeead4..0000000 --- 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 294ccc3..4130e29 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 443c7f8..75b6c8f 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 f2d9a7b..d9ed7d3 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 1e32621..41cf73d 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 616900c..b1000b6 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 25f055b..73a3118 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 28912c3..210b45b 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"; - } +} // end anonymous namespace - /// 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; - } -}; +const RefactoringDescriptor &RenameOccurrences::describe() { + static const RefactoringDescriptor Descriptor = { + "local-rename", + "Rename", + "Finds and renames symbols in code with no indexer support", + }; + return Descriptor; +} -} // end anonymous namespace +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); +} -std::unique_ptr createLocalRenameAction() { - return llvm::make_unique(); +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 132a3a4..f0b6466 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 -- 2.7.4