Stream: t-compiler/wg-polymorphization

Topic: the problem with drop glue and substs


nikomatsakis (Mar 11 2020 at 14:49, on Zulip):

Well, let me fork out a topic to make sure I understand what the problem is

nikomatsakis (Mar 11 2020 at 14:49, on Zulip):

but not conflate that discussion with what @davidtwco and @eddyb are saying right now

nikomatsakis (Mar 11 2020 at 14:49, on Zulip):

So e.g. imagine we are generating drop-glue for Foo<Bar<T>> or something

nikomatsakis (Mar 11 2020 at 14:49, on Zulip):

is the problem that we our "substs" would have Bar<T> in it

nikomatsakis (Mar 11 2020 at 14:50, on Zulip):

but when we generate the MIR body, we encode some facts of Bar in there?

nikomatsakis (Mar 11 2020 at 14:50, on Zulip):

i.e., should we be generating drop-glue for Foo<T> for "any T" (And presumably recursively invoking other drop glue for the T, in a way that would be resolved later on)

nikomatsakis (Mar 11 2020 at 14:53, on Zulip):

eddyb said:

nikomatsakis the problem is that InstanceDef::DropGlue contains a type with parameters in it, and that ends up in the MIR body for the shim, but those parameters shouldn't be substituted by Instance's substs (which are for the DefId that we resolved to a shim, not for the shim itself)

I feel like @eddyb that InstanceDef shouldn't really have a type in it

nikomatsakis (Mar 11 2020 at 14:54, on Zulip):

which might also be your point

eddyb (Mar 11 2020 at 14:54, on Zulip):

that's generally impossible

eddyb (Mar 11 2020 at 14:54, on Zulip):

or, like, hard

eddyb (Mar 11 2020 at 14:54, on Zulip):

and it doesn't solve the fact that you now need two substs

eddyb (Mar 11 2020 at 14:54, on Zulip):

one for the InstanceDef and one for the DefId

nikomatsakis (Mar 11 2020 at 14:54, on Zulip):

why do you need two substs?

eddyb (Mar 11 2020 at 14:55, on Zulip):

like I said before, the signature comes from (instance.def_id(), instance.substs) but the MIR body is the one that's unsubstitutable for shims

nikomatsakis (Mar 11 2020 at 14:55, on Zulip):

and why is it impossible?

eddyb (Mar 11 2020 at 14:55, on Zulip):

because many of these shims have arbitrary types

eddyb (Mar 11 2020 at 14:56, on Zulip):

the fn pointers ones support higher-ranked lifetimes IIRC

eddyb (Mar 11 2020 at 14:56, on Zulip):

it's hard to make a "skeleton" of the type, and potentially impossible

eddyb (Mar 11 2020 at 14:56, on Zulip):

(this is what I should've said in the first place)

eddyb (Mar 11 2020 at 14:56, on Zulip):

it's not like other cases where the skeleton is just a DefId that points to a generic definition

nikomatsakis (Mar 11 2020 at 14:57, on Zulip):

I don't buy it yet :)

nikomatsakis (Mar 11 2020 at 14:57, on Zulip):

For one thing, we could move those types to the substs

eddyb (Mar 11 2020 at 14:57, on Zulip):

the substs are for the DefId

nikomatsakis (Mar 11 2020 at 14:57, on Zulip):

If necessary

eddyb (Mar 11 2020 at 14:58, on Zulip):

okay so keep in mind that the whole reason for this weird setup is the MIR shim body is keyed by instance.def

eddyb (Mar 11 2020 at 14:58, on Zulip):

as in InstanceDef is what you get a MIR body from

eddyb (Mar 11 2020 at 14:58, on Zulip):

and then the substs only really make sense for InstanceDef::Item but we currently apply to everything which is fine because most shim InstanceDefs are fully monomorphic

eddyb (Mar 11 2020 at 14:59, on Zulip):

there are two competing interests: signature (obtained from DefId always) and body (obtained from InstanceDef)

eddyb (Mar 11 2020 at 14:59, on Zulip):

they can't possibly have the same Substs

nikomatsakis (Mar 11 2020 at 14:59, on Zulip):

well, they could, but I don't know if it matters

nikomatsakis (Mar 11 2020 at 15:00, on Zulip):

i.e., you could extend the substs of the body w/ add'l parameters

nikomatsakis (Mar 11 2020 at 15:00, on Zulip):

eddyb said:

as in InstanceDef is what you get a MIR body from

but this helps, I was missing this bit of context

nikomatsakis (Mar 11 2020 at 15:00, on Zulip):

(also, side note that I still can't find where we actually create InstanceDef::DropGlue, I guess it must be in the instance resolve code though..?)

eddyb (Mar 11 2020 at 15:00, on Zulip):

okay I should've just said fn mir_shim_body(InstanceDef) -> mir::Body :P

eddyb (Mar 11 2020 at 15:01, on Zulip):

yeah that code used to be in ty::instance but was moved to rustc_ty::instance

davidtwco (Mar 11 2020 at 15:01, on Zulip):

nikomatsakis said:

(also, side note that I still can't find where we actually create InstanceDef::DropGlue, I guess it must be in the instance resolve code though..?)

davidtwco said:

  1. https://github.com/rust-lang/rust/blob/303d8aff6092709edd4dbd35b1c88e9aa40bf6d8/src/librustc_mir/monomorphize/collector.rs#L615-L624
  2. https://github.com/rust-lang/rust/blob/303d8aff6092709edd4dbd35b1c88e9aa40bf6d8/src/librustc_mir/monomorphize/collector.rs#L650-L658
  3. https://github.com/rust-lang/rust/blob/303d8aff6092709edd4dbd35b1c88e9aa40bf6d8/src/librustc/ty/instance.rs#L340-L344
  4. https://github.com/rust-lang/rust/blob/303d8aff6092709edd4dbd35b1c88e9aa40bf6d8/src/librustc_ty/instance.rs#L35-L44
nikomatsakis (Mar 11 2020 at 15:01, on Zulip):

oh, that's what those links were :)

nikomatsakis (Mar 11 2020 at 15:01, on Zulip):

oh, I see, I overlooked the first because .. well, I was confused :)

nikomatsakis (Mar 11 2020 at 15:02, on Zulip):

well, no, because only the last actually creates a DropGlue

eddyb (Mar 11 2020 at 15:02, on Zulip):

you can also look at https://github.com/rust-lang/rust/pull/69036/files :P

davidtwco (Mar 11 2020 at 15:02, on Zulip):

Yeah, only the last, that's just how we get there.

nikomatsakis (Mar 11 2020 at 15:03, on Zulip):

so maybe I'll go back and re-read what y'all said elsewhere. definitely something is still "bothering me" here

eddyb (Mar 11 2020 at 15:04, on Zulip):

the situation is definitely suboptimal

eddyb (Mar 11 2020 at 15:04, on Zulip):

but there's only ~one easy fix

nikomatsakis (Mar 11 2020 at 15:04, on Zulip):

eddyb said:

you still need Instance to have substs because we still rely on (instance.def_id(), instance.substs) for e.g. getting the signature

though I understand this comment now a bit better. I guess this partly just seems wrong to me

nikomatsakis (Mar 11 2020 at 15:05, on Zulip):

but I probably just need to adjust my thinking

eddyb (Mar 11 2020 at 15:05, on Zulip):

there are two sources of truth and it would be a big refactor (if at all possible) to improve that

eddyb (Mar 11 2020 at 15:05, on Zulip):

we would need to get rid of the Instance::def_id method probably

eddyb (Mar 11 2020 at 15:05, on Zulip):

which today can never fail

nikomatsakis (Mar 11 2020 at 15:07, on Zulip):

ok, so, @eddyb, you're proposed fix is to not substitute MIR bodies for things that are not InstanceDef::Item...? @davidtwco can you spell out what the ICE is exactly? oh, maybe I see -- you're basically saying that types that appear in MIR bodies that are not the "item" case are not referencing the substs

eddyb (Mar 11 2020 at 15:07, on Zulip):

right

nikomatsakis (Mar 11 2020 at 15:07, on Zulip):

(and I see how this fits into your other PR)

nikomatsakis (Mar 11 2020 at 15:08, on Zulip):

and thus why they must be monomorphic for that to make any sense

davidtwco (Mar 11 2020 at 15:08, on Zulip):
error: internal compiler error: src/librustc/ty/subst.rs:565: type parameter `S/#1` (S/1) out of range when substituting (root type=Some(for<'r> fn(&'r mut OnDrop<[closure@/home/david/projects/rust/rust0/src/test/ui/polymorphization/foo.rs:14:24: 14:29]>) {<OnDrop<[closure@/home/david/projects/rust/rust0/src/test/ui/polymorphization/foo.rs:14:24: 14:29]> as std::ops::Drop>::drop})) substs=[OnDrop<[closure@/home/david/projects/rust/rust0/src/test/ui/polymorphization/foo.rs:14:24: 14:29]>]

(this is the ICE that we get for the simple test case w/ my PR)

nikomatsakis (Mar 11 2020 at 15:08, on Zulip):

I guess I'm on board with a surgical fix, presuming it works, but the setup does seem to be itching for a refactoring.

davidtwco (Mar 11 2020 at 15:08, on Zulip):

(from this line, when calling the drop_in_place shim)

nikomatsakis (Mar 11 2020 at 15:09, on Zulip):

However, i'm a bit confused about something else

nikomatsakis (Mar 11 2020 at 15:09, on Zulip):

In the example, when we compile it generally, we'll wind up with something like Drop<$ClosureType<R, S>>, right?

nikomatsakis (Mar 11 2020 at 15:10, on Zulip):

and we'll (presumably) see a call to drop_in_place on that type

eddyb (Mar 11 2020 at 15:11, on Zulip):

opened https://github.com/rust-lang/rust/issues/69925

nikomatsakis (Mar 11 2020 at 15:11, on Zulip):

so @eddyb your PR which refuses to "resolve" drop-glue for non-monomorphic types, I guess that this would conflict

eddyb (Mar 11 2020 at 15:11, on Zulip):

first line of the issue explains that the PR only "papers over" the problem :P

nikomatsakis (Mar 11 2020 at 15:12, on Zulip):

what is the def-id for drop-glue?

nikomatsakis (Mar 11 2020 at 15:12, on Zulip):

is it the def-id of "drop in place"?

davidtwco (Mar 11 2020 at 15:12, on Zulip):

I think so

eddyb (Mar 11 2020 at 15:12, on Zulip):

core::mem::drop_in_place

nikomatsakis (Mar 11 2020 at 15:16, on Zulip):

ok, so, one "more proper" refactoring would be to extend InstanceDef with variants for all the major categories -- e.g., DropAdt(DefId), DropTrivial, DropClosure -- that's... probably it, actually. Separately, if types were somewhat better factored (actually, more the way that chalk factors them), then we might just have DropType(ApplicationTy)

nikomatsakis (Mar 11 2020 at 15:17, on Zulip):

which would capture the "essential bits" that we need to generate the code, with everything else coming from the substs

nikomatsakis (Mar 11 2020 at 15:17, on Zulip):

(not proposing we do that now, just trying to think what I would expect from the system)

eddyb (Mar 11 2020 at 15:21, on Zulip):

yeah but you forgot tuples and arrays :P

eddyb (Mar 11 2020 at 15:21, on Zulip):

there's a reason I'm saying this isn't that easy

eddyb (Mar 11 2020 at 15:22, on Zulip):

and all of this requires Instance::def_id to die too, which is a potentially large amount of work

nikomatsakis (Mar 11 2020 at 15:24, on Zulip):

eddyb said:

yeah but you forgot tuples and arrays :P

sure, but application ty in chalk covers those :)

nikomatsakis (Mar 11 2020 at 15:24, on Zulip):

eddyb said:

and all of this requires Instance::def_id to die too, which is a potentially large amount of work

why?

nikomatsakis (Mar 11 2020 at 15:24, on Zulip):

well, ok, I guess I see why

nikomatsakis (Mar 11 2020 at 15:24, on Zulip):

i.e., code that assumes a def-id covers the signature really wants to be using InstanceDef, since there are cases (like the above) that don't have a def-id right now

eddyb (Mar 11 2020 at 15:24, on Zulip):

because everything relies on (instance.def_id(), instance.substs) for everything other than the body

nikomatsakis (Mar 11 2020 at 15:24, on Zulip):

another approach that I was considering woul be to extsend the concept of def-id with those things

nikomatsakis (Mar 11 2020 at 15:25, on Zulip):

which might be better

eddyb (Mar 11 2020 at 15:25, on Zulip):

something something new style intrinsics

eddyb (Mar 11 2020 at 15:25, on Zulip):

which drop_in_place technically is

eddyb (Mar 11 2020 at 15:25, on Zulip):

but you need variadic generics for tuples

eddyb (Mar 11 2020 at 15:25, on Zulip):

at least arrays are doable now yay

eddyb (Mar 11 2020 at 15:26, on Zulip):

