From 18440547d3520b78c9ab929685309419fc1fbe95 Mon Sep 17 00:00:00 2001 From: Eric Li Date: Mon, 21 Mar 2022 18:19:17 +0000 Subject: [PATCH] [libTooling] Generalize string explanation as templated metadata Change RewriteRule from holding an `Explanation` to being able to generate arbitrary metadata. Where TransformerClangTidyCheck was interested in a string description for the diagnostic, other tools may be interested in richer metadata at a higher level of abstraction than at the edit level (which is currently available as ASTEdit::Metadata). Reviewed By: ymandel Differential Revision: https://reviews.llvm.org/D120360 --- .../clang-tidy/abseil/CleanupCtadCheck.cpp | 2 +- .../abseil/StringFindStrContainsCheck.cpp | 6 +- .../clang-tidy/bugprone/StringviewNullptrCheck.cpp | 2 +- .../clang-tidy/utils/TransformerClangTidyCheck.cpp | 32 ++-- .../clang-tidy/utils/TransformerClangTidyCheck.h | 17 ++- .../clang-tidy/TransformerClangTidyCheckTest.cpp | 29 ++-- .../clang/Tooling/Transformer/RewriteRule.h | 148 ++++++++++++++----- .../clang/Tooling/Transformer/Transformer.h | 162 +++++++++++++++++++-- clang/lib/Tooling/Transformer/RewriteRule.cpp | 53 ++++--- clang/lib/Tooling/Transformer/Transformer.cpp | 75 +++++++--- clang/unittests/Tooling/TransformerTest.cpp | 86 +++++++++-- 11 files changed, 479 insertions(+), 133 deletions(-) diff --git a/clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.cpp b/clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.cpp index bbf8603..e6617fb 100644 --- a/clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.cpp +++ b/clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.cpp @@ -23,7 +23,7 @@ namespace clang { namespace tidy { namespace abseil { -RewriteRule CleanupCtadCheckImpl() { +RewriteRuleWith CleanupCtadCheckImpl() { auto warning_message = cat("prefer absl::Cleanup's class template argument " "deduction pattern in C++17 and higher"); diff --git a/clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp b/clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp index 601b987..964826a 100644 --- a/clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp +++ b/clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp @@ -30,7 +30,7 @@ using ::clang::transformer::cat; using ::clang::transformer::changeTo; using ::clang::transformer::makeRule; using ::clang::transformer::node; -using ::clang::transformer::RewriteRule; +using ::clang::transformer::RewriteRuleWith; AST_MATCHER(Type, isCharType) { return Node.isCharType(); } @@ -39,7 +39,7 @@ static const char DefaultStringLikeClasses[] = "::std::basic_string;" "::absl::string_view"; static const char DefaultAbseilStringsMatchHeader[] = "absl/strings/match.h"; -static transformer::RewriteRule +static transformer::RewriteRuleWith makeRewriteRule(const std::vector &StringLikeClassNames, StringRef AbseilStringsMatchHeader) { auto StringLikeClass = cxxRecordDecl(hasAnyName(SmallVector( @@ -62,7 +62,7 @@ makeRewriteRule(const std::vector &StringLikeClassNames, hasArgument(1, cxxDefaultArgExpr())), onImplicitObjectArgument(expr().bind("string_being_searched"))); - RewriteRule Rule = applyFirst( + RewriteRuleWith Rule = applyFirst( {makeRule( binaryOperator(hasOperatorName("=="), hasOperands(ignoringParenImpCasts(StringNpos), diff --git a/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp index b45aa93..205f0c4 100644 --- a/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp @@ -37,7 +37,7 @@ AST_MATCHER(clang::VarDecl, isDirectInitialization) { } // namespace -RewriteRule StringviewNullptrCheckImpl() { +RewriteRuleWith StringviewNullptrCheckImpl() { auto construction_warning = cat("constructing basic_string_view from null is undefined; replace with " "the default constructor"); diff --git a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp index 9f64c56..c18838f 100644 --- a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp +++ b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp @@ -13,16 +13,16 @@ namespace clang { namespace tidy { namespace utils { -using transformer::RewriteRule; +using transformer::RewriteRuleWith; #ifndef NDEBUG -static bool hasExplanation(const RewriteRule::Case &C) { - return C.Explanation != nullptr; +static bool hasGenerator(const transformer::Generator &G) { + return G != nullptr; } #endif -static void verifyRule(const RewriteRule &Rule) { - assert(llvm::all_of(Rule.Cases, hasExplanation) && +static void verifyRule(const RewriteRuleWith &Rule) { + assert(llvm::all_of(Rule.Metadata, hasGenerator) && "clang-tidy checks must have an explanation by default;" " explicitly provide an empty explanation if none is desired"); } @@ -39,23 +39,24 @@ TransformerClangTidyCheck::TransformerClangTidyCheck(StringRef Name, // we would be accessing `getLangOpts` and `Options` before the underlying // `ClangTidyCheck` instance was properly initialized. TransformerClangTidyCheck::TransformerClangTidyCheck( - std::function(const LangOptions &, - const OptionsView &)> + std::function>(const LangOptions &, + const OptionsView &)> MakeRule, StringRef Name, ClangTidyContext *Context) : TransformerClangTidyCheck(Name, Context) { - if (Optional R = MakeRule(getLangOpts(), Options)) + if (Optional> R = + MakeRule(getLangOpts(), Options)) setRule(std::move(*R)); } -TransformerClangTidyCheck::TransformerClangTidyCheck(RewriteRule R, - StringRef Name, - ClangTidyContext *Context) +TransformerClangTidyCheck::TransformerClangTidyCheck( + RewriteRuleWith R, StringRef Name, ClangTidyContext *Context) : TransformerClangTidyCheck(Name, Context) { setRule(std::move(R)); } -void TransformerClangTidyCheck::setRule(transformer::RewriteRule R) { +void TransformerClangTidyCheck::setRule( + transformer::RewriteRuleWith R) { verifyRule(R); Rule = std::move(R); } @@ -77,8 +78,9 @@ void TransformerClangTidyCheck::check( if (Result.Context->getDiagnostics().hasErrorOccurred()) return; - RewriteRule::Case Case = transformer::detail::findSelectedCase(Result, Rule); - Expected> Edits = Case.Edits(Result); + size_t I = transformer::detail::findSelectedCase(Result, Rule); + Expected> Edits = + Rule.Cases[I].Edits(Result); if (!Edits) { llvm::errs() << "Rewrite failed: " << llvm::toString(Edits.takeError()) << "\n"; @@ -89,7 +91,7 @@ void TransformerClangTidyCheck::check( if (Edits->empty()) return; - Expected Explanation = Case.Explanation->eval(Result); + Expected Explanation = Rule.Metadata[I]->eval(Result); if (!Explanation) { llvm::errs() << "Error in explanation: " << llvm::toString(Explanation.takeError()) << "\n"; diff --git a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h index d267379..38ca201 100644 --- a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h +++ b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h @@ -48,15 +48,16 @@ public: /// cases where the options disable the check. /// /// See \c setRule for constraints on the rule. - TransformerClangTidyCheck(std::function( - const LangOptions &, const OptionsView &)> - MakeRule, - StringRef Name, ClangTidyContext *Context); + TransformerClangTidyCheck( + std::function>( + const LangOptions &, const OptionsView &)> + MakeRule, + StringRef Name, ClangTidyContext *Context); /// Convenience overload of the constructor when the rule doesn't have any /// dependencies. - TransformerClangTidyCheck(transformer::RewriteRule R, StringRef Name, - ClangTidyContext *Context); + TransformerClangTidyCheck(transformer::RewriteRuleWith R, + StringRef Name, ClangTidyContext *Context); void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) override; @@ -74,10 +75,10 @@ public: /// is a bug. If no explanation is desired, indicate that explicitly (for /// example, by passing `text("no explanation")` to `makeRule` as the /// `Explanation` argument). - void setRule(transformer::RewriteRule R); + void setRule(transformer::RewriteRuleWith R); private: - transformer::RewriteRule Rule; + transformer::RewriteRuleWith Rule; IncludeInserter Inserter; }; diff --git a/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp b/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp index 53ea4f5..832b8b8 100644 --- a/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp +++ b/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp @@ -28,14 +28,14 @@ using transformer::IncludeFormat; using transformer::makeRule; using transformer::node; using transformer::noopEdit; -using transformer::RewriteRule; +using transformer::RewriteRuleWith; using transformer::RootID; using transformer::statement; // Invert the code of an if-statement, while maintaining its semantics. -RewriteRule invertIf() { +RewriteRuleWith invertIf() { StringRef C = "C", T = "T", E = "E"; - RewriteRule Rule = makeRule( + RewriteRuleWith Rule = makeRule( ifStmt(hasCondition(expr().bind(C)), hasThen(stmt().bind(T)), hasElse(stmt().bind(E))), change(statement(RootID), cat("if(!(", node(std::string(C)), ")) ", @@ -140,8 +140,9 @@ TEST(TransformerClangTidyCheckTest, TwoMatchesInMacroExpansion) { } // A trivial rewrite-rule generator that requires Objective-C code. -Optional needsObjC(const LangOptions &LangOpts, - const ClangTidyCheck::OptionsView &Options) { +Optional> +needsObjC(const LangOptions &LangOpts, + const ClangTidyCheck::OptionsView &Options) { if (!LangOpts.ObjC) return None; return makeRule(clang::ast_matchers::functionDecl(), @@ -165,8 +166,9 @@ TEST(TransformerClangTidyCheckTest, DisableByLang) { } // A trivial rewrite rule generator that checks config options. -Optional noSkip(const LangOptions &LangOpts, - const ClangTidyCheck::OptionsView &Options) { +Optional> +noSkip(const LangOptions &LangOpts, + const ClangTidyCheck::OptionsView &Options) { if (Options.get("Skip", "false") == "true") return None; return makeRule(clang::ast_matchers::functionDecl(), @@ -194,10 +196,11 @@ TEST(TransformerClangTidyCheckTest, DisableByConfig) { Input, nullptr, "input.cc", None, Options)); } -RewriteRule replaceCall(IncludeFormat Format) { +RewriteRuleWith replaceCall(IncludeFormat Format) { using namespace ::clang::ast_matchers; - RewriteRule Rule = makeRule(callExpr(callee(functionDecl(hasName("f")))), - change(cat("other()")), cat("no message")); + RewriteRuleWith Rule = + makeRule(callExpr(callee(functionDecl(hasName("f")))), + change(cat("other()")), cat("no message")); addInclude(Rule, "clang/OtherLib.h", Format); return Rule; } @@ -243,10 +246,10 @@ TEST(TransformerClangTidyCheckTest, AddIncludeAngled) { } class IncludeOrderCheck : public TransformerClangTidyCheck { - static RewriteRule rule() { + static RewriteRuleWith rule() { using namespace ::clang::ast_matchers; - RewriteRule Rule = transformer::makeRule(integerLiteral(), change(cat("5")), - cat("no message")); + RewriteRuleWith Rule = transformer::makeRule( + integerLiteral(), change(cat("5")), cat("no message")); addInclude(Rule, "bar.h", IncludeFormat::Quoted); return Rule; } diff --git a/clang/include/clang/Tooling/Transformer/RewriteRule.h b/clang/include/clang/Tooling/Transformer/RewriteRule.h index 6b14861..b460909 100644 --- a/clang/include/clang/Tooling/Transformer/RewriteRule.h +++ b/clang/include/clang/Tooling/Transformer/RewriteRule.h @@ -61,7 +61,9 @@ enum class IncludeFormat { /// of `EditGenerator`. using EditGenerator = MatchConsumer>; -using TextGenerator = std::shared_ptr>; +template using Generator = std::shared_ptr>; + +using TextGenerator = Generator; using AnyGenerator = MatchConsumer; @@ -262,12 +264,9 @@ inline EditGenerator shrinkTo(RangeSelector outer, RangeSelector inner) { // // * Edits: a set of Edits to the source code, described with ASTEdits. // -// * Explanation: explanation of the rewrite. This will be displayed to the -// user, where possible; for example, in clang-tidy diagnostics. -// // However, rules can also consist of (sub)rules, where the first that matches -// is applied and the rest are ignored. So, the above components are gathered -// as a `Case` and a rule is a list of cases. +// is applied and the rest are ignored. So, the above components together form +// a logical "case" and a rule is a sequence of cases. // // Rule cases have an additional, implicit, component: the parameters. These are // portions of the pattern which are left unspecified, yet bound in the pattern @@ -275,37 +274,82 @@ inline EditGenerator shrinkTo(RangeSelector outer, RangeSelector inner) { // // The \c Transformer class can be used to apply the rewrite rule and obtain the // corresponding replacements. -struct RewriteRule { +struct RewriteRuleBase { struct Case { ast_matchers::internal::DynTypedMatcher Matcher; EditGenerator Edits; - TextGenerator Explanation; }; // We expect RewriteRules will most commonly include only one case. SmallVector Cases; +}; - /// DEPRECATED: use `::clang::transformer::RootID` instead. - static const llvm::StringRef RootID; +/// A source-code transformation with accompanying metadata. +/// +/// When a case of the rule matches, the \c Transformer invokes the +/// corresponding metadata generator and provides it alongside the edits. +template struct RewriteRuleWith : RewriteRuleBase { + SmallVector, 1> Metadata; }; -/// Constructs a simple \c RewriteRule. +template <> struct RewriteRuleWith : RewriteRuleBase {}; + +using RewriteRule = RewriteRuleWith; + +namespace detail { + +RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M, + EditGenerator Edits); + +template +RewriteRuleWith makeRule(ast_matchers::internal::DynTypedMatcher M, + EditGenerator Edits, + Generator Metadata) { + RewriteRuleWith R; + R.Cases = {{std::move(M), std::move(Edits)}}; + R.Metadata = {std::move(Metadata)}; + return R; +} + +inline EditGenerator makeEditGenerator(EditGenerator Edits) { return Edits; } +EditGenerator makeEditGenerator(llvm::SmallVector Edits); +EditGenerator makeEditGenerator(ASTEdit Edit); + +} // namespace detail + +/// Constructs a simple \c RewriteRule. \c Edits can be an \c EditGenerator, +/// multiple \c ASTEdits, or a single \c ASTEdit. +/// @{ +template +RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M, + EditsT &&Edits) { + return detail::makeRule( + std::move(M), detail::makeEditGenerator(std::forward(Edits))); +} + RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M, - EditGenerator Edits, TextGenerator Explanation = nullptr); - -/// Constructs a \c RewriteRule from multiple `ASTEdit`s. -inline RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M, - llvm::SmallVector Edits, - TextGenerator Explanation = nullptr) { - return makeRule(std::move(M), editList(std::move(Edits)), - std::move(Explanation)); + std::initializer_list Edits); +/// @} + +/// Overloads of \c makeRule that also generate metadata when matching. +/// @{ +template +RewriteRuleWith makeRule(ast_matchers::internal::DynTypedMatcher M, + EditsT &&Edits, + Generator Metadata) { + return detail::makeRule( + std::move(M), detail::makeEditGenerator(std::forward(Edits)), + std::move(Metadata)); } -/// Overload of \c makeRule for common case of only one edit. -inline RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M, - ASTEdit Edit, - TextGenerator Explanation = nullptr) { - return makeRule(std::move(M), edit(std::move(Edit)), std::move(Explanation)); +template +RewriteRuleWith makeRule(ast_matchers::internal::DynTypedMatcher M, + std::initializer_list Edits, + Generator Metadata) { + return detail::makeRule(std::move(M), + detail::makeEditGenerator(std::move(Edits)), + std::move(Metadata)); } +/// @} /// For every case in Rule, adds an include directive for the given header. The /// common use is assumed to be a rule with only one case. For example, to @@ -317,7 +361,7 @@ inline RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M, /// addInclude(R, "path/to/bar_header.h"); /// addInclude(R, "vector", IncludeFormat::Angled); /// \endcode -void addInclude(RewriteRule &Rule, llvm::StringRef Header, +void addInclude(RewriteRuleBase &Rule, llvm::StringRef Header, IncludeFormat Format = IncludeFormat::Quoted); /// Applies the first rule whose pattern matches; other rules are ignored. If @@ -359,7 +403,45 @@ void addInclude(RewriteRule &Rule, llvm::StringRef Header, // makeRule(left_call, left_call_action), // makeRule(right_call, right_call_action)}); // ``` -RewriteRule applyFirst(ArrayRef Rules); +/// @{ +template +RewriteRuleWith +applyFirst(ArrayRef> Rules) { + RewriteRuleWith R; + for (auto &Rule : Rules) { + assert(Rule.Cases.size() == Rule.Metadata.size() && + "mis-match in case and metadata array size"); + R.Cases.append(Rule.Cases.begin(), Rule.Cases.end()); + R.Metadata.append(Rule.Metadata.begin(), Rule.Metadata.end()); + } + return R; +} + +template <> +RewriteRuleWith applyFirst(ArrayRef> Rules); + +template +RewriteRuleWith +applyFirst(const std::vector> &Rules) { + return applyFirst(llvm::makeArrayRef(Rules)); +} + +template +RewriteRuleWith +applyFirst(std::initializer_list> Rules) { + return applyFirst(llvm::makeArrayRef(Rules.begin(), Rules.end())); +} +/// @} + +/// Converts a \c RewriteRuleWith to a \c RewriteRule by stripping off the +/// metadata generators. +template +std::enable_if_t::value, RewriteRule> +stripMetadata(RewriteRuleWith Rule) { + RewriteRule R; + R.Cases = std::move(Rule.Cases); + return R; +} /// Applies `Rule` to all descendants of the node bound to `NodeId`. `Rule` can /// refer to nodes bound by the calling rule. `Rule` is not applied to the node @@ -423,7 +505,8 @@ rewriteDescendants(const DynTypedNode &Node, RewriteRule Rule, /// Only supports Rules whose cases' matchers share the same base "kind" /// (`Stmt`, `Decl`, etc.) Deprecated: use `buildMatchers` instead, which /// supports mixing matchers of different kinds. -ast_matchers::internal::DynTypedMatcher buildMatcher(const RewriteRule &Rule); +ast_matchers::internal::DynTypedMatcher +buildMatcher(const RewriteRuleBase &Rule); /// Builds a set of matchers that cover the rule. /// @@ -433,7 +516,7 @@ ast_matchers::internal::DynTypedMatcher buildMatcher(const RewriteRule &Rule); /// for rewriting. If any such matchers are included, will return an empty /// vector. std::vector -buildMatchers(const RewriteRule &Rule); +buildMatchers(const RewriteRuleBase &Rule); /// Gets the beginning location of the source matched by a rewrite rule. If the /// match occurs within a macro expansion, returns the beginning of the @@ -441,11 +524,10 @@ buildMatchers(const RewriteRule &Rule); SourceLocation getRuleMatchLoc(const ast_matchers::MatchFinder::MatchResult &Result); -/// Returns the \c Case of \c Rule that was selected in the match result. -/// Assumes a matcher built with \c buildMatcher. -const RewriteRule::Case & -findSelectedCase(const ast_matchers::MatchFinder::MatchResult &Result, - const RewriteRule &Rule); +/// Returns the index of the \c Case of \c Rule that was selected in the match +/// result. Assumes a matcher built with \c buildMatcher. +size_t findSelectedCase(const ast_matchers::MatchFinder::MatchResult &Result, + const RewriteRuleBase &Rule); } // namespace detail } // namespace transformer } // namespace clang diff --git a/clang/include/clang/Tooling/Transformer/Transformer.h b/clang/include/clang/Tooling/Transformer/Transformer.h index 3e0a026..53dd61f 100644 --- a/clang/include/clang/Tooling/Transformer/Transformer.h +++ b/clang/include/clang/Tooling/Transformer/Transformer.h @@ -18,6 +18,62 @@ namespace clang { namespace tooling { + +namespace detail { +/// Implementation details of \c Transformer with type erasure around +/// \c RewriteRule and \c RewriteRule as well as the corresponding consumers. +class TransformerImpl { +public: + virtual ~TransformerImpl() = default; + + void onMatch(const ast_matchers::MatchFinder::MatchResult &Result); + + virtual std::vector + buildMatchers() const = 0; + +protected: + /// Converts a set of \c Edit into a \c AtomicChange per file modified. + /// Returns an error if the edits fail to compose, e.g. overlapping edits. + static llvm::Expected> + convertToAtomicChanges(const llvm::SmallVectorImpl &Edits, + const ast_matchers::MatchFinder::MatchResult &Result); + +private: + virtual void + onMatchImpl(const ast_matchers::MatchFinder::MatchResult &Result) = 0; +}; + +/// Implementation for when no metadata is generated as a part of the +/// \c RewriteRule. +class NoMetadataImpl final : public TransformerImpl { + transformer::RewriteRule Rule; + std::function>)> Consumer; + +public: + explicit NoMetadataImpl( + transformer::RewriteRule R, + std::function>)> + Consumer) + : Rule(std::move(R)), Consumer(std::move(Consumer)) { + assert(llvm::all_of(Rule.Cases, + [](const transformer::RewriteRule::Case &Case) { + return Case.Edits; + }) && + "edit generator must be provided for each rule"); + } + +private: + void onMatchImpl(const ast_matchers::MatchFinder::MatchResult &Result) final; + std::vector + buildMatchers() const final { + return transformer::detail::buildMatchers(Rule); + } +}; + +// FIXME: Use std::type_identity or backport when available. +template struct type_identity { using type = T; }; +} // namespace detail + /// Handles the matcher and callback registration for a single `RewriteRule`, as /// defined by the arguments of the constructor. class Transformer : public ast_matchers::MatchFinder::MatchCallback { @@ -31,16 +87,38 @@ public: using ChangeSetConsumer = std::function> Changes)>; - /// \param Consumer Receives all rewrites for a single match, or an error. + template struct Result { + llvm::MutableArrayRef Changes; + T Metadata; + }; + + // Specialization provided only to avoid SFINAE on the Transformer + // constructor; not intended for use. + template <> struct Result { + llvm::MutableArrayRef Changes; + }; + + /// \param Consumer receives all rewrites for a single match, or an error. /// Will not necessarily be called for each match; for example, if the rule /// generates no edits but does not fail. Note that clients are responsible /// for handling the case that independent \c AtomicChanges conflict with each /// other. - explicit Transformer(transformer::RewriteRule Rule, + explicit Transformer(transformer::RewriteRuleWith Rule, ChangeSetConsumer Consumer) - : Rule(std::move(Rule)), Consumer(std::move(Consumer)) { - assert(this->Consumer && "Consumer is empty"); - } + : Impl(std::make_unique(std::move(Rule), + std::move(Consumer))) {} + + /// \param Consumer receives all rewrites and the associated metadata for a + /// single match, or an error. Will always be called for each match, even if + /// the rule generates no edits. Note that clients are responsible for + /// handling the case that independent \c AtomicChanges conflict with each + /// other. + template + explicit Transformer( + transformer::RewriteRuleWith Rule, + std::function::type>>)> + Consumer); /// N.B. Passes `this` pointer to `MatchFinder`. So, this object should not /// be moved after this call. @@ -51,11 +129,77 @@ public: void run(const ast_matchers::MatchFinder::MatchResult &Result) override; private: - transformer::RewriteRule Rule; - /// Receives sets of successful rewrites as an - /// \c llvm::ArrayRef. - ChangeSetConsumer Consumer; + std::unique_ptr Impl; +}; + +namespace detail { +/// Implementation when metadata is generated as a part of the rewrite. This +/// happens when we have a \c RewriteRuleWith. +template class WithMetadataImpl final : public TransformerImpl { + transformer::RewriteRuleWith Rule; + std::function>)> Consumer; + +public: + explicit WithMetadataImpl( + transformer::RewriteRuleWith R, + std::function>)> Consumer) + : Rule(std::move(R)), Consumer(std::move(Consumer)) { + assert(llvm::all_of(Rule.Cases, + [](const transformer::RewriteRuleBase::Case &Case) + -> bool { return !!Case.Edits; }) && + "edit generator must be provided for each rule"); + assert(llvm::all_of(Rule.Metadata, + [](const typename transformer::Generator &Metadata) + -> bool { return !!Metadata; }) && + "metadata generator must be provided for each rule"); + } + +private: + void onMatchImpl(const ast_matchers::MatchFinder::MatchResult &Result) final { + size_t I = transformer::detail::findSelectedCase(Result, Rule); + auto Transformations = Rule.Cases[I].Edits(Result); + if (!Transformations) { + Consumer(Transformations.takeError()); + return; + } + + llvm::SmallVector Changes; + if (!Transformations->empty()) { + auto C = convertToAtomicChanges(*Transformations, Result); + if (C) { + Changes = std::move(*C); + } else { + Consumer(C.takeError()); + return; + } + } + + auto Metadata = Rule.Metadata[I]->eval(Result); + if (!Metadata) { + Consumer(Metadata.takeError()); + return; + } + + Consumer(Transformer::Result{ + llvm::MutableArrayRef(Changes), std::move(*Metadata)}); + } + + std::vector + buildMatchers() const final { + return transformer::detail::buildMatchers(Rule); + } }; +} // namespace detail + +template +Transformer::Transformer( + transformer::RewriteRuleWith Rule, + std::function::type>>)> + Consumer) + : Impl(std::make_unique>( + std::move(Rule), std::move(Consumer))) {} + } // namespace tooling } // namespace clang diff --git a/clang/lib/Tooling/Transformer/RewriteRule.cpp b/clang/lib/Tooling/Transformer/RewriteRule.cpp index 93bd7e9..3e76489 100644 --- a/clang/lib/Tooling/Transformer/RewriteRule.cpp +++ b/clang/lib/Tooling/Transformer/RewriteRule.cpp @@ -166,10 +166,26 @@ ASTEdit transformer::addInclude(RangeSelector Target, StringRef Header, return E; } -RewriteRule transformer::makeRule(DynTypedMatcher M, EditGenerator Edits, - TextGenerator Explanation) { - return RewriteRule{{RewriteRule::Case{std::move(M), std::move(Edits), - std::move(Explanation)}}}; +EditGenerator +transformer::detail::makeEditGenerator(llvm::SmallVector Edits) { + return editList(std::move(Edits)); +} + +EditGenerator transformer::detail::makeEditGenerator(ASTEdit Edit) { + return edit(std::move(Edit)); +} + +RewriteRule transformer::detail::makeRule(DynTypedMatcher M, + EditGenerator Edits) { + RewriteRule R; + R.Cases = {{std::move(M), std::move(Edits)}}; + return R; +} + +RewriteRule transformer::makeRule(ast_matchers::internal::DynTypedMatcher M, + std::initializer_list Edits) { + return detail::makeRule(std::move(M), + detail::makeEditGenerator(std::move(Edits))); } namespace { @@ -247,9 +263,8 @@ public: void run(const MatchFinder::MatchResult &Result) override { if (!Edits) return; - transformer::RewriteRule::Case Case = - transformer::detail::findSelectedCase(Result, Rule); - auto Transformations = Case.Edits(Result); + size_t I = transformer::detail::findSelectedCase(Result, Rule); + auto Transformations = Rule.Cases[I].Edits(Result); if (!Transformations) { Edits = Transformations.takeError(); return; @@ -325,7 +340,7 @@ EditGenerator transformer::rewriteDescendants(std::string NodeId, }; } -void transformer::addInclude(RewriteRule &Rule, StringRef Header, +void transformer::addInclude(RewriteRuleBase &Rule, StringRef Header, IncludeFormat Format) { for (auto &Case : Rule.Cases) Case.Edits = flatten(std::move(Case.Edits), addInclude(Header, Format)); @@ -366,7 +381,9 @@ static std::vector taggedMatchers( // Simply gathers the contents of the various rules into a single rule. The // actual work to combine these into an ordered choice is deferred to matcher // registration. -RewriteRule transformer::applyFirst(ArrayRef Rules) { +template <> +RewriteRuleWith +transformer::applyFirst(ArrayRef> Rules) { RewriteRule R; for (auto &Rule : Rules) R.Cases.append(Rule.Cases.begin(), Rule.Cases.end()); @@ -374,12 +391,13 @@ RewriteRule transformer::applyFirst(ArrayRef Rules) { } std::vector -transformer::detail::buildMatchers(const RewriteRule &Rule) { +transformer::detail::buildMatchers(const RewriteRuleBase &Rule) { // Map the cases into buckets of matchers -- one for each "root" AST kind, // which guarantees that they can be combined in a single anyOf matcher. Each // case is paired with an identifying number that is converted to a string id // in `taggedMatchers`. - std::map, 1>> + std::map, 1>> Buckets; const SmallVectorImpl &Cases = Rule.Cases; for (int I = 0, N = Cases.size(); I < N; ++I) { @@ -405,7 +423,7 @@ transformer::detail::buildMatchers(const RewriteRule &Rule) { return Matchers; } -DynTypedMatcher transformer::detail::buildMatcher(const RewriteRule &Rule) { +DynTypedMatcher transformer::detail::buildMatcher(const RewriteRuleBase &Rule) { std::vector Ms = buildMatchers(Rule); assert(Ms.size() == 1 && "Cases must have compatible matchers."); return Ms[0]; @@ -428,19 +446,16 @@ SourceLocation transformer::detail::getRuleMatchLoc(const MatchResult &Result) { // Finds the case that was "selected" -- that is, whose matcher triggered the // `MatchResult`. -const RewriteRule::Case & -transformer::detail::findSelectedCase(const MatchResult &Result, - const RewriteRule &Rule) { +size_t transformer::detail::findSelectedCase(const MatchResult &Result, + const RewriteRuleBase &Rule) { if (Rule.Cases.size() == 1) - return Rule.Cases[0]; + return 0; auto &NodesMap = Result.Nodes.getMap(); for (size_t i = 0, N = Rule.Cases.size(); i < N; ++i) { std::string Tag = ("Tag" + Twine(i)).str(); if (NodesMap.find(Tag) != NodesMap.end()) - return Rule.Cases[i]; + return i; } llvm_unreachable("No tag found for this rule."); } - -const llvm::StringRef RewriteRule::RootID = ::clang::transformer::RootID; diff --git a/clang/lib/Tooling/Transformer/Transformer.cpp b/clang/lib/Tooling/Transformer/Transformer.cpp index 1219afc..1cf824d 100644 --- a/clang/lib/Tooling/Transformer/Transformer.cpp +++ b/clang/lib/Tooling/Transformer/Transformer.cpp @@ -16,35 +16,29 @@ #include #include -using namespace clang; -using namespace tooling; +namespace clang { +namespace tooling { -using ast_matchers::MatchFinder; +using ::clang::ast_matchers::MatchFinder; -void Transformer::registerMatchers(MatchFinder *MatchFinder) { - for (auto &Matcher : transformer::detail::buildMatchers(Rule)) - MatchFinder->addDynamicMatcher(Matcher, this); -} +namespace detail { -void Transformer::run(const MatchFinder::MatchResult &Result) { +void TransformerImpl::onMatch( + const ast_matchers::MatchFinder::MatchResult &Result) { if (Result.Context->getDiagnostics().hasErrorOccurred()) return; - transformer::RewriteRule::Case Case = - transformer::detail::findSelectedCase(Result, Rule); - auto Transformations = Case.Edits(Result); - if (!Transformations) { - Consumer(Transformations.takeError()); - return; - } - - if (Transformations->empty()) - return; + onMatchImpl(Result); +} +llvm::Expected> +TransformerImpl::convertToAtomicChanges( + const llvm::SmallVectorImpl &Edits, + const MatchFinder::MatchResult &Result) { // Group the transformations, by file, into AtomicChanges, each anchored by // the location of the first change in that file. std::map ChangesByFileID; - for (const auto &T : *Transformations) { + for (const auto &T : Edits) { auto ID = Result.SourceManager->getFileID(T.Range.getBegin()); auto Iter = ChangesByFileID .emplace(ID, AtomicChange(*Result.SourceManager, @@ -55,8 +49,7 @@ void Transformer::run(const MatchFinder::MatchResult &Result) { case transformer::EditKind::Range: if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) { - Consumer(std::move(Err)); - return; + return std::move(Err); } break; case transformer::EditKind::AddInclude: @@ -69,5 +62,43 @@ void Transformer::run(const MatchFinder::MatchResult &Result) { Changes.reserve(ChangesByFileID.size()); for (auto &IDChangePair : ChangesByFileID) Changes.push_back(std::move(IDChangePair.second)); - Consumer(llvm::MutableArrayRef(Changes)); + + return Changes; +} + +void NoMetadataImpl::onMatchImpl(const MatchFinder::MatchResult &Result) { + size_t I = transformer::detail::findSelectedCase(Result, Rule); + auto Transformations = Rule.Cases[I].Edits(Result); + if (!Transformations) { + Consumer(Transformations.takeError()); + return; + } + + if (Transformations->empty()) + return; + + auto Changes = convertToAtomicChanges(*Transformations, Result); + if (!Changes) { + Consumer(Changes.takeError()); + return; + } + + Consumer(llvm::MutableArrayRef(*Changes)); } + +} // namespace detail + +void Transformer::registerMatchers(MatchFinder *MatchFinder) { + for (auto &Matcher : Impl->buildMatchers()) + MatchFinder->addDynamicMatcher(Matcher, this); +} + +void Transformer::run(const MatchFinder::MatchResult &Result) { + if (Result.Context->getDiagnostics().hasErrorOccurred()) + return; + + Impl->onMatch(Result); +} + +} // namespace tooling +} // namespace clang diff --git a/clang/unittests/Tooling/TransformerTest.cpp b/clang/unittests/Tooling/TransformerTest.cpp index 4ab3984..9ca1daa 100644 --- a/clang/unittests/Tooling/TransformerTest.cpp +++ b/clang/unittests/Tooling/TransformerTest.cpp @@ -31,9 +31,11 @@ using ::clang::transformer::makeRule; using ::clang::transformer::member; using ::clang::transformer::name; using ::clang::transformer::node; +using ::clang::transformer::noEdits; using ::clang::transformer::remove; using ::clang::transformer::rewriteDescendants; using ::clang::transformer::RewriteRule; +using ::clang::transformer::RewriteRuleWith; using ::clang::transformer::statement; using ::testing::ElementsAre; using ::testing::IsEmpty; @@ -129,7 +131,7 @@ protected: Changes.insert(Changes.end(), std::make_move_iterator(C->begin()), std::make_move_iterator(C->end())); } else { - // FIXME: stash this error rather then printing. + // FIXME: stash this error rather than printing. llvm::errs() << "Error generating changes: " << llvm::toString(C.takeError()) << "\n"; ++ErrorCount; @@ -137,27 +139,58 @@ protected: }; } - template - void testRule(R Rule, StringRef Input, StringRef Expected) { + auto consumerWithStringMetadata() { + return [this](Expected> C) { + if (C) { + Changes.insert(Changes.end(), + std::make_move_iterator(C->Changes.begin()), + std::make_move_iterator(C->Changes.end())); + StringMetadata.push_back(std::move(C->Metadata)); + } else { + // FIXME: stash this error rather than printing. + llvm::errs() << "Error generating changes: " + << llvm::toString(C.takeError()) << "\n"; + ++ErrorCount; + } + }; + } + + void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) { Transformers.push_back( std::make_unique(std::move(Rule), consumer())); Transformers.back()->registerMatchers(&MatchFinder); compareSnippets(Expected, rewrite(Input)); } - template void testRuleFailure(R Rule, StringRef Input) { + void testRule(RewriteRuleWith Rule, StringRef Input, + StringRef Expected) { + Transformers.push_back(std::make_unique( + std::move(Rule), consumerWithStringMetadata())); + Transformers.back()->registerMatchers(&MatchFinder); + compareSnippets(Expected, rewrite(Input)); + } + + void testRuleFailure(RewriteRule Rule, StringRef Input) { Transformers.push_back( std::make_unique(std::move(Rule), consumer())); Transformers.back()->registerMatchers(&MatchFinder); ASSERT_FALSE(rewrite(Input)) << "Expected failure to rewrite code"; } + void testRuleFailure(RewriteRuleWith Rule, StringRef Input) { + Transformers.push_back(std::make_unique( + std::move(Rule), consumerWithStringMetadata())); + Transformers.back()->registerMatchers(&MatchFinder); + ASSERT_FALSE(rewrite(Input)) << "Expected failure to rewrite code"; + } + // Transformers are referenced by MatchFinder. std::vector> Transformers; clang::ast_matchers::MatchFinder MatchFinder; // Records whether any errors occurred in individual changes. int ErrorCount = 0; AtomicChanges Changes; + std::vector StringMetadata; private: FileContentMappings FileContents = {{"header.h", ""}}; @@ -169,7 +202,7 @@ protected: }; // Given string s, change strlen($s.c_str()) to REPLACED. -static RewriteRule ruleStrlenSize() { +static RewriteRuleWith ruleStrlenSize() { StringRef StringExpr = "strexpr"; auto StringType = namedDecl(hasAnyName("::basic_string", "::string")); auto R = makeRule( @@ -886,12 +919,12 @@ TEST_F(TransformerTest, FlattenWithMixedArgs) { TEST_F(TransformerTest, OrderedRuleUnrelated) { StringRef Flag = "flag"; - RewriteRule FlagRule = makeRule( + RewriteRuleWith FlagRule = makeRule( cxxMemberCallExpr(on(expr(hasType(cxxRecordDecl( hasName("proto::ProtoCommandLineFlag")))) .bind(Flag)), unless(callee(cxxMethodDecl(hasName("GetProto"))))), - changeTo(node(std::string(Flag)), cat("PROTO"))); + changeTo(node(std::string(Flag)), cat("PROTO")), cat("")); std::string Input = R"cc( proto::ProtoCommandLineFlag flag; @@ -1657,8 +1690,8 @@ TEST_F(TransformerTest, MultiFileEdit) { makeRule(callExpr(callee(functionDecl(hasName("Func"))), forEachArgumentWithParam(expr().bind("arg"), parmVarDecl().bind("param"))), - editList({changeTo(node("arg"), cat("ARG")), - changeTo(node("param"), cat("PARAM"))})), + {changeTo(node("arg"), cat("ARG")), + changeTo(node("param"), cat("PARAM"))}), [&](Expected> Changes) { if (Changes) ChangeSets.push_back(AtomicChanges(Changes->begin(), Changes->end())); @@ -1682,4 +1715,39 @@ TEST_F(TransformerTest, MultiFileEdit) { "./input.h")))); } +TEST_F(TransformerTest, GeneratesMetadata) { + std::string Input = R"cc(int target = 0;)cc"; + std::string Expected = R"cc(REPLACE)cc"; + RewriteRuleWith Rule = makeRule( + varDecl(hasName("target")), changeTo(cat("REPLACE")), cat("METADATA")); + testRule(std::move(Rule), Input, Expected); + EXPECT_EQ(ErrorCount, 0); + EXPECT_THAT(StringMetadata, UnorderedElementsAre("METADATA")); +} + +TEST_F(TransformerTest, GeneratesMetadataWithNoEdits) { + std::string Input = R"cc(int target = 0;)cc"; + RewriteRuleWith Rule = makeRule( + varDecl(hasName("target")).bind("var"), noEdits(), cat("METADATA")); + testRule(std::move(Rule), Input, Input); + EXPECT_EQ(ErrorCount, 0); + EXPECT_THAT(StringMetadata, UnorderedElementsAre("METADATA")); +} + +TEST_F(TransformerTest, PropagateMetadataErrors) { + class AlwaysFail : public transformer::MatchComputation { + llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &, + std::string *) const override { + return llvm::createStringError(llvm::errc::invalid_argument, "ERROR"); + } + std::string toString() const override { return "AlwaysFail"; } + }; + std::string Input = R"cc(int target = 0;)cc"; + RewriteRuleWith Rule = makeRule( + varDecl(hasName("target")).bind("var"), changeTo(cat("REPLACE")), + std::make_shared()); + testRuleFailure(std::move(Rule), Input); + EXPECT_EQ(ErrorCount, 1); +} + } // namespace -- 2.7.4