Page MenuHomePhabricator

[libTooling] Fix Transformer to work with ambient traversal kinds
ClosedPublic

Authored by ymandel on May 26 2020, 8:02 PM.

Details

Summary

RewriteRule's applyFirst was brittle with respect to the default setting of the TraversalKind (revealed by D72534). This patch builds awareness of traversal kinds directly into rewrite rules so that they are insensitive to any changes in defaults.

Diff Detail

Event Timeline

ymandel created this revision.May 26 2020, 8:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2020, 8:02 PM

It might make sense for now (in order to unblock you) to make the Transformer library explicitly require the AsIs mode. I am not so familiar with the transformer library, but I think you can do that by adding traverse(AsIs, ...) in Transformer::registerMatchers.

The real fix is probably in adjusting transformer::detail::buildMatchers. I can look into that in a few hours.

It might make sense for now (in order to unblock you) to make the Transformer library explicitly require the AsIs mode. I am not so familiar with the transformer library, but I think you can do that by adding traverse(AsIs, ...) in Transformer::registerMatchers.

The real fix is probably in adjusting transformer::detail::buildMatchers. I can look into that in a few hours.

Thanks for the suggestion - I agree that changing registerMatchers would be a short term fix, but I think its better to just fix buildMatchers directly. I'm working on a patch now, so need to look into it (though thanks for the offer!)

ymandel updated this revision to Diff 266729.May 27 2020, 8:32 PM

adds a fix to transformer

ymandel retitled this revision from [libTooling][NFC] Demo bug introduced in D72534. to [libTooling] Fix Transformer to work with ambient traversal kinds.May 27 2020, 8:34 PM
ymandel edited the summary of this revision. (Show Details)
ymandel edited reviewers, added: steveire; removed: hokein.
ymandel edited subscribers, added: hokein; removed: steveire.
MaskRay accepted this revision.May 27 2020, 9:03 PM
This revision is now accepted and ready to land.May 27 2020, 9:03 PM
gribozavr2 accepted this revision.May 28 2020, 2:08 AM
gribozavr2 added a subscriber: gribozavr2.
gribozavr2 added inline comments.
clang/lib/Tooling/Transformer/RewriteRule.cpp
154

I don't see any callers using the new argument. Do we need this flexibility?

clang/unittests/Tooling/TransformerTest.cpp
574

a matcher => a top-level matcher

575

I'd say "when the AST traversal skips implicit nodes".

576

ti => to

AsIs) => AsIs

ymandel updated this revision to Diff 266831.May 28 2020, 6:02 AM

updated per comments and after rebasing

ymandel updated this revision to Diff 266835.May 28 2020, 6:09 AM
ymandel marked 2 inline comments as done.

typo fix

ymandel updated this revision to Diff 266837.May 28 2020, 6:11 AM

more typo fixes

ymandel marked 3 inline comments as done.May 28 2020, 6:11 AM
ymandel added inline comments.
clang/lib/Tooling/Transformer/RewriteRule.cpp
154

No. Abortive effort to support pulling it from the context. However, turned out the context is not available when this is called. Moreover, I've decided it's the wrong UX altogether. Matchers are not polymorphic (usually) -- they only make sense with respect to a single TK, whether explicit or implicit. So, my goal is that RewriteRules will specify how "open" matchers are interpreted -- that is, the matchers TK will be set at rule creation time rather than at rule execution time (by pulling from the context). I don't achieve that goal in this revision because I'm only updating applyFirst, but I should follow up with changes makeRule and the corresponding docs to make this clear.`

That said, I chose TK_IgnoreUnlessSpelledInSource as the default setting for "open" matchers to be consistent with the consensus that its worth trying this out as a default.

gribozavr2 accepted this revision.May 28 2020, 8:41 AM
This revision was automatically updated to reflect the committed changes.