without fallback, never type introduces regressions #67225
Assignees
Labels
F-never_type
`#![feature(never_type)]`
T-lang
Relevant to the language team, which will review and decide on the PR/issue.
Comments
nikomatsakis
added
F-never_type
2 tasks
bors
added a commit
that referenced
this issue
Jan 18, 2020
Add lint for never type regressions Fixes #67225 Tl;DR: This PR introduces a lint that detects the 'bad' never-type fallback in `objc` (which results in U.B.), while allowing safe fallback to occur without a warning. See https://hackmd.io/@FohtAO04T92wF-8_TVATYQ/SJ0vcjyWL for some background on never-type fallback. ### The problem We want to reject "bad" code like this: ```rust fn unconstrained_return<T>() -> Result<T, String> { let ffi: fn() -> T = transmute(some_pointer); Ok(ffi()) } fn foo() { match unconstrained_return::<_>() { Ok(x) => x, // `x` has type `_`, which is unconstrained Err(s) => panic!(s), // … except for unifying with the type of `panic!()` // so that both `match` arms have the same type. // Therefore `_` resolves to `!` and we "return" an `Ok(!)` value. }; } ``` in which enabling never-type fallback can cause undefined behavior. However, we want to allow "good" code like this: ```rust struct E; impl From<!> for E { fn from(x: !) -> E { x } } fn foo(never: !) { <E as From<_>>::from(never); } fn main() {} ``` which relies on never-type fallback being enabled, but is perfectly safe. ### The solution The key difference between these two examples lies in how the result of never-type fallback is used. In the first example, we end up inferring the generic parameter of `unconstrained_return` to be `!`. In the second example, we still infer a generic parameter to be `!` (`Box::<!>::new(!)`), but we also pass an uninhabited parameter to the function. Another way of looking at this is that the call to `unconstrained_return` is **potentially live* - none of its arguments are uninhabited, so we might (and in fact, do) end up actually executing the call at runtime. In the second example, `Box::new()` has an uninhabited argument (the `!` type). This means that this call is **definitely dead** - since the `!` type can never be instantiated, it's impossible for the call to every be executed. This forms the basis for the check. For each method call, we check the following: 1. Did the generic arguments have unconstrained type variables prior to fallback? 2. Did any of the generic arguments become uninhabited after fallback? 3. Are all of the method arguments inhabited? If the answer to all of these is *yes*, we emit an error. I've left extensive comments in the code describing how this is accomplished. These conditions ensure that we don't error on the `Box` and `From<!>` examples, while still erroring on the bad `objc` code. ### Further notes You can test out this branch with the original bad `objc` code as follows: 1. Clone `https://github.com/Aaron1011/rust-objc` 2. Checkout the `bad-fallback` branch. 3. With a local rustc toolchain built from this branch, run `cargo build --example example` 4. Note that you get an error due to an unconstrained return type ### Unresolved questions 1. This lint only checks method calls. I believe this is sufficient to catch any undefined behavior caused by fallback changes. Since the introduced undefined behavior relies on actually 'producing' a `!` type instance, the user must be doing something 'weird' (calling `transmute` or some other intrinsic). I don't think it's possible to trigger this without *some* kind of intrinsic call - however, I'm not 100% certain. 2. This lint requires us to perform extra work during the type-checking of every single method. This is not ideal - however, changing this would have required significant refactoring to method type-checking. It would be a good idea to due to a perf run to see what kind of impact this has, and it another approach will be required. 3. This 'lint' is currently a hard error. I believe it should always be possible to fix this error by adding explicit type annotations *somewhere* (though in the `obj` case, this may be in the caller of a macro). Unfortunately, I think actually emitting any kind of suggestion to the user will be extremely difficult. Hopefully, this error is so rare that the lack of suggestion isn't a problem. If users are running into this with any frequency, I think we'll need a different approach. 4. If this PR is accepted, I see two ways of rolling this out: 1. If the bad `objc` crate is the only crate known to be affected, we could potentially go from no warning/lint to a hard error in a single release (coupled enabling never-type fallback0. 2. If we're worried that this could break a lot of crates, we could make this into a future compatibility lint. At some point in the future, we could enable never-type fallback while simultaneously making this a hard error. What we should **not** do is make the never-type fallback changes without making this lint (or whatever lint ends up getting accepted) into a hard error. A lint, even a deny-by-default one, would be insufficient, as we would run a serious risk introducing undefined behavior without any kind of explicit acknowledgment from the user.
bors
added a commit
that referenced
this issue
Jan 18, 2020
Add lint for never type regressions Fixes #67225 Tl;DR: This PR introduces a lint that detects the 'bad' never-type fallback in `objc` (which results in U.B.), while allowing safe fallback to occur without a warning. See https://hackmd.io/@FohtAO04T92wF-8_TVATYQ/SJ0vcjyWL for some background on never-type fallback. ### The problem We want to reject "bad" code like this: ```rust fn unconstrained_return<T>() -> Result<T, String> { let ffi: fn() -> T = transmute(some_pointer); Ok(ffi()) } fn foo() { match unconstrained_return::<_>() { Ok(x) => x, // `x` has type `_`, which is unconstrained Err(s) => panic!(s), // … except for unifying with the type of `panic!()` // so that both `match` arms have the same type. // Therefore `_` resolves to `!` and we "return" an `Ok(!)` value. }; } ``` in which enabling never-type fallback can cause undefined behavior. However, we want to allow "good" code like this: ```rust struct E; impl From<!> for E { fn from(x: !) -> E { x } } fn foo(never: !) { <E as From<_>>::from(never); } fn main() {} ``` which relies on never-type fallback being enabled, but is perfectly safe. ### The solution The key difference between these two examples lies in how the result of never-type fallback is used. In the first example, we end up inferring the generic parameter of `unconstrained_return` to be `!`. In the second example, we still infer a generic parameter to be `!` (`Box::<!>::new(!)`), but we also pass an uninhabited parameter to the function. Another way of looking at this is that the call to `unconstrained_return` is **potentially live* - none of its arguments are uninhabited, so we might (and in fact, do) end up actually executing the call at runtime. In the second example, `Box::new()` has an uninhabited argument (the `!` type). This means that this call is **definitely dead** - since the `!` type can never be instantiated, it's impossible for the call to every be executed. This forms the basis for the check. For each method call, we check the following: 1. Did the generic arguments have unconstrained type variables prior to fallback? 2. Did any of the generic arguments become uninhabited after fallback? 3. Are all of the method arguments inhabited? If the answer to all of these is *yes*, we emit an error. I've left extensive comments in the code describing how this is accomplished. These conditions ensure that we don't error on the `Box` and `From<!>` examples, while still erroring on the bad `objc` code. ### Further notes You can test out this branch with the original bad `objc` code as follows: 1. Clone `https://github.com/Aaron1011/rust-objc` 2. Checkout the `bad-fallback` branch. 3. With a local rustc toolchain built from this branch, run `cargo build --example example` 4. Note that you get an error due to an unconstrained return type ### Unresolved questions 1. This lint only checks method calls. I believe this is sufficient to catch any undefined behavior caused by fallback changes. Since the introduced undefined behavior relies on actually 'producing' a `!` type instance, the user must be doing something 'weird' (calling `transmute` or some other intrinsic). I don't think it's possible to trigger this without *some* kind of intrinsic call - however, I'm not 100% certain. 2. This lint requires us to perform extra work during the type-checking of every single method. This is not ideal - however, changing this would have required significant refactoring to method type-checking. It would be a good idea to due to a perf run to see what kind of impact this has, and it another approach will be required. 3. This 'lint' is currently a hard error. I believe it should always be possible to fix this error by adding explicit type annotations *somewhere* (though in the `obj` case, this may be in the caller of a macro). Unfortunately, I think actually emitting any kind of suggestion to the user will be extremely difficult. Hopefully, this error is so rare that the lack of suggestion isn't a problem. If users are running into this with any frequency, I think we'll need a different approach. 4. If this PR is accepted, I see two ways of rolling this out: 1. If the bad `objc` crate is the only crate known to be affected, we could potentially go from no warning/lint to a hard error in a single release (coupled enabling never-type fallback0. 2. If we're worried that this could break a lot of crates, we could make this into a future compatibility lint. At some point in the future, we could enable never-type fallback while simultaneously making this a hard error. What we should **not** do is make the never-type fallback changes without making this lint (or whatever lint ends up getting accepted) into a hard error. A lint, even a deny-by-default one, would be insufficient, as we would run a serious risk introducing undefined behavior without any kind of explicit acknowledgment from the user.
bors
added a commit
that referenced
this issue
Jan 19, 2020
Add lint for never type regressions Fixes #67225 Tl;DR: This PR introduces a lint that detects the 'bad' never-type fallback in `objc` (which results in U.B.), while allowing safe fallback to occur without a warning. See https://hackmd.io/@FohtAO04T92wF-8_TVATYQ/SJ0vcjyWL for some background on never-type fallback. ### The problem We want to reject "bad" code like this: ```rust fn unconstrained_return<T>() -> Result<T, String> { let ffi: fn() -> T = transmute(some_pointer); Ok(ffi()) } fn foo() { match unconstrained_return::<_>() { Ok(x) => x, // `x` has type `_`, which is unconstrained Err(s) => panic!(s), // … except for unifying with the type of `panic!()` // so that both `match` arms have the same type. // Therefore `_` resolves to `!` and we "return" an `Ok(!)` value. }; } ``` in which enabling never-type fallback can cause undefined behavior. However, we want to allow "good" code like this: ```rust struct E; impl From<!> for E { fn from(x: !) -> E { x } } fn foo(never: !) { <E as From<_>>::from(never); } fn main() {} ``` which relies on never-type fallback being enabled, but is perfectly safe. ### The solution The key difference between these two examples lies in how the result of never-type fallback is used. In the first example, we end up inferring the generic parameter of `unconstrained_return` to be `!`. In the second example, we still infer a generic parameter to be `!` (`Box::<!>::new(!)`), but we also pass an uninhabited parameter to the function. Another way of looking at this is that the call to `unconstrained_return` is **potentially live* - none of its arguments are uninhabited, so we might (and in fact, do) end up actually executing the call at runtime. In the second example, `Box::new()` has an uninhabited argument (the `!` type). This means that this call is **definitely dead** - since the `!` type can never be instantiated, it's impossible for the call to every be executed. This forms the basis for the check. For each method call, we check the following: 1. Did the generic arguments have unconstrained type variables prior to fallback? 2. Did any of the generic arguments become uninhabited after fallback? 3. Are all of the method arguments inhabited? If the answer to all of these is *yes*, we emit an error. I've left extensive comments in the code describing how this is accomplished. These conditions ensure that we don't error on the `Box` and `From<!>` examples, while still erroring on the bad `objc` code. ### Further notes You can test out this branch with the original bad `objc` code as follows: 1. Clone `https://github.com/Aaron1011/rust-objc` 2. Checkout the `bad-fallback` branch. 3. With a local rustc toolchain built from this branch, run `cargo build --example example` 4. Note that you get an error due to an unconstrained return type ### Unresolved questions 1. This lint only checks method calls. I believe this is sufficient to catch any undefined behavior caused by fallback changes. Since the introduced undefined behavior relies on actually 'producing' a `!` type instance, the user must be doing something 'weird' (calling `transmute` or some other intrinsic). I don't think it's possible to trigger this without *some* kind of intrinsic call - however, I'm not 100% certain. 2. This lint requires us to perform extra work during the type-checking of every single method. This is not ideal - however, changing this would have required significant refactoring to method type-checking. It would be a good idea to due to a perf run to see what kind of impact this has, and it another approach will be required. 3. This 'lint' is currently a hard error. I believe it should always be possible to fix this error by adding explicit type annotations *somewhere* (though in the `obj` case, this may be in the caller of a macro). Unfortunately, I think actually emitting any kind of suggestion to the user will be extremely difficult. Hopefully, this error is so rare that the lack of suggestion isn't a problem. If users are running into this with any frequency, I think we'll need a different approach. 4. If this PR is accepted, I see two ways of rolling this out: 1. If the bad `objc` crate is the only crate known to be affected, we could potentially go from no warning/lint to a hard error in a single release (coupled enabling never-type fallback0. 2. If we're worried that this could break a lot of crates, we could make this into a future compatibility lint. At some point in the future, we could enable never-type fallback while simultaneously making this a hard error. What we should **not** do is make the never-type fallback changes without making this lint (or whatever lint ends up getting accepted) into a hard error. A lint, even a deny-by-default one, would be insufficient, as we would run a serious risk introducing undefined behavior without any kind of explicit acknowledgment from the user.
bors
added a commit
that referenced
this issue
Jan 19, 2020
Add lint for never type regressions Fixes #67225 Tl;DR: This PR introduces a lint that detects the 'bad' never-type fallback in `objc` (which results in U.B.), while allowing safe fallback to occur without a warning. See https://hackmd.io/@FohtAO04T92wF-8_TVATYQ/SJ0vcjyWL for some background on never-type fallback. ### The problem We want to reject "bad" code like this: ```rust fn unconstrained_return<T>() -> Result<T, String> { let ffi: fn() -> T = transmute(some_pointer); Ok(ffi()) } fn foo() { match unconstrained_return::<_>() { Ok(x) => x, // `x` has type `_`, which is unconstrained Err(s) => panic!(s), // … except for unifying with the type of `panic!()` // so that both `match` arms have the same type. // Therefore `_` resolves to `!` and we "return" an `Ok(!)` value. }; } ``` in which enabling never-type fallback can cause undefined behavior. However, we want to allow "good" code like this: ```rust struct E; impl From<!> for E { fn from(x: !) -> E { x } } fn foo(never: !) { <E as From<_>>::from(never); } fn main() {} ``` which relies on never-type fallback being enabled, but is perfectly safe. ### The solution The key difference between these two examples lies in how the result of never-type fallback is used. In the first example, we end up inferring the generic parameter of `unconstrained_return` to be `!`. In the second example, we still infer a generic parameter to be `!` (`Box::<!>::new(!)`), but we also pass an uninhabited parameter to the function. Another way of looking at this is that the call to `unconstrained_return` is **potentially live* - none of its arguments are uninhabited, so we might (and in fact, do) end up actually executing the call at runtime. In the second example, `Box::new()` has an uninhabited argument (the `!` type). This means that this call is **definitely dead** - since the `!` type can never be instantiated, it's impossible for the call to every be executed. This forms the basis for the check. For each method call, we check the following: 1. Did the generic arguments have unconstrained type variables prior to fallback? 2. Did any of the generic arguments become uninhabited after fallback? 3. Are all of the method arguments inhabited? If the answer to all of these is *yes*, we emit an error. I've left extensive comments in the code describing how this is accomplished. These conditions ensure that we don't error on the `Box` and `From<!>` examples, while still erroring on the bad `objc` code. ### Further notes You can test out this branch with the original bad `objc` code as follows: 1. Clone `https://github.com/Aaron1011/rust-objc` 2. Checkout the `bad-fallback` branch. 3. With a local rustc toolchain built from this branch, run `cargo build --example example` 4. Note that you get an error due to an unconstrained return type ### Unresolved questions 1. This lint only checks method calls. I believe this is sufficient to catch any undefined behavior caused by fallback changes. Since the introduced undefined behavior relies on actually 'producing' a `!` type instance, the user must be doing something 'weird' (calling `transmute` or some other intrinsic). I don't think it's possible to trigger this without *some* kind of intrinsic call - however, I'm not 100% certain. 2. This lint requires us to perform extra work during the type-checking of every single method. This is not ideal - however, changing this would have required significant refactoring to method type-checking. It would be a good idea to due to a perf run to see what kind of impact this has, and it another approach will be required. 3. This 'lint' is currently a hard error. I believe it should always be possible to fix this error by adding explicit type annotations *somewhere* (though in the `obj` case, this may be in the caller of a macro). Unfortunately, I think actually emitting any kind of suggestion to the user will be extremely difficult. Hopefully, this error is so rare that the lack of suggestion isn't a problem. If users are running into this with any frequency, I think we'll need a different approach. 4. If this PR is accepted, I see two ways of rolling this out: 1. If the bad `objc` crate is the only crate known to be affected, we could potentially go from no warning/lint to a hard error in a single release (coupled enabling never-type fallback0. 2. If we're worried that this could break a lot of crates, we could make this into a future compatibility lint. At some point in the future, we could enable never-type fallback while simultaneously making this a hard error. What we should **not** do is make the never-type fallback changes without making this lint (or whatever lint ends up getting accepted) into a hard error. A lint, even a deny-by-default one, would be insufficient, as we would run a serious risk introducing undefined behavior without any kind of explicit acknowledgment from the user.
|
@rustbot assign |
|
Error: Parsing assign command in comment failed: ...'t assign Please let |
|
Mentoring instructions here |
|
I found this RFC helpful to understand what this feature is in the first place and explains my grave unfamiliarity with Rust. |
This comment has been minimized.
This comment has been minimized.
|
I'm going to close this issue, since we have landed a version of #79366 (gated under never_type_fallback still) and it seems pretty clear at this point we cannot stabilize never type without fallback changes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
When we attempted to stabilize the never type (
!) in #65355, we encountered unexpected regressions (#66757). These regressions resulted from this canonical test, soon to be in the repo assrc/test/never_type/never-value-fallback-issue-66757.rs. The heart of the test is:The problem here is that the
nevervariable, when referenced, gets assigned a fresh diverging type variable as its type. In the second call, that type variable is unconstrained, and hence it falls back -- to(). This leads to a compilation error.We need to resolve this. @SimonSapin did a great job of outlining the alternatives here. The current preference is probably to enable the new never type fallback. If that should fail, we could consider altering the fallback for rvalues of
!type from those of diverging expressions, as described here, but that will require more discussion to reach consensus (there are other options too, see Simon's comment).The text was updated successfully, but these errors were encountered: