Add Metadata to Transformer tooling

[asoffer]

Summary

This change adds a Metadata field to ASTEdit, Edit, and AtomicChange so that edits can have associated metadata and that metadata can be constructed with Transformer-based RewriteRules. Metadata is ignored when applying edits to source, but other consumers of AtomicChange can use this metadata to direct how they want to consume each edit.

[ymandel]

Looks good! Only real question is one of design -- should we consider the (deeper) change of templating the various types rather than using dynamic typing? For that matter, the decision doesn't even have to be the same for both AtomicChange and the Transformer types. Transformer could be templated while AtomicChange uses any.

I'm inclined towards what you have, but that may just be laziness on part (since this requires minimal changes). I'm curious to hear what Dmitri thinks.

clang/include/clang/Tooling/Refactoring/AtomicChange.h

144

Please add a field comment. Something like what you have in the revision description would be good.

clang/lib/Tooling/Refactoring/AtomicChange.cpp

210

I assume that

: AtomicChange(SM, KeyPosition), Metadata(std::move(M)) {}

didn't work?

clang/unittests/Tooling/RefactoringTest.cpp

1299

any reason not to use int directly as the type carried in the metadata?

clang/unittests/Tooling/TransformerTest.cpp

460

Why expect vs assert?

[asoffer]

In D82226#2114111, @ymandel wrote:

Looks good! Only real question is one of design -- should we consider the (deeper) change of templating the various types rather than using dynamic typing? For that matter, the decision doesn't even have to be the same for both AtomicChange and the Transformer types. Transformer could be templated while AtomicChange uses any.

I think the tradeoff here is
Dynamic typing -- faster compile times, type safety checked at run-time (in tests), lower maintenance cost
Templates -- Faster runtime, type safety checked at compile-time, better user expereience

I don't think the runtime speed is particularly important here in the sense that it's unlikely to be the bottleneck. Given the crash-on-invalid-type semantics, we're essentially trading off usability for maintenance cost. In this case I don't think asking users to do the extra cast is a significant enough burden to warrant the cost of templating everything.

I also don't strongly object to it. The nice thing about llvm::Any, is that this can be mostly changed incrementally if we later decide that's appropriate (only having to change the one getMetadata call at the call-site).

clang/lib/Tooling/Refactoring/AtomicChange.cpp

210

Correct. You can't use a forwarding constructor followed by a field initializer (which I learned by attempt it on this PR).

clang/unittests/Tooling/TransformerTest.cpp

460

EXPECT when the test can reasonably continue to produce useful output without crashing. So here, if the tool fails, we can still safely access Changes, but if Changes.size() != 1, we can't safely index into it.

[asoffer]

Commenting fields and simplifying test.

[asoffer]

Updating other test.

[asoffer]

Last diff was a mistake.

[gribozavr2]

In D82226#2115406, @asoffer wrote:

I think the tradeoff here is
Dynamic typing -- faster compile times, type safety checked at run-time (in tests), lower maintenance cost
Templates -- Faster runtime, type safety checked at compile-time, better user expereience

For me, the more important part of the tradeoff is whether we will have one type or multiple. If we use Any, then all AtomicChange regardless of what produced them, will have the same type. If we use templates, then AtomicChange<T> is a different type from AtomicChange<U>. So based on that I think using Any is a better choice, since infrastructure code would want to handle with AtomicChange objects and not have to be implemented as a template over arbitrary metadata type.

Regarding the metadata itself, WDYT about using a map from string to any instead of just one any? This way multiple layers of the system would be able to attach metadata without interfering.

[ymandel]

In D82226#2116931, @gribozavr2 wrote:

In D82226#2115406, @asoffer wrote:

I think the tradeoff here is
Dynamic typing -- faster compile times, type safety checked at run-time (in tests), lower maintenance cost
Templates -- Faster runtime, type safety checked at compile-time, better user expereience

For me, the more important part of the tradeoff is whether we will have one type or multiple. If we use Any, then all AtomicChange regardless of what produced them, will have the same type. If we use templates, then AtomicChange<T> is a different type from AtomicChange<U>. So based on that I think using Any is a better choice, since infrastructure code would want to handle with AtomicChange objects and not have to be implemented as a template over arbitrary metadata type.

