Implement pattern types as the backing logic of rustc_scalar_valid_range attributes #107299
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @BoxyUwU (or someone else) soon. Please see the contribution instructions for more information. |
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #107314) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
the
S-waiting-on-author
oli-obk
force-pushed
the
pattern_types
branch
from
039d03f
to
7b9502d
Compare
rustbot
added
A-rustdoc-json
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor Some changes occurred in src/librustdoc/clean/types.rs cc @camelid rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt
cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki This PR changes MIR cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
|
Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 |
This comment has been minimized.
This comment has been minimized.
bjorn3
reviewed
Jan 26, 2023
| @@ -9,12 +9,24 @@ pub const UNIX_EPOCH: SystemTime = SystemTime { t: Timespec::zero() }; | |||
| pub const TIMESPEC_MAX: libc::timespec = | |||
| libc::timespec { tv_sec: <libc::time_t>::MAX, tv_nsec: 1_000_000_000 - 1 }; | |||
|
|
|||
| #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] | |||
| #[derive(Copy, Clone, Eq, Ord, Hash)] | |||
There was a problem hiding this comment.
This will lose StructuralPartialEq for SystemTime, right?
There was a problem hiding this comment.
oops yea, I'll fix that.
|
Hmm... I cannot reproduce the CI failure locally, even with llvm built locally and with debug assertions on. Seems to be a LLVM 13 bug @bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 32d11e6 with merge bfa3ec2e98602555e614e97839ef4ac24b82bf84... |
|
💔 Test failed - checks-actions |
fbstj
reviewed
Jan 27, 2023
This comment has been minimized.
This comment has been minimized.
lcnr
removed
the
WG-trait-system-refactor
rustbot
added
A-query-system
This comment has been minimized.
This comment has been minimized.
oli-obk
force-pushed
the
pattern_types
branch
from
be24398
to
a1c0a3e
Compare
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
|
⌛ Trying commit a1c0a3e with merge eb5fc32d562d9e47e2e39272f7361bce86b65f51... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☔ The latest upstream changes (presumably #101692) made this pull request unmergeable. Please resolve the merge conflicts. |
| @@ -565,6 +565,12 @@ pub enum Type { | |||
| type_: Box<Type>, | |||
| len: String, | |||
| }, | |||
| /// u32 is 0..=100 | |||
| Pat { | |||
There was a problem hiding this comment.
Could you bump the FORMAT_VERSION constant in the top of this file please.
Also, CI will fail unless you add a new case to
rust/src/tools/jsondoclint/src/validator.rs
Lines 243 to 245 in 226b249
All it needs to do is recurse into the inner type.
RalfJung
reviewed
Jan 30, 2023
| #[inline(always)] | ||
| fn strip_pat_ty(mut self) -> Self { | ||
| let ty::Pat(raw_ptr_ty, _) = *self.layout.ty.kind() else { | ||
| bug!("NonNull must contain a pattern type, but had {}", self.layout.ty) |
There was a problem hiding this comment.
These assertions are confusing. The method name and doc comment to not talk about NonNull in any way.
RalfJung
reviewed
Jan 30, 2023
| @@ -83,6 +83,9 @@ pub(crate) fn eval_nullary_intrinsic<'tcx>( | |||
| ty::Alias(..) | ty::Param(_) | ty::Placeholder(_) | ty::Infer(_) => { | |||
| throw_inval!(TooGeneric) | |||
| } | |||
| ty::Pat(..) => { | |||
| unimplemented!("pattern types need to calculate pattern from their pattern") | |||
There was a problem hiding this comment.
I can't parse this panic message... too many patterns?^^
RalfJung
reviewed
Jan 30, 2023
Comment on lines
+501
to
+512
| // Then check the extra pattern restrictions. | ||
| let scalar = self.read_immediate(&value, "initialized scalar value")?; | ||
| match (*scalar, value.layout.abi) { | ||
| (Immediate::Scalar(scalar), Abi::Scalar(s)) | ||
| | (Immediate::ScalarPair(scalar, _), Abi::ScalarPair(s, _)) => { | ||
| self.visit_scalar(scalar, s)? | ||
| } | ||
| other => span_bug!( | ||
| self.ecx.cur_span(), | ||
| "invalid abi {other:?} for pattern type {ty:?}" | ||
| ), | ||
| } |
There was a problem hiding this comment.
Isn't this duplicating the scalar range check after this match?
I assume for now patterns are restricted to things that can be actually represented in the Scalar ABI? If 0 | 5 | 10 is accepted as a pattern then we should properly check it here...
Closed
|
Closing in favor of #107606 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alternative to #103724
Some explanation of the three alternatives can be found on my blog
r? @BoxyUwU
cc @joshtriplett
This PR causes fields of types that use
rustc_scalar_valid_rangeto be of a newTyKind: thePatkind which at present has a nested type and a u128 range of valid values. This represents the status quo exactly, but before I rip out the old attribute checks and implement them on top of pattern types, I wanted to open this PR for discussion.For now I added an inherently unsafe way to construct these types: you cast an arbitrary other type to them. So initializing
NonNullworks similar toNonNull { pointer: some_raw_ptr as _ }. The underscore figures our the right type, as right now there is no user visible syntax for these types.As a next step I would like to remove the cast and instead implement it so that initialization of
NonNullworks likewhere we teach pattern matching that the type of
pointeris one of the new pattern types. The type ofvalueis(*const T) is 1..At this stage we would not support
because that requires reasoning across arms, which requires more work than just doing some local reasoning. The compiler already has this information for exhaustiveness checking, so we're not inventing anything new here, but we need to still fetch the information from exhaustiveness checking, so it seems best to start simple.