-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
[WIP] Never type experiments #84573
[WIP] Never type experiments #84573
Conversation
This comment has been minimized.
This comment has been minimized.
581c04b
to
0b0cf19
Compare
As an update - I just pushed the new v2 fallback algorithm we had originally discussed; this currently doesn't quite pass tests in-tree but does address some of the known cases the v1 fallback algorithm failed at. Niko and I spent a bit of time discussing possible solutions to the problems v2 brings up but didn't arrive at anything concrete yet. |
This comment has been minimized.
This comment has been minimized.
0b0cf19
to
59c2b48
Compare
This comment has been minimized.
This comment has been minimized.
59c2b48
to
9f3dbe2
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #84107) made this pull request unmergeable. Please resolve the merge conflicts. |
☔ The latest upstream changes (presumably #84767) made this pull request unmergeable. Please resolve the merge conflicts. |
8d23405
to
df59117
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
e34b518
to
ab133e7
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #85078) made this pull request unmergeable. Please resolve the merge conflicts. |
a5d60c5
to
3dc4f18
Compare
This comment has been minimized.
This comment has been minimized.
3e88c1b
to
4c74421
Compare
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #84985) made this pull request unmergeable. Please resolve the merge conflicts. |
Instead, we now rely on the code that looks for a NeverToAny adjustment.
This could cause us to visit the start node twice.
Extend the `DepthFirstSearch` iterator so that it can be re-used and extended with add'l start nodes. Then replace the FxHashSets of nodes we were using in the fallback analysis with a single iterator. This way we won't re-walk portions of the graph that are reached more than once, and we also do less allocation etc.
f8b5a32
to
6fd5dee
Compare
This comment has been minimized.
This comment has been minimized.
This avoids select_obligations_where_possible having an effect, which future proofs the algorithm.
This doesn't yet fully work -- running coercions seems to be prone to causing errors in the environment, which creates a little noise in some cases. It should be enough to let us re-run crater, though, I think.
6fd5dee
to
40d1e69
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
@Mark-Simulacrum what's the plan here? Are you planning to split out some of this into more PRs? I can definitely try to review some, especially code that was mostly written by Niko :) |
Yeah, the next steps are:
|
Going to go ahead and close this -- work is proceeding in #88804 and this isn't useful to keep open for now. |
…matsakis,jackh726 Revise never type fallback algorithm This is a rebase of rust-lang#84573, but dropping the stabilization of never type (and the accompanying large test diff). Each commit builds & has tests updated alongside it, and could be reviewed in a more or less standalone fashion. But it may make more sense to review the PR as a whole, I'm not sure. It should be noted that tests being updated isn't really a good indicator of final behavior -- never_type_fallback is not enabled by default in this PR, so we can't really see the full effects of the commits here. This combines the work by Niko, which is [documented in this gist](https://gist.github.com/nikomatsakis/7a07b265dc12f5c3b3bd0422018fa660), with some additional rules largely derived to target specific known patterns that regress with the algorithm solely derived by Niko. We build these from an intuition that: * In general, fallback to `()` is *sound* in all cases * But, in general, we *prefer* fallback to `!` as it accepts more code, particularly that written to intentionally use `!` (e.g., Result's with a Infallible/! variant). When evaluating Niko's proposed algorithm, we find that there are certain cases where fallback to `!` leads to compilation failures in real-world code, and fallback to `()` fixes those errors. In order to allow for stabilization, we need to fix a good portion of these patterns. The final rule set this PR proposes is that, by default, we fallback from `?T` to `!`, with the following exceptions: 1. `?T: Foo` and `Bar::Baz = ?T` and `(): Foo`, then fallback to `()` 2. Per [Niko's algorithm](https://gist.github.com/nikomatsakis/7a07b265dc12f5c3b3bd0422018fa660#proposal-fallback-chooses-between--and--based-on-the-coercion-graph), the "live" `?T` also fallback to `()`. The first rule is necessary to address a fairly common pattern which boils down to something like the snippet below. Without rule 1, we do not see the closure's return type as needing a () fallback, which leads to compilation failure. ```rust #![feature(never_type_fallback)] trait Bar { } impl Bar for () { } impl Bar for u32 { } fn foo<R: Bar>(_: impl Fn() -> R) {} fn main() { foo(|| panic!()); } ``` r? `@jackh726`
This currently just rebases #79470 (hopefully all correctly), but will "soon" follow up with an implementation as per rust-lang/lang-team#60 (comment).
r? @ghost