diff --git a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp --- a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp +++ b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp @@ -17,7 +17,7 @@ #ifndef NDEBUG static bool hasExplanation(const RewriteRule::Case &C) { - return C.Explanation != nullptr; + return C.Metadata != nullptr; } #endif @@ -89,15 +89,21 @@ if (Edits->empty()) return; - Expected Explanation = Case.Explanation->eval(Result); + Expected Explanation = Case.Metadata->eval(Result); if (!Explanation) { llvm::errs() << "Error in explanation: " << llvm::toString(Explanation.takeError()) << "\n"; return; } + if (!llvm::any_isa(*Explanation)) { + llvm::errs() << "Metadata is not string\n"; + return; + } + // Associate the diagnostic with the location of the first change. - DiagnosticBuilder Diag = diag((*Edits)[0].Range.getBegin(), *Explanation); + DiagnosticBuilder Diag = diag((*Edits)[0].Range.getBegin(), + *llvm::any_cast(&*Explanation)); for (const auto &T : *Edits) switch (T.Kind) { case transformer::EditKind::Range: diff --git a/clang/include/clang/Tooling/Transformer/RewriteRule.h b/clang/include/clang/Tooling/Transformer/RewriteRule.h --- a/clang/include/clang/Tooling/Transformer/RewriteRule.h +++ b/clang/include/clang/Tooling/Transformer/RewriteRule.h @@ -65,6 +65,8 @@ using AnyGenerator = MatchConsumer; +using AnyGenerator2 = std::shared_ptr>; + // Description of a source-code edit, expressed in terms of an AST node. // Includes: an ID for the (bound) node, a selector for source related to the // node, a replacement and, optionally, an explanation for the edit. @@ -279,7 +281,7 @@ struct Case { ast_matchers::internal::DynTypedMatcher Matcher; EditGenerator Edits; - TextGenerator Explanation; + AnyGenerator2 Metadata; }; // We expect RewriteRules will most commonly include only one case. SmallVector Cases; @@ -288,25 +290,80 @@ static const llvm::StringRef RootID; }; +namespace detail { +template class AnyWrapper : public MatchComputation { + std::shared_ptr> Wrapped; + +public: + explicit AnyWrapper(std::shared_ptr> Wrapped) + : Wrapped(std::move(Wrapped)) {} + +private: + llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &Match, + llvm::Any *Result) const final { + if (!Result->hasValue()) + *Result = T{}; + else if (!llvm::any_isa(*Result)) + return llvm::make_error( + llvm::errc::invalid_argument, + "accumulated `Any` result has incorrect type"); + + return Wrapped->eval(Match, llvm::any_cast(Result)); + } + + std::string toString() const final { return "Any of " + Wrapped->toString(); } +}; + +template +std::shared_ptr> +wrapGenerator(std::shared_ptr> G) { + if (G) + return std::make_shared>(std::move(G)); + return nullptr; +} + +} // namespace detail + /// Constructs a simple \c RewriteRule. RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M, - EditGenerator Edits, TextGenerator Explanation = nullptr); + EditGenerator Edits, AnyGenerator2 Metadata = nullptr); /// Constructs a \c RewriteRule from multiple `ASTEdit`s. inline RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M, llvm::SmallVector Edits, - TextGenerator Explanation = nullptr) { + AnyGenerator2 Metadata = nullptr) { return makeRule(std::move(M), editList(std::move(Edits)), - std::move(Explanation)); + 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)); + ASTEdit Edit, AnyGenerator2 Metadata = nullptr) { + return makeRule(std::move(M), edit(std::move(Edit)), std::move(Metadata)); } +/// Overload of \c makeRule for the common case of \c std::string metadata. +/// @{ +inline RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M, + EditGenerator Edits, TextGenerator Explanation) { + return makeRule(std::move(M), std::move(Edits), + detail::wrapGenerator(std::move(Explanation))); +} + +inline RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M, + llvm::SmallVector Edits, + TextGenerator Explanation) { + return makeRule(std::move(M), editList(std::move(Edits)), + detail::wrapGenerator(std::move(Explanation))); +} + +inline RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M, + ASTEdit Edit, TextGenerator Explanation) { + return makeRule(std::move(M), edit(std::move(Edit)), + detail::wrapGenerator(std::move(Explanation))); +} +/// @} + /// 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 /// replace a function call and add headers corresponding to the new code, one diff --git a/clang/include/clang/Tooling/Transformer/Transformer.h b/clang/include/clang/Tooling/Transformer/Transformer.h --- a/clang/include/clang/Tooling/Transformer/Transformer.h +++ b/clang/include/clang/Tooling/Transformer/Transformer.h @@ -18,9 +18,52 @@ namespace clang { namespace tooling { + +namespace internal_transformer { +/// Type erasure around the consumer callback that is provided by users of +/// \c Transformer. +class ConsumerAdapter { +public: + virtual ~ConsumerAdapter() = default; + + void onMatch(const ast_matchers::MatchFinder::MatchResult &Result, + const transformer::RewriteRule::Case &Case); + +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, + const transformer::RewriteRule::Case &Case) = 0; +}; + +/// Implementation for when no metadata is expected by the consumer. +class PlainConsumer final : public ConsumerAdapter { + std::function>)> Consumer; + +public: + explicit PlainConsumer( + std::function>)> C) + : Consumer(std::move(C)) { + assert(Consumer && "consumer must not be empty"); + } + +private: + void onMatchImpl(const ast_matchers::MatchFinder::MatchResult &Result, + const transformer::RewriteRule::Case &Case) final; +}; +} // namespace internal_transformer + /// 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 { + transformer::RewriteRule Rule; + std::unique_ptr Adapter; + public: /// Provides the set of changes to the consumer. The callback is free to move /// or destructively consume the changes as needed. @@ -31,17 +74,42 @@ 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; + }; + + /// \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, - ChangeSetConsumer Consumer) - : Rule(std::move(Rule)), Consumer(std::move(Consumer)) { - assert(this->Consumer && "Consumer is empty"); + template >>::value, + int> = 0> + explicit Transformer(transformer::RewriteRule R, ConsumerFn Consumer) + : Rule(std::move(R)), + Adapter(std::make_unique( + std::move(Consumer))) { + assert(llvm::all_of(Rule.Cases, + [](const transformer::RewriteRule::Case &Case) { + return Case.Edits; + }) && + "edit generator must be provided for each case"); } + /// \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::RewriteRule R, + std::function>)> Consumer); + /// N.B. Passes `this` pointer to `MatchFinder`. So, this object should not /// be moved after this call. void registerMatchers(ast_matchers::MatchFinder *MatchFinder); @@ -49,13 +117,73 @@ /// Not called directly by users -- called by the framework, via base class /// pointer. void run(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +namespace internal_transformer { +/// Implementation when metadata is expected by the consumer. +template class MetadataConsumer final : public ConsumerAdapter { + std::function>)> Consumer; + +public: + explicit MetadataConsumer( + std::function>)> C) + : Consumer(std::move(C)) { + assert(Consumer && "consumer must not be empty"); + } private: - transformer::RewriteRule Rule; - /// Receives sets of successful rewrites as an - /// \c llvm::ArrayRef. - ChangeSetConsumer Consumer; + void onMatchImpl(const ast_matchers::MatchFinder::MatchResult &Result, + const transformer::RewriteRule::Case &Case) final { + auto Transformations = Case.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 = Case.Metadata->eval(Result); + if (!Metadata) { + Consumer(Metadata.takeError()); + return; + } + + if (!llvm::any_isa(*Metadata)) { + Consumer(llvm::make_error( + llvm::errc::invalid_argument, "Metadata is not correct type")); + return; + } + + Consumer(Transformer::Result{ + llvm::MutableArrayRef(Changes), + std::move(*llvm::any_cast(&*Metadata))}); + } }; +} // namespace internal_transformer + +template +Transformer::Transformer( + transformer::RewriteRule R, + std::function>)> Consumer) + : Rule(std::move(R)), + Adapter(std::make_unique>( + std::move(Consumer))) { + assert(llvm::all_of(Rule.Cases, + [](const transformer::RewriteRule::Case &Case) { + return Case.Edits && Case.Metadata; + }) && + "edit and metadata generator must be provided for each case"); +} + } // namespace tooling } // namespace clang diff --git a/clang/lib/Tooling/Transformer/RewriteRule.cpp b/clang/lib/Tooling/Transformer/RewriteRule.cpp --- a/clang/lib/Tooling/Transformer/RewriteRule.cpp +++ b/clang/lib/Tooling/Transformer/RewriteRule.cpp @@ -167,9 +167,9 @@ } RewriteRule transformer::makeRule(DynTypedMatcher M, EditGenerator Edits, - TextGenerator Explanation) { - return RewriteRule{{RewriteRule::Case{std::move(M), std::move(Edits), - std::move(Explanation)}}}; + AnyGenerator2 Metadata) { + return RewriteRule{ + {RewriteRule::Case{std::move(M), std::move(Edits), std::move(Metadata)}}}; } namespace { @@ -430,7 +430,7 @@ // `MatchResult`. const RewriteRule::Case & transformer::detail::findSelectedCase(const MatchResult &Result, - const RewriteRule &Rule) { + const RewriteRule &Rule) { if (Rule.Cases.size() == 1) return Rule.Cases[0]; diff --git a/clang/lib/Tooling/Transformer/Transformer.cpp b/clang/lib/Tooling/Transformer/Transformer.cpp --- a/clang/lib/Tooling/Transformer/Transformer.cpp +++ b/clang/lib/Tooling/Transformer/Transformer.cpp @@ -16,35 +16,30 @@ #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 internal_transformer { -void Transformer::run(const MatchFinder::MatchResult &Result) { +void ConsumerAdapter::onMatch( + const ast_matchers::MatchFinder::MatchResult &Result, + const transformer::RewriteRule::Case &Case) { 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, Case); +} +llvm::Expected> +ConsumerAdapter::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 +50,7 @@ case transformer::EditKind::Range: if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) { - Consumer(std::move(Err)); - return; + return Err; } break; case transformer::EditKind::AddInclude: @@ -69,5 +63,43 @@ Changes.reserve(ChangesByFileID.size()); for (auto &IDChangePair : ChangesByFileID) Changes.push_back(std::move(IDChangePair.second)); - Consumer(llvm::MutableArrayRef(Changes)); + + return Changes; +} + +void PlainConsumer::onMatchImpl(const MatchFinder::MatchResult &Result, + const transformer::RewriteRule::Case &Case) { + auto Transformations = Case.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 internal_transformer + +void Transformer::registerMatchers(MatchFinder *MatchFinder) { + for (auto &Matcher : transformer::detail::buildMatchers(Rule)) + MatchFinder->addDynamicMatcher(Matcher, this); +} + +void Transformer::run(const MatchFinder::MatchResult &Result) { + if (Result.Context->getDiagnostics().hasErrorOccurred()) + return; + + Adapter->onMatch(Result, transformer::detail::findSelectedCase(Result, Rule)); +} + +} // namespace tooling +} // namespace clang 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 @@ -31,6 +31,7 @@ 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; @@ -137,27 +138,59 @@ }; } - template - void testRule(R Rule, StringRef Input, StringRef Expected) { + std::function>)> + 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 then 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 testRuleWithMetadata(RewriteRule 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 testRuleFailureWithMetadata(RewriteRule 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", ""}}; @@ -1682,4 +1715,40 @@ "./input.h")))); } +TEST_F(TransformerTest, GeneratesMetadata) { + std::string Input = R"cc(int target = 0;)cc"; + std::string Expected = R"cc(REPLACE)cc"; + testRuleWithMetadata(makeRule(varDecl(hasName("target")), + edit(changeTo(cat("REPLACE"))), + cat("METADATA")), + Input, Expected); + EXPECT_EQ(ErrorCount, 0); + EXPECT_THAT(StringMetadata, UnorderedElementsAre("METADATA")); +} + +TEST_F(TransformerTest, GeneratesMetadataWithNoEdits) { + std::string Input = R"cc(int target = 0;)cc"; + testRuleWithMetadata(makeRule(varDecl(hasName("target")).bind("var"), + noEdits(), cat("METADATA")), + 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"; + testRuleFailureWithMetadata(makeRule(varDecl(hasName("target")).bind("var"), + edit(changeTo(cat("REPLACE"))), + std::make_shared()), + Input); + EXPECT_EQ(ErrorCount, 1); +} + } // namespace