Stream: t-compiler/rust-2018

Topic: #56032 prelude rebase


nikomatsakis (Nov 20 2018 at 21:54, on Zulip):

@Vadim Petrochenkov did you plan to rebase https://github.com/rust-lang/rust/pull/56032 ?

nikomatsakis (Nov 20 2018 at 22:00, on Zulip):

I'm trying to backport it to beta for https://github.com/rust-lang/rust/pull/56102

nikomatsakis (Nov 20 2018 at 22:00, on Zulip):

jfyi

Vadim Petrochenkov (Nov 21 2018 at 01:02, on Zulip):

@nikomatsakis
One more regression fix - https://github.com/rust-lang/rust/pull/56117

Vadim Petrochenkov (Nov 21 2018 at 10:00, on Zulip):

@nikomatsakis
By the way, why is https://github.com/rust-lang/rust/pull/55275 in 2018 release milestone?
It's under feature gate, so it won't be too useful on stable.
Or you are suggesting to insta-stabilize it as well?

nikomatsakis (Nov 21 2018 at 10:01, on Zulip):

good point, I am happy to take it off, I thought @eddyb suggested it was necessary but maybe I misunderstood

nikomatsakis (Nov 21 2018 at 10:01, on Zulip):

tbh I am not fully awake yet :) was planning to take a look more closely later today

eddyb (Nov 21 2018 at 10:52, on Zulip):

I thought it was needed and that it would be stabilized, but maybe not?

Vadim Petrochenkov (Nov 21 2018 at 10:57, on Zulip):

Looks like https://github.com/rust-lang/rust/issues/54647 has a workaround with CARGO_PKG_NAME + it's not mandatory to immediately migrate proc macro crates to 2018, so it's probably okay to delay this.

Vadim Petrochenkov (Nov 21 2018 at 10:58, on Zulip):

(That said, I'm not aware of any technical problems preventing its stabilization.)

nikomatsakis (Nov 21 2018 at 18:32, on Zulip):

@Vadim Petrochenkov if you're around and have any insights into #56128, would be appreciated; I've left some notes here. Also perhaps @mw or @Pietro Albini might have a clue

nikomatsakis (Nov 21 2018 at 18:32, on Zulip):

though probably not @mw =)

nikomatsakis (Nov 21 2018 at 18:32, on Zulip):

I find the node-id management around visibilities a bit tricky

Pietro Albini (Nov 21 2018 at 18:34, on Zulip):

well, I guess we now have the first exception to the beta freeze... cc @simulacrum

Pietro Albini (Nov 21 2018 at 18:36, on Zulip):

oh, that's actually from my use_nested_groups pr a year ago... woops!

nikomatsakis (Nov 21 2018 at 18:38, on Zulip):

that's why I cc'd you :) but I don't think your PR is the cause

nikomatsakis (Nov 21 2018 at 18:38, on Zulip):

but it's code you may understand

Pietro Albini (Nov 21 2018 at 18:39, on Zulip):

let me take a look

nikomatsakis (Nov 21 2018 at 18:39, on Zulip):

@Pietro Albini see my comment here in particular

nikomatsakis (Nov 21 2018 at 18:39, on Zulip):

I'm not sure if we should backport this or not

nikomatsakis (Nov 21 2018 at 18:39, on Zulip):

but...it seems like maybe yes

Pietro Albini (Nov 21 2018 at 18:40, on Zulip):

well, I mean, we shouldn't ICE there

nikomatsakis (Nov 21 2018 at 18:42, on Zulip):

so @Pietro Albini one question is, why did you reset the visibility there in the first place?

Pietro Albini (Nov 21 2018 at 18:42, on Zulip):

that's not my code actually

nikomatsakis (Nov 21 2018 at 18:42, on Zulip):

ah, perhaps it goes further back

Pietro Albini (Nov 21 2018 at 18:42, on Zulip):

yep

nikomatsakis (Nov 21 2018 at 18:42, on Zulip):

I traced it as far as your PR in the git annotate and then stopped

nikomatsakis (Nov 21 2018 at 18:42, on Zulip):

ok

Pietro Albini (Nov 21 2018 at 18:42, on Zulip):

https://github.com/rust-lang/rust/commit/bc096549e84d1edbd051f0b42662c038dd935ff6#diff-64b696b0ef6ad44140e973801ed82b25R718

nikomatsakis (Nov 21 2018 at 18:43, on Zulip):

ok, I thought that might be the reason

nikomatsakis (Nov 21 2018 at 18:43, on Zulip):

or something like that, anyway

nikomatsakis (Nov 21 2018 at 18:43, on Zulip):

seems like a comment worth keeping

nikomatsakis (Nov 21 2018 at 18:43, on Zulip):

I can probably do a rather more complex fix

nikomatsakis (Nov 21 2018 at 18:43, on Zulip):

(if I understand what is happening, anyway)

Pietro Albini (Nov 21 2018 at 18:44, on Zulip):

btw, I sort of remember what that code does

nikomatsakis (Nov 21 2018 at 18:44, on Zulip):

oh hey the comment is there

nikomatsakis (Nov 21 2018 at 18:44, on Zulip):

I just overlooked it

nikomatsakis (Nov 21 2018 at 18:44, on Zulip):

sigh

nikomatsakis (Nov 21 2018 at 18:44, on Zulip):

I was going to say 'why did that comment go away' :P

Pietro Albini (Nov 21 2018 at 18:45, on Zulip):

during lowering use foo::{a, b} gets desugared to use foo::a; use foo::b; use foo::{};, and the last one is hidden away from rustdoc if it's a pub use

nikomatsakis (Nov 21 2018 at 18:45, on Zulip):

right, I figured out that much

nikomatsakis (Nov 21 2018 at 18:46, on Zulip):

so there is this loop here

nikomatsakis (Nov 21 2018 at 18:46, on Zulip):

presumably iterating over the a and the b trees

Pietro Albini (Nov 21 2018 at 18:46, on Zulip):

yep

nikomatsakis (Nov 21 2018 at 18:46, on Zulip):

for each one it recursively lowers the trees

nikomatsakis (Nov 21 2018 at 18:47, on Zulip):

which will wind up in the Single case, at least for my test case

Pietro Albini (Nov 21 2018 at 18:47, on Zulip):

yep

nikomatsakis (Nov 21 2018 at 18:48, on Zulip):

here we iterate over the namespaces, and for each one we create a new use item -- we clone the visibility ids for each such item

nikomatsakis (Nov 21 2018 at 18:49, on Zulip):

oh, I see, but we keep the *first* item to be returned

nikomatsakis (Nov 21 2018 at 18:49, on Zulip):

so that's why the vis id doesn't get cloned for that item

nikomatsakis (Nov 21 2018 at 18:50, on Zulip):

what confuses me a bit is that we are cloning the vis id here for each of the a and b already

nikomatsakis (Nov 21 2018 at 18:50, on Zulip):

but the original vis id seems to never wind up in the tree at all

nikomatsakis (Nov 21 2018 at 18:50, on Zulip):

which is precisely the same situation that is causing an ICE now

nikomatsakis (Nov 21 2018 at 18:50, on Zulip):

I don't..quite know why it was not ICEing before

Pietro Albini (Nov 21 2018 at 18:51, on Zulip):

that's introduced by https://github.com/rust-lang/rust/pull/51425 cc @QuietMisdreavus

Pietro Albini (Nov 21 2018 at 18:51, on Zulip):

https://github.com/rust-lang/rust/commit/122b5b47c2d8387a5095c3cac50fe11c9aaf1369#diff-64b696b0ef6ad44140e973801ed82b25R2413

nikomatsakis (Nov 21 2018 at 18:52, on Zulip):

yes, I realized that as I was writing out those notes

nikomatsakis (Nov 21 2018 at 18:52, on Zulip):

(I had seen that in the git annotate)

nikomatsakis (Nov 21 2018 at 18:55, on Zulip):

(btw I'm trying a more complex fix)

nikomatsakis (Nov 21 2018 at 18:55, on Zulip):

(basically renumber the segment ids for everything but the final item in the list)

QuietMisdreavus (Nov 21 2018 at 18:57, on Zulip):

oof, that pr had like three different designs before finally being accepted

QuietMisdreavus (Nov 21 2018 at 18:57, on Zulip):

and it's been long enough that i don't remember all the discussion that went into this one :sweat_smile:

nikomatsakis (Nov 21 2018 at 18:58, on Zulip):

ok; I'm still confused why it doesn't ICE today

nikomatsakis (Nov 21 2018 at 18:59, on Zulip):

but maybe it's ok

nikomatsakis (Nov 21 2018 at 19:03, on Zulip):

oh, perhaps I get it now

nikomatsakis (Nov 21 2018 at 19:04, on Zulip):

no, no I don't :P

nikomatsakis (Nov 21 2018 at 19:05, on Zulip):

/me frustrated

nikomatsakis (Nov 21 2018 at 19:27, on Zulip):

@QuietMisdreavus one thing you might recall — the motivation for this line is [that we don't want the UseStem pub use to show up in docs

nikomatsakis (Nov 21 2018 at 19:27, on Zulip):

from what I can tell though rustdoc ignores those

nikomatsakis (Nov 21 2018 at 19:28, on Zulip):

e.g., here

nikomatsakis (Nov 21 2018 at 19:28, on Zulip):

I'd like to remove that line so I am hoping this is true

nikomatsakis (Nov 21 2018 at 19:29, on Zulip):

this makes sense to me anyway... there's nothing to document about such an item

QuietMisdreavus (Nov 21 2018 at 19:31, on Zulip):

that seems like a valid reading

QuietMisdreavus (Nov 21 2018 at 19:34, on Zulip):

oh, need to look to see whether it also ignores these when inlining across crates

nikomatsakis (Nov 21 2018 at 19:37, on Zulip):

@QuietMisdreavus where would I look for that?

QuietMisdreavus (Nov 21 2018 at 19:38, on Zulip):

https://github.com/rust-lang/rust/blob/4b3a1d9112f4fb40c0318bac010aba98643584c3/src/librustdoc/clean/inline.rs

QuietMisdreavus (Nov 21 2018 at 19:38, on Zulip):

but i think the concern is moot, since it doesn't read AST, only HIR

nikomatsakis (Nov 21 2018 at 19:38, on Zulip):

this is in the HIR

QuietMisdreavus (Nov 21 2018 at 19:38, on Zulip):

so that ListStem should be gone by then

QuietMisdreavus (Nov 21 2018 at 19:38, on Zulip):

oh, hm

QuietMisdreavus (Nov 21 2018 at 19:39, on Zulip):

https://github.com/rust-lang/rust/blob/4b3a1d9112f4fb40c0318bac010aba98643584c3/src/librustdoc/clean/inline.rs#L397 this is where it starts reading in modules, which calls the top-level try_inline on everything

QuietMisdreavus (Nov 21 2018 at 19:40, on Zulip):

https://github.com/rust-lang/rust/blob/4b3a1d9112f4fb40c0318bac010aba98643584c3/src/librustdoc/clean/inline.rs#L48 but it looks like it doesn't even try to handle imports at all, so it could be fine?

nikomatsakis (Nov 21 2018 at 19:40, on Zulip):

yeah I can't see any relevant code

nikomatsakis (Nov 21 2018 at 19:40, on Zulip):

maybe @Vadim Petrochenkov will have a better idea how to fix that line anyway

Last update: Nov 15 2019 at 11:20UTC