Stream: wg-traits

Topic: object upcasting


nikomatsakis (Apr 08 2019 at 17:09, on Zulip):

Hey @Alexander Regueiro -- I was thinking more about dyn Trait upcasting while I was away. Not sure if you are still looking for a "next task", but I think that is what we should consider. It is basically a real pain that we cannot do it =)

Alexander Regueiro (Apr 08 2019 at 17:10, on Zulip):

@nikomatsakis Yep! I've had nothing serious on my plate for about a week now (mainly waiting on feedback), so that sounds cool.

nikomatsakis (Apr 08 2019 at 17:10, on Zulip):

I also don't think it's all that complex a problem.

nikomatsakis (Apr 08 2019 at 17:10, on Zulip):

I guess the way to start would be by doing some review of how the current setup works etc

nikomatsakis (Apr 08 2019 at 17:10, on Zulip):

(TBH, I wonder if it's worth making this a working group of its own)

nikomatsakis (Apr 08 2019 at 17:11, on Zulip):

maybe, maybe not

Alexander Regueiro (Apr 08 2019 at 17:11, on Zulip):

had a question for you about a) maybe bounds in trait objects, b) RPIT existential lifetimes (can wait a bit maybe)

Alexander Regueiro (Apr 08 2019 at 17:11, on Zulip):

yeah... not sure, honestly. I'm up for it if you are though.

Alexander Regueiro (Apr 08 2019 at 17:11, on Zulip):

shall we start a HackMD doc at least?

Alexander Regueiro (Apr 08 2019 at 17:11, on Zulip):

or similar

Alexander Regueiro (Apr 08 2019 at 17:15, on Zulip):

@nikomatsakis ^

nikomatsakis (Apr 08 2019 at 17:17, on Zulip):

shall we start a HackMD doc at least?

start a doc for upcasting?

Alexander Regueiro (Apr 08 2019 at 17:17, on Zulip):

@nikomatsakis yeah

nikomatsakis (Apr 08 2019 at 17:17, on Zulip):

seems reasonable

nikomatsakis (Apr 08 2019 at 17:17, on Zulip):

there is a bit of prior work we can research in terms of vtable layout,

Alexander Regueiro (Apr 08 2019 at 17:17, on Zulip):

we can summarise the current state

Alexander Regueiro (Apr 08 2019 at 17:18, on Zulip):

then part B can be an action plan

nikomatsakis (Apr 08 2019 at 17:18, on Zulip):

but I think ultimately it's not that important what we settle on here

Alexander Regueiro (Apr 08 2019 at 17:18, on Zulip):

sure

nikomatsakis (Apr 08 2019 at 17:18, on Zulip):

(as long as we leave room for us to change later)

nikomatsakis (Apr 08 2019 at 17:18, on Zulip):

what we probably want to do is to try and schedule a time to draw up an action plan with first few steps

nikomatsakis (Apr 08 2019 at 17:18, on Zulip):

maybe we can get @eddyb involved

Alexander Regueiro (Apr 08 2019 at 17:19, on Zulip):

yeah that would be good, if he's free

Alexander Regueiro (Apr 08 2019 at 17:19, on Zulip):

okay, we have a Zoom meeting to kick things off... got any suggestions for when?

Alexander Regueiro (Apr 08 2019 at 17:20, on Zulip):

tomorrow is difficult for me, but other than that, afternoons should be fine...

nikomatsakis (Apr 08 2019 at 17:49, on Zulip):

@Alexander Regueiro can you make a doodle with some options that work for you?

Alexander Regueiro (Apr 08 2019 at 22:16, on Zulip):

Sure, here it is: https://doodle.com/poll/7xdsf8rcy4g989v7 @nikomatsakis

Alexander Regueiro (Apr 08 2019 at 22:16, on Zulip):

CC @dhardy @Guillaume (people whom I know have interest in this)

GuillaumeGomez (Apr 09 2019 at 06:47, on Zulip):

Here I am!

Alexander Regueiro (Apr 09 2019 at 14:19, on Zulip):

@Guillaume Cool, thanks for the response. Hopefully we'll figure out when Niko and maybe others can make it too.

Alexander Regueiro (Apr 09 2019 at 14:19, on Zulip):

@eddyb Do you want to participate too? :-)

nikomatsakis (Apr 09 2019 at 21:11, on Zulip):

@Alexander Regueiro ok I filled it out

Alexander Regueiro (Apr 09 2019 at 21:12, on Zulip):

@nikomatsakis cheers. we'll go with Fri at 6:00 then probably...

Alexander Regueiro (Apr 09 2019 at 21:12, on Zulip):

unless someone else replies soon

Alexander Regueiro (Apr 09 2019 at 21:12, on Zulip):

sorry if time zone was confusing

nikomatsakis (Apr 09 2019 at 21:55, on Zulip):

Created a calendar event, which includes a zoom link

nikomatsakis (Apr 09 2019 at 21:55, on Zulip):

(Or did we want to do this on Zulip?)

Alexander Regueiro (Apr 09 2019 at 21:57, on Zulip):

Either is fine. What do you think's more efficient? Incidentally, I put 1 hour duration in Doodle, but if you think we only need 45 mins, then that's cool.

Alexander Regueiro (Apr 09 2019 at 21:57, on Zulip):

@nikomatsakis could we get your feedback on https://github.com/rust-lang/rust/issues/59656 at some point soon btw? :-)

Alexander Regueiro (Apr 09 2019 at 21:58, on Zulip):

Created a calendar event, which includes a zoom link

CC @Guillaume

centril (Apr 10 2019 at 00:22, on Zulip):

I might attend; can you add the calendar link to t-compiler's calendar? or t-lang's

nikomatsakis (Apr 10 2019 at 09:18, on Zulip):

it is on the compiler calendar. Though your comment confused me :P and I might have messed up the link just now, if so, here is another one.

centril (Apr 10 2019 at 14:30, on Zulip):

oh; I might have looked at the wrong week :P

Alexander Regueiro (Apr 10 2019 at 15:11, on Zulip):

it's this week :-P

centril (Apr 10 2019 at 15:23, on Zulip):

@Alexander Regueiro yeah I looked 1 week into the future

Alexander Regueiro (Apr 10 2019 at 15:23, on Zulip):

heh okay

GuillaumeGomez (Apr 10 2019 at 18:08, on Zulip):

I should be able to attend. I added the event to my calendar. :)

nikomatsakis (Apr 12 2019 at 15:56, on Zulip):

Hey @Alexander Regueiro (cc @Guillaume, @centril, @eddyb, whomever else) -- I had made this a Zoom meeting, but I was wondering if we really want that? I've found Zulip often works as well or better. I'm inclined probably to do it over Zulip myself.

nikomatsakis (Apr 12 2019 at 15:57, on Zulip):

(To be clear, it doesn't start for a few hours, I just wanted to check because -- if so -- I'll edit the event)

Alexander Regueiro (Apr 12 2019 at 16:00, on Zulip):

Zulip is fine with me too, yeah

Alexander Regueiro (Apr 12 2019 at 16:00, on Zulip):

The time was set for now, I thought, @nikomatsakis...

Alexander Regueiro (Apr 12 2019 at 16:03, on Zulip):

at least that's what's in the calendar

nikomatsakis (Apr 12 2019 at 16:06, on Zulip):

the calendar says in 1 hr?

Alexander Regueiro (Apr 12 2019 at 16:07, on Zulip):

hmm, not for me

Alexander Regueiro (Apr 12 2019 at 16:07, on Zulip):

weird

Alexander Regueiro (Apr 12 2019 at 16:07, on Zulip):

anyway, 1hr is fine

Alexander Regueiro (Apr 12 2019 at 16:07, on Zulip):

no worries

Alexander Regueiro (Apr 12 2019 at 16:07, on Zulip):

maybe iCal confused the timezones

Alexander Regueiro (Apr 12 2019 at 16:18, on Zulip):

@nikomatsakis in the meanwhile, did you see the GitHub issue about maybe bounds in trait objects?

Alexander Regueiro (Apr 12 2019 at 16:18, on Zulip):

@nikomatsakis it's part of the PR I made for enforcing single-trait objects with trait aliases. I did some other things like removed the concept of principal traits, which arielby never got around to finishing.

Alexander Regueiro (Apr 12 2019 at 16:19, on Zulip):

this is the last remaining thing...

GuillaumeGomez (Apr 12 2019 at 16:39, on Zulip):

for me it's in 21 min

centril (Apr 12 2019 at 16:46, on Zulip):

@nikomatsakis I would prefer Zoom; I find that you often get more done via audio since you don't have to think and type :P

Alexander Regueiro (Apr 12 2019 at 16:50, on Zulip):

Let's do Zulip though, sorry Centril. Niko is leading this meeting so if he slightly prefers that it's probably best... I slightly prefer it too.

nikomatsakis (Apr 12 2019 at 16:50, on Zulip):

nikomatsakis in the meanwhile, did you see the GitHub issue about maybe bounds in trait objects?

I did not

nikomatsakis (Apr 12 2019 at 16:51, on Zulip):

Link?

centril (Apr 12 2019 at 16:51, on Zulip):

https://github.com/rust-lang/rust/issues/59656

Alexander Regueiro (Apr 12 2019 at 16:55, on Zulip):

@centril thanks

GuillaumeGomez (Apr 12 2019 at 16:59, on Zulip):

Should we start?

nikomatsakis (Apr 12 2019 at 17:01, on Zulip):

Yep, let's start

nikomatsakis (Apr 12 2019 at 17:01, on Zulip):

Hey @WG-traits -- we're going to talk about trait upcasting (cc @eddyb if you're around and interested)

nikomatsakis (Apr 12 2019 at 17:02, on Zulip):

First, to set the stage

nikomatsakis (Apr 12 2019 at 17:02, on Zulip):

Let me clarify what I think we're talking about :)

Alexander Regueiro (Apr 12 2019 at 17:02, on Zulip):

yep...

nikomatsakis (Apr 12 2019 at 17:02, on Zulip):

Or perhaps what we're not talking about

nikomatsakis (Apr 12 2019 at 17:02, on Zulip):

I think we are not talking about supporting dyn Foo + Bar for arbitrary Foo and Bar

nikomatsakis (Apr 12 2019 at 17:02, on Zulip):

That is, the definition of a trait object type as dyn Principal + AutoTraits would not be altered

nikomatsakis (Apr 12 2019 at 17:02, on Zulip):

But we would permit upcasting to supertraits via a coercion

nikomatsakis (Apr 12 2019 at 17:03, on Zulip):

so e.g. if you had trait Foo: Bar + Baz, then dyn Foo could be coerced to dyn Bar or dyn Baz

nikomatsakis (Apr 12 2019 at 17:03, on Zulip):

if we later add support for dyn Bar + Baz, of course we would want to permit coercion to that too

nikomatsakis (Apr 12 2019 at 17:03, on Zulip):

or perhaps I should say when

nikomatsakis (Apr 12 2019 at 17:03, on Zulip):

since I am assuming we will do so at some point

nikomatsakis (Apr 12 2019 at 17:03, on Zulip):

but I think that's a more complex topic

nikomatsakis (Apr 12 2019 at 17:03, on Zulip):

Does everybody agree with this so far? :)

centril (Apr 12 2019 at 17:04, on Zulip):

@nikomatsakis don't we permit dyn Principal + Auto ~> dyn Principal today?

nikomatsakis (Apr 12 2019 at 17:04, on Zulip):

We may, I forget.

nikomatsakis (Apr 12 2019 at 17:04, on Zulip):

Good to clarify.

nikomatsakis (Apr 12 2019 at 17:04, on Zulip):

Also, if we do, if we do it via coercion or subtyping (internally to the compiler).

centril (Apr 12 2019 at 17:05, on Zulip):

(and maybe dyn Principal + Auto ~> dyn Auto but I don't think we allow just auto traits...)

nikomatsakis (Apr 12 2019 at 17:06, on Zulip):

this builds:

trait Foo { }

fn coerce(x: &(dyn Foo + Send)) {
    let _: &dyn Foo = x;
    // let _: &dyn Send = x;
}

fn main() { }
nikomatsakis (Apr 12 2019 at 17:06, on Zulip):

but the commented line does not

nikomatsakis (Apr 12 2019 at 17:07, on Zulip):

you get this:

Standard Error

   Compiling playground v0.0.1 (/playground)
error[E0308]: mismatched types
 --> src/main.rs:5:24
  |
5 |     let _: &dyn Send = x;
  |                        ^ expected trait `std::marker::Send`, found trait `Foo + std::marker::Send`
  |
  = note: expected type `&dyn std::marker::Send`
             found type `&dyn Foo + std::marker::Send`
Alexander Regueiro (Apr 12 2019 at 17:07, on Zulip):

@nikomatsakis Yep I agree.

nikomatsakis (Apr 12 2019 at 17:07, on Zulip):

I suspect this is because we have "broken-ish" code doing that coercion

Alexander Regueiro (Apr 12 2019 at 17:07, on Zulip):

multi-trait objects can come later. it's a natural progression, but this is the natural first step.

Alexander Regueiro (Apr 12 2019 at 17:07, on Zulip):

I see

nikomatsakis (Apr 12 2019 at 17:07, on Zulip):

basically I suspect we permit reducing the set of auto traits, but not changing the principal (in this case, from something to nothing)

nikomatsakis (Apr 12 2019 at 17:07, on Zulip):

but let's find the actual code

Alexander Regueiro (Apr 12 2019 at 17:08, on Zulip):

@nikomatsakis I wonder if the PR you just looked at happens to fix that... since it removes principals as a concept ;-)

Alexander Regueiro (Apr 12 2019 at 17:08, on Zulip):

I could test now actually heh

centril (Apr 12 2019 at 17:09, on Zulip):

@nikomatsakis another thing: type constructors seem relevant here, because I assume were are not going to allow F<dyn Sub> ~> F<dyn Super> for arbitrary F -- but still we do want to allow Box<dyn Sub> ~> Box<dyn Super>

centril (Apr 12 2019 at 17:09, on Zulip):

IOW, dyn Sub <: dyn Super does not hold

Alexander Regueiro (Apr 12 2019 at 17:09, on Zulip):

ah nope, same error in the build for that PR

nikomatsakis (Apr 12 2019 at 17:10, on Zulip):

but let's find the actual code

this is the subtyping code for trait objects, unless I'm confused it seems to require an exact match, so this is presumably a coercion

nikomatsakis (Apr 12 2019 at 17:11, on Zulip):

Yes, the coercion is here, and it says:

nikomatsakis (Apr 12 2019 at 17:11, on Zulip):
 // Upcasts permit two things:
                //
                // 1. Dropping builtin bounds, e.g., `Foo+Send` to `Foo`
                // 2. Tightening the region bound, e.g., `Foo+'a` to `Foo+'b` if `'a : 'b`
                //
                // Note that neither of these changes requires any
                // change at runtime.  Eventually this will be
                // generalized.
                //
                // We always upcast when we can because of reason
// #2 (region bounds).
nikomatsakis (Apr 12 2019 at 17:11, on Zulip):

nikomatsakis another thing: type constructors seem relevant here, because I assume were are not going to allow F<dyn Sub> ~> F<dyn Super> for arbitrary F -- but still we do want to allow Box<dyn Sub> ~> Box<dyn Super>

this should just fall out from the unsizing rules, @centril

centril (Apr 12 2019 at 17:11, on Zulip):

Yeah that seems reasonable

centril (Apr 12 2019 at 17:12, on Zulip):

this should just fall out from the unsizing rules, centril

Also reasonable :slight_smile:

nikomatsakis (Apr 12 2019 at 17:12, on Zulip):

basically, any place that we would permit the existing upcast (which is tied to unsizing), we would also permit the trait object upcast

nikomatsakis (Apr 12 2019 at 17:12, on Zulip):

the key point is that doing this upcast may change the vtable

nikomatsakis (Apr 12 2019 at 17:12, on Zulip):

so it is definitely not subtyping in the compiler's sense of the word

nikomatsakis (Apr 12 2019 at 17:12, on Zulip):

so maybe we turn to that for a second, that is, how I would expect to implement

nikomatsakis (Apr 12 2019 at 17:13, on Zulip):

my expectation is that we will basically have the vtable for a given trait

nikomatsakis (Apr 12 2019 at 17:13, on Zulip):

include pointers to the vtables for its supertraits

nikomatsakis (Apr 12 2019 at 17:13, on Zulip):

we know that, at least presently, the vtables are structured in a tree

nikomatsakis (Apr 12 2019 at 17:13, on Zulip):

er, sorry, supertrait relations are structured in a tree

nikomatsakis (Apr 12 2019 at 17:13, on Zulip):

I'd actually like to lift that restriction

nikomatsakis (Apr 12 2019 at 17:14, on Zulip):

but that seems separable (though maybe worth thinking through how it would work at runtime)

centril (Apr 12 2019 at 17:14, on Zulip):

(btw, do we have an RFC for upcasting?)

nikomatsakis (Apr 12 2019 at 17:14, on Zulip):

not that I know of

nikomatsakis (Apr 12 2019 at 17:14, on Zulip):

I was going to bring up the question of what procedure to follow

nikomatsakis (Apr 12 2019 at 17:14, on Zulip):

it's hard to imagine it being controversial

nikomatsakis (Apr 12 2019 at 17:14, on Zulip):

but I think documenting the plan is good

centril (Apr 12 2019 at 17:15, on Zulip):

ok, it seems to me that an rfc needs to happen at some point

GuillaumeGomez (Apr 12 2019 at 17:15, on Zulip):

Is it required to have one? There doesn't seem to have much debates about the feature itself?

nikomatsakis (Apr 12 2019 at 17:15, on Zulip):

I don't think there's debate, but that's not a reason not to have the RFC

nikomatsakis (Apr 12 2019 at 17:15, on Zulip):

put another way, you won't know if there's debate, until you create a forum for it :)

nikomatsakis (Apr 12 2019 at 17:15, on Zulip):

but I would expect us to move to FCP quickly

GuillaumeGomez (Apr 12 2019 at 17:15, on Zulip):

Fair enough!

nikomatsakis (Apr 12 2019 at 17:15, on Zulip):

it's a good use of the staging mechanism -- the lang design questions are relatively sparse

nikomatsakis (Apr 12 2019 at 17:16, on Zulip):

(it's mostly an impl plan question, and that mostly isn't exposed to users)

centril (Apr 12 2019 at 17:16, on Zulip):

but also, the purpose of RFCs is not just debates, it is also for clearly communicating what we are doing, make sure its well thought, and also spread knowledge

nikomatsakis (Apr 12 2019 at 17:16, on Zulip):

Is it required to have one? There doesn't seem to have much debates about the feature itself?

still I did wonder the same thing to myself :)

nikomatsakis (Apr 12 2019 at 17:16, on Zulip):

but also, the purpose of RFCs is not just debates, it is also for clearly communicating what we are doing, make sure its well thought, and also spread knowledge

yes very much this

nikomatsakis (Apr 12 2019 at 17:16, on Zulip):

include pointers to the vtables for its supertraits

there is another impl route

GuillaumeGomez (Apr 12 2019 at 17:16, on Zulip):

This is another fair point :)

nikomatsakis (Apr 12 2019 at 17:17, on Zulip):

after all the vtable for the trait Foo already contains all the methods for its supertraits

nikomatsakis (Apr 12 2019 at 17:17, on Zulip):

we could lay them out in such a way that

nikomatsakis (Apr 12 2019 at 17:17, on Zulip):

we can compute a vtable for the supertrait by applying an offset to the vtable for Foo

nikomatsakis (Apr 12 2019 at 17:17, on Zulip):

there's a whole bunch of literature on this from the C++ space

nikomatsakis (Apr 12 2019 at 17:17, on Zulip):

I actually don't care much at all what we do here, though I think embedding some pointers for supertraits might be easier

nikomatsakis (Apr 12 2019 at 17:18, on Zulip):

(I definitely don't think we should promise any particular layout to users)

Alexander Regueiro (Apr 12 2019 at 17:18, on Zulip):

@nikomatsakis yeah, I wonder if that will work better when we come to supporting multi-trait objects and their vtables later

centril (Apr 12 2019 at 17:18, on Zulip):

@nikomatsakis imo, we can also iterate on layout

centril (Apr 12 2019 at 17:18, on Zulip):

like, even start with the "dumb" version at first

centril (Apr 12 2019 at 17:19, on Zulip):

trait object layout is probably also not a wg-traits area of expertise?

nikomatsakis (Apr 12 2019 at 17:20, on Zulip):

I don't know why not

nikomatsakis (Apr 12 2019 at 17:20, on Zulip):

but I wouldn't expect this to ultimately be in wg-traits from compiler POV

nikomatsakis (Apr 12 2019 at 17:20, on Zulip):

I think we should just make a WG for this particular effort

Alexander Regueiro (Apr 12 2019 at 17:20, on Zulip):

sounds reasonable

Alexander Regueiro (Apr 12 2019 at 17:21, on Zulip):

wg-subtyping? :-P

centril (Apr 12 2019 at 17:21, on Zulip):

wg-coercion ;)

nikomatsakis (Apr 12 2019 at 17:21, on Zulip):

no :) wg-dyn-upcast :)

nikomatsakis (Apr 12 2019 at 17:21, on Zulip):

(by which I mean I think we should be focused)

nikomatsakis (Apr 12 2019 at 17:22, on Zulip):

anyway, i know I checked in with @eddyb earlier and they seemed to agree with everything we've written so far, so that's good =)

centril (Apr 12 2019 at 17:22, on Zulip):

Here's an idea: 1) work on the type system logic, 2) work on the layout logic, 3) work on an RFC, 4) iterate on 2., 5) stabilize

nikomatsakis (Apr 12 2019 at 17:22, on Zulip):

nikomatsakis imo, we can also iterate on layout

also I meant to say yes to this :) I agree

eddyb (Apr 12 2019 at 17:23, on Zulip):

/me grumbles something about most of the logic being around anyway

centril (Apr 12 2019 at 17:23, on Zulip):

you might even be able to separate 1 & 2 completely by producing some well placed ICEs anyways

eddyb (Apr 12 2019 at 17:23, on Zulip):

like, this isn't even that much work, implementation-wise?

centril (Apr 12 2019 at 17:23, on Zulip):

@eddyb ok, seems like you'll have a full implementation up in a PR tomorrow then ^^

eddyb (Apr 12 2019 at 17:23, on Zulip):

and hasn't been for ages

eddyb (Apr 12 2019 at 17:23, on Zulip):

my issue is I have to do several things already

eddyb (Apr 12 2019 at 17:24, on Zulip):

if not for that, sure

nikomatsakis (Apr 12 2019 at 17:24, on Zulip):

I want to nominate @eddyb to help mentor @Alexander Regueiro =)

eddyb (Apr 12 2019 at 17:24, on Zulip):

the "is supertrait" check is easy, like, copypaste another place that does it already

nikomatsakis (Apr 12 2019 at 17:24, on Zulip):

but I agree it's not all that much work

Alexander Regueiro (Apr 12 2019 at 17:24, on Zulip):

@eddyb don't worry, Niko planned for me to do it anywya. if you want to mentor me, that's cool though.

