conditional fallback for the ! type
#79366
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We used to avoid doing this because we didn't want to make coercion depend on the state of inference. For better or worse, we have moved away from this position over time. Therefore, I am going to go ahead and resolve the `b` target type early on so that it is done uniformly. (The older technique for managing this was always something of a hack regardless; if we really wanted to avoid integrating coercion and inference we needed to be more disciplined about it.)
I didn't like the sub-unify code executing when a predicate was ENQUEUED, that felt fragile. I would have preferred to move the sub-unify code so that it only occurred during generalization, but that impacted diagnostics, so having it also occur when we process subtype predicates felt pretty reasonable. (I guess we only need one or the other, but I kind of prefer both, since the generalizer ultimately feels like the *right* place to guarantee the properties we want.)
Motivation: in upcoming commits, we are going to create a graph of the coercion relationships between variables. We want to distinguish *coercion* specifically from other sorts of subtyping, as it indicates values flowing from one place to another via assignment.
Along the way, simplify and document the logic more clearly.
We now fallback type variables using the following rules: * Construct a coercion graph `A -> B` where `A` and `B` are unresolved type variables or the `!` type. * Let D be those variables that are reachable from `!`. * Let N be those variables that are reachable from a variable not in D. * All variables in (D \ N) fallback to `!`. * All variables in (D & N) fallback to `()`.
The comment seems incorrect. Testing revealed that the examples in question still work (as well as some variants) even without the special casing here.
Instead, we now record those type variables that are the target of a `NeverToAny` adjustment and consider those to be the "diverging" type variables. This allows us to remove the special case logic that creates a type variable for `!` in coercion.
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.
rust-highfive
added
the
S-waiting-on-review
nikomatsakis
added
I-nominated
T-lang
Is this the right one? Also am I correct in assuming that this paves away for finally stabilizing |
Nope, fixed, thanks =)
That is my hope, yes. |
This was referenced Nov 24, 2020
|
@bors rollup=never |
|
I'm thinking of preparing a variant of this PR that "stabilizes" the |
|
That sounds like a great plan. |
8 tasks
|
Created #79470 for crater run |
crlf0710
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #80449) made this pull request unmergeable. Please resolve the merge conflicts. |
camelid
added
the
S-waiting-on-author
JohnCSimon
removed
the
S-waiting-on-review
JohnCSimon
added
S-waiting-on-author
|
@nikomatsakis do you still need this PR or we can close it and reopen later? |
|
@Dylan-DPC I think we can close it, although @Mark-Simulacrum and I were talking about some possible extensions here. But I don't see that having this PR open is helping anything. |
|
Yeah, I don't think the PR being open is helpful. I still need to type up a summary of our discussion, but that'll go into a new issue. |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 21, 2021
…ckh726 Refactor fallback code to prepare for never type This PR contains cherry-picks of some of `@nikomatsakis's` work from rust-lang#79366, and shouldn't (AFAICT) represent any change in behavior. However, the refactoring is good regardless of the never type work being landed, and will reduce the size of those eventual PR(s) (and rebase pain). I am not personally an expert on this code, and the commits are essentially 100% `@nikomatsakis's,` but they do seem reasonable to me by my understanding. Happy to edit with review, of course. Commits are best reviewed in sequence rather than all together. r? `@jackh726` perhaps?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This branch represents a proposed means to handle fallback for the
!type. The intuition is that it sometimes falls back to!and sometimes(). We prefer to fallback to!but will fallback to()if the!type may escape into live code.Here is a write-up:
https://gist.github.com/nikomatsakis/7a07b265dc12f5c3b3bd0422018fa660
Fixes #66173
Fixes #67225
Fixes #66757
cc @rust-lang/lang
r? @matthewjasper