Change metadata to deferred evalutaion in Clang Transformer.

[asoffer]

Summary

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

[gribozavr2]

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

270–290

What is "this" who is "we"? Should this be an implementation comment instead (in the function body next to the relevant line)?

[ymandel]

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

92–96

Why default, especially when we don't default the others? Is it worth a comment?

271

Why generalize to support any callable? Perhaps worth explaining in comment?

[asoffer]

Add comments describing why we provide defaults for Metadata generation and design of withMetadata function template.

[asoffer]

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

92–96

Added comments.

270–290

Added comments.

[ymandel]

Looks like your changes to the .cpp and test files were reverted...

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

93

nit: typos

The same applies to Note. Since this is a nullable type, can we ask that the user check Metadata != nullptr?

269–270

I think I understand now. Is the idea that we'll only get one implicit construction from the compiler, so we want to use that "free" conversion on the llvm::Any, rather than on the llvm::Expected, which would force the user to explicitly construct the llvm::Any?

I think we should address this with separate functions (or overloads at least), one for the case of never-fail metadata constructors and one for failable constructors. That said, any computation that takes the matchresult is prone to failure because of unbound nodes. so, I would expect it to be common for the Callable to be failable. however, if you feel that's the uncommon case, let's optimize towards the common case, like you've done.

With all that said, I agree that if the Callable returns an Expected we should rewrap correctly, not bury in Any.

[asoffer]

Typo fix.

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

93

I don't think I'm understanding the question. Where/when would users check for Metadata != nullptr?

Currently in this diff, any library construction of the Metadata field will be non-null. Users would have to explicitly pass null in if they wanted it to be null which would pretty quickly break when the generator is called, so this seems like a not-too-big foot-gun? Does that answer the question?

269–270

I think you're asking about why the generic callable instead of a std::function or LLVM equivalent type. It's not about implicit conversions but rather about deduction. Type deduction allows only for conversions on qualifiers (T to const T&, for example). So if we specified a std::function, it would either require:

1. The cannot pass lambdas without explicitly wrapping them in a std::function construction so that the return type can be deduced.
2. We have an explicit std::function<llvm::Any(...)>, but then the user-provided callable would have to return an llvm::Any explicitly.

Neither of these seem ideal.

The Expected-unwrapping requires some ugly SFINAE, so I'd prefer to punt on this until we feel the need to have Expected's in metadata.

[ymandel]

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

93

Sorry, I meant code that consumes this struct. Mostly that's just translateEdits. I realize now that Note is not a great example, since we totally ignore it. However, it is intended as optional, so if we had the code, it would be checking it for null first. Conversely, TargetRange and Replacement are assumed to never be null, yet we don't set them to default values here. So, I think the first step is to decide if this is optional, and (if not) the second is to consistently handle non-optional values in this struct.

So, your code below would become something like:

llvm::Any Metadata;
if (E.Metadata != nullptr) {
  if (auto M = E.Metadata(Result))
    Metadata = std::move(*M);
  else
    return M.takeError();
}

269–270

Punting is fine with me. I'm not against making the user explicitly create the Any, but I don't have a strong opinion on this.

[asoffer]

Fix formatting.

[asoffer]

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

93

I don't see any particular reason to allow this field to take on it's null value. A null check could essentially avoid attaching metadata, but that's also what the default lambda would do here (because Any is also a nullable type).

That being said, sometimes I'm a bit UB-trigger-happy.

[ymandel]

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

93

Sure. The lack of null check is consistent with the other two fields. so, that just leaves it inconsistent with Note, which doesn't have as easy an answer (since string isn't nullable). Given that Note isn't used (yet), let's punt on the consistency issue until we deal with Note properly. You're good to go. Thanks for talking this through!

93

typo: requierd

[asoffer]

Typo fix.

[asoffer]

Construct types for default metadata explicitly.

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

 /// 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>>;
 
+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.
 //
 // * 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
 //   replacement applied by the replacement term.  By default, the extent is the
 //   node matched by the pattern term (\c NodePart::Node). Target's are typed

 // use the single-argument override of \c change:
 // \code
 //   changeTo(cat("different_expr"))
 // \endcode
 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 &) {
+    return llvm::Any();
+  };
 };
 
 /// 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

 /// Inserts \p Replacement after \p S, leaving the source selected by \S
 /// unchanged.
 inline ASTEdit insertAfter(RangeSelector S, TextGenerator Replacement) {
   return changeTo(after(std::move(S)), std::move(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
 /// API. We expose them for use in implementing the fixtures that interpret
 /// RewriteRule, like Transformer and TransfomerTidy, or for more advanced
 /// users.
 //
 // FIXME: These functions are really public, if advanced, elements of the

clang/lib/Tooling/Transformer/RewriteRule.cpp

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<Edit, 0>();
     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;
 }
 
 EditGenerator transformer::editList(SmallVector<ASTEdit, 1> Edits) {
   return [Edits = std::move(Edits)](const MatchResult &Result) {
     return translateEdits(Result, Edits);

clang/unittests/Tooling/TransformerTest.cpp

TEST_F(TransformerTest, RemoveEdit) {
 
   StringRef Decl = "decl";
   testRule(
       makeRule(declStmt().bind(Decl), remove(statement(std::string(Decl)))),
       Input, Expected);
 }
 
 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;
       return 7;
     }
   )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);
   EXPECT_TRUE(runToolOnCodeWithArgs(
       Factory->create(), Input, std::vector<std::string>(), "input.cc",
       "clang-tool", std::make_shared<PCHContainerOperations>(), {}));
   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) {
   std::string Input = R"cc(
     void foo() {
       if (10 > 1.0)
         log(1) << "oh no!";
       else