diff --git a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp index 1c31cf56c68ab4..515997b142313d 100644 --- a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp +++ b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp @@ -85,7 +85,7 @@ void TransformerClangTidyCheck::check( if (Transformations->empty()) return; - Expected Explanation = Case.Explanation(Result); + Expected Explanation = Case.Explanation->eval(Result); if (!Explanation) { llvm::errs() << "Error in explanation: " << llvm::toString(Explanation.takeError()) << "\n"; diff --git a/clang/include/clang/Tooling/Transformer/MatchConsumer.h b/clang/include/clang/Tooling/Transformer/MatchConsumer.h index 0a1dbe13ea1e41..f407ffce3d2526 100644 --- a/clang/include/clang/Tooling/Transformer/MatchConsumer.h +++ b/clang/include/clang/Tooling/Transformer/MatchConsumer.h @@ -51,6 +51,53 @@ MatchConsumer ifBound(std::string ID, MatchConsumer TrueC, return (Map.find(ID) != Map.end() ? TrueC : FalseC)(Result); }; } + +/// A failable computation over nodes bound by AST matchers, with (limited) +/// reflection via the `toString` method. +/// +/// The computation should report any errors though its return value (rather +/// than terminating the program) to enable usage in interactive scenarios like +/// clang-query. +/// +/// This is a central abstraction of the Transformer framework. It is a +/// generalization of `MatchConsumer` and intended to replace it. +template class MatchComputation { +public: + virtual ~MatchComputation() = default; + + /// Evaluates the computation and (potentially) updates the accumulator \c + /// Result. \c Result is undefined in the case of an error. `Result` is an + /// out parameter to optimize case where the computation involves composing + /// the result of sub-computation evaluations. + virtual llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &Match, + T *Result) const = 0; + + /// Convenience version of `eval`, for the case where the computation is being + /// evaluated on its own. + llvm::Expected eval(const ast_matchers::MatchFinder::MatchResult &R) const; + + /// Constructs a string representation of the computation, for informational + /// purposes. The representation must be deterministic, but is not required to + /// be unique. + virtual std::string toString() const = 0; + +protected: + MatchComputation() = default; + + // Since this is an abstract class, copying/assigning only make sense for + // derived classes implementing `clone()`. + MatchComputation(const MatchComputation &) = default; + MatchComputation &operator=(const MatchComputation &) = default; +}; + +template +llvm::Expected MatchComputation::eval( + const ast_matchers::MatchFinder::MatchResult &R) const { + T Output; + if (auto Err = eval(R, &Output)) + return std::move(Err); + return Output; +} } // namespace transformer namespace tooling { diff --git a/clang/include/clang/Tooling/Transformer/RewriteRule.h b/clang/include/clang/Tooling/Transformer/RewriteRule.h index 11ff2bc9867fd2..7daf6ea154bede 100644 --- a/clang/include/clang/Tooling/Transformer/RewriteRule.h +++ b/clang/include/clang/Tooling/Transformer/RewriteRule.h @@ -30,7 +30,7 @@ namespace clang { namespace transformer { -using TextGenerator = MatchConsumer; +using TextGenerator = 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 @@ -223,11 +223,7 @@ inline ASTEdit insertAfter(RangeSelector S, TextGenerator Replacement) { } /// Removes the source selected by \p S. -inline ASTEdit remove(RangeSelector S) { - return change(std::move(S), - [](const ast_matchers::MatchFinder::MatchResult &) - -> Expected { return ""; }); -} +ASTEdit remove(RangeSelector S); /// The following three functions are a low-level part of the RewriteRule /// API. We expose them for use in implementing the fixtures that interpret @@ -294,10 +290,7 @@ namespace tooling { /// Wraps a string as a TextGenerator. using TextGenerator = transformer::TextGenerator; -inline TextGenerator text(std::string M) { - return [M](const ast_matchers::MatchFinder::MatchResult &) - -> Expected { return M; }; -} +TextGenerator text(std::string M); using transformer::addInclude; using transformer::applyFirst; diff --git a/clang/include/clang/Tooling/Transformer/Stencil.h b/clang/include/clang/Tooling/Transformer/Stencil.h index 5c1139875a1933..dd65e68b7e8af7 100644 --- a/clang/include/clang/Tooling/Transformer/Stencil.h +++ b/clang/include/clang/Tooling/Transformer/Stencil.h @@ -32,33 +32,8 @@ namespace clang { namespace transformer { -class StencilInterface { -public: - virtual ~StencilInterface() = default; - - /// Evaluates this part to a string and appends it to \c Result. \c Result is - /// undefined in the case of an error. `Result` is an out parameter to - /// optimize the expected common case of evaluating a sequence of generators. - virtual llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &Match, - std::string *Result) const = 0; - - /// Convenience version of `eval`, for the case where the generator is being - /// evaluated on its own. - llvm::Expected - eval(const ast_matchers::MatchFinder::MatchResult &R) const; - - /// Constructs a string representation of the StencilInterface. The - /// representation must be deterministic, but is not required to be unique. - virtual std::string toString() const = 0; - -protected: - StencilInterface() = default; - - // Since this is an abstract class, copying/assigning only make sense for - // derived classes implementing `clone()`. - StencilInterface(const StencilInterface &) = default; - StencilInterface &operator=(const StencilInterface &) = default; -}; + +using StencilInterface = MatchComputation; /// A sequence of code fragments, references to parameters and code-generation /// operations that together can be evaluated to (a fragment of) source code or @@ -68,27 +43,13 @@ class StencilInterface { /// Since `StencilInterface` is an immutable interface, the sharing doesn't /// impose any risks. Otherwise, we would have to add a virtual `copy` method to /// the API and implement it for all derived classes. -class Stencil { - std::shared_ptr Impl; - -public: - Stencil() = default; - explicit Stencil(std::shared_ptr Ptr) - : Impl(std::move(Ptr)) {} - - StencilInterface *operator->() const { return Impl.get(); } - - llvm::Expected - operator()(const ast_matchers::MatchFinder::MatchResult &R) { - return Impl->eval(R); - } -}; +using Stencil = std::shared_ptr; namespace detail { /// Convenience function to construct a \c Stencil. Overloaded for common cases /// so that user doesn't need to specify which factory function to use. This /// pattern gives benefits similar to implicit constructors, while maintaing a -/// higher degree of explictness. +/// higher degree of explicitness. Stencil makeStencil(llvm::StringRef Text); Stencil makeStencil(RangeSelector Selector); inline Stencil makeStencil(Stencil S) { return S; } diff --git a/clang/lib/Tooling/Transformer/RewriteRule.cpp b/clang/lib/Tooling/Transformer/RewriteRule.cpp index b96b5eeabad0b8..20d3a371950af9 100644 --- a/clang/lib/Tooling/Transformer/RewriteRule.cpp +++ b/clang/lib/Tooling/Transformer/RewriteRule.cpp @@ -43,7 +43,7 @@ transformer::detail::translateEdits(const MatchResult &Result, // it as is currently done. if (!EditRange) return SmallVector(); - auto Replacement = Edit.Replacement(Result); + auto Replacement = Edit.Replacement->eval(Result); if (!Replacement) return Replacement.takeError(); transformer::detail::Transformation T; @@ -61,6 +61,28 @@ ASTEdit transformer::changeTo(RangeSelector S, TextGenerator Replacement) { return E; } +namespace { +/// A \c TextGenerator that always returns a fixed string. +class SimpleTextGenerator : public MatchComputation { + std::string S; + +public: + SimpleTextGenerator(std::string S) : S(std::move(S)) {} + llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &, + std::string *Result) const override { + Result->append(S); + return llvm::Error::success(); + } + std::string toString() const override { + return (llvm::Twine("text(\"") + S + "\")").str(); + } +}; +} // namespace + +ASTEdit transformer::remove(RangeSelector S) { + return change(std::move(S), std::make_shared("")); +} + RewriteRule transformer::makeRule(DynTypedMatcher M, SmallVector Edits, TextGenerator Explanation) { return RewriteRule{{RewriteRule::Case{ @@ -176,3 +198,7 @@ transformer::detail::findSelectedCase(const MatchResult &Result, } constexpr llvm::StringLiteral RewriteRule::RootID; + +TextGenerator tooling::text(std::string M) { + return std::make_shared(std::move(M)); +} diff --git a/clang/lib/Tooling/Transformer/Stencil.cpp b/clang/lib/Tooling/Transformer/Stencil.cpp index d994fdcf870768..486e18b341f7ca 100644 --- a/clang/lib/Tooling/Transformer/Stencil.cpp +++ b/clang/lib/Tooling/Transformer/Stencil.cpp @@ -272,14 +272,6 @@ template class StencilImpl : public StencilInterface { }; } // namespace -llvm::Expected -StencilInterface::eval(const MatchFinder::MatchResult &R) const { - std::string Output; - if (auto Err = eval(R, &Output)) - return std::move(Err); - return Output; -} - Stencil transformer::detail::makeStencil(StringRef Text) { return text(Text); } Stencil transformer::detail::makeStencil(RangeSelector Selector) { @@ -287,52 +279,50 @@ Stencil transformer::detail::makeStencil(RangeSelector Selector) { } Stencil transformer::text(StringRef Text) { - return Stencil(std::make_shared>(Text)); + return std::make_shared>(Text); } Stencil transformer::selection(RangeSelector Selector) { - return Stencil( - std::make_shared>(std::move(Selector))); + return std::make_shared>(std::move(Selector)); } Stencil transformer::dPrint(StringRef Id) { - return Stencil(std::make_shared>(Id)); + return std::make_shared>(Id); } Stencil transformer::expression(llvm::StringRef Id) { - return Stencil(std::make_shared>( - UnaryNodeOperator::Parens, Id)); + return std::make_shared>( + UnaryNodeOperator::Parens, Id); } Stencil transformer::deref(llvm::StringRef ExprId) { - return Stencil(std::make_shared>( - UnaryNodeOperator::Deref, ExprId)); + return std::make_shared>( + UnaryNodeOperator::Deref, ExprId); } Stencil transformer::addressOf(llvm::StringRef ExprId) { - return Stencil(std::make_shared>( - UnaryNodeOperator::Address, ExprId)); + return std::make_shared>( + UnaryNodeOperator::Address, ExprId); } Stencil transformer::access(StringRef BaseId, Stencil Member) { - return Stencil( - std::make_shared>(BaseId, std::move(Member))); + return std::make_shared>(BaseId, std::move(Member)); } Stencil transformer::ifBound(StringRef Id, Stencil TrueStencil, Stencil FalseStencil) { - return Stencil(std::make_shared>( - Id, std::move(TrueStencil), std::move(FalseStencil))); + return std::make_shared>(Id, std::move(TrueStencil), + std::move(FalseStencil)); } Stencil transformer::run(MatchConsumer Fn) { - return Stencil( - std::make_shared>>(std::move(Fn))); + return std::make_shared>>( + std::move(Fn)); } Stencil transformer::catVector(std::vector Parts) { // Only one argument, so don't wrap in sequence. if (Parts.size() == 1) return std::move(Parts[0]); - return Stencil(std::make_shared>(std::move(Parts))); + return std::make_shared>(std::move(Parts)); } diff --git a/clang/unittests/Tooling/StencilTest.cpp b/clang/unittests/Tooling/StencilTest.cpp index 5bbdfea398ab4f..7f530fe9fbf6a4 100644 --- a/clang/unittests/Tooling/StencilTest.cpp +++ b/clang/unittests/Tooling/StencilTest.cpp @@ -24,7 +24,6 @@ using ::llvm::Failed; using ::llvm::HasValue; using ::llvm::StringError; using ::testing::AllOf; -using ::testing::Eq; using ::testing::HasSubstr; using MatchResult = MatchFinder::MatchResult; @@ -135,26 +134,6 @@ TEST_F(StencilTest, SingleStatement) { HasValue("if (!true) return 0; else return 1;")); } -// Tests `stencil`. -TEST_F(StencilTest, StencilFactoryFunction) { - StringRef Condition("C"), Then("T"), Else("E"); - const std::string Snippet = R"cc( - if (true) - return 1; - else - return 0; - )cc"; - auto StmtMatch = matchStmt( - Snippet, ifStmt(hasCondition(expr().bind(Condition)), - hasThen(stmt().bind(Then)), hasElse(stmt().bind(Else)))); - ASSERT_TRUE(StmtMatch); - // Invert the if-then-else. - auto Consumer = cat("if (!", node(Condition), ") ", statement(Else), " else ", - statement(Then)); - EXPECT_THAT_EXPECTED(Consumer(StmtMatch->Result), - HasValue("if (!true) return 0; else return 1;")); -} - TEST_F(StencilTest, UnboundNode) { const std::string Snippet = R"cc( if (true) diff --git a/clang/unittests/Tooling/TransformerTest.cpp b/clang/unittests/Tooling/TransformerTest.cpp index c99bf974f88e60..c382783319b833 100644 --- a/clang/unittests/Tooling/TransformerTest.cpp +++ b/clang/unittests/Tooling/TransformerTest.cpp @@ -575,13 +575,16 @@ TEST_F(TransformerTest, TextGeneratorFailure) { std::string Input = "int conflictOneRule() { return 3 + 7; }"; // Try to change the whole binary-operator expression AND one its operands: StringRef O = "O"; - auto AlwaysFail = [](const ast_matchers::MatchFinder::MatchResult &) - -> llvm::Expected { - return llvm::createStringError(llvm::errc::invalid_argument, "ERROR"); + 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"; } }; - Transformer T( - makeRule(binaryOperator().bind(O), changeTo(node(O), AlwaysFail)), - consumer()); + Transformer T(makeRule(binaryOperator().bind(O), + changeTo(node(O), std::make_shared())), + consumer()); T.registerMatchers(&MatchFinder); EXPECT_FALSE(rewrite(Input)); EXPECT_THAT(Changes, IsEmpty());