-
Notifications
You must be signed in to change notification settings - Fork 13.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Start using pattern types in libcore #136006
base: master
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
e3e2cbd
to
ff4d569
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well r=me when/if it does get unblocked.
This comment has been minimized.
This comment has been minimized.
library/core/src/num/niche_types.rs
Outdated
#[repr(transparent)] | ||
$(#[$m])* | ||
#[cfg(not(bootstrap))] | ||
$vis struct $name(pattern_type!($int is $low..=$high)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure: at least for the NonZeroBlahInner
ones, it'd be nicer if these could just be type $name = pattern_type!($int is $low..=$high);
, without the extra wrapper. Is that feasible, or are the trait implementations too far off?
(Relatedly, I'd love to be able to just derive traits on these again, particularly to not have to manually StructuralPartialEq
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea... I wanna get there, but directly using pattern types is not a great experience at present
ff4d569
to
0409353
Compare
This comment has been minimized.
This comment has been minimized.
0409353
to
4db40b9
Compare
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
Re rust-analyzer blocker, as it turns out to implement pattern types (to a degree that would unblock this) it comes down to the same architecture changes required as for rust-lang/rust-analyzer#7434 (which I am currently looking into), so that will take a bit. |
This comment has been minimized.
This comment has been minimized.
4db40b9
to
0afd16b
Compare
This comment has been minimized.
This comment has been minimized.
0afd16b
to
ed90eb8
Compare
This comment has been minimized.
This comment has been minimized.
ed90eb8
to
9514a86
Compare
This comment has been minimized.
This comment has been minimized.
9514a86
to
9774687
Compare
This comment has been minimized.
This comment has been minimized.
Some changes occurred to the CTFE machinery cc @rust-lang/wg-const-eval Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
@Veykril I think for the most part you could just treat pattern types opaquely and ignore the pattern's details. Casts to the base type don't require looking at the pattern and instantiating pattern types from literals could just be ignored by r-a and left to rustc. I don't think you actually need the layout of pattern types for anything else |
This comment has been minimized.
This comment has been minimized.
308a384
to
3e87ee2
Compare
This comment has been minimized.
This comment has been minimized.
3e87ee2
to
f1070f2
Compare
impl $name { | ||
#[inline] | ||
pub const fn new(val: $int) -> Option<Self> { | ||
if (val as $uint) >= ($low as $uint) && (val as $uint) <= ($high as $uint) { | ||
#[allow(non_contiguous_range_endpoints)] | ||
if let $pat = val { | ||
// SAFETY: just checked the inclusive range |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// SAFETY: just checked the inclusive range | |
// SAFETY: just checked that the value matches the pattern |
This comment has been minimized.
This comment has been minimized.
But isn't the point of them that they have niches? If we treat them opaquely we won't see the niches of the type for layout calculation. Either way I should be unblocked regarding rust-lang/rust-analyzer#7434 so I can start working on that again starting next week. Though if you want to get this merged and r-a is the remaining blocking reason we can work around it by special casing the relevant std/core type(s) if needed I think |
Ah then the sizes shown by ra would be wrong. Hmm I've never seem the sizes, didn't think about that |
f1070f2
to
bbf47b3
Compare
This comment has been minimized.
This comment has been minimized.
bbf47b3
to
c5481a8
Compare
This comment has been minimized.
This comment has been minimized.
c5481a8
to
6f2e921
Compare
This comment has been minimized.
This comment has been minimized.
6f2e921
to
e65f76e
Compare
The Miri subtree was changed cc @rust-lang/miri |
@@ -86,6 +86,9 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { | |||
let (_, field) = layout.non_1zst_field(self).unwrap(); | |||
self.unfold_transparent(field, may_unfold) | |||
} | |||
ty::Pat(base, _) => self.layout_of(*base).expect( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should only be changed here if we also change the docs to guarantee that a pattern type is ABI-compatible with its inner type. And then the ABI compatibility tests should be extended to cover that (in the rustc test suite and the Miri test suite).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, considering it's the only way to guarantee that NonZeroU8
is still compatible with u8
, I think the answer is yes, and that we already have at least tests for that. I'll add additional tests for bare pattern types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make a note on the tracking issue so that before stabilization, we decide whether this is true for all pattern types or only for some.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We unconditionally allow transmuting from pattern types to their base type. For any pattern. I don't see a design that could work differently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"A can be transmuted to B" does not imply "A and B are ABI-compatible". For instance, i32
and u32
are not ABI-compatible. The code you changed here is about Miri ensuring ABI compatibility of caller and callee.
@@ -70,6 +70,9 @@ pub(crate) fn eval_nullary_intrinsic<'tcx>( | |||
ty::Pat(_, pat) => match **pat { | |||
ty::PatternKind::Range { .. } => ConstValue::from_target_usize(0u64, &tcx), | |||
// Future pattern kinds may have more variants | |||
// FIXME(pattern_types): make this report the number of distinct variants used in the | |||
// or pattern in case the base type is an enum. | |||
ty::PatternKind::Or(_) => ConstValue::from_target_usize(0_u64, &tcx), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't there be a similar FIXME somewhere in codegen?
☔ The latest upstream changes (presumably #138464) made this pull request unmergeable. Please resolve the merge conflicts. |
cc #135996
blocked on rust-analyzer getting support for computing the right layout for pattern types
cc @Veykril no rush here, as long as we can't replace
NonNull
, there's no point in doing this change just yet