[libTooling] Simplify the representation of Transformer's RewriteRules.

Summary:
This revision simplifies the representation of edits in rewrite rules. The
simplified form is more general, allowing the user more flexibility in building
custom edit specifications.

The changes extend the API, without changing the signature of existing
functions. So this only risks breaking users that directly accessed the
`RewriteRule` struct.

Reviewers: gribozavr2

Subscribers: jfb, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D77419

clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp

@@ -73,16 +73,15 @@ void TransformerClangTidyCheck::check(
 
   assert(Rule && "check() should not fire if Rule is None");
   RewriteRule::Case Case = transformer::detail::findSelectedCase(Result, *Rule);
-  Expected<SmallVector<transformer::detail::Transformation, 1>>
+  Expected<SmallVector<transformer::Edit, 1>> Edits = Case.Edits(Result);
-      Transformations = transformer::detail::translateEdits(Result, Case.Edits);
+  if (!Edits) {
-  if (!Transformations) {
+    llvm::errs() << "Rewrite failed: " << llvm::toString(Edits.takeError())
-    llvm::errs() << "Rewrite failed: "
+                 << "\n";
-                 << llvm::toString(Transformations.takeError()) << "\n";
     return;
   }
 
   // No rewrite applied, but no error encountered either.
-  if (Transformations->empty())
+  if (Edits->empty())
     return;
 
   Expected<std::string> Explanation = Case.Explanation->eval(Result);
@@ -93,9 +92,8 @@ void TransformerClangTidyCheck::check(
   }
 
   // Associate the diagnostic with the location of the first change.
-  DiagnosticBuilder Diag =
+  DiagnosticBuilder Diag = diag((*Edits)[0].Range.getBegin(), *Explanation);
-      diag((*Transformations)[0].Range.getBegin(), *Explanation);
+  for (const auto &T : *Edits)
-  for (const auto &T : *Transformations)
     Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);
 
   for (const auto &I : Case.AddedIncludes) {

clang/include/clang/Tooling/Transformer/RewriteRule.h

@@ -30,6 +30,19 @@
 
 namespace clang {
 namespace transformer {
+/// A concrete description of a source edit, represented by a character range in
+/// the source to be replaced and a corresponding replacement string.
+struct Edit {
+  CharSourceRange Range;
+  std::string Replacement;
+};
+
+/// Maps a match result to a list of concrete edits (with possible
+/// failure). This type is a building block of rewrite rules, but users will
+/// generally work in terms of `ASTEdit`s (below) rather than directly in terms
+/// of `EditGenerator`.
+using EditGenerator = MatchConsumer<llvm::SmallVector<Edit, 1>>;
+
 using TextGenerator = std::shared_ptr<MatchComputation<std::string>>;
 
 // Description of a source-code edit, expressed in terms of an AST node.
@@ -74,6 +87,19 @@ struct ASTEdit {
   TextGenerator Note;
 };
 
+/// Lifts a list of `ASTEdit`s into an `EditGenerator`.
+///
+/// The `EditGenerator` will return an empty vector if any of the edits apply to
+/// portions of the source that are ineligible for rewriting (certain
+/// interactions with macros, for example) and it will fail if any invariants
+/// are violated relating to bound nodes in the match.  However, it does not
+/// fail in the case of conflicting edits -- conflict handling is left to
+/// clients.  We recommend use of the \c AtomicChange or \c Replacements classes
+/// for assistance in detecting such conflicts.
+EditGenerator editList(llvm::SmallVector<ASTEdit, 1> Edits);
+// Convenience form of `editList` for a single edit.
+EditGenerator edit(ASTEdit);
+
 /// Format of the path in an include directive -- angle brackets or quotes.
 enum class IncludeFormat {
   Quoted,
@@ -106,7 +132,7 @@ enum class IncludeFormat {
 struct RewriteRule {
   struct Case {
     ast_matchers::internal::DynTypedMatcher Matcher;
-    SmallVector<ASTEdit, 1> Edits;
+    EditGenerator Edits;
     TextGenerator Explanation;
     // Include paths to add to the file affected by this case.  These are
     // bundled with the `Case`, rather than the `RewriteRule`, because each case
@@ -123,16 +149,22 @@ struct RewriteRule {
 
 /// Convenience function for constructing a simple \c RewriteRule.
 RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M,
-                     SmallVector<ASTEdit, 1> Edits,
+                     EditGenerator Edits, TextGenerator Explanation = nullptr);
-                     TextGenerator Explanation = nullptr);
+
+/// Convenience function for constructing a \c RewriteRule from multiple
+/// `ASTEdit`s.
+inline RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M,
+                            llvm::SmallVector<ASTEdit, 1> Edits,
+                            TextGenerator Explanation = nullptr) {
+  return makeRule(std::move(M), editList(std::move(Edits)),
+                  std::move(Explanation));
+}
 
 /// Convenience overload of \c makeRule for common case of only one edit.
 inline RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M,
                             ASTEdit Edit,
                             TextGenerator Explanation = nullptr) {
-  SmallVector<ASTEdit, 1> Edits;
+  return makeRule(std::move(M), edit(std::move(Edit)), std::move(Explanation));
-  Edits.emplace_back(std::move(Edit));
-  return makeRule(std::move(M), std::move(Edits), std::move(Explanation));
 }
 
 /// For every case in Rule, adds an include directive for the given header. The
@@ -260,28 +292,6 @@ getRuleMatchLoc(const ast_matchers::MatchFinder::MatchResult &Result);
 const RewriteRule::Case &
 findSelectedCase(const ast_matchers::MatchFinder::MatchResult &Result,
                  const RewriteRule &Rule);
-
-/// 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 translate `Edits`, which are in terms of AST nodes bound in the
-/// match `Result`, into Transformations, which are in terms of the source code
-/// text.
-///
-/// Returns an empty vector if any of the edits apply to portions of the source
-/// that are ineligible for rewriting (certain interactions with macros, for
-/// example).  Fails if any invariants are violated relating to bound nodes in
-/// the match.  However, it does not fail in the case of conflicting edits --
-/// conflict handling is left to clients.  We recommend use of the \c
-/// AtomicChange or \c Replacements classes for assistance in detecting such
-/// conflicts.
-Expected<SmallVector<Transformation, 1>>
-translateEdits(const ast_matchers::MatchFinder::MatchResult &Result,
-               llvm::ArrayRef<ASTEdit> Edits);
 } // namespace detail
 } // namespace transformer
 

clang/lib/Tooling/Transformer/RewriteRule.cpp

@@ -28,12 +28,11 @@ using ast_matchers::internal::DynTypedMatcher;
 
 using MatchResult = MatchFinder::MatchResult;
 
-Expected<SmallVector<transformer::detail::Transformation, 1>>
+static Expected<SmallVector<transformer::Edit, 1>>
-transformer::detail::translateEdits(const MatchResult &Result,
+translateEdits(const MatchResult &Result, ArrayRef<ASTEdit> ASTEdits) {
-                                llvm::ArrayRef<ASTEdit> Edits) {
+  SmallVector<transformer::Edit, 1> Edits;
-  SmallVector<transformer::detail::Transformation, 1> Transformations;
+  for (const auto &E : ASTEdits) {
-  for (const auto &Edit : Edits) {
+    Expected<CharSourceRange> Range = E.TargetRange(Result);
-    Expected<CharSourceRange> Range = Edit.TargetRange(Result);
     if (!Range)
       return Range.takeError();
     llvm::Optional<CharSourceRange> EditRange =
@@ -41,21 +40,33 @@ transformer::detail::translateEdits(const MatchResult &Result,
     // FIXME: let user specify whether to treat this case as an error or ignore
     // it as is currently done.
     if (!EditRange)
-      return SmallVector<Transformation, 0>();
+      return SmallVector<Edit, 0>();
-    auto Replacement = Edit.Replacement->eval(Result);
+    auto Replacement = E.Replacement->eval(Result);
     if (!Replacement)
       return Replacement.takeError();
-    transformer::detail::Transformation T;
+    transformer::Edit T;
     T.Range = *EditRange;
     T.Replacement = std::move(*Replacement);
-    Transformations.push_back(std::move(T));
+    Edits.push_back(std::move(T));
   }
-  return Transformations;
+  return Edits;
 }
 
-ASTEdit transformer::changeTo(RangeSelector S, TextGenerator Replacement) {
+EditGenerator transformer::editList(SmallVector<ASTEdit, 1> Edits) {
+  return [Edits = std::move(Edits)](const MatchResult &Result) {
+    return translateEdits(Result, Edits);
+  };
+}
+
+EditGenerator transformer::edit(ASTEdit Edit) {
+  return [Edit = std::move(Edit)](const MatchResult &Result) {
+    return translateEdits(Result, {Edit});
+  };
+}
+
+ASTEdit transformer::changeTo(RangeSelector Target, TextGenerator Replacement) {
   ASTEdit E;
-  E.TargetRange = std::move(S);
+  E.TargetRange = std::move(Target);
   E.Replacement = std::move(Replacement);
   return E;
 }
@@ -82,8 +93,9 @@ ASTEdit transformer::remove(RangeSelector S) {
   return change(std::move(S), std::make_shared<SimpleTextGenerator>(""));
 }
 
-RewriteRule transformer::makeRule(DynTypedMatcher M, SmallVector<ASTEdit, 1> Edits,
+RewriteRule transformer::makeRule(ast_matchers::internal::DynTypedMatcher M,
-                              TextGenerator Explanation) {
+                                  EditGenerator Edits,
+                                  TextGenerator Explanation) {
   return RewriteRule{{RewriteRule::Case{
       std::move(M), std::move(Edits), std::move(Explanation), {}}}};
 }

clang/lib/Tooling/Transformer/Transformer.cpp

@@ -31,7 +31,7 @@ void Transformer::run(const MatchFinder::MatchResult &Result) {
 
   transformer::RewriteRule::Case Case =
       transformer::detail::findSelectedCase(Result, Rule);
-  auto Transformations = transformer::detail::translateEdits(Result, Case.Edits);
+  auto Transformations = Case.Edits(Result);
   if (!Transformations) {
     Consumer(Transformations.takeError());
     return;