Stream: wg-traits

Topic: object upcasting


view this post on Zulip nikomatsakis (Apr 08 2019 at 17:09):

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 =)

view this post on Zulip Alexander Regueiro (Apr 08 2019 at 17:10):

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

view this post on Zulip nikomatsakis (Apr 08 2019 at 17:10):

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

view this post on Zulip nikomatsakis (Apr 08 2019 at 17:10):

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

view this post on Zulip nikomatsakis (Apr 08 2019 at 17:10):

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

view this post on Zulip nikomatsakis (Apr 08 2019 at 17:11):

maybe, maybe not

view this post on Zulip Alexander Regueiro (Apr 08 2019 at 17:11):

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

view this post on Zulip Alexander Regueiro (Apr 08 2019 at 17:11):

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

view this post on Zulip Alexander Regueiro (Apr 08 2019 at 17:11):

shall we start a HackMD doc at least?

view this post on Zulip Alexander Regueiro (Apr 08 2019 at 17:11):

or similar

view this post on Zulip Alexander Regueiro (Apr 08 2019 at 17:15):

@nikomatsakis ^

view this post on Zulip nikomatsakis (Apr 08 2019 at 17:17):

shall we start a HackMD doc at least?

start a doc for upcasting?

view this post on Zulip Alexander Regueiro (Apr 08 2019 at 17:17):

@nikomatsakis yeah

view this post on Zulip nikomatsakis (Apr 08 2019 at 17:17):

seems reasonable

view this post on Zulip nikomatsakis (Apr 08 2019 at 17:17):

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

view this post on Zulip Alexander Regueiro (Apr 08 2019 at 17:17):

we can summarise the current state

view this post on Zulip Alexander Regueiro (Apr 08 2019 at 17:18):

then part B can be an action plan

view this post on Zulip nikomatsakis (Apr 08 2019 at 17:18):

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

view this post on Zulip Alexander Regueiro (Apr 08 2019 at 17:18):

sure

view this post on Zulip nikomatsakis (Apr 08 2019 at 17:18):

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

view this post on Zulip nikomatsakis (Apr 08 2019 at 17:18):

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

view this post on Zulip nikomatsakis (Apr 08 2019 at 17:18):

maybe we can get @eddyb involved

view this post on Zulip Alexander Regueiro (Apr 08 2019 at 17:19):

yeah that would be good, if he's free

view this post on Zulip Alexander Regueiro (Apr 08 2019 at 17:19):

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

view this post on Zulip Alexander Regueiro (Apr 08 2019 at 17:20):

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

view this post on Zulip nikomatsakis (Apr 08 2019 at 17:49):

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

view this post on Zulip Alexander Regueiro (Apr 08 2019 at 22:16):

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

view this post on Zulip Alexander Regueiro (Apr 08 2019 at 22:16):

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

view this post on Zulip GuillaumeGomez (Apr 09 2019 at 06:47):

Here I am!

view this post on Zulip Alexander Regueiro (Apr 09 2019 at 14:19):

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

view this post on Zulip Alexander Regueiro (Apr 09 2019 at 14:19):

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

view this post on Zulip nikomatsakis (Apr 09 2019 at 21:11):

@Alexander Regueiro ok I filled it out

view this post on Zulip Alexander Regueiro (Apr 09 2019 at 21:12):

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

view this post on Zulip Alexander Regueiro (Apr 09 2019 at 21:12):

unless someone else replies soon

view this post on Zulip Alexander Regueiro (Apr 09 2019 at 21:12):

sorry if time zone was confusing

view this post on Zulip nikomatsakis (Apr 09 2019 at 21:55):

Created a calendar event, which includes a zoom link

view this post on Zulip nikomatsakis (Apr 09 2019 at 21:55):

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

view this post on Zulip Alexander Regueiro (Apr 09 2019 at 21:57):

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.

view this post on Zulip Alexander Regueiro (Apr 09 2019 at 21:57):

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

view this post on Zulip Alexander Regueiro (Apr 09 2019 at 21:58):

Created a calendar event, which includes a zoom link

CC @Guillaume

view this post on Zulip centril (Apr 10 2019 at 00:22):

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

view this post on Zulip nikomatsakis (Apr 10 2019 at 09:18):

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.

view this post on Zulip centril (Apr 10 2019 at 14:30):

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

view this post on Zulip Alexander Regueiro (Apr 10 2019 at 15:11):

it's this week :-P

view this post on Zulip centril (Apr 10 2019 at 15:23):

@Alexander Regueiro yeah I looked 1 week into the future

view this post on Zulip Alexander Regueiro (Apr 10 2019 at 15:23):

heh okay

view this post on Zulip GuillaumeGomez (Apr 10 2019 at 18:08):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 15:56):

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.

view this post on Zulip nikomatsakis (Apr 12 2019 at 15:57):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 16:00):

Zulip is fine with me too, yeah

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 16:00):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 16:03):

at least that's what's in the calendar

view this post on Zulip nikomatsakis (Apr 12 2019 at 16:06):

the calendar says in 1 hr?

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 16:07):

hmm, not for me

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 16:07):

weird

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 16:07):

anyway, 1hr is fine

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 16:07):

no worries

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 16:07):

maybe iCal confused the timezones

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 16:18):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 16:18):

@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.

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 16:19):

this is the last remaining thing...

view this post on Zulip GuillaumeGomez (Apr 12 2019 at 16:39):

for me it's in 21 min

view this post on Zulip centril (Apr 12 2019 at 16:46):

@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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 16:50):

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.

view this post on Zulip nikomatsakis (Apr 12 2019 at 16:50):

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

I did not

view this post on Zulip nikomatsakis (Apr 12 2019 at 16:51):

Link?

view this post on Zulip centril (Apr 12 2019 at 16:51):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 16:55):

@centril thanks

view this post on Zulip GuillaumeGomez (Apr 12 2019 at 16:59):

Should we start?

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:01):

Yep, let's start

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:01):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:02):

First, to set the stage

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:02):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:02):

yep...

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:02):

Or perhaps what we're not talking about

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:02):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:02):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:02):

But we would permit upcasting to supertraits via a coercion

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:03):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:03):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:03):

or perhaps I should say when

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:03):

since I am assuming we will do so at some point

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:03):

but I think that's a more complex topic

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:03):

Does everybody agree with this so far? :)

view this post on Zulip centril (Apr 12 2019 at 17:04):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:04):

We may, I forget.

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:04):

Good to clarify.

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:04):

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

view this post on Zulip centril (Apr 12 2019 at 17:05):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:06):

this builds:

trait Foo { }

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

fn main() { }

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:06):

