[LibTooling] Add Transformer, a library for source-to-source transformations.

[ymandel]

Summary

Adds a basic version of Transformer, a library supporting the concise specification of clang-based source-to-source transformations. A full discussion of the end goal can be found on the cfe-dev list with subject "[RFC] Easier source-to-source transformations with clang tooling".

[ilya-biryukov]

Looks very polished, thanks!
Will have to sink the change in a bit, will come back with more comments on Monday.
In the meantime, a few initial comments and suggestions.

clang/include/clang/Tooling/Refactoring/Transformer.h

54 ↗

(On Diff #190688)

Intuitively, it feels that any filtering should be possible at the level of the AST matchers. Is that not the case?
Could you provide some motivating examples where AST matchers cannot be used to nail down the matching nodes and we need MatchFilter?

Please note I have limited experience with AST matchers, so there might be some obvious things that I'm missing.

135 ↗

(On Diff #190688)

Maybe consider separating the fluent API to build the rewrite rule from the rewrite rule itself?

Not opposed to having the fluent builder API, this does look nice and seems to nicely align with the matcher APIs.
However, it is somewhat hard to figure out what can RewriteRule do after it was built when looking at the code right now.

class RewriteRule {
public:
  RewriteRule(DynTypedMatcher, TextGenerator Replacement, TextGenerator Explanation);

  DynTypedMatcher matcher() const;
  Expected<string> replacement() const;
  Expected<string> explanation() const;
};

struct RewriteRuleBuilder { // Having a better name than 'Builder' would be nice.
  RewriteRule finish() &&; // produce the final RewriteRule.

  template <typename T>
  RewriteRuleBuilder &change(const TypedNodeId<T> &Target,
                      NodePart Part = NodePart::Node) &;
  RewriteRuleBuilder &replaceWith(TextGenerator Replacement) &;
  RewriteRuleBuilder &because(TextGenerator Explanation) &;
private:
  RewriteRule RuleInProgress;
};
RewriteRuleBuilder makeRewriteRule();

[ymandel]

clang/include/clang/Tooling/Refactoring/Transformer.h

54 ↗

(On Diff #190688)

Good point. The examples I have would actually be perfectly suited to a matcher. That said, there is not matcher support for a simple predicate of this form, along the lines of gtest's Truly(predicate). I'll remove this and separately try to add something like Truly to the matchers.

135 ↗

(On Diff #190688)

I see your point, but do you think it might be enough to improve the comments on the class? My concern with a builder is the mental burden on the user of another concept (the builder) and the need for an extra .build() everywhere. To a lesser extent, I also don't love the cost of an extra copy, although I doubt it matters and I suppose we could support moves in the build method.

[ilya-biryukov]

clang/include/clang/Tooling/Refactoring/Transformer.h

135 ↗

(On Diff #190688)

The issues with the builder interface is that it requires lots of boilerplate, which is hard to throw away when reading the code of the class. I agree that having a separate builder class is also annoying (more concepts, etc).

Keeping them separate would be my personal preference, but if you'd prefer to keep it in the same class than maybe move the non-builder pieces to the top of the class and separate the builder methods with a comment.
WDYT?

To a lesser extent, I also don't love the cost of an extra copy, although I doubt it matters and I suppose we could support moves in the build method.

I believe we can be as efficient (up to an extra move) with builders as without them. If most usages are of the form RewriteRule R = rewriteRule(...).change(...).replaceWith(...).because(...);
Then we could make all builder functions return r-value reference to a builder and have an implicit conversion operator that would consume the builder, producing the final RewriteRule:

class RewriteRuleBuilder {
  operator RewriteRule () &&;
  /// ...
};
RewriteRuleBuilder rewriteRule();

void addRule(RewriteRule R);
void clientCode() {
  addRule(rewriteRule().change(Matcher).replaceWith("foo").because("bar"));
}

[ymandel]

clang/include/clang/Tooling/Refactoring/Transformer.h

135 ↗

(On Diff #190688)

I didn't realize that implicit conversion functions are allowed. With that, I'm fine w/ splitting. Thanks!

[ilya-biryukov]

I've just realized the review is probably missing the latest revision.
@ymandel, could you upload the new version?

clang/include/clang/Tooling/Refactoring/Transformer.h

54 ↗

(On Diff #190688)

Makes sense! Maybe put a FIXME here to let the readers know this is moving to the ast matchers?

57 ↗

(On Diff #190688)

Note: I'm probably not seeing the latest version, so this comment might be outdated. Feel free to ignore if you've already moved this to the matchers.

BTW, I was wondering whether MatchFilter carries its weight? What are the pros over:

using MatchFilter = std::function<bool(const ast_matchers::MatchFinder::MatchResult &Result)>;

The only thing thats come to mind is that default-constructed function cannot be called, which is less useful than returning true in a default-constructed.
We could handle this in the builder once, the rewrite rule can assert the predicate is always set.

Do you have other reasons to have it in mind that I'm missing?

85 ↗

(On Diff #190688)

What happens if member is composed of multiple tokens? E.g.

foo.bar::baz; // member tokens are ['bar', '::', 'baz']
foo.template baz<int>; // member tokens are ['template', 'baz', '<', 'int', '>']
foo.operator +; // member tokens are ['operator', '+']

I might be misinterpreting the meaning of "member" token.

89 ↗

(On Diff #190688)

Same here, what happens to the template arguments and multi-token names, e.g.
operator + or foo<int, int>?

135 ↗

(On Diff #190688)

Have you uploaded the new version? I don't seem to see the split.

[ilya-biryukov]

clang/unittests/Tooling/TransformerTest.cpp

157 ↗

(On Diff #190688)

NIT: maybe consider adding overloads for these function that allow to pass char literals there? This is probably a common case, so it'd be nice to write this as:

replaceWith("REPLACED").because("Use size() method directly on string.");

[ymandel]

Working on the new version now. Will note with "PTAL" once that's ready. Sorry that wasn't clear in earlier responses.

[ilya-biryukov]

In D59376#1439928, @ymandel wrote:

Working on the new version now. Will note with "PTAL" once that's ready.

Got you! Waiting for the new revision.

Sorry that wasn't clear in earlier responses.

Or maybe it's me misreading earlier responses. Happy to review the new changes as soon as they land.

[ymandel]

Split RewriteRule into two classes, remove support for where clause, support multi-token targets, and add corresponding tests.

[ymandel]

Addressed the most major comments. Still working on some smaller ones. PTAL. Thanks!

clang/include/clang/Tooling/Refactoring/Transformer.h

54 ↗

(On Diff #190688)

I've removed the where clause entirely. I'll separately add the matcher support (I've figured out how to do it within the existing framework).

85 ↗

(On Diff #190688)

Good catch! I've updated to handle these correctly (I believe). Added some tests, plan to add some more.

89 ↗

(On Diff #190688)

Good point. This seems difficult to get right, since NamedDecl does not carry sufficient loc data. However, I've updated the code to explicitly fail in such cases, so at least we won't have bad rewrites.

BTW, any idea whether constructor initializers can ever be multi token?

135 ↗

(On Diff #190688)

I have now. FWIW, I've left both ref overloads, but what do you think of dropping the lvalue-ref overloads and only supporting rvalue refs? Users can still pretty easily use an lvalue, just by inserting a trivial std::move() around the lvalue.

clang/unittests/Tooling/TransformerTest.cpp

157 ↗

(On Diff #190688)

Sure, will do in next revision. The version that's integrated w/ stencils does exactly this, btw. I wonder, though, whether we should add an implicit constructor to the TextGenerator class instead of adding overloads. I have a general distrust of implicit constructors, but I see that's not the case in the clang codebase. WDYT?

[ilya-biryukov]

clang/include/clang/Tooling/Refactoring/Transformer.h

89 ↗

(On Diff #190688)

BTW, any idea whether constructor initializers can ever be multi token?

Two cases come to mind:

1. arbitrary names when initializing base classes, something like ::ns::X<int>(10)
2. template packs with ellipsis (although ellipsis shouldn't be normally part that we replace, I guess): Base(10)...

Full example:

namespace ns {
  struct X {
    X(int);
  };
}


template <class ...Bases>
struct Y : ns::X, Bases... {
  Y() : ns::X(10), Bases(10)... {
  }
};

struct Z {
  Z(int);
};
struct W {
  W(int);
};

Y<Z, W> y;

[ymandel]

• Remove lvalue-ref overloads in builder
• add StringRef overloads for TextGenerator-taking methods
• constrain Name targeting for ctor initializers to explicitly written initializers.
• add multi-token member test

[ymandel]

clang/include/clang/Tooling/Refactoring/Transformer.h

89 ↗

(On Diff #190688)

Turns out the code was already filtering these cases. I added an addition constrain of I->isWritten() for initializers. So, only explicit initialization of fields is allowed, and therefore I'd venture guaranteed to be a single token. I noticed that Kythe seems to make the same assumption.

That said, I could change to code to specify the range as a char-range of getMemberLoc(), getLParenLoc() if we can't rely on that (single-token) guarantee.

[ilya-biryukov]

Mostly nits and small API questions form my side.
This looks very good overall! I'm planning to take a closer look at the handling of macros and various AST nodes in more detail this week, but the approach looks solid from a higher level.

clang/include/clang/Tooling/Refactoring/Transformer.h

38 ↗

(On Diff #192267)

I'm not sure if this is in the LLVM style guide, but we might want to avoid introducing these names into clang::tooling namespaces in the headers.

My fear is that users will rely on those using without knowing that explicitly and won't add corresponding using directives or qualifiers to their .cpp files, making refactoring and moving the code around harder.

Could you fully-qualify those names in the header instead? There does not seem to be too many of them.

65 ↗

(On Diff #192267)

Why would a TextGenerator fail?
I imagine all of the failure cases are programming errors (matchers in the rewrite rule were not aligned with the corresponding text generating function). For those cases, using the assert macro seems cleaner.

116 ↗

(On Diff #192267)

NIT: maybe move all inits to the constructor?
To have all initializers in one place.

170 ↗

(On Diff #192267)

NIT: maybe remove the =default copy and move ctors and assignments?
They should be generated automatically anyway, right?

191 ↗

(On Diff #192267)

NIT: maybe accept a std::string to let the clients pass a std::string if they already have it, avoiding extra copies in some cases?

135 ↗

(On Diff #190688)

Yeah, LG, having only a single overload means less boilerplate and the fluent-builder APIs are mostly used exclusively as r-values anyway.

clang/lib/Tooling/Refactoring/Transformer.cpp

28 ↗

(On Diff #192267)

Other files in the tooling library seem to be adding using namespace clang instead of putting the declaration into a namespace.
Could you please change the new code to do the same for consistency?

30 ↗

(On Diff #192267)

Why put using directives into an anonymous namespace?
I have not seen this pattern before, could you point me to explanations on why this is useful?

44 ↗

(On Diff #192267)

NIT: use UpperCamelCase for local variable names to conform to LLVM style.

110 ↗

(On Diff #192267)

NIT: maybe add the corresponding assertion at the start of the function?

110 ↗

(On Diff #192267)

s/node/Node
s/target_part/TargetPart

111 ↗

(On Diff #192267)

NIT: consider merging verifyTarget into getTarget and making getTarget return Expected<CharSourceRange>.
Would allow avoiding to write one of the complicated switches and error-checking arguably looks just as natural in the getTarget as it is in verifyTarget.

Also, having the invariants closer to the code using them makes it easier to certify both are correct, e.g. seeing that NamedDecl.isIdentifier() was checked before accessing the NamedDecl.getName() in the same function is simpler.

134 ↗

(On Diff #192267)

NIT: start a fixme at a new line to make it more visible.
Or is it clang-format merging it into the previous line?

144 ↗

(On Diff #192267)

This could be operator+ too, right?

struct X {
  X operator+(int);

  void test() {
    X (X::*ptr)(int) = &X::operator+;
  }
};

Would probably want to bail out in this case as well.

160 ↗

(On Diff #192267)

The comment still mentions the where clause. A leftover from the previous version?

199 ↗

(On Diff #192267)

Maybe use std::string in the public interface anyway?

• This function is definitely not a bottleneck, so an extra copy is not a big deal for performance.
• Using std::string would let the clients save a copy in their client code if they can (by using std::move or creating an r-value string in the first place).
• We will have a copy in the body of our function (we have it anyway). We'll eliminate it after switching to C++14 in the body of our function without changing the clients.

228 ↗

(On Diff #192267)

NIT: remove braces around a single statement.

[ymandel]

Simplified TextGenerator, changed namespace handling, collapsed verifyTarget() into getTarget(), and other changes in response to comments.

[ymandel]

Adjusted test to cover case discussed in comments.

[ymandel]

Thanks for the detailed review and really helpful comments!

clang/include/clang/Tooling/Refactoring/Transformer.h

38 ↗

(On Diff #192267)

right. I'd intended to introduce these into the clang tooling namespace -- that is, these weren't just convenience aliases for the header file. But, I no longer think that's useful in any case, so dropping them is certainly best.

65 ↗

(On Diff #192267)

Sure. I could go either way. I think some of these cases fall on the border between an invariant violation and "invalid argument" or some such. But, let's keep it simpler for now.

116 ↗

(On Diff #192267)

nicer, thanks.

170 ↗

(On Diff #192267)

Sure. I was going based on google's style recommendations, but personally i prefer leaving them implicit.

clang/lib/Tooling/Refactoring/Transformer.cpp

28 ↗

(On Diff #192267)

Done. Agreed about being consistent. FWIW, I can't say I like this style. Perhaps because I'm not used to it, but it feels too implicit. It forces the reader to figure out where each definition is being associated. Also, I discovered it only works for method definitions. Free functions still need to be explicitly namespaced.

Any idea what the reason for this style is?

30 ↗

(On Diff #192267)

You gain an extra little bit of robustness against clashing with something declared in the enclosing namespace. But, this is overkill even for me -- I generally only do this when I already have an anonymous namespace; there's no good reason to create one for this purpose.

111 ↗

(On Diff #192267)

Yes, I like it much better this way. The split wasn't worth it.

134 ↗

(On Diff #192267)

Done.
It's emacs (c-fill-paragraph), clang-format is better about this, because it's very conservative about touching comments.

144 ↗

(On Diff #192267)

The check E->getNameInfo().getName().isIdentifier() rules that out. It was hidden in verifyTarget(). I hope the new code makes this clearer?
Also, I updated the test TransformerTest.NodePartNameDeclRefFailure to cover this case.

160 ↗

(On Diff #192267)

right. deleted the comment altogether.

199 ↗

(On Diff #192267)

Agreed. Updated all relevant places.

[ilya-biryukov]

The only real question I have is about returning an error vs an empty transformation in of macros.
The rest are just NITs.

Thanks for the change! I'll get to the NodeId patch right away :-)

clang/include/clang/Tooling/Refactoring/Transformer.h

70 ↗

(On Diff #192623)

NIT: maybe mention what it should be used for? we plan to show to to the user (e.g. in the clang-tidy fix description), right?

89 ↗

(On Diff #192623)

Could you also add examples on how to apply the rewrite rule here?
So that the users can have an idea about the full workflow when reading the code.

97 ↗

(On Diff #192623)

NIT: maybe assert this invariant in the constructor?

122 ↗

(On Diff #192623)

This method does not seem to be used anywhere.
Are we missing the usages in this patch? Maybe remove it from the initial implementation and re-add later when we have the callsites?

175 ↗

(On Diff #192623)

NIT: the top-level std::move looks redundant, maybe remove it?

170 ↗

(On Diff #192267)

+1. Means less boilterplate.

clang/lib/Tooling/Refactoring/Transformer.cpp

31 ↗

(On Diff #192623)

NIT: we could leave out the trailing :: for clang and llvm, they are well-known and unambiguous namespace names.

42 ↗

(On Diff #192623)

NIT: could you add a brief comment on what this function is expected to return?

133 ↗

(On Diff #192623)

NIT: remove namespaces for consistency with the rest of the code.

(Probably a leftover from the previous version)

150 ↗

(On Diff #192623)

NIT: consider simply propagating the original error up the stack in that case to avoid boilerplate.
Although adding the .target() information to the error might be useful, so up to you.

157 ↗

(On Diff #192623)

Why not return an error in case of macros too? Is there any use of the empty transformation to the clients?
Alternatively, we might want to document this behavior (applyRewriteRule can return empty transformations) and it's rationale.

28 ↗

(On Diff #192267)

I personally have a slight preference towards using namespace style myself because it produces compiler errors whenever declaration and definition signatures don't match, catching errors early in cases where one forgets to update either of those, e.g.

// foo.h
namespace clang{
std::unique_ptr<Something> createSomething();
}

// foo1.cpp
using namespace clang;

std::unique_ptr<Something> clang::createSomething(int Param) { // <-- error
}


// foo2.cpp
namespace clang {
std::unique_ptr<Something> clang::createSomething(int Param) { // <-- all ok, will get linker errors on usages instead.
}
}

There might be other reasons and I'm not sure why exactly the code in clang uses this approach.

144 ↗

(On Diff #192267)

Right, thanks! Moving the check into the code makes it more explicit now, thanks!

[ilya-biryukov]

clang/unittests/Tooling/TransformerTest.cpp

61 ↗

(On Diff #192623)

Maybe put the whole file into an anonymous namespace?
To avoid potential name clashes with other test files (and ODR violations that might follow)

88 ↗

(On Diff #192623)

NIT: remove redundant braces

159 ↗

(On Diff #192623)

These three lines are in each test. Consider moving them to a helper function (e.g. returning the final result as text?) to reduce the boilerplate in the test.

[ymandel]

• Assorted changes in response to comments.

[ymandel]

Thanks for (more) helpful comments. I think the code is a lot better now than it started out. :)

Also, converted RewriteRule to a simple struct as per our discussion.

clang/include/clang/Tooling/Refactoring/Transformer.h

70 ↗

(On Diff #192623)

Done. But, given that we don't use this field yet, I'm fine deleting it until a future revision. I'm also fine leaving it given that we know we'll be needing it later.

89 ↗

(On Diff #192623)

Is this what you had in mind? Unlike clang-tidy, there is no neat infrastructure into which we can drop it.

97 ↗

(On Diff #192623)

The constructor sets this field, so no need to assert.

122 ↗

(On Diff #192623)

It was used above in makeMatcher. But, it was overkill -- users can just reference RootId directly, so I've removed it.

175 ↗

(On Diff #192623)

Yes, not sure why did that. thanks.

clang/lib/Tooling/Refactoring/Transformer.cpp

133 ↗

(On Diff #192623)

This is necessary as far as I can tell. W/o it, I get a linker failure (missing definition). Based on the declaration in the header, the compiler resolves the reference below to clang::tooling::applyRewriteRule() but this definition ends up in the global namespace.

I think the using decls only work for method definitions -- the type seems to resolve to be the one in the namespace. But free functions don't get the same treatment. Unless I"m doing something wrong?

150 ↗

(On Diff #192623)

integrated id into getTarget so we can avoid this error handling step.

157 ↗

(On Diff #192623)

I think the common case is that we simply want to skip macros -- there's nothing erroneous about them, they're just usually hard to deal w/ correctly. That said, we might want to change this to *never* return an error and just assert when things go wrong. The advantage of the current design is that callers can in principle ignore failed rewrites.

However, in practice, if the rewrite fails that means it had a bug, so it might be best to asssert(). Only real advantage, then, is that this is easier to test since it doesn't crash the program.

WDYT?

clang/unittests/Tooling/TransformerTest.cpp

61 ↗

(On Diff #192623)

also removed static qualifiers from functions since they are now redundant.

159 ↗

(On Diff #192623)

Thanks, much cleaner this way.

[ilya-biryukov]

Looks neat, thanks!
A final set of NITs from my side, I don't think any of the comments have to do with the actual design of the library, so expect LGTM in the next round.

clang/include/clang/Tooling/Refactoring/Transformer.h

82 ↗

(On Diff #192889)

NIT: Maybe be more specific: string-based binding ids?

125 ↗

(On Diff #192889)

NIT: bound to this id

136 ↗

(On Diff #192889)

NIT: Maybe remove the constructor and let the builder handle this?
Technically, users can break the invariants after creating the rewrite rules anyway, so having this constructor does not buy much in terms of safer coding practices, but makes it harder to use RewriteRule as a plain struct (specifically, having no default constructor might make it awkward).

Up to you, of course.

158 ↗

(On Diff #192889)

NIT: return by value to make the signatures less clunky. RVO should insure the generated code is the same in practice.
Bonus point: that'd mean we don't need && qualifier on the builder functions too, only at consume().

184 ↗

(On Diff #192889)

Isn't this overload redundant in presence of buildRule(DynTypedMatcher)? Both seem to call into the constructor that accept a DynTypedMatcher, so Matcher<T> is convertible to DynTypedMatcher, right?.

70 ↗

(On Diff #192623)

Up to you, to me it feels like the presence of this field defines what this class is used for.

1. If there's an explanation, it feels like it should represent a complete fix or refactoring that could be presented to the user.
2. Without an explanation, it might feel like something lower-level (e.g. one could write a bunch of RewriteRule whose changes are later combined and surfaced to the user as a full refactoring).

Both approaches make sense, and I assume (1) is the desired abstraction this class represents, so keeping the field looks ok.

89 ↗

(On Diff #192623)

Yeah, exactly, but could we keep is a bit shorter by removing the pieces which are non-relevant to the actual transformer usage.
Something like:

// To apply a rule to the clang AST, use Transformer class:
// \code
//   AtomicChanges Changes;
//   Transformer T(
//       MyRule, [&Changes](const AtomicChange &C) { Changes.push_back(C); };);
//   ast_matchers::MatchFinder MatchFinder;
//   T.registerMatchers(&MatchFinder);
//   // ... insert code to run the ast matchers.
//   // Consume the changes.
//   applyAtomicChanges(..., Changes);

Or just mention that Transformer class should be used to apply the rewrite rule and obtain the corresponding replacements.

clang/lib/Tooling/Refactoring/Transformer.cpp

133 ↗

(On Diff #192623)

Yeah, you should explicitly qualify to let the compiler know the namespace of a corresponding declaration:

Expected<Transformation>
clang::tooling::applyRewriteRule(...) {
  // ...
}

tooling::applyRewriteRule would also work since we have using namespace clang::tooling; at the top of the file.

157 ↗

(On Diff #192623)

The current approach LG, and we have the comments about those cases.

I agree with you that handling macros is hard, so skipping them makes sense for most rewrite rules one would write and at the same time, it'd be useful to let the users of the code know that a rewrite rule could not be applied because of macros.
Returning an empty transformation in that case seems like the best option, the alternative that I can think is less nice: we could have a special error for macros and make users llvm::handleErrors to figure out those cases. This is clunky and the resulting code is not very nice.

clang/unittests/Tooling/TransformerTest.cpp

61 ↗

(On Diff #192623)

LG.
As a note, my colleagues mentioned that the LLVM Style Guide says one should put 'static' even in that case, because it makes it easier for readers of the code to spot the file-private functions (they don't need to look for an enclosing anonymous namespace).

I'm personally happy either with or without static.

[ymandel]

Assorted changes in response to comments.

Most notably, dropped constructor from RewriteRule, in favor of putting meaningful initialization in the builder.

[ymandel]

clang/include/clang/Tooling/Refactoring/Transformer.h

136 ↗

(On Diff #192889)

Agreed, but DynTypedMatcher has no default constructor, so we have to provide something. I dropped the constructor, but added an initializer to Matcher to enable the default constructor.

184 ↗

(On Diff #192889)

I think I was avoiding some extra construction, but even if so, I can't see its worth the added complexity. thx

70 ↗

(On Diff #192623)

Agreed. Keeping it.

89 ↗

(On Diff #192623)

went w/ the second suggestion.

clang/lib/Tooling/Refactoring/Transformer.cpp

133 ↗

(On Diff #192623)

ah...

157 ↗

(On Diff #192623)

Agreed. If error handling was nicer, then I think that your suggestion would be ideal.

clang/unittests/Tooling/TransformerTest.cpp

61 ↗

(On Diff #192623)

static sounds good to me.

[ilya-biryukov]

Looks great, thanks!

clang/include/clang/Tooling/Refactoring/Transformer.h

110 ↗

(On Diff #193219)

NIT: llvm::StringLiteral is a vocabulary type with compile-time size that could be used here.
Although I don't think it has any actual benefits over char array, so leaving as is also looks good.

113 ↗

(On Diff #193219)

NIT: comma seems redundant, this should probably be: A fluent builder class

[ymandel]

Small tweaks in response to comments.

[ymandel]

Thank you for the extremely helpful and detailed review!!

clang/include/clang/Tooling/Refactoring/Transformer.h

110 ↗

(On Diff #193219)

Went with StringLiteral -- given that we always wrap RootId in a stringref to use it, seems better just to define it that way to begin with.

[ymandel]

Remove dependency on NodeIds.

[ymandel]

I've removed the dependency on NodeIds given our (off thread) discussions regarding their overall value and appropriateness in clang::tooling vs clang::ast_matchers. PTAL.

[ilya-biryukov]

LGTM with the new changes. Specifying the clang::Expr type explicitly when calling change looks a bit less clear than the original approach, but also looks pretty clear to me.

[ABataev]

Patch breaks the build with the shared libraries, for example, http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage/builds/9498. Seems to me, you need to add an extra dependency on clangTooling. clangToolingCore is not enough.

[ABataev]

You need to provide -DBUILD_SHARED_LIBS=true to cmake. Unfortunately, there is a dependency from the libToolingRefactor to libTooling. I don't think it will be easy to fix this.

[ymandel]

Reopening to fix the breakage caused by previous attempt at commit.

[ymandel]

Sever dependency of clangToolingRefactor on clangTooling via FixIt.

Transformer used FixIt, which causes a problematic dependency. This diff copies
the (minimal) code from FixIt to Transformer.cpp to break the dependency. For
the future, though, we should consider whether the FixIt library should live
somewhere accessible to components in Refactoring (perhaps Core?).

[ymandel]

Ilya, this diff fixes the breakage. PTAL.

[ilya-biryukov]

Given that we don't any usages of getExtendedRange, could you remove it from the Fixit.h and move to a header somewhere in the clangToolingRefactoring (e.g. into Transformer.h itself)?

Having two copies of this functions in the codebase does not look good, especially given that one of them is never used anyway.

[ilya-biryukov]

Keeping it only in the cpp file also LG if we don't plan to have other usages for now, but please remove the version from FixIt.h

[ymandel]

In D59376#1454617, @ilya-biryukov wrote:

Keeping it only in the cpp file also LG if we don't plan to have other usages for now, but please remove the version from FixIt.h

The Stencil library will need this as well and I've heard from another user who's already using this (a standalone tool, not in clang), so it seems to have a general value. Might the fixit library belong in Core? I have a whole bunch more utility functions coming along, so we need a place for them as well.

I propose that I create a new library in Core with getExtendedRange() and remove it from FixIt. The other utility functions that I need will also go there. We can separately investigate moving the remaining pieces of FixIt into Core.

[ilya-biryukov]

In D59376#1454748, @ymandel wrote:

I propose that I create a new library in Core with getExtendedRange() and remove it from FixIt. The other utility functions that I need will also go there. We can separately investigate moving the remaining pieces of FixIt into Core.

Moving this to Core makes sense to me, the only concern that I have is that it's actually a small and focused library as it stands today and adding utility functions to it would seem to cause some chaos.
But it's fine as long as the maintainers of the library are ok with this. I'll ask around to find out who's actually responsible for the library and get back to you...

[ilya-biryukov]

Per @ioeric's suggestion: why not move the helper into Tooling/Refactoring/ExtendedRange.h?
If it's in ToolingRefactoring, both stencil and transformer can access it.

For external users, a dependency on either ToolingCore or ToolingRefactoring should be fine, since they're fine with a dependency on Tooling already.

[ymandel]

In D59376#1454768, @ilya-biryukov wrote:

Per @ioeric's suggestion: why not move the helper into Tooling/Refactoring/ExtendedRange.h?
If it's in ToolingRefactoring, both stencil and transformer can access it.

For external users, a dependency on either ToolingCore or ToolingRefactoring should be fine, since they're fine with a dependency on Tooling already.

This sounds perfect. Can I do the same for the future additions -- small, focused libraries for each group of functions? I just want to be sure that we don't regret the name "ExtendedRange" when I need to add the next batch.

[ioeric]

In D59376#1454834, @ymandel wrote:

In D59376#1454768, @ilya-biryukov wrote:

Per @ioeric's suggestion: why not move the helper into Tooling/Refactoring/ExtendedRange.h?
If it's in ToolingRefactoring, both stencil and transformer can access it.

For external users, a dependency on either ToolingCore or ToolingRefactoring should be fine, since they're fine with a dependency on Tooling already.

This sounds perfect. Can I do the same for the future additions -- small, focused libraries for each group of functions? I just want to be sure that we don't regret the name "ExtendedRange" when I need to add the next batch.

In clangd, we have a library called SourceCode.h that keeps source code related helpers including those that deal with ranges. I think it's reasonable to use SourceCode.h or something similar here.

[ymandel]

Switch from using FixIt to SourceCode, as solution to circular dependency problem.

[ymandel]

Rebasing to parent D60269 so that diffs show correctly.

[ilya-biryukov]

One more LGTM.

[ymandel]

Rebase onto master

cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h

+//===--- Transformer.h - Clang source-rewriting library ---------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+///
+///  \file
+///  Defines a library supporting the concise specification of clang-based
+///  source-to-source transformations.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLING_REFACTOR_TRANSFORMER_H_
+#define LLVM_CLANG_TOOLING_REFACTOR_TRANSFORMER_H_
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersInternal.h"
+#include "clang/Tooling/Refactoring/AtomicChange.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/Error.h"
+#include <deque>
+#include <functional>
+#include <string>
+#include <type_traits>
+#include <utility>
+#include <vector>
+
+namespace clang {
+namespace tooling {
+/// Determines the part of the AST node to replace.  We support this to work
+/// around the fact that the AST does not differentiate various syntactic
+/// elements into their own nodes, so users can specify them relative to a node,
+/// instead.
+enum class NodePart {
+  /// The node itself.
+  Node,
+  /// Given a \c MemberExpr, selects the member's token.
+  Member,
+  /// Given a \c NamedDecl or \c CxxCtorInitializer, selects that token of the
+  /// relevant name, not including qualifiers.
+  Name,
+};
+
+using TextGenerator =
+    std::function<std::string(const ast_matchers::MatchFinder::MatchResult &)>;
+
+/// Description of a source-code transformation.
+//
+// A *rewrite rule* describes a transformation of source code. It has the
+// following components:
+//
+// * Matcher: the pattern term, expressed as clang matchers (with Transformer
+//   extensions).
+//
+// * Target: the source code impacted by the rule. This identifies an AST node,
+//   or part thereof (\c TargetPart), 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 (\c TargetKind), which guides the determination of the node extent
+//   and might, in the future, statically constrain the set of eligible
+//   NodeParts for a given node.
+//
+// * Replacement: a function that produces a replacement string for the target,
+//   based on the match result.
+//
+// * Explanation: explanation of the rewrite.  This will be displayed to the
+//   user, where possible (for example, in clang-tidy fix descriptions).
+//
+// Rules have an additional, implicit, component: the parameters. These are
+// portions of the pattern which are left unspecified, yet named so that we can
+// reference them in the replacement term.  The structure of parameters can be
+// partially or even fully specified, in which case they serve just to identify
+// matched nodes for later reference rather than abstract over portions of the
+// AST.  However, in all cases, we refer to named portions of the pattern as
+// parameters.
+//
+// RewriteRule is constructed in a "fluent" style, by creating a builder and
+// chaining setters of individual components.
+// \code
+//   RewriteRule MyRule = buildRule(functionDecl(...)).replaceWith(...);
+// \endcode
+//
+// The \c Transformer class should then be used to apply the rewrite rule and
+// obtain the corresponding replacements.
+struct RewriteRule {
+  // `Matcher` describes the context of this rule. It should always be bound to
+  // at least `RootId`.  The builder class below takes care of this
+  // binding. Here, we bind it to a trivial Matcher to enable the default
+  // constructor, since DynTypedMatcher has no default constructor.
+  ast_matchers::internal::DynTypedMatcher Matcher = ast_matchers::stmt();
+  // The (bound) id of the node whose source will be replaced.  This id should
+  // never be the empty string.
+  std::string Target;
+  ast_type_traits::ASTNodeKind TargetKind;
+  NodePart TargetPart;
+  TextGenerator Replacement;
+  TextGenerator Explanation;
+
+  // Id used as the default target of each match. The node described by the
+  // matcher is guaranteed to be bound to this id, for all rewrite rules
+  // constructed with the builder class.
+  static constexpr llvm::StringLiteral RootId = "___root___";
+};
+
+/// A fluent builder class for \c RewriteRule.  See comments on \c RewriteRule.
+class RewriteRuleBuilder {
+  RewriteRule Rule;
+
+public:
+  RewriteRuleBuilder(ast_matchers::internal::DynTypedMatcher M) {
+    M.setAllowBind(true);
+    // `tryBind` is guaranteed to succeed, because `AllowBind` was set to true.
+    Rule.Matcher = *M.tryBind(RewriteRule::RootId);
+    Rule.Target = RewriteRule::RootId;
+    Rule.TargetKind = M.getSupportedKind();
+    Rule.TargetPart = NodePart::Node;
+  }
+
+  /// (Implicit) "build" operator to build a RewriteRule from this builder.
+  operator RewriteRule() && { return std::move(Rule); }
+
+  // Sets the target kind based on a clang AST node type.
+  template <typename T> RewriteRuleBuilder as();
+
+  template <typename T>
+  RewriteRuleBuilder change(llvm::StringRef Target,
+                            NodePart Part = NodePart::Node);
+
+  RewriteRuleBuilder replaceWith(TextGenerator Replacement);
+  RewriteRuleBuilder replaceWith(std::string Replacement) {
+    return replaceWith(text(std::move(Replacement)));
+  }
+
+  RewriteRuleBuilder because(TextGenerator Explanation);
+  RewriteRuleBuilder because(std::string Explanation) {
+    return because(text(std::move(Explanation)));
+  }
+
+private:
+  // Wraps a string as a TextGenerator.
+  static TextGenerator text(std::string M) {
+    return [M](const ast_matchers::MatchFinder::MatchResult &) { return M; };
+   }
+};
+
+/// Convenience factory functions for starting construction of a \c RewriteRule.
+inline RewriteRuleBuilder buildRule(ast_matchers::internal::DynTypedMatcher M) {
+  return RewriteRuleBuilder(std::move(M));
+}
+
+template <typename T> RewriteRuleBuilder RewriteRuleBuilder::as() {
+  Rule.TargetKind = ast_type_traits::ASTNodeKind::getFromNodeKind<T>();
+  return *this;
+}
+
+template <typename T>
+RewriteRuleBuilder RewriteRuleBuilder::change(llvm::StringRef TargetId,
+                                              NodePart Part) {
+  Rule.Target = TargetId;
+  Rule.TargetKind = ast_type_traits::ASTNodeKind::getFromNodeKind<T>();
+  Rule.TargetPart = Part;
+  return *this;
+}
+
+/// 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 apply a rule to a match.  Returns an empty transformation if the
+/// match is not eligible for rewriting (certain interactions with macros, for
+/// example).  Fails if any invariants are violated relating to bound nodes in
+/// the match.
+Expected<Transformation>
+applyRewriteRule(const RewriteRule &Rule,
+                 const ast_matchers::MatchFinder::MatchResult &Match);
+
+/// Handles the matcher and callback registration for a single rewrite rule, as
+/// defined by the arguments of the constructor.
+class Transformer : public ast_matchers::MatchFinder::MatchCallback {
+public:
+  using ChangeConsumer =
+      std::function<void(const clang::tooling::AtomicChange &Change)>;
+
+  /// \param Consumer Receives each successful rewrites as an \c AtomicChange.
+  Transformer(RewriteRule Rule, ChangeConsumer Consumer)
+      : Rule(std::move(Rule)), Consumer(std::move(Consumer)) {}
+
+  /// N.B. Passes `this` pointer to `MatchFinder`.  So, this object should not
+  /// be moved after this call.
+  void registerMatchers(ast_matchers::MatchFinder *MatchFinder);
+
+  /// Not called directly by users -- called by the framework, via base class
+  /// pointer.
+  void run(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  RewriteRule Rule;
+  /// Receives each successful rewrites as an \c AtomicChange.
+  ChangeConsumer Consumer;
+};
+} // namespace tooling
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLING_REFACTOR_TRANSFORMER_H_

cfe/trunk/lib/Tooling/Refactoring/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS Support)
 
 add_clang_library(clangToolingRefactor
   ASTSelection.cpp
   ASTSelectionRequirements.cpp
   AtomicChange.cpp
   Extract/Extract.cpp
   Extract/SourceExtraction.cpp
   RefactoringActions.cpp
   Rename/RenamingAction.cpp
   Rename/SymbolOccurrences.cpp
   Rename/USRFinder.cpp
   Rename/USRFindingAction.cpp
   Rename/USRLocFinder.cpp
   SourceCode.cpp
+  Transformer.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangFormat
   clangIndex
   clangLex
   clangRewrite
   clangToolingCore
   )

cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp

+//===--- Transformer.cpp - Transformer library implementation ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include "clang/AST/Expr.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/Tooling/Refactoring/AtomicChange.h"
+#include "clang/Tooling/Refactoring/SourceCode.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+#include <deque>
+#include <string>
+#include <utility>
+#include <vector>
+
+using namespace clang;
+using namespace tooling;
+
+using ast_matchers::MatchFinder;
+using ast_type_traits::ASTNodeKind;
+using ast_type_traits::DynTypedNode;
+using llvm::Error;
+using llvm::Expected;
+using llvm::Optional;
+using llvm::StringError;
+using llvm::StringRef;
+using llvm::Twine;
+
+using MatchResult = MatchFinder::MatchResult;
+
+// Did the text at this location originate in a macro definition (aka. body)?
+// For example,
+//
+//   #define NESTED(x) x
+//   #define MACRO(y) { int y  = NESTED(3); }
+//   if (true) MACRO(foo)
+//
+// The if statement expands to
+//
+//   if (true) { int foo = 3; }
+//                   ^     ^
+//                   Loc1  Loc2
+//
+// For SourceManager SM, SM.isMacroArgExpansion(Loc1) and
+// SM.isMacroArgExpansion(Loc2) are both true, but isOriginMacroBody(sm, Loc1)
+// is false, because "foo" originated in the source file (as an argument to a
+// macro), whereas isOriginMacroBody(SM, Loc2) is true, because "3" originated
+// in the definition of MACRO.
+static bool isOriginMacroBody(const clang::SourceManager &SM,
+                              clang::SourceLocation Loc) {
+  while (Loc.isMacroID()) {
+    if (SM.isMacroBodyExpansion(Loc))
+      return true;
+    // Otherwise, it must be in an argument, so we continue searching up the
+    // invocation stack. getImmediateMacroCallerLoc() gives the location of the
+    // argument text, inside the call text.
+    Loc = SM.getImmediateMacroCallerLoc(Loc);
+  }
+  return false;
+}
+
+static llvm::Error invalidArgumentError(Twine Message) {
+  return llvm::make_error<StringError>(llvm::errc::invalid_argument, Message);
+}
+
+static llvm::Error typeError(StringRef Id, const ASTNodeKind &Kind,
+                             Twine Message) {
+  return invalidArgumentError(
+      Message + " (node id=" + Id + " kind=" + Kind.asStringRef() + ")");
+}
+
+static llvm::Error missingPropertyError(StringRef Id, Twine Description,
+                                        StringRef Property) {
+  return invalidArgumentError(Description + " requires property '" + Property +
+                              "' (node id=" + Id + ")");
+}
+
+static Expected<CharSourceRange>
+getTargetRange(StringRef Target, const DynTypedNode &Node, ASTNodeKind Kind,
+               NodePart TargetPart, ASTContext &Context) {
+  switch (TargetPart) {
+  case NodePart::Node: {
+    // For non-expression statements, associate any trailing semicolon with the
+    // statement text.  However, if the target was intended as an expression (as
+    // indicated by its kind) then we do not associate any trailing semicolon
+    // with it.  We only associate the exact expression text.
+    if (Node.get<Stmt>() != nullptr) {
+      auto ExprKind = ASTNodeKind::getFromNodeKind<clang::Expr>();
+      if (!ExprKind.isBaseOf(Kind))
+        return getExtendedRange(Node, tok::TokenKind::semi, Context);
+    }
+    return CharSourceRange::getTokenRange(Node.getSourceRange());
+  }
+  case NodePart::Member:
+    if (auto *M = Node.get<clang::MemberExpr>())
+      return CharSourceRange::getTokenRange(
+          M->getMemberNameInfo().getSourceRange());
+    return typeError(Target, Node.getNodeKind(),
+                     "NodePart::Member applied to non-MemberExpr");
+  case NodePart::Name:
+    if (const auto *D = Node.get<clang::NamedDecl>()) {
+      if (!D->getDeclName().isIdentifier())
+        return missingPropertyError(Target, "NodePart::Name", "identifier");
+      SourceLocation L = D->getLocation();
+      auto R = CharSourceRange::getTokenRange(L, L);
+      // Verify that the range covers exactly the name.
+      // FIXME: extend this code to support cases like `operator +` or
+      // `foo<int>` for which this range will be too short.  Doing so will
+      // require subcasing `NamedDecl`, because it doesn't provide virtual
+      // access to the \c DeclarationNameInfo.
+      if (getText(R, Context) != D->getName())
+        return CharSourceRange();
+      return R;
+    }
+    if (const auto *E = Node.get<clang::DeclRefExpr>()) {
+      if (!E->getNameInfo().getName().isIdentifier())
+        return missingPropertyError(Target, "NodePart::Name", "identifier");
+      SourceLocation L = E->getLocation();
+      return CharSourceRange::getTokenRange(L, L);
+    }
+    if (const auto *I = Node.get<clang::CXXCtorInitializer>()) {
+      if (!I->isMemberInitializer() && I->isWritten())
+        return missingPropertyError(Target, "NodePart::Name",
+                                    "explicit member initializer");
+      SourceLocation L = I->getMemberLocation();
+      return CharSourceRange::getTokenRange(L, L);
+    }
+    return typeError(
+        Target, Node.getNodeKind(),
+        "NodePart::Name applied to neither DeclRefExpr, NamedDecl nor "
+        "CXXCtorInitializer");
+  }
+  llvm_unreachable("Unexpected case in NodePart type.");
+}
+
+Expected<Transformation>
+tooling::applyRewriteRule(const RewriteRule &Rule,
+                          const ast_matchers::MatchFinder::MatchResult &Match) {
+  if (Match.Context->getDiagnostics().hasErrorOccurred())
+    return Transformation();
+
+  auto &NodesMap = Match.Nodes.getMap();
+  auto It = NodesMap.find(Rule.Target);
+  assert (It != NodesMap.end() && "Rule.Target must be bound in the match.");
+
+  Expected<CharSourceRange> TargetOrErr =
+      getTargetRange(Rule.Target, It->second, Rule.TargetKind, Rule.TargetPart,
+                     *Match.Context);
+  if (auto Err = TargetOrErr.takeError())
+    return std::move(Err);
+  auto &Target = *TargetOrErr;
+  if (Target.isInvalid() ||
+      isOriginMacroBody(*Match.SourceManager, Target.getBegin()))
+    return Transformation();
+
+  return Transformation{Target, Rule.Replacement(Match)};
+}
+
+constexpr llvm::StringLiteral RewriteRule::RootId;
+
+RewriteRuleBuilder RewriteRuleBuilder::replaceWith(TextGenerator T) {
+  Rule.Replacement = std::move(T);
+  return *this;
+}
+
+RewriteRuleBuilder RewriteRuleBuilder::because(TextGenerator T) {
+  Rule.Explanation = std::move(T);
+  return *this;
+}
+
+void Transformer::registerMatchers(MatchFinder *MatchFinder) {
+  MatchFinder->addDynamicMatcher(Rule.Matcher, this);
+}
+
+void Transformer::run(const MatchResult &Result) {
+  auto ChangeOrErr = applyRewriteRule(Rule, Result);
+  if (auto Err = ChangeOrErr.takeError()) {
+    llvm::errs() << "Rewrite failed: " << llvm::toString(std::move(Err))
+                 << "\n";
+    return;
+  }
+  auto &Change = *ChangeOrErr;
+  auto &Range = Change.Range;
+  if (Range.isInvalid()) {
+    // No rewrite applied (but no error encountered either).
+    return;
+  }
+  AtomicChange AC(*Result.SourceManager, Range.getBegin());
+  if (auto Err = AC.replace(*Result.SourceManager, Range, Change.Replacement))
+    AC.setError(llvm::toString(std::move(Err)));
+  Consumer(AC);
+}

cfe/trunk/unittests/Tooling/CMakeLists.txt

add_clang_unittest(ToolingTests
   RecursiveASTVisitorTestTypeLocVisitor.cpp
   RefactoringActionRulesTest.cpp
   RefactoringCallbacksTest.cpp
   RefactoringTest.cpp
   ReplacementsYamlTest.cpp
   RewriterTest.cpp
   SourceCodeTest.cpp
   ToolingTest.cpp
+  TransformerTest.cpp
   )
 
 target_link_libraries(ToolingTests
   PRIVATE
   clangAST
   clangASTMatchers
   clangBasic
   clangFormat

cfe/trunk/unittests/Tooling/TransformerTest.cpp

+//===- unittest/Tooling/TransformerTest.cpp -------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Tooling/Refactoring/Transformer.h"
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+using ast_matchers::anyOf;
+using ast_matchers::argumentCountIs;
+using ast_matchers::callee;
+using ast_matchers::callExpr;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::cxxMethodDecl;
+using ast_matchers::cxxRecordDecl;
+using ast_matchers::declRefExpr;
+using ast_matchers::expr;
+using ast_matchers::functionDecl;
+using ast_matchers::hasAnyName;
+using ast_matchers::hasArgument;
+using ast_matchers::hasDeclaration;
+using ast_matchers::hasElse;
+using ast_matchers::hasName;
+using ast_matchers::hasType;
+using ast_matchers::ifStmt;
+using ast_matchers::member;
+using ast_matchers::memberExpr;
+using ast_matchers::namedDecl;
+using ast_matchers::on;
+using ast_matchers::pointsTo;
+using ast_matchers::to;
+using ast_matchers::unless;
+
+using llvm::StringRef;
+
+constexpr char KHeaderContents[] = R"cc(
+  struct string {
+    string(const char*);
+    char* c_str();
+    int size();
+  };
+  int strlen(const char*);
+
+  namespace proto {
+  struct PCFProto {
+    int foo();
+  };
+  struct ProtoCommandLineFlag : PCFProto {
+    PCFProto& GetProto();
+  };
+  }  // namespace proto
+)cc";
+
+static ast_matchers::internal::Matcher<clang::QualType>
+isOrPointsTo(const clang::ast_matchers::DeclarationMatcher &TypeMatcher) {
+  return anyOf(hasDeclaration(TypeMatcher), pointsTo(TypeMatcher));
+}
+
+static std::string format(StringRef Code) {
+  const std::vector<Range> Ranges(1, Range(0, Code.size()));
+  auto Style = format::getLLVMStyle();
+  const auto Replacements = format::reformat(Style, Code, Ranges);
+  auto Formatted = applyAllReplacements(Code, Replacements);
+  if (!Formatted) {
+    ADD_FAILURE() << "Could not format code: "
+                  << llvm::toString(Formatted.takeError());
+    return std::string();
+  }
+  return *Formatted;
+}
+
+static void compareSnippets(StringRef Expected,
+                     const llvm::Optional<std::string> &MaybeActual) {
+  ASSERT_TRUE(MaybeActual) << "Rewrite failed. Expecting: " << Expected;
+  auto Actual = *MaybeActual;
+  std::string HL = "#include \"header.h\"\n";
+  auto I = Actual.find(HL);
+  if (I != std::string::npos)
+    Actual.erase(I, HL.size());
+  EXPECT_EQ(format(Expected), format(Actual));
+}
+
+// FIXME: consider separating this class into its own file(s).
+class ClangRefactoringTestBase : public testing::Test {
+protected:
+  void appendToHeader(StringRef S) { FileContents[0].second += S; }
+
+  void addFile(StringRef Filename, StringRef Content) {
+    FileContents.emplace_back(Filename, Content);
+  }
+
+  llvm::Optional<std::string> rewrite(StringRef Input) {
+    std::string Code = ("#include \"header.h\"\n" + Input).str();
+    auto Factory = newFrontendActionFactory(&MatchFinder);
+    if (!runToolOnCodeWithArgs(
+            Factory->create(), Code, std::vector<std::string>(), "input.cc",
+            "clang-tool", std::make_shared<PCHContainerOperations>(),
+            FileContents)) {
+      return None;
+    }
+    auto ChangedCodeOrErr =
+        applyAtomicChanges("input.cc", Code, Changes, ApplyChangesSpec());
+    if (auto Err = ChangedCodeOrErr.takeError()) {
+      llvm::errs() << "Change failed: " << llvm::toString(std::move(Err))
+                   << "\n";
+      return None;
+    }
+    return *ChangedCodeOrErr;
+  }
+
+  void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) {
+    Transformer T(std::move(Rule),
+                  [this](const AtomicChange &C) { Changes.push_back(C); });
+    T.registerMatchers(&MatchFinder);
+    compareSnippets(Expected, rewrite(Input));
+  }
+
+  clang::ast_matchers::MatchFinder MatchFinder;
+  AtomicChanges Changes;
+
+private:
+  FileContentMappings FileContents = {{"header.h", ""}};
+};
+
+class TransformerTest : public ClangRefactoringTestBase {
+protected:
+  TransformerTest() { appendToHeader(KHeaderContents); }
+};
+
+// Given string s, change strlen($s.c_str()) to $s.size().
+static RewriteRule ruleStrlenSize() {
+  StringRef StringExpr = "strexpr";
+  auto StringType = namedDecl(hasAnyName("::basic_string", "::string"));
+  return buildRule(
+             callExpr(
+                 callee(functionDecl(hasName("strlen"))),
+                 hasArgument(0, cxxMemberCallExpr(
+                                    on(expr(hasType(isOrPointsTo(StringType)))
+                                           .bind(StringExpr)),
+                                    callee(cxxMethodDecl(hasName("c_str")))))))
+      // Specify the intended type explicitly, because the matcher "type" of
+      // `callExpr()` is `Stmt`, not `Expr`.
+      .as<clang::Expr>()
+      .replaceWith("REPLACED")
+      .because("Use size() method directly on string.");
+}
+
+TEST_F(TransformerTest, StrlenSize) {
+  std::string Input = "int f(string s) { return strlen(s.c_str()); }";
+  std::string Expected = "int f(string s) { return REPLACED; }";
+  testRule(ruleStrlenSize(), Input, Expected);
+}
+
+// Tests that no change is applied when a match is not expected.
+TEST_F(TransformerTest, NoMatch) {
+  std::string Input = "int f(string s) { return s.size(); }";
+  testRule(ruleStrlenSize(), Input, Input);
+}
+
+// Tests that expressions in macro arguments are rewritten (when applicable).
+TEST_F(TransformerTest, StrlenSizeMacro) {
+  std::string Input = R"cc(
+#define ID(e) e
+    int f(string s) { return ID(strlen(s.c_str())); })cc";
+  std::string Expected = R"cc(
+#define ID(e) e
+    int f(string s) { return ID(REPLACED); })cc";
+  testRule(ruleStrlenSize(), Input, Expected);
+}
+
+// Tests replacing an expression.
+TEST_F(TransformerTest, Flag) {
+  StringRef Flag = "flag";
+  RewriteRule Rule =
+      buildRule(
+          cxxMemberCallExpr(on(expr(hasType(cxxRecordDecl(hasName(
+                                        "proto::ProtoCommandLineFlag"))))
+                                   .bind(Flag)),
+                            unless(callee(cxxMethodDecl(hasName("GetProto"))))))
+          .change<clang::Expr>(Flag)
+          .replaceWith("EXPR")
+          .because("Use GetProto() to access proto fields.");
+
+  std::string Input = R"cc(
+    proto::ProtoCommandLineFlag flag;
+    int x = flag.foo();
+    int y = flag.GetProto().foo();
+  )cc";
+  std::string Expected = R"cc(
+    proto::ProtoCommandLineFlag flag;
+    int x = EXPR.foo();
+    int y = flag.GetProto().foo();
+  )cc";
+
+  testRule(std::move(Rule), Input, Expected);
+}
+
+TEST_F(TransformerTest, NodePartNameNamedDecl) {
+  StringRef Fun = "fun";
+  RewriteRule Rule = buildRule(functionDecl(hasName("bad")).bind(Fun))
+                         .change<clang::FunctionDecl>(Fun, NodePart::Name)
+                         .replaceWith("good");
+
+  std::string Input = R"cc(
+    int bad(int x);
+    int bad(int x) { return x * x; }
+  )cc";
+  std::string Expected = R"cc(
+    int good(int x);
+    int good(int x) { return x * x; }
+  )cc";
+
+  testRule(Rule, Input, Expected);
+}
+
+TEST_F(TransformerTest, NodePartNameDeclRef) {
+  std::string Input = R"cc(
+    template <typename T>
+    T bad(T x) {
+      return x;
+    }
+    int neutral(int x) { return bad<int>(x) * x; }
+  )cc";
+  std::string Expected = R"cc(
+    template <typename T>
+    T bad(T x) {
+      return x;
+    }
+    int neutral(int x) { return good<int>(x) * x; }
+  )cc";
+
+  StringRef Ref = "ref";
+  testRule(buildRule(declRefExpr(to(functionDecl(hasName("bad")))).bind(Ref))
+               .change<clang::Expr>(Ref, NodePart::Name)
+               .replaceWith("good"),
+           Input, Expected);
+}
+
+TEST_F(TransformerTest, NodePartNameDeclRefFailure) {
+  std::string Input = R"cc(
+    struct Y {
+      int operator*();
+    };
+    int neutral(int x) {
+      Y y;
+      int (Y::*ptr)() = &Y::operator*;
+      return *y + x;
+    }
+  )cc";
+
+  StringRef Ref = "ref";
+  testRule(buildRule(declRefExpr(to(functionDecl())).bind(Ref))
+               .change<clang::Expr>(Ref, NodePart::Name)
+               .replaceWith("good"),
+           Input, Input);
+}
+
+TEST_F(TransformerTest, NodePartMember) {
+  StringRef E = "expr";
+  RewriteRule Rule = buildRule(memberExpr(member(hasName("bad"))).bind(E))
+                         .change<clang::Expr>(E, NodePart::Member)
+                         .replaceWith("good");
+
+  std::string Input = R"cc(
+    struct S {
+      int bad;
+    };
+    int g() {
+      S s;
+      return s.bad;
+    }
+  )cc";
+  std::string Expected = R"cc(
+    struct S {
+      int bad;
+    };
+    int g() {
+      S s;
+      return s.good;
+    }
+  )cc";
+
+  testRule(Rule, Input, Expected);
+}
+
+TEST_F(TransformerTest, NodePartMemberQualified) {
+  std::string Input = R"cc(
+    struct S {
+      int bad;
+      int good;
+    };
+    struct T : public S {
+      int bad;
+    };
+    int g() {
+      T t;
+      return t.S::bad;
+    }
+  )cc";
+  std::string Expected = R"cc(
+    struct S {
+      int bad;
+      int good;
+    };
+    struct T : public S {
+      int bad;
+    };
+    int g() {
+      T t;
+      return t.S::good;
+    }
+  )cc";
+
+  StringRef E = "expr";
+  testRule(buildRule(memberExpr().bind(E))
+               .change<clang::Expr>(E, NodePart::Member)
+               .replaceWith("good"),
+           Input, Expected);
+}
+
+TEST_F(TransformerTest, NodePartMemberMultiToken) {
+  std::string Input = R"cc(
+    struct Y {
+      int operator*();
+      int good();
+      template <typename T> void foo(T t);
+    };
+    int neutral(int x) {
+      Y y;
+      y.template foo<int>(3);
+      return y.operator *();
+    }
+  )cc";
+  std::string Expected = R"cc(
+    struct Y {
+      int operator*();
+      int good();
+      template <typename T> void foo(T t);
+    };
+    int neutral(int x) {
+      Y y;
+      y.template good<int>(3);
+      return y.good();
+    }
+  )cc";
+
+  StringRef MemExpr = "member";
+  testRule(buildRule(memberExpr().bind(MemExpr))
+               .change<clang::Expr>(MemExpr, NodePart::Member)
+               .replaceWith("good"),
+           Input, Expected);
+}
+
+//
+// Negative tests (where we expect no transformation to occur).
+//
+
+TEST_F(TransformerTest, NoTransformationInMacro) {
+  std::string Input = R"cc(
+#define MACRO(str) strlen((str).c_str())
+    int f(string s) { return MACRO(s); })cc";
+  testRule(ruleStrlenSize(), Input, Input);
+}
+
+// This test handles the corner case where a macro called within another macro
+// expands to matching code, but the matched code is an argument to the nested
+// macro.  A simple check of isMacroArgExpansion() vs. isMacroBodyExpansion()
+// will get this wrong, and transform the code. This test verifies that no such
+// transformation occurs.
+TEST_F(TransformerTest, NoTransformationInNestedMacro) {
+  std::string Input = R"cc(
+#define NESTED(e) e
+#define MACRO(str) NESTED(strlen((str).c_str()))
+    int f(string s) { return MACRO(s); })cc";
+  testRule(ruleStrlenSize(), Input, Input);
+}
+} // namespace
+} // namespace tooling
+} // namespace clang