Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Auto merge of #68350 - Aaron1011:its-daylight-saving-time, r=<try>
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.
- Loading branch information
