[clang] e5b3202 - [libTooling] In Clang Transformer, change `Metadata` field to deferred evaluation.

[Yitzhak Mandelbaum via cfe-commits]

Author: Andy Soffer
Date: 2020-07-21T18:05:49Z
New Revision: e5b3202b6f9484590c9e70b8bb82d2778d1ca4fe

URL: https://github.com/llvm/llvm-project/commit/e5b3202b6f9484590c9e70b8bb82d2778d1ca4fe
DIFF: https://github.com/llvm/llvm-project/commit/e5b3202b6f9484590c9e70b8bb82d2778d1ca4fe.diff

LOG: [libTooling] In Clang Transformer, change `Metadata` field to deferred evaluation.

`Metadata` is being changed from an `llvm::Any` to a `MatchConsumer<llvm::Any>`
so that it's evaluation can be be dependent on on `MatchResult`s passed in.

Reviewed By: ymandel, gribozavr2

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

Added: 
    

Modified: 
    clang/include/clang/Tooling/Transformer/RewriteRule.h
    clang/lib/Tooling/Transformer/RewriteRule.cpp
    clang/unittests/Tooling/TransformerTest.cpp

Removed: 
    


################################################################################
diff  --git a/clang/include/clang/Tooling/Transformer/RewriteRule.h b/clang/include/clang/Tooling/Transformer/RewriteRule.h
index d9e68717d5c8..1be572736460 100644
--- a/clang/include/clang/Tooling/Transformer/RewriteRule.h
+++ b/clang/include/clang/Tooling/Transformer/RewriteRule.h
@@ -47,6 +47,8 @@ using EditGenerator = MatchConsumer<llvm::SmallVector<Edit, 1>>;
 
 using TextGenerator = std::shared_ptr<MatchComputation<std::string>>;
 
+using AnyGenerator = MatchConsumer<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.
@@ -87,7 +89,12 @@ struct ASTEdit {
   RangeSelector TargetRange;
   TextGenerator Replacement;
   TextGenerator Note;
-  llvm::Any Metadata;
+  // Not all transformations will want or need to attach metadata and therefore
+  // should not be requierd to do so.
+  AnyGenerator Metadata = [](const ast_matchers::MatchFinder::MatchResult &)
+      -> llvm::Expected<llvm::Any> {
+    return llvm::Expected<llvm::Any>(llvm::Any());
+  };
 };
 
 /// Lifts a list of `ASTEdit`s into an `EditGenerator`.
@@ -261,9 +268,27 @@ inline ASTEdit insertAfter(RangeSelector S, TextGenerator Replacement) {
 /// Removes the source selected by \p S.
 ASTEdit remove(RangeSelector S);
 
-inline ASTEdit withMetadata(ASTEdit edit, llvm::Any Metadata) {
-  edit.Metadata = std::move(Metadata);
-  return edit;
+// FIXME: If `Metadata` returns an `llvm::Expected<T>` the `AnyGenerator` will
+// construct an `llvm::Expected<llvm::Any>` where no error is present but the
+// `llvm::Any` holds the error. This is unlikely but potentially surprising.
+// Perhaps the `llvm::Expected` should be unwrapped, or perhaps this should be a
+// compile-time error. No solution here is perfect.
+//
+// Note: This function template accepts any type callable with a MatchResult
+// rather than a `std::function` because the return-type needs to be deduced. If
+// it accepted a `std::function<R(MatchResult)>`, lambdas or other callable types
+// would not be able to deduce `R`, and users would be forced to specify
+// explicitly the type they intended to return by wrapping the lambda at the
+// call-site.
+template <typename Callable>
+inline ASTEdit withMetadata(ASTEdit Edit, Callable Metadata) {
+  Edit.Metadata =
+      [Gen = std::move(Metadata)](
+          const ast_matchers::MatchFinder::MatchResult &R) -> llvm::Any {
+    return Gen(R);
+  };
+
+  return Edit;
 }
 
 /// The following three functions are a low-level part of the RewriteRule

diff  --git a/clang/lib/Tooling/Transformer/RewriteRule.cpp b/clang/lib/Tooling/Transformer/RewriteRule.cpp
index 995bec03cd66..a212a868c81d 100644
--- a/clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ b/clang/lib/Tooling/Transformer/RewriteRule.cpp
@@ -44,10 +44,13 @@ translateEdits(const MatchResult &Result, ArrayRef<ASTEdit> ASTEdits) {
     auto Replacement = E.Replacement->eval(Result);
     if (!Replacement)
       return Replacement.takeError();
+    auto Metadata = E.Metadata(Result);
+    if (!Metadata)
+      return Metadata.takeError();
     transformer::Edit T;
     T.Range = *EditRange;
     T.Replacement = std::move(*Replacement);
-    T.Metadata = E.Metadata;
+    T.Metadata = std::move(*Metadata);
     Edits.push_back(std::move(T));
   }
   return Edits;

diff  --git a/clang/unittests/Tooling/TransformerTest.cpp b/clang/unittests/Tooling/TransformerTest.cpp
index 7d6b63293748..59b334b0ea5a 100644
--- a/clang/unittests/Tooling/TransformerTest.cpp
+++ b/clang/unittests/Tooling/TransformerTest.cpp
@@ -440,6 +440,12 @@ TEST_F(TransformerTest, RemoveEdit) {
 }
 
 TEST_F(TransformerTest, WithMetadata) {
+  auto makeMetadata = [](const MatchFinder::MatchResult &R) -> llvm::Any {
+    int N =
+        R.Nodes.getNodeAs<IntegerLiteral>("int")->getValue().getLimitedValue();
+    return N;
+  };
+
   std::string Input = R"cc(
     int f() {
       int x = 5;
@@ -448,8 +454,11 @@ TEST_F(TransformerTest, WithMetadata) {
   )cc";
 
   Transformer T(
-      makeRule(declStmt().bind("decl"),
-               withMetadata(remove(statement(std::string("decl"))), 17)),
+      makeRule(
+          declStmt(containsDeclaration(0, varDecl(hasInitializer(
+                                              integerLiteral().bind("int")))))
+              .bind("decl"),
+          withMetadata(remove(statement(std::string("decl"))), makeMetadata)),
       consumer());
   T.registerMatchers(&MatchFinder);
   auto Factory = newFrontendActionFactory(&MatchFinder);
@@ -459,7 +468,7 @@ TEST_F(TransformerTest, WithMetadata) {
   ASSERT_EQ(Changes.size(), 1u);
   const llvm::Any &Metadata = Changes[0].getMetadata();
   ASSERT_TRUE(llvm::any_isa<int>(Metadata));
-  EXPECT_THAT(llvm::any_cast<int>(Metadata), 17);
+  EXPECT_THAT(llvm::any_cast<int>(Metadata), 5);
 }
 
 TEST_F(TransformerTest, MultiChange) {