Stream: t-compiler/rust-2018

Topic: #56032 prelude rebase


view this post on Zulip nikomatsakis (Nov 20 2018 at 21:54):

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

view this post on Zulip nikomatsakis (Nov 20 2018 at 22:00):

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

view this post on Zulip nikomatsakis (Nov 20 2018 at 22:00):

jfyi

view this post on Zulip Vadim Petrochenkov (Nov 21 2018 at 01:02):

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

view this post on Zulip Vadim Petrochenkov (Nov 21 2018 at 10:00):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 10:01):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 10:01):

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

view this post on Zulip eddyb (Nov 21 2018 at 10:52):

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

view this post on Zulip Vadim Petrochenkov (Nov 21 2018 at 10:57):

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.

view this post on Zulip Vadim Petrochenkov (Nov 21 2018 at 10:58):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:32):

@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

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:32):

though probably not @mw =)

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:32):

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

view this post on Zulip Pietro Albini (Nov 21 2018 at 18:34):

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

view this post on Zulip Pietro Albini (Nov 21 2018 at 18:36):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:38):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:38):

but it's code you may understand

view this post on Zulip Pietro Albini (Nov 21 2018 at 18:39):

let me take a look

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:39):

@Pietro Albini see my comment here in particular

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:39):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:39):

but...it seems like maybe yes

view this post on Zulip Pietro Albini (Nov 21 2018 at 18:40):

well, I mean, we shouldn't ICE there

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:42):

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

view this post on Zulip Pietro Albini (Nov 21 2018 at 18:42):

that's not my code actually

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:42):

ah, perhaps it goes further back

view this post on Zulip Pietro Albini (Nov 21 2018 at 18:42):

yep

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:42):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:42):

ok

view this post on Zulip Pietro Albini (Nov 21 2018 at 18:42):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:43):

ok, I thought that might be the reason

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:43):

or something like that, anyway

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:43):

seems like a comment worth keeping

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:43):

I can probably do a rather more complex fix

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:43):

(if I understand what is happening, anyway)

view this post on Zulip Pietro Albini (Nov 21 2018 at 18:44):

btw, I sort of remember what that code does

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:44):

oh hey the comment is there

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:44):

I just overlooked it

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:44):

sigh

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:44):

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

view this post on Zulip Pietro Albini (Nov 21 2018 at 18:45):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:45):

right, I figured out that much

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:46):

so there is this loop here

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:46):

presumably iterating over the a and the b trees

view this post on Zulip Pietro Albini (Nov 21 2018 at 18:46):

yep

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:46):

for each one it recursively lowers the trees

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:47):

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

view this post on Zulip Pietro Albini (Nov 21 2018 at 18:47):

yep

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:48):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:49):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:49):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:50):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:50):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:50):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:50):

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

view this post on Zulip Pietro Albini (Nov 21 2018 at 18:51):

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

view this post on Zulip Pietro Albini (Nov 21 2018 at 18:51):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:52):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:52):

(I had seen that in the git annotate)

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:55):

(btw I'm trying a more complex fix)

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:55):

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

view this post on Zulip QuietMisdreavus (Nov 21 2018 at 18:57):

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

view this post on Zulip QuietMisdreavus (Nov 21 2018 at 18:57):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:58):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 18:59):

but maybe it's ok

view this post on Zulip nikomatsakis (Nov 21 2018 at 19:03):

oh, perhaps I get it now

view this post on Zulip nikomatsakis (Nov 21 2018 at 19:04):

no, no I don't :P

view this post on Zulip nikomatsakis (Nov 21 2018 at 19:05):

/me frustrated

view this post on Zulip nikomatsakis (Nov 21 2018 at 19:27):

@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

view this post on Zulip nikomatsakis (Nov 21 2018 at 19:27):

from what I can tell though rustdoc ignores those

view this post on Zulip nikomatsakis (Nov 21 2018 at 19:28):

e.g., here

view this post on Zulip nikomatsakis (Nov 21 2018 at 19:28):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 19:29):

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

view this post on Zulip QuietMisdreavus (Nov 21 2018 at 19:31):

that seems like a valid reading

view this post on Zulip QuietMisdreavus (Nov 21 2018 at 19:34):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 19:37):

@QuietMisdreavus where would I look for that?

view this post on Zulip QuietMisdreavus (Nov 21 2018 at 19:38):

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

view this post on Zulip QuietMisdreavus (Nov 21 2018 at 19:38):

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

view this post on Zulip nikomatsakis (Nov 21 2018 at 19:38):

this is in the HIR

view this post on Zulip QuietMisdreavus (Nov 21 2018 at 19:38):

so that ListStem should be gone by then

view this post on Zulip QuietMisdreavus (Nov 21 2018 at 19:38):

oh, hm

view this post on Zulip QuietMisdreavus (Nov 21 2018 at 19:39):

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

view this post on Zulip QuietMisdreavus (Nov 21 2018 at 19:40):

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?

view this post on Zulip nikomatsakis (Nov 21 2018 at 19:40):

yeah I can't see any relevant code

view this post on Zulip nikomatsakis (Nov 21 2018 at 19:40):

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


Last updated: Jan 26 2022 at 07:02 UTC