Add a `Ranged` wrapper struct to replace the rustc_scalar_range_valid attributes #103724

oli-obk · 2022-10-29T11:43:48Z

blocked on a decision on rust-lang/rfcs#3334

This PR removes the attribute and instead adds a type to the standard library that takes a field type and a const generic range to constraint that field type. All the previous logic that was required in the compiler to make the attribute sound has been removed and replaced by library functions and privacy rules.

It also eliminated all uses of cfg_attr(pointer_width=xxx), as we can now just use const eval and usize/isize to abstract over the pointer sized integer ranges.

The main limitation of the current implementation is that struct Foo<T, const C: T>(T); is not legal Rust yet. It is impossible as of yet to use generic parameters as the type of generic constants. Even with this limitation, I believe the compiler simplifications brought by this PR are important enough to warrant reconsidering the attribute stabilization RFC. Though I do not expect this type to get stabilized before const generics of generic types.

Zulip discussion on this topic

rustbot · 2022-10-29T11:43:55Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon.

Please see the contribution instructions for more information.

library/core/src/time.rs

library/core/src/num/ranged.rs

jhpratt

Quick look at the diff LGTM, aside from CI of course. Just the one nit that was already mentioned.

library/core/src/time.rs

oli-obk · 2022-11-02T20:59:20Z

@bors try @rust-timer queue

rust-timer · 2022-11-02T20:59:22Z

Awaiting bors try build completion.

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

bors · 2022-11-02T20:59:30Z

⌛ Trying commit 7fba3595ed0357e7c844ca106473c4cc968dc4d9 with merge 6a3b3432328dc85a19d780ed0e3b8be54628d292...

bors · 2022-11-02T23:18:21Z

☀️ Try build successful - checks-actions
Build commit: 6a3b3432328dc85a19d780ed0e3b8be54628d292 (6a3b3432328dc85a19d780ed0e3b8be54628d292)

rust-timer · 2022-11-02T23:18:23Z

Queued 6a3b3432328dc85a19d780ed0e3b8be54628d292 with parent edf0182, future comparison URL.

rust-timer · 2022-11-03T00:23:44Z

Finished benchmarking commit (6a3b3432328dc85a19d780ed0e3b8be54628d292): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Warning ⚠: The following benchmark(s) failed to build:

rustc
regression-31157

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.3%	[0.3%, 7.6%]	77
Regressions ❌ (secondary)	2.4%	[0.2%, 15.2%]	44
Improvements ✅ (primary)	-0.8%	[-2.5%, -0.2%]	115
Improvements ✅ (secondary)	-1.6%	[-5.3%, -0.2%]	91
All ❌✅ (primary)	0.4%	[-2.5%, 7.6%]	192

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.5%	[1.6%, 6.0%]	8
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.6%	[-5.7%, -0.5%]	11
Improvements ✅ (secondary)	-3.1%	[-6.5%, -1.0%]	18
All ❌✅ (primary)	-0.0%	[-5.7%, 6.0%]	19

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.5%	[0.8%, 8.3%]	48
Regressions ❌ (secondary)	5.7%	[1.9%, 14.7%]	12
Improvements ✅ (primary)	-1.9%	[-2.2%, -1.7%]	4
Improvements ✅ (secondary)	-5.2%	[-7.0%, -2.5%]	8
All ❌✅ (primary)	3.1%	[-2.2%, 8.3%]	52

oli-obk · 2022-11-03T09:44:49Z

@bors try @rust-timer queue

rust-timer · 2022-11-03T09:44:50Z

Awaiting bors try build completion.

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

bors · 2022-11-03T09:45:03Z

⌛ Trying commit e5c9675481e48da4a88e473f0d5c39a4d2f11fd6 with merge 8983d23afaf5483644a8983f75cb575b8e430d59...

bors · 2022-11-03T12:25:27Z

☀️ Try build successful - checks-actions
Build commit: 8983d23afaf5483644a8983f75cb575b8e430d59 (8983d23afaf5483644a8983f75cb575b8e430d59)

rust-timer · 2022-11-03T12:25:29Z

Queued 8983d23afaf5483644a8983f75cb575b8e430d59 with parent 432b1a4, future comparison URL.

rust-timer · 2022-11-03T15:12:11Z

Finished benchmarking commit (8983d23afaf5483644a8983f75cb575b8e430d59): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Warning ⚠: The following benchmark(s) failed to build:

regression-31157

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.2%, 2.2%]	99
Regressions ❌ (secondary)	1.4%	[0.2%, 6.1%]	39
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.3%	[-3.1%, -0.3%]	22
All ❌✅ (primary)	0.7%	[0.2%, 2.2%]	99

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.9%	[1.3%, 10.1%]	6
Regressions ❌ (secondary)	2.5%	[1.3%, 4.7%]	3
Improvements ✅ (primary)	-5.1%	[-6.0%, -3.8%]	3
Improvements ✅ (secondary)	-3.6%	[-4.2%, -2.2%]	6
All ❌✅ (primary)	0.9%	[-6.0%, 10.1%]	9

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.6%	[0.9%, 2.9%]	30
Regressions ❌ (secondary)	2.7%	[1.8%, 4.7%]	15
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.6%	[0.9%, 2.9%]	30

… attributes

bors · 2022-11-19T03:07:10Z

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

Various cleanups around scalar layout restrictions Pulled out of rust-lang#103724

scottmcm · 2022-12-10T08:40:59Z

Big 👍 from me! The RFC about attributes had to spend so much effort on describing parsing rules and such that just completely disappear if it can go into const generics as normal Rust values that already have all that defined, and it's more useful to boot to be able to make it generic!

oli-obk · 2022-12-10T09:16:30Z

Unfortunately I don't believe we can eliminate the perf regression unless we codegen repr(transparent) field accesses to the non-zst field as transmutes. LLVM seems to have a hard time figuring out all those field accesses in boxes

joshtriplett · 2023-01-19T10:38:51Z

@oli-obk You've convinced me. I'm going to close RFC 3334 in favor of this.

I do agree that we shouldn't stabilize this until we can write the range in terms of the T. And I do still think we should have a straightforward (unsafe) way to get a &mut T. But apart from that, I think we should ship this, and I'd be happy to see this PR merged as unstable if you believe we'll be able to switch it to use a range of T in the not too distant future.

Given a range based on T, how would you propose handling NonZeroI32, or something else with a niche in the middle?

oli-obk · 2023-01-19T10:53:54Z

Given a range based on T, how would you propose handling NonZeroI32, or something else with a niche in the middle?

The ranges given are wraparound, so there's no issue with 1..=-1 as a range, even if as an iterator that doesn't work.

And I do still think we should have a straightforward (unsafe) way to get a &mut T. But apart from that, I think we should ship this, and I'd be happy to see this PR merged as unstable if you believe we'll be able to switch it to use a range of T in the not too distant future.

The LLVM perf regression due to the extra field access is still fairly annoying and I am exploring not adding a level of indirection, but desugaring the existing unstable attributes to a (not surface-syntax representable) pattern type. Pattern types are essentially a pair of an existing type and a pattern that restricts said type to a subset. So for example u32 is 1.. or u32 matches 1...

If this experiment succeeds for fields of the NonZero and NonNull types, I will write up a proper proposal to add it to the surface syntax and replacing the existing struct NonZero and struct NonNull with type NonZeroU32 = u32 is 1.. and type NonNull<T> = (*const T) is {std::ptr::null<T>().wrapping_offset(1)}...

These pattern types have all the same advantages of this PR's wrapper struct, but avoids nesting things unnecessarily in structs.

mikeleany · 2023-01-19T18:08:58Z

library/core/src/num/ranged.rs

+    #[unstable(feature = "ranged_int", issue = "none")]
+    #[rustc_const_unstable(feature = "ranged_int", issue = "none")]
+    #[inline(always)]
+    pub const fn get(self) -> T {


Why do the get and new_unchecked implementations require T: CheckInRange when they don't seem to need it? And if it is needed, shouldn't the struct itself or at least the Deref implementation also have this requirement? Otherwise, you can easily bypass the requirement with transmute (for new_unchecked) and deref (for get), the latter of which would be the most concerning since it's safe and usually automatic.

mikeleany · 2023-01-19T18:09:14Z

library/core/src/num/ranged.rs

+/// A helper trait until we can declare `struct Ranged<T, const RANGE: RangeInclusive<T>>(T);`.
+/// This is not meant to ever be stabilized.
+#[unstable(feature = "ranged_int", issue = "none")]
+pub trait CheckInRange {


Shouldn't the CheckInRange trait be unsafe to implement since Ranged::new depends on its correct implementation?

clarfonthey · 2023-01-21T10:04:01Z

This is definitely something that can be fixed later, but one clear thing that will need to be worked around is how Ranged interplays with strict provenance. Right now, even though NonNull internally just assumes that provenance isn't actually stored inside the type, it doesn't guarantee this because it's opaque. So, this probably means that we'd want to ensure that Ranged for pointers is at least under a separate feature flag, or maybe RangedPtr can be a separate type.

oli-obk · 2023-02-02T19:06:18Z

Closing in favor of #107606

oli-obk added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Oct 29, 2022

rustbot assigned joshtriplett Oct 29, 2022

rustbot added 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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 29, 2022

This comment has been minimized.

Sign in to view

fbstj reviewed Oct 29, 2022

View reviewed changes

library/core/src/time.rs Outdated Show resolved Hide resolved

jruderman reviewed Oct 29, 2022

View reviewed changes

library/core/src/num/ranged.rs Show resolved Hide resolved

jhpratt reviewed Oct 30, 2022

View reviewed changes

library/core/src/time.rs Outdated Show resolved Hide resolved

oli-obk force-pushed the ranged_ints branch from 009f29d to 3a37ed1 Compare October 30, 2022 11:44

rustbot added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Oct 30, 2022

oli-obk force-pushed the ranged_ints branch from 3a37ed1 to b0a4772 Compare October 30, 2022 11:56

This comment has been minimized.

Sign in to view

oli-obk force-pushed the ranged_ints branch from b0a4772 to 408b862 Compare October 30, 2022 12:38

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 Nov 2, 2022

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 3, 2022

oli-obk mentioned this pull request Nov 3, 2022

Various cleanups around scalar layout restrictions #103917

Merged

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 3, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 3, 2022

Add a Ranged wrapper struct to replace the rustc_scalar_range_valid…

097d12e

… attributes

oli-obk force-pushed the ranged_ints branch from e5c9675 to 097d12e Compare November 3, 2022 15:31

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 27, 2022

Auto merge of rust-lang#103917 - oli-obk:layout_math, r=RalfJung,lcnr

df04d28

Various cleanups around scalar layout restrictions Pulled out of rust-lang#103724

oli-obk mentioned this pull request Nov 29, 2022

Niches rust-lang/rfcs#3334

Closed

mikeleany reviewed Jan 19, 2023

View reviewed changes

oli-obk mentioned this pull request Jan 25, 2023

Implement pattern types as the backing logic of rustc_scalar_valid_range attributes #107299

Closed

oli-obk closed this Feb 2, 2023

soooch mentioned this pull request Feb 15, 2023

Optimize the representation of NotNaN reem/rust-ordered-float#114

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a `Ranged` wrapper struct to replace the rustc_scalar_range_valid attributes #103724

Add a `Ranged` wrapper struct to replace the rustc_scalar_range_valid attributes #103724

oli-obk commented Oct 29, 2022 •

edited

Loading

rustbot commented Oct 29, 2022

This comment has been minimized.

jhpratt left a comment •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

oli-obk commented Nov 2, 2022

rust-timer commented Nov 2, 2022

bors commented Nov 2, 2022

bors commented Nov 2, 2022

rust-timer commented Nov 2, 2022

rust-timer commented Nov 3, 2022

oli-obk commented Nov 3, 2022

rust-timer commented Nov 3, 2022

bors commented Nov 3, 2022

bors commented Nov 3, 2022

rust-timer commented Nov 3, 2022

rust-timer commented Nov 3, 2022

bors commented Nov 19, 2022

scottmcm commented Dec 10, 2022

oli-obk commented Dec 10, 2022

joshtriplett commented Jan 19, 2023 •

edited

Loading

oli-obk commented Jan 19, 2023

mikeleany Jan 19, 2023

mikeleany Jan 19, 2023

clarfonthey commented Jan 21, 2023

oli-obk commented Feb 2, 2023

Add a Ranged wrapper struct to replace the rustc_scalar_range_valid attributes #103724

Add a Ranged wrapper struct to replace the rustc_scalar_range_valid attributes #103724

Conversation

oli-obk commented Oct 29, 2022 • edited Loading

rustbot commented Oct 29, 2022

This comment has been minimized.

jhpratt left a comment • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

oli-obk commented Nov 2, 2022

rust-timer commented Nov 2, 2022

bors commented Nov 2, 2022

bors commented Nov 2, 2022

rust-timer commented Nov 2, 2022

rust-timer commented Nov 3, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

oli-obk commented Nov 3, 2022

rust-timer commented Nov 3, 2022

bors commented Nov 3, 2022

bors commented Nov 3, 2022

rust-timer commented Nov 3, 2022

rust-timer commented Nov 3, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

bors commented Nov 19, 2022

scottmcm commented Dec 10, 2022

oli-obk commented Dec 10, 2022

joshtriplett commented Jan 19, 2023 • edited Loading

oli-obk commented Jan 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clarfonthey commented Jan 21, 2023

oli-obk commented Feb 2, 2023

Add a `Ranged` wrapper struct to replace the rustc_scalar_range_valid attributes #103724

Add a `Ranged` wrapper struct to replace the rustc_scalar_range_valid attributes #103724

oli-obk commented Oct 29, 2022 •

edited

Loading

jhpratt left a comment •

edited

Loading

joshtriplett commented Jan 19, 2023 •

edited

Loading