but the commented line does not

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:07):

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`

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:07):

@nikomatsakis Yep I agree.

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:07):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:07):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:07):

I see

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:07):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:07):

but let's find the actual code

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:08):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:08):

I could test now actually heh

view this post on Zulip centril (Apr 12 2019 at 17:09):

@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>

view this post on Zulip centril (Apr 12 2019 at 17:09):

IOW, dyn Sub <: dyn Super does not hold

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:09):

ah nope, same error in the build for that PR

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:10):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:11):

Yes, the coercion is here, and it says:

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:11):

 // 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).

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:11):

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

view this post on Zulip centril (Apr 12 2019 at 17:11):

Yeah that seems reasonable

view this post on Zulip centril (Apr 12 2019 at 17:12):

this should just fall out from the unsizing rules, centril

Also reasonable :slight_smile:

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:12):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:12):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:12):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:12):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:13):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:13):

include pointers to the vtables for its supertraits

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:13):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:13):

er, sorry, supertrait relations are structured in a tree

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:13):

I'd actually like to lift that restriction

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:14):

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

view this post on Zulip centril (Apr 12 2019 at 17:14):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:14):

not that I know of

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:14):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:14):

it's hard to imagine it being controversial

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:14):

but I think documenting the plan is good

view this post on Zulip centril (Apr 12 2019 at 17:15):

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

view this post on Zulip GuillaumeGomez (Apr 12 2019 at 17:15):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:15):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:15):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:15):

but I would expect us to move to FCP quickly

view this post on Zulip GuillaumeGomez (Apr 12 2019 at 17:15):

Fair enough!

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:15):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:16):

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

view this post on Zulip centril (Apr 12 2019 at 17:16):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:16):

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 :)

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:16):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:16):

include pointers to the vtables for its supertraits

there is another impl route

view this post on Zulip GuillaumeGomez (Apr 12 2019 at 17:16):

This is another fair point :)

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:17):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:17):

we could lay them out in such a way that

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:17):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:17):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:17):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:18):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:18):

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

view this post on Zulip centril (Apr 12 2019 at 17:18):

@nikomatsakis imo, we can also iterate on layout

view this post on Zulip centril (Apr 12 2019 at 17:18):

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

view this post on Zulip centril (Apr 12 2019 at 17:19):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:20):

I don't know why not

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:20):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:20):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:20):

sounds reasonable

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:21):

wg-subtyping? :-P

view this post on Zulip centril (Apr 12 2019 at 17:21):

wg-coercion ;)

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:21):

no :) wg-dyn-upcast :)

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:21):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:22):

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 =)

view this post on Zulip centril (Apr 12 2019 at 17:22):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:22):

nikomatsakis imo, we can also iterate on layout

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

view this post on Zulip eddyb (Apr 12 2019 at 17:23):

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

view this post on Zulip centril (Apr 12 2019 at 17:23):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:23):

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

view this post on Zulip centril (Apr 12 2019 at 17:23):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:23):

and hasn't been for ages

view this post on Zulip eddyb (Apr 12 2019 at 17:23):

my issue is I have to do several things already

view this post on Zulip eddyb (Apr 12 2019 at 17:24):

if not for that, sure

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:24):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:24):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:24):

but I agree it's not all that much work

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:24):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:24):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:24):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:24):

lemme grab it

view this post on Zulip centril (Apr 12 2019 at 17:24):

copypaste another place that does it already

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:24):

I am also happy to do so

view this post on Zulip eddyb (Apr 12 2019 at 17:25):

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

view this post on Zulip centril (Apr 12 2019 at 17:25):

@eddyb fiiine :P

view this post on Zulip centril (Apr 12 2019 at 17:26):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:26):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:26):

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

view this post on Zulip centril (Apr 12 2019 at 17:26):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:26):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:26):

regardles I think we want this upcast :)

view this post on Zulip eddyb (Apr 12 2019 at 17:26):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:26):

even though it's not >:P

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:26):

(going to be afk for a bit)

view this post on Zulip eddyb (Apr 12 2019 at 17:27):

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

view this post on Zulip centril (Apr 12 2019 at 17:27):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:27):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:28):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:28):

like, vtable layout is already done by the trait system

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:29):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:29):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:29):

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

view this post on Zulip centril (Apr 12 2019 at 17:31):

someone needs to write a bunch of tests as well

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:31):

what's this magical 3? :-P

view this post on Zulip centril (Apr 12 2019 at 17:31):

for the implementation

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:31):

@centril you know that's you ;-)

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:31):

mr. tests

view this post on Zulip centril (Apr 12 2019 at 17:31):

:sob:

view this post on Zulip centril (Apr 12 2019 at 17:32):

yeah maybe ;)

view this post on Zulip centril (Apr 12 2019 at 17:33):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:34):

3 is drop+size+align

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:35):

(going to be afk for a bit)

sorry, back

view this post on Zulip centril (Apr 12 2019 at 17:36):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:36):

3 is drop+size+align

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

view this post on Zulip centril (Apr 12 2019 at 17:36):

but there's no syntax to check here at least

view this post on Zulip centril (Apr 12 2019 at 17:37):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:37):

oh yeaaaah sooo

view this post on Zulip eddyb (Apr 12 2019 at 17:37):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:38):

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

feel free to make a subdirectory there

view this post on Zulip eddyb (Apr 12 2019 at 17:41):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:41):

or... something like that

view this post on Zulip eddyb (Apr 12 2019 at 17:42):

so this is for calling methods on trait objects

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:43):

interesting

view this post on Zulip eddyb (Apr 12 2019 at 17:43):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:43):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:43):

then I can have a go maybe

view this post on Zulip eddyb (Apr 12 2019 at 17:43):

I mean, this is it?

view this post on Zulip centril (Apr 12 2019 at 17:43):

(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?

view this post on Zulip eddyb (Apr 12 2019 at 17:43):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:44):

that's all my fold does

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:44):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:44):

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

what do you mean?

view this post on Zulip centril (Apr 12 2019 at 17:46):

@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?

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:47):

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.

view this post on Zulip eddyb (Apr 12 2019 at 17:47):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:47):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:48):

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.

view this post on Zulip eddyb (Apr 12 2019 at 17:48):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:49):

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)

view this post on Zulip eddyb (Apr 12 2019 at 17:49):

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)

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:49):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:49):

yeah ofc you need to duplicate them

view this post on Zulip eddyb (Apr 12 2019 at 17:49):

wait did everyone discuss something else?

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:49):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:49):

/me pretends to have been paying attention

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:49):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:49):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:49):

I think this is the one we can trivially do today

view this post on Zulip eddyb (Apr 12 2019 at 17:50):

and it's not much of a perf loss

view this post on Zulip eddyb (Apr 12 2019 at 17:50):

(if at all)

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:50):

neither has perf loss

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:50):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:50):

embedding pointers means virtual calls are more expensive

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:50):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:50):

but also pointers to the vtables of the supertraits

view this post on Zulip eddyb (Apr 12 2019 at 17:50):

oh that sounds silly though?

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:50):

why?

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:50):

it supports cycles, among other things :)

view this post on Zulip eddyb (Apr 12 2019 at 17:51):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:51):

you're not duplicating more

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:51):

I mean you have to duplicate 3 fields per supertrait

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:51):

I am duplicating 1

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:51):

er, adding 1

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:51):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:51):

and anyway come on, it's vtables

view this post on Zulip eddyb (Apr 12 2019 at 17:51):

but you have 3 + methods elsewhere

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:51):

why*

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:52):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:52):

@Alexander Regueiro because all 3 traits have the prefix

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:52):

I would add 2

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:52):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:52):

though you could imagine not doing so

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:52):

but it'd be hard across crates etc

view this post on Zulip eddyb (Apr 12 2019 at 17:53):

@Alexander Regueiro Sub shares it with Sup1

view this post on Zulip nikomatsakis (Apr 12 2019 at 17:53):

(what am I missing?)

view this post on Zulip eddyb (Apr 12 2019 at 17:53):

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

view this post on Zulip eddyb (Apr 12 2019 at 17:53):

cross-crate reuse has been solved by mw :P

view this post on Zulip eddyb (Apr 12 2019 at 17:53):

we have infra in the compiler to reuse any monomorphization downstream

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 17:55):

@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?

view this post on Zulip eddyb (Apr 12 2019 at 17:55):

@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

view this post on Zulip eddyb (Apr 12 2019 at 17:56):

you use 17 x usize, I use 12 x usize

view this post on Zulip eddyb (Apr 12 2019 at 17:57):

@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)

view this post on Zulip eddyb (Apr 12 2019 at 17:57):

(deleted)

view this post on Zulip eddyb (Apr 12 2019 at 17:58):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 18:00):

oh sorry

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:00):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 18:00):

got my associativity all wrong haha

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:00):

but also I don't care to argue about this

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:00):

do whaever

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:00):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:00):

no, I mean

view this post on Zulip eddyb (Apr 12 2019 at 18:00):

we build vtables on demand

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 18:00):

so yeah, makes sense now eddyb

view this post on Zulip eddyb (Apr 12 2019 at 18:00):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:01):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:01):

but I still don't think you can know that

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:01):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:01):

no, that's not what I was referring to

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:01):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:01):

and it might do the upcast

view this post on Zulip eddyb (Apr 12 2019 at 18:01):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:01):

oh, I see

view this post on Zulip eddyb (Apr 12 2019 at 18:01):

unless there is a direct cast

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:01):

sure, ok

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:01):

/me shrugs

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:01):

whoop-dee-doo :P

view this post on Zulip eddyb (Apr 12 2019 at 18:01):

offsetting is also cheaper

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:02):

yes, that is true

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:02):

is there a way to handle cycles with offsetting?

view this post on Zulip eddyb (Apr 12 2019 at 18:02):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:02):

why?

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:02):

just so we can shave off a few cycles?

view this post on Zulip eddyb (Apr 12 2019 at 18:02):

because they're an abomination :P?

view this post on Zulip eddyb (Apr 12 2019 at 18:02):

and they complicate everything

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:02):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:02):

so we can decide this later

view this post on Zulip eddyb (Apr 12 2019 at 18:02):

but yeah you can do it with offsets

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:03):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:03):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:03):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:03):

yeah, ok

view this post on Zulip eddyb (Apr 12 2019 at 18:03):

at that point there is no reason to offset at all

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:03):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:03):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:04):

SCC, yes, ok

view this post on Zulip eddyb (Apr 12 2019 at 18:04):

would just use the same vtable

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:04):

that's a good point :)

view this post on Zulip eddyb (Apr 12 2019 at 18:04):

they are one trait

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:04):

(that's actually kind of neat) =)

view this post on Zulip centril (Apr 12 2019 at 18:04):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:04):

this is another reason cycles are silly

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:04):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:04):

mutual cyclical things are one cyclical thing

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:04):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:05):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 18:05):

soo....

view this post on Zulip eddyb (Apr 12 2019 at 18:05):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:05):

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

view this post on Zulip nikomatsakis (Apr 12 2019 at 18:05):

and who will be doing them:)

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 18:06):

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? :-)

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 18:06):

yep, thanks @nikomatsakis

view this post on Zulip centril (Apr 12 2019 at 18:06):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 18:07):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 18:07):

;-)

view this post on Zulip centril (Apr 12 2019 at 18:07):

I can probably write a bit on the rfc

view this post on Zulip eddyb (Apr 12 2019 at 18:07):

you might want to get @oli in on this

view this post on Zulip centril (Apr 12 2019 at 18:07):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:07):

wrt miri and layout of vtables

view this post on Zulip eddyb (Apr 12 2019 at 18:08):

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

view this post on Zulip centril (Apr 12 2019 at 18:08):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:09):

not related I don't think

view this post on Zulip eddyb (Apr 12 2019 at 18:09):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 18:13):

@eddyb anyway does the above sound okay re notes?

view this post on Zulip eddyb (Apr 12 2019 at 18:15):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:15):

that was the main thing of note :P

view this post on Zulip eddyb (Apr 12 2019 at 18:16):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:16):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:17):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 18:18):

@eddyb okay thanks. what about MIR?

view this post on Zulip eddyb (Apr 12 2019 at 18:18):

what about it?

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 18:18):

nothing to be done there?

view this post on Zulip eddyb (Apr 12 2019 at 18:18):

not that I can think of

view this post on Zulip eddyb (Apr 12 2019 at 18:19):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 18:19):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:19):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:19):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:19):

the miri stuff is creating the vtable :P

view this post on Zulip eddyb (Apr 12 2019 at 18:20):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:21):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:21):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 18:22):

okay...

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 18:22):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:22):

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)

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 18:22):

did you refer to that before?

view this post on Zulip eddyb (Apr 12 2019 at 18:22):

yeah, vtables

view this post on Zulip eddyb (Apr 12 2019 at 18:22):

miri builds the actual vtable bytes, so to say

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 18:23):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 18:23):

I see

view this post on Zulip eddyb (Apr 12 2019 at 18:23):

no

view this post on Zulip eddyb (Apr 12 2019 at 18:23):

those are traits::select

view this post on Zulip eddyb (Apr 12 2019 at 18:24):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:24):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:24):

I guess it hasn't happened yet

view this post on Zulip eddyb (Apr 12 2019 at 18:26):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:26):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:27):

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

view this post on Zulip eddyb (Apr 12 2019 at 18:27):

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

view this post on Zulip Alexander Regueiro (Apr 12 2019 at 18:29):

@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)

view this post on Zulip RalfJ (Apr 12 2019 at 20:18):

@eddyb

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

not that I know of

view this post on Zulip oli (Apr 12 2019 at 21:49):

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

it has not

view this post on Zulip oli (Apr 12 2019 at 21:50):

oh lol

view this post on Zulip oli (Apr 12 2019 at 21:50):

I should've scrolled down

view this post on Zulip oli (Apr 12 2019 at 21:51):

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

view this post on Zulip eddyb (Apr 13 2019 at 13:50):

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

view this post on Zulip eddyb (Apr 13 2019 at 13:50):

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

view this post on Zulip eddyb (Apr 13 2019 at 13:51):

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

view this post on Zulip Alexander Regueiro (May 05 2019 at 21:52):

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

why can they share the first? @eddyb

view this post on Zulip eddyb (May 06 2019 at 11:49):

uhhh

view this post on Zulip eddyb (May 06 2019 at 11:56):

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

view this post on Zulip eddyb (May 06 2019 at 11:56):

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

view this post on Zulip Alexander Regueiro (May 07 2019 at 00:07):

@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...

view this post on Zulip eddyb (May 07 2019 at 00:18):

OH!

view this post on Zulip eddyb (May 07 2019 at 00:18):

that's where the confusion was

view this post on Zulip eddyb (May 07 2019 at 00:19):

Sub and Sup1 are traits

view this post on Zulip eddyb (May 07 2019 at 00:19):

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

view this post on Zulip eddyb (May 07 2019 at 00:20):

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

view this post on Zulip eddyb (May 07 2019 at 00:21):

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

view this post on Zulip Alexander Regueiro (May 07 2019 at 00:30):

@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)?

view this post on Zulip eddyb (May 07 2019 at 00:31):

yupp

view this post on Zulip Alexander Regueiro (May 07 2019 at 00:31):

@eddyb thanks. all clear now.

view this post on Zulip Alexander Regueiro (May 07 2019 at 00:36):

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]?

view this post on Zulip Alexander Regueiro (May 15 2019 at 13:29):

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

view this post on Zulip Alexander Regueiro (May 15 2019 at 13:29):

                // Register an obligation for `TraitA: TraitB`.
                nested.extend(
                    data_b.iter()
                        .map(|d| predicate_to_obligation(d.with_self_ty(tcx, source_ty))),
                );

view this post on Zulip eddyb (May 15 2019 at 14:13):

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

view this post on Zulip Alexander Regueiro (May 15 2019 at 16:09):

@eddyb no prob

view this post on Zulip Alexander Regueiro (May 15 2019 at 19:35):

@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

view this post on Zulip eddyb (May 16 2019 at 06:21):

I think that's correct

view this post on Zulip Alexander Regueiro (May 16 2019 at 14:57):

@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...

view this post on Zulip Alexander Regueiro (May 16 2019 at 15:17):

looks reasonably straightforward.

view this post on Zulip Alexander Regueiro (May 16 2019 at 15:18):

unless I've misunderstood something

view this post on Zulip Alexander Regueiro (May 16 2019 at 21:43):

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

view this post on Zulip Alexander Regueiro (May 18 2019 at 18:20):

@eddyb noticed the PR I opened yet?

view this post on Zulip eddyb (May 20 2019 at 12:35):

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

view this post on Zulip eddyb (May 20 2019 at 12:36):

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)

view this post on Zulip oli (May 20 2019 at 13:54):

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

view this post on Zulip Alexander Regueiro (May 20 2019 at 15:11):

don't worry, it was a silly idea

view this post on Zulip Alexander Regueiro (May 20 2019 at 15:11):

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

view this post on Zulip Alexander Regueiro (May 20 2019 at 15:12):

@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.

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 15:13):

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

view this post on Zulip nikomatsakis (Jul 05 2019 at 15:14):

I see

view this post on Zulip nikomatsakis (Jul 05 2019 at 15:14):

presumably due to some changes to coercion

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 15:15):

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

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 15:16):

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

view this post on Zulip nikomatsakis (Jul 05 2019 at 15:16):

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

- data_a.principal_def_id() == data_b.principal_def_id()

view this post on Zulip nikomatsakis (Jul 05 2019 at 15:16):

though perhaps not at fault

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 15:16):

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

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 15:17):

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

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 15:17):

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

view this post on Zulip nikomatsakis (Jul 05 2019 at 15:20):

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

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 15:20):

yep

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 15:20):

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

view this post on Zulip nikomatsakis (Jul 05 2019 at 15:22):

indeed, always easiest in the home environment

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 15:22):

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

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 15:23):

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

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 15:23):

@nikomatsakis

view this post on Zulip nikomatsakis (Jul 05 2019 at 15:23):

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

view this post on Zulip nikomatsakis (Jul 05 2019 at 15:23):

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

view this post on Zulip nikomatsakis (Jul 05 2019 at 15:24):

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

view this post on Zulip nikomatsakis (Jul 05 2019 at 15:24):

but that's not related to you :)

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 15:24):

yeah no worries Niko

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 15:24):

heh

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 15:24):

when are you taking your holiday by the way?

view this post on Zulip nikomatsakis (Jul 05 2019 at 15:25):

it starts July 15

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 15:25):

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

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 15:26):

@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.)

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 15:36):

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

view this post on Zulip nikomatsakis (Jul 05 2019 at 17:26):

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.

view this post on Zulip nikomatsakis (Jul 05 2019 at 17:27):

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

view this post on Zulip nikomatsakis (Jul 05 2019 at 17:29):

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>();
}

view this post on Zulip nikomatsakis (Jul 05 2019 at 17:29):

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.

view this post on Zulip nikomatsakis (Jul 05 2019 at 17:30):

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. =)

view this post on Zulip nikomatsakis (Jul 05 2019 at 17:33):

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".

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 17:48):

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

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 17:48):

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

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 17:48):

anyway, I see the problem now, thanks for explaining

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 17:48):

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

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 17:55):

@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.

view this post on Zulip nikomatsakis (Jul 05 2019 at 20:06):

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.

view this post on Zulip nikomatsakis (Jul 05 2019 at 20:07):

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.

view this post on Zulip nikomatsakis (Jul 05 2019 at 20:08):

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)

view this post on Zulip nikomatsakis (Jul 05 2019 at 20:08):

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

view this post on Zulip nikomatsakis (Jul 05 2019 at 20:08):

i.e., in a separate PR

view this post on Zulip nikomatsakis (Jul 05 2019 at 20:08):

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

view this post on Zulip nikomatsakis (Jul 05 2019 at 20:09):

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

view this post on Zulip nikomatsakis (Jul 05 2019 at 20:09):

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

view this post on Zulip nikomatsakis (Jul 05 2019 at 20:09):

but I have to think what that means

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 23:53):

@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. :-)

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 23:54):

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

view this post on Zulip Alexander Regueiro (Jul 05 2019 at 23:54):

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?

view this post on Zulip Alexander Regueiro (Aug 05 2019 at 15:43):

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?

view this post on Zulip varkor (Aug 05 2019 at 15:46):

I'm not sure, no

view this post on Zulip Alexander Regueiro (Aug 05 2019 at 16:09):

no prob

view this post on Zulip Alexander Regueiro (Aug 05 2019 at 16:11):

@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.

view this post on Zulip varkor (Aug 05 2019 at 16:11):

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

view this post on Zulip Alexander Regueiro (Aug 05 2019 at 16:15):

Cool, thanks.

view this post on Zulip nikomatsakis (Aug 27 2019 at 17:43):

Hey @Alexander Regueiro

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 17:43):

Hey Niko.

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 17:43):

got a bit of time now then?

view this post on Zulip nikomatsakis (Aug 27 2019 at 17:43):

A bit, yeah

view this post on Zulip nikomatsakis (Aug 27 2019 at 17:44):

So I guess this is still blocking you :)

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 17:44):

yeah, a bit...

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 17:44):

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

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 17:44):

and what exactly I need to do

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 17:44):

you went over them at a high level above.

view this post on Zulip nikomatsakis (Aug 27 2019 at 17:47):

Yeah I'm reviewing the code

view this post on Zulip nikomatsakis (Aug 27 2019 at 17:47):

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

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 17:49):

oh I see

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 17:50):

hrmm

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 17:50):

and they're needed why exactly?

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 17:54):

@nikomatsakis

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:00):

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

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:00):

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

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:00):

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

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:00):

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

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:01):

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

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:02):

oh

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:02):

                nested.extend(
                    data_b.iter()
                        .map(|d| predicate_to_obligation(d.with_self_ty(tcx, source_ty))),
                );

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:02):

of course

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:02):

not that the code is wrong

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:02):

mhm...

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:02):

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

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:02):

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

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:02):

which in turn hits that ambiguous case

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:03):

right

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:04):

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

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:04):

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

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:04):

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

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:04):

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

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:04):

mhm...

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:04):

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

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:06):

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

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:06):

hmm

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:06):

yes, does sound quite hacky though

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:06):

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

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:06):

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

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:06):

it got farther

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:06):

I don't mind really

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:06):

okay.

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:07):

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),
    |                                                ^^^^^^^^^^^^^^^^^^^^^^^^^

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:07):

not sure what's up with that

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:07):

anyway, I think I would try to add some kind of "skip auto traits" check with a FIXME

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:07):

we could file a related issue, using my example above

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:08):

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

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:08):

yeah. I didn't know chalk was being used already by default though...?

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:08):

mhm

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:08):

sounds fair enough

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:08):

how would you see a less hacky fix going though?

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:09):

well, e.g. for chalk, it isn't necessary to find a unique path necessarily

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:09):

so one option would be to just not have a problem with multiple options

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:10):

but that requires some deeper changes in how trait resolution works

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:12):

I see

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:12):

so many best just to leave that to you and the Chalk guys?

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:19):

@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?)

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:20):

@Alexander Regueiro yeah I'm hoping you don't :)

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:20):

I did a hackier change, after all

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:20):

exactly

view this post on Zulip nikomatsakis (Aug 27 2019 at 18:20):

but let me know and I'll do some debugging

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:20):

and that error msg is very weird heh

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:21):

@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.

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:21):

I suppose we'll be having meetings again now, at least.

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:21):

traits-WG ones

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:37):

@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?

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 18:37):

I guess I'd just give some empty ParamEnv too

view this post on Zulip nikomatsakis (Aug 27 2019 at 20:10):

@Alexander Regueiro no, the best way is to use the evaluate_obligation query

view this post on Zulip Alexander Regueiro (Aug 27 2019 at 21:28):

Okay thanks!

view this post on Zulip Alexander Regueiro (Sep 04 2019 at 01:20):

Okay thanks!

view this post on Zulip Alexander Regueiro (Sep 10 2019 at 21:00):

@nikomatsakis We'll talk here then.

view this post on Zulip Alexander Regueiro (Sep 10 2019 at 22:45):

@nikomatsakis got a little time for this today or not? no worries if not.

view this post on Zulip Alexander Regueiro (Sep 11 2019 at 20:41):

@nikomatsakis how about today? :-)

view this post on Zulip nikomatsakis (Sep 11 2019 at 22:54):

Hey @Alexander Regueiro, sorry, today was crazy so far

view this post on Zulip nikomatsakis (Sep 11 2019 at 22:54):

Let me review the PR :)

view this post on Zulip Alexander Regueiro (Sep 11 2019 at 22:54):

hey, no problem...

view this post on Zulip Alexander Regueiro (Sep 11 2019 at 22:54):

cheers!

view this post on Zulip nikomatsakis (Sep 11 2019 at 22:55):

is current status still that those examples don't work, @Alexander Regueiro ?

view this post on Zulip Alexander Regueiro (Sep 11 2019 at 22:55):

yeah

view this post on Zulip nikomatsakis (Sep 11 2019 at 22:55):

if so, it does seem like it's probably some kind of vtable counting problem

view this post on Zulip Alexander Regueiro (Sep 11 2019 at 22:56):

the bit of code I highlighted in my last comment is surely the key

view this post on Zulip nikomatsakis (Sep 11 2019 at 22:56):

yeah, prob, let me do a local build

view this post on Zulip Alexander Regueiro (Sep 11 2019 at 22:56):

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?

view this post on Zulip Alexander Regueiro (Sep 11 2019 at 22:56):

(since this is in codegen)

view this post on Zulip Alexander Regueiro (Sep 11 2019 at 22:58):

@nikomatsakis ^

view this post on Zulip nikomatsakis (Sep 11 2019 at 23:00):

hmm

view this post on Zulip nikomatsakis (Sep 11 2019 at 23:00):

remind me, we are doing the "roll out all the methods into the vtable" approach, right?

view this post on Zulip nikomatsakis (Sep 11 2019 at 23:00):

i.e., one big flat vtable?

view this post on Zulip Alexander Regueiro (Sep 11 2019 at 23:00):

yeah

view this post on Zulip Alexander Regueiro (Sep 11 2019 at 23:01):

there's a flat_map somewhere that does that, I forget where exactly...

view this post on Zulip nikomatsakis (Sep 11 2019 at 23:01):

I guess I would expect you'd do some sort of query, yes, but I'm not sure just what

view this post on Zulip nikomatsakis (Sep 11 2019 at 23:01):

would have to look at the select.rs code I guess

view this post on Zulip Alexander Regueiro (Sep 11 2019 at 23:02):

the vtable looks like D S A [trait methods] D S A [supertrait 1 methods] D S A [supertrait 2 methods] ...

view this post on Zulip Alexander Regueiro (Sep 11 2019 at 23:02):

yeah...

view this post on Zulip Alexander Regueiro (Sep 11 2019 at 23:02):

I just want to avoid the compiler duplicating work as much as possible, really

view this post on Zulip Alexander Regueiro (Sep 11 2019 at 23:03):

@nikomatsakis grep for count_own_vtable_entries in the diff and you'll see the relevant select.rs code

view this post on Zulip Alexander Regueiro (Sep 11 2019 at 23:03):

it's simple

view this post on Zulip Alexander Regueiro (Sep 11 2019 at 23:03):

well, ish

view this post on Zulip Alexander Regueiro (Sep 11 2019 at 23:04):

there's a supertraits query

view this post on Zulip Alexander Regueiro (Sep 11 2019 at 23:04):

which I may need to repeat...?

view this post on Zulip Alexander Regueiro (Sep 12 2019 at 00:59):

@nikomatsakis still building? :-)

view this post on Zulip nikomatsakis (Sep 12 2019 at 12:15):

@Alexander Regueiro sorry, didn't finish before I had to log out for the night :)

view this post on Zulip Alexander Regueiro (Sep 12 2019 at 18:05):

ah okay

view this post on Zulip Alexander Regueiro (Sep 12 2019 at 18:05):

@nikomatsakis today...?

view this post on Zulip Alexander Regueiro (Sep 13 2019 at 16:31):

@nikomatsakis I'll be out the rest of today, but feel free to leave notes while I'm gone, if you have thoughts.

view this post on Zulip nikomatsakis (Sep 13 2019 at 19:29):

@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?

view this post on Zulip Alexander Regueiro (Sep 13 2019 at 20:55):

@nikomatsakis oh sorry. The way I built it was with —keep-stage 0 after building master stage 0. Easier to debug that way.

view this post on Zulip Alexander Regueiro (Sep 19 2019 at 16:45):

@nikomatsakis hey. did you get it built yet then?

view this post on Zulip Alexander Regueiro (Sep 19 2019 at 16:45):

anyway you may just want to look at the bit of code I highlighted

view this post on Zulip Alexander Regueiro (Sep 19 2019 at 16:45):

in my comment on GH

view this post on Zulip Alexander Regueiro (Sep 19 2019 at 16:45):

and tell me roughly what should go there :-)

view this post on Zulip Alexander Regueiro (Sep 20 2019 at 16:23):

@nikomatsakis well, I'll have a go doing a trait query there and see if I get progress...

view this post on Zulip nikomatsakis (Sep 20 2019 at 20:28):

Sorry @Alexander Regueiro been slammed this wek

view this post on Zulip nikomatsakis (Sep 20 2019 at 20:28):

Trying to catch up a bit

view this post on Zulip nikomatsakis (Sep 20 2019 at 20:34):

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?

view this post on Zulip Alexander Regueiro (Sep 20 2019 at 20:46):

@nikomatsakis well, I'm using it just to get rustc building

view this post on Zulip Alexander Regueiro (Sep 20 2019 at 20:46):

so I can test it on smaller examples

view this post on Zulip Alexander Regueiro (Sep 20 2019 at 20:46):

rather than all of rustc

view this post on Zulip Alexander Regueiro (Sep 20 2019 at 21:06):

@nikomatsakis don't worry if you don't have time today though

view this post on Zulip Alexander Regueiro (Sep 20 2019 at 21:06):

I'll try anyway

view this post on Zulip Alexander Regueiro (Sep 20 2019 at 21:07):

I wouldn't think you'd gain much by a working build like me anyway

view this post on Zulip Alexander Regueiro (Sep 20 2019 at 21:07):

it's mainly about what to put in that bit of codeen_ssa

view this post on Zulip nikomatsakis (Sep 23 2019 at 20:40):

well, I'm using it just to get rustc building

oh, I see

view this post on Zulip nikomatsakis (Sep 23 2019 at 20:41):

useful hack, yes

view this post on Zulip nikomatsakis (Sep 25 2019 at 12:40):

In answer to your question @Alexander Regueiro -- in codegen, when we dispatch a foo.bar() call, we invoke codegen_fulfill_obligation

view this post on Zulip Alexander Regueiro (Sep 25 2019 at 13:59):

okay, thanks

view this post on Zulip Alexander Regueiro (Sep 25 2019 at 13:59):

I'll still need to do a query in the codegen for the upcast then, I suppose? don't see a way around it

view this post on Zulip nikomatsakis (Sep 25 2019 at 14:37):

@Alexander Regueiro can you send me a quick link to where you would need the info?

view this post on Zulip nikomatsakis (Sep 25 2019 at 14:37):

sorry if the link's already in the PR

view this post on Zulip Alexander Regueiro (Sep 25 2019 at 14:57):

@nikomatsakis yeah, it's here: https://github.com/rust-lang/rust/blob/45859b7ca764cafb14efb8c63a83d5e48dc5d016/src/librustc_codegen_ssa/base.rs#L141

view this post on Zulip Alexander Regueiro (Sep 25 2019 at 14:57):

also curious how I do a pointer offset in codegen_ssa (I have basicaly no experience with codegen in rustc, I'm afraid.)

view this post on Zulip Alexander Regueiro (Sep 25 2019 at 15:01):

so yeah, if we can figure out how to do that, and get the appropriate VtableObject value, then bingo.

view this post on Zulip Alexander Regueiro (Sep 25 2019 at 18:01):

@nikomatsakis hmm, so maybe I can/should call codegen_fulfill_obligation from within the unsized_info method?

view this post on Zulip Alexander Regueiro (Sep 25 2019 at 18:01):

(in codegen_ssa)

view this post on Zulip nikomatsakis (Sep 26 2019 at 15:29):

@Alexander Regueiro you said that @eddyb helped you out here, or do you still have pending questions?

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 15:33):

@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?

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 16:16):

he's gone AFK for now, but here's my last question @nikomatsakis

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 16:16):

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)?

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:00):

hey @nikomatsakis. around now? :-)

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:01):

yep, just reading your last few comments

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:02):

I'm going to have to go read into that code

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:02):

it is pretty thoroughly out of cache

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:02):

okay thanks

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:02):

ps, is your branch up to date?

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:02):

@nikomatsakis I can push my WIP code right now if you like too

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:02):

heh

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:02):

let me do that.

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:02):

always useful, thanks

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:09):

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

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:09):

done

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:09):

@nikomatsakis ^

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:10):

@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.

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:10):

was focusing more on codegn

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:14):

yep, ok, I'm reading your code now

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:14):

I still don't really understand quite what you meant when you were saying you needed "something equivalent to coerce_unsized_into

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:15):

okay, well...

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:16):

@nikomatsakis src/librustc_codegen_ssa/mir/rvalue.rs|:227 (the FIXME)

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:16):

does that help clarify?

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:18):

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

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:18):

hmm ok

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:19):

is that clearer? sorry, the code is a bit messy still...

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:19):

well.... it wasn't exactly super-clean to begin with heh.

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:20):

it's clear-ish

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:20):

this code has changed a lot since I last looke at it

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:20):

well idk if that's true, maybe I just forget :)

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:20):

heh, easily done in any casee

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:23):

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?

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:23):

er, well, I guess we also return an operand

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:23):

right, I think so.

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:23):

whihc I guess is sort that store you were talking about

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:23):

both work with OpereandValues though

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:24):

in their corresponding matches

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:24):

yeah so the heart of coerce_unsized_into is this code:

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:24):

    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);
    };

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:25):

which doesn't really want to take a PlaceRef->PlaceRef

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:25):

that is, it's kind of a wrapper around a Operand -> OPerand I think

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:25):

right, I'm thinking that can be factored out

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:25):

that sounds right

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:25):

and used in codegen_rvalue_operand

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:26):

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(...);
}

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:26):

but we still need something like src/librustc_codegen_ssa/base.rs:299 (that other match) in codegen_rvalue_operand, no?

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:26):

exactly

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:26):

maybe call it fn coerce_operand_unsized though (?)

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:27):

or fn coerce_unsized_ptr?

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:27):

yeah I mean whatever :)

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:27):

I was just being sloppy

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:27):

yeah that's me unnecessarily focusing on details sorry heh

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:27):

so....

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:27):

anyway, do you need that match on line 299...

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:28):

I guess the answer is yes

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:28):

@nikomatsakis well I'm alluding to something like that in rvalue.rs:227(the FIXME comment)

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:28):

because we presumably want to be able to coerce (e.g.) Arc<Foo> to Arc<Bar>

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:28):

yeah

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:28):

but all that line does

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:29):

the final arm of that match is the tricky one though. it involves recursion of the fn.

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:29):

well I guess the fn I wrote isn't really how you want to factor it

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:29):

you probably want a function that takes the result of load_operand combined with a type

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:29):

as the "source"

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:29):

and seems to be predicated specifically on using places

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:29):

and then does the match etc

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:29):

hmm

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:29):

oh yes

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:29):

hmm so the final case, that's the Arc<...> cse

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:29):

I hadn't read closely and just assumed that :-)

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:29):

and yes it is predicated on places

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:30):

well so the other thing you can do of course

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:30):

is to spill :)

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:31):

(which might well make sense)

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:31):

see the match at rvalue.rs:56... I wonder if we can do something similar in codgen_rvalue_operand?

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:31):

that's what I was alluding to before

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:31):

oh, "spill"?

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:31):

right, that's what I meant by "spill"

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:31):

create a temporary if you need one

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:31):

aha yep

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:31):

that last case feels very painful to handle without spilling; you could potentially spill only in tha case

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:32):

yeah, that's an interesting thought

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:32):

you will typically have a PlaceRef anyway I think

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:32):

well I'm not sure how OperandRef works exactly

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:32):

but in the MIR you are usually casting from a place

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:32):

would it be more efficient to special-case the last case though?

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:32):

(in theory it can be a constant)

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:32):

because it's obviously more of a pain

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:32):

I see

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:32):

not sure what you mean by special-case

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:32):

I am proposing to special case it :)

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:33):

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

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:34):

sorry, I meant whether to "special-case" in the opposite sense: not create a temporary for the simpler cases.

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:34):

easier to write, but potentially less efficient

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:35):

ok. yes, I think we're saying the same thing

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:35):

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

view this post on Zulip nikomatsakis (Sep 26 2019 at 17:35):

@eddyb is probably also a good one to help, indeed, as they're much closer to this code than I am

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:36):

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!)

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:36):

@nikomatsakis sure. as for the codegen_fulfill_obligation... should I be calling that from unsized_info, or what?

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:36):

feel feel to reply whenever.

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:36):

thanks for your time anyway!

view this post on Zulip Alexander Regueiro (Sep 26 2019 at 17:36):

it's been helpful

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:21):

@Alexander Regueiro let's chat here

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:21):

so I saw the gist but maybe the best is if I just look at the state of your branch?

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:22):

sure

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:22):

@nikomatsakis yep, let me push now...

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:28):

@nikomatsakis pull and grep for // FIXME: what should go here? :-)

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:28):

or...

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:28):

just take a look at the whole of the last commit

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:29):

I think it's all new stuff

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:34):

ok

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:38):

something about the names feels wrong to me here

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:40):

names of the fns? sure, suggest new ones, by all means

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:41):

I'm confused about coerce_unsized_operand

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:41):

the main thing I'm concerned about right now is the functionality though

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:41):

hmm

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:41):

there's no such fn?

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:42):

indeed, there is no such fn

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:42):

are you looking at an old commit by chance?

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:42):

that is what is confusing me :)

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:42):

no, it's commented out code

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:42):

where did it come from?

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:42):

ah

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:42):

I don't even see it commented out

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:42):

grep returns nothing hmm!

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:43):

well anyway not imp't

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:43):

to answer your question

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:43):

let info = info; // FIXME: what should go here?

that question, I mean

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:44):

yeah

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:44):

I thnk that the setup is sort of off

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:44):

okay it may well be

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:44):

I'm really a newb when it comes to codegen!

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:44):

just been looking at examples in other places to try to learn

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:45):

basically I think the OperandValue you want to return is to load from dst_scratch

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:45):

right, exactly...

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:45):

but I'm wondering now how that would interact with storage-dea

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:45):

ah yes

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:45):

I was wondering about that

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:45):

whether it invalidates the operand even if I do storage-dead after fetching the operand value

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:46):

if not, maybe I can just do OperandValue::Pair(dst_scratch.llval, dst_scratch.llextra) for that line? though that seems suspect too?

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:48):

I think that's invalid, yes

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:48):

(BTW, I pushed again. no major changes, just fixed some build errors due to copy & paste coding...)

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:49):

hmm

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:49):

so I feel like, to go this way, you really don't want to be returning an operand

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:49):

you want to be given a dest

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:49):

looking a bit further out

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:49):

at the callers of this function

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:49):

one of them has a destination

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:49):

possibly

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:49):

the other does not

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:49):

however, the other caller comes from a statement assign

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:49):

in particular we have some logic for cases like x = foo where we tried to avoid an alloca for x

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:50):

if x has a "sufficiently complicated" type we'll create one

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:50):

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.

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:50):

basically a non-immediate type (and we include pairs in that case)

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:50):

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"

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:50):

is what I'm getting at

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:50):

it's only called a) recursively b) by cosdegen_rvalue_operand

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:51):

in particular, codegen_rvalue_operand is invoked from the assignment code I was talking about above

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:51):

which itself needs to spit out an OperandRef

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:51):

not assign to a PlaceRef

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:51):

hmm

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:52):

notice that codegen_rvalue_operand already has some logic that assumes this

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:52):

e.g.

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:52):

            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)
            }

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:52):

i.e., it assumes it can be invoked with a Foo { ... } operand that constructs a struct, but only if that struct is a ZST

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:52):

now I don't think this means you can't have an ADT necessarily

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:52):

hmm

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:52):

it's just that your ADT must be represented by a (ptr, info) pair

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:53):

and not some more general structure

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:53):

I'm not 100% sure about that

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:53):

you could test with e.g. Arc

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:53):

yeah, so it's already a fat pointer to an ADT?

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:53):

but the decision of whether to use an alloca for a type is made in the non_ssa_locals function

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:53):

       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.

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:53):

and it seems to be based on the layout

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:53):

(which makes sense)

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:54):

yeah, so it's already a fat pointer to an ADT?

no

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:54):

well I don't know what "fat pointer to an ADT means"

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:54):

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

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:54):

e.g. &dyn FooStruct or Box<dyn FooStruct>?

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:55):

dyn FooStruct is a contradiction, no?

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:55):

okay

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:55):

ughh

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:55):

I'm tired, sorry

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:55):

long day

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:55):

basically if you had a Arc<T>, it would be represented by (in C terms) a *const T

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:55):

actually a *const ArcData<T> or something

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:55):

and so the layout in this case would be a (*const ArcLayout<T>, vtable)

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:56):

I guess that's C terms but Rust syntax :) or .. something

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:56):

guess I was thinking &dyn Trait where FooStruct: Trait

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:56):

point is, the Arc<T> struct is really just a pointer

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:56):

okay

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:56):

anyway the reason I mention all that is

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:56):

the logic that "spills" to temporary stack slots is intended for more complex cases

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:56):

in that case... wouldn't it just be handled by one of the previous two arms of the match?

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:56):

where the struct has arbitray fields and things

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:57):

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

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:57):

however

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:57):

right, not its LLVM type...

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:57):

makes sense

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:58):

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

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:59):

do we want something more like the final match arm in coerce_unsized_into then?

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 21:59):

don't know how that can be adapted for operands and not places though

view this post on Zulip nikomatsakis (Sep 30 2019 at 21:59):

heh man this is annoying

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:00):

so let's first just try to get it working

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:00):

then we'll worry about making it a bit more efficient

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:00):

yep

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:00):

at least the simple case is working now :-P

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:00):

oh wait, wait

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:00):

just not upcasting things like Arc

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:00):

I gues...

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:00):

ok ok so there are two callers of coerce_ptr_unsized but

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:00):

both of them are assuming that the source operand is either a pointer or a (pointer, info) pair

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:01):

in the case of coerce_unsized_into, that's because it's already done the logic of matching the fields of the adts

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:01):

yes that's true

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:01):

but the second?

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:01):

I was worried about that one

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:01):

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)

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:02):

oh. so you're saying codegen_rvalue_operand would never be called for complex ADTs even?

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:03):

correct

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:03):

we would call codegen_rvalue_into or something like that

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:03):

cool

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:03):

or just bug! out?

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:03):

up to you

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:03):

no I mean

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:03):

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

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:03):

the caller ought to have invoked one of the into variants

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:03):

so I think what you want to do in coerce_ptr_unsized

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:04):

you basically want to invoke unsized_info, but the trick is figuring out what type to invoke it with

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:04):

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?

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:04):

okay...

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:04):

ah wait

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:05):

I think maybe this will work?

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:05):

                (&ty::Adt(def_a, _), &ty::Adt(def_b, _)) => {
                    assert_eq!(def_a, def_b);
                    unsized_info(bx, src.ty, dst.ty, Some(info))
                }

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:06):

in particular, unsized_info seems to already expect two instance of the same ADT, and it knows to walk their fields in lockstep

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:06):

(the code for box might be similarly simplified and mergd into this arm, actually)

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:06):

line 131 then?

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:06):

of that file

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:07):

I pushed a commit to your branch

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:07):

not sure if it works

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:07):

but I got to run

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:07):

I think it may be correct

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:07):

in which case I think the match arm I just modified and the match arm before it can be mergd

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:07):

okay thanks!

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:07):

sounds good

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:07):

makes sense actually

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:08):

I'll see if it gets rid of the bootstrap error btw, which is: https://gist.github.com/e20f1817eb2ced1ec25ba706f8a9c31a

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:08):

cool, I'm doing a local build too

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:08):

(the box code is taking Box<T> -> Box<U> coercion and invoking that helper with T, U...

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:08):

it's an LLVM assertion issue with the bootstrap, so that's a bit disconcerting, but oh well..

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:08):

but from what I can see, the helper would deal just fine with Box arguments)

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:08):

with the bootstrap*

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:09):

cool

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:09):

cool, I'm doing a local build too

I'm just doing a ./x.py build -i so I guess i'll find out

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:09):

yeah heh

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:10):

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!

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:11):

@nikomatsakis anyway, see you later. I'm off for now too.

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:12):

@nikomatsakis oh, and I don't think you pushed FYI.

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:12):

oh it got rejected

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:12):

not sure why

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:13):

sorry. probably because of my fixing-build-errors force-push while we were chatting

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:13):

ok

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:13):

I'll rebase

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:13):

or force push, and I can rebase. don't mind much.

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 22:13):

cheers!

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:14):

done

view this post on Zulip nikomatsakis (Sep 30 2019 at 22:16):

(I get a crash trying to bootstrap =)

view this post on Zulip Alexander Regueiro (Sep 30 2019 at 23:18):

heh not too surprised

view this post on Zulip Alexander Regueiro (Oct 01 2019 at 17:21):

hey @nikomatsakis

view this post on Zulip Alexander Regueiro (Oct 01 2019 at 17:21):

so..

view this post on Zulip Alexander Regueiro (Oct 01 2019 at 17:21):

    let bar: Arc<dyn Bar> = Arc::new(42);
    let foo: Arc<dyn Foo> = bar;

view this post on Zulip Alexander Regueiro (Oct 01 2019 at 17:21):

that should work,right?

view this post on Zulip Alexander Regueiro (Oct 01 2019 at 17:22):

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>

view this post on Zulip Alexander Regueiro (Oct 01 2019 at 17:22):

@nikomatsakis let me know if you have any ideas (about this or the LLVM assertion bootstrapping issue)

view this post on Zulip Alexander Regueiro (Oct 01 2019 at 17:31):

there may be an easy fix to that actually... let's see

view this post on Zulip Alexander Regueiro (Oct 01 2019 at 17:55):

@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")
        }

view this post on Zulip nikomatsakis (Oct 03 2019 at 13:26):

@Alexander Regueiro that hack seems ok for now, let's revisit that problem

view this post on Zulip nikomatsakis (Oct 03 2019 at 13:26):

we gotta figure out the cause of these segfaults though

view this post on Zulip Alexander Regueiro (Oct 03 2019 at 16:09):

sure

view this post on Zulip Alexander Regueiro (Oct 03 2019 at 16:09):

@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!

view this post on Zulip nikomatsakis (Oct 04 2019 at 18:41):

@Alexander Regueiro I don't see any LLVM error when bootstrapping

view this post on Zulip nikomatsakis (Oct 04 2019 at 18:41):

I guess maybe it doesn't get that far

view this post on Zulip nikomatsakis (Oct 04 2019 at 18:41):

but those are usually good hint :)

view this post on Zulip nikomatsakis (Oct 04 2019 at 18:42):

let me try rebuilding with verify-llvm-ir=true I guess

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 19:38):

oh interesting. maybe it was a consequence of --keep-stage 0?

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 19:39):

or actually, of -i

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 19:39):

and not removing the LLVM artefacts

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 19:39):

you saw the segfault though yes?

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 19:39):

@nikomatsakis

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:05):

I do see a segfault

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:05):

I'm trying to reproduce it in some way in a debugger

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:05):

and failing

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:06):

like, I literally cannot figure out what command to run :)

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:06):

the last thing I see is

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:06):

     Running `/home/nmatsakis/versioned/rust-2/build/bootstrap/debug/rustc --edition=2018 --crate-name core

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:06):

and the "boostrap rustc" is kind of impossible to run from outside x.py in my experience :)

view this post on Zulip lqd (Oct 04 2019 at 20:09):

one can add --on-fail=env to print the environment IIRC

view this post on Zulip lqd (Oct 04 2019 at 20:10):

likely that --on-fail=bash might work to launch a debugger from there but I've never tried that

view this post on Zulip simulacrum (Oct 04 2019 at 20:20):

@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

view this post on Zulip simulacrum (Oct 04 2019 at 20:20):

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

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:21):

lately i've foudn that is not true

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:21):

that used to be true

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:21):

I'll give it a try though, maybe I'm wrong

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:21):

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

view this post on Zulip lqd (Oct 04 2019 at 20:23):

yeah I now remember there's an issue saying it doesn't work anymore :/

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:24):

ah, good news

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:24):

for some reason I am now seeing the actual rustc: command output...

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:24):

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...

view this post on Zulip simulacrum (Oct 04 2019 at 20:25):

hm strange.. I would not expect that to be the case.

view this post on Zulip simulacrum (Oct 04 2019 at 20:25):

maybe for stage 0 but not stage 1

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:26):

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

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:26):

although hmm

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:28):

I do see a segfault

for the Arc case, @nikomatsakis ?

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:29):

ah righg

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:29):

so this looks like a

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:30):

&mut dyn Write -> &mut dyn Write coercion

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:30):

going wrong somehow ;)

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:30):

hmm

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:30):

how is that a coercion even though?

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:30):

the same type

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:30):

it is

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:30):

because of lifetime bounds

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:30):

oh

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:30):

duh

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:31):

so... maybe I removed too much of the previous code? hmm

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:31):

honestly I'm having a hard time even parsing that type ;)

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:31):

which only had to deal with lifetime bound coercions for trait objects

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:31):

  %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

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:31):

since auto-traits were basically irrelevant

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:31):

yeah... that's not exactly readable

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:32):

presumably called for println! stmt or such though

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:33):

  %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

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:33):

used emacs to at least pare up all the delimiters :)

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:34):

I guess that's some kind of vtable cast

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:35):

insertvalue llvm docs

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:35):

yes... I think so.

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:36):

ok I sort of see what it's complaining about

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:36):

the gep winds up pointing at a specific i64 from the vtable

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:36):

but it is being inserted into a [3 x i164]* field

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:36):

it kind of looks like there is one argument too many on the gep

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:36):

i.e., instead of gep(..., 0, 0) it should be gep(..., 0)

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:36):

is this maybe ringing any bells for any of your edits? :)

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:37):

/me thinks

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:37):

I'm guessing it has to do something with the code that finds a "child" vtable

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:37):

I do this: bx.struct_gep(source_ptr, offset as u64)

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:37):

this is this code

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:37):

            debug!("FOO: unsized_info: vtable={:?} offset={:?} ", vtable, offset);
            bx.struct_gep(source_ptr, offset as u64)

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:37):

the Rust interface to LLVM is very strongly-typed so I don't see how that can go wrong

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:37):

unless it's simply the wrong offset?

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:38):

no, it's more the number of arugments

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:38):

than the value of them

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:38):

I don't think the rust interface to llvm is this strongly typed

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:38):

it doesn't reflect the llvm types of what's inside, does it?

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:38):

hmm

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:39):

of what's inside ptr's? no

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:39):

you're right there

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:39):

anyway I'm trying to dump out sme debug print outs

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:39):

this is also an inbounds gep

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:39):

so maybe it's not that code

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:40):

struct_gep is the right call, isn't it?

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:41):

I figured out the vtable is stored as a struct not an array

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:41):

so presume it is

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:42):

not sure

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:42):

I was doing inbounds_gep before, and that created problems

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:42):

so thismakes sense

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:43):

I forget the difference to be totally honest :P

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:43):

struct_gep isn't translated to that LLVM at any stage, is it?

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:43):

heh yeah

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:43):

it's weird to me

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:43):

don't think so

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:43):

but I'm looking at the backtrace

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:43):

still trying to figure out where this particular llvm instruction is being generated

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:43):

I'm not sure it's that code you cited

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:43):

doesn't quite fit

view this post on Zulip Matthew Jasper (Oct 04 2019 at 20:43):

ProjectionElem::Index uses inbounds_gep

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:44):

yeah

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:44):

I saw that...

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:44):

/me runs in gdb to get backtrace

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:44):

oh I should disable threading

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:45):

that's probably confusing a lot of things

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:45):

how...do you do that

view this post on Zulip simulacrum (Oct 04 2019 at 20:45):

-Ccodegen-units=1 is a good start

view this post on Zulip simulacrum (Oct 04 2019 at 20:46):

-Zno-parallel-llvm also

view this post on Zulip simulacrum (Oct 04 2019 at 20:46):

(maybe just the latter is enough)

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:46):

yeah I just came to both of those :)

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:46):

thanks

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:46):

I don't think it hurts here either way, I'll add 'em both

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:51):

hmm that causes a segfault and not the llvm assertion

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:51):

so maybe that was a red herring

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:51):

@nikomatsakis there's projections being done for complex ADT upcasting... and I'm changing how that code is hit

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:53):

@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);
  }

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:53):

ahh

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:54):

so that explains that

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 20:54):

yeah I wondered

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:54):

can you remind me what you are trying to do here :)

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:54):

like, what vtable layout you have created

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:54):

it's basically a big array

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:54):

with "subvtables" embedded in it, right?

view this post on Zulip nikomatsakis (Oct 04 2019 at 20:55):

er, "Super" vtables I guess :)

view this post on Zulip nikomatsakis (Oct 04 2019 at 21:00):

also, what is the offset measured in?

view this post on Zulip nikomatsakis (Oct 04 2019 at 21:00):

I'm pretty sure the assert was not wrong, but something about -Zno-parallel-llvm leads to us squelching the error somehow

view this post on Zulip nikomatsakis (Oct 04 2019 at 21:00):

I think the struct_gep is just wrong

view this post on Zulip nikomatsakis (Oct 04 2019 at 21:01):

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.

view this post on Zulip nikomatsakis (Oct 04 2019 at 21:02):

I suspect you want a gep with one argument, to basically simulate (in C) ptr + offset

view this post on Zulip nikomatsakis (Oct 04 2019 at 21:02):

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)

view this post on Zulip nikomatsakis (Oct 04 2019 at 21:02):

I guess that when bootstrapping the offset should always be zero

view this post on Zulip nikomatsakis (Oct 04 2019 at 21:09):

@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

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 21:09):

@nikomatsakis offset is measured in... not sure? eddyb just told me it was the right type layout

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 21:10):

yeah, it segfaults fo offset != 0

view this post on Zulip nikomatsakis (Oct 04 2019 at 21:10):

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

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 21:10):

anyway, thanks for explaining

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 21:10):

yeah

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 21:10):

probably

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 21:10):

one sec, I can look now

view this post on Zulip nikomatsakis (Oct 04 2019 at 21:12):

I'm not sure why the type is [i64 x 3]* and not just i64* or somethign

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 21:12):

@nikomatsakis yep, words

view this post on Zulip nikomatsakis (Oct 04 2019 at 21:12):

it seems like i64* is what we really want

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 21:12):

const_usize is used to generate some values

view this post on Zulip nikomatsakis (Oct 04 2019 at 21:12):

@eddyb would probably have informed opinions here

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 21:17):

@nikomatsakis grep for pub fn get_vtable if you want to see how the vtable is constructed now

view this post on Zulip Alexander Regueiro (Oct 04 2019 at 22:46):

@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

view this post on Zulip nikomatsakis (Oct 07 2019 at 15:45):

@Alexander Regueiro did you push any updates?

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 15:49):

@nikomatsakis pushed (last commit is experimentation).

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 15:50):

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.

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 15:50):

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

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 15:50):

anyway, I still get the segfault here

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 15:51):

segfaults on the last println of this

view this post on Zulip nikomatsakis (Oct 07 2019 at 17:45):

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

view this post on Zulip nikomatsakis (Oct 07 2019 at 17:45):

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`

view this post on Zulip nikomatsakis (Oct 07 2019 at 17:46):

I think we were going to add some sort of .. hack or something to work around these, right?

view this post on Zulip nikomatsakis (Oct 07 2019 at 17:46):

I guess building with parallel-compiler=true ought to be enough for now

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 18:02):

we already added the hack, @nikomatsakis

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 18:02):

at least, I added something

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 18:03):

see the bit where I handle auto traits separately

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 18:04):

@nikomatsakis I'm stil doing the hack to bootstrap by building stage 0 master than stage 1 of this branch on top of it

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 18:05):

@nikomatsakis BTW was trying to get the vtable ptr to be the right think using const_ptrcast in my "experimentation" commit

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 18:05):

doesn't seem to have worked

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 18:05):

that is, it's not hurt the situation, but it's not made the second println! in my Gist work either

view this post on Zulip nikomatsakis (Oct 07 2019 at 19:47):

@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?

view this post on Zulip nikomatsakis (Oct 07 2019 at 19:47):

(I'm not yet using the latest tip of your branch)

view this post on Zulip nikomatsakis (Oct 07 2019 at 19:47):

ok but I see it has a workaround for that

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 20:58):

Yeah exactly

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 20:59):

Latest commit fixes that (maybe?)

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 21:00):

But still issue with Arc

view this post on Zulip nikomatsakis (Oct 07 2019 at 21:15):

So I'm trying to summon @eddyb

view this post on Zulip nikomatsakis (Oct 07 2019 at 21:19):

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

view this post on Zulip nikomatsakis (Oct 07 2019 at 21:19):

and then cast back

view this post on Zulip nikomatsakis (Oct 07 2019 at 21:19):

it doesn't feel the most elegant and it seems like our "typing" of vtables in general could use to be updated

view this post on Zulip nikomatsakis (Oct 07 2019 at 21:27):

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

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 22:13):

@nikomatsakis makes sense.

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 22:13):

@nikomatsakis could try that for now though... and leave a note. should I?

view this post on Zulip nikomatsakis (Oct 07 2019 at 22:18):

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

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 22:33):

@nikomatsakis will do, ta

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 23:10):

@nikomatsakis we want u64 and not usize?

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 23:13):

--deleted--

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 23:13):

--deleted--

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 23:27):

@nikomatsakis curious error now: https://gist.github.com/54483ab2c86451c0ba36d006f2a13c64

view this post on Zulip Alexander Regueiro (Oct 07 2019 at 23:39):

--deleted--

view this post on Zulip Alexander Regueiro (Oct 08 2019 at 00:22):

@nikomatsakis pushed a bit more experimentation, but now I'm skeptical that src/librustc_codegen_ssa/base.rs:283 is the right approach / sufficient.

view this post on Zulip Alexander Regueiro (Oct 08 2019 at 00:23):

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?

view this post on Zulip nikomatsakis (Oct 08 2019 at 15:29):

the problem is assertion failures?

view this post on Zulip nikomatsakis (Oct 08 2019 at 15:29):

I'll try to take a look

view this post on Zulip Alexander Regueiro (Oct 08 2019 at 16:01):

@nikomatsakis segfault with the same Gist as before. didn't try bootstrapping, so no assertion failures.

view this post on Zulip Alexander Regueiro (Oct 08 2019 at 16:01):

sorry for keeping on bugging you. this is a pretty gnarly issue, I hope you agree!

view this post on Zulip Alexander Regueiro (Oct 09 2019 at 14:49):

@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() });

view this post on Zulip nikomatsakis (Oct 09 2019 at 21:25):

gnarly indeed! ping away, but I may not get time to investigate until friday

view this post on Zulip Alexander Regueiro (Oct 09 2019 at 22:02):

@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.

view this post on Zulip Alexander Regueiro (Oct 09 2019 at 22:02):

whenever you do get to it, just ping me likewise. ta.

view this post on Zulip Alexander Regueiro (Oct 10 2019 at 15:25):

--deleted--

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 14:40):

@nikomatsakis let me know if you get around to this today :-)

view this post on Zulip nikomatsakis (Oct 11 2019 at 17:15):

@Alexander Regueiro I'll take a look now

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 17:20):

thanks @nikomatsakis

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 17:21):

BTW

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 17:21):

I have to be honest, this whole design process thing sounds like a lot of painful bureaucracy, and introduces many more delays.

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 17:21):

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...

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 17:23):

it's just not very encouraging for me, when the time and it's continuing to demand from me is so great.

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 17:23):

sorry... just had to clear that up.

view this post on Zulip nikomatsakis (Oct 11 2019 at 17:23):

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.

view this post on Zulip nikomatsakis (Oct 11 2019 at 17:24):

One of the biggest problems I think we have now is that we've historically had no great procs

view this post on Zulip nikomatsakis (Oct 11 2019 at 17:24):

which means that big PRs can easily just get stuck

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 17:24):

sure

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 17:24):

but

view this post on Zulip nikomatsakis (Oct 11 2019 at 17:24):

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.

view this post on Zulip nikomatsakis (Oct 11 2019 at 17:24):

Unfortunately, it has to start somewhere.

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 17:25):

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

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 17:25):

I just don't have the willpower or desire to go through that right now, I'm afraid... and not sure when I will.

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 17:26):

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

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 17:26):

so a fork sounds like the best way to go now

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 17:36):

anyway, re. trait upcasting

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 17:36):

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.

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 21:40):

@nikomatsakis ^

view this post on Zulip nikomatsakis (Oct 11 2019 at 21:46):

@Alexander Regueiro I'm looking :)

view this post on Zulip nikomatsakis (Oct 11 2019 at 21:47):

I got distracted because my sister is here for a visit

view this post on Zulip nikomatsakis (Oct 11 2019 at 21:47):

At the moment I'm trying to reproduce the error in gdb so I can see the stack trace

view this post on Zulip nikomatsakis (Oct 11 2019 at 21:47):

this is kind of driving me crazy

view this post on Zulip nikomatsakis (Oct 11 2019 at 21:47):

but I think I finally found the thing to do

view this post on Zulip nikomatsakis (Oct 11 2019 at 21:47):

lol though it's not that useful

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 22:15):

heh

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 22:15):

@nikomatsakis no worries.. :-)

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 22:15):

what's the "thing to do then"?

view this post on Zulip Alexander Regueiro (Oct 11 2019 at 22:27):

@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.

view this post on Zulip simulacrum (Oct 12 2019 at 02:58):

@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

view this post on Zulip Alexander Regueiro (Oct 12 2019 at 03:38):

(deleted)

view this post on Zulip Alexander Regueiro (Oct 12 2019 at 03:40):

@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.

view this post on Zulip nikomatsakis (Oct 12 2019 at 15:34):

@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.

view this post on Zulip nikomatsakis (Oct 12 2019 at 15:34):

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();
}

view this post on Zulip nikomatsakis (Oct 12 2019 at 16:12):

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

view this post on Zulip nikomatsakis (Oct 12 2019 at 16:13):

(ps, we're going to be needing a design doc for this work, too, I think; I'm happy to help some with that)

view this post on Zulip Alexander Regueiro (Oct 12 2019 at 17:38):

@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?

view this post on Zulip Alexander Regueiro (Oct 12 2019 at 17:39):

@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.

view this post on Zulip nikomatsakis (Oct 12 2019 at 23:04):

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

view this post on Zulip Alexander Regueiro (Oct 12 2019 at 23:08):

Will do, ta.

view this post on Zulip nikomatsakis (Oct 14 2019 at 09:38):

@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

view this post on Zulip nikomatsakis (Oct 14 2019 at 09:38):

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

view this post on Zulip nikomatsakis (Oct 14 2019 at 09:39):

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

view this post on Zulip nikomatsakis (Oct 14 2019 at 09:39):

seems to be arising out of this confusion

view this post on Zulip nikomatsakis (Oct 14 2019 at 09:39):

in this case, we expect a i64*

view this post on Zulip nikomatsakis (Oct 14 2019 at 09:40):

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 :)

view this post on Zulip nikomatsakis (Oct 14 2019 at 09:41):

still, hmm, looking at the logs the old_info is always a [3 x i64]*

view this post on Zulip nikomatsakis (Oct 14 2019 at 09:41):

so maybe the problem is somewhere else

view this post on Zulip nikomatsakis (Oct 14 2019 at 09:49):

@Alexander Regueiro btw, do you build with

[llvm]
assertions = true

I am giving that a try now, I forgot this required an extra step

view this post on Zulip nikomatsakis (Oct 14 2019 at 09:50):

also, pushed a few commits

view this post on Zulip nikomatsakis (Oct 14 2019 at 10:43):

Ah, nice, enabling llvm.assertions causes some errors earlier

view this post on Zulip Alexander Regueiro (Oct 14 2019 at 16:00):

@nikomatsakis I don’t believe so (away from computer now). But fair point.

view this post on Zulip Alexander Regueiro (Oct 14 2019 at 16:01):

@nikomatsakis That’s annoying about the different LLVM types, but good spot.

view this post on Zulip Alexander Regueiro (Oct 14 2019 at 17:39):

@nikomatsakis oh, I had set assertions on... that probably explains why I got that error previously

view this post on Zulip Alexander Regueiro (Oct 14 2019 at 17:39):

the bootstrapping one

view this post on Zulip Alexander Regueiro (Oct 14 2019 at 17:39):

a couple of weeks ago

view this post on Zulip nikomatsakis (Oct 14 2019 at 21:31):

@Alexander Regueiro seems good

view this post on Zulip nikomatsakis (Oct 15 2019 at 00:46):

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]* }

view this post on Zulip Alexander Regueiro (Oct 15 2019 at 03:17):

@nikomatsakis huh. I'm not sure what to make of that!

view this post on Zulip Alexander Regueiro (Oct 15 2019 at 03:17):

we need consistency though, either way (which is the better LLVM type?)

view this post on Zulip nikomatsakis (Oct 15 2019 at 18:16):

@Alexander Regueiro yeah I'm not quite sure

view this post on Zulip nikomatsakis (Oct 15 2019 at 18:17):

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

view this post on Zulip Alexander Regueiro (Oct 15 2019 at 18:18):

yeah hmm

view this post on Zulip Alexander Regueiro (Oct 15 2019 at 18:18):

should we be bitcasting rather than pointer-casting?

view this post on Zulip Alexander Regueiro (Oct 15 2019 at 18:18):

not sure of the exact difference in fact, @nikomatsakis

view this post on Zulip Alexander Regueiro (Oct 15 2019 at 18:19):

I see both are done in existing code...

view this post on Zulip nikomatsakis (Oct 15 2019 at 18:22):

So let me drop down a few notes

view this post on Zulip nikomatsakis (Oct 15 2019 at 18:22):

What happens is that we get a Rc<dyn Foo> and we want to "upcast" it to Rc<dyn Foo>

view this post on Zulip nikomatsakis (Oct 15 2019 at 18:23):

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]*

view this post on Zulip nikomatsakis (Oct 15 2019 at 18:23):

hmm the MIR is:

view this post on Zulip nikomatsakis (Oct 15 2019 at 18:24):

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
    }
}

view this post on Zulip nikomatsakis (Oct 15 2019 at 18:25):

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

view this post on Zulip nikomatsakis (Oct 15 2019 at 18:25):

then the return tries to use insertvalue to build the return value

view this post on Zulip nikomatsakis (Oct 15 2019 at 18:25):

but at that point the types disagree

view this post on Zulip nikomatsakis (Oct 15 2019 at 18:25):

some kind of adaptation is missing

view this post on Zulip nikomatsakis (Oct 15 2019 at 18:28):

Some pointers @eddyb to relevant bits of source:

view this post on Zulip Alexander Regueiro (Oct 15 2019 at 19:12):

Okay let’s wait and see what he suggests. Thanks @nikomatsakis.

view this post on Zulip nikomatsakis (Oct 16 2019 at 17:40):

@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)

view this post on Zulip Alexander Regueiro (Oct 16 2019 at 17:54):

@nikomatsakis yeah, true (and I hate open-forever PRs too)

view this post on Zulip Alexander Regueiro (Oct 16 2019 at 18:24):

@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.

view this post on Zulip Alexander Regueiro (Oct 21 2019 at 16:06):

@nikomatsakis I’ll try to just get all the stuff but the codegen merged now okay? Maybe you can review?

view this post on Zulip nikomatsakis (Oct 21 2019 at 16:09):

@Alexander Regueiro sounds great! I've not had time to do anything further yet

view this post on Zulip Alexander Regueiro (Oct 21 2019 at 19:26):

@nikomatsakis no worries. want to talk about it briefly now though? where we go from here.

view this post on Zulip nikomatsakis (Oct 21 2019 at 21:04):

hey @Alexander Regueiro sorry I'm around-ish now

view this post on Zulip nikomatsakis (Oct 21 2019 at 21:05):

one question I was thinking about, did you ever solve that issue with type-checking when parallel compilation is not enable?

view this post on Zulip nikomatsakis (Oct 21 2019 at 21:05):

I remember we had discussed various work arounds

view this post on Zulip Alexander Regueiro (Oct 21 2019 at 21:05):

that's alright

view this post on Zulip Alexander Regueiro (Oct 21 2019 at 21:05):

err

view this post on Zulip Alexander Regueiro (Oct 21 2019 at 21:05):

I'd implemented one

view this post on Zulip Alexander Regueiro (Oct 21 2019 at 21:05):

and I thought it was working yes

view this post on Zulip Alexander Regueiro (Oct 21 2019 at 21:06):

can't remember when in the code it is though @nikomatsakis

view this post on Zulip nikomatsakis (Oct 21 2019 at 21:06):

hmm

view this post on Zulip Alexander Regueiro (Oct 21 2019 at 21:06):

I put it before the recent commits though

view this post on Zulip nikomatsakis (Oct 21 2019 at 21:06):

ok :)

view this post on Zulip Alexander Regueiro (Oct 21 2019 at 21:06):

let me look...

view this post on Zulip Alexander Regueiro (Oct 21 2019 at 21:07):

@nikomatsakis it's where the obligations are generated

view this post on Zulip Alexander Regueiro (Oct 21 2019 at 21:07):

I forget the file for that, sorry

view this post on Zulip nikomatsakis (Oct 21 2019 at 21:09):

@Alexander Regueiro it's ok,

view this post on Zulip nikomatsakis (Oct 21 2019 at 21:09):

I'll find it soon enough :)

view this post on Zulip nikomatsakis (Oct 21 2019 at 21:10):

Did you look at what it means to land the code without codegen changes?

view this post on Zulip nikomatsakis (Oct 21 2019 at 21:10):

Presumably we have to introduce some feature gates

view this post on Zulip nikomatsakis (Oct 21 2019 at 21:11):

I think we are also going to want to try and document the vtable formats

view this post on Zulip nikomatsakis (Oct 21 2019 at 21:11):

I guess I need to spend a bit of time reviewing the PR again

view this post on Zulip Alexander Regueiro (Oct 21 2019 at 21:26):

yep

view this post on Zulip Alexander Regueiro (Oct 21 2019 at 21:26):

it's a real pain this eh

view this post on Zulip Alexander Regueiro (Oct 21 2019 at 21:26):

@nikomatsakis feature gate with a warning message would be ideal

view this post on Zulip Alexander Regueiro (Oct 21 2019 at 21:26):

that's no problem for me to do though

view this post on Zulip Alexander Regueiro (Oct 21 2019 at 21:53):

@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!

view this post on Zulip nikomatsakis (Oct 21 2019 at 21:56):

sounds good

view this post on Zulip Alexander Regueiro (Oct 22 2019 at 16:46):

@eddyb let us know if you have any thoughts on the above BTW :-)

view this post on Zulip nikomatsakis (Nov 04 2019 at 20:01):

@Alexander Regueiro so what's the latest here?

view this post on Zulip Alexander Regueiro (Nov 04 2019 at 20:02):

@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.

view this post on Zulip Alexander Regueiro (Nov 04 2019 at 20:02):

a review either now or once we have a few more tests would be super.

view this post on Zulip Alexander Regueiro (Nov 04 2019 at 20:03):

also trying to implement good diagnostics to suggest turning on the feature when trait upcasting is appropriate

view this post on Zulip Alexander Regueiro (Nov 04 2019 at 20:03):

(i.e. only when a coercion is possible)

view this post on Zulip Alexander Regueiro (Nov 04 2019 at 20:07):

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)?

view this post on Zulip Alexander Regueiro (Nov 04 2019 at 20:12):

@nikomatsakis ^

view this post on Zulip nikomatsakis (Nov 04 2019 at 20:14):

a review either now or once we have a few more tests would be super.

ok!

view this post on Zulip nikomatsakis (Nov 04 2019 at 20:14):

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

view this post on Zulip nikomatsakis (Nov 04 2019 at 20:14):

No, there is no way

view this post on Zulip nikomatsakis (Nov 04 2019 at 20:14):

let me look what that method does

view this post on Zulip nikomatsakis (Nov 04 2019 at 20:16):

@Alexander Regueiro oh, are you saying from within that method, you wish to get teh FnCtxt?

view this post on Zulip Alexander Regueiro (Nov 04 2019 at 20:55):

@nikomatsakis sorry I didn't get a notification!

view this post on Zulip Alexander Regueiro (Nov 04 2019 at 20:55):

hrmm

view this post on Zulip Alexander Regueiro (Nov 04 2019 at 20:56):

@nikomatsakis right, that's what I want

view this post on Zulip Alexander Regueiro (Nov 04 2019 at 20:56):

basically I want to see if type a is coercible to type b from within that method doing diagnostics

view this post on Zulip Alexander Regueiro (Nov 04 2019 at 20:56):

and it needs the appropriate FnCtxt I guess

view this post on Zulip nikomatsakis (Nov 04 2019 at 21:12):

Hmm. I think we should try to "not want" this

view this post on Zulip nikomatsakis (Nov 04 2019 at 21:12):

Can you say more about why you want it?

view this post on Zulip Alexander Regueiro (Nov 04 2019 at 21:36):

@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

view this post on Zulip Alexander Regueiro (Nov 04 2019 at 21:37):

@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?

view this post on Zulip nikomatsakis (Nov 04 2019 at 21:49):

seems good enough to me, @Alexander Regueiro

view this post on Zulip Alexander Regueiro (Nov 04 2019 at 22:12):

@nikomatsakis cool.

view this post on Zulip Alexander Regueiro (Nov 04 2019 at 22:13):

@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. :-)

view this post on Zulip Alexander Regueiro (Nov 08 2019 at 16:12):

@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.

view this post on Zulip nikomatsakis (Nov 08 2019 at 16:14):

@Alexander Regueiro awesome

view this post on Zulip nikomatsakis (Nov 08 2019 at 16:14):

I'm so excited for this to land

view this post on Zulip nikomatsakis (Nov 08 2019 at 16:14):

In this case, do you have trait Foo: Send?

view this post on Zulip nikomatsakis (Nov 08 2019 at 16:15):

I'm guessing this is an artifact of the semi-hokey way we handle auto traits

view this post on Zulip Alexander Regueiro (Nov 08 2019 at 16:18):

@nikomatsakis exactly

view this post on Zulip Alexander Regueiro (Nov 08 2019 at 16:18):

yeah

view this post on Zulip Alexander Regueiro (Nov 08 2019 at 16:18):

probably that

view this post on Zulip Alexander Regueiro (Nov 08 2019 at 17:04):

@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.

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 17:21):

@nikomatsakis Hey Niko. If you have a minute today, would you mind explaining this bit of code to me? (in select.rs)

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 17:21):

                // 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);

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 17:22):

specifically, why do we use b's auto-traits?

view this post on Zulip nikomatsakis (Nov 11 2019 at 18:09):

@Alexander Regueiro looking now

view this post on Zulip nikomatsakis (Nov 11 2019 at 18:12):

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?

view this post on Zulip nikomatsakis (Nov 11 2019 at 18:13):

let me find the code in question

view this post on Zulip nikomatsakis (Nov 11 2019 at 18:16):

(yes, confirmed)

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 19:01):

@nikomatsakis right, makes sense, but that no longer applies now... which is why I think this is a problem

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 19:17):

@nikomatsakis maybe let's address this towards the end of the meeting :-)

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 19:17):

actually, I'll probably be away still

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 19:17):

but feel free to leave ideas

view this post on Zulip nikomatsakis (Nov 11 2019 at 19:40):

right, makes sense, but that no longer applies now... which is why I think this is a problem

@Alexander Regueiro that seems true :)

view this post on Zulip nikomatsakis (Nov 11 2019 at 20:17):

@Alexander Regueiro so that code is used to create source_ty

view this post on Zulip nikomatsakis (Nov 11 2019 at 20:17):

I think that code should move into the else branch

view this post on Zulip nikomatsakis (Nov 11 2019 at 20:17):

i.e., you only need it if you have don't have trait-upcasting enabled

view this post on Zulip nikomatsakis (Nov 11 2019 at 20:17):

the trait-upcasting branch can use the variable source

view this post on Zulip nikomatsakis (Nov 11 2019 at 20:18):

probably we should rename source_ty, too

view this post on Zulip nikomatsakis (Nov 11 2019 at 20:18):

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

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 23:21):

@nikomatsakis heh, that's exactly what I did just after I messaged you. sorry. but good to have confirmation!

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 23:21):

and yes, I renamed source_ty to just source, shadowing the existing source parameter

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 23:21):

which I think makes sense.

view this post on Zulip nikomatsakis (Nov 11 2019 at 23:37):

@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

view this post on Zulip nikomatsakis (Nov 11 2019 at 23:37):

or source_with_target_auto_traits :)

view this post on Zulip nikomatsakis (Nov 11 2019 at 23:37):

I just finished reading the PR, it seems great so far

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 23:38):

hah, that's verbose alright

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 23:38):

@nikomatsakis cool. I just made that fix and reran tests BTW. upcasting with auto-traits working nicely now!

view this post on Zulip nikomatsakis (Nov 11 2019 at 23:38):

:tada: =)

view this post on Zulip nikomatsakis (Nov 11 2019 at 23:38):

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

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 23:39):

yeah you're right

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 23:39):

I'll use that then!

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 23:39):

@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+? :-)

view this post on Zulip nikomatsakis (Nov 11 2019 at 23:39):

@Alexander Regueiro yep, I think so :)

view this post on Zulip nikomatsakis (Nov 11 2019 at 23:40):

I would like to think about what rustc-guide additions are needed here, but I think it can come after the PR

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 23:40):

great. I'll try to have that done by tomorrow (just in case you have the time tomorrow)

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 23:40):

yeah

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 23:40):

indeed

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 23:40):

(I added a brief chapter to the unstable book in this PR of course.)

view this post on Zulip Alexander Regueiro (Nov 11 2019 at 23:52):

anyway, thanks for the review. I'll try to address those comments tonight too, else tomorrow.

view this post on Zulip Alexander Regueiro (Nov 13 2019 at 01:29):

@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. all other tests work however.
  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)

view this post on Zulip nikomatsakis (Nov 13 2019 at 14:30):

@Alexander Regueiro ok! great

view this post on Zulip nikomatsakis (Nov 13 2019 at 14:30):

let me start a local build

view this post on Zulip nikomatsakis (Nov 13 2019 at 14:30):

and investigate

view this post on Zulip Alexander Regueiro (Nov 13 2019 at 14:49):

@nikomatsakis cool. thanks.

view this post on Zulip nikomatsakis (Nov 13 2019 at 15:05):

ok I can reproduce the problems at least :)

view this post on Zulip Alexander Regueiro (Nov 13 2019 at 18:35):

heh okay, good. let me know if you have ideas @nikomatsakis

view this post on Zulip nikomatsakis (Nov 14 2019 at 14:39):

@Alexander Regueiro so i'm looking at the diamond case

view this post on Zulip nikomatsakis (Nov 14 2019 at 14:39):

I simplified the test file a lot

view this post on Zulip nikomatsakis (Nov 14 2019 at 14:40):

my simplified test

view this post on Zulip nikomatsakis (Nov 14 2019 at 14:40):

this still crashes

view this post on Zulip nikomatsakis (Nov 14 2019 at 14:40):

the LLVM output is .. basically illegible :)

view this post on Zulip nikomatsakis (Nov 14 2019 at 14:40):

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

view this post on Zulip nikomatsakis (Nov 14 2019 at 14:40):

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

view this post on Zulip nikomatsakis (Nov 14 2019 at 14:41):

I would guess that the methods from Foo need to be duplicated across the table

view this post on Zulip nikomatsakis (Nov 14 2019 at 14:41):

and that they are not

view this post on Zulip nikomatsakis (Nov 14 2019 at 14:41):

probably because when we are enumerating the set of supertraits etc we are avoiding walking the same thing twice

view this post on Zulip nikomatsakis (Nov 14 2019 at 14:41):

but we need to if we are going to use this vtable generation strategy

view this post on Zulip Alexander Regueiro (Nov 14 2019 at 16:31):

@nikomatsakis yes I think you're spot on.

view this post on Zulip Alexander Regueiro (Nov 14 2019 at 16:53):

@nikomatsakis and thanks for investigating!

view this post on Zulip Alexander Regueiro (Nov 14 2019 at 16:59):

@nikomatsakis I wonder... what cases precisely I need to duplicate (some of) the supertraits in. probably just diamond inheritance?

view this post on Zulip nikomatsakis (Nov 16 2019 at 10:13):

@Alexander Regueiro I think all of them

view this post on Zulip nikomatsakis (Nov 16 2019 at 10:13):

that's the whole point with this style -- you have to make a tree, not a DAG

view this post on Zulip nikomatsakis (Nov 16 2019 at 10:29):

@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

view this post on Zulip nikomatsakis (Nov 16 2019 at 10:29):

But I do think you have to de-duplicate everything

view this post on Zulip nikomatsakis (Nov 16 2019 at 10:29):

Put another way, the trait should have one complete copy of the vtables for each of its supertraits

view this post on Zulip nikomatsakis (Nov 16 2019 at 10:29):

if you have trait Foo: Bar + Bar, then you don't need two copies of Bar

view this post on Zulip nikomatsakis (Nov 16 2019 at 10:30):

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

view this post on Zulip Alexander Regueiro (Nov 16 2019 at 21:11):

@nikomatsakis oh dear! well, thanks for trying to investigate anyway

view this post on Zulip Alexander Regueiro (Nov 16 2019 at 21:12):

@nikomatsakis can I can only deduplicate if the type args are the same though right?

view this post on Zulip Alexander Regueiro (Nov 16 2019 at 21:17):

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?

view this post on Zulip Alexander Regueiro (Nov 16 2019 at 21:21):

^ hmm, presumably because we go up the type hierarchy as we offset into the vtable, right @nikomatsakis ?

view this post on Zulip Alexander Regueiro (Nov 16 2019 at 21:22):

so we don't want to duplicate everything then

view this post on Zulip Alexander Regueiro (Nov 17 2019 at 01:19):

@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.

view this post on Zulip Alexander Regueiro (Nov 17 2019 at 01:23):

@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?

view this post on Zulip Alexander Regueiro (Nov 17 2019 at 01:24):

I believe what you were suggesting (while it obviously works) requires a change to the "supertrait counting" algorithm for finding method offsets, anyway

view this post on Zulip Alexander Regueiro (Nov 17 2019 at 01:24):

well, let me know your thoughts :-)

view this post on Zulip nikomatsakis (Nov 18 2019 at 20:45):

@Alexander Regueiro I don't understand your suggestion

view this post on Zulip nikomatsakis (Nov 18 2019 at 20:45):

If you have Baz, Bar2, Bar1, Foo

view this post on Zulip nikomatsakis (Nov 18 2019 at 20:45):

and you try to upcast to Bar1, you're all set (sort of)

view this post on Zulip nikomatsakis (Nov 18 2019 at 20:45):

but if you try to upcast to Bar2, you're not

view this post on Zulip nikomatsakis (Nov 18 2019 at 20:45):

that is, the Bar2 vtable expects Bar2, Foo

view this post on Zulip nikomatsakis (Nov 18 2019 at 20:46):

and the Bar1 vtable expects Bar1, Foo

view this post on Zulip nikomatsakis (Nov 18 2019 at 20:46):

therefore, there is no way to have both a Bar1 and a Bar2 vtable embedded without two copies of Foo

view this post on Zulip Alexander Regueiro (Nov 18 2019 at 21:09):

@nikomatsakis but that's the thing... does the Bar1 vtable need to expect Bar1, Foo? I thought it could just expect Bar1, X, Foo if we accommodate for that?

view this post on Zulip Alexander Regueiro (Nov 19 2019 at 01:34):

@nikomatsakis eh, no, I'm an idiot... that isn't possible because the method offsets all have to be fixed at compile-time, and be the same for all types implementing a given trait, right?

view this post on Zulip Alexander Regueiro (Nov 19 2019 at 01:39):

@nikomatsakis anyway, I think the solution here is to have something like the supertraits query (that performs a DFS), but allow visiting the same trait twice, at different points in the search. the question is: is infinite recursion still possible? I don't know in precisely what cases it occurs... is it just with auto traits? cf. src/librustc/traits/util.rs:135

view this post on Zulip Alexander Regueiro (Nov 19 2019 at 19:23):

@nikomatsakis anyway if that now makes sense to you, let me know how you think we should implement it, and I'll try to get on it ASAP

view this post on Zulip Alexander Regueiro (Nov 21 2019 at 18:12):

@nikomatsakis just checking you saw the above message... no problem if you're too busy to address it today!

view this post on Zulip Alexander Regueiro (Nov 22 2019 at 00:04):

oh, also, if you have any idea about the auto-traits thing, would be great...

view this post on Zulip Alexander Regueiro (Nov 22 2019 at 17:46):

heading out now for the evening, but feel free to leave notes/idea here :-)

view this post on Zulip nikomatsakis (Nov 22 2019 at 20:19):

nikomatsakis anyway, I think the solution here is to have something like the supertraits query (that performs a DFS), but allow visiting the same trait twice, at different points in the search. the question is: is infinite recursion still possible? I don't know in precisely what cases it occurs... is it just with auto traits? cf. src/librustc/traits/util.rs:135

@Alexander Regueiro this sounds correct

view this post on Zulip nikomatsakis (Nov 22 2019 at 20:20):

I don't believe infinite recursion should be possible at that late stage

view this post on Zulip nikomatsakis (Nov 22 2019 at 20:20):

because earlier stages already check that supertraits are a DAG

view this post on Zulip nikomatsakis (Nov 22 2019 at 20:20):

so presumably we need some second "enumerator" that will run -- I think it'd be safest if it only runs during codegen

view this post on Zulip Alexander Regueiro (Nov 23 2019 at 00:47):

@nikomatsakis yep, thoughts were my thoughts too. thanks. I may be able to reuse some of the underlying machinery for the computation of supertraits. I wonder if it's necessary make this new 'enumerator' an actual query though?

view this post on Zulip DPC (Apr 03 2020 at 17:11):

@nikomatsakis @Alexander Regueiro is there any update on this? I'm considering closing https://github.com/rust-lang/rust/pull/60900 right now :slight_smile:

view this post on Zulip nikomatsakis (Apr 03 2020 at 17:17):

@DPC @Alexander Regueiro and I were discussing that the PR needs to be refactored. They mentioned they were going to try, I'm not sure if they've had a chance yet. I think closing the PR is not unreasonable, though, as hopefully there will be a smaller on that does some of the initial work and then follow-up PRs.

view this post on Zulip Charles Lew (Jun 15 2021 at 01:05):

I'm currently reviving this work, since it's a very useful feature, and the implementation doesn't seem very hard.

view this post on Zulip Charles Lew (Jun 15 2021 at 01:07):

I've created and posted the initial two PRs, #86264 (typecheck and feature gate) and #86291 (codegen refactoring as preparation), they're decoupled and can be landed separately.

view this post on Zulip Charles Lew (Jun 15 2021 at 01:10):

@nikomatsakis If you've got some bandwidth would you minding taking a look at #86264 ? It's a small change verifying the "compatibilty" of two principal traits of trait objects during the unsizing operation.

view this post on Zulip nikomatsakis (Jun 15 2021 at 19:02):

@Charles Lew I'm thrilled about that

view this post on Zulip nikomatsakis (Jun 15 2021 at 19:02):

but I think we should put it on the lang team project board

view this post on Zulip nikomatsakis (Jun 15 2021 at 19:02):

how would you feel about creating a project proposal ?

view this post on Zulip nikomatsakis (Jun 15 2021 at 19:02):

I'd second it

view this post on Zulip Charles Lew (Jun 15 2021 at 19:45):

Sure, https://github.com/rust-lang/lang-team/issues/98

view this post on Zulip Charles Lew (Jun 15 2021 at 19:50):

@nikomatsakis

view this post on Zulip nikomatsakis (Jun 15 2021 at 20:00):

@Charles Lew great, thanks!

view this post on Zulip nikomatsakis (Jun 15 2021 at 20:00):

also, can you add a link to the PR that was closed from before, @Charles Lew ?

view this post on Zulip nikomatsakis (Jun 15 2021 at 20:00):

I was a bit surprised by some of the upcasting logic in #86264 -- comparing for equality seems a bit suspicious

view this post on Zulip nikomatsakis (Jun 15 2021 at 20:00):

not wrong

view this post on Zulip Charles Lew (Jun 15 2021 at 20:01):

I added that to the tracking issue before. The tracking issue is linked from the proposal. Do you want a separate copy in the project proposal?

view this post on Zulip Charles Lew (Jun 15 2021 at 20:05):

Now that i think more about it, it is a little suspicious.

view this post on Zulip Charles Lew (Jun 15 2021 at 20:06):

The replaced previous code was just comparing def_id. So maybe there would be cases where the def_id matched but the substs doesn't really match, and maybe that would be unsound?

view this post on Zulip Charles Lew (Jun 15 2021 at 20:07):

The new code i wrote directly compares the trait ref, and i think this might reject legal code somehow...

view this post on Zulip Charles Lew (Jun 15 2021 at 20:38):

nevermind, there's more follow up checks in confirm_builtin_unsize_candidate so these two parts needs to work together.

view this post on Zulip Charles Lew (Jun 15 2021 at 21:01):

In fact, it seems the logic in confirm_builtin_unsize_candidate is mostly all we want. (I don't know why it leaves the part of auto trait checking to assemble_candidates_for_unsizing though)

I guess the proper implementation is just removing the principal trait identical test, leaving that check to At::sup within confirm_builtin_unsize_candidate. (Not sure whether i should also move the auto traits to the confirmation stage)

With this said, the feature gate checking is not easily done within the obligation engine. Maybe i'll duplicate the logic and leave the feature gating part within the assemble_candidates_for_unsizing function.

view this post on Zulip Charles Lew (Jun 19 2021 at 11:46):


view this post on Zulip Charles Lew (Jun 19 2021 at 11:46):

Update: Codegen refactor pr #86291 was merged and, third PR #86461 is up, which refactors the vtable to use the new scheme.

view this post on Zulip oli (Jun 19 2021 at 11:57):

ideally we'd first deduplicate codegen and miri vtable generation by always going through miri

view this post on Zulip oli (Jun 19 2021 at 11:57):

then we'd only have a single site. we've been wanting to do that since a while, and I think recently someone was working on it, but I can't remember details

view this post on Zulip Charles Lew (Jun 19 2021 at 11:58):

That's fine. I have to understand the details anyway. Maybe i'll start with the miri part.

view this post on Zulip Charles Lew (Jun 19 2021 at 12:00):

For the feature of trait_upcasting I think there're two things left: One is typecheck changes, previously a pr is up but there's something wrong with it so i think it'll get it right with @nikomatsakis 's help when GAT/TAIT work is not so busy.

view this post on Zulip Charles Lew (Jun 19 2021 at 12:00):

The other thing will be actually changing the pointer conversion behavior.

view this post on Zulip Charles Lew (Jun 19 2021 at 12:01):

Basically it will be changing the fat pointer => fat pointer conversion.

view this post on Zulip Charles Lew (Jun 19 2021 at 12:01):

Previously @alexreg 's PR doesn't cover this part at all, only a few changes to llvm codegen. But i guess i'll start from miri side. @RalfJ

view this post on Zulip RalfJ (Jun 19 2021 at 12:51):

Yes? :)

view this post on Zulip Charles Lew (Jun 19 2021 at 13:42):

I'm totally unfamiliar with miri code =) So i guess it'll take a few days before i start asking questions. Basically about pointer-cast unsizing.

view this post on Zulip Charles Lew (Jun 19 2021 at 13:44):

Before that i want to ask a small question about previous comments from bjorn3 in this issue: https://github.com/rust-lang/rust/issues/86324

view this post on Zulip Charles Lew (Jun 19 2021 at 13:46):

The comment was about

create an Allocation outside of the context of a Machine

view this post on Zulip Charles Lew (Jun 19 2021 at 13:46):

I'm curious why is this possible... Since vtable includes pointers to functions and other vtables...

view this post on Zulip Charles Lew (Jun 19 2021 at 13:47):

Also, is there some introduction materials like rustc-dev-guide but about miri? @RalfJ

view this post on Zulip bjorn3 (Jun 19 2021 at 16:27):

Pointers to functions are represented by AllocId's that point to a GlobalAlloc::Function. For vtable pointers it would be backed by GlobalAlloc::Memory. AllocId's can be allocated in both a Machine or tcx.alloc_map. Currently miri's vtable generation allocates them in a Machine. The goal is to allocate them in tcx.alloc_map instead using tcx.create_memory_alloc.

view this post on Zulip bjorn3 (Jun 19 2021 at 16:27):

@Charles Lew

view this post on Zulip Charles Lew (Jun 19 2021 at 16:29):

So is it something that "records" all write operations to a piece of "memory allocation"?

view this post on Zulip bjorn3 (Jun 19 2021 at 16:33):

No, you would make a full Allocation and then call tcx.create_memory_alloc(tcx.intern_allocation(alloc)) to get an AllocId to the now immutable allocation..

view this post on Zulip Charles Lew (Jun 19 2021 at 16:47):

Sounds good, let me try to see if i get it right.

view this post on Zulip Charles Lew (Jun 19 2021 at 17:20):

@bjorn3 I made some progress. Though i still haven't get how to convert AllocId to Scalar ...

view this post on Zulip Charles Lew (Jun 19 2021 at 17:22):

Oh, i think i figured it out... there's a Pointer intermediate type.

view this post on Zulip RalfJ (Jun 19 2021 at 17:26):

Charles Lew said:

Also, is there some introduction materials like rustc-dev-guide but about miri? RalfJ

well, there is https://rustc-dev-guide.rust-lang.org/miri.html. it should even be mostly up-to-date...

view this post on Zulip RalfJ (Jun 19 2021 at 17:29):

I am also a bit confused about what you are trying to achieve. it seems there are two things:

view this post on Zulip Charles Lew (Jun 20 2021 at 03:52):

Thanks!

view this post on Zulip Charles Lew (Jul 01 2021 at 03:13):

Status update:

These two preparation PRs has landed:

view this post on Zulip Charles Lew (Jul 01 2021 at 03:14):

Third PR:

view this post on Zulip Charles Lew (Jul 02 2021 at 03:23):

@bjorn3 about #86461 , do you want to review it, or do you want me to pass it to @WG-traits for reviewing?

view this post on Zulip bjorn3 (Jul 02 2021 at 05:41):

I did a quick review. I would like to pass the rest of the review to @WG-traits as I am not familiar enough with this area of the compiler.

view this post on Zulip Charles Lew (Jul 21 2021 at 12:33):

office hour slot in 1hr :) @nikomatsakis would you mind using this Zulip topic?

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:32):

no problem!

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:32):

I'm here

view this post on Zulip Charles Lew (Jul 21 2021 at 13:32):

thanks!

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:32):

Did you want to talk about #86461 ?

view this post on Zulip Charles Lew (Jul 21 2021 at 13:32):

yes!

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:32):

I've read it over once

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:32):

Let me look back over it

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:33):

oh, this testing infra is beautful

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:33):

did you have specific questions or things you wanted to discuss?

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:33):

otherwise I can read and poke you with questions :) I see @eddyb left a comment

view this post on Zulip Charles Lew (Jul 21 2021 at 13:34):

i think you can go ahead and read it first.

view this post on Zulip Charles Lew (Jul 21 2021 at 13:35):

I still want to ask about whether it's ok to use ParamEnv::reveal_all() here... I think since the types are "concrete" enough, maybe it's ok?

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:37):

ok so

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:38):

the logic of the vtable layout makes sense, very cool

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:38):

the testing makes sense

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:38):

I would love to see each of your examples as a test, or at least this monster one

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:38):

    // For a more complex inheritance relationship like this:
    //   O --> G --> C --> A
    //     \     \     \-> B
    //     |     |-> F --> D
    //     |           \-> E
    //     |-> N --> J --> H
    //           \     \-> I
    //           |-> M --> K
    //                 \-> L
    // The resulting vtable will consists of these segments:
    //  DSA, A, B, B-vptr, C, D, D-vptr, E, E-vptr, F, F-vptr, G,
    //  H, H-vptr, I, I-vptr, J, J-vptr, K, K-vptr, L, L-vptr, M, M-vptr,
    //  N, N-vptr, O

view this post on Zulip Charles Lew (Jul 21 2021 at 13:38):

yes, i can add it :)

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:38):

I didn't read the loop and definition in detail

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:39):

tbh I think that having the test would give me more confidence in its correctness than me reading it :)

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:39):

Charles Lew said:

I still want to ask about whether it's ok to use ParamEnv::reveal_all() here... I think since the types are "concrete" enough, maybe it's ok?

where precisely do you mean

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:39):

one thing that is missing from the "monster" test

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:39):

is any chain of single inheritance

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:40):

I think a good test might be

view this post on Zulip Charles Lew (Jul 21 2021 at 13:40):

https://github.com/rust-lang/rust/pull/86461/files#diff-64ba6669d816a72d0c001062233739d967f0cc66938c6e934f4d947ceea33337R65
This one^

And
https://github.com/rust-lang/rust/pull/86461/files#diff-b81e352311722abf2e93dff8b5af345f3a434adf9c7c068b17de9c815ec114d7R688

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:40):

A -> B -> C
|
D -> E -> F
     \---> G --> H

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:40):

something like that

view this post on Zulip Charles Lew (Jul 21 2021 at 13:40):

Sure, i'll add this as another test.

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:41):

because then we can see that, indeed, the B and C are adjacent, and you have an "isa" relationship between A, B, and C

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:41):

and between D, E, F, etc

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:41):

I guess maybe that's still observable, I just thought the monster test never resulted in any output like A B C where there was an is-a relationship between them

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:41):

anyway

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:41):

as to reveal-all:

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:42):

those lnks dont' work for me :(

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:42):

but I will try searching :)

view this post on Zulip Charles Lew (Jul 21 2021 at 13:42):

Oops

view this post on Zulip Charles Lew (Jul 21 2021 at 13:42):

yeah, they're truncated somehow

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:42):

I presume that this vtable construction logic runs only in the very back-end of the compiler?

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:42):

codegen?

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:42):

(what we used to call trans...)

view this post on Zulip Charles Lew (Jul 21 2021 at 13:43):

yeah, indeed.

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:43):

then reveal-all is fine

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:43):

that is an appropriate time to reveal all, basically

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:43):

all the uses I saw looked ok

view this post on Zulip Charles Lew (Jul 21 2021 at 13:44):

If CTFE use it in a later time, is reveal all still ok?

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:45):

I think so, I'm pondering

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:46):

how/when would CTFE use it?

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:46):

to model dyn Trait values?

view this post on Zulip Charles Lew (Jul 21 2021 at 13:46):

yeah

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:46):

it's an interesting question but it must be ok

view this post on Zulip Charles Lew (Jul 21 2021 at 13:46):

ok, cool

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:46):

presumably specializations down the line etc canot change the methods that get called

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:46):

for dyn values that are created in this crate

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:47):

or else something went wrong in our design :)

view this post on Zulip Charles Lew (Jul 21 2021 at 13:47):

thanks :)

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:47):

do you believe this PR is ready to land?

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:47):

(apart from maybe a test or two being added)

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:47):

that is, it looks like you already addressed @eddyb's comment

view this post on Zulip Charles Lew (Jul 21 2021 at 13:47):

I think so, it doesn't change program behavior.

view this post on Zulip Charles Lew (Jul 21 2021 at 13:49):

The vtable will grow a little for program that uses complex trait inheritance, but that won't hurt i guess :)

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:51):

ok. I'm going to leave a comment suggesting a few tests but with r=me

view this post on Zulip Charles Lew (Jul 21 2021 at 13:53):

thanks!

view this post on Zulip nikomatsakis (Jul 21 2021 at 13:55):

I'm very excited you're picking this up

view this post on Zulip Charles Lew (Jul 21 2021 at 13:57):

I'm also taking it as a chance to learn more about the traits system. I hope i can get to understand the trait system implementation better when you're back from vacation :) I'll book another office hour at that time.

view this post on Zulip nikomatsakis (Jul 22 2021 at 14:01):

Hey @Charles Lew, @Alexa VanHattum was asking me the following and I thought you would be better positioned to answer:

It looks like it's still the case that the index idx passed at call site from InstanceDef::Virtual(def_id, idx) will always correspond to the index into the slice returned by fn vtable_entries , since the segments are built in there, right? Otherwise, would it make sense for VtblEntry::Method to include that index as well?

view this post on Zulip Charles Lew (Jul 22 2021 at 14:51):

@Alexa VanHattum @nikomatsakis Yes, the idx corresponds exactly to the index into vtable_entries slice.

view this post on Zulip Charles Lew (Jul 22 2021 at 14:55):

-

view this post on Zulip Alexa VanHattum (Jul 22 2021 at 15:31):

@Charles Lew great, thank you! And thanks for the extensive comments in your PR!

view this post on Zulip Charles Lew (Jul 22 2021 at 15:32):

np =)

view this post on Zulip Charles Lew (Jul 25 2021 at 14:38):

@bjorn3 @RalfJ Hello, i'm working on trait upcasting coercion's pointer cast handling. I'm a little unsure about what the best approach would be. Would you mind give me a few advices?

view this post on Zulip Charles Lew (Jul 25 2021 at 14:40):

Now i'm reading code related to PointerCast::Unsize

view this post on Zulip Charles Lew (Jul 25 2021 at 14:41):

Its comment says this:

    /// Unsize a pointer/reference value, e.g., `&[T; n]` to
    /// `&[T]`. Note that the source could be a thin or fat pointer.
    /// This will do things like convert thin pointers to fat
    /// pointers, or convert structs containing thin pointers to
    /// structs containing fat pointers, or convert between fat
    /// pointers. We don't store the details of how the transform is
    /// done (in fact, we don't know that, because it might depend on
    /// the precise type parameters). We just store the target
    /// type. Codegen backends and miri figure out what has to be done
    /// based on the precise source/target type at hand.

view this post on Zulip Charles Lew (Jul 25 2021 at 14:42):

So basically it's asking this be implemented separatedly in each of codegen/miri .

view this post on Zulip bjorn3 (Jul 25 2021 at 15:51):

For cg_clif this would be changing https://github.com/bjorn3/rustc_codegen_cranelift/blob/356360836e128e1d1eb11caf6ff5186efb211960/src/unsize.rs#L28-L33 For cg_ssa this would be changing https://github.com/rust-lang/rust/blob/6489ee10410f7be70dbefad322d1a3e1533ab282/compiler/rustc_codegen_ssa/src/base.rs#L144-L147. For miri this would be https://github.com/rust-lang/rust/blob/6489ee10410f7be70dbefad322d1a3e1533ab282/compiler/rustc_mir/src/interpret/cast.rs#L273-L277

view this post on Zulip RalfJ (Jul 25 2021 at 16:43):

oh funny, I did not know about this comment :D

view this post on Zulip RalfJ (Jul 25 2021 at 16:44):

but yeah, new casts need to be implement in each codegen backend separately, and miri is like a codegen backend here

view this post on Zulip RalfJ (Jul 25 2021 at 16:44):

to make sure I understand, this is about casting &dyn Trait1 to &dyn Trait2 when Trait2 is a supertrait of Trait1?

view this post on Zulip Charles Lew (Jul 25 2021 at 16:50):

Yes, indeed!

view this post on Zulip Charles Lew (Jul 25 2021 at 17:04):

@RalfJ I believe for * dyn Trait1 to *dyn Trait2 coercion, an unsafe block will be needed. I'm not yet sure how to implement this yet, nor am i very sure about the exact language rules should be...

view this post on Zulip Charles Lew (Jul 25 2021 at 17:05):

@bjorn3 I'll start with cg_clif then. it's 1am now here, so let me quickly scratch out a simple version... I think i've got a few questions to ask, basically about emitting code that reads memory.

view this post on Zulip RalfJ (Jul 25 2021 at 17:16):

Charles Lew said:

RalfJ I believe for * dyn Trait1 to *dyn Trait2 coercion, an unsafe block will be needed. I'm not yet sure how to implement this yet, nor am i very sure about the exact language rules should be...

why that?
how exactly is &dyn Trait1 to &dyn Trait2 implemented?

view this post on Zulip RalfJ (Jul 25 2021 at 17:17):

but if you are unsure it is probably best to make it unsafe. the unsafety checker is here: https://github.com/rust-lang/rust/blob/master/compiler/rustc_mir/src/transform/check_unsafety.rs

view this post on Zulip Charles Lew (Jul 25 2021 at 17:21):

@RalfJ I haven't write it down yet, there're two code paths, for the simpler case, the original pointer scalar pair is not changed at all.
For the second, more complex case, it will reach out to the vtable through the metadata scalar in the scalar pair, access its one slot at a fixed offset calculated out using Trait1 and Trait2 type info, and use that as the new metadata value.

view this post on Zulip Charles Lew (Jul 25 2021 at 17:23):

so for &dyn Trait1 this operation will always succeed. While for *dyn Trait1 this has to depend on the initial validity of the pointer itself.

view this post on Zulip Charles Lew (Jul 25 2021 at 18:01):

@bjorn3 I think this is almost the version for cg_clif ... modulo necessary type conversions. Is there something i need to pay extra attention to?

            let old_info =
                old_info.expect("unsized_info: missing old info for trait upcasting coercion");
            if data_a.principal_def_id() == data_b.principal_def_id() {
                old_info
            } else {
                // trait upcasting coercion
                let principal_a = data_a
                    .principal()
                    .expect("unsized_info: missing principal trait for trait upcasting coercion");
                let principal_b = data_b
                    .principal()
                    .expect("unsized_info: missing principal trait for trait upcasting coercion");

                let vptr_entry_idx = fx.tcx.vtable_trait_upcasting_slot_idx(
                    source,
                    principal_a,
                    principal_b.def_id(),
                );

                if let Some(entry_idx) = vptr_entry_idx {
                    let entry_offset = entry_idx * fx.pointer_type.bytes();
                    let vptr_ptr = Pointer::new(old_info).offset(entry_offset).load(
                        fx,
                        fx.pointer_type,
                        MemFlags::new(),
                    );
                    vptr_ptr
                } else {
                    old_info
                }
            }

view this post on Zulip bjorn3 (Jul 25 2021 at 18:01):

You can use MemFlags::trusted() instead of MemFlags::new(). That makes it UB for it to trap or be unaligned.

view this post on Zulip Charles Lew (Jul 25 2021 at 18:02):

ok

view this post on Zulip bjorn3 (Jul 25 2021 at 18:02):

Apart from that I think it looks fine.

view this post on Zulip Charles Lew (Jul 25 2021 at 18:04):

Thanks! 2am now. Need to get some sleep... I'll try to write the miri one tomorrow evening when i'm back from work =)

view this post on Zulip RalfJ (Jul 25 2021 at 18:21):

For the second, more complex case, it will reach out to the vtable through the metadata scalar in the scalar pair, access its one slot at a fixed offset calculated out using Trait1 and Trait2 type info, and use that as the new metadata value.

okay, so basically vtables "link to" the vtables of supertraits, forming a kind of linked list? makes sense.

view this post on Zulip RalfJ (Jul 25 2021 at 18:22):

this will be interesting input for the discussion around whether raw pointers have to have valid vtables

view this post on Zulip RalfJ (Jul 25 2021 at 18:22):

so far we say yes they do but also want to remain future compaible with changing our mind -- so yeah that cast needs to be unsafe

view this post on Zulip Charles Lew (Jul 27 2021 at 03:44):

Progress updated: Implemented all three "unsized_info" functions. But seems needs more change, for example, llvm version:
https://github.com/rust-lang/rust/blob/master/compiler/rustc_codegen_ssa/src/base.rs#L221

view this post on Zulip Charles Lew (Jul 27 2021 at 03:46):

I'm thinking about expanding the duty of "unsize_thin_ptr"...

view this post on Zulip Charles Lew (Jul 27 2021 at 03:50):

But i'm a little confused about https://github.com/rust-lang/rust/blob/master/compiler/rustc_codegen_ssa/src/mir/rvalue.rs#L221
which seems to be another duplication ...

view this post on Zulip Charles Lew (Jul 27 2021 at 04:01):

Is it ok to unify unsize_thin_ptr into unsize_ptr which supports both thin and fat source ptr? cc @eddyb @bjorn3

view this post on Zulip bjorn3 (Jul 27 2021 at 06:28):

I think that would be fine. cg_clif needs the same change I think.

view this post on Zulip Charles Lew (Jul 29 2021 at 16:58):

office hour slot in 1hr :) let's continue to use this topic to discuss #86264

view this post on Zulip Charles Lew (Jul 29 2021 at 16:58):

@nikomatsakis

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:03):

wave

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:03):

@Charles Lew

view this post on Zulip Charles Lew (Jul 29 2021 at 18:03):

hi~

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:04):

I'm skimming PR but

view this post on Zulip Charles Lew (Jul 29 2021 at 18:04):

So today we'll move on to the trait-system related part of trait upcasting coercion work.

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:04):

did you have specific questions you wanted to discuss

view this post on Zulip Charles Lew (Jul 29 2021 at 18:04):

Yes!

view this post on Zulip Charles Lew (Jul 29 2021 at 18:05):

For this time, i think maybe we'll have one decision to made....

view this post on Zulip Charles Lew (Jul 29 2021 at 18:05):

It's about * dyn Trait raw pointer upcasting

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:05):

ok

view this post on Zulip Charles Lew (Jul 29 2021 at 18:06):

i think we'll choose between 1. make it an unsafe operation 2. make it impossible to Unsize

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:06):

I saw some earlier discussion

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:06):

the key point is that we haven't yet decided whether a *dyn Trait is required to have a valid vtable?

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:06):

I think that making it unsafe is a reasoable idea

view this post on Zulip Charles Lew (Jul 29 2021 at 18:06):

Yes, indeed!

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:06):

I'm not sure what the connection is to Unsize though

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:07):

can you elaborate on that?

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:07):

that trait is a bit out of cache for me

view this post on Zulip Charles Lew (Jul 29 2021 at 18:08):

It's a symbolic representation of all unsizing impl. Through the real implementations may not exist.

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:08):

I don't follow that either :)

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:09):

in particular it sounded like you were saying that if we make it unsafe to upcast a *dyn Foo

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:09):

then it is possible to Unsize?

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:09):

(but otherwise, it is not?)

view this post on Zulip Charles Lew (Jul 29 2021 at 18:09):

I think so, but i may have got it wrong ...

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:10):

can you say a bit more about the connection between those two

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:10):

reading over your PR, the main thing I am a bit uncomfortable with is the way that we walk through the supertraits list, I'm tying to decide how we should know where to stop

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:11):

just checking if the trait def-ids match doesn't seem right

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:11):

I know that earlier you had == and I complained about that, too

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:11):

I'm imagining a case like trait Foo: Bar<u32> + Bar<i32>

view this post on Zulip Charles Lew (Jul 29 2021 at 18:11):

Oh!

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:13):

I'm looking to see what fns exist

view this post on Zulip Charles Lew (Jul 29 2021 at 18:14):

Yes, i think i'll need some mentoring here.

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:14):

I'm thinking about it

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:15):

we might have a type with inference variables where it's actually ambiguous

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:15):

e.g. Box<dyn Bar<_>>

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:15):

er

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:15):

I guess it's more about the target type

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:15):

Box<dyn Foo> as Box<dyn Bar<_>>

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:16):

we probably want something like the "can unify" test

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:17):

though I don't love that

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:17):

anyway, let's leave it for now, I'll leave a review

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:17):

I'll have to dig into it a bit

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:17):

regarding safety, I think that the upcast should be unsafe

view this post on Zulip Charles Lew (Jul 29 2021 at 18:18):

for raw pointers, yes

view this post on Zulip Charles Lew (Jul 29 2021 at 18:19):

I haven't read the unsafety checking code in the compiler yet. I'll dig into it next week or so. I want to put off the unsafety checking part into next PR.

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:21):

that sounds reasonable

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:21):

is there a tracking issue for this where we can note it so it's not forgotten, though?

view this post on Zulip Charles Lew (Jul 29 2021 at 18:22):

Yes, i've wrote it in the tracking issue: #65991

view this post on Zulip Charles Lew (Jul 29 2021 at 18:22):

In the implementation history part

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:22):

ok, good

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:23):

cool. I've got some time tomorrow that I hope to use for reviewing

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:24):

so I'll look at the question of how to pick from the list then but

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:24):

one thing we might do that's very, very simple :P

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:24):

only allow the upcast if the trait appears exactly once for now

view this post on Zulip Charles Lew (Jul 29 2021 at 18:24):

sounds good, i'll update the pr

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:24):

not really what we want but it won't do the wrong thing

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:25):

if you do that, we should add another bullet to address it in a better way later

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:25):

I think ti'd be good though to kcik this can down the road a bit

view this post on Zulip Charles Lew (Jul 29 2021 at 18:25):

i wonder if it is possible to make the above case into "multiple candidates", so each get confirmed separately

view this post on Zulip Charles Lew (Jul 29 2021 at 18:25):

though i don't really know how to do that... for now

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:25):

interesting thought

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:26):

I have to review the code a bit

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:26):

that might make sense

view this post on Zulip Charles Lew (Jul 29 2021 at 18:26):

anyway, i'll use the simple strategy for now. i'll update the pr and the tracking issue.

view this post on Zulip nikomatsakis (Jul 29 2021 at 18:29):

thanks @Charles Lew !

view this post on Zulip Charles Lew (Jul 29 2021 at 18:29):

thank you for your time !

view this post on Zulip Charles Lew (Jul 29 2021 at 19:27):

@nikomatsakis PR and tracking issue updated following your advice here!

view this post on Zulip nikomatsakis (Jul 30 2021 at 12:49):

@Charles Lew I read the PR -- it looks good. I'm thinking that it would be nice to come up with a "canonical set of tests" for some of the corner cases.

view this post on Zulip Charles Lew (Jul 30 2021 at 12:54):

Yes, at least i plan to include those tests introduced by @alexreg in https://github.com/rust-lang/rust/pull/60900/commits/9549fbd6255fb9f7e4ce1a8859fae7335465e014

view this post on Zulip Charles Lew (Jul 30 2021 at 12:54):

Most of those are run-pass though... So they can't pass without the codegen changes.

view this post on Zulip Charles Lew (Jul 30 2021 at 12:59):

Should i include them as check-pass tests in the first pr?

view this post on Zulip Charles Lew (Jul 30 2021 at 13:02):

@nikomatsakis

view this post on Zulip nikomatsakis (Jul 30 2021 at 13:02):

@Charles Lew I added a coment with a hackmd of tests to add

view this post on Zulip nikomatsakis (Jul 30 2021 at 13:03):

and delegated r+ to you, on the assumption they behave as I expected :)

view this post on Zulip nikomatsakis (Jul 30 2021 at 13:03):

they are poking at the interactions of type/region checking

view this post on Zulip Charles Lew (Jul 30 2021 at 13:03):

ok, thanks!

view this post on Zulip nikomatsakis (Jul 30 2021 at 13:03):

the hackmd

view this post on Zulip Charles Lew (Jul 30 2021 at 13:06):

For the "// OK, eventually" cases, i should make them into FIXMEs in this first pr, is this correct?

view this post on Zulip nikomatsakis (Jul 30 2021 at 13:14):

@Charles Lew yep

view this post on Zulip Charles Lew (Jul 30 2021 at 14:09):

@nikomatsakis Hello, there's one test that's failing from the hackmd. After reading it, i wonder if this really should be ambiguous error or not:

trait Foo: Bar<i32> { }
trait Bar<T> { fn bar(&self) -> Option<T> { None } }

fn test_infer_arg(x: &dyn Foo) {
    let a = x as &dyn Bar<_>; // Ambiguous
    let _ = a.bar();
}

view this post on Zulip Charles Lew (Jul 30 2021 at 14:15):

this is in the second test group within the hackmd.

view this post on Zulip Charles Lew (Jul 30 2021 at 16:57):

After some thinking, i changed the definition of Foo to:
trait Foo<T>: Bar<i32> + Bar<T> {}

Now this test group looks like this:
https://github.com/rust-lang/rust/blob/d60ae35deaaee047ef186c6538bf06f198cb7cbe/src/test/ui/traits/trait-upcasting/type-checking-test-2.rs

I hope this matches your original idea :)

view this post on Zulip Charles Lew (Jul 31 2021 at 15:33):

Hooray, #86264 has landed and we can continue with #87515 now. I've updated the PR and addressed the previous comments. Please give it another look when you've got time. Thanks! @bjorn3 @RalfJ

view this post on Zulip Charles Lew (Aug 19 2021 at 14:43):

Office hour in ~3.5hr. Today's topic will be reviewing the trait upcasting coercion part 3 PR (5th in total). If there's time left maybe we can also discuss a little about the relationship between raw pointer validity and unsizing coercion.

view this post on Zulip Charles Lew (Aug 19 2021 at 17:46):

Link to that PR: https://github.com/rust-lang/rust/pull/88135

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:00):

Hey @Charles Lew !

view this post on Zulip Charles Lew (Aug 19 2021 at 18:01):

hi!

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:02):

So, I should take a look at the PR?

view this post on Zulip Charles Lew (Aug 19 2021 at 18:02):

yeah

view this post on Zulip Charles Lew (Aug 19 2021 at 18:02):

Basically i've got the "OK eventually" parts of the tests fixed.

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:02):

commit by commit?

view this post on Zulip Charles Lew (Aug 19 2021 at 18:03):

both are ok.

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:04):

the approach is quite clever

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:04):

(multiple candidates)

view this post on Zulip Charles Lew (Aug 19 2021 at 18:05):

yeah, i think this basically matches the design expectation of the trait solving engine

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:12):

OK, I read the first commit

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:13):

it is roughly what I expected it to be, but very nice

view this post on Zulip Charles Lew (Aug 19 2021 at 18:13):

thanks

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:15):

reading the second commit now

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:16):

not quite sure what's going on here :)

view this post on Zulip Charles Lew (Aug 19 2021 at 18:18):

For codegen backends, they need the information about which candidate is was selected.

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:18):

it seems lke it's just shuffling things around a bit

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:18):

and I'm kind of inclined to assume you know what you're doing

view this post on Zulip Charles Lew (Aug 19 2021 at 18:19):

I think i know the process, but i'm less sure whether the usages of SelectionContext and other APIs are correctly used.

view this post on Zulip Charles Lew (Aug 19 2021 at 18:23):

So i invoked the trait engine again and extracted the information from ImplSource

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:23):

ok, I think I get it now

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:23):

but I have to look I guess at super::super::prepare_vtable_segments in a touch more detail

view this post on Zulip Charles Lew (Aug 19 2021 at 18:23):

That's the same procedure that built the vtable.

view this post on Zulip Charles Lew (Aug 19 2021 at 18:24):

It's taking different callbacks to decide what it's doing. Here's it's just counting slots.

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:24):

ImplSourceTraitUpcastingData

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:24):

that name is kind of surprising

view this post on Zulip Charles Lew (Aug 19 2021 at 18:24):

I'm not good at naming things...

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:25):

oh, well,

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:25):

reviewing the enum I don't think it's your fault

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:25):

it's more that the impl is surprisingly named

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:28):

I dn't really see any problems

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:28):

the PR looks quite nice :tada:

view this post on Zulip Charles Lew (Aug 19 2021 at 18:28):

thanks

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:29):

I guess we'll see what happens when we r+ :P

view this post on Zulip Charles Lew (Aug 19 2021 at 18:29):

:tada:

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:30):

well ok so one question @Charles Lew

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:30):

vtable_trait_upcasting_coercion_new_vptr_slot -- this is invoked only from the backends, I guess

view this post on Zulip Charles Lew (Aug 19 2021 at 18:30):

indeed.

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:31):

I thnk we should add an assertion that needs_infer is not true

view this post on Zulip Charles Lew (Aug 19 2021 at 18:31):

ok, i'll add it.

view this post on Zulip nikomatsakis (Aug 19 2021 at 18:31):

I think apart from that it should be ok

view this post on Zulip Charles Lew (Aug 19 2021 at 18:31):

great, thanks

view this post on Zulip Charles Lew (Aug 19 2021 at 18:40):

Thank you for your time today. I'll book another slot to talk about raw pointer validity i think.

view this post on Zulip Charles Lew (Aug 19 2021 at 18:40):

Basically it's about unsafety checking on

#![feature(unsize)]

use std::marker::Unsize;

fn unsize<A, B>(v: *mut A) -> *mut B
where
    B: ?Sized,
    A: Unsize<B>,
{
    v
}

view this post on Zulip nikomatsakis (Aug 19 2021 at 21:30):

sounds good

view this post on Zulip Charles Lew (Aug 25 2021 at 12:22):

office hour in ~0.5 hr.

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:04):

@Charles Lew :wave:

view this post on Zulip Charles Lew (Aug 25 2021 at 13:05):

hi~

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:05):

Sorry to be a few minutes late!

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:05):

What's on your mind for today

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:05):

raw pointer validity?

view this post on Zulip Charles Lew (Aug 25 2021 at 13:06):

Yeah

view this post on Zulip Charles Lew (Aug 25 2021 at 13:06):

So at this point, most of the trait object coercion support is actually done. (trait Foo: Bar {} => dyn Foo: Unsize<dyn Bar>)

view this post on Zulip Charles Lew (Aug 25 2021 at 13:06):

However, there's one remaining issue related-to raw pointers.
fn foo(v: *const dyn Foo) -> * const dyn Bar { v }

currently compiles. If v is arbitrarily created using
std::ptr::from_raw_parts(an unstable safe API), foo can cause UB when it tries to read the vtable contents(with multiple inheritance).

view this post on Zulip Charles Lew (Aug 25 2021 at 13:07):

which is bad.

view this post on Zulip Charles Lew (Aug 25 2021 at 13:07):

Currently I can see three paths forward, but maybe there're other solutions too.

view this post on Zulip Charles Lew (Aug 25 2021 at 13:07):

  1. Announce that every fat pointer needs to have valid metadata part.
    Needs to switch the std::ptr::from_raw_parts{,_mut} APIs to be unsafe.
    And updates other documentations.

view this post on Zulip Charles Lew (Aug 25 2021 at 13:08):

  1. Make vtables "flat", by removing all pointer indirections in vtables and appending all the data to the tail. This makes upcasting coercion codegen become adding an offset to the metadata scalar, so won't cause real UB. Will waste some more static bytes in multiple inheritance cases than before, might make embedded-dev people unhappy.

view this post on Zulip Charles Lew (Aug 25 2021 at 13:08):

  1. Announce that raw pointer unsizing coercion must happen in unsafe blocks, while other unsizing coercions can happen outside an unsafe block. This is actually a small breaking change. So need a future compat lint to migrate existing users dealing with raw pointers and some more changes to std(POC PR at #88239 explains the details but it's a bit long). A few other MIR-level details become observable by user: whether the compiler thinks it's a unsizing coercion or not.

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:08):

(cc @RalfJ, if you happen to be around)

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:10):

Option 3 is a breaking change because some limited forms of coercion are available now?

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:10):

e.g. *dyn (Foo + Send) to *dyn Foo

view this post on Zulip Charles Lew (Aug 25 2021 at 13:10):

Yes, and array to slice conversion too

view this post on Zulip Charles Lew (Aug 25 2021 at 13:10):

(behind a raw pointer)

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:11):

we could be more discriminating

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:11):

in terms of whether unsafe is required

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:11):

but you're saying that would be a pain

view this post on Zulip Charles Lew (Aug 25 2021 at 13:12):

There's this snippet

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:12):

something about option 2 feels surprsing to me

view this post on Zulip Charles Lew (Aug 25 2021 at 13:12):

#![feature(unsize)]

use std::marker::Unsize;

fn unsize<A, B>(v: *mut A) -> *mut B
where
    B: ?Sized,
    A: Unsize<B>,
{
    v
}

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:12):

ok, yes

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:12):

that makes sense

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:12):

"generic code" --> always the problem :)

view this post on Zulip Charles Lew (Aug 25 2021 at 13:12):

Indeed!

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:12):

btw, unrelated:

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:13):

I see this as an active lang team initiative and I'd like to "retcon" it into that structure

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:13):

specifically I think I will create a repository and try to capture some of these design questions and decisions in there

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:13):

I'll add you as an owner

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:13):

anyway, hmm

view this post on Zulip Charles Lew (Aug 25 2021 at 13:13):

I'm happy to help.

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:13):

i would definitely say you are "owner" and I am "liaison" i this case

view this post on Zulip Charles Lew (Aug 25 2021 at 13:14):

ok, that's also fine to me.

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:14):

Charles Lew said:

  1. Make vtables "flat", by removing all pointer indirections in vtables and appending all the data to the tail. This makes upcasting coercion codegen become adding an offset to the metadata scalar, so won't cause real UB. Will waste some more static bytes in multiple inheritance cases than before, might make embedded-dev people unhappy.

I'm trying to figure out if this indeed removes all potential for UB -- example

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:14):

is it UB in some sense to to "adjust" the point way outside the bounds of the underlying object?

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:14):

I guess we would have to not use "in-bounds-gep"

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:15):

I know that this discussion has a bit of a long history, I remember talking about it with @Gankra and @Manish Goregaokar and others from time to time

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:15):

so I'm wondering also the "catch" to (1) -- I guess an example of what it means is that you can't have an equivalent of a "null" pointer for a *dyn Foo

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:16):

currently ptr::null<T> requires T: Sized

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:16):

the only way to do it then would be MaybeUnint basically?

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:16):

well, I guess you can use an Option

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:16):

and since we have a niche, that doesn't even add extra space

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:16):

is that right?

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:17):

(side note: I regret that *T can be null)

view this post on Zulip Charles Lew (Aug 25 2021 at 13:17):

I think so.

view this post on Zulip Charles Lew (Aug 25 2021 at 13:17):

Maybe it's also possible to provide some placeholder metadata to signal this is null

view this post on Zulip Charles Lew (Aug 25 2021 at 13:18):

that's just potential possibility. i don't know whether people really want that.

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:18):

I thought about that

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:18):

I think what you could do is say "if the data ptr is null, the metadata ptr is arbitrary"

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:18):

but then you need an if, right?

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:19):

e.g., the upcasting has to be a no-op

view this post on Zulip Charles Lew (Aug 25 2021 at 13:19):

I meant maybe a null_unsized function, when creating a null trait object ptr, assign some meaningful metadata to it.

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:20):

yes-- but what

view this post on Zulip Charles Lew (Aug 25 2021 at 13:20):

Then the metadata is "well-formed" not arbitrary.

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:20):

I guess we could make a metadata that is "strcturally sound"

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:20):

but contains null pointers for each fn

view this post on Zulip Charles Lew (Aug 25 2021 at 13:20):

yeah, that's right.

view this post on Zulip Charles Lew (Aug 25 2021 at 13:21):

just undef for methods slots is also ok, i think

view this post on Zulip Charles Lew (Aug 25 2021 at 13:22):

This reminds me of in C++ there's a You can't call pure virtual function shim in some implementations.

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:23):

I don't hate that

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:23):

I think right now I somewhat lean towards "always valid" metadata

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:23):

and having some way to create "dummy" metadata

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:23):

One of my other regrets are raw pointers is making the set of unsafe operations so narrow (just deref)

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:24):

I didn't truly appreciate how many operations are unsafe in C :)

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:24):

( even though I should've known better )

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:24):

this feels like we should write it up and perhaps have a lang team design meeting proposal or something

view this post on Zulip Charles Lew (Aug 25 2021 at 13:25):

sure

view this post on Zulip Charles Lew (Aug 25 2021 at 13:26):

There's really nothing much else in trait upcasting coercion left now. When this issue is solved, i can mark it no longer an incomplete feature.

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:26):

that's awesome

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:26):

I guess we should invite libs to that discussion too

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:27):

let me go and create that repo real quick

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:27):

I'm going to use the same "basic template" as https://github.com/nikomatsakis/generic-associated-types-initiative/

view this post on Zulip nikomatsakis (Aug 25 2021 at 13:27):

which in turn is based on @XAMPPRocky's excellent https://github.com/rust-lang/project-group-template

view this post on Zulip nikomatsakis (Aug 25 2021 at 16:07):

@Charles Lew I created and updated the repo a bit

view this post on Zulip Charles Lew (Aug 25 2021 at 16:08):

Thanks! I'm still writing summaries for the status quo and recent discussions. I'll try to finish it today.

view this post on Zulip Charles Lew (Aug 25 2021 at 16:09):

@nikomatsakis could you merge these message into https://rust-lang.zulipchat.com/#narrow/stream/144729-wg-traits/topic/dyn.20upcasting.20coercion topic? I was trying to rename that topic and made it a mess ...

view this post on Zulip nikomatsakis (Aug 25 2021 at 16:11):

https://github.com/nikomatsakis/dyn-upcasting-coercion-initiative/blob/master/design-questions/upcast-safety.md

view this post on Zulip nikomatsakis (Aug 25 2021 at 16:11):

oh ok


Last updated: Jan 26 2022 at 07:32 UTC