Simplify Encodable derives

[panstromek]

View all comments

Few cleanups I found when investigating encoding code.

• avoid generating match for structs
• remove second match for enums if it is empty
• clarify why is the enum matched twice
• cleanup dead code and style

This is not super impactful but it makes the expanded code simpler, which is nice when you look at it for debugging purposes.
Best reviewed commit by commit

r? nnethercote
(I believe you've looked into this code quite a bit before)

[panstromek]

I don't expect much, maybe some little changes in bootstrap.

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: ba8d592 (ba8d592b3bf183de5683c92c8c4a2d3b560b6a0e)
Base parent: 110d7e5 (110d7e55dbd37e65cdc3321a204a9911af35f533)

[panstromek]

@rust-timer build ba8d592

[rust-timer]

Finished benchmarking commit (ba8d592): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary -0.8%, secondary -0.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.6%, 0.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.2%	[-2.2%, -2.2%]	1
Improvements ✅ (secondary)	-0.8%	[-0.8%, -0.7%]	2
All ❌✅ (primary)	-0.8%	[-2.2%, 0.6%]	2

Cycles

Results (secondary 3.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	6.1%	[1.6%, 10.0%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.7%	[-4.5%, -2.9%]	2
All ❌✅ (primary)	-	-	0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 462.121s -> 459.222s (-0.63%)
Artifact size: 396.97 MiB -> 398.98 MiB (0.51%)

[Zalathar]

It would be helpful to add and bless a tests/ui-fulldeps test as the first commit, and then re-bless it to show the effects of each change.

Here's an example of what that might look like:

//@ edition: 2024
//@ check-pass
//@ compile-flags: -Zunpretty=expanded

#![crate_type = "rlib"]
#![feature(rustc_private)]

extern crate rustc_macros;
extern crate rustc_serialize;
extern crate rustc_span;

use rustc_macros::Encodable;

#[derive(Encodable)]
struct UnitStruct;

#[derive(Encodable)]
struct EmptyStruct {}

#[derive(Encodable)]
enum EmptyEnum {}

#[derive(Encodable)]
enum SingleFieldlessEnum {
    A,
}

#[derive(Encodable)]
enum SingleEnum {
    A(u32),
}

#[derive(Encodable)]
enum FieldlessEnum {
    A,
    B,
}

#[derive(Encodable)]
enum PartlyFieldlessEnum {
    A,
    B(u32),
}

[panstromek]

It would be helpful to add and bless a tests/ui-fulldeps test

That's a good idea, I will do that. I was thinking of posting the diff here to make the changes more visible for the review, but this is much better.

❤️ Thanks a lot for writing down the example, it would probably take me a lot longer to figure out myself.

[Zalathar]

Yeah, I figured it would be a bit mean to just say “please write a ui-fulldeps test”, and from past experience I already had a pretty good idea of what it needs to look like. 😅

[rustbot]

This PR was rebased onto a different main 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.

[panstromek]

I added the test based on your comment. Not sure if you checked it before posting, but it was actually 100% right, I didn't need to change or debug anything about it 👍

I re-blessed it with each edit to make it clear what it does. The test also made it clear that I didn't handle all unit-like types so I added a case to cover that in the last commit.

[nnethercote]

Looks good, just one nit.

View changes since this review

[panstromek]

@rustbot ready

[rustbot]

Error: shortcut handler unexpectedly failed in this comment: failed to add labels

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip.