Refactor fallback code to prepare for never type #88149
Conversation
|
Some changes occurred in src/tools/clippy. cc @rust-lang/clippy |
rust-highfive
added
the
S-waiting-on-review
| // Note that we can just skip the binders here because | ||
| // type variables can't (at present, at | ||
| // least) capture any of the things bound by this binder. | ||
| { |
There was a problem hiding this comment.
Nit: Do we really need the extra brackets?
| let r_b = self.shallow_resolve(predicate.skip_binder().b); | ||
| match (r_a.kind(), r_b.kind()) { | ||
| (&ty::Infer(ty::TyVar(a_vid)), &ty::Infer(ty::TyVar(b_vid))) => { | ||
| self.inner.borrow_mut().type_variables().sub(a_vid, b_vid); |
There was a problem hiding this comment.
I'm curious what the diagnostics look like without this.
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.)
Mark-Simulacrum
force-pushed
the
prep-never-type
branch
from
7122258 to
01decf3
Compare
|
Force-pushed squashed in fixes to your current comments (since they were all mostly "nit"-like).
https://gist.github.com/nikomatsakis/7a07b265dc12f5c3b3bd0422018fa660 is a pretty good write up of the "v1" algorithm proposed for landing never type. There's some additional extensions that are currently suggested in #84573 to mitigate some of the cases v1 doesn't address, but those are about detecting particular edge cases really. The goal of the coercion predicates in particular is to build up the coercion graph added in the commits after these ~8 in #84573, which is an attempt to track the "liveness" of particular type variables during fallback.
You can see a diff here - acd7444 -- nothing obviously major either way, though issue-57843 under NLL interestingly compiles without errors? |
|
Yes, I started to go back and read through the write up. It's really interesting that issue-57843 compiles in nll without that extra sub. (Actually, I feel like the behavior with the change is more of what we want, but it's still a change). This makes me a bit suspicious. At the very least the comments need to be changed to address that it's not purely diagnostics. Maybe @nikomatsakis might be interested or have thoughts. (P.s. I think it's funny and a nice coincidence how similar 84573 and 57843 are to each other) |
|
@Mark-Simulacrum 536da6f can be removed in isolation, right? If you can remove that, then r=me on this. Given that it does not end up as purely diagnostics changes, I'm more wary to approve that right now. Given that everything else is straightforward, I wouldn't one that one commit to up the whole PR. So, r=me on the other 7 commits. We can discuss that commit in a separate PR? |
Mark-Simulacrum
force-pushed
the
prep-never-type
branch
from
01decf3 to
907390c
Compare
This comment has been minimized.
This comment has been minimized.
|
Ah interesting. Seems less straightforward than I thought. In this case, I'm fine with adding the commit back in with a proper comment that the extra sub is not only for diagnostics but may have semantic effects. r=me with that |
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.
Mark-Simulacrum
force-pushed
the
prep-never-type
branch
from
907390c to
60cc00f
Compare
|
Added the note here 020655b#diff-79cce3ca916fa3f7d4f1b1879dbec1b3852414a5cfc719087851635aaa240a97R1018-R1019, will see if CI is passing (just in case, though it really should). |
|
@bors r=jackh726 |
|
📌 Commit 60cc00f has been approved by |
bors
added
S-waiting-on-bors
|
@bors rollup=never in case this has perf effects, though we don't really expect it. touches some possibly sensitive code though. |
|
☀️ Test successful - checks-actions |
This PR contains cherry-picks of some of @nikomatsakis's work from #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?