Use declarative macro for #[derive(TryFromU32)]

[fee1-dead]

cc @Zalathar

[rustbot]

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

Some changes occurred in coverage instrumentation.

cc @Zalathar

[jieyouxu]

r? @Zalathar

[Zalathar]

Is there some reason to do this? I don’t understand the motivation for wanting to make these enums considerably harder to maintain.

[fee1-dead]

procedural macros hurt build times because it feeds tokens to a parser for each procedural macro called for each type. There's no point in using a procedural macro for something this trivial.

There are alternatives to this, if you don't like maintaining two lists (is that really hard? just two lists, and you get compiler errors if one doesn't match the other).

One is to wrap the whole thing in the declarative macro, doable but I don't like how it looks.

The other is to use the upcoming macro-attr to write attributes as declarative macros. That will have to wait until beta bumps and it's an item in my todo list.

On the other hand, one additional procedural macro in rustc_macro, with ui-fulldeps tests seem like a lot of weight.

[hanna-kruppe]

I've written and used macros like this before and as I've kept working on and around the types they're applied to, I've noticed various small papercuts that make it mildly more annoying to work with than derive(Foo). For example, rust-analyzer's autocomplete tends to be less helpful when editing the tokens passed to the macro, rustfmt handling is dependent on the input token tree parsing as expression, and grepping for mentions or impls of a trait rarely catches the use of macros (you can make impl TheTrait for a required part of the input, but in that case rustfmt is less useful).

I have less hands-on experience with attribute macros but they also have some papercuts (e.g., rust-analyzer problems like rust-lang/rust-analyzer#19944). They're also conceptually the wrong tool here: derive is exactly for adding impls, attr macros are the bigger gun for when you need to transform and re-emit the input tokens.

I'd suggest waiting for #145208 to be merged and reach beta, then it can stay a derive with the same use-site ergonomics as always. This is a perfect use case for it, and this change doesn't seem particularly time critical. There shouldn't be any build time benefits since cg_llvm still depends on rustc_macros and in this case the derives neither process a lot of input tokens nor generate enormous amounts of code.

[Zalathar]

Normally I'm very much in favour of removing needlessly-complex macros from rustc; we have far too many of them already.

The fact that I was willing to add a proc-macro (with tests!) in this case should indicate how strongly I believe that it really is justified. Not having to redundantly list all variants of an FFI enum is a really big deal, as is being able to easily add #[derive(TryFromU32)] to existing or new enums.

When #145208 is available in stage0, I think it might we worth trying to use that instead.

But I don't see any benefit in removing the existing derive at this time. It meaningfully increases the difficulty of maintaining FFI bindings to LLVM, in exchange for shuffling macro complexity around without actually decreasing it.

[fee1-dead]

cool, #145208 is a thing. Marking this as blocked until that one lands.

[fee1-dead]

@rustbot ready

[Zalathar]

I have some style nits, but otherwise LGTM.

[bors]

☔ The latest upstream changes (presumably #147384) made this pull request unmergeable. Please resolve the merge conflicts.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[fee1-dead]

@rustbot ready

[Zalathar]

Thanks.

@bors r+ rollup

[bors]

📌 Commit 696b6ac has been approved by Zalathar

It is now in the queue for this repository.