Page MenuHomePhabricator

[clang-tidy] Add support writing a check as a Transformer rewrite rule.
ClosedPublic

Authored by ymandel on May 1 2019, 9:40 AM.
Tags
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project
  • Restricted Project
Subscribers

Details

Summary

This revision introduces an adaptor from Transformer's rewrite rules
(clang::tooling::RewriteRule) to ClangTidyCheck. For example, given a
RewriteRule MyCheckAsRewriteRule, it lets one define a tidy check as follows:

class MyTidyCheck : public TransformerClangTidyCheck {
 public:
  MyTidyCheck(StringRef Name, ClangTidyContext *Context)
      : TransformerClangTidyCheck(MyCheckAsRewriteRule, Name, Context) {}
};

Diff Detail

Repository
rL LLVM

Event Timeline

ymandel created this revision.May 1 2019, 9:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2019, 9:40 AM
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/utils/TransformerTidy.cpp
22 ↗(On Diff #197573)

Please don't use auto when return type is not spelled at same statement or iterator. Same for other places.

clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp
10 ↗(On Diff #197573)

Unnecessary empty line.

Eugene.Zelenko added a project: Restricted Project.
ymandel updated this revision to Diff 197589.May 1 2019, 10:32 AM

Reduced use of auto.

ymandel marked 2 inline comments as done.May 1 2019, 10:36 AM
ymandel added inline comments.
clang-tools-extra/clang-tidy/utils/TransformerTidy.cpp
22 ↗(On Diff #197573)

I left only the iterator's auto, expanded the rest. Let me know if I've converted too many. I wonder about this one here, for example, because the type of the map is essentially an internal detail of BoundNodes.

Eugene.Zelenko added inline comments.May 1 2019, 10:52 AM
clang-tools-extra/clang-tidy/utils/TransformerTidy.cpp
51 ↗(On Diff #197589)

It's iterator, so auto could be used instead of type.

ymandel updated this revision to Diff 197597.May 1 2019, 11:13 AM
ymandel marked an inline comment as done.

reinstated an auto

ymandel marked 2 inline comments as done.May 1 2019, 11:14 AM
ymandel added inline comments.
clang-tools-extra/clang-tidy/utils/TransformerTidy.cpp
51 ↗(On Diff #197589)

great, i wasn't sure whether this counted as such.

I like the new framework!

clang-tools-extra/clang-tidy/utils/TransformerTidy.cpp
43 ↗(On Diff #197597)

these if-else are single statement, so the braces can be ellided.

clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp
38 ↗(On Diff #197597)

If we allow to pass in a callable, that returns a RewriteRulewe would have a more flexible framework in clang-tidy.
Going with this for now is ok in my opinion as the only good usecase that comes to my mind would be statistics.

But If some transformation needs to remember what has been done before only a RewriteRule as state is not sufficient and stateful actions would require global state (MEH!).

ymandel updated this revision to Diff 197836.May 2 2019, 12:00 PM
ymandel marked 2 inline comments as done.

responded to comments and shortened (unnecessarily long) variable name.

ymandel marked an inline comment as done.May 2 2019, 12:02 PM

I like the new framework!

Great!

clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp
38 ↗(On Diff #197597)

If we allow to pass in a callable, that returns a RewriteRule we would have a more flexible framework in clang-tidy.
Going with this for now is ok in my opinion as the only good usecase that comes to my mind would be statistics.

I'm fine with something more general, but I don't think I follow the particulars you have in mind. Even w/ a callable, wouldn't that only be invoked *once* by clang-tidy, when it creates the class? What kind of statistics do you have in mind?

But If some transformation needs to remember what has been done before only a RewriteRule as state is not sufficient and stateful actions would require global state (MEH!).

Indeed. The current design does preclude a (non gross) way of collecting state over multiple invocations. Since this is rare, I think that's fine for now. But, our plan is to add support for a Translation Unit level API that supports things like collecting state, adding and removing header includes, inserting using decls and any other TU level operations.

Gentle ping...

alexfh edited reviewers, added: ilya-biryukov, gribozavr; removed: alexfh, hokein.May 7 2019, 7:50 AM

@aaron.ballman, would be a good reviewer for this? I'm happy to look at the transformer bits, but I'm definitely the wrong one to asses it from the clang-tidy perspective.

ymandel updated this revision to Diff 200072.May 17 2019, 11:39 AM

Updated to incorporate changes to Transformer.

ymandel added a subscriber: gribozavr.

Ping. Is anyone willing to be the reviewer for this revision?

Thanks!

gribozavr accepted this revision.May 21 2019, 9:05 AM
gribozavr added inline comments.
clang-tools-extra/clang-tidy/utils/TransformerTidy.h
32 ↗(On Diff #200072)

I'd prefer a name like "TransformerClangTidyCheck", it will only appear in a few places, but is much more clear.

38 ↗(On Diff #200072)

I'd suggest to add comments telling users to not override these methods in subclasses.

This revision is now accepted and ready to land.May 21 2019, 9:05 AM
ymandel marked 2 inline comments as done.May 21 2019, 9:52 AM
ymandel added inline comments.
clang-tools-extra/clang-tidy/utils/TransformerTidy.h
32 ↗(On Diff #200072)

will do, but should I also rename the files correspondingly?

38 ↗(On Diff #200072)

sure. would marking them as final make sense, then?

gribozavr added inline comments.May 21 2019, 10:16 AM
clang-tools-extra/clang-tidy/utils/TransformerTidy.h
32 ↗(On Diff #200072)

Yes please.

38 ↗(On Diff #200072)

Right, that's the perfect tool for the job.

ymandel updated this revision to Diff 200536.May 21 2019, 10:33 AM

Renamed TransformerTidy to TransformerClangTidyCheck; classes and files.

ymandel retitled this revision from [clang-tidy] Add support writing a check as a Transformer rewrite rule. to [clang-tidy] Add support writing a check as a Transformer rewrite rule..May 21 2019, 10:36 AM
ymandel edited the summary of this revision. (Show Details)
ymandel marked 4 inline comments as done.

Thanks for the review.

Just a passing comment, Is this:

a) a different type of check that would be used for a different type of check than previous clang-tidy checks
b) or is this a replacement on the previous style?

Is there any documentation about the benefits of this method vs the old method? or of why we would potentially use one and not the other?

Is there a change needed to add_new_check.py to have it generate this new style? (its ok if the answer is yes in a future revision)

Do you foresee people using this mechanism going foward?

I do like the brevity of it and I do like the GTEST unit tests, (I struggled more with the lit tests than anything else, mainly for python on cygwin reasons)

It looks good, even if it looks like I need to go learn how tooling::stencil works... a little bit of documentation might not go a miss on how to get started with it...

ymandel added a comment.EditedMay 21 2019, 11:59 AM

Just a passing comment, Is this:

a) a different type of check that would be used for a different type of check than previous clang-tidy checks
b) or is this a replacement on the previous style?

This is a replacement for a certain class of checks. Mostly for simple checks that are primarily pattern + reconstruction, without complex logic (at least for now). The long-term goal is to provide a concise alternative for the large majority of checks.

Is there any documentation about the benefits of this method vs the old method? or of why we would potentially use one and not the other?

There's a doc describing the framework here
https://docs.google.com/document/d/1ppw0RhjwsrbBcHYhI85pe6ISDbA6r5d00ot3N8cQWeQ/edit?usp=sharing
and some corresponding discussion on the cfe-dev mailing list (and in the doc comments).

Is there a change needed to add_new_check.py to have it generate this new style? (its ok if the answer is yes in a future revision)

Do you foresee people using this mechanism going forward?

Not yet, but once it is more mature, yes. I would hope that it will be the more common approach. At that point, we can adjust the scripts accordingly.

I do like the brevity of it and I do like the GTEST unit tests, (I struggled more with the lit tests than anything else, mainly for python on cygwin reasons)

It looks good, even if it looks like I need to go learn how tooling::stencil works... a little bit of documentation might not go a miss on how to get started with it...

Thanks. Yes, it very much requires that you familiarize yourself with Stencil and RewriteRule (Tooling/Refactoring/Stencil.h and Transformer.h). I'm not sure what more to add that is specific to clang tidy. The intent is that, if your check can be expressed as a RewriteRule then there's really nothing you need to know about clang-tidy beyond what's in the header file -- you just drop the rule into TransformerClangTidyCheck. If you have questions after you see the Tooling documentation, please let me know and I'm happy to expand the comments here. Both Stencil and Transformer are quite and still under active development. Once they've stabilized a bit, I also plan to send out a more general announcement about this to the cfe-dev list.

ilya-biryukov added inline comments.May 22 2019, 12:07 AM
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
46 ↗(On Diff #200536)

The users will see this for every case without explanation, right?
I'd expect the rules without explanation to be somewhat common, maybe don't show any message at all in that case?

clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
1 ↗(On Diff #200536)

NIT: maybe use TransformerCheck for brevity? I'm not a clang-tidy maintainer, though, so not sure whether that aligns with the rest of the code.

ymandel marked 4 inline comments as done.May 22 2019, 5:31 AM
ymandel added inline comments.
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
46 ↗(On Diff #200536)

There's no option to call diag() without a message. We could pass an empty string , but that may be confusing given the way the message is concatenated here:
https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp#L204

So, no matter what, there will be some message to go w/ the diagnostic. I figure that being explicit about the lack of explanation is better than an empty string, but don't feel strongly about this.

clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
1 ↗(On Diff #200536)

It was TransformerTidy and Dmitri suggested I be explicit. I think TransformerCheck is better than TransformerTidy, but I also see the argument in spelling it out. WDYT?

ilya-biryukov marked an inline comment as done.May 22 2019, 8:03 AM
ilya-biryukov added inline comments.
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
46 ↗(On Diff #200536)

Ah, so all the options are bad. I can see why you had this design in transformers in the first place.
I wonder if we should check the explanations are always set for rewrite rules inside the clang-tidy transformation?

clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
1 ↗(On Diff #200536)

Let's stick with Dmitri's suggestion. He has much more experience with LLVM than I do :-)

ymandel marked 4 inline comments as done.May 22 2019, 8:13 AM
ymandel added inline comments.
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
46 ↗(On Diff #200536)

Ah, so all the options are bad. I can see why you had this design in transformers in the first place.

Heh. indeed.

I wonder if we should check the explanations are always set for rewrite rules inside the clang-tidy transformation?> Quoted Text

I would have thought so, but AFAIK, most folks who write one-off transformations use clang-tidy, rather than writing a standalone tool. They just ignore the diagnostics, i gather. Transformer may shift that somewhat if we improve the experience of writing a (throwaway) standalone tool, but for the time being I think we can't assume that.

ymandel updated this revision to Diff 200801.May 22 2019, 11:33 AM
ymandel marked an inline comment as done.

adjusted as required after syncing latest changes to Stencil and Transformer.

Herald added a reviewer: jfb. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
ymandel updated this revision to Diff 200802.May 22 2019, 11:34 AM
ymandel removed a subscriber: dexonsmith.

Replace previous diff which had the wrong base.

This revision was automatically updated to reflect the committed changes.
ilya-biryukov added inline comments.May 23 2019, 1:41 AM
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
46 ↗(On Diff #200536)

We should focus on minimizing maintenance cost for long-term fixes rather than one-off transformations. The cost of passing an empty string to a required explanation field for one-off transformations is rather small and falls into the hands of a person writing the check, the cost of constantly finding and fixing the long-lived upstream checks that don't have explanation (leading to bad user experience) is high and will probably fall into the hands of clang-tidy maintainers in addition to people writing the checks.

FWIW, having a new API of top of plain transformers should be better than clang-tidy for those writing one-off transformations in the long run. I don't think clang-tidy was ever designed to cover to cover those use-cases, even though today it seems to be the fastest way to do those.

ymandel marked 2 inline comments as done.May 23 2019, 5:22 AM
ymandel added inline comments.
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
46 ↗(On Diff #200536)

Agreed on all points. I'll send a followup that enforces that constraint.