-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Tracking Issue for Never Patterns (never_patterns
)
#118155
Comments
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…take-3, r=<try> Don't warn an empty pattern unreachable if we're not sure the data is valid Exhaustiveness checking used to be naive about the possibility of a place containing invalid data. This could cause it to emit an "unreachable pattern" lint on an arm that was in fact reachable, as in rust-lang#117119. This PR fixes that. We now track whether a place that is matched on may hold invalid data. This also forced me to be extra precise about how exhaustiveness manages empty types. Note that this now errs in the opposite direction: the following arm is truly unreachable but not linted as such. I'd rather not recommend writing a `match ... {}` that has the implicit side-effect of loading the value. [Never patterns](rust-lang#118155) will solve this cleanly. ```rust match union.value { _x => unreachable!(), } ``` I recommend reviewing commit by commit. I went all-in on the test suite because this went through a lot of iterations and I kept everything. The bit I'm least confident in is `is_known_valid_scrutinee` in `check_match.rs`. Fixes rust-lang#117119.
…r=compiler-errors Add `never_patterns` feature gate This PR adds the feature gate and most basic parsing for the experimental `never_patterns` feature. See the tracking issue (rust-lang#118155) for details on the experiment. `@scottmcm` has agreed to be my lang-team liaison for this experiment.
…r=compiler-errors Add `never_patterns` feature gate This PR adds the feature gate and most basic parsing for the experimental `never_patterns` feature. See the tracking issue (rust-lang#118155) for details on the experiment. ``@scottmcm`` has agreed to be my lang-team liaison for this experiment.
…r=compiler-errors Add `never_patterns` feature gate This PR adds the feature gate and most basic parsing for the experimental `never_patterns` feature. See the tracking issue (rust-lang#118155) for details on the experiment. `@scottmcm` has agreed to be my lang-team liaison for this experiment.
Rollup merge of rust-lang#118157 - Nadrieril:never_pat-feature-gate, r=compiler-errors Add `never_patterns` feature gate This PR adds the feature gate and most basic parsing for the experimental `never_patterns` feature. See the tracking issue (rust-lang#118155) for details on the experiment. `@scottmcm` has agreed to be my lang-team liaison for this experiment.
…r-errors Add `never_patterns` feature gate This PR adds the feature gate and most basic parsing for the experimental `never_patterns` feature. See the tracking issue (rust-lang/rust#118155) for details on the experiment. `@scottmcm` has agreed to be my lang-team liaison for this experiment.
The first match is UB. First, in Rust, any reads on uninitialized memory are considered undefined behavior; and second, if the Err variant cannot be constructed, it can't ever exist. Thus, the check for it can (and should) be removed by the optimizer, wiping away any matching on the Err variant and the code associated with it. Basically, the match would always execute the Ok branch regardless of the existence of valid data on the inner, which is exactly what you're not trying to do there (it would always get optimized down to the second match, which is equivalent to the ones proposed that are not unsafe). I think the fact that we do not want to touch uninitialized memory directly (w/o |
Hi! I thought like you not that long ago, until I digged into the subtleties of rust operational semantics. Allow me to clarify.
This is correct, but that match does not read from the value in the
While this is a likely possibility for rust opsem to decide, this has not been decided yet. Therefore I am erring on the side of assuming it is possible to read
I'm confused, there's an unsafe block in the code you quote. Dereferencing a pointer always requires an unsafe block.
No, this is an experimental feature gate, meant to iron out the details of the feature before we write the RFC. The lang-team took a quick look at my proposal and approved the experiment. I have discussed details of rust opsem with @RalfJung for a while before I felt confident proposing this. While I believe my approach is sound, this will have to be fully settled in the (to-be-written) RFC.
This isn't quite correct. We need the possibility of uninitialized data behind a pointer. In fact to initialize a The
This is the opposite of what this proposal is about. "Allow elision of unreachable patterns" is implemented in the |
This is not correct. Please consult the reference for what is and is not UB. Here is an example of code that reads uninit memory (for some definition of "reads") and has no UB. You need to be careful with blanket statements like that. Miri's current implementation means that reading the discriminant of a
Never Patterns help because they let us make explicit when we want to exploit that a type is empty. We can then factor the exhaustive_patterns discussion into "how do Never Patterns work" and "when do we implicitly insert Never Patterns", i.e., when can they be elided. For the case of |
The summary of this issue doesn't entirely capture this factoring I described though, it could be improved. Currently it mixes up the concept of Never Patterns with the concept of eliding Never Patterns, while I think these concepts should be discussed completely separately. In that sense gathering feedback on a separate writeup might be helpful. The experimental process means we don't have to wait until the RFC is all set and done, but it can still help to have the RFC at least sketched so that key stakeholders can even understand what the experimental change is doing without reverse engineering that from the implementation. It can also serve as a guideline to the reviewer. The first example in this issue does not use a never pattern, making it a strange example for explaining never patterns. |
Oh well, every time I think I get it I discover I'm missing something. Thx for pointing it out.
No, my proposal is different. I'm intentionally moving away from this "implicit/explicit" frame that was in Niko's original never patterns blog post. Maybe this will turn out to be mistaken, but my current stance is: if a branch is reachable without UB, then it can't be omitted. That would be exhaustiveness being unsound. That's it, no elided never patterns or such things. Then I add the never patterns syntax as a simpler alternative to I find that a lot easier to understand and explain than a contextual desugaring. The drawback is that exhaustiveness now depends on the source of the scrutinee place and not just its type.
I linked this sketch in the OP, have you read it?
Well yeah, that's because this proposal has two parts: there's a refinement of exhaustiveness checking, and there's the never patterns syntax. I do intend to split off the exhaustiveness part into its own feature that we might be able to stabilize sooner. |
I consulted the Rustonomicon, sorry if I was misinformed. EDIT: It seems that in this context (enums) I was right, but my statement without it wasn't, thanks for pointing that out. I wasn't aware of the following:
|
But how do you know that it is an Err? Because you read the discriminant, which is either valid (Ok/Err) or invalid and UB to operate with it. You can't say "it is Err because it was not Ok", you're assuming something the optimizer hasn't yet and might not do. If, for example, it decides to use jump tables, you could be jumping to dead code for all you know. The unsafe thus should mean that at least, you ensure that the enum has a valid representation, and if it has it, it still has to be only Ok because the Err is uninhabited (more on that on the next paragraph).
But that post is about the layout and how we manage it internally, and I'm talking about the semantics of uninhabited (never) types. According to the reference, casting uninhabited (never) types out of thin air is unsound, and that's the same as having the discriminant be Err on that value, you're saying that whatever is next (in this case, zero bytes), is a constructed uninhabited type, which is equivalent to a singularity. The Err variant is an invalid (ergo unsound) state of the program. So you have to extend the meaning of that unsafe statement to mean "I'm sure it is an Ok", but it appears to me that's exactly the opposite of what you're trying to do...
Now I see it, I was looking for it, but for some reason I couldn't find it yesterday, sorry for that, mea culpa.
Indeed, raw pointers indeed can point to uninitialized data, but that was not my point. My point is that you're reading a value that you don't know if it is initialized and in a valid state (the discriminant), and you're checking that validity using semantics that assume the value is in a valid state, so the checks may get removed altogether.
Oh, great then. |
I feel like we're talking a bit past each other, I ask your patience as we try to find what we're disagreeing on.
That doesn't seem correct. First I can very well make a Note the very important fact that all of this happens behind a pointer. If it was instead a plain Also I'm not sure what we're arguing about because it is already the case on stable rust that the
That is not what I'm doing. If the match expression was compiled that way I'm pretty sure it would be a bug. What the If on the other hand I allowed
Until opsem decides for sure that |
Maybe I should be clearer about what's currently required on stable rust. In both stable rust and under the proposal, the first match below is accepted, and the second match below gives a "non-exhaustive" error. enum Void {}
unsafe {
let ptr: *const Result<u32, Void> = ...;
match *ptr {
Ok(x) => { ... }
Err(_) => { ... }
}
match *ptr { // ERROR: non-exhaustive: `Err` not covered
Ok(x) => { ... }
}
} As long as we're behind a pointer/reference/union, there's no changes compared to stable. Where it does change is in the other cases, e.g. a plain value. In stable rust, we get: enum Void {}
let val: Result<u32, Void> = ...;
match val { // accepted
Ok(x) => ...
Err(_) => ...
}
match val { // ERROR: non-exhaustive: `Err` not covered
Ok(x) => ...
} With enum Void {}
let val: Result<u32, Void> = ...;
match val {
Ok(x) => ...
Err(_) => ... // WARN: unreachable
}
match val { // accepted
Ok(x) => ...
} |
…take-3, r=compiler-errors Don't warn an empty pattern unreachable if we're not sure the data is valid Exhaustiveness checking used to be naive about the possibility of a place containing invalid data. This could cause it to emit an "unreachable pattern" lint on an arm that was in fact reachable, as in rust-lang#117119. This PR fixes that. We now track whether a place that is matched on may hold invalid data. This also forced me to be extra precise about how exhaustiveness manages empty types. Note that this now errs in the opposite direction: the following arm is truly unreachable (because the binding causes a read of the value) but not linted as such. I'd rather not recommend writing a `match ... {}` that has the implicit side-effect of loading the value. [Never patterns](rust-lang#118155) will solve this cleanly. ```rust match union.value { _x => unreachable!(), } ``` I recommend reviewing commit by commit. I went all-in on the test suite because this went through a lot of iterations and I kept everything. The bit I'm least confident in is `is_known_valid_scrutinee` in `check_match.rs`. Fixes rust-lang#117119.
But you are reading it! That's where our disagreement comes from, and it's not even the root of it. You're reading it when you dereference the pointer, because you're not reborrowing it (you then discard/drop it later with fn main() {
let mut x = Foo::<i32, Uninhabited>::X(1);
assert_eq!(std::mem::size_of_val(&x), std::mem::size_of::<i32>() * 2); // Uninhabited is a ZST.
assert_eq!(std::mem::size_of::<()>(), std::mem::size_of::<Uninhabited>()); // () is also a ZST.
let mut y = Foo::<i32, ()>::X(1);
assert_eq!(x, unsafe { std::mem::transmute(y) }); // Ergo, reprs are the same.
y = Foo::Y(());
let mut x = unsafe { std::mem::transmute::<_, FooLayout<i32, Uninhabited>>(x) };
x.tag = 1; // Y's tag.
let x = unsafe { std::mem::transmute::<_, Foo<i32, Uninhabited>>(x) };
}
#[derive(Copy, Clone, Debug, PartialEq)]
enum Uninhabited {}
#[repr(C, i32)]
#[derive(Debug, PartialEq)]
enum Foo<A, B> where A: Copy, B: Copy {
X(A) = 0,
Y(B) = 1,
}
#[repr(C)]
union UntaggedFoo<A, B> where A: Copy, B: Copy {
x: A,
y: B,
}
#[repr(C)]
struct FooLayout<A, B> where A: Copy, B: Copy {
tag: i32,
union: UntaggedFoo<A, B>,
} When run with miri, notice that the write is, indeed, valid, but even if unused or unread, thinking about it as a Foo is already UB. On your initial piece of code you're checking if the discriminant is valid, and if it is not, you execute some code. However, when you do that, the potentially invalid data escapes the raw pointer, which is unsound. |
I do not understand your explanation. Anyway, what do you concretely propose we should do differently? |
I propose to split off the exhaustiveness-affecting changes into their own feature gate: #118803. If that gets accepted, then |
That comment disappeared. So I guess you found them? :) |
I didn't but I realized hackmd was overall very buggy y'day for me so that was probably the cause |
I see it! And it definitely wasn't there yesterday x) |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Hey sorry, I quote:
Please open a dedicated issue or come ask your question on Zulip. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I have split off the exhaustiveness-affecting parts of this feature to |
… r=compiler-errors Add the `min_exhaustive_patterns` feature gate ## Motivation Pattern-matching on empty types is tricky around unsafe code. For that reason, current stable rust conservatively requires arms for empty types in all but the simplest case. It has long been the intention to allow omitting empty arms when it's safe to do so. The [`exhaustive_patterns`](rust-lang#51085) feature allows the omission of all empty arms, but hasn't been stabilized because that was deemed dangerous around unsafe code. ## Proposal This feature aims to stabilize an uncontroversial subset of exhaustive_patterns. Namely: when `min_exhaustive_patterns` is enabled and the data we're matching on is guaranteed to be valid by rust's operational semantics, then we allow empty arms to be omitted. E.g.: ```rust let x: Result<T, !> = foo(); match x { // ok Ok(y) => ..., } let Ok(y) = x; // ok ``` If the place is not guaranteed to hold valid data (namely ptr dereferences, ref dereferences (conservatively) and union field accesses), then we keep stable behavior i.e. we (usually) require arms for the empty cases. ```rust unsafe { let ptr: *const Result<u32, !> = ...; match *ptr { Ok(x) => { ... } Err(_) => { ... } // still required } } let foo: Result<u32, &!> = ...; match foo { Ok(x) => { ... } Err(&_) => { ... } // still required because of the dereference } unsafe { let ptr: *const ! = ...; match *ptr {} // already allowed on stable } ``` Note that we conservatively consider that a valid reference can point to invalid data, hence we don't allow arms of type `&!` and similar cases to be omitted. This could eventually change depending on [opsem decisions](rust-lang/unsafe-code-guidelines#413). Whenever opsem is undecided on a case, we conservatively keep today's stable behavior. I proposed this behavior in the [`never_patterns`](rust-lang#118155) feature gate but it makes sense on its own and could be stabilized more quickly. The two proposals nicely complement each other. ## Unresolved Questions Part of the question is whether this requires an RFC. I'd argue this doesn't need one since there is no design question beyond the intent to omit unreachable patterns, but I'm aware the problem can be framed in ways that require design (I'm thinking of the [original never patterns proposal](https://smallcultfollowing.com/babysteps/blog/2018/08/13/never-patterns-exhaustive-matching-and-uninhabited-types-oh-my/), which would frame this behavior as "auto-nevering" happening). EDIT: I initially proposed a future-compatibility lint as part of this feature, I don't anymore.
… r=compiler-errors Add the `min_exhaustive_patterns` feature gate ## Motivation Pattern-matching on empty types is tricky around unsafe code. For that reason, current stable rust conservatively requires arms for empty types in all but the simplest case. It has long been the intention to allow omitting empty arms when it's safe to do so. The [`exhaustive_patterns`](rust-lang#51085) feature allows the omission of all empty arms, but hasn't been stabilized because that was deemed dangerous around unsafe code. ## Proposal This feature aims to stabilize an uncontroversial subset of exhaustive_patterns. Namely: when `min_exhaustive_patterns` is enabled and the data we're matching on is guaranteed to be valid by rust's operational semantics, then we allow empty arms to be omitted. E.g.: ```rust let x: Result<T, !> = foo(); match x { // ok Ok(y) => ..., } let Ok(y) = x; // ok ``` If the place is not guaranteed to hold valid data (namely ptr dereferences, ref dereferences (conservatively) and union field accesses), then we keep stable behavior i.e. we (usually) require arms for the empty cases. ```rust unsafe { let ptr: *const Result<u32, !> = ...; match *ptr { Ok(x) => { ... } Err(_) => { ... } // still required } } let foo: Result<u32, &!> = ...; match foo { Ok(x) => { ... } Err(&_) => { ... } // still required because of the dereference } unsafe { let ptr: *const ! = ...; match *ptr {} // already allowed on stable } ``` Note that we conservatively consider that a valid reference can point to invalid data, hence we don't allow arms of type `&!` and similar cases to be omitted. This could eventually change depending on [opsem decisions](rust-lang/unsafe-code-guidelines#413). Whenever opsem is undecided on a case, we conservatively keep today's stable behavior. I proposed this behavior in the [`never_patterns`](rust-lang#118155) feature gate but it makes sense on its own and could be stabilized more quickly. The two proposals nicely complement each other. ## Unresolved Questions Part of the question is whether this requires an RFC. I'd argue this doesn't need one since there is no design question beyond the intent to omit unreachable patterns, but I'm aware the problem can be framed in ways that require design (I'm thinking of the [original never patterns proposal](https://smallcultfollowing.com/babysteps/blog/2018/08/13/never-patterns-exhaustive-matching-and-uninhabited-types-oh-my/), which would frame this behavior as "auto-nevering" happening). EDIT: I initially proposed a future-compatibility lint as part of this feature, I don't anymore.
… r=compiler-errors Add the `min_exhaustive_patterns` feature gate ## Motivation Pattern-matching on empty types is tricky around unsafe code. For that reason, current stable rust conservatively requires arms for empty types in all but the simplest case. It has long been the intention to allow omitting empty arms when it's safe to do so. The [`exhaustive_patterns`](rust-lang#51085) feature allows the omission of all empty arms, but hasn't been stabilized because that was deemed dangerous around unsafe code. ## Proposal This feature aims to stabilize an uncontroversial subset of exhaustive_patterns. Namely: when `min_exhaustive_patterns` is enabled and the data we're matching on is guaranteed to be valid by rust's operational semantics, then we allow empty arms to be omitted. E.g.: ```rust let x: Result<T, !> = foo(); match x { // ok Ok(y) => ..., } let Ok(y) = x; // ok ``` If the place is not guaranteed to hold valid data (namely ptr dereferences, ref dereferences (conservatively) and union field accesses), then we keep stable behavior i.e. we (usually) require arms for the empty cases. ```rust unsafe { let ptr: *const Result<u32, !> = ...; match *ptr { Ok(x) => { ... } Err(_) => { ... } // still required } } let foo: Result<u32, &!> = ...; match foo { Ok(x) => { ... } Err(&_) => { ... } // still required because of the dereference } unsafe { let ptr: *const ! = ...; match *ptr {} // already allowed on stable } ``` Note that we conservatively consider that a valid reference can point to invalid data, hence we don't allow arms of type `&!` and similar cases to be omitted. This could eventually change depending on [opsem decisions](rust-lang/unsafe-code-guidelines#413). Whenever opsem is undecided on a case, we conservatively keep today's stable behavior. I proposed this behavior in the [`never_patterns`](rust-lang#118155) feature gate but it makes sense on its own and could be stabilized more quickly. The two proposals nicely complement each other. ## Unresolved Questions Part of the question is whether this requires an RFC. I'd argue this doesn't need one since there is no design question beyond the intent to omit unreachable patterns, but I'm aware the problem can be framed in ways that require design (I'm thinking of the [original never patterns proposal](https://smallcultfollowing.com/babysteps/blog/2018/08/13/never-patterns-exhaustive-matching-and-uninhabited-types-oh-my/), which would frame this behavior as "auto-nevering" happening). EDIT: I initially proposed a future-compatibility lint as part of this feature, I don't anymore.
Rollup merge of rust-lang#118803 - Nadrieril:min-exhaustive-patterns, r=compiler-errors Add the `min_exhaustive_patterns` feature gate ## Motivation Pattern-matching on empty types is tricky around unsafe code. For that reason, current stable rust conservatively requires arms for empty types in all but the simplest case. It has long been the intention to allow omitting empty arms when it's safe to do so. The [`exhaustive_patterns`](rust-lang#51085) feature allows the omission of all empty arms, but hasn't been stabilized because that was deemed dangerous around unsafe code. ## Proposal This feature aims to stabilize an uncontroversial subset of exhaustive_patterns. Namely: when `min_exhaustive_patterns` is enabled and the data we're matching on is guaranteed to be valid by rust's operational semantics, then we allow empty arms to be omitted. E.g.: ```rust let x: Result<T, !> = foo(); match x { // ok Ok(y) => ..., } let Ok(y) = x; // ok ``` If the place is not guaranteed to hold valid data (namely ptr dereferences, ref dereferences (conservatively) and union field accesses), then we keep stable behavior i.e. we (usually) require arms for the empty cases. ```rust unsafe { let ptr: *const Result<u32, !> = ...; match *ptr { Ok(x) => { ... } Err(_) => { ... } // still required } } let foo: Result<u32, &!> = ...; match foo { Ok(x) => { ... } Err(&_) => { ... } // still required because of the dereference } unsafe { let ptr: *const ! = ...; match *ptr {} // already allowed on stable } ``` Note that we conservatively consider that a valid reference can point to invalid data, hence we don't allow arms of type `&!` and similar cases to be omitted. This could eventually change depending on [opsem decisions](rust-lang/unsafe-code-guidelines#413). Whenever opsem is undecided on a case, we conservatively keep today's stable behavior. I proposed this behavior in the [`never_patterns`](rust-lang#118155) feature gate but it makes sense on its own and could be stabilized more quickly. The two proposals nicely complement each other. ## Unresolved Questions Part of the question is whether this requires an RFC. I'd argue this doesn't need one since there is no design question beyond the intent to omit unreachable patterns, but I'm aware the problem can be framed in ways that require design (I'm thinking of the [original never patterns proposal](https://smallcultfollowing.com/babysteps/blog/2018/08/13/never-patterns-exhaustive-matching-and-uninhabited-types-oh-my/), which would frame this behavior as "auto-nevering" happening). EDIT: I initially proposed a future-compatibility lint as part of this feature, I don't anymore.
never_patterns
)
RFC is up: rust-lang/rfcs#3719 |
This is a tracking issue for the experimental "never patterns" feature (as per the T-lang experimental feature process).
The feature gate for the issue is
#![feature(never_patterns)]
.RFC: rust-lang/rfcs#3719
This feature introduces a new syntax for patterns:
!
. In a pattern,!
is valid for any uninhabited type and simply acknowledges that the type is empty. Because a never pattern is statically known to be unreachable, it obeys slightly different rules:This feature does not affect exhaustiveness or pattern reachability; see the
exhaustive_patterns
andmin_exhaustive_patterns
features for that.About tracking issues
Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.
Steps
Unresolved Questions
Implementation plans/details
!
argument not detected as diverging on async fn #120240Implementation history
never_patterns
feature gate #118157!
bindings as diverging #120104!
to_
. #120517!
patterns on non-exhaustive matches #121823Unreachable
in MIR #123332The text was updated successfully, but these errors were encountered: