Stream: t-compiler

Topic: trait object crate


nikomatsakis (Dec 20 2018 at 15:02, on Zulip):

@Alexander Regueiro how many crates depend on that traitobject crate?

nikomatsakis (Dec 20 2018 at 15:03, on Zulip):

I wonder if we can find some other solution

Alexander Regueiro (Dec 20 2018 at 15:06, on Zulip):

I was thinking the same

Alexander Regueiro (Dec 20 2018 at 15:06, on Zulip):

let me see...

nikomatsakis (Dec 20 2018 at 15:06, on Zulip):

one extreme possibility might be finding some way to accept the specific thing the traitobject crate does

nikomatsakis (Dec 20 2018 at 15:06, on Zulip):

(e.g., with some form of warning)

nikomatsakis (Dec 20 2018 at 15:06, on Zulip):

at least for a time

nikomatsakis (Dec 20 2018 at 15:07, on Zulip):

I have to go refresh my memory though of what it is doing

Alexander Regueiro (Dec 20 2018 at 15:08, on Zulip):

wait, where did the crater run go?

Alexander Regueiro (Dec 20 2018 at 15:08, on Zulip):

https://github.com/rust-lang/rust/pull/55994

Alexander Regueiro (Dec 20 2018 at 15:08, on Zulip):

I thought it owuld be there

Pietro Albini (Dec 20 2018 at 15:09, on Zulip):

it's in the hidden comments at the middle

Alexander Regueiro (Dec 20 2018 at 15:10, on Zulip):

I'm kind of annoyed to be honest that this guy has carelessly put in silly incoherent impls and then isn't replying to any attempts to fix his crate.

Alexander Regueiro (Dec 20 2018 at 15:10, on Zulip):

@Pietro Albini right, thanks.

Alexander Regueiro (Dec 20 2018 at 15:16, on Zulip):

@Pietro Albini could we get some sort of transitive dependency analysis on crater runs in the future, per chance? :-) like, crater suggesting that large subsets of failures may be due to including a certain crate.

Pietro Albini (Dec 20 2018 at 15:16, on Zulip):

at the moment you can use https://github.com/pietroalbini/crater-tree to do that

Pietro Albini (Dec 20 2018 at 15:16, on Zulip):

it's a really big hack, but it sort of works

Alexander Regueiro (Dec 20 2018 at 15:17, on Zulip):

aha

Pietro Albini (Dec 20 2018 at 15:18, on Zulip):

sometimes that also outputs the right data!

Alexander Regueiro (Dec 20 2018 at 15:20, on Zulip):

anyway, will give it a run, thanks.

Alexander Regueiro (Dec 20 2018 at 15:26, on Zulip):

holy... this is going to take forever.

Pietro Albini (Dec 20 2018 at 15:27, on Zulip):

creating dummy cargo projects for every regressed crate was the easiest way to get the dependency graphs shrugs

Pietro Albini (Dec 20 2018 at 15:28, on Zulip):

some refactorings I'm doing right now should allow crater itself to get this information though

Alexander Regueiro (Dec 20 2018 at 15:28, on Zulip):

@Pietro Albini yeah, I don't doubt it heh. eventually I guess you'll build dependency graph generation into crater itself, but that's a lot more work. ;-)

Alexander Regueiro (Dec 20 2018 at 15:29, on Zulip):

^

Pietro Albini (Dec 20 2018 at 15:29, on Zulip):

I think the easiest solution is to just get the name of the crate that actually failed from the build logs

Pietro Albini (Dec 20 2018 at 15:30, on Zulip):

and soon crater will be able to do that

Alexander Regueiro (Dec 20 2018 at 15:30, on Zulip):

ah

Alexander Regueiro (Dec 20 2018 at 15:30, on Zulip):

very good

Pietro Albini (Dec 20 2018 at 15:30, on Zulip):

that should also allow to categorize regressions by error code

Pietro Albini (Dec 20 2018 at 15:30, on Zulip):

and to detect regressions in crates that fail to build

Alexander Regueiro (Dec 20 2018 at 15:31, on Zulip):

yes

Alexander Regueiro (Dec 20 2018 at 15:31, on Zulip):

would be very useful

Alexander Regueiro (Dec 20 2018 at 15:45, on Zulip):

@nikomatsakis here's the output of crater-tree... seems a bit worse than I thought. traitobject is responsible for most failures still (I think?), but there are clearly some other not-so-common crates too.

Alexander Regueiro (Dec 20 2018 at 15:45, on Zulip):

https://gist.github.com/d305248be22d77c97c60dc0b1833dd46

Ariel Ben-Yehuda (Dec 21 2018 at 12:10, on Zulip):

the problem with the traitobject crate is basically

pub unsafe trait Trait {}
unsafe impl Trait for ::std::marker::Send + Sync { }
unsafe impl Trait for ::std::marker::Send + Send + Sync { }

If we can enable overlapping_marker_traits, then the problem will be solved because there will be no problem with the double impl.

Ariel Ben-Yehuda (Dec 21 2018 at 12:12, on Zulip):

We might do a point fix that is equivalent to overlapping_marker_traits but only for principal-less trait objects.

Ariel Ben-Yehuda (Dec 21 2018 at 12:13, on Zulip):
nikomatsakis (Dec 26 2018 at 16:31, on Zulip):

We might do a point fix that is equivalent to overlapping_marker_traits but only for principal-less trait objects.

that is sort of what I had in mind, @Ariel Ben-Yehuda

nikomatsakis (Dec 26 2018 at 16:31, on Zulip):

or at least we might do this with a warning period for that

nikomatsakis (Dec 26 2018 at 16:32, on Zulip):

though I could imagine also just saying "ah screw it, that's ok because back-compat", though it's quite a hack

centril (Dec 26 2018 at 17:14, on Zulip):

I'm OK with a warning period, but we might want to try more and more creative ways to reach @reem and ask them to fix it; if that doesn't work I have a controversial solution: we push a point release ourselves by virtue of asking T-crates-io nicely; what the crate does is sorta UB anyways

centril (Dec 26 2018 at 17:15, on Zulip):

its assuming ABI details about trait objects; not OK... :P

nikomatsakis (Dec 26 2018 at 17:16, on Zulip):

I've wondered about that from time to time

centril (Dec 26 2018 at 17:18, on Zulip):

whatever we do I strongly feel that its a bug (that must be fixed) in the type system not to have dyn A + B + A == dyn A + B or dyn A + B == dyn B + A be true

nikomatsakis (Dec 26 2018 at 17:29, on Zulip):

that seems clear

centril (Dec 26 2018 at 17:32, on Zulip):

@nikomatsakis hmm... reem seems like a fellow someone should know personally; maybe they can contact them outside of the internetz?

nikomatsakis (Dec 26 2018 at 17:54, on Zulip):

maybe

centril (Dec 27 2018 at 23:35, on Zulip):

I've sent an email to @reem to try to speed up the process

Ariel Ben-Yehuda (Jan 03 2019 at 19:56, on Zulip):

I have the fix working locally

Ariel Ben-Yehuda (Jan 03 2019 at 19:56, on Zulip):

will push it later today

nikomatsakis (Jan 03 2019 at 21:19, on Zulip):

@Ariel Ben-Yehuda what fix are you doing exactly?

Ariel Ben-Yehuda (Jan 03 2019 at 21:50, on Zulip):

Just pushed it

Ariel Ben-Yehuda (Jan 03 2019 at 21:51, on Zulip):

https://github.com/rust-lang/rust/pull/56837/commits/ae924b45608a17f859a1576c905c6ac81558e093

Ariel Ben-Yehuda (Jan 03 2019 at 21:52, on Zulip):

if you're still online

nikomatsakis (Jan 04 2019 at 19:14, on Zulip):

@Ariel Ben-Yehuda ah, nice

nikomatsakis (Jan 04 2019 at 19:14, on Zulip):

just read it

Ariel Ben-Yehuda (Jan 04 2019 at 19:22, on Zulip):

@nikomatsakis: Would you want to r+ https://github.com/rust-lang/rust/pull/57335

Ariel Ben-Yehuda (Jan 04 2019 at 19:22, on Zulip):

to fix that ICE

Ariel Ben-Yehuda (Jan 04 2019 at 19:22, on Zulip):

or should we wait for https://github.com/rust-lang/rust/pull/56837 ?

nikomatsakis (Jan 04 2019 at 19:36, on Zulip):

@Ariel Ben-Yehuda what's your opinion?

Ariel Ben-Yehuda (Jan 04 2019 at 19:36, on Zulip):

not sure much about the compat risk

nikomatsakis (Jan 04 2019 at 19:36, on Zulip):

that's what i'm wondering about — is this a beta regression yet?

nikomatsakis (Jan 04 2019 at 19:36, on Zulip):

or is still nightly only

Ariel Ben-Yehuda (Jan 04 2019 at 19:36, on Zulip):

no

Ariel Ben-Yehuda (Jan 04 2019 at 19:36, on Zulip):

still nightly only

nikomatsakis (Jan 04 2019 at 19:36, on Zulip):

ok

Ariel Ben-Yehuda (Jan 04 2019 at 19:37, on Zulip):

but merging 56837 means that "traitobject-like, but not exactly traitobject" crates will go to hard error with no warning cycle

nikomatsakis (Jan 04 2019 at 19:37, on Zulip):

I'm maybe inclined to land #56837 — I guess I have to check but iirc the code felt pretty clean

Ariel Ben-Yehuda (Jan 04 2019 at 19:37, on Zulip):

in that case, let me merge the test

Ariel Ben-Yehuda (Jan 04 2019 at 19:37, on Zulip):

and the log changes

nikomatsakis (Jan 04 2019 at 19:37, on Zulip):

I don't think we've seen such crates in crater runs, though of course they might be outside the scope of crater

Ariel Ben-Yehuda (Jan 04 2019 at 19:37, on Zulip):

sure

nikomatsakis (Jan 04 2019 at 19:37, on Zulip):

I guess it seems like something people are not that likely to abuse to me?

nikomatsakis (Jan 04 2019 at 19:37, on Zulip):

but you never know

Ariel Ben-Yehuda (Jan 04 2019 at 19:37, on Zulip):

I think that's the main question

Ariel Ben-Yehuda (Jan 04 2019 at 19:37, on Zulip):

we didn't think anything like traitobject would exist

nikomatsakis (Jan 04 2019 at 19:38, on Zulip):

indeed :)

nikomatsakis (Jan 04 2019 at 19:38, on Zulip):

I guess the alternative is to land the ICE fix

nikomatsakis (Jan 04 2019 at 19:38, on Zulip):

and do a crater run on #56837 to gather more data?

nikomatsakis (Jan 04 2019 at 19:38, on Zulip):

did we already do a crater run?

Ariel Ben-Yehuda (Jan 04 2019 at 19:38, on Zulip):

yea

nikomatsakis (Jan 04 2019 at 19:38, on Zulip):

I can't remember

nikomatsakis (Jan 04 2019 at 19:38, on Zulip):

ok

nikomatsakis (Jan 04 2019 at 19:38, on Zulip):

so no more data to be had

Ariel Ben-Yehuda (Jan 04 2019 at 19:38, on Zulip):

and it discovered that the only issue was traitobject

Pietro Albini (Jan 04 2019 at 19:38, on Zulip):

please do not start crater run right now

nikomatsakis (Jan 04 2019 at 19:38, on Zulip):

then I feel like we'll never know more than we do now

nikomatsakis (Jan 04 2019 at 19:38, on Zulip):

well I guess the point is they would at least have a warning cycle

Ariel Ben-Yehuda (Jan 04 2019 at 19:39, on Zulip):

why?

nikomatsakis (Jan 04 2019 at 19:39, on Zulip):

how much time until the next beta release?

Ariel Ben-Yehuda (Jan 04 2019 at 19:39, on Zulip):

2 weeks

Ariel Ben-Yehuda (Jan 04 2019 at 19:39, on Zulip):

that's why the "land #57335 to fix ICE, wait 2 weeks, then land #56837" approach is tempting

Pietro Albini (Jan 04 2019 at 19:39, on Zulip):

have to deploy a change, should take a few minutes

Ariel Ben-Yehuda (Jan 04 2019 at 19:39, on Zulip):

it will give us that 6 week warning cycle

nikomatsakis (Jan 04 2019 at 19:39, on Zulip):

if these theoretical crates are testing against nightly, they'll have already started seeing warnings, right?

Ariel Ben-Yehuda (Jan 04 2019 at 19:39, on Zulip):

sure

nikomatsakis (Jan 04 2019 at 19:40, on Zulip):

that's why the "land #57335 to fix ICE, wait 2 weeks, then land #56837" approach is tempting

yeah I mean it is the conservative thing to do

nikomatsakis (Jan 04 2019 at 19:40, on Zulip):

I guess the question is how much stuff is "blocked" on #56837

Ariel Ben-Yehuda (Jan 04 2019 at 19:40, on Zulip):

I would prefer to land @alexreg's PR after #56837

Ariel Ben-Yehuda (Jan 04 2019 at 19:40, on Zulip):

it has some cross-cutting concerns

nikomatsakis (Jan 04 2019 at 19:40, on Zulip):

that's the thing I was thinking about

nikomatsakis (Jan 04 2019 at 19:41, on Zulip):

(cc @Alexander Regueiro )

Ariel Ben-Yehuda (Jan 04 2019 at 19:41, on Zulip):

I'm not sure how critical is landing his PR in the next 2 weeks

nikomatsakis (Jan 04 2019 at 19:41, on Zulip):

well, it's just been open for a long time

Ariel Ben-Yehuda (Jan 04 2019 at 19:41, on Zulip):

sure

nikomatsakis (Jan 04 2019 at 19:41, on Zulip):

I'm not sure it's criticial, I think it's all specific to trait aliases

nikomatsakis (Jan 04 2019 at 19:42, on Zulip):

otoh the question is how likely it is that there is a crater which:

hard to say...

nikomatsakis (Jan 04 2019 at 19:42, on Zulip):

it seems clear that if we landed #56837 we will not "break the ecosystem"

Ariel Ben-Yehuda (Jan 04 2019 at 19:42, on Zulip):

sure

nikomatsakis (Jan 04 2019 at 19:42, on Zulip):

I'm always afraid of something like "we break winapi"

nikomatsakis (Jan 04 2019 at 19:42, on Zulip):

since that is not tested on crater

nikomatsakis (Jan 04 2019 at 19:42, on Zulip):

:)

nikomatsakis (Jan 04 2019 at 19:43, on Zulip):

(I think we test that specific crate now anyway, but you know what I mean)

Ariel Ben-Yehuda (Jan 04 2019 at 19:43, on Zulip):

but we would have knew that

nikomatsakis (Jan 04 2019 at 19:43, on Zulip):

right, hopefully

Ariel Ben-Yehuda (Jan 04 2019 at 19:43, on Zulip):

because winapi runs on windows

Pietro Albini (Jan 04 2019 at 19:43, on Zulip):

(we don't test winapi on crater)

nikomatsakis (Jan 04 2019 at 19:43, on Zulip):

I know we don't test on crater

nikomatsakis (Jan 04 2019 at 19:43, on Zulip):

but I think we test it somewhere else?

nikomatsakis (Jan 04 2019 at 19:43, on Zulip):

(as part of bors?)

nikomatsakis (Jan 04 2019 at 19:43, on Zulip):

or maybe I'm misremembering

nikomatsakis (Jan 04 2019 at 19:43, on Zulip):

we've talked about it

Ariel Ben-Yehuda (Jan 04 2019 at 19:46, on Zulip):

I think we test it in bors

Ariel Ben-Yehuda (Jan 04 2019 at 19:48, on Zulip):

so what we'll do?/

nikomatsakis (Jan 04 2019 at 19:49, on Zulip):

I'm sort of inclined to land #56837

nikomatsakis (Jan 04 2019 at 19:49, on Zulip):

even though I guess prudence would push towards the other path

nikomatsakis (Jan 04 2019 at 19:50, on Zulip):

in particular, it feels like a clear bug fix, and fixing the affected code is pretty easy (kill the second impl)

nikomatsakis (Jan 04 2019 at 19:50, on Zulip):

the main concern is that it'll be some dependency that's hard to fix

nikomatsakis (Jan 04 2019 at 19:50, on Zulip):

and I think I consider that unlikely at this point given the crater results

Ariel Ben-Yehuda (Jan 04 2019 at 19:51, on Zulip):

sure

Ariel Ben-Yehuda (Jan 04 2019 at 19:51, on Zulip):

that sounds like an ok idea

Ariel Ben-Yehuda (Jan 04 2019 at 19:51, on Zulip):

let's go with that? I'll cherry-pick the improvements

nikomatsakis (Jan 04 2019 at 19:52, on Zulip):

ok

nikomatsakis (Jan 04 2019 at 19:52, on Zulip):

:+1:

Ariel Ben-Yehuda (Jan 04 2019 at 19:53, on Zulip):

pushed

nikomatsakis (Jan 04 2019 at 20:47, on Zulip):

@Ariel Ben-Yehuda did you plan to fix https://github.com/rust-lang/rust/issues/56934 in a follow-up PR?

nikomatsakis (Jan 04 2019 at 20:48, on Zulip):

seems like it's worth closing sooner rather than later

Ariel Ben-Yehuda (Jan 04 2019 at 21:03, on Zulip):

yes

Ariel Ben-Yehuda (Jan 04 2019 at 21:03, on Zulip):

I'll do it now

Ariel Ben-Yehuda (Jan 04 2019 at 21:04, on Zulip):

but #56837 is already growing a bit too big

Ariel Ben-Yehuda (Jan 04 2019 at 21:04, on Zulip):

and I'll like to crater that other PR

Alexander Regueiro (Jan 04 2019 at 21:12, on Zulip):

@nikomatsakis sorry, I can't see what you CCed me on. (Zulip UI is terrible, eh...)

Alexander Regueiro (Jan 04 2019 at 21:12, on Zulip):

something about that regression?

nikomatsakis (Jan 04 2019 at 21:12, on Zulip):

@Alexander Regueiro the context is that #56837 will land and it intersects the trait alias PR you've been working on; might be good to rebase that atop #56837

Alexander Regueiro (Jan 04 2019 at 21:13, on Zulip):

@nikomatsakis oh, I guess you didn't see my PM. :-)

Alexander Regueiro (Jan 04 2019 at 21:14, on Zulip):

(from yesterday)

davidtwco (Jan 04 2019 at 21:24, on Zulip):

@nikomatsakis sorry, I can't see what you CCed me on. (Zulip UI is terrible, eh...)

You can click the mentions button in the top left sidebar and it'll show all of your mentions for all of time, if you click on a message that mentions you so it is highlighted then click on the topic name just above it'll take you to the full conversation and the context.

Last update: Nov 22 2019 at 04:40UTC