-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Normalize parameter environment with its own substitution #22338
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
Conversation
r? @pcwalton (rust_highfive has picked a reviewer for you, use r? to override) |
c4bc3b4
to
b2103ef
Compare
// here we just return ty_err. | ||
(tcx.types.err, vec![]) | ||
}, | ||
move |assoc_type| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this should not need move
b2103ef
to
835807f
Compare
So I did some digging here. I think what's going wrong is that we wind up creating a region variable during environment normalization and then we don't replace it with its inferred variable. This is a pretty big oversight on my part! I'm trying to figure out the correct fix here, but I don't think that applying the substitutions in this way is right. The best thing would be to fixup the parameter environment normalization to replace the region variables with an inferred result, but my naive attempt to do so revealed that there were not enough constraints getting generated to yield reasonable results. I need to dig further to understand why not and what needs to be done to fix it. |
835807f
to
725f085
Compare
No more dubious "ambient substs", but otherwise the patch employes the same approach. So this is still FYI, I guess. |
041770f
to
200483e
Compare
r? @nikomatsakis |
So I've got a rather different patch. The branch is here: https://github.com/nikomatsakis/rust/tree/issue-21750-normalization-with-regions It's a bit rough atm but it also fixes #21750/bootstraps etc. Basically what I think is going wrong with #21750 is that I forgot that although we fully resolve the type variables in the param env, we AREN'T replace the region variables. That is quite the no-no, because those region variables are created in a temporary inference context that we then throw away, leading to ICEs and other nonsense. The problem is that just resolving the region variables fails, because the results are inferred to be empty regions. This is due to the way that variance and associated types interact at present. The problem is that the I think that the current variance rules are sound, but they are not perhaps super helpful. This has bitten me before. In that patch I've modified the variance inference so that any trait which defines associated types is effectively invariant with respect to its inputs. This solves the problem. (It also fits well with the subtyping rules for projected types, which require an exact match on the trait reference being projected out of.) The patch still needs to be cleaned up to handle errors better. I also would like to draw up some examples showing why I think this variance treatment for assoc types is superior. |
Ah, thanks for the very detailed explanation. Feel free to use test cases from this patch that you think useful. |
Otherwise, the substitution is carried out from outside perspective, which would be against the very purpose of `ParameterEnvironment`. Closes rust-lang#21750 Closes rust-lang#22077
200483e
to
73bd5cf
Compare
Otherwise, the substitution is carried out from outside perspective,
which would be against the very purpose of
ParameterEnvironment
.Closes #21750
Closes #22066
Closes #22077