@nikomatsakis btw does this solve your concerns? https://github.com/rust-lang/rust/pull/69036/commits/7727bd9d3053743d0dbb885a0c5e2cc19efa095d

nikomatsakis (Mar 11 2020 at 15:30, on Zulip):

eddyb said:

but you need variadic generics for tuples

no, you just need Tuple(usize) as the "instance-def", so to speak

nikomatsakis (Mar 11 2020 at 15:30, on Zulip):

though variadic generics would work too :)

eddyb (Mar 11 2020 at 15:30, on Zulip):

if not clear I want a definition in libcore that's both a lang item and an intrinsic at the same time :P

nikomatsakis (Mar 11 2020 at 15:31, on Zulip):

what does libcore have to do with anything?

eddyb (Mar 11 2020 at 15:31, on Zulip):

if it even needs to be an intrinsic

nikomatsakis (Mar 11 2020 at 15:31, on Zulip):

I also continue to not understand the difference

nikomatsakis (Mar 11 2020 at 15:31, on Zulip):

between a lang-item and an intrinsic

nikomatsakis (Mar 11 2020 at 15:31, on Zulip):

and I feel they should be unified :)

eddyb (Mar 11 2020 at 15:31, on Zulip):

it's very much a 1 vs many thing

nikomatsakis (Mar 11 2020 at 15:31, on Zulip):

it seems like both are "things well known to the compiler"

eddyb (Mar 11 2020 at 15:31, on Zulip):

or export vs import

eddyb (Mar 11 2020 at 15:32, on Zulip):

and the reason I want it in libcore is because crateless DefIds are a pandora's box I don't want to open

nikomatsakis (Mar 11 2020 at 15:32, on Zulip):

what is "it" even here?

nikomatsakis (Mar 11 2020 at 15:32, on Zulip):

ok, I guses I see

nikomatsakis (Mar 11 2020 at 15:32, on Zulip):

I guess the question is whether every DefId has to "originate" with some source

nikomatsakis (Mar 11 2020 at 15:32, on Zulip):

which I was not assuming

nikomatsakis (Mar 11 2020 at 15:33, on Zulip):

but even if it is technically associated with libcore

eddyb (Mar 11 2020 at 15:33, on Zulip):

just like removing Instance::def_id, it has a large amounts of potential ramifications

nikomatsakis (Mar 11 2020 at 15:33, on Zulip):

it doesn't necessarily have to tie to some declared item

nikomatsakis (Mar 11 2020 at 15:33, on Zulip):

anyway it's fine, I don't care to debate it right now

nikomatsakis (Mar 11 2020 at 15:33, on Zulip):

though this does seem like a fruitful area to talk over at some point

eddyb (Mar 11 2020 at 15:34, on Zulip):

me neither, I'm wasting my time on this, I just want to point out how nontrivial some of this stuff is

eddyb (Mar 11 2020 at 15:34, on Zulip):

as much as I want reforms for every single thing you brought up

nikomatsakis (Mar 11 2020 at 15:34, on Zulip):

I feel like library-ification can help as a forcing function in some sense, though it's orthogonal

nikomatsakis (Mar 11 2020 at 15:35, on Zulip):

because I'd like us to be establishing "core concepts" that are as clean-and-simple and uniform as we can make them

nikomatsakis (Mar 11 2020 at 15:35, on Zulip):

anyway definitely non-trivial

Last update: Apr 03 2020 at 18:20UTC