I'm convinced (both yours and Andy's arguments). Andy, please consider whether any of this belongs in comments. (I don't have much opinion).

Regarding the metadata itself, WDYT about using a map from string to any instead of just one any? This way multiple layers of the system would be able to attach metadata without interfering.

Sounds good, but I'm fine either way.

[asoffer]

I'm not against a map, but I don't see the need for it right now. We don't have multiple stages that write to the same AtomicChange in any tool I'm aware of. Given that these are immutable after construction, I think just the Any is simpler. So long as we're willing to break things later (Does LLVM have a policy on API compatibility?) I think it's better to start with the simpler thing.

[asoffer]

Signed-comparison issue in test fixed.

clang/include/clang/Tooling/Refactoring/AtomicChange.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLING_REFACTOR_ATOMICCHANGE_H
 #define LLVM_CLANG_TOOLING_REFACTOR_ATOMICCHANGE_H
 
 #include "clang/Basic/SourceManager.h"
 #include "clang/Format/Format.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/Any.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 
 namespace clang {
 namespace tooling {
 
 /// An atomic change is used to create and group a set of source edits,
 /// e.g. replacements or header insertions. Edits in an AtomicChange should be
 /// related, e.g. replacements for the same type reference and the corresponding
 /// header insertion/deletion.
 ///
 /// An AtomicChange is uniquely identified by a key and will either be fully
 /// applied or not applied at all.
 ///
 /// Calling setError on an AtomicChange stores the error message and marks it as
 /// bad, i.e. none of its source edits will be applied.
 class AtomicChange {
 public:
   /// Creates an atomic change around \p KeyPosition with the key being a
   /// concatenation of the file name and the offset of \p KeyPosition.
   /// \p KeyPosition should be the location of the key syntactical element that
   /// is being changed, e.g. the call to a refactored method.
   AtomicChange(const SourceManager &SM, SourceLocation KeyPosition);
 
+  AtomicChange(const SourceManager &SM, SourceLocation KeyPosition,
+               llvm::Any Metadata);
+
   /// Creates an atomic change for \p FilePath with a customized key.
   AtomicChange(llvm::StringRef FilePath, llvm::StringRef Key)
       : Key(Key), FilePath(FilePath) {}
 
   AtomicChange(AtomicChange &&) = default;
   AtomicChange(const AtomicChange &) = default;
 
   AtomicChange &operator=(AtomicChange &&) = default;
public:
   llvm::ArrayRef<std::string> getInsertedHeaders() const {
     return InsertedHeaders;
   }
 
   llvm::ArrayRef<std::string> getRemovedHeaders() const {
     return RemovedHeaders;
   }
 
+  const llvm::Any &getMetadata() const { return Metadata; }
+
 private:
   AtomicChange() {}
 
   AtomicChange(std::string Key, std::string FilePath, std::string Error,
                std::vector<std::string> InsertedHeaders,
                std::vector<std::string> RemovedHeaders,
                clang::tooling::Replacements Replaces);
 
   // This uniquely identifies an AtomicChange.
   std::string Key;
   std::string FilePath;
   std::string Error;
   std::vector<std::string> InsertedHeaders;
   std::vector<std::string> RemovedHeaders;
   tooling::Replacements Replaces;
+  llvm::Any Metadata;
 };
 
 using AtomicChanges = std::vector<AtomicChange>;
 
 // Defines specs for applying changes.
 struct ApplyChangesSpec {
   // If true, cleans up redundant/erroneous code around changed code with
   // clang-format's cleanup functionality, e.g. redundant commas around deleted

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

 
 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;
+  llvm::Any Metadata;
 };
 
 /// 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>>;
 

 // 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;
 };
 
 /// 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

 /// 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;
+}
+
 /// 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
 // RewriteRule API.  Recast them as such.  Or, just declare these functions
 // public and well-supported and move them out of `detail`.

clang/lib/Tooling/Refactoring/AtomicChange.cpp

AtomicChange::AtomicChange(const SourceManager &SM,
   std::pair<FileID, unsigned> FileIDAndOffset =
       FullKeyPosition.getSpellingLoc().getDecomposedLoc();
   const FileEntry *FE = SM.getFileEntryForID(FileIDAndOffset.first);
   assert(FE && "Cannot create AtomicChange with invalid location.");
   FilePath = std::string(FE->getName());
   Key = FilePath + ":" + std::to_string(FileIDAndOffset.second);
 }
 
+AtomicChange::AtomicChange(const SourceManager &SM, SourceLocation KeyPosition,
+                           llvm::Any M)
+    : AtomicChange(SM, KeyPosition) {
+  Metadata = std::move(M);
+}
+
 AtomicChange::AtomicChange(std::string Key, std::string FilePath,
                            std::string Error,
                            std::vector<std::string> InsertedHeaders,
                            std::vector<std::string> RemovedHeaders,
                            clang::tooling::Replacements Replaces)
     : Key(std::move(Key)), FilePath(std::move(FilePath)),
       Error(std::move(Error)), InsertedHeaders(std::move(InsertedHeaders)),
       RemovedHeaders(std::move(RemovedHeaders)), Replaces(std::move(Replaces)) {

clang/lib/Tooling/Transformer/RewriteRule.cpp

for (const auto &E : ASTEdits) {
     if (!EditRange)
       return SmallVector<Edit, 0>();
     auto Replacement = E.Replacement->eval(Result);
     if (!Replacement)
       return Replacement.takeError();
     transformer::Edit T;
     T.Range = *EditRange;
     T.Replacement = std::move(*Replacement);
+    T.Metadata = E.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/lib/Tooling/Transformer/Transformer.cpp

void Transformer::run(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) {
     auto ID = Result.SourceManager->getFileID(T.Range.getBegin());
     auto Iter = ChangesByFileID
                     .emplace(ID, AtomicChange(*Result.SourceManager,
-                                              T.Range.getBegin()))
+                                              T.Range.getBegin(), T.Metadata))
                     .first;
     auto &AC = Iter->second;
     if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) {
       Consumer(std::move(Err));
       return;
     }
   }
 

