Page MenuHomePhabricator

[LibTooling] Extend Transformer to support multiple simultaneous changes.
ClosedPublic

Authored by ymandel on Apr 8 2019, 7:56 AM.

Details

Summary

This revision allows users to specify independent changes to multiple (related) sections of the input. Previously, only a single section of input could be selected for replacement.

Diff Detail

Repository
rL LLVM

Event Timeline

ymandel created this revision.Apr 8 2019, 7:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2019, 7:56 AM
Herald added a subscriber: jfb. · View Herald Transcript

gentle ping

Sorry for the delay.
There seem to be a few changes that are unrelated to the actual patch. Could we separate various non-functional changes (moving code around, etc.) into a separate change to keep the diff for this one minimal?

clang/include/clang/Tooling/Refactoring/Transformer.h
80 ↗(On Diff #194147)

MatchChange or something similar might be a better name.
This actually tries to change the matched AST node to a textual replacement.

87 ↗(On Diff #194147)

I would've expected explanation to be the trait of the rewrite rule, since all changes have to be applied.
What's the reasoning behind having it at the level of separate changes? How would this explanation be used? For debugging purposes or displaying that to the user?

ymandel updated this revision to Diff 194883.Apr 12 2019, 7:30 AM
ymandel marked 2 inline comments as done.

Restore code ordering for unrelated code.

ymandel updated this revision to Diff 194886.Apr 12 2019, 7:32 AM

More code movement (putting things back).

Harbormaster completed remote builds in B30447: Diff 194886.
ymandel updated this revision to Diff 194887.Apr 12 2019, 7:36 AM

Final code shifting.

Sorry for the delay.
There seem to be a few changes that are unrelated to the actual patch. Could we separate various non-functional changes (moving code around, etc.) into a separate change to keep the diff for this one minimal?

I've done that as far as I can tell. Please let me know if I've missed anything.

clang/include/clang/Tooling/Refactoring/Transformer.h
80 ↗(On Diff #194147)

I chose this to contrast with an AST change. The idea being that we're specifying a replacement relative to source code locations (informed by the ast). If we later, say, integrate with your library I could imagine specifying changes to AST nodes. But, maybe I'm overthinking... If we're going to drop "text", what about "source?" be clearer than "text"? E.g, SourceChange or (my preference) SourceEdit?

87 ↗(On Diff #194147)

I think that for most cases, one explanation sufficies for the whole transformation. However, there are some tidies that emit multiple diagnoses (for example if changing before a declaration and a definition). Would it help if I clarify in the comments?

ilya-biryukov added inline comments.Apr 12 2019, 9:49 AM
clang/include/clang/Tooling/Refactoring/Transformer.h
80 ↗(On Diff #194147)

The reasons I find TextChange confusing is because I immediately think of something super-simple (a range of text + the replaced text), and I definitely don't think of the AST.

SourceChange and SourceEdit does not cause this confusion for me personally, so both look ok. Although they do look pretty similar.
Also, it's not actually a final edit, rather a description of it. So something like EditDescription could work too.

87 ↗(On Diff #194147)

Yeah, absolutely! Please document what it's used for and that would clear that up for me.
I actually thing that explaining every part of the transformation is probably too complicated, so most of the time you would want to have an explanation for the RewriteRule, not for each individual change.

The other challenge that I see is show to display these explanations to the user, i.e. how should the clients combine these explanations in order to get the full one? Should the RewriteRule have an explanation of the full transformation too?

190 ↗(On Diff #194887)

NIT: I'd suggest just paying a few extra lines for initializing the struct instead of using the ctor.

Transformation T;
T.Range = ...;
T.Replacement = ...;

v.push_back(std::move(T));

or

v.emplace_back();
v.back().Range = ...;
v.back().Replacement = ...;

But I can see why you might want a ctor instead. If you decide to leave it, consider re-adding the default ctor that got implicitly deleted as you defined this other one.

204 ↗(On Diff #194887)

Why would we change the interface here? Rewrite rule seemed like a perfect input to this function

clang/lib/Tooling/Refactoring/Transformer.cpp
164 ↗(On Diff #194887)

What if the changes intersect? I'd expect this function to handle this somehow, handling this is complicated and I feel we shouldn't rely on the clients to make it fast.

I've done that as far as I can tell. Please let me know if I've missed anything.

Many thanks!

ymandel updated this revision to Diff 194944.Apr 12 2019, 1:00 PM
ymandel marked 8 inline comments as done.

Responses to comments, including renaming TextChange and applyRewriteRule.

ymandel added inline comments.Apr 12 2019, 1:00 PM
clang/include/clang/Tooling/Refactoring/Transformer.h
80 ↗(On Diff #194147)

Right, I'm now tending way from the whole focus on text/source/etc. I agree with your point that this is operating at the AST level, especially in light of the discussion below on applyRewriteRule. Given that these are all anchored by an AST node, let's go with ASTEdit, unless that will conflict with the library that you're developing?

I agree that "Description" is more apt, but I feel like this is (somewhat) implicit in the fact that its a struct versus a function (which would actually be carrying out the action). I'm also afraid that "EditDescription" will read like an action, which may be a bit confusing (although the casing should help distinguish).

87 ↗(On Diff #194147)

I've revised the comments, changed the name to "Note" and put "Explanation" back into RewriteRule.

As for how to display these, I imagine an interface like clang tidy's fixit hints. The Explanation (if any) will be associated with the span of the entire match. The Notes will be associated with the target node span of each annotated change. WDYT?

190 ↗(On Diff #194887)

Agreed. I only use it one place, hardly worth the cost. thanks

204 ↗(On Diff #194887)

Good point. For the name, you're right -- but I think the name was misleading. The function doesn't really "apply" anything and what it does use is only the _change_ part of the rule -- it doesn't handle (and never even looks at) the matcher part, because it's already given the match.

Rather, this just translates change descriptions that are in terms of the AST into ones in terms of the source code. I've renamed it to translateChanges and updates the comments describing it. Let me know what you think.

clang/lib/Tooling/Refactoring/Transformer.cpp
164 ↗(On Diff #194887)

Right now, we rely on clients (as you mention). The clients will receive AtomicChanges that overlap and will have to handle that. clang::tooling::applyAtomicChanges will fail on this, although one could imagine a client that could be smarter.

That said, I'm fine with us handling this here, instead. My only argument against is that the same issue applies for matchers in general, so catching it here won't prevent the case where separate firings of the same rule (or of different rules if more than one is registered) try to change the source. Moreover, since we already have the logic elsewhere (clang::tooling::Replacements) we would be duplicating that here.

I've thought of using AtomicChange here directly, but there's no way to get the changes back in terms of SourceLocations, which we need for this function to be useable inside the ClangTidy adapter (FixItHint uses SourceLocations, not raw filename and offset).

What would you say to a FIXME to find a better means of failing fast?

On a related note, I see that this struct is very similar to clang::FixItHint. Do you think its worth just using that directly? Or, is this a sufficiently different situation to merit a different type definition?

ilya-biryukov added inline comments.Apr 15 2019, 5:05 AM
clang/include/clang/Tooling/Refactoring/Transformer.h
80 ↗(On Diff #194147)

ASTEdit seems like a proper name here, LG!

87 ↗(On Diff #194147)

Do we really need the AST information to expose the edits to the users?
IIUC, clang-tidy uses the information from textual replacements to render the changes produced by the fix-its today.

I guess it might be useful to add extra notes to clang-tidy warnings that have corresponding fix-its, but is the transformers library the right layer to produce those?
I haven't seen the proposed glue to clang-tidy yet, maybe that would make more sense when I see it.

One of the other reasons I ask this is that it seems that without Note we don't strictly ASTEditBuilder, we could replace

change("ref").to("something"); // without nodepart
change("ref", NodePart::Member).to("something");

with

change("ref", "something")
change("ref", NodePart::Member, "something");

That would remove the boilerplate of the builder, simplifying the code a bit.

That trick would be hard to pull if we have a Note field inside, we'll need more overloads and having note and replacement after each other might be confusing (they have the same type, might be hard to read without the guiding .to() and .because())

204 ↗(On Diff #194887)

The new name looks ok. Although if the RewriteRule is the central concept of this file (and it looks like it is), I think it'd make sense to make this a narrower interface, accepting a RewriteRule as a parameter.

But up to you, as long as we have this spelled out in the docs, I don't think this should cause problems to the users.

clang/lib/Tooling/Refactoring/Transformer.cpp
164 ↗(On Diff #194887)

I can hardly imagine clients doing anything smarter than failing on intersecting changes (at least for the general case).

That said, I do see the reasons to keep this error-handling out of this function for the matter of simplicity. And totally agree that in order to deal with it properly we'd want to return something like tooling::Replacements to make sure we can reuse facilities that handle the overlapping cases properly.

Could we add a comment to the declaration, explaining that returned ranges might overlap and clients are expected to deal with that (and maybe mentioning AtomicChanges and tooling::Replacements as potential options to do so).

I'd avoid using clang::FixItHint while we keep experimenting with the interface of the library, having our own type gives us more freedom to change it. As soon as the interface is stable, we can take revisit this (and FWIW, FixItHint does not look like a good name for this).

ymandel updated this revision to Diff 195201.Apr 15 2019, 9:09 AM
ymandel marked 10 inline comments as done.

Clarified comments for translateEdits function.

ymandel added inline comments.Apr 15 2019, 9:10 AM
clang/include/clang/Tooling/Refactoring/Transformer.h
87 ↗(On Diff #194147)

Breaking this explicitly into two questions:

  1. Do Notes belong here?
  2. Can we drop the builder?
  1. Do Notes belong here?

I think so. When users provide a diagnostic, they are specifying its location. So, we don't quite need the AST but we do need location info. The diagnostic generator does take information from the replacements themselves, but that's not alone. For example: clang-tidy/readability/ConstReturnTypeCheck.cpp:104. That demos both the diagnostic construction and multiple diagnostics in a single tidy result.

Given that, i think that RewriteRules are the appropriate place. The goal is that they be self contained, so I think the explanations should be bundled with the description of that actual change. An example approach to merging with clang tidy is here: https://github.com/ymand/llvm-project/blob/transformer/clang-tools-extra/clang-tidy/utils/TransformerTidy.cpp (although that's based on a somewhat different version of the Transformer library; it should make clear what I have in mind).

  1. Do we need the builder?

No, despite my opinion that notes belong. Given the explanation on RewriteRule, I think notes will be used only rarely (like in the example clang-tidy above; but that's not standard practice). So, we can provide the two overloads of change that you mention and then let users assign the note directly in those rare cases they need it. then, we can drop the builder.

So, I propose keeping the note but dropping the builder. WDYT?

204 ↗(On Diff #194887)

SG. To be clear -- this is intended only as an "advanced" part of the API, to factor out code that will be common to:

  1. Transformer (here)
  2. TransformerTidy -- the clang-tidy version of this code that creates a ClangTidy from a RewriteRule
  3. applyRewriteRule(ASTContext) -- an upcoming function that will do what this function looked like it was doing. It will apply a rewriterule (including the matcher) to a node or an ASTContext.

However, I don't expect that any standard users should need this. I'd originally placed it in an "internal" namespace, but it's not quite internal because TransformerTidy will be living somewhere else. So, it should be exposed, but its really only for other implementors rather than standard users.

clang/lib/Tooling/Refactoring/Transformer.cpp
164 ↗(On Diff #194887)

Agreed. I've updated the comments as per your suggestion. Also added a note to the Transformer constructor below along the same lines.

For the future: we should think about adding an option to have Transformer handle conflicts instead of leaving it up to the user. It would probably require a somewhat different API -- e.g. creating one large AtomicChange and handing that off at destruction time, rather then calling Consumer with independent changes for each match.

A few quick responses, will take a closer look again tomorrow.

clang/include/clang/Tooling/Refactoring/Transformer.h
87 ↗(On Diff #194147)

Thanks for pointing out how this usage in clang-tidy. Seems to be the only way to put it into clang-tidy (and there are good reasons for that, e.g. having the full context of the change).

So, I propose keeping the note but dropping the builder. WDYT?

LGTM!

204 ↗(On Diff #194887)

The tidy use-case makes sense now, thanks! The fact that fix-it hints is the only way to report changes there kinda forces us to have a function that works on a level of transformations.

clang/lib/Tooling/Refactoring/Transformer.cpp
164 ↗(On Diff #194887)

Thanks, LG!
Agree that having a simpler interface would be nice, still not sure how to properly wire it up with matchers, especially in the clang-tidy use-case.
But if we can run the library function would run the matchers on its own, a simpler interface seems doable.

ymandel updated this revision to Diff 195234.Apr 15 2019, 12:22 PM
ymandel marked 2 inline comments as done.

Deleted ASTEdit builder; added more change overloads.

ymandel updated this revision to Diff 195235.Apr 15 2019, 12:24 PM

Fix include ordering (bad clang-format config).

ymandel marked an inline comment as done.Apr 15 2019, 12:27 PM
ymandel added inline comments.
clang/include/clang/Tooling/Refactoring/Transformer.h
87 ↗(On Diff #194147)

Removed the builder, added more change overloads. Also renamed changeRoot to changeMatched because I think it's a better description.

The only part I don't love (here and elsewhere) is the duplicate overloads one each for std::string and TextGenerator.

Thanks for the update, looks good! Just a few nits from my side.

The only part I don't love (here and elsewhere) is the duplicate overloads one each for std::string and TextGenerator.

Totally agree, that's not ideal. Would be nice to find a way to workaround this.

clang/include/clang/Tooling/Refactoring/Transformer.h
76 ↗(On Diff #195235)

Comment and the code example still mention ASTBuilder.

132 ↗(On Diff #195235)

Any reason to use a different name? Having an overload for change without Target does not look confusing.

change<clang::Expr>("replacement(1,2,3)");
vs 
changeMatched<clang::Expr>("replacement(1,2,3)");
147 ↗(On Diff #195235)

Are addition/removal of headers something that is going to land in the future?

Maybe leave out this comment until it actually lands?

clang/lib/Tooling/Refactoring/Transformer.cpp
185 ↗(On Diff #195235)

Could we add a test for this case?
(Sorry for spam if there's one already and I missed it)

213 ↗(On Diff #195235)

NIT: maybe add braces?
I believe they're not necessary in LLVM Style, but IMO add readability in case of nested statements with their own braces, like if in this code.

clang/unittests/Tooling/TransformerTest.cpp
355 ↗(On Diff #195235)

Could we add a test with intersecting ranges?
To have a regression test in case we'll change behavior in this corner case later.

ymandel updated this revision to Diff 195364.Apr 16 2019, 6:23 AM

Cleaned up header comments; added tests.

ymandel updated this revision to Diff 195365.Apr 16 2019, 6:29 AM
ymandel marked 6 inline comments as done.

Cleaned up header comments; added tests.

ymandel marked 5 inline comments as done.Apr 16 2019, 6:32 AM
ymandel added inline comments.
clang/include/clang/Tooling/Refactoring/Transformer.h
76 ↗(On Diff #195235)

Thanks. I noticed I was also using an older version that integrated with Stencil. I've updated to make that explicit. I expect Stencil to land really soon (and can wait to commit until then), but I can remove the reference if you prefer. Without Stencils, though, the examples are pretty lame.

I also noticed other spurious mentions of "builder" below and deleted them as well.

132 ↗(On Diff #195235)

Good idea. changed as you suggested.

147 ↗(On Diff #195235)

Good catch

clang/lib/Tooling/Refactoring/Transformer.cpp
185 ↗(On Diff #195235)

Nope -- I hadn't tested this yet. Thanks!

213 ↗(On Diff #195235)

Done. I personally always prefer braces on for loops, so always happy to add them. ;)

clang/unittests/Tooling/TransformerTest.cpp
355 ↗(On Diff #195235)

Added two tests: one for conflict in single match, one for conflict between matches.

This revision is now accepted and ready to land.Apr 16 2019, 6:47 AM

Not sure if you want to land this before or after stencil. This seems useful even without the latter.

This revision was automatically updated to reflect the committed changes.
ymandel marked 2 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2019, 10:50 AM

hi ymandel,

When I run "check-all", there are some warning/error in TransformerTest.cpp as follow. My version is llvm:0ee120077 and clang:d87ee8e678. Could you please have a fix or guild me how to fix it?

In file included from myLLVM/llvm/utils/unittest/googlemock/include/gmock/internal/gmock-internal-utils.h:47:0,

from myLLVM/llvm/utils/unittest/googlemock/include/gmock/gmock-actions.h:46,
from myLLVM/llvm/utils/unittest/googlemock/include/gmock/gmock.h:58,
from myLLVM/llvm/tools/clang/unittests/Tooling/TransformerTest.cpp:13:

myLLVM/llvm/utils/unittest/googletest/include/gtest/gtest.h: In instantiation of 'testing::AssertionResult testing::internal::CmpHelperEQ(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int; T2 = int]':
myLLVM/llvm/utils/unittest/googletest/include/gtest/gtest.h:1421:23: required from 'static testing::AssertionResult testing::internal::EqHelper<lhs_is_null_literal>::Compare(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int; T2 = int; bool lhs_is_null_literal = false]'
myLLVM/llvm/tools/clang/unittests/Tooling/TransformerTest.cpp:372:3: required from here
myLLVM/llvm/utils/unittest/googletest/include/gtest/gtest.h:1392:11: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

if (lhs == rhs) {
        ^

hi ymandel,
When I run "check-all", there are some warning/error in TransformerTest.cpp as follow. My version is llvm:0ee120077 and clang:d87ee8e678. Could you please have a fix or guild me how to fix it?


In file included from myLLVM/llvm/utils/unittest/googlemock/include/gmock/internal/gmock-internal-utils.h:47:0,

from myLLVM/llvm/utils/unittest/googlemock/include/gmock/gmock-actions.h:46,
from myLLVM/llvm/utils/unittest/googlemock/include/gmock/gmock.h:58,
from myLLVM/llvm/tools/clang/unittests/Tooling/TransformerTest.cpp:13:

myLLVM/llvm/utils/unittest/googletest/include/gtest/gtest.h: In instantiation of 'testing::AssertionResult testing::internal::CmpHelperEQ(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int; T2 = int]':
myLLVM/llvm/utils/unittest/googletest/include/gtest/gtest.h:1421:23: required from 'static testing::AssertionResult testing::internal::EqHelper<lhs_is_null_literal>::Compare(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int; T2 = int; bool lhs_is_null_literal = false]'
myLLVM/llvm/tools/clang/unittests/Tooling/TransformerTest.cpp:372:3: required from here
myLLVM/llvm/utils/unittest/googletest/include/gtest/gtest.h:1392:11: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

if (lhs == rhs) {
        ^

Sorry about that. Have you sync'd to head? I thought that was fixed with r358745 (from Bjorn Pettersson).

hi ymandel,
When I run "check-all", there are some warning/error in TransformerTest.cpp as follow. My version is llvm:0ee120077 and clang:d87ee8e678. Could you please have a fix or guild me how to fix it?


In file included from myLLVM/llvm/utils/unittest/googlemock/include/gmock/internal/gmock-internal-utils.h:47:0,

from myLLVM/llvm/utils/unittest/googlemock/include/gmock/gmock-actions.h:46,
from myLLVM/llvm/utils/unittest/googlemock/include/gmock/gmock.h:58,
from myLLVM/llvm/tools/clang/unittests/Tooling/TransformerTest.cpp:13:

myLLVM/llvm/utils/unittest/googletest/include/gtest/gtest.h: In instantiation of 'testing::AssertionResult testing::internal::CmpHelperEQ(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int; T2 = int]':
myLLVM/llvm/utils/unittest/googletest/include/gtest/gtest.h:1421:23: required from 'static testing::AssertionResult testing::internal::EqHelper<lhs_is_null_literal>::Compare(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int; T2 = int; bool lhs_is_null_literal = false]'
myLLVM/llvm/tools/clang/unittests/Tooling/TransformerTest.cpp:372:3: required from here
myLLVM/llvm/utils/unittest/googletest/include/gtest/gtest.h:1392:11: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

if (lhs == rhs) {
        ^

Sorry about that. Have you sync'd to head? I thought that was fixed with r358745 (from Bjorn Pettersson).

I see. Thank you very much!