Author: ymandel Date: Wed May 22 07:48:19 2019 New Revision: 361392 URL: http://llvm.org/viewvc/llvm-project?rev=361392&view=rev Log: [LibTooling] Update Transformer to use RangeSelector instead of NodePart enum.
Transformer provides an enum to indicate the range of source text to be edited. That support is now redundant with the new (and more general) RangeSelector library, so we remove the custom enum support in favor of supporting any RangeSelector. Reviewers: ilya-biryukov Subscribers: cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D62149 Modified: cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp cfe/trunk/unittests/Tooling/TransformerTest.cpp Modified: cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h?rev=361392&r1=361391&r2=361392&view=diff ============================================================================== --- cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h (original) +++ cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h Wed May 22 07:48:19 2019 @@ -19,6 +19,7 @@ #include "clang/ASTMatchers/ASTMatchers.h" #include "clang/ASTMatchers/ASTMatchersInternal.h" #include "clang/Tooling/Refactoring/AtomicChange.h" +#include "clang/Tooling/Refactoring/RangeSelector.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallVector.h" #include "llvm/Support/Error.h" @@ -30,19 +31,6 @@ namespace clang { namespace tooling { -/// Determines the part of the AST node to replace. We support this to work -/// around the fact that the AST does not differentiate various syntactic -/// elements into their own nodes, so users can specify them relative to a node, -/// instead. -enum class NodePart { - /// The node itself. - Node, - /// Given a \c MemberExpr, selects the member's token. - Member, - /// Given a \c NamedDecl or \c CxxCtorInitializer, selects that token of the - /// relevant name, not including qualifiers. - Name, -}; // Note that \p TextGenerator is allowed to fail, e.g. when trying to access a // matched node that was not bound. Allowing this to fail simplifies error @@ -76,73 +64,28 @@ inline TextGenerator text(std::string M) // (`RewriteRule::Explanation`) instead. Notes serve the rare cases wherein // edit-specific diagnostics are required. // -// `ASTEdit` should be built using the `change` convenience fucntions. For +// `ASTEdit` should be built using the `change` convenience functions. For // example, // \code -// change<FunctionDecl>(fun, NodePart::Name, "Frodo") +// change(name(fun), text("Frodo")) // \endcode // Or, if we use Stencil for the TextGenerator: // \code -// change<Stmt>(thenNode, stencil::cat("{", thenNode, "}")) -// change<Expr>(call, NodePart::Args, stencil::cat(x, ",", y)) -// .note("argument order changed.") +// using stencil::cat; +// change(statement(thenNode), cat("{", thenNode, "}")) +// change(callArgs(call), cat(x, ",", y)) // \endcode // Or, if you are changing the node corresponding to the rule's matcher, you can // use the single-argument override of \c change: // \code -// change<Expr>("different_expr") +// change(cat("different_expr")) // \endcode struct ASTEdit { - // The (bound) id of the node whose source will be replaced. This id should - // never be the empty string. - std::string Target; - ast_type_traits::ASTNodeKind Kind; - NodePart Part; + RangeSelector TargetRange; TextGenerator Replacement; TextGenerator Note; }; -// Convenience functions for creating \c ASTEdits. They all must be explicitly -// instantiated with the desired AST type. Each overload includes both \c -// std::string and \c TextGenerator versions. - -// FIXME: For overloads taking a \c NodePart, constrain the valid values of \c -// Part based on the type \c T. -template <typename T> -ASTEdit change(StringRef Target, NodePart Part, TextGenerator Replacement) { - ASTEdit E; - E.Target = Target.str(); - E.Kind = ast_type_traits::ASTNodeKind::getFromNodeKind<T>(); - E.Part = Part; - E.Replacement = std::move(Replacement); - return E; -} - -template <typename T> -ASTEdit change(StringRef Target, NodePart Part, std::string Replacement) { - return change<T>(Target, Part, text(std::move(Replacement))); -} - -/// Variant of \c change for which the NodePart defaults to the whole node. -template <typename T> -ASTEdit change(StringRef Target, TextGenerator Replacement) { - return change<T>(Target, NodePart::Node, std::move(Replacement)); -} - -/// Variant of \c change for which the NodePart defaults to the whole node. -template <typename T> -ASTEdit change(StringRef Target, std::string Replacement) { - return change<T>(Target, text(std::move(Replacement))); -} - -/// Variant of \c change that selects the node of the entire match. -template <typename T> ASTEdit change(TextGenerator Replacement); - -/// Variant of \c change that selects the node of the entire match. -template <typename T> ASTEdit change(std::string Replacement) { - return change<T>(text(std::move(Replacement))); -} - /// Description of a source-code transformation. // // A *rewrite rule* describes a transformation of source code. A simple rule @@ -175,9 +118,9 @@ struct RewriteRule { // We expect RewriteRules will most commonly include only one case. SmallVector<Case, 1> Cases; - // Id used as the default target of each match. The node described by the + // ID used as the default target of each match. The node described by the // matcher is should always be bound to this id. - static constexpr llvm::StringLiteral RootId = "___root___"; + static constexpr llvm::StringLiteral RootID = "___root___"; }; /// Convenience function for constructing a simple \c RewriteRule. @@ -235,10 +178,17 @@ inline RewriteRule makeRule(ast_matchers // ``` RewriteRule applyFirst(ArrayRef<RewriteRule> Rules); -// Define this overload of `change` here because RewriteRule::RootId is not in -// scope at the declaration point above. -template <typename T> ASTEdit change(TextGenerator Replacement) { - return change<T>(RewriteRule::RootId, NodePart::Node, std::move(Replacement)); +/// Replaces a portion of the source text with \p Replacement. +ASTEdit change(RangeSelector Target, TextGenerator Replacement); + +/// Replaces the entirety of a RewriteRule's match with \p Replacement. For +/// example, to replace a function call, one could write: +/// \code +/// makeRule(callExpr(callee(functionDecl(hasName("foo")))), +/// change(text("bar()"))) +/// \endcode +inline ASTEdit change(TextGenerator Replacement) { + return change(node(RewriteRule::RootID), std::move(Replacement)); } /// The following three functions are a low-level part of the RewriteRule Modified: cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp?rev=361392&r1=361391&r2=361392&view=diff ============================================================================== --- cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp (original) +++ cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp Wed May 22 07:48:19 2019 @@ -32,11 +32,7 @@ using ast_matchers::internal::DynTypedMa using ast_type_traits::ASTNodeKind; using ast_type_traits::DynTypedNode; using llvm::Error; -using llvm::Expected; -using llvm::Optional; using llvm::StringError; -using llvm::StringRef; -using llvm::Twine; using MatchResult = MatchFinder::MatchResult; @@ -71,91 +67,12 @@ static bool isOriginMacroBody(const clan return false; } -static llvm::Error invalidArgumentError(Twine Message) { - return llvm::make_error<StringError>(llvm::errc::invalid_argument, Message); -} - -static llvm::Error typeError(StringRef Id, const ASTNodeKind &Kind, - Twine Message) { - return invalidArgumentError( - Message + " (node id=" + Id + " kind=" + Kind.asStringRef() + ")"); -} - -static llvm::Error missingPropertyError(StringRef Id, Twine Description, - StringRef Property) { - return invalidArgumentError(Description + " requires property '" + Property + - "' (node id=" + Id + ")"); -} - -static Expected<CharSourceRange> -getTargetRange(StringRef Target, const DynTypedNode &Node, ASTNodeKind Kind, - NodePart TargetPart, ASTContext &Context) { - switch (TargetPart) { - case NodePart::Node: { - // For non-expression statements, associate any trailing semicolon with the - // statement text. However, if the target was intended as an expression (as - // indicated by its kind) then we do not associate any trailing semicolon - // with it. We only associate the exact expression text. - if (Node.get<Stmt>() != nullptr) { - auto ExprKind = ASTNodeKind::getFromNodeKind<clang::Expr>(); - if (!ExprKind.isBaseOf(Kind)) - return getExtendedRange(Node, tok::TokenKind::semi, Context); - } - return CharSourceRange::getTokenRange(Node.getSourceRange()); - } - case NodePart::Member: - if (auto *M = Node.get<clang::MemberExpr>()) - return CharSourceRange::getTokenRange( - M->getMemberNameInfo().getSourceRange()); - return typeError(Target, Node.getNodeKind(), - "NodePart::Member applied to non-MemberExpr"); - case NodePart::Name: - if (const auto *D = Node.get<clang::NamedDecl>()) { - if (!D->getDeclName().isIdentifier()) - return missingPropertyError(Target, "NodePart::Name", "identifier"); - SourceLocation L = D->getLocation(); - auto R = CharSourceRange::getTokenRange(L, L); - // Verify that the range covers exactly the name. - // FIXME: extend this code to support cases like `operator +` or - // `foo<int>` for which this range will be too short. Doing so will - // require subcasing `NamedDecl`, because it doesn't provide virtual - // access to the \c DeclarationNameInfo. - if (getText(R, Context) != D->getName()) - return CharSourceRange(); - return R; - } - if (const auto *E = Node.get<clang::DeclRefExpr>()) { - if (!E->getNameInfo().getName().isIdentifier()) - return missingPropertyError(Target, "NodePart::Name", "identifier"); - SourceLocation L = E->getLocation(); - return CharSourceRange::getTokenRange(L, L); - } - if (const auto *I = Node.get<clang::CXXCtorInitializer>()) { - if (!I->isMemberInitializer() && I->isWritten()) - return missingPropertyError(Target, "NodePart::Name", - "explicit member initializer"); - SourceLocation L = I->getMemberLocation(); - return CharSourceRange::getTokenRange(L, L); - } - return typeError( - Target, Node.getNodeKind(), - "NodePart::Name applied to neither DeclRefExpr, NamedDecl nor " - "CXXCtorInitializer"); - } - llvm_unreachable("Unexpected case in NodePart type."); -} - Expected<SmallVector<tooling::detail::Transformation, 1>> tooling::detail::translateEdits(const MatchResult &Result, llvm::ArrayRef<ASTEdit> Edits) { - SmallVector<Transformation, 1> Transformations; - auto &NodesMap = Result.Nodes.getMap(); + SmallVector<tooling::detail::Transformation, 1> Transformations; for (const auto &Edit : Edits) { - auto It = NodesMap.find(Edit.Target); - assert(It != NodesMap.end() && "Edit target must be bound in the match."); - - Expected<CharSourceRange> Range = getTargetRange( - Edit.Target, It->second, Edit.Kind, Edit.Part, *Result.Context); + Expected<CharSourceRange> Range = Edit.TargetRange(Result); if (!Range) return Range.takeError(); if (Range->isInvalid() || @@ -164,7 +81,7 @@ tooling::detail::translateEdits(const Ma auto Replacement = Edit.Replacement(Result); if (!Replacement) return Replacement.takeError(); - Transformation T; + tooling::detail::Transformation T; T.Range = *Range; T.Replacement = std::move(*Replacement); Transformations.push_back(std::move(T)); @@ -172,6 +89,13 @@ tooling::detail::translateEdits(const Ma return Transformations; } +ASTEdit tooling::change(RangeSelector S, TextGenerator Replacement) { + ASTEdit E; + E.TargetRange = std::move(S); + E.Replacement = std::move(Replacement); + return E; +} + RewriteRule tooling::makeRule(DynTypedMatcher M, SmallVector<ASTEdit, 1> Edits) { return RewriteRule{ @@ -255,7 +179,7 @@ DynTypedMatcher tooling::detail::buildMa DynTypedMatcher M = joinCaseMatchers(Rule); M.setAllowBind(true); // `tryBind` is guaranteed to succeed, because `AllowBind` was set to true. - return *M.tryBind(RewriteRule::RootId); + return *M.tryBind(RewriteRule::RootID); } // Finds the case that was "selected" -- that is, whose matcher triggered the @@ -275,7 +199,7 @@ tooling::detail::findSelectedCase(const llvm_unreachable("No tag found for this rule."); } -constexpr llvm::StringLiteral RewriteRule::RootId; +constexpr llvm::StringLiteral RewriteRule::RootID; void Transformer::registerMatchers(MatchFinder *MatchFinder) { MatchFinder->addDynamicMatcher(tooling::detail::buildMatcher(Rule), this); @@ -287,7 +211,7 @@ void Transformer::run(const MatchResult // Verify the existence and validity of the AST node that roots this rule. auto &NodesMap = Result.Nodes.getMap(); - auto Root = NodesMap.find(RewriteRule::RootId); + auto Root = NodesMap.find(RewriteRule::RootID); assert(Root != NodesMap.end() && "Transformation failed: missing root node."); SourceLocation RootLoc = Result.SourceManager->getExpansionLoc( Root->second.getSourceRange().getBegin()); Modified: cfe/trunk/unittests/Tooling/TransformerTest.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/TransformerTest.cpp?rev=361392&r1=361391&r2=361392&view=diff ============================================================================== --- cfe/trunk/unittests/Tooling/TransformerTest.cpp (original) +++ cfe/trunk/unittests/Tooling/TransformerTest.cpp Wed May 22 07:48:19 2019 @@ -7,8 +7,8 @@ //===----------------------------------------------------------------------===// #include "clang/Tooling/Refactoring/Transformer.h" - #include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Tooling/Refactoring/RangeSelector.h" #include "clang/Tooling/Tooling.h" #include "llvm/Support/Errc.h" #include "llvm/Support/Error.h" @@ -147,7 +147,7 @@ static RewriteRule ruleStrlenSize() { on(expr(hasType(isOrPointsTo(StringType))) .bind(StringExpr)), callee(cxxMethodDecl(hasName("c_str")))))), - change<clang::Expr>("REPLACED")); + change(text("REPLACED"))); R.Cases[0].Explanation = text("Use size() method directly on string."); return R; } @@ -183,7 +183,7 @@ TEST_F(TransformerTest, Flag) { hasName("proto::ProtoCommandLineFlag")))) .bind(Flag)), unless(callee(cxxMethodDecl(hasName("GetProto"))))), - change<clang::Expr>(Flag, "EXPR")); + change(node(Flag), text("EXPR"))); std::string Input = R"cc( proto::ProtoCommandLineFlag flag; @@ -201,9 +201,8 @@ TEST_F(TransformerTest, Flag) { TEST_F(TransformerTest, NodePartNameNamedDecl) { StringRef Fun = "fun"; - RewriteRule Rule = - makeRule(functionDecl(hasName("bad")).bind(Fun), - change<clang::FunctionDecl>(Fun, NodePart::Name, "good")); + RewriteRule Rule = makeRule(functionDecl(hasName("bad")).bind(Fun), + change(name(Fun), text("good"))); std::string Input = R"cc( int bad(int x); @@ -235,7 +234,7 @@ TEST_F(TransformerTest, NodePartNameDecl StringRef Ref = "ref"; testRule(makeRule(declRefExpr(to(functionDecl(hasName("bad")))).bind(Ref), - change<clang::Expr>(Ref, NodePart::Name, "good")), + change(name(Ref), text("good"))), Input, Expected); } @@ -253,7 +252,7 @@ TEST_F(TransformerTest, NodePartNameDecl StringRef Ref = "ref"; Transformer T(makeRule(declRefExpr(to(functionDecl())).bind(Ref), - change<clang::Expr>(Ref, NodePart::Name, "good")), + change(name(Ref), text("good"))), consumer()); T.registerMatchers(&MatchFinder); EXPECT_FALSE(rewrite(Input)); @@ -262,7 +261,7 @@ TEST_F(TransformerTest, NodePartNameDecl TEST_F(TransformerTest, NodePartMember) { StringRef E = "expr"; RewriteRule Rule = makeRule(memberExpr(member(hasName("bad"))).bind(E), - change<clang::Expr>(E, NodePart::Member, "good")); + change(member(E), text("good"))); std::string Input = R"cc( struct S { @@ -315,8 +314,7 @@ TEST_F(TransformerTest, NodePartMemberQu )cc"; StringRef E = "expr"; - testRule(makeRule(memberExpr().bind(E), - change<clang::Expr>(E, NodePart::Member, "good")), + testRule(makeRule(memberExpr().bind(E), change(member(E), text("good"))), Input, Expected); } @@ -348,7 +346,7 @@ TEST_F(TransformerTest, NodePartMemberMu StringRef MemExpr = "member"; testRule(makeRule(memberExpr().bind(MemExpr), - change<clang::Expr>(MemExpr, NodePart::Member, "good")), + change(member(MemExpr), text("good"))), Input, Expected); } @@ -371,8 +369,9 @@ TEST_F(TransformerTest, MultiChange) { StringRef C = "C", T = "T", E = "E"; testRule(makeRule(ifStmt(hasCondition(expr().bind(C)), hasThen(stmt().bind(T)), hasElse(stmt().bind(E))), - {change<Expr>(C, "true"), change<Stmt>(T, "{ /* then */ }"), - change<Stmt>(E, "{ /* else */ }")}), + {change(node(C), text("true")), + change(statement(T), text("{ /* then */ }")), + change(statement(E), text("{ /* else */ }"))}), Input, Expected); } @@ -383,7 +382,7 @@ TEST_F(TransformerTest, OrderedRuleUnrel hasName("proto::ProtoCommandLineFlag")))) .bind(Flag)), unless(callee(cxxMethodDecl(hasName("GetProto"))))), - change<clang::Expr>(Flag, "PROTO")); + change(node(Flag), text("PROTO"))); std::string Input = R"cc( proto::ProtoCommandLineFlag flag; @@ -410,7 +409,7 @@ RewriteRule ruleStrlenSizeDistinct() { hasArgument(0, cxxMemberCallExpr( on(expr().bind(S)), callee(cxxMethodDecl(hasName("c_str")))))), - change<clang::Expr>("DISTINCT")); + change(text("DISTINCT"))); } TEST_F(TransformerTest, OrderedRuleRelated) { @@ -475,7 +474,7 @@ TEST_F(TransformerTest, TextGeneratorFai -> llvm::Expected<std::string> { return llvm::createStringError(llvm::errc::invalid_argument, "ERROR"); }; - Transformer T(makeRule(binaryOperator().bind(O), change<Expr>(O, AlwaysFail)), + Transformer T(makeRule(binaryOperator().bind(O), change(node(O), AlwaysFail)), consumer()); T.registerMatchers(&MatchFinder); EXPECT_FALSE(rewrite(Input)); @@ -488,10 +487,10 @@ TEST_F(TransformerTest, OverlappingEdits std::string Input = "int conflictOneRule() { return 3 + 7; }"; // Try to change the whole binary-operator expression AND one its operands: StringRef O = "O", L = "L"; - Transformer T( - makeRule(binaryOperator(hasLHS(expr().bind(L))).bind(O), - {change<Expr>(O, "DELETE_OP"), change<Expr>(L, "DELETE_LHS")}), - consumer()); + Transformer T(makeRule(binaryOperator(hasLHS(expr().bind(L))).bind(O), + {change(node(O), text("DELETE_OP")), + change(node(L), text("DELETE_LHS"))}), + consumer()); T.registerMatchers(&MatchFinder); EXPECT_FALSE(rewrite(Input)); EXPECT_THAT(Changes, IsEmpty()); @@ -503,7 +502,7 @@ TEST_F(TransformerTest, OverlappingEdits std::string Input = "int conflictOneRule() { return -7; }"; // Try to change the whole binary-operator expression AND one its operands: StringRef E = "E"; - Transformer T(makeRule(expr().bind(E), change<Expr>(E, "DELETE_EXPR")), + Transformer T(makeRule(expr().bind(E), change(node(E), text("DELETE_EXPR"))), consumer()); T.registerMatchers(&MatchFinder); // The rewrite process fails because the changes conflict with each other... @@ -517,7 +516,7 @@ TEST_F(TransformerTest, ErrorOccurredMat // Syntax error in the function body: std::string Input = "void errorOccurred() { 3 }"; Transformer T(makeRule(functionDecl(hasName("errorOccurred")), - change<Decl>("DELETED;")), + change(text("DELETED;"))), consumer()); T.registerMatchers(&MatchFinder); // The rewrite process itself fails... _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
