This is an archive of the discontinued LLVM Phabricator instance.

[libTooling] Generalize string explanation as templated metadata
ClosedPublic

Authored by li.zhe.hua on Feb 22 2022, 2:36 PM.

Details

Summary

Change RewriteRule from holding an Explanation to being able to generate
arbitrary metadata. Where TransformerClangTidyCheck was interested in a string
description for the diagnostic, other tools may be interested in richer metadata
at a higher level of abstraction than at the edit level (which is currently
available as ASTEdit::Metadata).

Diff Detail

Event Timeline

li.zhe.hua created this revision.Feb 22 2022, 2:36 PM
li.zhe.hua requested review of this revision.Feb 22 2022, 2:36 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 22 2022, 2:36 PM
li.zhe.hua added inline comments.Feb 22 2022, 2:39 PM
clang/include/clang/Tooling/Transformer/RewriteRule.h
70

Placeholder name; suggestions welcome.

High level question: why use Any rather than templating?

clang/include/clang/Tooling/Transformer/RewriteRule.h
70

hmm. How about MetadataGenerator? Less general, but to the point.

Update alias

li.zhe.hua marked an inline comment as done.Feb 23 2022, 12:28 PM

High level question: why use Any rather than templating?

(Aside: Not sure how to "respond" to comments... just quote in a new comment?)

So, I had a version of this, but I ended up bifurcating the type into RewriteRule (with no metadata) and RewriteRuleWith<T> (with T metadata). I wasn't sure if the cost of having two types here was worth it. For interfaces that didn't care about the metadata generation, there wasn't an easy way to accept a RewriteRule-like parameter.

Finally, there was a question of migration. All uses of RewriteRule with TransformerClangTidyCheck would need to switch over to RewriteRuleWith<std::string>, and I wasn't sure how pervasive that might be. I'm not set on Any, but splitting the type does seem rather costly.

li.zhe.hua updated this revision to Diff 412245.Mar 1 2022, 2:01 PM

As discussed offline, switch to using a template instead of Any.

Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 2:01 PM
ymandel accepted this revision.Mar 2 2022, 8:04 AM

Looks really good!

clang/include/clang/Tooling/Transformer/RewriteRule.h
64–66

(and move down to after Generator def.)

286–291

Please comment.

329

Maybe add an adaptor for edit generator like we do in Stencil::cat. That will allow us to collapse the 6 overloads (2 * 3) into 2 (for makerule) + 3 (for the adaptor). Only saves 1 but I think it will clearer and more flexible for the future.

526–527

please update

clang/include/clang/Tooling/Transformer/Transformer.h
22

in RewriteRule.h, we use "detail". Maybe do the same here?

48

I'm afraid the use of the name Consumer (here and below) might be confused with the Consumer argument. What about just Impl or something similar?

129

why won't we have the same SFINAE here to ensure ConsumerFn is invocable?

This revision is now accepted and ready to land.Mar 2 2022, 8:04 AM
li.zhe.hua updated this revision to Diff 412746.Mar 3 2022, 9:38 AM
li.zhe.hua marked 7 inline comments as done.

Fixes from comments

clang/include/clang/Tooling/Transformer/RewriteRule.h
329

So, I ended up needing to add an overload for std::initializer_list as well, as it was somewhat common for people to do something like

makeRule(matchers(), {doThing1(), doThing2()})

which would have gone to the llvm::SmallVector<ASTEdit, 1> overload.

clang/include/clang/Tooling/Transformer/Transformer.h
48

I went with NoMetadataImpl (to be more descriptive than "plain") and WithMetadataImpl.

129

I can't figure out how to implement the SFINAE without instantiating Result<void> (which is invalid because T Metadata is illegal then). My current attempt is

template <
    typename T, typename ConsumerFn,
    std::enable_if_t<
        std::conjunction<
            std::negation<std::is_same<T, void>>,
            llvm::is_invocable<ConsumerFn, llvm::Expected<Result<T>>>>::value,
        int> = 0>
explicit Transformer(transformer::RewriteRuleWith<T> Rule,
                     ConsumerFn Consumer);

I suppose I could provide a specialization of Result<void> that is valid? I guess I'll go with that, and just document why it exists?

Fix use-after-move in asserts

li.zhe.hua retitled this revision from [libTooling] Generalize string explanation as Any metadata to [libTooling] Generalize string explanation as templated metadata.Mar 3 2022, 10:07 AM
asoffer added inline comments.Mar 3 2022, 11:06 AM
clang/include/clang/Tooling/Transformer/Transformer.h
110–124

Given that we're simply passing this off to the NoMetadataImpl which accepts a std::function, I don't see any reason to accept a generic parameter via sfinae. We could just accept the std::function and move it. Then the implicit conversion from whatever is passed in to std::function will do the heavy sfinae lifting.

129

Accepting the std:function directly should simplify this and avoid the need for the Result<void> specialization.

ymandel accepted this revision.Mar 3 2022, 11:10 AM
ymandel added inline comments.
clang/include/clang/Tooling/Transformer/Transformer.h
129

Seems fine, but I'll admit to being out of my depth. Seemed nice to have, but not necessary.

li.zhe.hua added inline comments.Mar 3 2022, 11:27 AM
clang/include/clang/Tooling/Transformer/Transformer.h
110–124

So, I ran into this during D119745, where one of the Windows CI builds doesn't have the fix for DR 2132 backported, so overloading on different std::function doesn't work. (To be clear, this using std::function and doing the whole overloading thing works fine in clang locally.)

Thoughts?

Try removing all the SFINAE

li.zhe.hua updated this revision to Diff 412823.Mar 3 2022, 1:25 PM
li.zhe.hua marked 2 inline comments as done.

Update comments

clang/include/clang/Tooling/Transformer/Transformer.h
110–124

Nevermind! I just tried and it works. I suspect this is because the non-template constructor and the template constructor are not ambiguous, as the non-template is tried first, and if it doesn't match, it goes to the template.

li.zhe.hua reopened this revision.Mar 21 2022, 1:04 PM
This revision is now accepted and ready to land.Mar 21 2022, 1:04 PM

Fix issue with "explicit specialization in non-namespace scope"

This revision was landed with ongoing or failed builds.Mar 21 2022, 1:39 PM
This revision was automatically updated to reflect the committed changes.