Skip to content
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

handle universes in canonicalization #48696

Closed
nikomatsakis opened this issue Mar 3, 2018 · 6 comments · Fixed by #55921
Closed

handle universes in canonicalization #48696

nikomatsakis opened this issue Mar 3, 2018 · 6 comments · Fixed by #55921
Assignees
Labels
A-traits Area: Trait system C-enhancement Category: An issue proposing an enhancement or a PR with one. T-lang Relevant to the language team, which will review and decide on the PR/issue. WG-traits Working group: Traits, https://internals.rust-lang.org/t/announcing-traits-working-group/6804

Comments

@nikomatsakis
Copy link
Contributor

Now that #47861 has landed, there is a notion of universes for type variables. For now, in order to get #48411 landed, the canonicalization code is simply using UniverseIndex::ROOT for the universes of type variables it creates (those universes aren't really being used anyway presently).

However, what we ought to be doing is recording (during canonicalization) the universe of the variables that are being fed in. We can then canonicalize those universes (what chalk calls u_canonicalize) and then recreate them on the other side. And finally we map the universes back.

I'll try to leave more detailed notes later. I expect to either do this or mentor this as follow-up.

cc @sgrif

@nikomatsakis nikomatsakis added A-traits Area: Trait system WG-traits Working group: Traits, https://internals.rust-lang.org/t/announcing-traits-working-group/6804 labels Mar 3, 2018
@sgrif
Copy link
Contributor

sgrif commented Mar 3, 2018

Definitely interested in working through this

@nikomatsakis
Copy link
Contributor Author

@sgrif and I had a brief conversation in gitter going into a bit more detail. I'll try to get to writing up a rustc-guide chapter here.

sgrif added a commit to sgrif/rust that referenced this issue Apr 2, 2018
Since `ReSkolemized` has the flag `KEEP_IN_LOCAL_TCX`, and
canonicalization is meant to ensure that flag is no longer present, it
implicitly is depending on the existence of the leak check. However,
since we want to remove that in rust-lang#48407, we can no longer make that
assumption.

This is related to rust-lang#48696, but does not resolve it. This does unblock that
PR, however. It could have been tacked on there, but I wanted to make
sure this is a reasonable short term approach separately from that.
@nikomatsakis
Copy link
Contributor Author

@sgrif ok so I was thinking about this last night after we talked. My PR to move skolemized things into the global tcx works, but as I said on gitter I'm now revisiting that question. Maybe you were onto something in #49598.

Perhaps we could set it up like so:

When you canonicalize, we replace skolemized regions with canonical variables, as in your PR.

We would also add information to CanonicalVarInfo indicating whether this was "universally" or "existentially" bound:

pub struct CanonicalVarInfo {
pub kind: CanonicalVarKind,
}

Then when we instantiate each canonical variable, we would create "skolemized" variables for those things that were skolemized (in the appropriate universe):

pub fn fresh_inference_var_for_canonical_var(
&self,
span: Span,
cv_info: CanonicalVarInfo,
) -> Kind<'tcx> {

I think we could start with "identity mapping" between universes, in which case we would still need to carry (in the Canonical value) the max universe of any skolemized things we encountered, so that when we create a fresh inference context, we can make it contain that many universes.

Once we add a real map, I think we could put the map into CanonicalVarValues, or maybe a new kind of struct that wraps it (CanonicalMapping):

pub struct CanonicalVarValues<'tcx> {
pub var_values: IndexVec<CanonicalVar, Kind<'tcx>>,
}

then instantiate_query_result would take a CanonicalMapping instead of just a CanonicalVarValues:

original_values: &CanonicalVarValues<'tcx>,

and use the info to do the mapping.

@sgrif
Copy link
Contributor

sgrif commented Apr 5, 2018

Makes sense. Thanks for looking into it

@sgrif
Copy link
Contributor

sgrif commented Apr 23, 2018

@nikomatsakis Sorry for the radio silence, I've been at a conference the last week. After poking at the solution you described, I think we need to take #49598 as an intermediate step. InferCtxt doesn't know anything about universes until #48407, but that is blocked on some solution to this in the short term. I can roll this change into that PR, but it's already huge and we're not even sure we want to land it until we can get a crater run.

@XAMPPRocky XAMPPRocky added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels May 21, 2018
@nikomatsakis nikomatsakis self-assigned this Oct 15, 2018
@nikomatsakis
Copy link
Contributor Author

I think this stuff is basically fixed by #55305 — we're not doing anything smart yet, but we're handling things correctly now iirc

pietroalbini added a commit to pietroalbini/rust that referenced this issue Nov 16, 2018
Add placeholder types

Fixes rust-lang#48696 (handle universes in canonicalization of type inference vars), and fixes rust-lang#55098.
bors added a commit that referenced this issue Nov 25, 2018
Add placeholder types

Fixes #48696 (handle universes in canonicalization of type inference vars), and fixes #55098.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-traits Area: Trait system C-enhancement Category: An issue proposing an enhancement or a PR with one. T-lang Relevant to the language team, which will review and decide on the PR/issue. WG-traits Working group: Traits, https://internals.rust-lang.org/t/announcing-traits-working-group/6804
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants