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

[ymandel]

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.

[gribozavr2]

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

40

"A map"? it is a function.

Maybe "Maps a match result to..."

Also, "concrete errors" probably should be "concrete edits".

44

Could you explain the idea behind the word "list" in the name? I don't get it.

Maybe "EditGenerator"?

[ymandel]

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

40

agreed on both. will update.

44

It returns a list of edits. So, EditListGenerator is probably most precise, but I wasn't sure that "generator" added much. I'm fine with any of the names, EditGenerator probably being best (if we want Generator in the name) because its concise while still aligning with multiple edits (since a generator often can generate more than one item). WDYT?

[ymandel]

replaced name; fixed comments.

[ymandel]

clang-format

[gribozavr2]

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

44

I suggested "generator" to make some indication that it is not a data structure that stores edits, but some precursor. EditGenerator SGTM.

[ymandel]

Thanks for the review!

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

 
 void TransformerClangTidyCheck::check(
     const ast_matchers::MatchFinder::MatchResult &Result) {
   if (Result.Context->getDiagnostics().hasErrorOccurred())
     return;
 
   assert(Rule && "check() should not fire if Rule is None");
   RewriteRule::Case Case = transformer::detail::findSelectedCase(Result, *Rule);
-  Expected<SmallVector<transformer::detail::Transformation, 1>>
-      Transformations = transformer::detail::translateEdits(Result, Case.Edits);
-  if (!Transformations) {
-    llvm::errs() << "Rewrite failed: "
-                 << llvm::toString(Transformations.takeError()) << "\n";
+  Expected<SmallVector<transformer::Edit, 1>> Edits = Case.Edits(Result);
+  if (!Edits) {
+    llvm::errs() << "Rewrite failed: " << llvm::toString(Edits.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);
   if (!Explanation) {
     llvm::errs() << "Error in explanation: "
                  << llvm::toString(Explanation.takeError()) << "\n";
     return;
   }
 
   // Associate the diagnostic with the location of the first change.
-  DiagnosticBuilder Diag =
-      diag((*Transformations)[0].Range.getBegin(), *Explanation);
-  for (const auto &T : *Transformations)
+  DiagnosticBuilder Diag = diag((*Edits)[0].Range.getBegin(), *Explanation);
+  for (const auto &T : *Edits)
     Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);
 
   for (const auto &I : Case.AddedIncludes) {
     auto &Header = I.first;
     if (Optional<FixItHint> Fix = Inserter->CreateIncludeInsertion(
             Result.SourceManager->getMainFileID(), Header,
             /*IsAngled=*/I.second == transformer::IncludeFormat::Angled)) {
       Diag << *Fix;
     }
   }
 }
 
 } // namespace utils
 } // namespace tidy
 } // namespace clang

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

 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Error.h"
 #include <functional>
 #include <string>
 #include <utility>
 
 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.
 // Includes: an ID for the (bound) node, a selector for source related to the
 // node, a replacement and, optionally, an explanation for the edit.
 //
 // * Target: the source code impacted by the rule. This identifies an AST node,
 //   or part thereof (\c Part), whose source range indicates the extent of the

 //   changeTo(cat("different_expr"))
 // \endcode
 struct ASTEdit {
   RangeSelector TargetRange;
   TextGenerator Replacement;
   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,
   Angled,
 };
 
 /// Description of a source-code transformation.
 //

 // portions of the pattern which are left unspecified, yet bound in the pattern
 // so that we can reference them in the edits.
 //
 // The \c Transformer class can be used to apply the rewrite rule and obtain the
 // corresponding replacements.
 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
     // might have different associated changes to the includes.
     std::vector<std::pair<std::string, IncludeFormat>> AddedIncludes;
   };
   // We expect RewriteRules will most commonly include only one case.
   SmallVector<Case, 1> Cases;
 
   // ID used as the default target of each match. The node described by the
   // matcher is should always be bound to this id.
   static constexpr llvm::StringLiteral RootID = "___root___";
 };
 
 /// Convenience function for constructing a simple \c RewriteRule.
 RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M,
-                     SmallVector<ASTEdit, 1> Edits,
-                     TextGenerator Explanation = nullptr);
+                     EditGenerator Edits, 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;
-  Edits.emplace_back(std::move(Edit));
-  return makeRule(std::move(M), std::move(Edits), std::move(Explanation));
+  return makeRule(std::move(M), edit(std::move(Edit)), 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
 /// could write:
 /// \code
 ///   auto R = makeRule(callExpr(callee(functionDecl(hasName("foo")))),

 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);
-
-/// 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
 
 namespace tooling {
 // DEPRECATED: These are temporary aliases supporting client migration to the
 // `transformer` namespace.
 /// Wraps a string as a TextGenerator.
 using TextGenerator = transformer::TextGenerator;

clang/lib/Tooling/Transformer/RewriteRule.cpp

 using namespace clang;
 using namespace transformer;
 
 using ast_matchers::MatchFinder;
 using ast_matchers::internal::DynTypedMatcher;
 
 using MatchResult = MatchFinder::MatchResult;
 
-Expected<SmallVector<transformer::detail::Transformation, 1>>
-transformer::detail::translateEdits(const MatchResult &Result,
-                                llvm::ArrayRef<ASTEdit> Edits) {
-  SmallVector<transformer::detail::Transformation, 1> Transformations;
-  for (const auto &Edit : Edits) {
-    Expected<CharSourceRange> Range = Edit.TargetRange(Result);
+static Expected<SmallVector<transformer::Edit, 1>>
+translateEdits(const MatchResult &Result, ArrayRef<ASTEdit> ASTEdits) {
+  SmallVector<transformer::Edit, 1> Edits;
+  for (const auto &E : ASTEdits) {
+    Expected<CharSourceRange> Range = E.TargetRange(Result);
     if (!Range)
       return Range.takeError();
     llvm::Optional<CharSourceRange> EditRange =
         tooling::getRangeForEdit(*Range, *Result.Context);
     // 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>();
-    auto Replacement = Edit.Replacement->eval(Result);
+      return SmallVector<Edit, 0>();
+    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;
 }
 
 namespace {
 /// A \c TextGenerator that always returns a fixed string.
 class SimpleTextGenerator : public MatchComputation<std::string> {
   std::string S;
public:
   }
 };
 } // namespace
 
 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,
+                                  EditGenerator Edits,
                                   TextGenerator Explanation) {
   return RewriteRule{{RewriteRule::Case{
       std::move(M), std::move(Edits), std::move(Explanation), {}}}};
 }
 
 void transformer::addInclude(RewriteRule &Rule, StringRef Header,
                          IncludeFormat Format) {
   for (auto &Case : Rule.Cases)
     Case.AddedIncludes.emplace_back(Header.str(), Format);

clang/lib/Tooling/Transformer/Transformer.cpp

 }
 
 void Transformer::run(const MatchFinder::MatchResult &Result) {
   if (Result.Context->getDiagnostics().hasErrorOccurred())
     return;
 
   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;
   }
 
   if (Transformations->empty()) {
     // No rewrite applied (but no error encountered either).
     transformer::detail::getRuleMatchLoc(Result).print(