clang/unittests/Tooling/RefactoringTest.cpp

TEST_F(AtomicChangeTest, InsertAfterWithInvalidLocation) {
   Err = Change.insert(Context.Sources, SourceLocation(), "b");
   ASSERT_TRUE((bool)Err);
   EXPECT_TRUE(checkReplacementError(
       std::move(Err), replacement_error::wrong_file_path,
       Replacement(Context.Sources, DefaultLoc, 0, "a"),
       Replacement(Context.Sources, SourceLocation(), 0, "b")));
 }
 
+struct SomeMetadata {
+  int Data;
+};
+
+TEST_F(AtomicChangeTest, Metadata) {
+  AtomicChange Change(Context.Sources, DefaultLoc, SomeMetadata{17});
+  const llvm::Any &Metadata = Change.getMetadata();
+  ASSERT_TRUE(llvm::any_isa<SomeMetadata>(Metadata));
+  EXPECT_EQ(llvm::any_cast<SomeMetadata>(Metadata).Data, 17);
+}
+
+TEST_F(AtomicChangeTest, NoMetadata) {
+  AtomicChange Change(Context.Sources, DefaultLoc);
+  EXPECT_FALSE(Change.getMetadata().hasValue());
+}
+
 class ApplyAtomicChangesTest : public ::testing::Test {
 protected:
   ApplyAtomicChangesTest() : FilePath("file.cc") {
     Spec.Cleanup = true;
     Spec.Format = ApplyChangesSpec::kAll;
     Spec.Style = format::getLLVMStyle();
   }
 

clang/unittests/Tooling/TransformerTest.cpp

TEST_F(TransformerTest, RemoveEdit) {
   )cc";
 
   StringRef Decl = "decl";
   testRule(
       makeRule(declStmt().bind(Decl), remove(statement(std::string(Decl)))),
       Input, Expected);
 }
 
+struct SomeMetadata {
+  int Data;
+};
+
+TEST_F(TransformerTest, WithMetadata) {
+  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"))),
+                                      SomeMetadata{17})),
+                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(), 1);
+  const llvm::Any &Metadata = Changes[0].getMetadata();
+  ASSERT_TRUE(llvm::any_isa<SomeMetadata>(Metadata));
+  EXPECT_THAT(llvm::any_cast<SomeMetadata>(Metadata).Data, 17);
+}
+
 TEST_F(TransformerTest, MultiChange) {
   std::string Input = R"cc(
     void foo() {
       if (10 > 1.0)
         log(1) << "oh no!";
       else
         log(0) << "ok";
     }