Page MenuHomePhabricator

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

Authored by ymandel on Mar 14 2019, 11:20 AM.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ilya-biryukov added inline comments.Mar 29 2019, 5:59 AM
clang/unittests/Tooling/TransformerTest.cpp
62

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

89

NIT: remove redundant braces

160

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 updated this revision to Diff 192889.Mar 29 2019, 12:24 PM
ymandel marked an inline comment as done.
  • Assorted changes in response to comments.
ymandel marked 19 inline comments as done.Mar 29 2019, 12:33 PM

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
71

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.

90

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

98

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

123

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

176

Yes, not sure why did that. thanks.

clang/lib/Tooling/Refactoring/Transformer.cpp
134

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?

151

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

158

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
62

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

160

Thanks, much cleaner this way.

ilya-biryukov marked an inline comment as done.Apr 1 2019, 10:38 AM

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
71

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.

82

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

90

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.

125

NIT: bound to this id

136

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

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

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?.

clang/lib/Tooling/Refactoring/Transformer.cpp
134

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.

158

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
62

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 updated this revision to Diff 193219.Apr 1 2019, 6:49 PM
ymandel marked 28 inline comments as done.

Assorted changes in response to comments.

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

ymandel added inline comments.Apr 1 2019, 6:50 PM
clang/include/clang/Tooling/Refactoring/Transformer.h
71

Agreed. Keeping it.

90

went w/ the second suggestion.

136

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

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

clang/lib/Tooling/Refactoring/Transformer.cpp
134

ah...

158

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

clang/unittests/Tooling/TransformerTest.cpp
62

static sounds good to me.

ilya-biryukov accepted this revision.Apr 2 2019, 1:00 AM

Looks great, thanks!

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

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.

114

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

This revision is now accepted and ready to land.Apr 2 2019, 1:00 AM
ymandel updated this revision to Diff 193260.Apr 2 2019, 5:41 AM
ymandel marked 3 inline comments as done.

Small tweaks in response to comments.

Thank you for the extremely helpful and detailed review!!

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

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 updated this revision to Diff 193306.Apr 2 2019, 8:53 AM

Remove dependency on NodeIds.

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 accepted this revision.Apr 3 2019, 12:55 AM

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.

This revision was automatically updated to reflect the committed changes.
ABataev added a subscriber: ABataev.Apr 3 2019, 9:42 AM

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.

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 reopened this revision.Apr 3 2019, 12:59 PM

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

This revision is now accepted and ready to land.Apr 3 2019, 12:59 PM
ABataev removed a subscriber: ABataev.Apr 3 2019, 1:00 PM
ymandel updated this revision to Diff 193584.Apr 3 2019, 1:36 PM
ymandel added a subscriber: ABataev.

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?).

Ilya, this diff fixes the breakage. PTAL.

ilya-biryukov added a comment.EditedApr 4 2019, 2:23 AM

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.

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

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.

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...

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.

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 added a comment.Apr 4 2019, 7:12 AM

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 updated this revision to Diff 193748.Apr 4 2019, 10:47 AM

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

ymandel updated this revision to Diff 193750.Apr 4 2019, 10:49 AM

Rebasing to parent D60269 so that diffs show correctly.

ilya-biryukov accepted this revision.Apr 5 2019, 2:42 AM

One more LGTM.

ymandel updated this revision to Diff 193869.Apr 5 2019, 7:15 AM

Rebase onto master

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2019, 8:12 AM