diff --git a/clang/include/clang/Tooling/Refactoring/Transformer.h b/clang/include/clang/Tooling/Refactoring/Transformer.h --- a/clang/include/clang/Tooling/Refactoring/Transformer.h +++ b/clang/include/clang/Tooling/Refactoring/Transformer.h @@ -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 @@ -93,56 +81,11 @@ // change("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 -ASTEdit change(StringRef Target, NodePart Part, TextGenerator Replacement) { - ASTEdit E; - E.Target = Target.str(); - E.Kind = ast_type_traits::ASTNodeKind::getFromNodeKind(); - E.Part = Part; - E.Replacement = std::move(Replacement); - return E; -} - -template -ASTEdit change(StringRef Target, NodePart Part, std::string Replacement) { - return change(Target, Part, text(std::move(Replacement))); -} - -/// Variant of \c change for which the NodePart defaults to the whole node. -template -ASTEdit change(StringRef Target, TextGenerator Replacement) { - return change(Target, NodePart::Node, std::move(Replacement)); -} - -/// Variant of \c change for which the NodePart defaults to the whole node. -template -ASTEdit change(StringRef Target, std::string Replacement) { - return change(Target, text(std::move(Replacement))); -} - -/// Variant of \c change that selects the node of the entire match. -template ASTEdit change(TextGenerator Replacement); - -/// Variant of \c change that selects the node of the entire match. -template ASTEdit change(std::string Replacement) { - return change(text(std::move(Replacement))); -} - /// Description of a source-code transformation. // // A *rewrite rule* describes a transformation of source code. A simple rule @@ -175,11 +118,35 @@ // We expect RewriteRules will most commonly include only one case. SmallVector 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___"; }; +/// Replaces a portion of the source text with \p Replacement. +ASTEdit change(RangeSelector Target, TextGenerator Replacement); + +/// These overloads of \c change allow the user to pass a string-type argument +/// where a \c TextGenerator, \c RangeSelector are otherwise expected. +inline ASTEdit change(RangeSelector Target, std::string Replacement) { + return change(std::move(Target), text(std::move(Replacement))); +} +inline ASTEdit change(StringRef Target, TextGenerator Replacement) { + return change(node(Target), std::move(Replacement)); +} +inline ASTEdit change(StringRef Target, std::string Replacement) { + return change(node(Target), text(Replacement)); +} + +/// These overloads implicitly identify the matched portion of a RewriteRule as +/// the target of the change. +inline ASTEdit change(std::string Replacement) { + return change(RewriteRule::RootID, std::move(Replacement)); +} +inline ASTEdit change(TextGenerator Replacement) { + return change(RewriteRule::RootID, std::move(Replacement)); +} + /// Convenience function for constructing a simple \c RewriteRule. RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M, SmallVector Edits); @@ -235,12 +202,6 @@ // ``` RewriteRule applyFirst(ArrayRef Rules); -// Define this overload of `change` here because RewriteRule::RootId is not in -// scope at the declaration point above. -template ASTEdit change(TextGenerator Replacement) { - return change(RewriteRule::RootId, NodePart::Node, std::move(Replacement)); -} - /// The following three functions are a low-level part of the RewriteRule /// API. We expose them for use in implementing the fixtures that interpret /// RewriteRule, like Transformer and TransfomerTidy, or for more advanced diff --git a/clang/lib/Tooling/Refactoring/Transformer.cpp b/clang/lib/Tooling/Refactoring/Transformer.cpp --- a/clang/lib/Tooling/Refactoring/Transformer.cpp +++ b/clang/lib/Tooling/Refactoring/Transformer.cpp @@ -32,11 +32,7 @@ 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 @@ return false; } -static llvm::Error invalidArgumentError(Twine Message) { - return llvm::make_error(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 -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() != nullptr) { - auto ExprKind = ASTNodeKind::getFromNodeKind(); - if (!ExprKind.isBaseOf(Kind)) - return getExtendedRange(Node, tok::TokenKind::semi, Context); - } - return CharSourceRange::getTokenRange(Node.getSourceRange()); - } - case NodePart::Member: - if (auto *M = Node.get()) - 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()) { - 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` 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()) { - 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()) { - 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> tooling::detail::translateEdits(const MatchResult &Result, llvm::ArrayRef Edits) { - SmallVector Transformations; - auto &NodesMap = Result.Nodes.getMap(); + SmallVector 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 Range = getTargetRange( - Edit.Target, It->second, Edit.Kind, Edit.Part, *Result.Context); + Expected Range = Edit.TargetRange(Result); if (!Range) return Range.takeError(); if (Range->isInvalid() || @@ -164,7 +81,7 @@ 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 @@ 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 Edits) { return RewriteRule{ @@ -255,7 +179,7 @@ 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 @@ 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 @@ // 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()); diff --git a/clang/unittests/Tooling/TransformerTest.cpp b/clang/unittests/Tooling/TransformerTest.cpp --- a/clang/unittests/Tooling/TransformerTest.cpp +++ b/clang/unittests/Tooling/TransformerTest.cpp @@ -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 @@ on(expr(hasType(isOrPointsTo(StringType))) .bind(StringExpr)), callee(cxxMethodDecl(hasName("c_str")))))), - change("REPLACED")); + change("REPLACED")); R.Cases[0].Explanation = text("Use size() method directly on string."); return R; } @@ -183,7 +183,7 @@ hasName("proto::ProtoCommandLineFlag")))) .bind(Flag)), unless(callee(cxxMethodDecl(hasName("GetProto"))))), - change(Flag, "EXPR")); + change(Flag, "EXPR")); std::string Input = R"cc( proto::ProtoCommandLineFlag flag; @@ -201,9 +201,8 @@ TEST_F(TransformerTest, NodePartNameNamedDecl) { StringRef Fun = "fun"; - RewriteRule Rule = - makeRule(functionDecl(hasName("bad")).bind(Fun), - change(Fun, NodePart::Name, "good")); + RewriteRule Rule = makeRule(functionDecl(hasName("bad")).bind(Fun), + change(name(Fun), "good")); std::string Input = R"cc( int bad(int x); @@ -235,7 +234,7 @@ StringRef Ref = "ref"; testRule(makeRule(declRefExpr(to(functionDecl(hasName("bad")))).bind(Ref), - change(Ref, NodePart::Name, "good")), + change(name(Ref), "good")), Input, Expected); } @@ -253,7 +252,7 @@ StringRef Ref = "ref"; Transformer T(makeRule(declRefExpr(to(functionDecl())).bind(Ref), - change(Ref, NodePart::Name, "good")), + change(name(Ref), "good")), consumer()); T.registerMatchers(&MatchFinder); EXPECT_FALSE(rewrite(Input)); @@ -262,7 +261,7 @@ TEST_F(TransformerTest, NodePartMember) { StringRef E = "expr"; RewriteRule Rule = makeRule(memberExpr(member(hasName("bad"))).bind(E), - change(E, NodePart::Member, "good")); + change(member(E), "good")); std::string Input = R"cc( struct S { @@ -315,9 +314,8 @@ )cc"; StringRef E = "expr"; - testRule(makeRule(memberExpr().bind(E), - change(E, NodePart::Member, "good")), - Input, Expected); + testRule(makeRule(memberExpr().bind(E), change(member(E), "good")), Input, + Expected); } TEST_F(TransformerTest, NodePartMemberMultiToken) { @@ -347,9 +345,9 @@ )cc"; StringRef MemExpr = "member"; - testRule(makeRule(memberExpr().bind(MemExpr), - change(MemExpr, NodePart::Member, "good")), - Input, Expected); + testRule( + makeRule(memberExpr().bind(MemExpr), change(member(MemExpr), "good")), + Input, Expected); } TEST_F(TransformerTest, MultiChange) { @@ -371,8 +369,8 @@ StringRef C = "C", T = "T", E = "E"; testRule(makeRule(ifStmt(hasCondition(expr().bind(C)), hasThen(stmt().bind(T)), hasElse(stmt().bind(E))), - {change(C, "true"), change(T, "{ /* then */ }"), - change(E, "{ /* else */ }")}), + {change(C, "true"), change(statement(T), "{ /* then */ }"), + change(statement(E), "{ /* else */ }")}), Input, Expected); } @@ -383,7 +381,7 @@ hasName("proto::ProtoCommandLineFlag")))) .bind(Flag)), unless(callee(cxxMethodDecl(hasName("GetProto"))))), - change(Flag, "PROTO")); + change(Flag, "PROTO")); std::string Input = R"cc( proto::ProtoCommandLineFlag flag; @@ -410,7 +408,7 @@ hasArgument(0, cxxMemberCallExpr( on(expr().bind(S)), callee(cxxMethodDecl(hasName("c_str")))))), - change("DISTINCT")); + change("DISTINCT")); } TEST_F(TransformerTest, OrderedRuleRelated) { @@ -475,7 +473,7 @@ -> llvm::Expected { return llvm::createStringError(llvm::errc::invalid_argument, "ERROR"); }; - Transformer T(makeRule(binaryOperator().bind(O), change(O, AlwaysFail)), + Transformer T(makeRule(binaryOperator().bind(O), change(O, AlwaysFail)), consumer()); T.registerMatchers(&MatchFinder); EXPECT_FALSE(rewrite(Input)); @@ -488,10 +486,9 @@ 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(O, "DELETE_OP"), change(L, "DELETE_LHS")}), - consumer()); + Transformer T(makeRule(binaryOperator(hasLHS(expr().bind(L))).bind(O), + {change(O, "DELETE_OP"), change(L, "DELETE_LHS")}), + consumer()); T.registerMatchers(&MatchFinder); EXPECT_FALSE(rewrite(Input)); EXPECT_THAT(Changes, IsEmpty()); @@ -503,8 +500,7 @@ 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(E, "DELETE_EXPR")), - consumer()); + Transformer T(makeRule(expr().bind(E), change(E, "DELETE_EXPR")), consumer()); T.registerMatchers(&MatchFinder); // The rewrite process fails because the changes conflict with each other... EXPECT_FALSE(rewrite(Input)); @@ -516,9 +512,9 @@ TEST_F(TransformerTest, ErrorOccurredMatchSkipped) { // Syntax error in the function body: std::string Input = "void errorOccurred() { 3 }"; - Transformer T(makeRule(functionDecl(hasName("errorOccurred")), - change("DELETED;")), - consumer()); + Transformer T( + makeRule(functionDecl(hasName("errorOccurred")), change("DELETED;")), + consumer()); T.registerMatchers(&MatchFinder); // The rewrite process itself fails... EXPECT_FALSE(rewrite(Input));