eddyb (Apr 12 2019 at 17:24, on Zulip):

and in terms of doing it, we already encode several things like these

nikomatsakis (Apr 12 2019 at 17:24, on Zulip):

anyway I know you're busy, leaving a few notes as to what work is needed is fine

eddyb (Apr 12 2019 at 17:24, on Zulip):

lemme grab it

centril (Apr 12 2019 at 17:24, on Zulip):

copypaste another place that does it already

preferably not literally copy-paste, refactoring a bit would be good?

nikomatsakis (Apr 12 2019 at 17:24, on Zulip):

I am also happy to do so

eddyb (Apr 12 2019 at 17:25, on Zulip):

@centril I mean it might be one line :P?

centril (Apr 12 2019 at 17:25, on Zulip):

@eddyb fiiine :P

centril (Apr 12 2019 at 17:26, on Zulip):

so... maybe we can iterate on the RFC together then?

eddyb (Apr 12 2019 at 17:26, on Zulip):

as in, the bulk of the operation is likely already present elsewhere

nikomatsakis (Apr 12 2019 at 17:26, on Zulip):

the RFC does't seem like it should be that hard to write

centril (Apr 12 2019 at 17:26, on Zulip):

@nikomatsakis also, if it is as little work as @eddyb describes then a new WG sounds overkill

nikomatsakis (Apr 12 2019 at 17:26, on Zulip):

(though I would be curious to think through how things would work if supertraits were permitted to be cyclic)

nikomatsakis (Apr 12 2019 at 17:26, on Zulip):

regardles I think we want this upcast :)

eddyb (Apr 12 2019 at 17:26, on Zulip):

ughhh niko how much I hate now you naming a trait resolution thing a "vtable"

eddyb (Apr 12 2019 at 17:26, on Zulip):

even though it's not >:P

nikomatsakis (Apr 12 2019 at 17:26, on Zulip):

(going to be afk for a bit)

eddyb (Apr 12 2019 at 17:27, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc/traits/util.rs#L475

centril (Apr 12 2019 at 17:27, on Zulip):

yeah, the rfc is probably not hard to write, but some busy work

eddyb (Apr 12 2019 at 17:27, on Zulip):

https://github.com/rust-lang/rust/blob/9ebf47851a357faa4cd97f4b1dc7835f6376e639/src/librustc/traits/select.rs#L3028-L3032

eddyb (Apr 12 2019 at 17:28, on Zulip):

we already compute the base of SuperTrait's methods in SubTrait's vtable

eddyb (Apr 12 2019 at 17:28, on Zulip):

like, vtable layout is already done by the trait system

Alexander Regueiro (Apr 12 2019 at 17:29, on Zulip):

ughhh niko how much I hate now you naming a trait resolution thing a "vtable"

this confused the hell out of me for a while haha

eddyb (Apr 12 2019 at 17:29, on Zulip):

you just need to do a + 3 in a strategic place :P

eddyb (Apr 12 2019 at 17:29, on Zulip):

to get trait Foo: Sup1 + Sup2 to reuse the magical 3 for Sup1 but duplicate them for Sup2

centril (Apr 12 2019 at 17:31, on Zulip):

someone needs to write a bunch of tests as well

Alexander Regueiro (Apr 12 2019 at 17:31, on Zulip):

what's this magical 3? :-P

centril (Apr 12 2019 at 17:31, on Zulip):

for the implementation

Alexander Regueiro (Apr 12 2019 at 17:31, on Zulip):

@centril you know that's you ;-)

Alexander Regueiro (Apr 12 2019 at 17:31, on Zulip):

mr. tests

centril (Apr 12 2019 at 17:31, on Zulip):

:sob:

centril (Apr 12 2019 at 17:32, on Zulip):

yeah maybe ;)

centril (Apr 12 2019 at 17:33, on Zulip):

I'll copy the RFC template over to https://github.com/rust-lang/wg-traits and we can work on it there

eddyb (Apr 12 2019 at 17:34, on Zulip):

3 is drop+size+align

nikomatsakis (Apr 12 2019 at 17:35, on Zulip):

(going to be afk for a bit)

sorry, back

centril (Apr 12 2019 at 17:36, on Zulip):

for tests we need to check both the static and dynamic semantics

Alexander Regueiro (Apr 12 2019 at 17:36, on Zulip):

3 is drop+size+align

oh I see. just three usize's or something to offset eh?

centril (Apr 12 2019 at 17:36, on Zulip):

but there's no syntax to check here at least

centril (Apr 12 2019 at 17:37, on Zulip):

actually, we can iterate on tests in the same place?

eddyb (Apr 12 2019 at 17:37, on Zulip):

oh yeaaaah sooo

eddyb (Apr 12 2019 at 17:37, on Zulip):

https://github.com/rust-lang/rust/blob/9ebf47851a357faa4cd97f4b1dc7835f6376e639/src/librustc/traits/select.rs#L3032

nikomatsakis (Apr 12 2019 at 17:38, on Zulip):

actually, we can iterate on tests in the same place?

feel free to make a subdirectory there

eddyb (Apr 12 2019 at 17:41, on Zulip):

replace the .sum() with .fold(None, |i, n| Some(i.map_or(0, |i| i + 3) + n)).unwrap_or(0)

eddyb (Apr 12 2019 at 17:41, on Zulip):

or... something like that

eddyb (Apr 12 2019 at 17:42, on Zulip):

so this is for calling methods on trait objects

Alexander Regueiro (Apr 12 2019 at 17:43, on Zulip):

interesting

eddyb (Apr 12 2019 at 17:43, on Zulip):

for the cast you'd need to do this exact vtable_base thing, to figure out how much you need to offset

Alexander Regueiro (Apr 12 2019 at 17:43, on Zulip):

do you want to write this up into a few notes on GH or HackMD when you have a moment?

Alexander Regueiro (Apr 12 2019 at 17:43, on Zulip):

then I can have a go maybe

eddyb (Apr 12 2019 at 17:43, on Zulip):

I mean, this is it?

centril (Apr 12 2019 at 17:43, on Zulip):

(though I would be curious to think through how things would work if supertraits were permitted to be cyclic)

@nikomatsakis maybe let's dig into this?

eddyb (Apr 12 2019 at 17:43, on Zulip):

basically there's 3 more entries before each supertrait other than the first one

eddyb (Apr 12 2019 at 17:44, on Zulip):

that's all my fold does

Alexander Regueiro (Apr 12 2019 at 17:44, on Zulip):

Okay, I can have a go then... will need to reread later though

Alexander Regueiro (Apr 12 2019 at 17:44, on Zulip):

basically there's 3 more entries before each supertrait other than the first one

what do you mean?

centril (Apr 12 2019 at 17:46, on Zulip):

@nikomatsakis

Since we don't have that default in the context of trait objects, it makes no sense to suppress it (particularly since it would be an error).

It sounds like you are saying that dyn ?Sized should still be an error, right?

nikomatsakis (Apr 12 2019 at 17:47, on Zulip):

I would make it an error, yes. Or at least I see no reason to accept it, though I guess it's also not harmful to do so.

eddyb (Apr 12 2019 at 17:47, on Zulip):

like, if you have traits Sup1 { a, b }, Sup2 { c, d } and Sub: Sup1 + Sup2 { e, f } (those are methods)

eddyb (Apr 12 2019 at 17:47, on Zulip):

right now the vtable (ignoring the first 3) is a b c d e f

nikomatsakis (Apr 12 2019 at 17:48, on Zulip):

maybe let's dig into this?

@centril thinking more about it, I don't really see that there would be a problem -- if you had each vtable having pointer to the parent vtables, then you'd just have a cycle amongst them, for example.

eddyb (Apr 12 2019 at 17:48, on Zulip):

I'm saying it becomes a b D S A c d e f

nikomatsakis (Apr 12 2019 at 17:49, on Zulip):

I've not been closely following what @eddyb is suggesting, it sounds maybe like they're suggesting an alternative approach in which you adjust the pointer for methods to the right offset (which I think implies we have to duplicate the "magic 3" fields)

eddyb (Apr 12 2019 at 17:49, on Zulip):

with the full vtable being D S A a b D S A c d e f (while today it's D S A a b c d e f)

Alexander Regueiro (Apr 12 2019 at 17:49, on Zulip):

I would make it an error, yes. Or at least I see no reason to accept it, though I guess it's also not harmful to do so.

annoying thing is that maybe bounds are lost by the time we get to typeck

eddyb (Apr 12 2019 at 17:49, on Zulip):

yeah ofc you need to duplicate them

eddyb (Apr 12 2019 at 17:49, on Zulip):

wait did everyone discuss something else?

nikomatsakis (Apr 12 2019 at 17:49, on Zulip):

I don't think this "embedding" approach works as well for cyclic relations

eddyb (Apr 12 2019 at 17:49, on Zulip):

/me pretends to have been paying attention

nikomatsakis (Apr 12 2019 at 17:49, on Zulip):

No, I said there are two ways, and that is one of them

nikomatsakis (Apr 12 2019 at 17:49, on Zulip):

the other way would be to have the vtables embed pointers to the other vtables

eddyb (Apr 12 2019 at 17:49, on Zulip):

I think this is the one we can trivially do today

eddyb (Apr 12 2019 at 17:50, on Zulip):

and it's not much of a perf loss

eddyb (Apr 12 2019 at 17:50, on Zulip):

(if at all)

nikomatsakis (Apr 12 2019 at 17:50, on Zulip):

neither has perf loss

nikomatsakis (Apr 12 2019 at 17:50, on Zulip):

but I'm in favor of the one with less work

eddyb (Apr 12 2019 at 17:50, on Zulip):

embedding pointers means virtual calls are more expensive

nikomatsakis (Apr 12 2019 at 17:50, on Zulip):

I mean what I imagined more precisely is that the vtable for Foo would embed all the methods of Foo and its supertraits;

nikomatsakis (Apr 12 2019 at 17:50, on Zulip):

but also pointers to the vtables of the supertraits

eddyb (Apr 12 2019 at 17:50, on Zulip):

oh that sounds silly though?

nikomatsakis (Apr 12 2019 at 17:50, on Zulip):

why?

nikomatsakis (Apr 12 2019 at 17:50, on Zulip):

it supports cycles, among other things :)

eddyb (Apr 12 2019 at 17:51, on Zulip):

you're duplicating more. and do we have any plans for cycles?

nikomatsakis (Apr 12 2019 at 17:51, on Zulip):

you're not duplicating more

nikomatsakis (Apr 12 2019 at 17:51, on Zulip):

I mean you have to duplicate 3 fields per supertrait

nikomatsakis (Apr 12 2019 at 17:51, on Zulip):

I am duplicating 1

nikomatsakis (Apr 12 2019 at 17:51, on Zulip):

er, adding 1

Alexander Regueiro (Apr 12 2019 at 17:51, on Zulip):

@eddyb why is the second D S A before c d and not e f?

nikomatsakis (Apr 12 2019 at 17:51, on Zulip):

and anyway come on, it's vtables

eddyb (Apr 12 2019 at 17:51, on Zulip):

but you have 3 + methods elsewhere

Alexander Regueiro (Apr 12 2019 at 17:51, on Zulip):

why*

nikomatsakis (Apr 12 2019 at 17:52, on Zulip):

so e.g. for trait Foo: Bar + Baz, you would be adding 6 fields to the vtable for Foo

eddyb (Apr 12 2019 at 17:52, on Zulip):

@Alexander Regueiro because all 3 traits have the prefix

nikomatsakis (Apr 12 2019 at 17:52, on Zulip):

I would add 2

nikomatsakis (Apr 12 2019 at 17:52, on Zulip):

in both cases I imagine we'll still need to generate vtables for Bar and Baz

nikomatsakis (Apr 12 2019 at 17:52, on Zulip):

though you could imagine not doing so

nikomatsakis (Apr 12 2019 at 17:52, on Zulip):

but it'd be hard across crates etc

eddyb (Apr 12 2019 at 17:53, on Zulip):

@Alexander Regueiro Sub shares it with Sup1

nikomatsakis (Apr 12 2019 at 17:53, on Zulip):

(what am I missing?)

eddyb (Apr 12 2019 at 17:53, on Zulip):

this means with my version, single-inheritance is zero-cost :P

eddyb (Apr 12 2019 at 17:53, on Zulip):

cross-crate reuse has been solved by mw :P

eddyb (Apr 12 2019 at 17:53, on Zulip):

we have infra in the compiler to reuse any monomorphization downstream

Alexander Regueiro (Apr 12 2019 at 17:55, on Zulip):

@eddyb I don't get it... a b c d e f are all separate methods right? so why would e f apply to Sup2?

eddyb (Apr 12 2019 at 17:55, on Zulip):

@nikomatsakis if you never coerce P<T> to either P<Sup1> or P<Sup2> directly, then you don't even need T as Sup1 or T as Sup2 vtables

eddyb (Apr 12 2019 at 17:56, on Zulip):

you use 17 x usize, I use 12 x usize

eddyb (Apr 12 2019 at 17:57, on Zulip):

@Alexander Regueiro ugh I tried to keep it a tiny example. basically the nesting is like this:

((D S A a b) (D S A c d) e f)
eddyb (Apr 12 2019 at 17:57, on Zulip):

(deleted)

eddyb (Apr 12 2019 at 17:58, on Zulip):

Sub and Sup1 share the first D S A, while Sup2 needs its own

Alexander Regueiro (Apr 12 2019 at 18:00, on Zulip):

oh sorry

nikomatsakis (Apr 12 2019 at 18:00, on Zulip):

nikomatsakis if you never coerce P<T> to either P<Sup1> or P<Sup2> directly, then you don't even need T as Sup1 or T as Sup2 vtables

I don't know how you could know that

Alexander Regueiro (Apr 12 2019 at 18:00, on Zulip):

got my associativity all wrong haha

nikomatsakis (Apr 12 2019 at 18:00, on Zulip):

but also I don't care to argue about this

nikomatsakis (Apr 12 2019 at 18:00, on Zulip):

do whaever

nikomatsakis (Apr 12 2019 at 18:00, on Zulip):

I also don't care how man words are in m vtables

eddyb (Apr 12 2019 at 18:00, on Zulip):

no, I mean

eddyb (Apr 12 2019 at 18:00, on Zulip):

we build vtables on demand

Alexander Regueiro (Apr 12 2019 at 18:00, on Zulip):

so yeah, makes sense now eddyb

eddyb (Apr 12 2019 at 18:00, on Zulip):

so we would only need all 3 if there were unsizing coercions

nikomatsakis (Apr 12 2019 at 18:01, on Zulip):

I mean you leave the pointers to supertrait vtables as NULL, or not include them

nikomatsakis (Apr 12 2019 at 18:01, on Zulip):

but I still don't think you can know that

nikomatsakis (Apr 12 2019 at 18:01, on Zulip):

i.e., you have to include all the data for a potential upcast

eddyb (Apr 12 2019 at 18:01, on Zulip):

no, that's not what I was referring to

nikomatsakis (Apr 12 2019 at 18:01, on Zulip):

because you could return the trait object to something outside of the crate

nikomatsakis (Apr 12 2019 at 18:01, on Zulip):

and it might do the upcast

eddyb (Apr 12 2019 at 18:01, on Zulip):

if you don't use pointers, you don't need to create those vtables

nikomatsakis (Apr 12 2019 at 18:01, on Zulip):

oh, I see

eddyb (Apr 12 2019 at 18:01, on Zulip):

unless there is a direct cast

nikomatsakis (Apr 12 2019 at 18:01, on Zulip):

sure, ok

nikomatsakis (Apr 12 2019 at 18:01, on Zulip):

/me shrugs

nikomatsakis (Apr 12 2019 at 18:01, on Zulip):

whoop-dee-doo :P

eddyb (Apr 12 2019 at 18:01, on Zulip):

offsetting is also cheaper

nikomatsakis (Apr 12 2019 at 18:02, on Zulip):

yes, that is true

nikomatsakis (Apr 12 2019 at 18:02, on Zulip):

is there a way to handle cycles with offsetting?

eddyb (Apr 12 2019 at 18:02, on Zulip):

and I'd just make cyclic traits not object-safe

nikomatsakis (Apr 12 2019 at 18:02, on Zulip):

why?

nikomatsakis (Apr 12 2019 at 18:02, on Zulip):

just so we can shave off a few cycles?

eddyb (Apr 12 2019 at 18:02, on Zulip):

because they're an abomination :P?

eddyb (Apr 12 2019 at 18:02, on Zulip):

and they complicate everything

nikomatsakis (Apr 12 2019 at 18:02, on Zulip):

I mean right now it's an error anyway, so it's fine

nikomatsakis (Apr 12 2019 at 18:02, on Zulip):

so we can decide this later

eddyb (Apr 12 2019 at 18:02, on Zulip):

but yeah you can do it with offsets

nikomatsakis (Apr 12 2019 at 18:03, on Zulip):

maybe I'm missing something, but it's not very clear how you would do that

nikomatsakis (Apr 12 2019 at 18:03, on Zulip):

I guess you lay out the whole cycle as a unit and you know you can do some negative offsets or something

eddyb (Apr 12 2019 at 18:03, on Zulip):

I mean, it's physically possible, it just requires a... yeah that

nikomatsakis (Apr 12 2019 at 18:03, on Zulip):

yeah, ok

eddyb (Apr 12 2019 at 18:03, on Zulip):

at that point there is no reason to offset at all

nikomatsakis (Apr 12 2019 at 18:03, on Zulip):

anyway, as I said, offsets seems fine to me, particularly since I agree it's probably easier

eddyb (Apr 12 2019 at 18:03, on Zulip):

all traits in the cyclic group (which I can't remember the proper word for)

nikomatsakis (Apr 12 2019 at 18:04, on Zulip):

SCC, yes, ok

eddyb (Apr 12 2019 at 18:04, on Zulip):

would just use the same vtable

nikomatsakis (Apr 12 2019 at 18:04, on Zulip):

that's a good point :)

eddyb (Apr 12 2019 at 18:04, on Zulip):

they are one trait

nikomatsakis (Apr 12 2019 at 18:04, on Zulip):

(that's actually kind of neat) =)

centril (Apr 12 2019 at 18:04, on Zulip):

in any case, can you not have special logic for traits with cycles if you need to?

eddyb (Apr 12 2019 at 18:04, on Zulip):

this is another reason cycles are silly

nikomatsakis (Apr 12 2019 at 18:04, on Zulip):

I don't agree it's silly but we'll leave it

eddyb (Apr 12 2019 at 18:04, on Zulip):

mutual cyclical things are one cyclical thing

nikomatsakis (Apr 12 2019 at 18:04, on Zulip):

(I think it's useful to isolate out independent "views" on the same set of methods)

nikomatsakis (Apr 12 2019 at 18:05, on Zulip):

but it's not a big thing, I agree they are logically one trait

Alexander Regueiro (Apr 12 2019 at 18:05, on Zulip):

soo....

eddyb (Apr 12 2019 at 18:05, on Zulip):

I would hate it if we couldn't find a nicer way to do that than mutually cyclical traits :P

nikomatsakis (Apr 12 2019 at 18:05, on Zulip):

anyway I gotta run y'all but seems like we are underway, I think it'd be a good idea to step back and write out the actual steps

nikomatsakis (Apr 12 2019 at 18:05, on Zulip):

and who will be doing them:)

Alexander Regueiro (Apr 12 2019 at 18:06, on Zulip):

we probably have rough implementation notes from @eddyb 's messages here already. would you mind filtering them out into a short post though, just for a) the record (and whoever writes the RFC), b) my sanity? :-)

Alexander Regueiro (Apr 12 2019 at 18:06, on Zulip):

yep, thanks @nikomatsakis

centril (Apr 12 2019 at 18:06, on Zulip):

So we have:
1. implementation -- @Alexander Regueiro
2. tests -- ??
3. rfc -- ??

Alexander Regueiro (Apr 12 2019 at 18:07, on Zulip):

2. tests -- centril
3. rfc -- niko?

Alexander Regueiro (Apr 12 2019 at 18:07, on Zulip):

;-)

centril (Apr 12 2019 at 18:07, on Zulip):

I can probably write a bit on the rfc

eddyb (Apr 12 2019 at 18:07, on Zulip):

you might want to get @oli in on this

centril (Apr 12 2019 at 18:07, on Zulip):

I'll leave the representation bits to someone else, but those are not specified anyways so we can be light on this

eddyb (Apr 12 2019 at 18:07, on Zulip):

wrt miri and layout of vtables

eddyb (Apr 12 2019 at 18:08, on Zulip):

like, actually building them. I hope that code has been deduplicated already

centril (Apr 12 2019 at 18:08, on Zulip):

@eddyb iirc @RalfJ made a PR re. dyn FnOnce so maybe they are familiar as well

eddyb (Apr 12 2019 at 18:09, on Zulip):

not related I don't think

eddyb (Apr 12 2019 at 18:09, on Zulip):

but @RalfJ is indeed the other person working on miri :P

Alexander Regueiro (Apr 12 2019 at 18:13, on Zulip):

@eddyb anyway does the above sound okay re notes?

eddyb (Apr 12 2019 at 18:15, on Zulip):

replace the .sum() with .fold(None, |i, n| Some(i.map_or(0, |i| i + 3) + n)).unwrap_or(0) in https://github.com/rust-lang/rust/blob/9ebf47851a357faa4cd97f4b1dc7835f6376e639/src/librustc/traits/select.rs#L3032

eddyb (Apr 12 2019 at 18:15, on Zulip):

that was the main thing of note :P

eddyb (Apr 12 2019 at 18:16, on Zulip):

and, yeah, look at ObjectCandidate / VtableObject (it's for dyn SubTrait: SuperTrait)

eddyb (Apr 12 2019 at 18:16, on Zulip):

oh, actually, it's not just for method calls, my bad

eddyb (Apr 12 2019 at 18:17, on Zulip):

yeah so dyn SubTrait -> dyn SuperTrait can just add an ObjectCandidate

Alexander Regueiro (Apr 12 2019 at 18:18, on Zulip):

@eddyb okay thanks. what about MIR?

eddyb (Apr 12 2019 at 18:18, on Zulip):

what about it?

Alexander Regueiro (Apr 12 2019 at 18:18, on Zulip):

nothing to be done there?

eddyb (Apr 12 2019 at 18:18, on Zulip):

not that I can think of

eddyb (Apr 12 2019 at 18:19, on Zulip):

this is a dyn SubTrait: Unsize<dyn SuperTrait> coercion

Alexander Regueiro (Apr 12 2019 at 18:19, on Zulip):

okay... just miri stuff, and that can be done by oli later maybe?

eddyb (Apr 12 2019 at 18:19, on Zulip):

dyn Foo + Send: Unsize<dyn Foo> exists today

eddyb (Apr 12 2019 at 18:19, on Zulip):

it's allowed here, specifically https://github.com/rust-lang/rust/blob/9ebf47851a357faa4cd97f4b1dc7835f6376e639/src/librustc/traits/select.rs#L2183-L2185

eddyb (Apr 12 2019 at 18:19, on Zulip):

the miri stuff is creating the vtable :P

eddyb (Apr 12 2019 at 18:20, on Zulip):

but basically, if you remove this condition https://github.com/rust-lang/rust/blob/9ebf47851a357faa4cd97f4b1dc7835f6376e639/src/librustc/traits/select.rs#L2182

eddyb (Apr 12 2019 at 18:21, on Zulip):

then here the two traits could differ: https://github.com/rust-lang/rust/blob/9ebf47851a357faa4cd97f4b1dc7835f6376e639/src/librustc/traits/select.rs#L3299

eddyb (Apr 12 2019 at 18:21, on Zulip):

and you can nested.push(...) a dyn A: B obligation

Alexander Regueiro (Apr 12 2019 at 18:22, on Zulip):

okay...

Alexander Regueiro (Apr 12 2019 at 18:22, on Zulip):

I don't get what I have to do with miri though.

eddyb (Apr 12 2019 at 18:22, on Zulip):

you can ask someone who knows trait stuff about that, or just guess from looking around (I'd be just doing the latter if you wanted me to help)

Alexander Regueiro (Apr 12 2019 at 18:22, on Zulip):

did you refer to that before?

eddyb (Apr 12 2019 at 18:22, on Zulip):

yeah, vtables

eddyb (Apr 12 2019 at 18:22, on Zulip):

miri builds the actual vtable bytes, so to say

Alexander Regueiro (Apr 12 2019 at 18:23, on Zulip):

ObjectCandidate and VtableObject are miri? didn't know that ha

Alexander Regueiro (Apr 12 2019 at 18:23, on Zulip):

I see

eddyb (Apr 12 2019 at 18:23, on Zulip):

no

eddyb (Apr 12 2019 at 18:23, on Zulip):

those are traits::select

eddyb (Apr 12 2019 at 18:24, on Zulip):

nevermind, I'm wrong https://github.com/rust-lang/rust/blob/master/src/librustc_codegen_ssa/meth.rs#L69

eddyb (Apr 12 2019 at 18:24, on Zulip):

this was supposed to be deduplicated into miri ages ago :(

eddyb (Apr 12 2019 at 18:24, on Zulip):

I guess it hasn't happened yet

eddyb (Apr 12 2019 at 18:26, on Zulip):

so basically here you just need to inject the 3 D S A entries before the actual methods https://github.com/rust-lang/rust/blob/2226c09699a96520238e162777f44504f4a0a1a7/src/librustc/traits/mod.rs#L991

eddyb (Apr 12 2019 at 18:26, on Zulip):

(by replacing the Option with an enum indicating what data is in that entry)

eddyb (Apr 12 2019 at 18:27, on Zulip):

alternatively, change &'tcx [Option<(DefId, SubstsRef<'tcx>)>] in the return to &'tcx [&'tcx [Option<(DefId, SubstsRef<'tcx>)>]] to make it clear how the methods are split into traits :P

eddyb (Apr 12 2019 at 18:27, on Zulip):

(DefId, SubstsRef<'tcx>) also sounds like it should be an Instance, heh

Alexander Regueiro (Apr 12 2019 at 18:29, on Zulip):

@eddyb okay, I think that should be reasonably clear now. I'll have a look soon. thanks! (and I'll let you bug Oli or Ralf about deduplicating that code heh)

RalfJ (Apr 12 2019 at 20:18, on Zulip):

@eddyb

like, actually building them. I hope that code has been deduplicated already

not that I know of

oli (Apr 12 2019 at 21:49, on Zulip):

like, actually building them. I hope that code has been deduplicated already

it has not

oli (Apr 12 2019 at 21:50, on Zulip):

oh lol

oli (Apr 12 2019 at 21:50, on Zulip):

I should've scrolled down

oli (Apr 12 2019 at 21:51, on Zulip):

we can try dedupicating, but we need to bench that, because in many cases it might be more effort since we're creating miri allocations just to write them back to llvm

eddyb (Apr 13 2019 at 13:50, on Zulip):

@oli worst case we replace the vtable_methods query with one that produces a list of vtable entries :P

eddyb (Apr 13 2019 at 13:50, on Zulip):

but really, if a constant does an unsizing, we already use the miri vtable instead of the LLVM one...

eddyb (Apr 13 2019 at 13:51, on Zulip):

so I don't see any point in not using the miri one

Alexander Regueiro (May 05 2019 at 21:52, on Zulip):

Sub and Sup1 share the first D S A, while Sup2 needs its own

why can they share the first? @eddyb

eddyb (May 06 2019 at 11:49, on Zulip):

uhhh

eddyb (May 06 2019 at 11:56, on Zulip):

my point is that you can have Sub share a prefix with with Sup1 or Sup2, because you can start Sub's vtable with D S A followed by their respective methods

eddyb (May 06 2019 at 11:56, on Zulip):

but you can't have both Sup1 and Sup2 share the same D S A

Alexander Regueiro (May 07 2019 at 00:07, on Zulip):

@eddyb okay, I guess I'll see why that's the case as I proceed... I thought that D S A would need to be different for Sub and Sup1 because fundamentally they're different types, but...

eddyb (May 07 2019 at 00:18, on Zulip):

OH!

eddyb (May 07 2019 at 00:18, on Zulip):

that's where the confusion was

eddyb (May 07 2019 at 00:19, on Zulip):

Sub and Sup1 are traits

eddyb (May 07 2019 at 00:19, on Zulip):

the vtable is for a given type that implements those traits, and D/S/A are properties of that type that are always the same no matter what trait you're talking about

eddyb (May 07 2019 at 00:20, on Zulip):

like, it's almost literally (ptr::drop_in_place::<T>, mem::size_of::<T>(), mem::align_of::<T>())

eddyb (May 07 2019 at 00:21, on Zulip):

they're followed by, say, <T as Sup1>::whatever_method_name function pointers, but the D S A prefix is always the same for a given T

Alexander Regueiro (May 07 2019 at 00:30, on Zulip):

@eddyb ohh, I see (I think). so the second D S A is just a duplicate of the first in the vtable (in your above example)?

eddyb (May 07 2019 at 00:31, on Zulip):

yupp

Alexander Regueiro (May 07 2019 at 00:31, on Zulip):

@eddyb thanks. all clear now.

Alexander Regueiro (May 07 2019 at 00:36, on Zulip):

alternatively, change &'tcx [Option<(DefId, SubstsRef<'tcx>)>] in the return to &'tcx [&'tcx [Option<(DefId, SubstsRef<'tcx>)>]] to make it clear how the methods are split into traits :P

is this your preferred alternative, @eddyb? that is, &'tcx [Instance]?

Alexander Regueiro (May 15 2019 at 13:29, on Zulip):

@eddyb is this how I registeer the obligation in confirm_builtin_unsize_candidate?

Alexander Regueiro (May 15 2019 at 13:29, on Zulip):
                // Register an obligation for `TraitA: TraitB`.
                nested.extend(
                    data_b.iter()
                        .map(|d| predicate_to_obligation(d.with_self_ty(tcx, source_ty))),
                );
eddyb (May 15 2019 at 14:13, on Zulip):

sorry, I gtg, but I'll take a look at it later

Alexander Regueiro (May 15 2019 at 16:09, on Zulip):

@eddyb no prob

Alexander Regueiro (May 15 2019 at 19:35, on Zulip):

@eddyb that should read dyn TraitA: TraitB BTW. I can paste the whole file if you like, but I think that's the relevant bit

eddyb (May 16 2019 at 06:21, on Zulip):

I think that's correct

Alexander Regueiro (May 16 2019 at 14:57, on Zulip):

@eddyb okay thanks. I think something went wrong with my staged build, because it (kind of) worked now. I'm working on the MIR part now...

Alexander Regueiro (May 16 2019 at 15:17, on Zulip):

looks reasonably straightforward.

Alexander Regueiro (May 16 2019 at 15:18, on Zulip):

unless I've misunderstood something

Alexander Regueiro (May 16 2019 at 21:43, on Zulip):

@eddyb might be nice to provide Read and Write impls for Cursor<mir::Memory> or something no?

Alexander Regueiro (May 18 2019 at 18:20, on Zulip):

@eddyb noticed the PR I opened yet?

eddyb (May 20 2019 at 12:35, on Zulip):

nope, I'm back at the office today but idk how much time I'll have to review it

eddyb (May 20 2019 at 12:36, on Zulip):

as for miri things, talk to @oli (Read/Write don't seem that fitting, especially without the ability to change the error type from io::Error)

oli (May 20 2019 at 13:54, on Zulip):

I'm not sure what mir::Memory is, but if you mean mir::interpret::Memory, I have no idea what Read and Write would even do on a Memory

Alexander Regueiro (May 20 2019 at 15:11, on Zulip):

don't worry, it was a silly idea

Alexander Regueiro (May 20 2019 at 15:11, on Zulip):

I took another approach, which you seemed to have liked in the end.

Alexander Regueiro (May 20 2019 at 15:12, on Zulip):

@eddyb and no worries. when you do have time, feedback on the bootstrapping error I posted in the PR (to do with coercion) is the most important thing.

Alexander Regueiro (Jul 05 2019 at 15:13, on Zulip):

@nikomatsakis so, this is the problematic thing right now: https://github.com/rust-lang/rust/pull/60900#issuecomment-493593431

nikomatsakis (Jul 05 2019 at 15:14, on Zulip):

I see

nikomatsakis (Jul 05 2019 at 15:14, on Zulip):

presumably due to some changes to coercion

Alexander Regueiro (Jul 05 2019 at 15:15, on Zulip):

@nikomatsakis this seems to be the relevant bit of code: https://github.com/rust-lang/rust/pull/60900/commits/0dbc2adb7e7d4d078a47ad1cf554d5bc95fc50a9#r285305829

Alexander Regueiro (Jul 05 2019 at 15:16, on Zulip):

and in fact, whether or not I comment out the old bit of inference code you wrote, the error still occurs

nikomatsakis (Jul 05 2019 at 15:16, on Zulip):

Well, I think also that this change feels too loose.

- data_a.principal_def_id() == data_b.principal_def_id()
nikomatsakis (Jul 05 2019 at 15:16, on Zulip):

though perhaps not at fault

Alexander Regueiro (Jul 05 2019 at 15:16, on Zulip):

so it's caused by the new code probably, even though it seems right (eddyb thought so too)

Alexander Regueiro (Jul 05 2019 at 15:17, on Zulip):

it may be. I did that on eddyb's suggestion too. not that I blame him, because it made sense to me too. :-)

Alexander Regueiro (Jul 05 2019 at 15:17, on Zulip):

the relevant new code I think is the bit beginning with // Register an obligation for `dyn TraitA: TraitB`.

nikomatsakis (Jul 05 2019 at 15:20, on Zulip):

OK, yeah, I'm looking. Might be the easiest way to debug is to build and test.

Alexander Regueiro (Jul 05 2019 at 15:20, on Zulip):

yep

Alexander Regueiro (Jul 05 2019 at 15:20, on Zulip):

I have a build here, but you'll probably want your own

nikomatsakis (Jul 05 2019 at 15:22, on Zulip):

indeed, always easiest in the home environment

Alexander Regueiro (Jul 05 2019 at 15:22, on Zulip):

So, should I leave both things to you in your own time now? :-)

Alexander Regueiro (Jul 05 2019 at 15:23, on Zulip):

and you can just comment on the PRs or message me here

Alexander Regueiro (Jul 05 2019 at 15:23, on Zulip):

@nikomatsakis

nikomatsakis (Jul 05 2019 at 15:23, on Zulip):

OK, so I've kicked off a build. I think let's do this: I will add to my calendar some time to review #61812 today, and also #60900 once can observe the problem

nikomatsakis (Jul 05 2019 at 15:23, on Zulip):

prob in an hour or so, since I have to take that other call that I moved :)

nikomatsakis (Jul 05 2019 at 15:24, on Zulip):

I'll also try to review #61812...

nikomatsakis (Jul 05 2019 at 15:24, on Zulip):

but that's not related to you :)

Alexander Regueiro (Jul 05 2019 at 15:24, on Zulip):

yeah no worries Niko

Alexander Regueiro (Jul 05 2019 at 15:24, on Zulip):

heh

Alexander Regueiro (Jul 05 2019 at 15:24, on Zulip):

when are you taking your holiday by the way?

nikomatsakis (Jul 05 2019 at 15:25, on Zulip):

it starts July 15

Alexander Regueiro (Jul 05 2019 at 15:25, on Zulip):

Okay. Couple of weeks? Longer? Just curious, so I know not to pester you when you're enjoying your time off!

Alexander Regueiro (Jul 05 2019 at 15:26, on Zulip):

@nikomatsakis I'm confident we can get both these things sorted before then in any case. Maybe afterwards we could address impl-trait-in-bindings. (I know the intention was for you to compile the notes and musings scattered across various locations, but honestly maybe it makes more sense in terms of efficiency for me to hand over my existing work to you and me focus on other things, since you know exactly what needs to be done.)

Alexander Regueiro (Jul 05 2019 at 15:36, on Zulip):

For example, maybe I can work on multi-trait objects then if we can get trait upcasting through :-)

nikomatsakis (Jul 05 2019 at 17:26, on Zulip):

OK, @Alexander Regueiro, so I see the problem with upcasts. The good news is that it's, I think, a pre-existing problem. Pretty unrelated to your diff, though I think it is caused by the change to insert more obligations on an upcast coercion than we used to.

nikomatsakis (Jul 05 2019 at 17:27, on Zulip):

i.e., that makes us prove something we never had to prove before

nikomatsakis (Jul 05 2019 at 17:29, on Zulip):

Here is a minimized example illustrating the problem (playground):

#![feature(optin_builtin_traits)]

pub auto trait Foo { }

// Comment this out makes the code compile:
impl<T: ?Sized> Foo for T { }

pub trait Bar { }

fn foo<T: ?Sized + Foo>() {
}

fn main() {
    foo::<dyn Bar + Foo>();
}
nikomatsakis (Jul 05 2019 at 17:29, on Zulip):

In short, we get an ambiguity because dyn Bar + Foo: Foo holds in 2 different ways -- one via the impl, the other via the nature of object types.

nikomatsakis (Jul 05 2019 at 17:30, on Zulip):

Also, and completely unrelated, I wish we had made it dyn (Bar + Foo). The strange precedence of dyn and impl is convenient sometimes but man it reads odd to me. =)

nikomatsakis (Jul 05 2019 at 17:33, on Zulip):

I think this is only really relevant because of a somewhat epic hack we are using to make "dual mode" thread-safety compiles. We could plausibly do that hack a different way and sidestep the problem. We could also handle this particular "ambiguity".

Alexander Regueiro (Jul 05 2019 at 17:48, on Zulip):

@nikomatsakis I thought it was pre-existing too, indeed.

Alexander Regueiro (Jul 05 2019 at 17:48, on Zulip):

Yeah, I don't like that precedence either overmuch heh...

Alexander Regueiro (Jul 05 2019 at 17:48, on Zulip):

anyway, I see the problem now, thanks for explaining

Alexander Regueiro (Jul 05 2019 at 17:48, on Zulip):

what's your inclination when it comes to solving it?

Alexander Regueiro (Jul 05 2019 at 17:55, on Zulip):

@nikomatsakis anyway much appreciated. if you could leave instructions here, I may be able to have a go at tackling it over the weekend... whichever approach you prefer.

nikomatsakis (Jul 05 2019 at 20:06, on Zulip):

what's your inclination when it comes to solving it?

I'm debating this @Alexander Regueiro =) there is also this soundness issue that I've been meaning to review that is totally related.

nikomatsakis (Jul 05 2019 at 20:07, on Zulip):

One obvious thing would be to give precedence to object candidates, but that's a tricky question that has implications for type inference and I don't love it.

nikomatsakis (Jul 05 2019 at 20:08, on Zulip):

Another would be to revise the coercion logic to add fewer obligations =) that would probably just sidestep the problem enough for now and leave it for another day (though we should file an issue)

nikomatsakis (Jul 05 2019 at 20:08, on Zulip):

BTW @Alexander Regueiro you should consider landing that "cleanup commit" separately

nikomatsakis (Jul 05 2019 at 20:08, on Zulip):

i.e., in a separate PR

nikomatsakis (Jul 05 2019 at 20:08, on Zulip):

ah well I guess we've been back and forth on this :P

nikomatsakis (Jul 05 2019 at 20:09, on Zulip):

I'm only mentioning it because it was sort of annoying when trying to read the diffs, though having it at the beginning was a big help as I Could do git diff 12312414... or whatever

nikomatsakis (Jul 05 2019 at 20:09, on Zulip):

Another would be to revise the coercion logic to add fewer obligations =) that would probably just sidestep the problem enough for now and leave it for another day (though we should file an issue)

this is probably the thing to do for now

nikomatsakis (Jul 05 2019 at 20:09, on Zulip):

but I have to think what that means

Alexander Regueiro (Jul 05 2019 at 23:53, on Zulip):

@nikomatsakis sure... I just thought those "aggregated drive-by changes" (admittedly quite a few of them) were more likely to get merged as a separate commit in the same PR. :-)

Alexander Regueiro (Jul 05 2019 at 23:54, on Zulip):

anyway, I'll try to look at the coercion logic for now, thanks

Alexander Regueiro (Jul 05 2019 at 23:54, on Zulip):

In short, we get an ambiguity because dyn Bar + Foo: Foo holds in 2 different ways -- one via the impl, the other via the nature of object types.

does it require that both hold right now? maybe we just want "at least one" to hold?

Alexander Regueiro (Aug 05 2019 at 15:43, on Zulip):

I think this is only really relevant because of a somewhat epic hack we are using to make "dual mode" thread-safety compiles. We could plausibly do that hack a different way and sidestep the problem. We could also handle this particular "ambiguity".

@eddyb @varkor is either of you aware of this "epic hack" / where it is / how we could tackle it differently to avoid this problem?

varkor (Aug 05 2019 at 15:46, on Zulip):

I'm not sure, no

Alexander Regueiro (Aug 05 2019 at 16:09, on Zulip):

no prob

Alexander Regueiro (Aug 05 2019 at 16:11, on Zulip):

@varkor did you see my comment about my PR involving type-checking for ATB though? I thought maybe you'd be a good person to review. it's not too much to review, and Niko already partially reviewed and left notes how he wanted it (re)done... no great rush, but if you can look at it at some point this week, then great.

varkor (Aug 05 2019 at 16:11, on Zulip):

I haven't checked GH properly yet today — I'll take a look later 👍

Alexander Regueiro (Aug 05 2019 at 16:15, on Zulip):

Cool, thanks.

nikomatsakis (Aug 27 2019 at 17:43, on Zulip):

Hey @Alexander Regueiro

Alexander Regueiro (Aug 27 2019 at 17:43, on Zulip):

Hey Niko.

Alexander Regueiro (Aug 27 2019 at 17:43, on Zulip):

got a bit of time now then?

nikomatsakis (Aug 27 2019 at 17:43, on Zulip):

A bit, yeah

nikomatsakis (Aug 27 2019 at 17:44, on Zulip):

So I guess this is still blocking you :)

Alexander Regueiro (Aug 27 2019 at 17:44, on Zulip):

yeah, a bit...

Alexander Regueiro (Aug 27 2019 at 17:44, on Zulip):

I suppose it would be nice to decide which of your two suggested approaches to take

Alexander Regueiro (Aug 27 2019 at 17:44, on Zulip):

and what exactly I need to do

Alexander Regueiro (Aug 27 2019 at 17:44, on Zulip):

you went over them at a high level above.

nikomatsakis (Aug 27 2019 at 17:47, on Zulip):

Yeah I'm reviewing the code

nikomatsakis (Aug 27 2019 at 17:47, on Zulip):

The epic hack I was referring to are these duplicate Send + Sync auto traits

Alexander Regueiro (Aug 27 2019 at 17:49, on Zulip):

oh I see

Alexander Regueiro (Aug 27 2019 at 17:50, on Zulip):

hrmm

Alexander Regueiro (Aug 27 2019 at 17:50, on Zulip):

and they're needed why exactly?

Alexander Regueiro (Aug 27 2019 at 17:54, on Zulip):

@nikomatsakis

nikomatsakis (Aug 27 2019 at 18:00, on Zulip):

well, they're used to "cross-compile" the compiler between thread-safe and not thread-safe

nikomatsakis (Aug 27 2019 at 18:00, on Zulip):

the types always say librustc_data_structures::sync::Send and whatever

nikomatsakis (Aug 27 2019 at 18:00, on Zulip):

when in thread-safe mode, that is the normal Send

nikomatsakis (Aug 27 2019 at 18:00, on Zulip):

but otherwise it's this "no-op" Send that is defined for all types

nikomatsakis (Aug 27 2019 at 18:01, on Zulip):

the reason that your code is hitting problems -- which is what I was just researching -- is this code here:

Alexander Regueiro (Aug 27 2019 at 18:02, on Zulip):

oh

nikomatsakis (Aug 27 2019 at 18:02, on Zulip):
                nested.extend(
                    data_b.iter()
                        .map(|d| predicate_to_obligation(d.with_self_ty(tcx, source_ty))),
                );
Alexander Regueiro (Aug 27 2019 at 18:02, on Zulip):

of course

nikomatsakis (Aug 27 2019 at 18:02, on Zulip):

not that the code is wrong

Alexander Regueiro (Aug 27 2019 at 18:02, on Zulip):

mhm...

nikomatsakis (Aug 27 2019 at 18:02, on Zulip):

but when you upcast from dyn Foo + Send + 'a to dyn Foo + Send + 'b

nikomatsakis (Aug 27 2019 at 18:02, on Zulip):

it winds up forcing us to prove dyn Foo + Send: Send

nikomatsakis (Aug 27 2019 at 18:02, on Zulip):

which in turn hits that ambiguous case

Alexander Regueiro (Aug 27 2019 at 18:03, on Zulip):

right

Alexander Regueiro (Aug 27 2019 at 18:04, on Zulip):

this is a more general problem though, isn't it?

nikomatsakis (Aug 27 2019 at 18:04, on Zulip):

it is but I'm not sure I want to fix it in this PR :)

nikomatsakis (Aug 27 2019 at 18:04, on Zulip):

so one option might be to (somewhat hackily) avoid generating that obligation

Alexander Regueiro (Aug 27 2019 at 18:04, on Zulip):

because a trait object can implement a trait by impl or it being a "supertrait"

Alexander Regueiro (Aug 27 2019 at 18:04, on Zulip):

mhm...

nikomatsakis (Aug 27 2019 at 18:04, on Zulip):

(there is also a related soundness problem, as an aside...)

nikomatsakis (Aug 27 2019 at 18:06, on Zulip):

anyway a hack that would suffice to unblock the PR might be to look for auto traits that are already included in the list and skip them or something

Alexander Regueiro (Aug 27 2019 at 18:06, on Zulip):

hmm

Alexander Regueiro (Aug 27 2019 at 18:06, on Zulip):

yes, does sound quite hacky though

nikomatsakis (Aug 27 2019 at 18:06, on Zulip):

I'm doing a build with those lines commented out, as an aside, just to test my hypothesis

Alexander Regueiro (Aug 27 2019 at 18:06, on Zulip):

do you not want to go for a more solid solution right away?

nikomatsakis (Aug 27 2019 at 18:06, on Zulip):

it got farther

Alexander Regueiro (Aug 27 2019 at 18:06, on Zulip):

I don't mind really

Alexander Regueiro (Aug 27 2019 at 18:06, on Zulip):

okay.

nikomatsakis (Aug 27 2019 at 18:07, on Zulip):
error[E0284]: type annotations required: cannot resolve `<_ as context::Context>::GoalInEnvironment == <I as context::Context>::GoalInEnvironment`
   --> /home/nmatsakis/.cargo/registry/src/github.com-1ecc6299db9ec823/chalk-engine-0.9.0/src/logic.rs:675:48
    |
675 |             Literal::Positive(subgoal) => self.abstract_positive_literal(infer, subgoal),
    |                                                ^^^^^^^^^^^^^^^^^^^^^^^^^
nikomatsakis (Aug 27 2019 at 18:07, on Zulip):

not sure what's up with that

nikomatsakis (Aug 27 2019 at 18:07, on Zulip):

anyway, I think I would try to add some kind of "skip auto traits" check with a FIXME

nikomatsakis (Aug 27 2019 at 18:07, on Zulip):

we could file a related issue, using my example above

nikomatsakis (Aug 27 2019 at 18:08, on Zulip):

the idea would be to say "if you are upcasting to X and the original type includes the auto trait X, skip it" -- though we might want to be sure it's an auto trait with no type parameters

Alexander Regueiro (Aug 27 2019 at 18:08, on Zulip):

yeah. I didn't know chalk was being used already by default though...?

Alexander Regueiro (Aug 27 2019 at 18:08, on Zulip):

mhm

Alexander Regueiro (Aug 27 2019 at 18:08, on Zulip):

sounds fair enough

Alexander Regueiro (Aug 27 2019 at 18:08, on Zulip):

how would you see a less hacky fix going though?

nikomatsakis (Aug 27 2019 at 18:09, on Zulip):

well, e.g. for chalk, it isn't necessary to find a unique path necessarily

nikomatsakis (Aug 27 2019 at 18:09, on Zulip):

so one option would be to just not have a problem with multiple options

nikomatsakis (Aug 27 2019 at 18:10, on Zulip):

but that requires some deeper changes in how trait resolution works

Alexander Regueiro (Aug 27 2019 at 18:12, on Zulip):

I see

Alexander Regueiro (Aug 27 2019 at 18:12, on Zulip):

so many best just to leave that to you and the Chalk guys?

Alexander Regueiro (Aug 27 2019 at 18:19, on Zulip):

@nikomatsakis anyway I'll try your hack suggestion, thanks. if you have an idea about that subsequent error, let me know (maybe I won't run into it if I'm lucky though?)

nikomatsakis (Aug 27 2019 at 18:20, on Zulip):

@Alexander Regueiro yeah I'm hoping you don't :)

nikomatsakis (Aug 27 2019 at 18:20, on Zulip):

I did a hackier change, after all

Alexander Regueiro (Aug 27 2019 at 18:20, on Zulip):

exactly

nikomatsakis (Aug 27 2019 at 18:20, on Zulip):

but let me know and I'll do some debugging

Alexander Regueiro (Aug 27 2019 at 18:20, on Zulip):

and that error msg is very weird heh

Alexander Regueiro (Aug 27 2019 at 18:21, on Zulip):

@nikomatsakis I have a few thing I want to do after this trait upcasting gets merged (apart from getting my REPL project published and whatnot), but maybe I can start getting more involved with Chalk then. I'd like to... let's see.

Alexander Regueiro (Aug 27 2019 at 18:21, on Zulip):

I suppose we'll be having meetings again now, at least.

Alexander Regueiro (Aug 27 2019 at 18:21, on Zulip):

traits-WG ones

Alexander Regueiro (Aug 27 2019 at 18:37, on Zulip):

@nikomatsakis random question: is the best (easiest?) way to check whether a ty implements a trait – outside of type checking/inference phase – to do tcx.infer_ctxt().enter(|infcx| { infcx.at().sub(...) }) – or something like that?

Alexander Regueiro (Aug 27 2019 at 18:37, on Zulip):

I guess I'd just give some empty ParamEnv too

nikomatsakis (Aug 27 2019 at 20:10, on Zulip):

@Alexander Regueiro no, the best way is to use the evaluate_obligation query

Alexander Regueiro (Aug 27 2019 at 21:28, on Zulip):

Okay thanks!

Alexander Regueiro (Sep 04 2019 at 01:20, on Zulip):

Okay thanks!

Alexander Regueiro (Sep 10 2019 at 21:00, on Zulip):

@nikomatsakis We'll talk here then.

Alexander Regueiro (Sep 10 2019 at 22:45, on Zulip):

@nikomatsakis got a little time for this today or not? no worries if not.

Alexander Regueiro (Sep 11 2019 at 20:41, on Zulip):

@nikomatsakis how about today? :-)

nikomatsakis (Sep 11 2019 at 22:54, on Zulip):

Hey @Alexander Regueiro, sorry, today was crazy so far

nikomatsakis (Sep 11 2019 at 22:54, on Zulip):

Let me review the PR :)

Alexander Regueiro (Sep 11 2019 at 22:54, on Zulip):

hey, no problem...

Alexander Regueiro (Sep 11 2019 at 22:54, on Zulip):

cheers!

nikomatsakis (Sep 11 2019 at 22:55, on Zulip):

is current status still that those examples don't work, @Alexander Regueiro ?

Alexander Regueiro (Sep 11 2019 at 22:55, on Zulip):

yeah

nikomatsakis (Sep 11 2019 at 22:55, on Zulip):

if so, it does seem like it's probably some kind of vtable counting problem

Alexander Regueiro (Sep 11 2019 at 22:56, on Zulip):

the bit of code I highlighted in my last comment is surely the key

nikomatsakis (Sep 11 2019 at 22:56, on Zulip):

yeah, prob, let me do a local build

Alexander Regueiro (Sep 11 2019 at 22:56, on Zulip):

the question is how to go about it... do I use old_info in some way? do I do a query? do I reuse the vtable counting code from select.rs somehow?

Alexander Regueiro (Sep 11 2019 at 22:56, on Zulip):

(since this is in codegen)

Alexander Regueiro (Sep 11 2019 at 22:58, on Zulip):

@nikomatsakis ^

nikomatsakis (Sep 11 2019 at 23:00, on Zulip):

hmm

nikomatsakis (Sep 11 2019 at 23:00, on Zulip):

remind me, we are doing the "roll out all the methods into the vtable" approach, right?

nikomatsakis (Sep 11 2019 at 23:00, on Zulip):

i.e., one big flat vtable?

Alexander Regueiro (Sep 11 2019 at 23:00, on Zulip):

yeah

Alexander Regueiro (Sep 11 2019 at 23:01, on Zulip):

there's a flat_map somewhere that does that, I forget where exactly...

nikomatsakis (Sep 11 2019 at 23:01, on Zulip):

I guess I would expect you'd do some sort of query, yes, but I'm not sure just what

nikomatsakis (Sep 11 2019 at 23:01, on Zulip):

would have to look at the select.rs code I guess

Alexander Regueiro (Sep 11 2019 at 23:02, on Zulip):

the vtable looks like D S A [trait methods] D S A [supertrait 1 methods] D S A [supertrait 2 methods] ...

Alexander Regueiro (Sep 11 2019 at 23:02, on Zulip):

yeah...

Alexander Regueiro (Sep 11 2019 at 23:02, on Zulip):

I just want to avoid the compiler duplicating work as much as possible, really

Alexander Regueiro (Sep 11 2019 at 23:03, on Zulip):

@nikomatsakis grep for count_own_vtable_entries in the diff and you'll see the relevant select.rs code

Alexander Regueiro (Sep 11 2019 at 23:03, on Zulip):

it's simple

Alexander Regueiro (Sep 11 2019 at 23:03, on Zulip):

well, ish

Alexander Regueiro (Sep 11 2019 at 23:04, on Zulip):

there's a supertraits query

Alexander Regueiro (Sep 11 2019 at 23:04, on Zulip):

which I may need to repeat...?

Alexander Regueiro (Sep 12 2019 at 00:59, on Zulip):

@nikomatsakis still building? :-)

nikomatsakis (Sep 12 2019 at 12:15, on Zulip):

@Alexander Regueiro sorry, didn't finish before I had to log out for the night :)

Alexander Regueiro (Sep 12 2019 at 18:05, on Zulip):

ah okay

Alexander Regueiro (Sep 12 2019 at 18:05, on Zulip):

@nikomatsakis today...?

Alexander Regueiro (Sep 13 2019 at 16:31, on Zulip):

@nikomatsakis I'll be out the rest of today, but feel free to leave notes while I'm gone, if you have thoughts.

nikomatsakis (Sep 13 2019 at 19:29, on Zulip):

@Alexander Regueiro my build of your branch is dying with this error

error[E0283]: type annotations required: cannot resolve `dyn emitter::Emitter + rustc_data_structures::sync::Send: rustc_data_structures::sync::Send`

is that expected?

Alexander Regueiro (Sep 13 2019 at 20:55, on Zulip):

@nikomatsakis oh sorry. The way I built it was with —keep-stage 0 after building master stage 0. Easier to debug that way.

Alexander Regueiro (Sep 19 2019 at 16:45, on Zulip):

@nikomatsakis hey. did you get it built yet then?

Alexander Regueiro (Sep 19 2019 at 16:45, on Zulip):

anyway you may just want to look at the bit of code I highlighted

Alexander Regueiro (Sep 19 2019 at 16:45, on Zulip):

in my comment on GH

Alexander Regueiro (Sep 19 2019 at 16:45, on Zulip):

and tell me roughly what should go there :-)

Alexander Regueiro (Sep 20 2019 at 16:23, on Zulip):

@nikomatsakis well, I'll have a go doing a trait query there and see if I get progress...

nikomatsakis (Sep 20 2019 at 20:28, on Zulip):

Sorry @Alexander Regueiro been slammed this wek

nikomatsakis (Sep 20 2019 at 20:28, on Zulip):

Trying to catch up a bit

nikomatsakis (Sep 20 2019 at 20:34, on Zulip):

However, I didn't get this building, no -- I'm confused are you just using --keep-stage0 to work around the other problem for now?

Alexander Regueiro (Sep 20 2019 at 20:46, on Zulip):

@nikomatsakis well, I'm using it just to get rustc building

Alexander Regueiro (Sep 20 2019 at 20:46, on Zulip):

so I can test it on smaller examples

Alexander Regueiro (Sep 20 2019 at 20:46, on Zulip):

rather than all of rustc

Alexander Regueiro (Sep 20 2019 at 21:06, on Zulip):

@nikomatsakis don't worry if you don't have time today though

Alexander Regueiro (Sep 20 2019 at 21:06, on Zulip):

I'll try anyway

Alexander Regueiro (Sep 20 2019 at 21:07, on Zulip):

I wouldn't think you'd gain much by a working build like me anyway

Alexander Regueiro (Sep 20 2019 at 21:07, on Zulip):

it's mainly about what to put in that bit of codeen_ssa

nikomatsakis (Sep 23 2019 at 20:40, on Zulip):

well, I'm using it just to get rustc building

oh, I see

nikomatsakis (Sep 23 2019 at 20:41, on Zulip):

useful hack, yes

nikomatsakis (Sep 25 2019 at 12:40, on Zulip):

In answer to your question @Alexander Regueiro -- in codegen, when we dispatch a foo.bar() call, we invoke codegen_fulfill_obligation

Alexander Regueiro (Sep 25 2019 at 13:59, on Zulip):

okay, thanks

Alexander Regueiro (Sep 25 2019 at 13:59, on Zulip):

I'll still need to do a query in the codegen for the upcast then, I suppose? don't see a way around it

nikomatsakis (Sep 25 2019 at 14:37, on Zulip):

@Alexander Regueiro can you send me a quick link to where you would need the info?

nikomatsakis (Sep 25 2019 at 14:37, on Zulip):

sorry if the link's already in the PR

Alexander Regueiro (Sep 25 2019 at 14:57, on Zulip):

@nikomatsakis yeah, it's here: https://github.com/rust-lang/rust/blob/45859b7ca764cafb14efb8c63a83d5e48dc5d016/src/librustc_codegen_ssa/base.rs#L141

Alexander Regueiro (Sep 25 2019 at 14:57, on Zulip):

also curious how I do a pointer offset in codegen_ssa (I have basicaly no experience with codegen in rustc, I'm afraid.)

Alexander Regueiro (Sep 25 2019 at 15:01, on Zulip):

so yeah, if we can figure out how to do that, and get the appropriate VtableObject value, then bingo.

Alexander Regueiro (Sep 25 2019 at 18:01, on Zulip):

@nikomatsakis hmm, so maybe I can/should call codegen_fulfill_obligation from within the unsized_info method?

Alexander Regueiro (Sep 25 2019 at 18:01, on Zulip):

(in codegen_ssa)

nikomatsakis (Sep 26 2019 at 15:29, on Zulip):

@Alexander Regueiro you said that @eddyb helped you out here, or do you still have pending questions?

Alexander Regueiro (Sep 26 2019 at 15:33, on Zulip):

@nikomatsakis doing some experimentation right now. so I'll see shortly. would be good to have a short sync anyway, if you don't mind. we can cut it short if we don't need all the time?

Alexander Regueiro (Sep 26 2019 at 16:16, on Zulip):

he's gone AFK for now, but here's my last question @nikomatsakis

Alexander Regueiro (Sep 26 2019 at 16:16, on Zulip):

my wider issue is basically that in codegen_rvalue_operand, for unsizing, I need to do something equivalent to coerce_unsized_into but for operands. do I need to make that fn more abstract (to work on operands... not sure how exactly), or do something similar to codegen_rvalue in codegen_rvalue_operand (for unsizing)?

Alexander Regueiro (Sep 26 2019 at 17:00, on Zulip):

hey @nikomatsakis. around now? :-)

nikomatsakis (Sep 26 2019 at 17:01, on Zulip):

yep, just reading your last few comments

nikomatsakis (Sep 26 2019 at 17:02, on Zulip):

I'm going to have to go read into that code

nikomatsakis (Sep 26 2019 at 17:02, on Zulip):

it is pretty thoroughly out of cache

Alexander Regueiro (Sep 26 2019 at 17:02, on Zulip):

okay thanks

nikomatsakis (Sep 26 2019 at 17:02, on Zulip):

ps, is your branch up to date?

Alexander Regueiro (Sep 26 2019 at 17:02, on Zulip):

@nikomatsakis I can push my WIP code right now if you like too

Alexander Regueiro (Sep 26 2019 at 17:02, on Zulip):

heh

Alexander Regueiro (Sep 26 2019 at 17:02, on Zulip):

let me do that.

nikomatsakis (Sep 26 2019 at 17:02, on Zulip):

always useful, thanks

nikomatsakis (Sep 26 2019 at 17:09, on Zulip):

my wider issue is basically that in codegen_rvalue_operand, for unsizing, I need to do something equivalent to coerce_unsized_into but for operands.

ok i'm looking at this fn now

Alexander Regueiro (Sep 26 2019 at 17:09, on Zulip):

done

Alexander Regueiro (Sep 26 2019 at 17:09, on Zulip):

@nikomatsakis ^

Alexander Regueiro (Sep 26 2019 at 17:10, on Zulip):

@nikomatsakis rerunning the trait query in unsized_info is obviously non-ideal. I didn't properly look into your suggestion of using codegen_fulfill_obligation yet.

Alexander Regueiro (Sep 26 2019 at 17:10, on Zulip):

was focusing more on codegn

nikomatsakis (Sep 26 2019 at 17:14, on Zulip):

yep, ok, I'm reading your code now

nikomatsakis (Sep 26 2019 at 17:14, on Zulip):

I still don't really understand quite what you meant when you were saying you needed "something equivalent to coerce_unsized_into

Alexander Regueiro (Sep 26 2019 at 17:15, on Zulip):

okay, well...

Alexander Regueiro (Sep 26 2019 at 17:16, on Zulip):

@nikomatsakis src/librustc_codegen_ssa/mir/rvalue.rs|:227 (the FIXME)

Alexander Regueiro (Sep 26 2019 at 17:16, on Zulip):

does that help clarify?

Alexander Regueiro (Sep 26 2019 at 17:18, on Zulip):

also, note the similarity between the match expr there and the one in coerce_unsized_into (coerce_ptr closure), @nikomatsakis (basically that one just does an additional OperandValue::store but is otherwise no different I think

nikomatsakis (Sep 26 2019 at 17:18, on Zulip):

hmm ok

Alexander Regueiro (Sep 26 2019 at 17:19, on Zulip):

is that clearer? sorry, the code is a bit messy still...

Alexander Regueiro (Sep 26 2019 at 17:19, on Zulip):

well.... it wasn't exactly super-clean to begin with heh.

nikomatsakis (Sep 26 2019 at 17:20, on Zulip):

it's clear-ish

nikomatsakis (Sep 26 2019 at 17:20, on Zulip):

this code has changed a lot since I last looke at it

nikomatsakis (Sep 26 2019 at 17:20, on Zulip):

well idk if that's true, maybe I just forget :)

Alexander Regueiro (Sep 26 2019 at 17:20, on Zulip):

heh, easily done in any casee

nikomatsakis (Sep 26 2019 at 17:23, on Zulip):

in any case, the primary difference between coerce_unsized_into and codegen_rvalue_operand is that the former starts from two LLVM "typed pointers", basically (PlaceRef), whereas the latter also goes from MIR?

nikomatsakis (Sep 26 2019 at 17:23, on Zulip):

er, well, I guess we also return an operand

Alexander Regueiro (Sep 26 2019 at 17:23, on Zulip):

right, I think so.

nikomatsakis (Sep 26 2019 at 17:23, on Zulip):

whihc I guess is sort that store you were talking about

Alexander Regueiro (Sep 26 2019 at 17:23, on Zulip):

both work with OpereandValues though

Alexander Regueiro (Sep 26 2019 at 17:24, on Zulip):

in their corresponding matches

nikomatsakis (Sep 26 2019 at 17:24, on Zulip):

yeah so the heart of coerce_unsized_into is this code:

nikomatsakis (Sep 26 2019 at 17:24, on Zulip):
    let mut coerce_ptr = |ty_a, ty_b| {
        let (base, info) = match bx.load_operand(src).val {
            OperandValue::Pair(base, info) => {
                // FIXME(alexreg): this comment needs updating.
                // Fat-ptr to fat-ptr unsize preserves the vtable,
                // i.e., `&'a fmt::Debug + Send` => `&'a fmt::Debug`
                // So, we need to `pointercast` the base to ensure
                // the types match up.
                let thin_ptr = dst.layout.field(bx.cx(), FAT_PTR_ADDR);
                debug!("FOO: FAT PTR UNSIZE");
                // HACK(eddyb): have to bitcast pointers until LLVM removes pointee types.
                let base = bx.pointercast(base, bx.cx().backend_type(thin_ptr));
                let info = unsized_info(bx, ty_a, ty_b, Some(info));
                (base, info)
            }
            OperandValue::Immediate(base) => {
                unsize_thin_ptr(bx, base, src_ty, dst_ty)
            }
            OperandValue::Ref(..) => bug!(),
        };
        OperandValue::Pair(base, info).store(bx, dst);
    };
nikomatsakis (Sep 26 2019 at 17:25, on Zulip):

which doesn't really want to take a PlaceRef->PlaceRef

nikomatsakis (Sep 26 2019 at 17:25, on Zulip):

that is, it's kind of a wrapper around a Operand -> OPerand I think

Alexander Regueiro (Sep 26 2019 at 17:25, on Zulip):

right, I'm thinking that can be factored out

nikomatsakis (Sep 26 2019 at 17:25, on Zulip):

that sounds right

Alexander Regueiro (Sep 26 2019 at 17:25, on Zulip):

and used in codegen_rvalue_operand

nikomatsakis (Sep 26 2019 at 17:26, on Zulip):

it seems like it is

let mut coerce_ptr = |ty_a, ty_b| {
        let src_operand = bx.load_operand(src);
      let dst_operand = coerce_operand_operand(src_operand);
    dst_operand.store(...);
}
Alexander Regueiro (Sep 26 2019 at 17:26, on Zulip):

but we still need something like src/librustc_codegen_ssa/base.rs:299 (that other match) in codegen_rvalue_operand, no?

Alexander Regueiro (Sep 26 2019 at 17:26, on Zulip):

exactly

Alexander Regueiro (Sep 26 2019 at 17:26, on Zulip):

maybe call it fn coerce_operand_unsized though (?)

Alexander Regueiro (Sep 26 2019 at 17:27, on Zulip):

or fn coerce_unsized_ptr?

nikomatsakis (Sep 26 2019 at 17:27, on Zulip):

yeah I mean whatever :)

nikomatsakis (Sep 26 2019 at 17:27, on Zulip):

I was just being sloppy

Alexander Regueiro (Sep 26 2019 at 17:27, on Zulip):

yeah that's me unnecessarily focusing on details sorry heh

Alexander Regueiro (Sep 26 2019 at 17:27, on Zulip):

so....

nikomatsakis (Sep 26 2019 at 17:27, on Zulip):

anyway, do you need that match on line 299...

nikomatsakis (Sep 26 2019 at 17:28, on Zulip):

I guess the answer is yes

Alexander Regueiro (Sep 26 2019 at 17:28, on Zulip):

@nikomatsakis well I'm alluding to something like that in rvalue.rs:227(the FIXME comment)

nikomatsakis (Sep 26 2019 at 17:28, on Zulip):

because we presumably want to be able to coerce (e.g.) Arc<Foo> to Arc<Bar>

Alexander Regueiro (Sep 26 2019 at 17:28, on Zulip):

yeah

nikomatsakis (Sep 26 2019 at 17:28, on Zulip):

but all that line does

Alexander Regueiro (Sep 26 2019 at 17:29, on Zulip):

the final arm of that match is the tricky one though. it involves recursion of the fn.

nikomatsakis (Sep 26 2019 at 17:29, on Zulip):

well I guess the fn I wrote isn't really how you want to factor it

nikomatsakis (Sep 26 2019 at 17:29, on Zulip):

you probably want a function that takes the result of load_operand combined with a type

nikomatsakis (Sep 26 2019 at 17:29, on Zulip):

as the "source"

Alexander Regueiro (Sep 26 2019 at 17:29, on Zulip):

and seems to be predicated specifically on using places

nikomatsakis (Sep 26 2019 at 17:29, on Zulip):

and then does the match etc

Alexander Regueiro (Sep 26 2019 at 17:29, on Zulip):

hmm

Alexander Regueiro (Sep 26 2019 at 17:29, on Zulip):

oh yes

nikomatsakis (Sep 26 2019 at 17:29, on Zulip):

hmm so the final case, that's the Arc<...> cse

Alexander Regueiro (Sep 26 2019 at 17:29, on Zulip):

I hadn't read closely and just assumed that :-)

nikomatsakis (Sep 26 2019 at 17:29, on Zulip):

and yes it is predicated on places

nikomatsakis (Sep 26 2019 at 17:30, on Zulip):

well so the other thing you can do of course

nikomatsakis (Sep 26 2019 at 17:30, on Zulip):

is to spill :)

nikomatsakis (Sep 26 2019 at 17:31, on Zulip):

(which might well make sense)

Alexander Regueiro (Sep 26 2019 at 17:31, on Zulip):

see the match at rvalue.rs:56... I wonder if we can do something similar in codgen_rvalue_operand?

Alexander Regueiro (Sep 26 2019 at 17:31, on Zulip):

that's what I was alluding to before

Alexander Regueiro (Sep 26 2019 at 17:31, on Zulip):

oh, "spill"?

nikomatsakis (Sep 26 2019 at 17:31, on Zulip):

right, that's what I meant by "spill"

nikomatsakis (Sep 26 2019 at 17:31, on Zulip):

create a temporary if you need one

Alexander Regueiro (Sep 26 2019 at 17:31, on Zulip):

aha yep

nikomatsakis (Sep 26 2019 at 17:31, on Zulip):

that last case feels very painful to handle without spilling; you could potentially spill only in tha case

Alexander Regueiro (Sep 26 2019 at 17:32, on Zulip):

yeah, that's an interesting thought

nikomatsakis (Sep 26 2019 at 17:32, on Zulip):

you will typically have a PlaceRef anyway I think

nikomatsakis (Sep 26 2019 at 17:32, on Zulip):

well I'm not sure how OperandRef works exactly

nikomatsakis (Sep 26 2019 at 17:32, on Zulip):

but in the MIR you are usually casting from a place

Alexander Regueiro (Sep 26 2019 at 17:32, on Zulip):

would it be more efficient to special-case the last case though?

nikomatsakis (Sep 26 2019 at 17:32, on Zulip):

(in theory it can be a constant)

Alexander Regueiro (Sep 26 2019 at 17:32, on Zulip):

because it's obviously more of a pain

Alexander Regueiro (Sep 26 2019 at 17:32, on Zulip):

I see

nikomatsakis (Sep 26 2019 at 17:32, on Zulip):

not sure what you mean by special-case

nikomatsakis (Sep 26 2019 at 17:32, on Zulip):

I am proposing to special case it :)

nikomatsakis (Sep 26 2019 at 17:33, on Zulip):

that is, I am saying that you probably don't want to always create a temporary and store into it, because you typically don't need one

Alexander Regueiro (Sep 26 2019 at 17:34, on Zulip):

sorry, I meant whether to "special-case" in the opposite sense: not create a temporary for the simpler cases.

Alexander Regueiro (Sep 26 2019 at 17:34, on Zulip):

easier to write, but potentially less efficient

nikomatsakis (Sep 26 2019 at 17:35, on Zulip):

ok. yes, I think we're saying the same thing

nikomatsakis (Sep 26 2019 at 17:35, on Zulip):

I have to run but -- I can see why you find this tricky :) -- i'm starting to see what needs to be done, let me know if you do anything else and/or I'll try to look at it a bit more tomorrow

nikomatsakis (Sep 26 2019 at 17:35, on Zulip):

@eddyb is probably also a good one to help, indeed, as they're much closer to this code than I am

Alexander Regueiro (Sep 26 2019 at 17:36, on Zulip):

yeah. I was basically just asking you if it's worth it in terms of efficiency (obviously will defer to you on this, since I don't fully understand thee tradeoffs!)

Alexander Regueiro (Sep 26 2019 at 17:36, on Zulip):

@nikomatsakis sure. as for the codegen_fulfill_obligation... should I be calling that from unsized_info, or what?

Alexander Regueiro (Sep 26 2019 at 17:36, on Zulip):

feel feel to reply whenever.

Alexander Regueiro (Sep 26 2019 at 17:36, on Zulip):

thanks for your time anyway!

Alexander Regueiro (Sep 26 2019 at 17:36, on Zulip):

it's been helpful

nikomatsakis (Sep 30 2019 at 21:21, on Zulip):

@Alexander Regueiro let's chat here

nikomatsakis (Sep 30 2019 at 21:21, on Zulip):

so I saw the gist but maybe the best is if I just look at the state of your branch?

Alexander Regueiro (Sep 30 2019 at 21:22, on Zulip):

sure

Alexander Regueiro (Sep 30 2019 at 21:22, on Zulip):

@Alexander Regueiro @nikomatsakis yep, let me push now...

Alexander Regueiro (Sep 30 2019 at 21:28, on Zulip):

@nikomatsakis pull and grep for // FIXME: what should go here? :-)

Alexander Regueiro (Sep 30 2019 at 21:28, on Zulip):

or...

Alexander Regueiro (Sep 30 2019 at 21:28, on Zulip):

just take a look at the whole of the last commit

Alexander Regueiro (Sep 30 2019 at 21:29, on Zulip):

I think it's all new stuff

nikomatsakis (Sep 30 2019 at 21:34, on Zulip):

ok

nikomatsakis (Sep 30 2019 at 21:38, on Zulip):

something about the names feels wrong to me here

Alexander Regueiro (Sep 30 2019 at 21:40, on Zulip):

names of the fns? sure, suggest new ones, by all means

nikomatsakis (Sep 30 2019 at 21:41, on Zulip):

I'm confused about coerce_unsized_operand

Alexander Regueiro (Sep 30 2019 at 21:41, on Zulip):

the main thing I'm concerned about right now is the functionality though

Alexander Regueiro (Sep 30 2019 at 21:41, on Zulip):

hmm

Alexander Regueiro (Sep 30 2019 at 21:41, on Zulip):

there's no such fn?

nikomatsakis (Sep 30 2019 at 21:42, on Zulip):

indeed, there is no such fn

Alexander Regueiro (Sep 30 2019 at 21:42, on Zulip):

are you looking at an old commit by chance?

nikomatsakis (Sep 30 2019 at 21:42, on Zulip):

that is what is confusing me :)

nikomatsakis (Sep 30 2019 at 21:42, on Zulip):

no, it's commented out code

nikomatsakis (Sep 30 2019 at 21:42, on Zulip):

where did it come from?

Alexander Regueiro (Sep 30 2019 at 21:42, on Zulip):

ah

Alexander Regueiro (Sep 30 2019 at 21:42, on Zulip):

I don't even see it commented out

Alexander Regueiro (Sep 30 2019 at 21:42, on Zulip):

grep returns nothing hmm!

nikomatsakis (Sep 30 2019 at 21:43, on Zulip):

well anyway not imp't

nikomatsakis (Sep 30 2019 at 21:43, on Zulip):

to answer your question

nikomatsakis (Sep 30 2019 at 21:43, on Zulip):
let info = info; // FIXME: what should go here?

that question, I mean

Alexander Regueiro (Sep 30 2019 at 21:44, on Zulip):

yeah

nikomatsakis (Sep 30 2019 at 21:44, on Zulip):

I thnk that the setup is sort of off

Alexander Regueiro (Sep 30 2019 at 21:44, on Zulip):

okay it may well be

Alexander Regueiro (Sep 30 2019 at 21:44, on Zulip):

I'm really a newb when it comes to codegen!

Alexander Regueiro (Sep 30 2019 at 21:44, on Zulip):

just been looking at examples in other places to try to learn

nikomatsakis (Sep 30 2019 at 21:45, on Zulip):

basically I think the OperandValue you want to return is to load from dst_scratch

Alexander Regueiro (Sep 30 2019 at 21:45, on Zulip):

right, exactly...

nikomatsakis (Sep 30 2019 at 21:45, on Zulip):

but I'm wondering now how that would interact with storage-dea

Alexander Regueiro (Sep 30 2019 at 21:45, on Zulip):

ah yes

Alexander Regueiro (Sep 30 2019 at 21:45, on Zulip):

I was wondering about that

Alexander Regueiro (Sep 30 2019 at 21:45, on Zulip):

whether it invalidates the operand even if I do storage-dead after fetching the operand value

Alexander Regueiro (Sep 30 2019 at 21:46, on Zulip):

if not, maybe I can just do OperandValue::Pair(dst_scratch.llval, dst_scratch.llextra) for that line? though that seems suspect too?

nikomatsakis (Sep 30 2019 at 21:48, on Zulip):

I think that's invalid, yes

Alexander Regueiro (Sep 30 2019 at 21:48, on Zulip):

(BTW, I pushed again. no major changes, just fixed some build errors due to copy & paste coding...)

Alexander Regueiro (Sep 30 2019 at 21:49, on Zulip):

hmm

nikomatsakis (Sep 30 2019 at 21:49, on Zulip):

so I feel like, to go this way, you really don't want to be returning an operand

nikomatsakis (Sep 30 2019 at 21:49, on Zulip):

you want to be given a dest

nikomatsakis (Sep 30 2019 at 21:49, on Zulip):

looking a bit further out

nikomatsakis (Sep 30 2019 at 21:49, on Zulip):

at the callers of this function

nikomatsakis (Sep 30 2019 at 21:49, on Zulip):

one of them has a destination

Alexander Regueiro (Sep 30 2019 at 21:49, on Zulip):

possibly

nikomatsakis (Sep 30 2019 at 21:49, on Zulip):

the other does not

nikomatsakis (Sep 30 2019 at 21:49, on Zulip):

however, the other caller comes from a statement assign

nikomatsakis (Sep 30 2019 at 21:49, on Zulip):

in particular we have some logic for cases like x = foo where we tried to avoid an alloca for x

nikomatsakis (Sep 30 2019 at 21:50, on Zulip):

if x has a "sufficiently complicated" type we'll create one

Alexander Regueiro (Sep 30 2019 at 21:50, on Zulip):

the question is: can coerce_ptr_unsized even get called with an op that has an ADT, in practice? I presume the answer is yes.

nikomatsakis (Sep 30 2019 at 21:50, on Zulip):

basically a non-immediate type (and we include pairs in that case)

nikomatsakis (Sep 30 2019 at 21:50, on Zulip):

the question is: can coerce_ptr_unsized even get called with an op that has an ADT, in practice? I presume the answer is yes.

I think you kind of want to split out the path where the case is "yes" from the case where it is "no"

nikomatsakis (Sep 30 2019 at 21:50, on Zulip):

is what I'm getting at

Alexander Regueiro (Sep 30 2019 at 21:50, on Zulip):

it's only called a) recursively b) by cosdegen_rvalue_operand

nikomatsakis (Sep 30 2019 at 21:51, on Zulip):

in particular, codegen_rvalue_operand is invoked from the assignment code I was talking about above

Alexander Regueiro (Sep 30 2019 at 21:51, on Zulip):

which itself needs to spit out an OperandRef

Alexander Regueiro (Sep 30 2019 at 21:51, on Zulip):

not assign to a PlaceRef

Alexander Regueiro (Sep 30 2019 at 21:51, on Zulip):

hmm

nikomatsakis (Sep 30 2019 at 21:52, on Zulip):

notice that codegen_rvalue_operand already has some logic that assumes this

nikomatsakis (Sep 30 2019 at 21:52, on Zulip):

e.g.

nikomatsakis (Sep 30 2019 at 21:52, on Zulip):
            mir::Rvalue::Repeat(..) |
            mir::Rvalue::Aggregate(..) => {
                // According to `rvalue_creates_operand`, only ZST
                // aggregate rvalues are allowed to be operands.
                let ty = rvalue.ty(self.mir, self.cx.tcx());
                let operand = OperandRef::new_zst(
                    &mut bx,
                    self.cx.layout_of(self.monomorphize(&ty)),
                );
                (bx, operand)
            }
nikomatsakis (Sep 30 2019 at 21:52, on Zulip):

i.e., it assumes it can be invoked with a Foo { ... } operand that constructs a struct, but only if that struct is a ZST

nikomatsakis (Sep 30 2019 at 21:52, on Zulip):

now I don't think this means you can't have an ADT necessarily

Alexander Regueiro (Sep 30 2019 at 21:52, on Zulip):

hmm

nikomatsakis (Sep 30 2019 at 21:52, on Zulip):

it's just that your ADT must be represented by a (ptr, info) pair

nikomatsakis (Sep 30 2019 at 21:53, on Zulip):

and not some more general structure

nikomatsakis (Sep 30 2019 at 21:53, on Zulip):

I'm not 100% sure about that

nikomatsakis (Sep 30 2019 at 21:53, on Zulip):

you could test with e.g. Arc

Alexander Regueiro (Sep 30 2019 at 21:53, on Zulip):

yeah, so it's already a fat pointer to an ADT?

nikomatsakis (Sep 30 2019 at 21:53, on Zulip):

but the decision of whether to use an alloca for a type is made in the non_ssa_locals function

nikomatsakis (Sep 30 2019 at 21:53, on Zulip):
       let layout = fx.cx.spanned_layout_of(ty, span);
        if fx.cx.is_backend_immediate(layout) {
            // These sorts of types are immediates that we can store
            // in an Value without an alloca.
        } else if fx.cx.is_backend_scalar_pair(layout) {
            // We allow pairs and uses of any of their 2 fields.
nikomatsakis (Sep 30 2019 at 21:53, on Zulip):

and it seems to be based on the layout

nikomatsakis (Sep 30 2019 at 21:53, on Zulip):

(which makes sense)

nikomatsakis (Sep 30 2019 at 21:54, on Zulip):

yeah, so it's already a fat pointer to an ADT?

no

nikomatsakis (Sep 30 2019 at 21:54, on Zulip):

well I don't know what "fat pointer to an ADT means"

nikomatsakis (Sep 30 2019 at 21:54, on Zulip):

but e.g. an Arc<dyn Debug> would be represented (I think) by a (ptr, vtable) pair where the ptr is the Arc<_> pointer --- i.e., it's internal pointer to some shared box with a ref count

Alexander Regueiro (Sep 30 2019 at 21:54, on Zulip):

e.g. &dyn FooStruct or Box<dyn FooStruct>?

nikomatsakis (Sep 30 2019 at 21:55, on Zulip):

dyn FooStruct is a contradiction, no?

Alexander Regueiro (Sep 30 2019 at 21:55, on Zulip):

okay

Alexander Regueiro (Sep 30 2019 at 21:55, on Zulip):

ughh

Alexander Regueiro (Sep 30 2019 at 21:55, on Zulip):

I'm tired, sorry

Alexander Regueiro (Sep 30 2019 at 21:55, on Zulip):

long day

nikomatsakis (Sep 30 2019 at 21:55, on Zulip):

basically if you had a Arc<T>, it would be represented by (in C terms) a *const T

nikomatsakis (Sep 30 2019 at 21:55, on Zulip):

actually a *const ArcData<T> or something

nikomatsakis (Sep 30 2019 at 21:55, on Zulip):

and so the layout in this case would be a (*const ArcLayout<T>, vtable)

nikomatsakis (Sep 30 2019 at 21:56, on Zulip):

I guess that's C terms but Rust syntax :) or .. something

Alexander Regueiro (Sep 30 2019 at 21:56, on Zulip):

guess I was thinking &dyn Trait where FooStruct: Trait

nikomatsakis (Sep 30 2019 at 21:56, on Zulip):

point is, the Arc<T> struct is really just a pointer

Alexander Regueiro (Sep 30 2019 at 21:56, on Zulip):

okay

nikomatsakis (Sep 30 2019 at 21:56, on Zulip):

anyway the reason I mention all that is

nikomatsakis (Sep 30 2019 at 21:56, on Zulip):

the logic that "spills" to temporary stack slots is intended for more complex cases

Alexander Regueiro (Sep 30 2019 at 21:56, on Zulip):

in that case... wouldn't it just be handled by one of the previous two arms of the match?

nikomatsakis (Sep 30 2019 at 21:56, on Zulip):

where the struct has arbitray fields and things

nikomatsakis (Sep 30 2019 at 21:57, on Zulip):

in that case... wouldn't it just be handled by one of the previous two arms of the match?

no, because the match is matching on it's Rust type

nikomatsakis (Sep 30 2019 at 21:57, on Zulip):

however

Alexander Regueiro (Sep 30 2019 at 21:57, on Zulip):

right, not its LLVM type...

Alexander Regueiro (Sep 30 2019 at 21:57, on Zulip):

makes sense

nikomatsakis (Sep 30 2019 at 21:58, on Zulip):

this logic here:

                (&ty::Adt(def_a, _), &ty::Adt(def_b, _))
                    if def_a.is_box() && def_b.is_box() =>
                {
                    unsized_info(bx, src.ty.boxed_ty(), dst.ty.boxed_ty(), Some(info))
                }

is .. probably what we want, more or less, except that it's hardcoded to box

Alexander Regueiro (Sep 30 2019 at 21:59, on Zulip):

do we want something more like the final match arm in coerce_unsized_into then?

Alexander Regueiro (Sep 30 2019 at 21:59, on Zulip):

don't know how that can be adapted for operands and not places though

nikomatsakis (Sep 30 2019 at 21:59, on Zulip):

heh man this is annoying

nikomatsakis (Sep 30 2019 at 22:00, on Zulip):

so let's first just try to get it working

nikomatsakis (Sep 30 2019 at 22:00, on Zulip):

then we'll worry about making it a bit more efficient

Alexander Regueiro (Sep 30 2019 at 22:00, on Zulip):

yep

Alexander Regueiro (Sep 30 2019 at 22:00, on Zulip):

at least the simple case is working now :-P

nikomatsakis (Sep 30 2019 at 22:00, on Zulip):

oh wait, wait

Alexander Regueiro (Sep 30 2019 at 22:00, on Zulip):

just not upcasting things like Arc

Alexander Regueiro (Sep 30 2019 at 22:00, on Zulip):

I gues...

nikomatsakis (Sep 30 2019 at 22:00, on Zulip):

ok ok so there are two callers of coerce_ptr_unsized but

nikomatsakis (Sep 30 2019 at 22:00, on Zulip):

both of them are assuming that the source operand is either a pointer or a (pointer, info) pair

nikomatsakis (Sep 30 2019 at 22:01, on Zulip):

in the case of coerce_unsized_into, that's because it's already done the logic of matching the fields of the adts

Alexander Regueiro (Sep 30 2019 at 22:01, on Zulip):

yes that's true

Alexander Regueiro (Sep 30 2019 at 22:01, on Zulip):

but the second?

Alexander Regueiro (Sep 30 2019 at 22:01, on Zulip):

I was worried about that one

nikomatsakis (Sep 30 2019 at 22:01, on Zulip):

in the case of the rvalue coercion fn, that's because of the limits on local variables (more complex type would be spilled to alloca)

Alexander Regueiro (Sep 30 2019 at 22:02, on Zulip):

oh. so you're saying codegen_rvalue_operand would never be called for complex ADTs even?

nikomatsakis (Sep 30 2019 at 22:03, on Zulip):

correct

nikomatsakis (Sep 30 2019 at 22:03, on Zulip):

we would call codegen_rvalue_into or something like that

Alexander Regueiro (Sep 30 2019 at 22:03, on Zulip):

cool

Alexander Regueiro (Sep 30 2019 at 22:03, on Zulip):

or just bug! out?

Alexander Regueiro (Sep 30 2019 at 22:03, on Zulip):

up to you

nikomatsakis (Sep 30 2019 at 22:03, on Zulip):

no I mean

nikomatsakis (Sep 30 2019 at 22:03, on Zulip):

oh. so you're saying codegen_rvalue_operand would never be called for complex ADTs even?

it is a bug to call this for a more complex ADT

nikomatsakis (Sep 30 2019 at 22:03, on Zulip):

the caller ought to have invoked one of the into variants

nikomatsakis (Sep 30 2019 at 22:03, on Zulip):

so I think what you want to do in coerce_ptr_unsized

nikomatsakis (Sep 30 2019 at 22:04, on Zulip):

you basically want to invoke unsized_info, but the trick is figuring out what type to invoke it with

Alexander Regueiro (Sep 30 2019 at 22:04, on Zulip):

then isn't it a bug to call coerce_ptr_unsized on a more complex ADT likewise, since you gave the only two call sites above?

Alexander Regueiro (Sep 30 2019 at 22:04, on Zulip):

okay...

nikomatsakis (Sep 30 2019 at 22:04, on Zulip):

ah wait

nikomatsakis (Sep 30 2019 at 22:05, on Zulip):

I think maybe this will work?

nikomatsakis (Sep 30 2019 at 22:05, on Zulip):
                (&ty::Adt(def_a, _), &ty::Adt(def_b, _)) => {
                    assert_eq!(def_a, def_b);
                    unsized_info(bx, src.ty, dst.ty, Some(info))
                }
nikomatsakis (Sep 30 2019 at 22:06, on Zulip):

in particular, unsized_info seems to already expect two instance of the same ADT, and it knows to walk their fields in lockstep

nikomatsakis (Sep 30 2019 at 22:06, on Zulip):

(the code for box might be similarly simplified and mergd into this arm, actually)

Alexander Regueiro (Sep 30 2019 at 22:06, on Zulip):

line 131 then?

Alexander Regueiro (Sep 30 2019 at 22:06, on Zulip):

of that file

nikomatsakis (Sep 30 2019 at 22:07, on Zulip):

I pushed a commit to your branch

nikomatsakis (Sep 30 2019 at 22:07, on Zulip):

not sure if it works

nikomatsakis (Sep 30 2019 at 22:07, on Zulip):

but I got to run

nikomatsakis (Sep 30 2019 at 22:07, on Zulip):

I think it may be correct

nikomatsakis (Sep 30 2019 at 22:07, on Zulip):

in which case I think the match arm I just modified and the match arm before it can be mergd

Alexander Regueiro (Sep 30 2019 at 22:07, on Zulip):

okay thanks!

Alexander Regueiro (Sep 30 2019 at 22:07, on Zulip):

sounds good

Alexander Regueiro (Sep 30 2019 at 22:07, on Zulip):

makes sense actually

Alexander Regueiro (Sep 30 2019 at 22:08, on Zulip):

I'll see if it gets rid of the bootstrap error btw, which is: https://gist.github.com/e20f1817eb2ced1ec25ba706f8a9c31a

nikomatsakis (Sep 30 2019 at 22:08, on Zulip):

cool, I'm doing a local build too

nikomatsakis (Sep 30 2019 at 22:08, on Zulip):

(the box code is taking Box<T> -> Box<U> coercion and invoking that helper with T, U...

Alexander Regueiro (Sep 30 2019 at 22:08, on Zulip):

it's an LLVM assertion issue with the bootstrap, so that's a bit disconcerting, but oh well..

nikomatsakis (Sep 30 2019 at 22:08, on Zulip):

but from what I can see, the helper would deal just fine with Box arguments)

Alexander Regueiro (Sep 30 2019 at 22:08, on Zulip):

with the bootstrap*

Alexander Regueiro (Sep 30 2019 at 22:09, on Zulip):

cool

nikomatsakis (Sep 30 2019 at 22:09, on Zulip):

cool, I'm doing a local build too

I'm just doing a ./x.py build -i so I guess i'll find out

Alexander Regueiro (Sep 30 2019 at 22:09, on Zulip):

yeah heh

Alexander Regueiro (Sep 30 2019 at 22:10, on Zulip):

thanks for working over this with me. if you could look at what I wrote about the REPL PRs / compiler team meetings and whatnot in your own time, and let me know, that would be great. a synchronous chat about that is less important probably!

Alexander Regueiro (Sep 30 2019 at 22:11, on Zulip):

@nikomatsakis anyway, see you later. I'm off for now too.

Alexander Regueiro (Sep 30 2019 at 22:12, on Zulip):

@nikomatsakis oh, and I don't think you pushed FYI.

nikomatsakis (Sep 30 2019 at 22:12, on Zulip):

oh it got rejected

nikomatsakis (Sep 30 2019 at 22:12, on Zulip):

not sure why

Alexander Regueiro (Sep 30 2019 at 22:13, on Zulip):

sorry. probably because of my fixing-build-errors force-push while we were chatting

nikomatsakis (Sep 30 2019 at 22:13, on Zulip):

ok

nikomatsakis (Sep 30 2019 at 22:13, on Zulip):

I'll rebase

Alexander Regueiro (Sep 30 2019 at 22:13, on Zulip):

or force push, and I can rebase. don't mind much.

Alexander Regueiro (Sep 30 2019 at 22:13, on Zulip):

cheers!

nikomatsakis (Sep 30 2019 at 22:14, on Zulip):

done

nikomatsakis (Sep 30 2019 at 22:16, on Zulip):

(I get a crash trying to bootstrap =)

Alexander Regueiro (Sep 30 2019 at 23:18, on Zulip):

heh not too surprised

Alexander Regueiro (Oct 01 2019 at 17:21, on Zulip):

hey @nikomatsakis

Alexander Regueiro (Oct 01 2019 at 17:21, on Zulip):

so..

Alexander Regueiro (Oct 01 2019 at 17:21, on Zulip):
    let bar: Arc<dyn Bar> = Arc::new(42);
    let foo: Arc<dyn Foo> = bar;
Alexander Regueiro (Oct 01 2019 at 17:21, on Zulip):

that should work,right?

Alexander Regueiro (Oct 01 2019 at 17:22, on Zulip):

but I get

error: internal compiler error: src/librustc_codegen_ssa/base.rs:164: unsized_info: invalid unsizing std::marker::PhantomData<dyn Bar> -> std::marker::PhantomData<dyn Foo>
Alexander Regueiro (Oct 01 2019 at 17:22, on Zulip):

@nikomatsakis let me know if you have any ideas (about this or the LLVM assertion bootstrapping issue)

Alexander Regueiro (Oct 01 2019 at 17:31, on Zulip):

there may be an easy fix to that actually... let's see

Alexander Regueiro (Oct 01 2019 at 17:55, on Zulip):

@nikomatsakis added this to unsized_info, and the cast "works", but it segfaults when I try to print foo:

        (&ty::Adt(def_a, _), &ty::Adt(def_b, _))
            if def_a.is_phantom_data() && def_b.is_phantom_data() =>
        {
            old_info.expect("unsized_info: missing old info")
        }
nikomatsakis (Oct 03 2019 at 13:26, on Zulip):

@Alexander Regueiro that hack seems ok for now, let's revisit that problem

nikomatsakis (Oct 03 2019 at 13:26, on Zulip):

we gotta figure out the cause of these segfaults though

Alexander Regueiro (Oct 03 2019 at 16:09, on Zulip):

sure

Alexander Regueiro (Oct 03 2019 at 16:09, on Zulip):

@nikomatsakis I don't know if that hack is causing the segfault... but yeah the segfault there and the weird bootstrapping LLVM error... that's beyond me!

nikomatsakis (Oct 04 2019 at 18:41, on Zulip):

@Alexander Regueiro I don't see any LLVM error when bootstrapping

nikomatsakis (Oct 04 2019 at 18:41, on Zulip):

I guess maybe it doesn't get that far

nikomatsakis (Oct 04 2019 at 18:41, on Zulip):

but those are usually good hint :)

nikomatsakis (Oct 04 2019 at 18:42, on Zulip):

let me try rebuilding with verify-llvm-ir=true I guess

Alexander Regueiro (Oct 04 2019 at 19:38, on Zulip):

oh interesting. maybe it was a consequence of --keep-stage 0?

Alexander Regueiro (Oct 04 2019 at 19:39, on Zulip):

or actually, of -i

Alexander Regueiro (Oct 04 2019 at 19:39, on Zulip):

and not removing the LLVM artefacts

Alexander Regueiro (Oct 04 2019 at 19:39, on Zulip):

you saw the segfault though yes?

Alexander Regueiro (Oct 04 2019 at 19:39, on Zulip):

@nikomatsakis

nikomatsakis (Oct 04 2019 at 20:05, on Zulip):

I do see a segfault

nikomatsakis (Oct 04 2019 at 20:05, on Zulip):

I'm trying to reproduce it in some way in a debugger

nikomatsakis (Oct 04 2019 at 20:05, on Zulip):

and failing

nikomatsakis (Oct 04 2019 at 20:06, on Zulip):

like, I literally cannot figure out what command to run :)

nikomatsakis (Oct 04 2019 at 20:06, on Zulip):

the last thing I see is

nikomatsakis (Oct 04 2019 at 20:06, on Zulip):
     Running `/home/nmatsakis/versioned/rust-2/build/bootstrap/debug/rustc --edition=2018 --crate-name core
nikomatsakis (Oct 04 2019 at 20:06, on Zulip):

and the "boostrap rustc" is kind of impossible to run from outside x.py in my experience :)

lqd (Oct 04 2019 at 20:09, on Zulip):

one can add --on-fail=env to print the environment IIRC

lqd (Oct 04 2019 at 20:10, on Zulip):

likely that --on-fail=bash might work to launch a debugger from there but I've never tried that

simulacrum (Oct 04 2019 at 20:20, on Zulip):

@nikomatsakis whenever you see bootstrap rustc you should be able to easily replace it with the stage1/bin/rustc or so and that would generally just work

simulacrum (Oct 04 2019 at 20:20, on Zulip):

you might need to throw in a few env variables, but most of the time you can just SOME_ENV=foo and it'll be fine

nikomatsakis (Oct 04 2019 at 20:21, on Zulip):

lately i've foudn that is not true

nikomatsakis (Oct 04 2019 at 20:21, on Zulip):

that used to be true

nikomatsakis (Oct 04 2019 at 20:21, on Zulip):

I'll give it a try though, maybe I'm wrong

nikomatsakis (Oct 04 2019 at 20:21, on Zulip):

likely that --on-fail=bash might work to launch a debugger from there but I've never tried that

that used to work but it stopped working for me at some point

lqd (Oct 04 2019 at 20:23, on Zulip):

yeah I now remember there's an issue saying it doesn't work anymore :/

nikomatsakis (Oct 04 2019 at 20:24, on Zulip):

ah, good news

nikomatsakis (Oct 04 2019 at 20:24, on Zulip):

for some reason I am now seeing the actual rustc: command output...

nikomatsakis (Oct 04 2019 at 20:24, on Zulip):
rustc command: "LD_LIBRARY_PATH"="/home/nmatsakis/versioned/rust-2/build/x86_64-unknown-linux-gnu/stage1/lib:/home/nmatsakis/versioned/rust-2/build/x86_64-unknown-linux-gnu/stage1-std/release/deps:/home/nma\
tsakis/versioned/rust-2/build/x86_64-unknown-linux-gnu/stage0/lib"

copying and pasting that usually works for me...

simulacrum (Oct 04 2019 at 20:25, on Zulip):

hm strange.. I would not expect that to be the case.

simulacrum (Oct 04 2019 at 20:25, on Zulip):

maybe for stage 0 but not stage 1

nikomatsakis (Oct 04 2019 at 20:26, on Zulip):

ok @Alexander Regueiro so I am seeing this

Invalid InsertValueInst operands!
  %38 = insertvalue { {}*, [3 x i64]* } %37, i64* getelementptr inbounds ([3 x i64], [3 x i64]* bitcast ({ void (%"fmt::builders::PadAdapter"*)*, i64, i64, i1 (%"fmt::builders::PadAdapter"*, [0 x i8]*, i64)\
*, i1 (%"fmt::builders::PadAdapter"*, i32)*, i1 (%"fmt::builders::PadAdapter"*, %"fmt::Arguments"*)* }* @3 to [3 x i64]*), i32 0, i32 0), 1, !dbg !189
in function _ZN4core3fmt8builders10PadAdapter4wrap28_$u7b$$u7b$closure$u7d$$u7d$17he99dacb16de69f1eE
LLVM ERROR: Broken function found, compilation aborted!
``

now that I rebuilt with llvm assertions
nikomatsakis (Oct 04 2019 at 20:26, on Zulip):

although hmm

Alexander Regueiro (Oct 04 2019 at 20:28, on Zulip):

I do see a segfault

for the Arc case, @nikomatsakis ?

Alexander Regueiro (Oct 04 2019 at 20:29, on Zulip):

ah righg

nikomatsakis (Oct 04 2019 at 20:29, on Zulip):

so this looks like a

nikomatsakis (Oct 04 2019 at 20:30, on Zulip):

&mut dyn Write -> &mut dyn Write coercion

nikomatsakis (Oct 04 2019 at 20:30, on Zulip):

going wrong somehow ;)

Alexander Regueiro (Oct 04 2019 at 20:30, on Zulip):

hmm

Alexander Regueiro (Oct 04 2019 at 20:30, on Zulip):

how is that a coercion even though?

Alexander Regueiro (Oct 04 2019 at 20:30, on Zulip):

the same type

nikomatsakis (Oct 04 2019 at 20:30, on Zulip):

it is

nikomatsakis (Oct 04 2019 at 20:30, on Zulip):

because of lifetime bounds

Alexander Regueiro (Oct 04 2019 at 20:30, on Zulip):

oh

Alexander Regueiro (Oct 04 2019 at 20:30, on Zulip):

duh

Alexander Regueiro (Oct 04 2019 at 20:31, on Zulip):

so... maybe I removed too much of the previous code? hmm

nikomatsakis (Oct 04 2019 at 20:31, on Zulip):

honestly I'm having a hard time even parsing that type ;)

Alexander Regueiro (Oct 04 2019 at 20:31, on Zulip):

which only had to deal with lifetime bound coercions for trait objects

nikomatsakis (Oct 04 2019 at 20:31, on Zulip):
  %38 = insertvalue { {}*, [3 x i64]* } %37, i64* getelementptr inbounds ([3 x i64], [3 x i64]* bitcast ({ void (%"fmt::builders::PadAdapter"*)*, i64, i64, i1 (%"fmt::builders::PadAdapter"*, [0 x i8]*, i64)\
*, i1 (%"fmt::builders::PadAdapter"*, i32)*, i1 (%"fmt::builders::PadAdapter"*, %"fmt::Arguments"*)* }* @3 to [3 x i64]*), i32 0, i32 0), 1, !dbg !189
Alexander Regueiro (Oct 04 2019 at 20:31, on Zulip):

since auto-traits were basically irrelevant

Alexander Regueiro (Oct 04 2019 at 20:31, on Zulip):

yeah... that's not exactly readable

Alexander Regueiro (Oct 04 2019 at 20:32, on Zulip):

presumably called for println! stmt or such though

nikomatsakis (Oct 04 2019 at 20:33, on Zulip):
  %38 =
  insertvalue { {}*, [3 x i64]* } %37,
  i64* getelementptr inbounds (
      [3 x i64],
      [3 x i64]* bitcast (
          {
              void (%"fmt::builders::PadAdapter"*)*, i64, i64, i1 (%"fmt::builders::PadAdapter"*, [0 x i8]*, i64)*, i1 (%"fmt::builders::PadAdapter"*, i32)*, i1 (%"fmt::builders::PadAdapter"*, %"fmt::Argume\
nts"*)*
          }* @3
          to
          [3 x i64]*
      ),
      i32 0,
      i32 0
  ),
  1,
  !dbg !189
nikomatsakis (Oct 04 2019 at 20:33, on Zulip):

used emacs to at least pare up all the delimiters :)

nikomatsakis (Oct 04 2019 at 20:34, on Zulip):

I guess that's some kind of vtable cast

nikomatsakis (Oct 04 2019 at 20:35, on Zulip):

insertvalue llvm docs

Alexander Regueiro (Oct 04 2019 at 20:35, on Zulip):

yes... I think so.

nikomatsakis (Oct 04 2019 at 20:36, on Zulip):

ok I sort of see what it's complaining about

nikomatsakis (Oct 04 2019 at 20:36, on Zulip):

the gep winds up pointing at a specific i64 from the vtable

nikomatsakis (Oct 04 2019 at 20:36, on Zulip):

but it is being inserted into a [3 x i164]* field

nikomatsakis (Oct 04 2019 at 20:36, on Zulip):

it kind of looks like there is one argument too many on the gep

nikomatsakis (Oct 04 2019 at 20:36, on Zulip):

i.e., instead of gep(..., 0, 0) it should be gep(..., 0)

nikomatsakis (Oct 04 2019 at 20:36, on Zulip):

is this maybe ringing any bells for any of your edits? :)

Alexander Regueiro (Oct 04 2019 at 20:37, on Zulip):

/me thinks

nikomatsakis (Oct 04 2019 at 20:37, on Zulip):

I'm guessing it has to do something with the code that finds a "child" vtable

Alexander Regueiro (Oct 04 2019 at 20:37, on Zulip):

I do this: bx.struct_gep(source_ptr, offset as u64)

nikomatsakis (Oct 04 2019 at 20:37, on Zulip):

this is this code

nikomatsakis (Oct 04 2019 at 20:37, on Zulip):
            debug!("FOO: unsized_info: vtable={:?} offset={:?} ", vtable, offset);
            bx.struct_gep(source_ptr, offset as u64)
Alexander Regueiro (Oct 04 2019 at 20:37, on Zulip):

the Rust interface to LLVM is very strongly-typed so I don't see how that can go wrong

Alexander Regueiro (Oct 04 2019 at 20:37, on Zulip):

unless it's simply the wrong offset?

nikomatsakis (Oct 04 2019 at 20:38, on Zulip):

no, it's more the number of arugments

nikomatsakis (Oct 04 2019 at 20:38, on Zulip):

than the value of them

nikomatsakis (Oct 04 2019 at 20:38, on Zulip):

I don't think the rust interface to llvm is this strongly typed

nikomatsakis (Oct 04 2019 at 20:38, on Zulip):

it doesn't reflect the llvm types of what's inside, does it?

Alexander Regueiro (Oct 04 2019 at 20:38, on Zulip):

hmm

Alexander Regueiro (Oct 04 2019 at 20:39, on Zulip):

of what's inside ptr's? no

Alexander Regueiro (Oct 04 2019 at 20:39, on Zulip):

you're right there

nikomatsakis (Oct 04 2019 at 20:39, on Zulip):

anyway I'm trying to dump out sme debug print outs

nikomatsakis (Oct 04 2019 at 20:39, on Zulip):

this is also an inbounds gep

nikomatsakis (Oct 04 2019 at 20:39, on Zulip):

so maybe it's not that code

Alexander Regueiro (Oct 04 2019 at 20:40, on Zulip):

struct_gep is the right call, isn't it?

Alexander Regueiro (Oct 04 2019 at 20:41, on Zulip):

I figured out the vtable is stored as a struct not an array

Alexander Regueiro (Oct 04 2019 at 20:41, on Zulip):

so presume it is

nikomatsakis (Oct 04 2019 at 20:42, on Zulip):

not sure

Alexander Regueiro (Oct 04 2019 at 20:42, on Zulip):

I was doing inbounds_gep before, and that created problems

Alexander Regueiro (Oct 04 2019 at 20:42, on Zulip):

so thismakes sense

nikomatsakis (Oct 04 2019 at 20:43, on Zulip):

I forget the difference to be totally honest :P

Alexander Regueiro (Oct 04 2019 at 20:43, on Zulip):

struct_gep isn't translated to that LLVM at any stage, is it?

Alexander Regueiro (Oct 04 2019 at 20:43, on Zulip):

heh yeah

Alexander Regueiro (Oct 04 2019 at 20:43, on Zulip):

it's weird to me

nikomatsakis (Oct 04 2019 at 20:43, on Zulip):

don't think so

nikomatsakis (Oct 04 2019 at 20:43, on Zulip):

but I'm looking at the backtrace

nikomatsakis (Oct 04 2019 at 20:43, on Zulip):

still trying to figure out where this particular llvm instruction is being generated

nikomatsakis (Oct 04 2019 at 20:43, on Zulip):

I'm not sure it's that code you cited

nikomatsakis (Oct 04 2019 at 20:43, on Zulip):

doesn't quite fit

Matthew Jasper (Oct 04 2019 at 20:43, on Zulip):

ProjectionElem::Index uses inbounds_gep

nikomatsakis (Oct 04 2019 at 20:44, on Zulip):

yeah

nikomatsakis (Oct 04 2019 at 20:44, on Zulip):

I saw that...

nikomatsakis (Oct 04 2019 at 20:44, on Zulip):

/me runs in gdb to get backtrace

nikomatsakis (Oct 04 2019 at 20:44, on Zulip):

oh I should disable threading

nikomatsakis (Oct 04 2019 at 20:45, on Zulip):

that's probably confusing a lot of things

nikomatsakis (Oct 04 2019 at 20:45, on Zulip):

how...do you do that

simulacrum (Oct 04 2019 at 20:45, on Zulip):

-Ccodegen-units=1 is a good start

simulacrum (Oct 04 2019 at 20:46, on Zulip):

-Zno-parallel-llvm also

simulacrum (Oct 04 2019 at 20:46, on Zulip):

(maybe just the latter is enough)

nikomatsakis (Oct 04 2019 at 20:46, on Zulip):

yeah I just came to both of those :)

nikomatsakis (Oct 04 2019 at 20:46, on Zulip):

thanks

nikomatsakis (Oct 04 2019 at 20:46, on Zulip):

I don't think it hurts here either way, I'll add 'em both

nikomatsakis (Oct 04 2019 at 20:51, on Zulip):

hmm that causes a segfault and not the llvm assertion

nikomatsakis (Oct 04 2019 at 20:51, on Zulip):

so maybe that was a red herring

Alexander Regueiro (Oct 04 2019 at 20:51, on Zulip):

@nikomatsakis there's projections being done for complex ADT upcasting... and I'm changing how that code is hit

nikomatsakis (Oct 04 2019 at 20:53, on Zulip):

@Alexander Regueiro it looks like struct-gep is compiled to inbounds gep

  Value *CreateStructGEP(Type *Ty, Value *Ptr, unsigned Idx,
                         const Twine &Name = "") {
    return CreateConstInBoundsGEP2_32(Ty, Ptr, 0, Idx, Name);
  }
Alexander Regueiro (Oct 04 2019 at 20:53, on Zulip):

ahh

nikomatsakis (Oct 04 2019 at 20:54, on Zulip):

so that explains that

Alexander Regueiro (Oct 04 2019 at 20:54, on Zulip):

yeah I wondered

nikomatsakis (Oct 04 2019 at 20:54, on Zulip):

can you remind me what you are trying to do here :)

nikomatsakis (Oct 04 2019 at 20:54, on Zulip):

like, what vtable layout you have created

nikomatsakis (Oct 04 2019 at 20:54, on Zulip):

it's basically a big array

nikomatsakis (Oct 04 2019 at 20:54, on Zulip):

with "subvtables" embedded in it, right?

nikomatsakis (Oct 04 2019 at 20:55, on Zulip):

er, "Super" vtables I guess :)

nikomatsakis (Oct 04 2019 at 21:00, on Zulip):

also, what is the offset measured in?

nikomatsakis (Oct 04 2019 at 21:00, on Zulip):

I'm pretty sure the assert was not wrong, but something about -Zno-parallel-llvm leads to us squelching the error somehow

nikomatsakis (Oct 04 2019 at 21:00, on Zulip):

I think the struct_gep is just wrong

nikomatsakis (Oct 04 2019 at 21:01, on Zulip):

the type that LLVM sees for this vtable is [3 x i64]* basically -- so calling struct_gep(0), as you are doing, inserts a in-bounds gep of [0, 0]. In other words, you don't have a pointer to a struct from LLVM's POV, and that method is tailored to access a field of a struct.

nikomatsakis (Oct 04 2019 at 21:02, on Zulip):

I suspect you want a gep with one argument, to basically simulate (in C) ptr + offset

nikomatsakis (Oct 04 2019 at 21:02, on Zulip):

but the question is what the offset is measured in (I'd sort of expect "words", in which case we have the wrong llvm type)

nikomatsakis (Oct 04 2019 at 21:02, on Zulip):

I guess that when bootstrapping the offset should always be zero

nikomatsakis (Oct 04 2019 at 21:09, on Zulip):

@Alexander Regueiro so I changed to inbounds_gep to remove the extra 0 and I am bootstrapping; I pushed a commit for now to your branch, but the code is almost certainly wrong for offset != 0

Alexander Regueiro (Oct 04 2019 at 21:09, on Zulip):

@nikomatsakis offset is measured in... not sure? eddyb just told me it was the right type layout

Alexander Regueiro (Oct 04 2019 at 21:10, on Zulip):

yeah, it segfaults fo offset != 0

nikomatsakis (Oct 04 2019 at 21:10, on Zulip):

nikomatsakis offset is measured in... not sure? eddyb just told me it was the right type layout

I'll take a look later, but I suspect it's words

Alexander Regueiro (Oct 04 2019 at 21:10, on Zulip):

anyway, thanks for explaining

Alexander Regueiro (Oct 04 2019 at 21:10, on Zulip):

yeah

Alexander Regueiro (Oct 04 2019 at 21:10, on Zulip):

probably

Alexander Regueiro (Oct 04 2019 at 21:10, on Zulip):

one sec, I can look now

nikomatsakis (Oct 04 2019 at 21:12, on Zulip):

I'm not sure why the type is [i64 x 3]* and not just i64* or somethign

Alexander Regueiro (Oct 04 2019 at 21:12, on Zulip):

@nikomatsakis yep, words

nikomatsakis (Oct 04 2019 at 21:12, on Zulip):

it seems like i64* is what we really want

Alexander Regueiro (Oct 04 2019 at 21:12, on Zulip):

const_usize is used to generate some values

nikomatsakis (Oct 04 2019 at 21:12, on Zulip):

@eddyb would probably have informed opinions here

Alexander Regueiro (Oct 04 2019 at 21:17, on Zulip):

@nikomatsakis grep for pub fn get_vtable if you want to see how the vtable is constructed now

Alexander Regueiro (Oct 04 2019 at 22:46, on Zulip):

@nikomatsakis finding it hard to discern why the type of the vtable in LLVM is [i64 x 3]*... I think that's what's causing the segfault

nikomatsakis (Oct 07 2019 at 15:45, on Zulip):

@Alexander Regueiro did you push any updates?

Alexander Regueiro (Oct 07 2019 at 15:49, on Zulip):

@nikomatsakis pushed (last commit is experimentation).

Alexander Regueiro (Oct 07 2019 at 15:50, on Zulip):

basically, I think struct_gep somehow feels right still, given the vtable is a struct, and in practical terms it seems to get more working. the LLVM vtable type still seems wrong though, which is odd.

Alexander Regueiro (Oct 07 2019 at 15:50, on Zulip):

and I know struct_gep apparently passes one too many args, but maybe there's another cause of that? it's just all very weird to me

Alexander Regueiro (Oct 07 2019 at 15:50, on Zulip):

anyway, I still get the segfault here

Alexander Regueiro (Oct 07 2019 at 15:51, on Zulip):

segfaults on the last println of this

nikomatsakis (Oct 07 2019 at 17:45, on Zulip):

basically, I think struct_gep somehow feels right still, given the vtable is a struct, and in practical terms it seems to get more working. the LLVM vtable type still seems wrong though, which is odd.

struct-gep is only right if we have an accurate type for the vtable

nikomatsakis (Oct 07 2019 at 17:45, on Zulip):

hmm with my changes I am getting these errors, @Alexander Regueiro, which seem familiar:

error[E0283]: type annotations needed: cannot resolve `dyn emitter::Emitter + rustc_data_structures::sync::Send: rustc_data_structures::sync::Send`
   --> src/librustc_errors/lib.rs:427:13
    |
427 |             emitter,
    |             ^^^^^^^
    |
    = note: required for the cast to the object type `dyn emitter::Emitter + rustc_data_structures::sync::Send`

error[E0283]: type annotations needed: cannot resolve `dyn emitter::Emitter + rustc_data_structures::sync::Send: rustc_data_structures::sync::Send`
   --> src/librustc_errors/lib.rs:446:17
    |
446 |                 emitter,
    |                 ^^^^^^^
    |
    = note: required for the cast to the object type `dyn emitter::Emitter + rustc_data_structures::sync::Send`

error[E0283]: type annotations needed: cannot resolve `dyn SourceMapper + rustc_data_structures::sync::Send + rustc_data_structures::sync::Sync: rustc_data_structures::sync::Sync`
    --> src/librustc_errors/emitter.rs:1480:55
     |
1480 |             let suggestions = suggestion.splice_lines(&**sm);
     |                                                       ^^^^^
     |
     = note: required for the cast to the object type `dyn SourceMapper + rustc_data_structures::sync::Send + rustc_data_structures::sync::Sync`
nikomatsakis (Oct 07 2019 at 17:46, on Zulip):

I think we were going to add some sort of .. hack or something to work around these, right?

nikomatsakis (Oct 07 2019 at 17:46, on Zulip):

I guess building with parallel-compiler=true ought to be enough for now

Alexander Regueiro (Oct 07 2019 at 18:02, on Zulip):

we already added the hack, @nikomatsakis

Alexander Regueiro (Oct 07 2019 at 18:02, on Zulip):

at least, I added something

Alexander Regueiro (Oct 07 2019 at 18:03, on Zulip):

see the bit where I handle auto traits separately

Alexander Regueiro (Oct 07 2019 at 18:04, on Zulip):

@nikomatsakis I'm stil doing the hack to bootstrap by building stage 0 master than stage 1 of this branch on top of it

Alexander Regueiro (Oct 07 2019 at 18:05, on Zulip):

@nikomatsakis BTW was trying to get the vtable ptr to be the right think using const_ptrcast in my "experimentation" commit

Alexander Regueiro (Oct 07 2019 at 18:05, on Zulip):

doesn't seem to have worked

Alexander Regueiro (Oct 07 2019 at 18:05, on Zulip):

that is, it's not hurt the situation, but it's not made the second println! in my Gist work either

nikomatsakis (Oct 07 2019 at 19:47, on Zulip):

@Alexander Regueiro ok so now i'm seeing this

error: internal compiler error: src/librustc_codegen_ssa/base.rs:164: unsized_info: invalid unsizing std::marker::PhantomData<dyn rustc_errors::SourceMapper + std::marker::Send + std::marker::Sync> -> std::marker::PhantomData<dyn rustc_errors::SourceMapper + std::marker\
::Send + std::marker::Sync>

I think you pushed something realted to that?

nikomatsakis (Oct 07 2019 at 19:47, on Zulip):

(I'm not yet using the latest tip of your branch)

nikomatsakis (Oct 07 2019 at 19:47, on Zulip):

ok but I see it has a workaround for that

Alexander Regueiro (Oct 07 2019 at 20:58, on Zulip):

Yeah exactly

Alexander Regueiro (Oct 07 2019 at 20:59, on Zulip):

Latest commit fixes that (maybe?)

Alexander Regueiro (Oct 07 2019 at 21:00, on Zulip):

But still issue with Arc

nikomatsakis (Oct 07 2019 at 21:15, on Zulip):

So I'm trying to summon @eddyb

nikomatsakis (Oct 07 2019 at 21:19, on Zulip):

in any case I think the right fix here -- at least for short-term -- is to cast the vtable to a u64* and do an in-bounds gep

nikomatsakis (Oct 07 2019 at 21:19, on Zulip):

and then cast back

nikomatsakis (Oct 07 2019 at 21:19, on Zulip):

it doesn't feel the most elegant and it seems like our "typing" of vtables in general could use to be updated

nikomatsakis (Oct 07 2019 at 21:27, on Zulip):

ultimately the cleaner way would probably be to have an accurate Layout for the vtables, so we can cast to that and do a struct gep to the appropriate field

Alexander Regueiro (Oct 07 2019 at 22:13, on Zulip):

@nikomatsakis makes sense.

Alexander Regueiro (Oct 07 2019 at 22:13, on Zulip):

@nikomatsakis could try that for now though... and leave a note. should I?

nikomatsakis (Oct 07 2019 at 22:18, on Zulip):

in any case I think the right fix here -- at least for short-term -- is to cast the vtable to a u64* and do an in-bounds gep

I think you should try this for now

Alexander Regueiro (Oct 07 2019 at 22:33, on Zulip):

@nikomatsakis will do, ta

Alexander Regueiro (Oct 07 2019 at 23:10, on Zulip):

@nikomatsakis we want u64 and not usize?

Alexander Regueiro (Oct 07 2019 at 23:13, on Zulip):

i.e.

Alexander Regueiro (Oct 07 2019 at 23:13, on Zulip):

let vtable_layout = bx.layout_of(tcx.mk_mut_ptr(tcx.mk_ty(ty::TyKind::Uint(UintTy::Usize))));

Alexander Regueiro (Oct 07 2019 at 23:27, on Zulip):

@nikomatsakis curious error now: https://gist.github.com/54483ab2c86451c0ba36d006f2a13c64

Alexander Regueiro (Oct 07 2019 at 23:39, on Zulip):

hrmm, tried it with type_ptr_to and ty_usize but bx and bx.cx() don't implement the necessary trait

Alexander Regueiro (Oct 08 2019 at 00:22, on Zulip):

@nikomatsakis pushed a bit more experimentation, but now I'm skeptical that src/librustc_codegen_ssa/base.rs:283 is the right approach / sufficient.

Alexander Regueiro (Oct 08 2019 at 00:23, on Zulip):

because it leads to unsized_info having to handle not only PhantomData, but also other types like NonNull... that doesn't feel right. even if there's a finite number of special cases, surely this is wrong?

nikomatsakis (Oct 08 2019 at 15:29, on Zulip):

the problem is assertion failures?

nikomatsakis (Oct 08 2019 at 15:29, on Zulip):

I'll try to take a look

Alexander Regueiro (Oct 08 2019 at 16:01, on Zulip):

@nikomatsakis segfault with the same Gist as before. didn't try bootstrapping, so no assertion failures.

Alexander Regueiro (Oct 08 2019 at 16:01, on Zulip):

sorry for keeping on bugging you. this is a pretty gnarly issue, I hope you agree!

Alexander Regueiro (Oct 09 2019 at 14:49, on Zulip):

@nikomatsakis but yeah, if you do something like this, you get an invalid types error in unsized_info (because it's not equipped to handle NonNull)...

    let bar: NonNull<dyn Bar> = NonNull::new(&mut 42 as *mut _).unwrap();
    println!("Arc-bar: {}", unsafe { bar.as_ref() });
    let foo: NonNull<dyn Foo> = bar;
    println!("Arc-foo: {}", unsafe { foo.as_ref() });
nikomatsakis (Oct 09 2019 at 21:25, on Zulip):

gnarly indeed! ping away, but I may not get time to investigate until friday

Alexander Regueiro (Oct 09 2019 at 22:02, on Zulip):

@nikomatsakis cheers. this is all the investigation I've managed to do so far (not sure I can do much more of the useful variety given my current level of understanding), but Friday is fine with me.

Alexander Regueiro (Oct 09 2019 at 22:02, on Zulip):

whenever you do get to it, just ping me likewise. ta.

Alexander Regueiro (Oct 10 2019 at 15:25, on Zulip):

@nikomatsakis hey, to be clear, when you say we can schedule a time that's not the normal slot, I presume we can do it much sooner than 4 weeks in advanced as some are suggesting? i.e., next week?

Alexander Regueiro (Oct 11 2019 at 14:40, on Zulip):

@nikomatsakis let me know if you get around to this today :-)

nikomatsakis (Oct 11 2019 at 17:15, on Zulip):

@Alexander Regueiro I'll take a look now

Alexander Regueiro (Oct 11 2019 at 17:20, on Zulip):

thanks @nikomatsakis

Alexander Regueiro (Oct 11 2019 at 17:21, on Zulip):

BTW

Alexander Regueiro (Oct 11 2019 at 17:21, on Zulip):

I have to be honest, this whole design process thing sounds like a lot of painful bureaucracy, and introduces many more delays.

Alexander Regueiro (Oct 11 2019 at 17:21, on Zulip):

I'm not sure I really want to go through it, considering how much effort I've put into work on my REPL anyway, and the delays so far...

Alexander Regueiro (Oct 11 2019 at 17:23, on Zulip):

it's just not very encouraging for me, when the time and it's continuing to demand from me is so great.

Alexander Regueiro (Oct 11 2019 at 17:23, on Zulip):

sorry... just had to clear that up.

nikomatsakis (Oct 11 2019 at 17:23, on Zulip):

I appreciate that it sounds like bureaucracy. On the other hand, I think it's the reality of contributing to a major project. Tech debt is a big problem for us right now that slows everything down. A big part of getting out from under that is trying to (a) do design more actively, (b) be a bit careful about what we land and (c) help to spread understanding by talking over and documenting our intentions.

nikomatsakis (Oct 11 2019 at 17:24, on Zulip):

One of the biggest problems I think we have now is that we've historically had no great procs

nikomatsakis (Oct 11 2019 at 17:24, on Zulip):

which means that big PRs can easily just get stuck

Alexander Regueiro (Oct 11 2019 at 17:24, on Zulip):

sure

Alexander Regueiro (Oct 11 2019 at 17:24, on Zulip):

but

nikomatsakis (Oct 11 2019 at 17:24, on Zulip):

i.e., I think that if we'd had this process working smoothly from the beginning, you would have been doing it much earlier on, and thing sowuld be better.

nikomatsakis (Oct 11 2019 at 17:24, on Zulip):

Unfortunately, it has to start somewhere.

Alexander Regueiro (Oct 11 2019 at 17:25, on Zulip):

the fact I have to wait 2 weeks even to get the proposal considered, THEN a meeting booked in, THEN the meeting carried out, THEN a decision made, THEN probably more changes to the PR, THEN finally another review and acceptance

Alexander Regueiro (Oct 11 2019 at 17:25, on Zulip):

I just don't have the willpower or desire to go through that right now, I'm afraid... and not sure when I will.

Alexander Regueiro (Oct 11 2019 at 17:26, on Zulip):

I could understand if it were done fairly quickly, but it will be a month or more before a decision is made on entrance of REPL parts into the compiler, I think

Alexander Regueiro (Oct 11 2019 at 17:26, on Zulip):

so a fork sounds like the best way to go now

Alexander Regueiro (Oct 11 2019 at 17:36, on Zulip):

anyway, re. trait upcasting

Alexander Regueiro (Oct 11 2019 at 17:36, on Zulip):

thanks for taking a look. I have to be off now, but will be back later. feel free to leave notes here or on GH.

Alexander Regueiro (Oct 11 2019 at 21:40, on Zulip):

@nikomatsakis ^

nikomatsakis (Oct 11 2019 at 21:46, on Zulip):

@Alexander Regueiro I'm looking :)

nikomatsakis (Oct 11 2019 at 21:47, on Zulip):

I got distracted because my sister is here for a visit

nikomatsakis (Oct 11 2019 at 21:47, on Zulip):

At the moment I'm trying to reproduce the error in gdb so I can see the stack trace

nikomatsakis (Oct 11 2019 at 21:47, on Zulip):

this is kind of driving me crazy

nikomatsakis (Oct 11 2019 at 21:47, on Zulip):

but I think I finally found the thing to do

nikomatsakis (Oct 11 2019 at 21:47, on Zulip):

lol though it's not that useful

Alexander Regueiro (Oct 11 2019 at 22:15, on Zulip):

heh

Alexander Regueiro (Oct 11 2019 at 22:15, on Zulip):

@nikomatsakis no worries.. :-)

Alexander Regueiro (Oct 11 2019 at 22:15, on Zulip):

what's the "thing to do then"?

Alexander Regueiro (Oct 11 2019 at 22:27, on Zulip):

@nikomatsakis as for for the PR and REPL stuff... I thought about it again, and I'm struggling to see how this can't be construed as "experimentation under master", which shouldn't really need an RFC or design meeting or anything like that. You know I'd be happy with a compromise like a single meeting in the next week or 10 days to flesh things out (plus a design doc from me, if you like), but yeah, something like that would be the most I could feasibly offer, sorry.

simulacrum (Oct 12 2019 at 02:58, on Zulip):

@Alexander Regueiro I think a design doc is a great start -- personally I think that we probably want to aim for design doc and then FCP, which may lead to a design meeting if there are objections (but with a select few -- maybe PR author and those listing concerns) which can be more adhoc

Alexander Regueiro (Oct 12 2019 at 03:38, on Zulip):

@simulacrum Oh, that sounds much more bearable. I could probably be happy with something like that. If @nikomatsakis thinks that’s a reasonable way to go too, then I should be happy to proceed.

Alexander Regueiro (Oct 12 2019 at 03:40, on Zulip):

@simulacrum Oh, that sounds much more bearable. I could probably be happy with something like that. If @nikomatsakis thinks that’s a reasonable way to go too, then let’s proceed with that.

nikomatsakis (Oct 12 2019 at 15:34, on Zulip):

@Alexander Regueiro I agree with @simulacrum that a design doc is a good start. If the changes are simple enough, maybe that's all we need.

nikomatsakis (Oct 12 2019 at 15:34, on Zulip):

Regarding your branch:

I'm not quite sure what the problem is, but I obesrved that this example fails all on its own

use std::any::Any;
use std::rc::Rc;

fn foo() -> Rc<dyn Any> {
    panic!()
}

fn bar() -> Rc<dyn Any> {
    foo()
}

fn main() {
    bar();
}
nikomatsakis (Oct 12 2019 at 16:12, on Zulip):

honestly, this seems like a bit of a pre-existing bug -- at minimum, it's very confusing what the "llvm type" of a vtable is

nikomatsakis (Oct 12 2019 at 16:13, on Zulip):

(ps, we're going to be needing a design doc for this work, too, I think; I'm happy to help some with that)

Alexander Regueiro (Oct 12 2019 at 17:38, on Zulip):

@nikomatsakis Okay, that sounds like we could have a compromise, hopefully... thanks for understanding. I'll create that design doc this weekend if possible. Should it be a PR to the compiler team repo?

Alexander Regueiro (Oct 12 2019 at 17:39, on Zulip):

@nikomatsakis I agree... the LLVM type of the vtable is a point of confusion. Because the rustc type is a struct, that may be having odd consequences? Yes, I think we mentioned a design doc when we originally discussed this effort months ago. I know someone else had volunteered to help with that then, but forget who. But if we could do it together, that may be workable anyway.

nikomatsakis (Oct 12 2019 at 23:04, on Zulip):

I'll create that design doc this weekend if possible. Should it be a PR to the compiler team repo?

@Alexander Regueiro just start with a hackmd or something

Alexander Regueiro (Oct 12 2019 at 23:08, on Zulip):

Will do, ta.

nikomatsakis (Oct 14 2019 at 09:38, on Zulip):

@Alexander Regueiro to expand a bit on the vtable type, it seems like some code expects [i64 x 3]* and some code expects i64* and I can't quite understand why that would be

nikomatsakis (Oct 14 2019 at 09:38, on Zulip):

tbh I don't fully know where either of those types are coming from anymore, so I guess I would need to dig a bit more into it

nikomatsakis (Oct 14 2019 at 09:39, on Zulip):

but e.g. this error that I see when bootstrapping

Invalid InsertValueInst operands!
  %8 = insertvalue { i64*, i64* } %7, [3 x i64]* bitcast ({ void (%"cstore::CrateMetadata"*)*, i64, i64, i64 (%"cstore::CrateMetadata"*)* }* @458 to [3 x i64]*), 1, !dbg !69145
in function _ZN14rustc_metadata11cstore_impl94_$LT$impl$u20$rustc..middle..cstore..CrateStore$u20$for$u20$rustc_metadata..cstore..CStore$GT$20crate_data_as_rc_any17he9ec2a22b2b45625E
nikomatsakis (Oct 14 2019 at 09:39, on Zulip):

seems to be arising out of this confusion

nikomatsakis (Oct 14 2019 at 09:39, on Zulip):

in this case, we expect a i64*

nikomatsakis (Oct 14 2019 at 09:40, on Zulip):

the annoying thing being that we HAVE a i64* and we bitcast it to [i64 x 3]*, but I know that if you remove that bitcast, it does much earlier :)

nikomatsakis (Oct 14 2019 at 09:41, on Zulip):

still, hmm, looking at the logs the old_info is always a [3 x i64]*

nikomatsakis (Oct 14 2019 at 09:41, on Zulip):

so maybe the problem is somewhere else

nikomatsakis (Oct 14 2019 at 09:49, on Zulip):

@Alexander Regueiro btw, do you build with

[llvm]
assertions = true

I am giving that a try now, I forgot this required an extra step

nikomatsakis (Oct 14 2019 at 09:50, on Zulip):

also, pushed a few commits

nikomatsakis (Oct 14 2019 at 10:43, on Zulip):

Ah, nice, enabling llvm.assertions causes some errors earlier

Alexander Regueiro (Oct 14 2019 at 16:00, on Zulip):

@nikomatsakis I don’t believe so (away from computer now). But fair point.

Alexander Regueiro (Oct 14 2019 at 16:01, on Zulip):

@nikomatsakis That’s annoying about the different LLVM types, but good spot.

Alexander Regueiro (Oct 14 2019 at 17:39, on Zulip):

@nikomatsakis oh, I had set assertions on... that probably explains why I got that error previously

Alexander Regueiro (Oct 14 2019 at 17:39, on Zulip):

the bootstrapping one

Alexander Regueiro (Oct 14 2019 at 17:39, on Zulip):

a couple of weeks ago

nikomatsakis (Oct 14 2019 at 21:31, on Zulip):

@Alexander Regueiro seems good

nikomatsakis (Oct 15 2019 at 00:46, on Zulip):

digging a bit more it seems the llvm version of std::ptr::NonNull<std::rc::RcBox<dyn std::any::Any>> is {i64*,i64*} but (e.g.) Box<dyn Any + Send> is { {}*, [3 x i64]* }

Alexander Regueiro (Oct 15 2019 at 03:17, on Zulip):

@nikomatsakis huh. I'm not sure what to make of that!

Alexander Regueiro (Oct 15 2019 at 03:17, on Zulip):

we need consistency though, either way (which is the better LLVM type?)

nikomatsakis (Oct 15 2019 at 18:16, on Zulip):

@Alexander Regueiro yeah I'm not quite sure

nikomatsakis (Oct 15 2019 at 18:17, on Zulip):

I pinged @eddyb and they wrote this:

years ago I wasted a few months trying to help with the LLVM pointee type removal
specifically so we could unpack newtypes and stuff. this best-effort approach was basically the fallback
the reason we even do pointee types anymore is LLVM still relies on them in places, frustratingly enough
it's the same reason removing them is so hard
anyway the trick is to 1. not expect anything 2. always cast to scalar_pair_element_type_of or w/e

which seems pretty close to what we're already doing, so I'm not quite sure what the problem is

Alexander Regueiro (Oct 15 2019 at 18:18, on Zulip):

yeah hmm

Alexander Regueiro (Oct 15 2019 at 18:18, on Zulip):

should we be bitcasting rather than pointer-casting?

Alexander Regueiro (Oct 15 2019 at 18:18, on Zulip):

not sure of the exact difference in fact, @nikomatsakis

Alexander Regueiro (Oct 15 2019 at 18:19, on Zulip):

I see both are done in existing code...

nikomatsakis (Oct 15 2019 at 18:22, on Zulip):

So let me drop down a few notes

nikomatsakis (Oct 15 2019 at 18:22, on Zulip):

What happens is that we get a Rc<dyn Foo> and we want to "upcast" it to Rc<dyn Foo>

nikomatsakis (Oct 15 2019 at 18:23, on Zulip):

we take the old info, do a "zero" adjustment to it, and return back a OperandPair or whatever; the types of the 'old info' in this bit of code seems to be [3 x i64]*

nikomatsakis (Oct 15 2019 at 18:23, on Zulip):

hmm the MIR is:

nikomatsakis (Oct 15 2019 at 18:24, on Zulip):
fn  bar() -> std::rc::Rc<dyn std::any::Any> {
    let mut _0: std::rc::Rc<dyn std::any::Any>; // return place in scope 0 at /home/nmatsakis/tmp/any_any.rs:8:13: 8:24
    let mut _1: std::rc::Rc<dyn std::any::Any>; // in scope 0 at /home/nmatsakis/tmp/any_any.rs:9:5: 9:10
    let mut _2: std::rc::Rc<dyn std::any::Any>; // in scope 0 at /home/nmatsakis/tmp/any_any.rs:9:5: 9:10

    bb0: {
        StorageLive(_1);                 // bb0[0]: scope 0 at /home/nmatsakis/tmp/any_any.rs:9:5: 9:10
        StorageLive(_2);                 // bb0[1]: scope 0 at /home/nmatsakis/tmp/any_any.rs:9:5: 9:10
        _2 = const foo() -> bb1;         // bb0[2]: scope 0 at /home/nmatsakis/tmp/any_any.rs:9:5: 9:10
                                         // ty::Const
                                         // + ty: fn() -> std::rc::Rc<(dyn std::any::Any + 'static)> {foo}
                                         // + val: Scalar(<ZST>)
                                         // mir::Constant
                                         // + span: /home/nmatsakis/tmp/any_any.rs:9:5: 9:8
                                         // + literal: Const { ty: fn() -> std::rc::Rc<(dyn std::any::Any + 'static)> {foo}, val: Scalar\
(<ZST>) }
    }

    bb1: {
        _1 = move _2 as std::rc::Rc<dyn std::any::Any> (Pointer(Unsize)); // bb1[0]: scope 0 at /home/nmatsakis/tmp/any_any.rs:9:5: 9:10
        StorageDead(_2);                 // bb1[1]: scope 0 at /home/nmatsakis/tmp/any_any.rs:9:9: 9:10
        _0 = move _1 as std::rc::Rc<dyn std::any::Any> (Pointer(Unsize)); // bb1[2]: scope 0 at /home/nmatsakis/tmp/any_any.rs:9:5: 9:10
        StorageDead(_1);                 // bb1[3]: scope 0 at /home/nmatsakis/tmp/any_any.rs:10:1: 10:2
        return;                          // bb1[4]: scope 0 at /home/nmatsakis/tmp/any_any.rs:10:2: 10:2
    }
}
nikomatsakis (Oct 15 2019 at 18:25, on Zulip):

so I think what happens is that when we create the "pair" for _0, we associated it with the two llvm values, and they have the [3 x i64]* type

nikomatsakis (Oct 15 2019 at 18:25, on Zulip):

then the return tries to use insertvalue to build the return value

nikomatsakis (Oct 15 2019 at 18:25, on Zulip):

but at that point the types disagree

nikomatsakis (Oct 15 2019 at 18:25, on Zulip):

some kind of adaptation is missing

nikomatsakis (Oct 15 2019 at 18:28, on Zulip):

Some pointers @eddyb to relevant bits of source:

Alexander Regueiro (Oct 15 2019 at 19:12, on Zulip):

Okay let’s wait and see what he suggests. Thanks @nikomatsakis.

nikomatsakis (Oct 16 2019 at 17:40, on Zulip):

@Alexander Regueiro I'm thinking about how we can get some of this work landed -- I hate "open forever" PRs -- maybe we should try to extract the "type system" work into something we can land and leave the codegen work as a fixme (it could just ICE for now when a non-zero offset is found, for example)

Alexander Regueiro (Oct 16 2019 at 17:54, on Zulip):

@nikomatsakis yeah, true (and I hate open-forever PRs too)

Alexander Regueiro (Oct 16 2019 at 18:24, on Zulip):

@nikomatsakis if you have any more ideas of what to try while we're waiting for eddyb to response, let me know... otherwise I can separate out the codegen stuff, add in a feature gate, and we can get the stuff working so far merged.

Alexander Regueiro (Oct 21 2019 at 16:06, on Zulip):

@nikomatsakis I’ll try to just get all the stuff but the codegen merged now okay? Maybe you can review?

nikomatsakis (Oct 21 2019 at 16:09, on Zulip):

@Alexander Regueiro sounds great! I've not had time to do anything further yet

Alexander Regueiro (Oct 21 2019 at 19:26, on Zulip):

@nikomatsakis no worries. want to talk about it briefly now though? where we go from here.

nikomatsakis (Oct 21 2019 at 21:04, on Zulip):

hey @Alexander Regueiro sorry I'm around-ish now

nikomatsakis (Oct 21 2019 at 21:05, on Zulip):

one question I was thinking about, did you ever solve that issue with type-checking when parallel compilation is not enable?

nikomatsakis (Oct 21 2019 at 21:05, on Zulip):

I remember we had discussed various work arounds

Alexander Regueiro (Oct 21 2019 at 21:05, on Zulip):

that's alright

Alexander Regueiro (Oct 21 2019 at 21:05, on Zulip):

err

Alexander Regueiro (Oct 21 2019 at 21:05, on Zulip):

I'd implemented one

Alexander Regueiro (Oct 21 2019 at 21:05, on Zulip):

and I thought it was working yes

Alexander Regueiro (Oct 21 2019 at 21:06, on Zulip):

can't remember when in the code it is though @nikomatsakis

nikomatsakis (Oct 21 2019 at 21:06, on Zulip):

hmm

Alexander Regueiro (Oct 21 2019 at 21:06, on Zulip):

I put it before the recent commits though

nikomatsakis (Oct 21 2019 at 21:06, on Zulip):

ok :)

Alexander Regueiro (Oct 21 2019 at 21:06, on Zulip):

let me look...

Alexander Regueiro (Oct 21 2019 at 21:07, on Zulip):

@nikomatsakis it's where the obligations are generated

Alexander Regueiro (Oct 21 2019 at 21:07, on Zulip):

I forget the file for that, sorry

nikomatsakis (Oct 21 2019 at 21:09, on Zulip):

@Alexander Regueiro it's ok,

nikomatsakis (Oct 21 2019 at 21:09, on Zulip):

I'll find it soon enough :)

nikomatsakis (Oct 21 2019 at 21:10, on Zulip):

Did you look at what it means to land the code without codegen changes?

nikomatsakis (Oct 21 2019 at 21:10, on Zulip):

Presumably we have to introduce some feature gates

nikomatsakis (Oct 21 2019 at 21:11, on Zulip):

I think we are also going to want to try and document the vtable formats

nikomatsakis (Oct 21 2019 at 21:11, on Zulip):

I guess I need to spend a bit of time reviewing the PR again

Alexander Regueiro (Oct 21 2019 at 21:26, on Zulip):

yep

Alexander Regueiro (Oct 21 2019 at 21:26, on Zulip):

it's a real pain this eh

Alexander Regueiro (Oct 21 2019 at 21:26, on Zulip):

@nikomatsakis feature gate with a warning message would be ideal

Alexander Regueiro (Oct 21 2019 at 21:26, on Zulip):

that's no problem for me to do though

Alexander Regueiro (Oct 21 2019 at 21:53, on Zulip):

@nikomatsakis vtable formats are pretty well-documented now I think. anyway I'll try to do this tonight or tomorrow... will leave looking at the codegen to you though. happy to chat about it again, but not sure I can offer much until an expert like you has a closer look!

nikomatsakis (Oct 21 2019 at 21:56, on Zulip):

sounds good

Alexander Regueiro (Oct 22 2019 at 16:46, on Zulip):

@eddyb let us know if you have any thoughts on the above BTW :-)

nikomatsakis (Nov 04 2019 at 20:01, on Zulip):

@Alexander Regueiro so what's the latest here?

Alexander Regueiro (Nov 04 2019 at 20:02, on Zulip):

@nikomatsakis latest is very good. I got a few helpful tips from @eddyb to complete my implementation (it was working before, but a bit hacky). I basically need a proper test suite now. at least a minimal one, which can be expanded soon.

Alexander Regueiro (Nov 04 2019 at 20:02, on Zulip):

a review either now or once we have a few more tests would be super.

Alexander Regueiro (Nov 04 2019 at 20:03, on Zulip):

also trying to implement good diagnostics to suggest turning on the feature when trait upcasting is appropriate

Alexander Regueiro (Nov 04 2019 at 20:03, on Zulip):

(i.e. only when a coercion is possible)

Alexander Regueiro (Nov 04 2019 at 20:07, on Zulip):

Re. the second point: is there any way to get an appropriate FnCtxt from an InferCtxt in rustc::infer::error_reporting (specifically the note_type_err method)?

Alexander Regueiro (Nov 04 2019 at 20:12, on Zulip):

@nikomatsakis ^

nikomatsakis (Nov 04 2019 at 20:14, on Zulip):

a review either now or once we have a few more tests would be super.

ok!

nikomatsakis (Nov 04 2019 at 20:14, on Zulip):

Re. the second point: is there any way to get an appropriate FnCtxt from an InferCtxt in rustc::infer::error_reporting (specifically the note_type_err method)?

not sure what .. ah ok I see

nikomatsakis (Nov 04 2019 at 20:14, on Zulip):

No, there is no way

nikomatsakis (Nov 04 2019 at 20:14, on Zulip):

let me look what that method does

nikomatsakis (Nov 04 2019 at 20:16, on Zulip):

@Alexander Regueiro oh, are you saying from within that method, you wish to get teh FnCtxt?

Alexander Regueiro (Nov 04 2019 at 20:55, on Zulip):

@nikomatsakis sorry I didn't get a notification!

Alexander Regueiro (Nov 04 2019 at 20:55, on Zulip):

hrmm

Alexander Regueiro (Nov 04 2019 at 20:56, on Zulip):

@nikomatsakis right, that's what I want

Alexander Regueiro (Nov 04 2019 at 20:56, on Zulip):

basically I want to see if type a is coercible to type b from within that method doing diagnostics

Alexander Regueiro (Nov 04 2019 at 20:56, on Zulip):

and it needs the appropriate FnCtxt I guess

nikomatsakis (Nov 04 2019 at 21:12, on Zulip):

Hmm. I think we should try to "not want" this

nikomatsakis (Nov 04 2019 at 21:12, on Zulip):

Can you say more about why you want it?

Alexander Regueiro (Nov 04 2019 at 21:36, on Zulip):

@nikomatsakis sure. basically when the diagnostic is emitted for "found type a... expected type b", I want to suggest "enable the trait_upcasting feature", but only when it's actually relevant

Alexander Regueiro (Nov 04 2019 at 21:37, on Zulip):

@nikomatsakis I could use a more simplistic algorithm of just looking at the expected and found traits (I'm given their DefIds already) and seeing if the expected trait is amongst the supertraits of the found traits... but that wouldn't account for type params and whatnot. good enough, still?

nikomatsakis (Nov 04 2019 at 21:49, on Zulip):

seems good enough to me, @Alexander Regueiro

Alexander Regueiro (Nov 04 2019 at 22:12, on Zulip):

@nikomatsakis cool.

Alexander Regueiro (Nov 04 2019 at 22:13, on Zulip):

@nikomatsakis as for tests... I'm going to write some tomorrow, but if you a) have ideas of ones to include, let me know, b) want to review soon anyway, both those things would be great. :-)

Alexander Regueiro (Nov 08 2019 at 16:12, on Zulip):

@nikomatsakis writing tests lately, and encountered one problem (upcasting to an auto trait object):

67 |     let _: &dyn Send = foo;
   |                        ^^^ expected trait `std::marker::Send`, found trait `Foo`
   |
   = note: expected type `&dyn std::marker::Send`
              found type `&dyn Foo`

when you do have time, which I appreciate may not be till next week, let me know your thoughts. the latest code (pretty much) is already pushed to Github.

nikomatsakis (Nov 08 2019 at 16:14, on Zulip):

@Alexander Regueiro awesome

nikomatsakis (Nov 08 2019 at 16:14, on Zulip):

I'm so excited for this to land

nikomatsakis (Nov 08 2019 at 16:14, on Zulip):

In this case, do you have trait Foo: Send?

nikomatsakis (Nov 08 2019 at 16:15, on Zulip):

I'm guessing this is an artifact of the semi-hokey way we handle auto traits

Alexander Regueiro (Nov 08 2019 at 16:18, on Zulip):

@nikomatsakis exactly

Alexander Regueiro (Nov 08 2019 at 16:18, on Zulip):

yeah

Alexander Regueiro (Nov 08 2019 at 16:18, on Zulip):

probably that

Alexander Regueiro (Nov 08 2019 at 17:04, on Zulip):

@nikomatsakis anyway if you think of a pointer as to what I should investigate for this, let me know. otherwise we'll chat next week.

Alexander Regueiro (Nov 11 2019 at 17:21, on Zulip):

@nikomatsakis Hey Niko. If you have a minute today, would you mind explaining this bit of code to me? (in select.rs)

Alexander Regueiro (Nov 11 2019 at 17:21, on Zulip):
                // See `assemble_candidates_for_unsizing` for more info.
                let existential_predicates = data_a.map_bound(|data_a| {
                    let iter =
                        data_a.principal().map(|x| ty::ExistentialPredicate::Trait(x))
                            .into_iter().chain(
                                data_a
                                    .projection_bounds()
                                    .map(|x| ty::ExistentialPredicate::Projection(x)),
                            )
                            .chain(
                                data_b
                                    .auto_traits()
                                    .map(ty::ExistentialPredicate::AutoTrait),
                            );
                    tcx.mk_existential_predicates(iter)
                });
                let source_ty = tcx.mk_dynamic(existential_predicates, region_b);
Alexander Regueiro (Nov 11 2019 at 17:22, on Zulip):

specifically, why do we use b's auto-traits?

nikomatsakis (Nov 11 2019 at 18:09, on Zulip):

@Alexander Regueiro looking now

nikomatsakis (Nov 11 2019 at 18:12, on Zulip):

I think the answer to that question is because b is the target type, and we expect the autotraits to be a subset of a?

nikomatsakis (Nov 11 2019 at 18:13, on Zulip):

let me find the code in question

nikomatsakis (Nov 11 2019 at 18:16, on Zulip):

(yes, confirmed)

Alexander Regueiro (Nov 11 2019 at 19:01, on Zulip):

@nikomatsakis right, makes sense, but that no longer applies now... which is why I think this is a problem

Alexander Regueiro (Nov 11 2019 at 19:17, on Zulip):

@nikomatsakis maybe let's address this towards the end of the meeting :-)

Alexander Regueiro (Nov 11 2019 at 19:17, on Zulip):

actually, I'll probably be away still

Alexander Regueiro (Nov 11 2019 at 19:17, on Zulip):

but feel free to leave ideas

nikomatsakis (Nov 11 2019 at 19:40, on Zulip):

right, makes sense, but that no longer applies now... which is why I think this is a problem

@Alexander Regueiro that seems true :)

nikomatsakis (Nov 11 2019 at 20:17, on Zulip):

@Alexander Regueiro so that code is used to create source_ty

nikomatsakis (Nov 11 2019 at 20:17, on Zulip):

I think that code should move into the else branch

nikomatsakis (Nov 11 2019 at 20:17, on Zulip):

i.e., you only need it if you have don't have trait-upcasting enabled

nikomatsakis (Nov 11 2019 at 20:17, on Zulip):

the trait-upcasting branch can use the variable source

nikomatsakis (Nov 11 2019 at 20:18, on Zulip):

probably we should rename source_ty, too

nikomatsakis (Nov 11 2019 at 20:18, on Zulip):

the creation of source_ty seems like it basically just a hack to relate (a) the lifetimes and (b) the other type arguments that may appear in the principle bound

Alexander Regueiro (Nov 11 2019 at 23:21, on Zulip):

@nikomatsakis heh, that's exactly what I did just after I messaged you. sorry. but good to have confirmation!

Alexander Regueiro (Nov 11 2019 at 23:21, on Zulip):

and yes, I renamed source_ty to just source, shadowing the existing source parameter

Alexander Regueiro (Nov 11 2019 at 23:21, on Zulip):

which I think makes sense.

nikomatsakis (Nov 11 2019 at 23:37, on Zulip):

@Alexander Regueiro I would probably rename it to something like source_eq_ty or something, with a comment -- the current code is a bit... subtle

nikomatsakis (Nov 11 2019 at 23:37, on Zulip):

or source_with_target_auto_traits :)

nikomatsakis (Nov 11 2019 at 23:37, on Zulip):

I just finished reading the PR, it seems great so far

Alexander Regueiro (Nov 11 2019 at 23:38, on Zulip):

hah, that's verbose alright

Alexander Regueiro (Nov 11 2019 at 23:38, on Zulip):

@nikomatsakis cool. I just made that fix and reran tests BTW. upcasting with auto-traits working nicely now!

nikomatsakis (Nov 11 2019 at 23:38, on Zulip):

:tada: =)

nikomatsakis (Nov 11 2019 at 23:38, on Zulip):

hah, that's verbose alright

heh, I'm a fan of verbose names for wacky edge cases... but in any case I think shadowing is misleading since these are not the same type

Alexander Regueiro (Nov 11 2019 at 23:39, on Zulip):

yeah you're right

Alexander Regueiro (Nov 11 2019 at 23:39, on Zulip):

I'll use that then!

Alexander Regueiro (Nov 11 2019 at 23:39, on Zulip):

@nikomatsakis what I'll do is try to add some more tests, then you can give it a final review whenever you can this week, maybe, and we can r+? :-)

nikomatsakis (Nov 11 2019 at 23:39, on Zulip):

@Alexander Regueiro yep, I think so :)

nikomatsakis (Nov 11 2019 at 23:40, on Zulip):

I would like to think about what rustc-guide additions are needed here, but I think it can come after the PR

Alexander Regueiro (Nov 11 2019 at 23:40, on Zulip):

great. I'll try to have that done by tomorrow (just in case you have the time tomorrow)

Alexander Regueiro (Nov 11 2019 at 23:40, on Zulip):

yeah

Alexander Regueiro (Nov 11 2019 at 23:40, on Zulip):

indeed

Alexander Regueiro (Nov 11 2019 at 23:40, on Zulip):

(I added a brief chapter to the unstable book in this PR of course.)

Alexander Regueiro (Nov 11 2019 at 23:52, on Zulip):

anyway, thanks for the review. I'll try to address those comments tonight too, else tomorrow.

Alexander Regueiro (Nov 13 2019 at 01:29, on Zulip):

@nikomatsakis so, tests all written now, but two outstanding issues:

  1. the diamond inheritance case doesn't work (segfaults), and I'm not sure why. see line 66 of the diamond.rs test.
  2. we're currently generating D S A metadata for auto traits, which seems wasteful. the naive approach of not including auto traits in the vtable construction (meth.rs) and in offset calculation (select.rs) does not work (it gives this ICE)
nikomatsakis (Nov 13 2019 at 14:30, on Zulip):

@Alexander Regueiro ok! great

nikomatsakis (Nov 13 2019 at 14:30, on Zulip):

let me start a local build

nikomatsakis (Nov 13 2019 at 14:30, on Zulip):

and investigate

Alexander Regueiro (Nov 13 2019 at 14:49, on Zulip):

@nikomatsakis cool. thanks.

nikomatsakis (Nov 13 2019 at 15:05, on Zulip):

ok I can reproduce the problems at least :)

Alexander Regueiro (Nov 13 2019 at 18:35, on Zulip):

heh okay, good. let me know if you have ideas @nikomatsakis

nikomatsakis (Nov 14 2019 at 14:39, on Zulip):

@Alexander Regueiro so i'm looking at the diamond case

nikomatsakis (Nov 14 2019 at 14:39, on Zulip):

I simplified the test file a lot

nikomatsakis (Nov 14 2019 at 14:40, on Zulip):

my simplified test

nikomatsakis (Nov 14 2019 at 14:40, on Zulip):

this still crashes

nikomatsakis (Nov 14 2019 at 14:40, on Zulip):

the LLVM output is .. basically illegible :)

nikomatsakis (Nov 14 2019 at 14:40, on Zulip):

I guess the next step is either to try and format the LLVM IR and make sense of it or else to insert debug statements to try and log what's going on

nikomatsakis (Nov 14 2019 at 14:40, on Zulip):

I mean it seems pretty obvious that something is off, but I can't quite figure out what is being generated yet to tell what

nikomatsakis (Nov 14 2019 at 14:41, on Zulip):

I would guess that the methods from Foo need to be duplicated across the table

nikomatsakis (Nov 14 2019 at 14:41, on Zulip):

and that they are not

nikomatsakis (Nov 14 2019 at 14:41, on Zulip):

probably because when we are enumerating the set of supertraits etc we are avoiding walking the same thing twice

nikomatsakis (Nov 14 2019 at 14:41, on Zulip):

but we need to if we are going to use this vtable generation strategy

Alexander Regueiro (Nov 14 2019 at 16:31, on Zulip):

@nikomatsakis yes I think you're spot on.

Alexander Regueiro (Nov 14 2019 at 16:53, on Zulip):

@nikomatsakis and thanks for investigating!

Alexander Regueiro (Nov 14 2019 at 16:59, on Zulip):

@nikomatsakis I wonder... what cases precisely I need to duplicate (some of) the supertraits in. probably just diamond inheritance?

nikomatsakis (Nov 16 2019 at 10:13, on Zulip):

@Alexander Regueiro I think all of them

nikomatsakis (Nov 16 2019 at 10:13, on Zulip):

that's the whole point with this style -- you have to make a tree, not a DAG

nikomatsakis (Nov 16 2019 at 10:29, on Zulip):

@Alexander Regueiro ps, my desktop where I had checkouts of your branch and everything mysteriously died a horrific death yesterday, so I can't play with your code right now

nikomatsakis (Nov 16 2019 at 10:29, on Zulip):

But I do think you have to de-duplicate everything

nikomatsakis (Nov 16 2019 at 10:29, on Zulip):

Put another way, the trait should have one complete copy of the vtables for each of its supertraits

nikomatsakis (Nov 16 2019 at 10:29, on Zulip):

if you have trait Foo: Bar + Bar, then you don't need two copies of Bar

nikomatsakis (Nov 16 2019 at 10:30, on Zulip):

but if you have trait Foo: Bar1 + Bar2 and trait Bar1: Baz / trait Bar2: Baz, you need a copy of Bar1 and Bar2, and they each need a copy of Baz

Alexander Regueiro (Nov 16 2019 at 21:11, on Zulip):

@nikomatsakis oh dear! well, thanks for trying to investigate anyway

Alexander Regueiro (Nov 16 2019 at 21:12, on Zulip):

@nikomatsakis can I can only deduplicate if the type args are the same though right?

Alexander Regueiro (Nov 16 2019 at 21:17, on Zulip):

The thing that perplexed me is: how come you can call supertrait methods in the current vtable setup, if I'm not doing it the right way?

Alexander Regueiro (Nov 16 2019 at 21:21, on Zulip):

^ hmm, presumably because we go up the type hierarchy as we offset into the vtable, right @nikomatsakis ?

Alexander Regueiro (Nov 16 2019 at 21:22, on Zulip):

so we don't want to duplicate everything then

Alexander Regueiro (Nov 17 2019 at 01:19, on Zulip):

@nikomatsakis so, I was thinking about this more today and looking at some debug output, and I believe we don't actually need multiple copies of the vtables for the same supertrait at all. the problem stems from the fact that in the diamond test case, the supertraits are enumerated in the order Baz, Bar2, Foo, Bar1. I think this is because it does a depth-first visit, but the real point is the order isn't suitable if you offset to Bar1's vtable and then try to access a method of Foo (which should be legal). see the problem? if we had the order Baz, Bar2, Bar1, Foo, all would be fine! (Bar1, Bar2 is equally good, of course.) now, since the trait hierarchy forms a DAG, we're in luck. we can just do a topological sort on the supertraits and the resulting overall vtable will be properly "offsetable" no matter what sequence of upcasts you do. I think we just need to be careful that we consider the different PolyTraitRefs (modulo lifetimes?) return by supertraits as different 'vertices' in the top-sort.

Alexander Regueiro (Nov 17 2019 at 01:23, on Zulip):

@nikomatsakis I mean, this has the issue that if we upcast Baz -> Bar2 we somehow have to know to skip over Bar1 in the above example when finding the necessary method offset in Foo... since Bar1 isn't a supertrait of Bar2` of course, the normal "supertrait counting" thing won't work, but maybe we can create a look-up table or something?

Alexander Regueiro (Nov 17 2019 at 01:24, on Zulip):

I believe what you were suggesting (while it obviously works) requires a change to the "supertrait counting" algorithm for finding method offsets, anyway

Alexander Regueiro (Nov 17 2019 at 01:24, on Zulip):

well, let me know your thoughts :-)

Last update: Nov 18 2019 at 01:55UTC