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

Support polymorphic InstanceDef's by avoiding double substitution. #69925

Closed
eddyb opened this issue Mar 11, 2020 · 3 comments · Fixed by #75346
Closed

Support polymorphic InstanceDef's by avoiding double substitution. #69925

eddyb opened this issue Mar 11, 2020 · 3 comments · Fixed by #75346
Labels
-Zpolymorphize Unstable option: Polymorphization. A-codegen Area: Code generation A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@eddyb
Copy link
Member

eddyb commented Mar 11, 2020

#69036 papers over a problem in the use of MIR shim bodies, (i.e. applying Instance's substs), by preventing Instance::resolve from succeeding if any substitution could later occur.

A MIR shim's body already encapsulates the final types (i.e. monomorphic for codegen, modulo "polymorphization" work), so we shouldn't substitute it any further (in codegen, miri, or MIR inlining).

Once we address this, we could inline (or codegen, given "polymorphization") slightly polymorphic variants of these shims, but Instance::resolve would still need to enforce the minimum requirements for the shim MIR body being built in the first place, i.e.:

  • Instance::resolve(drop_in_place::<T>) should return None
  • Instance::resolve(drop_in_place::<Vec<T>>) could succeed

cc @nikomatsakis @davidtwco

@jonas-schievink jonas-schievink added A-codegen Area: Code generation A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 11, 2020
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Mar 11, 2020

So @davidtwco, @eddyb and I spent a while discussing this on Zulip in an effort to better understand what's going on. Folks might find this topic or this topic helpful.

Let me try to summarize my understanding of (a) how things work and (b) the problem:

Currently, an Instance is an InstanceDef plus some substitutions. The MIR for an instance is created once per instance-def, but we then apply the substitutions in two places: (1) to figure out the signature of the MIR function and (2) to the body, during monomorphization (which may, e.g., allow trait calls to be resolved to particular impls and so forth).

The problem is that for some kinds of InstanceDef, notably DropGlue, we don't really have a rich enough set of variants. So what we do is a kind of "cheat" where we encode a Ty that contains the details we need to operate on -- for example, the DropGlue variant includes the type being dropped.

Note: Some of the text below is kind of confused in its particulars, so striking out, but still relevant-ish.

The problem here is kind of hard to summarize. It's a couple of 'conflicting' assumptions that might individually be ok but interact poorly. For one thing, when we apply substitutions to an Instance, we do not alter the InstanceDef, but just the substs (since the InstanceDef is meant to be the "generic form" of the thing, with the substs specifying the values for its generic parameters). But also the substs are always meant to correspond to the generics of the instance_def.def_id() result. But for things like drop-glue, the def-id is for the drop_in_place function, but the InstanceDef also includes part of the substs (the type being dropped). So if we substitute into substs, that gets "out of sync" with the InstanceDef. e.g., if we started out with drop-glue for Vec<T> and then substituted T => Vec<U>, we'd have a substs with Vec<Vec<U>> but the instance-def would still say Vec<T>. This is then a problem because the two sets of types are expressed in terms of a different set of in-scope type parameters, which is particularly bad since the mir body will be in terms of one and the MIR signatures in terms of the other.

There is no "easy" fix here, but there are some refatorings to be done.

One very simple fix is to (a) not generate drop-glue variants until we have a "fully monomorphic" (or "ground") type that never requires substitution. This is #69036. This "staves off" the problem by not creating the problematic InstanceDef and leaving things unresolved.

Another might be refactoring how we represent types to be more like chalk, where we would have some type ApplicationTy that encapsulates "all rust types", more or less, and which factors out the "substitutions" for them. Then the drop glue could include ApplicationTy and not a fully general type (and hence wouldn't require substitution).

Have to stop now, but there would be other possibilities too. (e.g., extending DefId to subsume IsntanceDef)

@eddyb

This comment has been minimized.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Mar 11, 2020

If you mean calling .subst on Instance that's a different bug that we should fix and that I wasn't aware of.

No, I think I was wrong and didn't quite understand correctly (good to write things out, I suppose). In particular, Instance does implement TypeFoldable, but it does fold the types in the def, so the mismatch I was referring to doesn't quite happen that way.

In any case, the core problem is still (in my mind) one of namespaces:

The "public signature" of an Instance is expressed in terms of a def-id which carries attached generics G, and the substs are the values for those generic parameters (in terms, potentially, of some other set of in-scope generics G1).

In some cases, the body of the MIR is also expressed in terms of the generics G.

But in other cases (notably drop-glue) the body of the MIR is expressed in terms of the generics G1.

This is causing use ICEs and confusion because we do the wrong substitutions. In particular, the example of Vec<T> being substituted with T => Vec<T>, and leading to Vec<Vec<T>> is caused by treating the body as if it was in terms of G when it is in terms of G1 (and hence no substitution is required to bring it from G to G1).

For the moment, we are documenting this situation and working around it, but not changing the underlying mismatch. Fixing that underlying mismatch is desirable but hard -- i.e., a longer term proposition -- and that is precisely what this issue represents.

Centril added a commit to Centril/rust that referenced this issue Mar 19, 2020
codegen/mir: support polymorphic `InstanceDef`s

cc rust-lang#69925

This PR modifies the use of `subst_and_normalize_erasing_regions` on parts of the MIR bodies returned from `instance_mir`, so that `InstanceDef::CloneShim` and `InstanceDef::DropGlue` (where there is a type) do not perform substitutions. This avoids double substitutions and enables polymorphic `InstanceDef`s.

r? @eddyb
cc @nikomatsakis
JohnTitor added a commit to JohnTitor/rust that referenced this issue Mar 20, 2020
codegen/mir: support polymorphic `InstanceDef`s

cc rust-lang#69925

This PR modifies the use of `subst_and_normalize_erasing_regions` on parts of the MIR bodies returned from `instance_mir`, so that `InstanceDef::CloneShim` and `InstanceDef::DropGlue` (where there is a type) do not perform substitutions. This avoids double substitutions and enables polymorphic `InstanceDef`s.

r? @eddyb
cc @nikomatsakis
@davidtwco davidtwco added the -Zpolymorphize Unstable option: Polymorphization. label Aug 9, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 19, 2020
…nstancedef-fnptrshim, r=nikomatsakis

shim: monomorphic `FnPtrShim`s during construction

Fixes rust-lang#69925.

This PR adjusts MIR shim construction so that substitutions are applied to function pointer shims during construction, rather than during codegen (as determined by `substs_for_mir_body`).

r? @eddyb
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 19, 2020
…nstancedef-fnptrshim, r=nikomatsakis

shim: monomorphic `FnPtrShim`s during construction

Fixes rust-lang#69925.

This PR adjusts MIR shim construction so that substitutions are applied to function pointer shims during construction, rather than during codegen (as determined by `substs_for_mir_body`).

r? @eddyb
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 19, 2020
…nstancedef-fnptrshim, r=nikomatsakis

shim: monomorphic `FnPtrShim`s during construction

Fixes rust-lang#69925.

This PR adjusts MIR shim construction so that substitutions are applied to function pointer shims during construction, rather than during codegen (as determined by `substs_for_mir_body`).

r? @eddyb
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 20, 2020
…nstancedef-fnptrshim, r=nikomatsakis

shim: monomorphic `FnPtrShim`s during construction

Fixes rust-lang#69925.

This PR adjusts MIR shim construction so that substitutions are applied to function pointer shims during construction, rather than during codegen (as determined by `substs_for_mir_body`).

r? @eddyb
@bors bors closed this as completed in a3bc0e7 Sep 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-Zpolymorphize Unstable option: Polymorphization. A-codegen Area: Code generation A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants