Generate `valid_range`s for enums sign-agnostically by scottmcm · Pull Request #159509 · rust-lang/rust

scottmcm · 2026-07-18T10:45:53Z

Prompted by #159438 (comment), this reworks the generation of the tag layout information for enums.

Having gone down the wrong path at first here, I think the core trouble this is having is that the current calculation is trying to serve two incompatible desires:

When picking the value: Primitive, we need to respect the signedness of the original discriminant numbers as typed in Rust.
When picking the valid_range: WrappingRange, we want to ignore the signedness (and size!) of the discriminants and look only at the bit values (and size) of the tags themselves.

This thus splits the handling in layout_of_enum into those two parts, handled differently.

When looking at discriminants (to eventually call discr_range_of_repr), we obey the signedness of the inputs to find the smallest negative discriminant and largest positive discriminant -- notably without any wraparound logic.
Then after having used the discriminants to pick a tag size, then we run a wrapping-aware check to pick the best WrappingRange to represent those tags (which can be a different range than we'd have picked on the discriminants, especially for repr(Rust) where the discriminants are isize).

With apologies for sending this when you'd said you'd look at it, but I got inspired 😅
r? @oli-obk

Fixes #159438

I have a bunch of commits in here; it's probably best reviewed one at a time.

Just moving WrappingRange to its own file, since the lib.rs is over 2000 lines already so if I was going to add stuff I figured some reorganizing could help.
Adding a new constructor to WrappingRange that takes an iterator of values and returns a range containing those values. Dealing just in u128 + Size, like WrappingRange does in other places, it doesn't even have signedness information so structurally is prevented from having any "why are repr(u8) and `repr(i8) picking different ranges?" issues. I found it helpful to have this split out from the layout code, for example that it let me put some examples on the doc-comment. Nothing actually uses the new function in this commit though.
Stop giving layout discriminants as i128 since https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/util/struct.Discr.html#structfield.val points out that they're generated as u128 in the same way that WrappingRange uses already -- even with the field doc that -1_i8 is generated as 255_u128, so the as i128ing was probably just making everything more confusing here than it needed to be.
Adding some tests looking at the generated Primitive and valid_range, along with rustc support for #[rustc_dump_layout(largest_niche)] to help have these tests include specific ERROR annotations rather than just the stderr. The tests pass at this point, so the next commit can update them to show the change.
This one actually updates layout code, including using the WrappingRange constructor added earlier.

rustbot · 2026-07-18T10:45:56Z

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

scottmcm · 2026-07-18T21:44:05Z

Ok, the first version I'd misunderstood how we need to pick tag signedness, so redid that and added a bunch of new tests because there were disturbingly few failures when I'd probably broken a ton of stuff (albeit subtly).

OP updated as well to describe the fixed approach.
@rustbot ready

scottmcm · 2026-07-18T23:14:01Z

Not expecting a perf win (I think this does a bit more work now), but to make sure it's not horrible:
@bors try @rust-timer queue

Generate `valid_range`s for enums sign-agnostically

rust-bors · 2026-07-19T01:28:01Z

☀️ Try build successful (CI)
Build commit: 8d18ada (8d18adad4abcffe08e2c9c56e062fe6826106354)
Base parent: eff8269 (eff8269f797067c30555e77f160ec84c0ed15cd9)

rust-timer · 2026-07-19T02:08:20Z

Finished benchmarking commit (8d18ada): 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)

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

Cycles

Results (primary -2.4%)

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)	-	-	0
Improvements ✅ (primary)	-2.4%	[-2.4%, -2.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.4%	[-2.4%, -2.4%]	1

Binary size

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

Bootstrap: 489.126s -> 483.723s (-1.10%)
Artifact size: 389.52 MiB -> 389.47 MiB (-0.01%)

rustbot · 2026-07-19T20:12:56Z

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.

oli-obk

just some nits, the new enum range logic is much clearer now!

View changes since this review

oli-obk · 2026-07-21T08:34:53Z

+    /// // Both `100..=228` and `..=228 | 100..` are the same size, but we pick the one without zero.
+    /// let range = WrappingRange::smallest_range_containing([100, 228], Size::from_bytes(1));
+    /// assert_eq!(range.unwrap(), WrappingRange { start: 100, end: 228 });
+    /// // All 4 possibilities give a length of 193. We pick the one that doesn't contain zero.


There are several not containing it, we pick the one that starts closest to zero

I elaborated this comment; there's only one that doesn't include zero.

(Said otherwise, we have to include all the gaps except one, and there's obviously at most one gap containing zero, so the one range that doesn't include zero is the range that picks that gap.)

theemathas · 2026-07-25T07:29:33Z

Does this affect #159433?

scottmcm · 2026-07-25T23:30:54Z

@theemathas Not at all, because those types already have the correct valid range: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=555d30b3989104ea121c0d35d216a9b1

That bug is just CheckEnums being wrong, not anything to fix in layout.

oli-obk · 2026-07-29T11:11:32Z

@bors r+

rust-bors · 2026-07-29T11:11:35Z

📌 Commit 9cbba6e has been approved by oli-obk

It is now in the queue for this repository.

…uwer Rollup of 11 pull requests Successful merges: - #158460 (Remove llvm_enzyme feature outside of bootstrap) - #159509 (Generate `valid_range`s for enums sign-agnostically) - #159632 (CFI: Add support for the adt_const_params feature) - #159671 (Add semver check test command for checking API compatibility of stdlib) - #157058 (Rustdoc label badge for notable traits) - #159717 (Add `-Zimplicit-sysroot-deps`) - #159850 (Add regression test for closure in array-length const generic) - #159994 (Show jobs where a given test was executed in `test-dashboard`) - #160110 (convert rustc_hir::Target inherent methods to From impls) - #160123 (add additional license option for third-party dependencies) - #160131 (bootstrap: remove temporary bors email lookup)

@oli-obk

Rollup merge of #159509 - scottmcm:redo-valid-range, r=oli-obk Generate `valid_range`s for enums sign-agnostically Prompted by #159438 (comment), this reworks the generation of the tag layout information for enums. Having gone down the wrong path at first here, I think the core trouble this is having is that the current calculation is trying to serve two incompatible desires: - When picking the `value: Primitive`, we need to respect the signedness of the original *discriminant* numbers as typed in Rust. - When picking the `valid_range: WrappingRange`, we want to *ignore* the signedness (and size!) of the discriminants and look only at the bit values (and size) of the *tags* themselves. This thus splits the handling in `layout_of_enum` into those two parts, handled differently. - When looking at *discriminants* (to eventually call [`discr_range_of_repr`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/layout/trait.IntegerExt.html#tymethod.discr_range_of_repr)), we obey the signedness of the inputs to find the smallest negative discriminant and largest positive discriminant -- notably *without* any wraparound logic. - Then after having used the discriminants to pick a *tag* size, then we run a wrapping-aware check to pick the best `WrappingRange` to represent those tags (which can be a different range than we'd have picked on the discriminants, especially for `repr(Rust)` where the discriminants are `isize`). With apologies for sending this when you'd said you'd look at it, but I got inspired 😅 r? @oli-obk Fixes #159438 --- I have a bunch of commits in here; it's probably best reviewed one at a time. 1. Just moving `WrappingRange` to its own file, since the `lib.rs` is over 2000 lines already so if I was going to add stuff I figured some reorganizing could help. 2. Adding a new constructor to `WrappingRange` that takes an iterator of values and returns a range containing those values. Dealing just in `u128` + `Size`, like `WrappingRange` does in other places, it doesn't even *have* signedness information so structurally is prevented from having any "why are `repr(u8)` and `repr(i8) picking different ranges?" issues. I found it helpful to have this split out from the layout code, for example that it let me put some examples on the doc-comment. Nothing actually *uses* the new function in this commit though. 3. Stop giving layout discriminants as `i128` since <https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/util/struct.Discr.html#structfield.val> points out that they're generated as `u128` in the same way that `WrappingRange` uses already -- even with the field doc that `-1_i8` is generated as `255_u128`, so the `as i128`ing was probably just making everything more confusing here than it needed to be. 4. Adding some tests looking at the generated `Primitive` and `valid_range`, along with rustc support for `#[rustc_dump_layout(largest_niche)]` to help have these tests include specific ERROR annotations rather than just the stderr. The tests pass at this point, so the next commit can update them to show the change. 5. This one actually updates layout code, including using the `WrappingRange` constructor added earlier.

rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 18, 2026

rustbot assigned oli-obk Jul 18, 2026

This comment has been minimized.

Sign in to view

scottmcm commented Jul 18, 2026

View reviewed changes

Comment thread tests/ui/layout/hexagon-enum.stderr Outdated

scottmcm force-pushed the redo-valid-range branch 2 times, most recently from 89681fc to fc374f1 Compare July 18, 2026 11:04

This comment was marked as outdated.

Sign in to view

scottmcm marked this pull request as draft July 18, 2026 11:14

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 18, 2026

This comment has been minimized.

Sign in to view

scottmcm force-pushed the redo-valid-range branch from fc374f1 to f8b4450 Compare July 18, 2026 20:08

scottmcm marked this pull request as ready for review July 18, 2026 21:44

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 18, 2026

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 18, 2026

rust-bors Bot pushed a commit that referenced this pull request Jul 18, 2026

Auto merge of #159509 - scottmcm:redo-valid-range, r=<try>

8d18ada

Generate `valid_range`s for enums sign-agnostically

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 19, 2026

This comment has been minimized.

Sign in to view

rustc_abi: move WrappingRange to its own file

1e19b0b

scottmcm force-pushed the redo-valid-range branch from f8b4450 to c1e94d9 Compare July 19, 2026 20:12

oli-obk requested changes Jul 21, 2026

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 21, 2026

scottmcm commented Jul 21, 2026

View reviewed changes

Comment thread compiler/rustc_abi/src/layout.rs Outdated

scottmcm added 4 commits July 21, 2026 20:40

WrappingRange: add a constructor from the desired covered values

e278105

Stop passing the discriminant as i128 when Discr makes it as u128

e5f0680

Add some enum niche choice tests

934f93b

Fix signedness handling when picking enum ranges

9cbba6e

scottmcm force-pushed the redo-valid-range branch from c1e94d9 to 9cbba6e Compare July 22, 2026 03:44

scottmcm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 22, 2026

rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 29, 2026

JonathanBrouwer mentioned this pull request Jul 29, 2026

Rollup of 11 pull requests #160146

Merged

rust-bors Bot merged commit bb97a9c into rust-lang:main Jul 29, 2026
13 checks passed

rustbot added this to the 1.99.0 milestone Jul 29, 2026

scottmcm deleted the redo-valid-range branch July 30, 2026 05:15

scottmcm mentioned this pull request Jul 30, 2026

Rework smallest_range_containing to handle duplicates #160198

Open

Uh oh!

Uh oh!

Conversation

scottmcm commented Jul 18, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jul 18, 2026

Uh oh!

This comment has been minimized.

Uh oh!

This comment was marked as outdated.

This comment has been minimized.

scottmcm commented Jul 18, 2026

Uh oh!

scottmcm commented Jul 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

rust-bors Bot commented Jul 19, 2026

Uh oh!

This comment has been minimized.

rust-timer commented Jul 19, 2026

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

This comment has been minimized.

rustbot commented Jul 19, 2026

Uh oh!

oli-obk left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oli-obk Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

scottmcm Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theemathas commented Jul 25, 2026

Uh oh!

scottmcm commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oli-obk commented Jul 29, 2026

Uh oh!

rust-bors Bot commented Jul 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

scottmcm commented Jul 18, 2026 •

edited by rustbot

Loading

scottmcm commented Jul 18, 2026 •

edited

Loading

oli-obk left a comment •

edited by rustbot

Loading

scottmcm Jul 22, 2026 •

edited

Loading

scottmcm commented Jul 25, 2026 •

edited

Loading