implement or-patterns for pattern types #139909
Conversation
rustbot
added
S-waiting-on-review
|
changes to the core type system Some changes occurred to the CTFE machinery Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri changes to the core type system Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt HIR ty lowering was modified cc @fmease |
| // FIXME(pattern_types): check that the value is covered by one of the variants. | ||
| // The layout may pessimistically cover actually illegal ranges. | ||
| ty::PatternKind::Or(_patterns) => {} |
There was a problem hiding this comment.
Is it clear how one would do this? Do we have to try one after the other? That seems very tricky as it means we have to validate the value "speculatively" where we ignore errors, until we know that all paths error, then we report one of them... as long as it's just integers this is fairly harmless, but something like Ok(true) | Err(5) | Err(7) should ideally be checked by "read discriminant, then proceed with the pattern restricted to what we see there" -- basically, compiling the pattern to a discrimination tree similar to what happens during match lowering, and executing that here.
There was a problem hiding this comment.
I think we'll have to do something similar to whatever we'll want to do if pattern types can have user defined trait impls. We'll need to detect overlap and possibly exhaustiveness some day. I leave this problem to future me, because it's not necessary for the MVP of replacing the rustc_scalar_layout attributes
There was a problem hiding this comment.
Checking the validity of NonZeroI32 is definitely necessary, Miri must be able to detect that UB.
Checking the validity of general or patterns can be deferred, though I'd prefer if we did not use any or patterns in std that Miri can't support -- at least not for code that can be reached on stable.
There was a problem hiding this comment.
This is checked via the layout. That's what I was trying to convey with the comment.
There was a problem hiding this comment.
Okay so if the "or" pattern happens to fall into the fragment layout can handle, then Miri can check validity. If not, we don't actually use this information for codegen, so it's okay for Miri to not check validity. I take it patterns never reduce the size of the type, only the set of allowed values?
| ast::TyPatKind::Or(ref variants) => { | ||
| let mut first = true; | ||
| let mut s = String::new(); | ||
| for variant in variants { | ||
| if first { | ||
| first = false | ||
| } else { | ||
| s.push_str(" | "); | ||
| } | ||
| s.push_str(&variant.rewrite_result(context, shape)?); | ||
| } | ||
| Ok(s) | ||
| } |
There was a problem hiding this comment.
Do we have a syntax for this? looking at the UI tests I'm just seeing macro calls.
Also, I'm not sure the formatting implementation is correct. The TyPatKind::Or could exceed the max_width without wrapping if it's very long. I feel like we should implement this similarly to PatKind::Or.
If possible, can you add a test case with the relevant #![feature()] annotation?
| // FIXME(pattern_types): check that the value is covered by one of the variants. | ||
| // The layout may pessimistically cover actually illegal ranges. |
There was a problem hiding this comment.
| // FIXME(pattern_types): check that the value is covered by one of the variants. | |
| // The layout may pessimistically cover actually illegal ranges. | |
| // FIXME(pattern_types): check that the value is covered by one of the variants. | |
| // For now, we rely on layout computation setting the scalar's `valid_range` to | |
| // match the pattern. However, this cannot always work; the layout may | |
| // pessimistically cover actually illegal ranges and Miri would miss that UB. | |
| // The consolation here is that codegen also will miss that UB, so at least | |
| // we won't see optimizations actually breaking such programs. |
These are necessary to represent
NonZeroI32, as the range for that is..0 | 1... Therustc_scalar_layout_range_*attributes avoided this by just implementing wraparound and having a single1..=-1range effectively. See https://rust-lang.zulipchat.com/#narrow/channel/481660-t-lang.2Fpattern-types/topic/.60or.20pattern.60.20representation.20in.20type.20system/with/504217694 for some background discussionr? @BoxyUwU