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 @@ -20,13 +20,13 @@ #include "clang/ASTMatchers/ASTMatchersInternal.h" #include "clang/Tooling/Refactoring/AtomicChange.h" #include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/SmallVector.h" #include "llvm/Support/Error.h" #include #include #include #include #include -#include namespace clang { namespace tooling { @@ -47,6 +47,78 @@ using TextGenerator = std::function; +// Description of a textual change, based on an AST node. Includes: an ID for +// the (bound) node, a selector for text in relation to the node, a replacement +// and an explanation for the change. +// +// * Target: the source code impacted by the rule. This identifies an AST node, +// or part thereof (\c Part), whose source range indicates the extent of the +// replacement applied by the replacement term. By default, the extent is the +// node matched by the pattern term (\c NodePart::Node). Target's are typed +// (\c Kind), which guides the determination of the node extent. +// +// * Replacement: a function that produces a replacement string for the target, +// based on the match result. +// +// * Explanation: explanation of the rewrite (also based on the match result). +// This will be displayed to the user, where possible; for example, in +// clang-tidy fix descriptions. +// +// TextChange should be built using the corresponding builder class. For +// example, +// \code +// changeTextOf(call, NodePart::Args).to(x, ",", y) +// .because("Argument order has changed.") +// changeTextOf(thenNode).to("{", thenNode, "}") +// changeTextOf(fun, NodePart::Name).to("Frodo") +// \endcode +// Or, if you are changing the node corresponding to the rule's matcher, you can +// use the shorthand \c changeRoot: +// \code +// changeRoot().to(x, "+", y) +// \endcode +struct TextChange { + // 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; + TextGenerator Replacement; + TextGenerator Explanation; +}; + +/// A fluent builder class for \c TextChange. See comments on \c TextChange for +/// usage. +class TextChangeBuilder { + TextChange Change; + + // Wraps a string as a TextGenerator. + static TextGenerator text(std::string M) { + return [M](const ast_matchers::MatchFinder::MatchResult &) { return M; }; + } + +public: + TextChangeBuilder(StringRef Target, ast_type_traits::ASTNodeKind Kind, + NodePart Part) { + Change.Target = Target; + Change.Kind = Kind; + Change.Part = Part; + } + + /// (Implicit) "build" operator to build a TextChange from this builder. + operator TextChange() && { return std::move(Change); } + + TextChangeBuilder to(TextGenerator Replacement); + TextChangeBuilder to(std::string Replacement) { + return to(text(std::move(Replacement))); + } + + TextChangeBuilder because(TextGenerator Explanation); + TextChangeBuilder because(std::string Explanation) { + return because(text(std::move(Explanation))); + } +}; + /// Description of a source-code transformation. // // A *rewrite rule* describes a transformation of source code. It has the @@ -55,19 +127,8 @@ // * Matcher: the pattern term, expressed as clang matchers (with Transformer // extensions). // -// * Target: the source code impacted by the rule. This identifies an AST node, -// or part thereof (\c TargetPart), whose source range indicates the extent of -// the replacement applied by the replacement term. By default, the extent is -// the node matched by the pattern term (\c NodePart::Node). Target's are -// typed (\c TargetKind), which guides the determination of the node extent -// and might, in the future, statically constrain the set of eligible -// NodeParts for a given node. -// -// * Replacement: a function that produces a replacement string for the target, -// based on the match result. -// -// * Explanation: explanation of the rewrite. This will be displayed to the -// user, where possible (for example, in clang-tidy fix descriptions). +// * Changes: a set of changes to the code text, including addition/removal of +// headers and textual replacements. // // Rules have an additional, implicit, component: the parameters. These are // portions of the pattern which are left unspecified, yet named so that we can @@ -77,27 +138,16 @@ // AST. However, in all cases, we refer to named portions of the pattern as // parameters. // -// RewriteRule is constructed in a "fluent" style, by creating a builder and -// chaining setters of individual components. -// \code -// RewriteRule MyRule = buildRule(functionDecl(...)).replaceWith(...); -// \endcode -// -// The \c Transformer class should then be used to apply the rewrite rule and -// obtain the corresponding replacements. +// The \c Transformer class should be used to apply the rewrite rule and obtain +// the corresponding replacements. struct RewriteRule { // `Matcher` describes the context of this rule. It should always be bound to // at least `RootId`. The builder class below takes care of this // binding. Here, we bind it to a trivial Matcher to enable the default // constructor, since DynTypedMatcher has no default constructor. - ast_matchers::internal::DynTypedMatcher Matcher = ast_matchers::stmt(); - // 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 TargetKind; - NodePart TargetPart; - TextGenerator Replacement; - TextGenerator Explanation; + // ast_matchers::internal::DynTypedMatcher Matcher = ast_matchers::stmt(); + ast_matchers::internal::DynTypedMatcher Matcher; + SmallVector Changes; // Id used as the default target of each match. The node described by the // matcher is guaranteed to be bound to this id, for all rewrite rules @@ -105,81 +155,35 @@ static constexpr llvm::StringLiteral RootId = "___root___"; }; -/// A fluent builder class for \c RewriteRule. See comments on \c RewriteRule. -class RewriteRuleBuilder { - RewriteRule Rule; - -public: - RewriteRuleBuilder(ast_matchers::internal::DynTypedMatcher M) { - M.setAllowBind(true); - // `tryBind` is guaranteed to succeed, because `AllowBind` was set to true. - Rule.Matcher = *M.tryBind(RewriteRule::RootId); - Rule.Target = RewriteRule::RootId; - Rule.TargetKind = M.getSupportedKind(); - Rule.TargetPart = NodePart::Node; - } - - /// (Implicit) "build" operator to build a RewriteRule from this builder. - operator RewriteRule() && { return std::move(Rule); } - - // Sets the target kind based on a clang AST node type. - template RewriteRuleBuilder as(); - - template - RewriteRuleBuilder change(llvm::StringRef Target, - NodePart Part = NodePart::Node); - - RewriteRuleBuilder replaceWith(TextGenerator Replacement); - RewriteRuleBuilder replaceWith(std::string Replacement) { - return replaceWith(text(std::move(Replacement))); - } - - RewriteRuleBuilder because(TextGenerator Explanation); - RewriteRuleBuilder because(std::string Explanation) { - return because(text(std::move(Explanation))); - } - -private: - // Wraps a string as a TextGenerator. - static TextGenerator text(std::string M) { - return [M](const ast_matchers::MatchFinder::MatchResult &) { return M; }; - } -}; - -/// Convenience factory functions for starting construction of a \c RewriteRule. -inline RewriteRuleBuilder buildRule(ast_matchers::internal::DynTypedMatcher M) { - return RewriteRuleBuilder(std::move(M)); +/// Convenience function for constructing a \c RewriteRule. Takes care of +/// binding the matcher to RootId. +RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M, + SmallVector Changes); + +/// Convenience overload of \c makeRule for common case of only one change. +inline RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M, + TextChange Change) { + SmallVector Changes; + Changes.emplace_back(std::move(Change)); + return makeRule(std::move(M), std::move(Changes)); } -template RewriteRuleBuilder RewriteRuleBuilder::as() { - Rule.TargetKind = ast_type_traits::ASTNodeKind::getFromNodeKind(); - return *this; +/// Convenience function for changing the text of the given `Target` node. Must +/// be explicitly instantiated with the desired AST type. +// +// FIXME: constrain accepted values of \c Part based on type \c T. +template +TextChangeBuilder changeTextOf(StringRef Target, + NodePart Part = NodePart::Node) { + return TextChangeBuilder( + Target, ast_type_traits::ASTNodeKind::getFromNodeKind(), Part); } template -RewriteRuleBuilder RewriteRuleBuilder::change(llvm::StringRef TargetId, - NodePart Part) { - Rule.Target = TargetId; - Rule.TargetKind = ast_type_traits::ASTNodeKind::getFromNodeKind(); - Rule.TargetPart = Part; - return *this; +TextChangeBuilder changeRoot() { + return changeTextOf(RewriteRule::RootId); } -/// A source "transformation," represented by a character range in the source to -/// be replaced and a corresponding replacement string. -struct Transformation { - CharSourceRange Range; - std::string Replacement; -}; - -/// Attempts to apply a rule to a match. Returns an empty transformation if the -/// match is not eligible for rewriting (certain interactions with macros, for -/// example). Fails if any invariants are violated relating to bound nodes in -/// the match. -Expected -applyRewriteRule(const RewriteRule &Rule, - const ast_matchers::MatchFinder::MatchResult &Match); - /// Handles the matcher and callback registration for a single rewrite rule, as /// defined by the arguments of the constructor. class Transformer : public ast_matchers::MatchFinder::MatchCallback { @@ -187,6 +191,12 @@ using ChangeConsumer = std::function; +private: + RewriteRule Rule; + /// Receives each successful rewrites as an \c AtomicChange. + ChangeConsumer Consumer; + +public: /// \param Consumer Receives each successful rewrites as an \c AtomicChange. Transformer(RewriteRule Rule, ChangeConsumer Consumer) : Rule(std::move(Rule)), Consumer(std::move(Consumer)) {} @@ -198,12 +208,26 @@ /// Not called directly by users -- called by the framework, via base class /// pointer. void run(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; -private: - RewriteRule Rule; - /// Receives each successful rewrites as an \c AtomicChange. - ChangeConsumer Consumer; +/// A source "transformation," represented by a character range in the source to +/// be replaced and a corresponding replacement string. +struct Transformation { + // Trivial constructor to enable `emplace_back()` and the like. + Transformation(CharSourceRange Range, std::string Replacement) + : Range(Range), Replacement(std::move(Replacement)) {} + + CharSourceRange Range; + std::string Replacement; }; + +/// Attempts to apply a set of changes to a given a match. Returns an empty +/// vector if any of the changes apply to portions of the match that are +/// ineligible for rewriting (certain interactions with macros, for example). +/// Fails if any invariants are violated relating to bound nodes in the match. +Expected> +applyRewriteRule(const ast_matchers::MatchFinder::MatchResult &Result, + llvm::ArrayRef Changes); } // namespace tooling } // namespace clang 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 @@ -144,38 +144,44 @@ llvm_unreachable("Unexpected case in NodePart type."); } -Expected -tooling::applyRewriteRule(const RewriteRule &Rule, - const ast_matchers::MatchFinder::MatchResult &Match) { - if (Match.Context->getDiagnostics().hasErrorOccurred()) - return Transformation(); - - auto &NodesMap = Match.Nodes.getMap(); - auto It = NodesMap.find(Rule.Target); - assert (It != NodesMap.end() && "Rule.Target must be bound in the match."); - - Expected TargetOrErr = - getTargetRange(Rule.Target, It->second, Rule.TargetKind, Rule.TargetPart, - *Match.Context); - if (auto Err = TargetOrErr.takeError()) - return std::move(Err); - auto &Target = *TargetOrErr; - if (Target.isInvalid() || - isOriginMacroBody(*Match.SourceManager, Target.getBegin())) - return Transformation(); - - return Transformation{Target, Rule.Replacement(Match)}; +Expected> +tooling::applyRewriteRule(const MatchResult &Result, + llvm::ArrayRef Changes) { + SmallVector Transformations; + auto &NodesMap = Result.Nodes.getMap(); + for (const auto &Change : Changes) { + auto It = NodesMap.find(Change.Target); + assert(It != NodesMap.end() && "Change target must be bound in the match."); + + Expected RangeOrErr = getTargetRange( + Change.Target, It->second, Change.Kind, Change.Part, *Result.Context); + if (auto Err = RangeOrErr.takeError()) + return std::move(Err); + auto &TargetRange = *RangeOrErr; + if (TargetRange.isInvalid() || + isOriginMacroBody(*Result.SourceManager, TargetRange.getBegin())) + return SmallVector(); + Transformations.emplace_back(TargetRange, Change.Replacement(Result)); + } + return Transformations; +} + +RewriteRule tooling::makeRule(ast_matchers::internal::DynTypedMatcher M, + SmallVector Changes) { + M.setAllowBind(true); + // `tryBind` is guaranteed to succeed, because `AllowBind` was set to true. + return RewriteRule{*M.tryBind(RewriteRule::RootId), std::move(Changes)}; } constexpr llvm::StringLiteral RewriteRule::RootId; -RewriteRuleBuilder RewriteRuleBuilder::replaceWith(TextGenerator T) { - Rule.Replacement = std::move(T); +TextChangeBuilder TextChangeBuilder::to(TextGenerator T) { + Change.Replacement = std::move(T); return *this; } -RewriteRuleBuilder RewriteRuleBuilder::because(TextGenerator T) { - Rule.Explanation = std::move(T); +TextChangeBuilder TextChangeBuilder::because(TextGenerator T) { + Change.Explanation = std::move(T); return *this; } @@ -184,20 +190,39 @@ } void Transformer::run(const MatchResult &Result) { - auto ChangeOrErr = applyRewriteRule(Rule, Result); - if (auto Err = ChangeOrErr.takeError()) { - llvm::errs() << "Rewrite failed: " << llvm::toString(std::move(Err)) + if (Result.Context->getDiagnostics().hasErrorOccurred()) + return; + + // Verify the existence and validity of the AST node that roots this change. + auto &NodesMap = Result.Nodes.getMap(); + auto Root = NodesMap.find(RewriteRule::RootId); + assert(Root != NodesMap.end() && "Transformation failed: missing root node."); + SourceLocation RootLoc = Result.SourceManager->getExpansionLoc( + Root->second.getSourceRange().getBegin()); + assert(RootLoc.isValid() && "Invalid location for Root node of match."); + + auto TransformationsOrErr = applyRewriteRule(Result, Rule.Changes); + if (auto Err = TransformationsOrErr.takeError()) { + llvm::errs() << "Transformation failed: " << llvm::toString(std::move(Err)) << "\n"; return; } - auto &Change = *ChangeOrErr; - auto &Range = Change.Range; - if (Range.isInvalid()) { + auto &Transformations = *TransformationsOrErr; + if (Transformations.empty()) { // No rewrite applied (but no error encountered either). + RootLoc.print(llvm::errs() << "note: skipping match at loc ", + *Result.SourceManager); + llvm::errs() << "\n"; return; } - AtomicChange AC(*Result.SourceManager, Range.getBegin()); - if (auto Err = AC.replace(*Result.SourceManager, Range, Change.Replacement)) - AC.setError(llvm::toString(std::move(Err))); + + // Convert the result to an AtomicChange. + AtomicChange AC(*Result.SourceManager, RootLoc); + for (const auto &T : Transformations) + if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) { + AC.setError(llvm::toString(std::move(Err))); + break; + } + Consumer(AC); } 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 @@ -13,36 +13,12 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" -namespace clang { -namespace tooling { -namespace { -using ast_matchers::anyOf; -using ast_matchers::argumentCountIs; -using ast_matchers::callee; -using ast_matchers::callExpr; -using ast_matchers::cxxMemberCallExpr; -using ast_matchers::cxxMethodDecl; -using ast_matchers::cxxRecordDecl; -using ast_matchers::declRefExpr; -using ast_matchers::expr; -using ast_matchers::functionDecl; -using ast_matchers::hasAnyName; -using ast_matchers::hasArgument; -using ast_matchers::hasDeclaration; -using ast_matchers::hasElse; -using ast_matchers::hasName; -using ast_matchers::hasType; -using ast_matchers::ifStmt; -using ast_matchers::member; -using ast_matchers::memberExpr; -using ast_matchers::namedDecl; -using ast_matchers::on; -using ast_matchers::pointsTo; -using ast_matchers::to; -using ast_matchers::unless; - -using llvm::StringRef; +using namespace clang; +using namespace tooling; +using namespace ast_matchers; + +namespace { constexpr char KHeaderContents[] = R"cc( struct string { string(const char*); @@ -59,6 +35,9 @@ PCFProto& GetProto(); }; } // namespace proto + class Logger {}; + void operator<<(Logger& l, string msg); + Logger& log(int level); )cc"; static ast_matchers::internal::Matcher @@ -141,18 +120,15 @@ static RewriteRule ruleStrlenSize() { StringRef StringExpr = "strexpr"; auto StringType = namedDecl(hasAnyName("::basic_string", "::string")); - return buildRule( - callExpr( - callee(functionDecl(hasName("strlen"))), - hasArgument(0, cxxMemberCallExpr( - on(expr(hasType(isOrPointsTo(StringType))) - .bind(StringExpr)), - callee(cxxMethodDecl(hasName("c_str"))))))) - // Specify the intended type explicitly, because the matcher "type" of - // `callExpr()` is `Stmt`, not `Expr`. - .as() - .replaceWith("REPLACED") - .because("Use size() method directly on string."); + return makeRule( + callExpr(callee(functionDecl(hasName("strlen"))), + hasArgument(0, cxxMemberCallExpr( + on(expr(hasType(isOrPointsTo(StringType))) + .bind(StringExpr)), + callee(cxxMethodDecl(hasName("c_str")))))), + changeRoot() + .to("REPLACED") + .because("Use size() method directly on string.")); } TEST_F(TransformerTest, StrlenSize) { @@ -181,15 +157,13 @@ // Tests replacing an expression. TEST_F(TransformerTest, Flag) { StringRef Flag = "flag"; - RewriteRule Rule = - buildRule( - cxxMemberCallExpr(on(expr(hasType(cxxRecordDecl(hasName( - "proto::ProtoCommandLineFlag")))) - .bind(Flag)), - unless(callee(cxxMethodDecl(hasName("GetProto")))))) - .change(Flag) - .replaceWith("EXPR") - .because("Use GetProto() to access proto fields."); + RewriteRule Rule = makeRule( + cxxMemberCallExpr(on(expr(hasType(cxxRecordDecl( + hasName("proto::ProtoCommandLineFlag")))) + .bind(Flag)), + unless(callee(cxxMethodDecl(hasName("GetProto"))))), + changeTextOf(Flag).to("EXPR").because( + "Use GetProto() to access proto fields.")); std::string Input = R"cc( proto::ProtoCommandLineFlag flag; @@ -207,9 +181,9 @@ TEST_F(TransformerTest, NodePartNameNamedDecl) { StringRef Fun = "fun"; - RewriteRule Rule = buildRule(functionDecl(hasName("bad")).bind(Fun)) - .change(Fun, NodePart::Name) - .replaceWith("good"); + RewriteRule Rule = makeRule( + functionDecl(hasName("bad")).bind(Fun), + changeTextOf(Fun, NodePart::Name).to("good")); std::string Input = R"cc( int bad(int x); @@ -240,9 +214,8 @@ )cc"; StringRef Ref = "ref"; - testRule(buildRule(declRefExpr(to(functionDecl(hasName("bad")))).bind(Ref)) - .change(Ref, NodePart::Name) - .replaceWith("good"), + testRule(makeRule(declRefExpr(to(functionDecl(hasName("bad")))).bind(Ref), + changeTextOf(Ref, NodePart::Name).to("good")), Input, Expected); } @@ -259,17 +232,16 @@ )cc"; StringRef Ref = "ref"; - testRule(buildRule(declRefExpr(to(functionDecl())).bind(Ref)) - .change(Ref, NodePart::Name) - .replaceWith("good"), + testRule(makeRule(declRefExpr(to(functionDecl())).bind(Ref), + changeTextOf(Ref, NodePart::Name).to("good")), Input, Input); } TEST_F(TransformerTest, NodePartMember) { StringRef E = "expr"; - RewriteRule Rule = buildRule(memberExpr(member(hasName("bad"))).bind(E)) - .change(E, NodePart::Member) - .replaceWith("good"); + RewriteRule Rule = + makeRule(memberExpr(member(hasName("bad"))).bind(E), + changeTextOf(E, NodePart::Member).to("good")); std::string Input = R"cc( struct S { @@ -322,9 +294,8 @@ )cc"; StringRef E = "expr"; - testRule(buildRule(memberExpr().bind(E)) - .change(E, NodePart::Member) - .replaceWith("good"), + testRule(makeRule(memberExpr().bind(E), + changeTextOf(E, NodePart::Member).to("good")), Input, Expected); } @@ -355,9 +326,34 @@ )cc"; StringRef MemExpr = "member"; - testRule(buildRule(memberExpr().bind(MemExpr)) - .change(MemExpr, NodePart::Member) - .replaceWith("good"), + testRule( + makeRule(memberExpr().bind(MemExpr), + changeTextOf(MemExpr, NodePart::Member).to("good")), + Input, Expected); +} + +TEST_F(TransformerTest, MultiChange) { + std::string Input = R"cc( + void foo() { + if (10 > 1.0) + log(1) << "oh no!"; + else + log(0) << "ok"; + } + )cc"; + std::string Expected = R"cc( + void foo() { + if (true) { /* then */ } + else { /* else */ } + } + )cc"; + + StringRef C = "C", T = "T", E = "E"; + testRule(makeRule(ifStmt(hasCondition(expr().bind(C)), + hasThen(stmt().bind(T)), hasElse(stmt().bind(E))), + {changeTextOf(C).to("true"), + changeTextOf(T).to("{ /* then */ }"), + changeTextOf(E).to("{ /* else */ }")}), Input, Expected); } @@ -385,5 +381,3 @@ testRule(ruleStrlenSize(), Input, Input); } } // namespace -} // namespace tooling -} // namespace clang