li.zhe.hua created this revision.
li.zhe.hua added a reviewer: ymandel.
Herald added a subscriber: carlosgalvezp.
li.zhe.hua requested review of this revision.
Herald added projects: clang, clang-tools-extra.
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).
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D120360
Files:
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
clang/include/clang/Tooling/Transformer/RewriteRule.h
clang/include/clang/Tooling/Transformer/Transformer.h
clang/lib/Tooling/Transformer/RewriteRule.cpp
clang/lib/Tooling/Transformer/Transformer.cpp
clang/unittests/Tooling/TransformerTest.cpp
Index: clang/unittests/Tooling/TransformerTest.cpp
===================================================================
--- clang/unittests/Tooling/TransformerTest.cpp
+++ 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 <typename R>
- void testRule(R Rule, StringRef Input, StringRef Expected) {
+ std::function<void(Expected<Transformer::Result<std::string>>)>
+ consumerWithStringMetadata() {
+ return [this](Expected<Transformer::Result<std::string>> 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<Transformer>(std::move(Rule), consumer()));
Transformers.back()->registerMatchers(&MatchFinder);
compareSnippets(Expected, rewrite(Input));
}
- template <typename R> void testRuleFailure(R Rule, StringRef Input) {
+ void testRuleWithMetadata(RewriteRule Rule, StringRef Input,
+ StringRef Expected) {
+ Transformers.push_back(std::make_unique<Transformer>(
+ std::move(Rule), consumerWithStringMetadata()));
+ Transformers.back()->registerMatchers(&MatchFinder);
+ compareSnippets(Expected, rewrite(Input));
+ }
+
+ void testRuleFailure(RewriteRule Rule, StringRef Input) {
Transformers.push_back(
std::make_unique<Transformer>(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<Transformer>(
+ std::move(Rule), consumerWithStringMetadata()));
+ Transformers.back()->registerMatchers(&MatchFinder);
+ ASSERT_FALSE(rewrite(Input)) << "Expected failure to rewrite code";
+ }
+
// Transformers are referenced by MatchFinder.
std::vector<std::unique_ptr<Transformer>> Transformers;
clang::ast_matchers::MatchFinder MatchFinder;
// Records whether any errors occurred in individual changes.
int ErrorCount = 0;
AtomicChanges Changes;
+ std::vector<std::string> 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<std::string> {
+ 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<AlwaysFail>()),
+ Input);
+ EXPECT_EQ(ErrorCount, 1);
+}
+
} // namespace
Index: clang/lib/Tooling/Transformer/Transformer.cpp
===================================================================
--- clang/lib/Tooling/Transformer/Transformer.cpp
+++ clang/lib/Tooling/Transformer/Transformer.cpp
@@ -16,35 +16,30 @@
#include <utility>
#include <vector>
-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<llvm::SmallVector<AtomicChange, 1>>
+ConsumerAdapter::convertToAtomicChanges(
+ const llvm::SmallVectorImpl<transformer::Edit> &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<FileID, AtomicChange> 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<AtomicChange>(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<AtomicChange>(*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
Index: clang/lib/Tooling/Transformer/RewriteRule.cpp
===================================================================
--- clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ 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];
Index: clang/include/clang/Tooling/Transformer/Transformer.h
===================================================================
--- clang/include/clang/Tooling/Transformer/Transformer.h
+++ 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<llvm::SmallVector<AtomicChange, 1>>
+ convertToAtomicChanges(const llvm::SmallVectorImpl<transformer::Edit> &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<void(Expected<llvm::MutableArrayRef<AtomicChange>>)> Consumer;
+
+public:
+ explicit PlainConsumer(
+ std::function<void(Expected<llvm::MutableArrayRef<AtomicChange>>)> 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<internal_transformer::ConsumerAdapter> 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<void(
Expected<llvm::MutableArrayRef<AtomicChange>> Changes)>;
- /// \param Consumer Receives all rewrites for a single match, or an error.
+ template <typename T> struct Result {
+ llvm::MutableArrayRef<AtomicChange> 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 <typename ConsumerFn,
+ std::enable_if_t<
+ llvm::is_invocable<
+ ConsumerFn,
+ llvm::Expected<llvm::MutableArrayRef<AtomicChange>>>::value,
+ int> = 0>
+ explicit Transformer(transformer::RewriteRule R, ConsumerFn Consumer)
+ : Rule(std::move(R)),
+ Adapter(std::make_unique<internal_transformer::PlainConsumer>(
+ 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 <typename T>
+ explicit Transformer(transformer::RewriteRule R,
+ std::function<void(llvm::Expected<Result<T>>)> 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 <typename T> class MetadataConsumer final : public ConsumerAdapter {
+ std::function<void(llvm::Expected<Transformer::Result<T>>)> Consumer;
+
+public:
+ explicit MetadataConsumer(
+ std::function<void(llvm::Expected<Transformer::Result<T>>)> 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<AtomicChange>.
- 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<AtomicChange, 1> 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<T>(*Metadata)) {
+ Consumer(llvm::make_error<llvm::StringError>(
+ llvm::errc::invalid_argument, "Metadata is not correct type"));
+ return;
+ }
+
+ Consumer(Transformer::Result<T>{
+ llvm::MutableArrayRef<AtomicChange>(Changes),
+ std::move(*llvm::any_cast<std::string>(&*Metadata))});
+ }
};
+} // namespace internal_transformer
+
+template <typename T>
+Transformer::Transformer(
+ transformer::RewriteRule R,
+ std::function<void(llvm::Expected<Result<T>>)> Consumer)
+ : Rule(std::move(R)),
+ Adapter(std::make_unique<internal_transformer::MetadataConsumer<T>>(
+ 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
Index: clang/include/clang/Tooling/Transformer/RewriteRule.h
===================================================================
--- clang/include/clang/Tooling/Transformer/RewriteRule.h
+++ clang/include/clang/Tooling/Transformer/RewriteRule.h
@@ -65,6 +65,8 @@
using AnyGenerator = MatchConsumer<llvm::Any>;
+using AnyGenerator2 = std::shared_ptr<MatchComputation<llvm::Any>>;
+
// 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<Case, 1> Cases;
@@ -288,25 +290,80 @@
static const llvm::StringRef RootID;
};
+namespace detail {
+template <typename T> class AnyWrapper : public MatchComputation<llvm::Any> {
+ std::shared_ptr<MatchComputation<T>> Wrapped;
+
+public:
+ explicit AnyWrapper(std::shared_ptr<MatchComputation<T>> 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<T>(*Result))
+ return llvm::make_error<llvm::StringError>(
+ llvm::errc::invalid_argument,
+ "accumulated `Any` result has incorrect type");
+
+ return Wrapped->eval(Match, llvm::any_cast<T>(Result));
+ }
+
+ std::string toString() const final { return "Any of " + Wrapped->toString(); }
+};
+
+template <typename T>
+std::shared_ptr<MatchComputation<llvm::Any>>
+wrapGenerator(std::shared_ptr<MatchComputation<T>> G) {
+ if (G)
+ return std::make_shared<AnyWrapper<T>>(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<ASTEdit, 1> 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<ASTEdit, 1> 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
Index: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
+++ 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<std::string> Explanation = Case.Explanation->eval(Result);
+ Expected<llvm::Any> Explanation = Case.Metadata->eval(Result);
if (!Explanation) {
llvm::errs() << "Error in explanation: "
<< llvm::toString(Explanation.takeError()) << "\n";
return;
}
+ if (!llvm::any_isa<std::string>(*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<std::string>(&*Explanation));
for (const auto &T : *Edits)
switch (T.Kind) {
case transformer::EditKind::Range:
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
