Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue 56128 segment id ice nightly #56143

Merged
merged 10 commits into from
Nov 22, 2018
Merged

Issue 56128 segment id ice nightly #56143

merged 10 commits into from
Nov 22, 2018

Conversation

nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Nov 21, 2018 •
edited
Loading

Tentative fix for #56128

From what I can tell, the problem is that if you have pub(super) use foo::{a, b}, then when we explode the a and b, the segment ids from the super path were not getting cloned. However, once I fixed that, then I ran into a problem that the "visibility" node-ids were not present in the final HIR -- this is because the visibility of the "stem" that is returned in this case was getting reset to inherited. I don't think it is a problem to undo that, so that the visibility is returned unmodified.

Fixes #55475
Fixes #56128

cc @nrc @petrochenkov

@rust-highfive
Copy link
Collaborator

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 21, 2018
@nikomatsakis
Copy link
Contributor Author

r? @petrochenkov

@rust-highfive rust-highfive assigned petrochenkov and unassigned varkor Nov 21, 2018
This was referenced Nov 21, 2018
Closed
Closed
@alexcrichton alexcrichton added this to the Rust 2018 Release milestone Nov 21, 2018
@alexcrichton alexcrichton added beta-nominated Nominated for backporting to the compiler in the beta channel. beta-accepted Accepted for backporting to the compiler in the beta channel. labels Nov 21, 2018
@rust-highfive

This comment has been minimized.

Sign in to view
nrc
nrc reviewed Nov 21, 2018
View reviewed changes
Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to update the submodules?

LGTM otherwise

@nikomatsakis
Copy link
Contributor Author

Er, no, no I did not.

@nikomatsakis
preserve the original visibility for the "list stem" node
4687eeb 
Without this, the `vis` does not wind up in the tree anywhere, and
then we get ICEs because the node-ids it refers to are not present.
The motivation seemed to be documentation, but `ListStem` HIR nodes
are ignored in rustdoc, from what I can tell.
@nikomatsakis
add regression test
2bd2fc9
@nikomatsakis
pass vis by shared reference
4c7ce7c 
We are not mutating it now.
@nikomatsakis nikomatsakis force-pushed the issue-56128-segment-id-ice-nightly branch from 3440713 to 4c7ce7c Compare November 21, 2018 20:34
@nikomatsakis
Copy link
Contributor Author

I guess though that this is fallout from the visibility change:

[00:48:29] ---- [ui] ui/lint/unreachable_pub-pub_crate.rs stdout ----
[00:48:29] diff of stderr:
[00:48:29] 
[00:48:29] 14    = help: or consider exporting it for use by other crates
[00:48:29] 15 
[00:48:29] 16 warning: unreachable `pub` item
[00:48:29] +   --> $DIR/unreachable_pub-pub_crate.rs:26:5
[00:48:29] +    |
[00:48:29] + LL |     pub use std::env::{Args}; // braced-use has different item spans than unbraced
[00:48:29] +    |     ---^^^^^^^^^^^^^^^
[00:48:29] +    |     |
[00:48:29] +    |     help: consider restricting its visibility: `pub(crate)`
[00:48:29] +    |
[00:48:29] +    = help: or consider exporting it for use by other crates
[00:48:29] + 
[00:48:29] + warning: unreachable `pub` item
[00:48:29] 18    |
[00:48:29] 18    |
[00:48:29] 19 LL |     pub use std::env::{Args}; // braced-use has different item spans than unbraced

@nikomatsakis
Copy link
Contributor Author

I can make that code ignore ListStem but..ugh.

@nikomatsakis
Copy link
Contributor Author

I tried an alternate fix (use the vis for the last item) but it ice'd because the owner was wrong; more notes from discord here.

I don't have a better idea than what's in this PR -- I leave it to @nrc and @petrochenkov to decide best course of action =)

@alexcrichton
Copy link
Member

@bors: p=1

we're gonna want to backport this

@rust-highfive

This comment has been minimized.

Sign in to view
petrochenkov
petrochenkov reviewed Nov 22, 2018
View reviewed changes
src/librustc/hir/lowering.rs
let def = self.expect_full_def_from_use(id).next().unwrap_or(Def::Err);
let path = P(self.lower_path_extra(def, &prefix, ParamMode::Explicit, None));
*vis = respan(prefix.span.shrink_to_lo(), hir::VisibilityKind::Inherited);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, you still need to reset at least pub (hir::VisibilityKind::Public) into something more private, otherwise you'll get a lot of noise from rustdoc (the stem is emitted for all braced imports).

This way the hack for UnreachablePub can be avoided as well.

This should be ok since hir::VisibilityKind::Public doesn't have a path with node IDs inside it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything except for VisibilityKind::Restricted can be reset to VisibilityKind::Inherited basically.

@petrochenkov
Copy link
Contributor

petrochenkov commented Nov 22, 2018 •
edited
Loading

Beside #56143 (comment) everything looks reasonable.

(I never fully understood what are relations between node ids, def ids, their owners, parents, etc though, and why @QuietMisdreavus had to do all those horrible things with them in #51425.)

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 22, 2018

📌 Commit 5f2a173 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 22, 2018
@bors
Copy link
Contributor

bors commented Nov 22, 2018

⌛ Testing commit 5f2a173 with merge 00e03ee...

bors added a commit that referenced this pull request Nov 22, 2018
@bors
Auto merge of #56143 - nikomatsakis:issue-56128-segment-id-ice-nightl…
00e03ee 
…y, r=petrochenkov

Issue 56128 segment id ice nightly

Tentative fix for #56128

From what I can tell, the problem is that if you have `pub(super) use foo::{a, b}`, then when we explode the `a` and `b`, the segment ids from the `super` path were not getting cloned. However, once I fixed *that*, then I ran into a problem that the "visibility" node-ids were not present in the final HIR -- this is because the visibility of the "stem" that is returned in this case was getting reset to inherited. I don't *think* it is a problem to undo that, so that the visibility is returned unmodified.

Fixes #55475
Fixes #56128

cc @nrc @petrochenkov
@pietroalbini pietroalbini mentioned this pull request Nov 22, 2018
Merged
@bors
Copy link
Contributor

bors commented Nov 22, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 00e03ee to master...

@bors bors merged commit 5f2a173 into rust-lang:master Nov 22, 2018
@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 23, 2018
bors added a commit that referenced this pull request Nov 23, 2018
@bors
Auto merge of #56174 - pietroalbini:beta-rollup, r=alexcrichton
1c16fa4 
[beta] Rollup backports

* #56163: [master] Backport 1.30.1 release notes
* #56147: resolve: Fix some asserts in import validation
* #56118: Update books for Rust 2018
* #56117: resolve: Make "empty import canaries" invisible from other crates
* #56065: Replace the ICEing on const fn loops with an error
* #56143: Issue 56128 segment id ice nightly
* #56134: Fix clippy documentation links (first in #56156)

r? @ghost
@DJMcNab DJMcNab mentioned this pull request Dec 9, 2018
Closed
@ljedrz ljedrz mentioned this pull request Feb 14, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

@nrc nrc nrc left review comments

Assignees

@petrochenkov petrochenkov

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
Rust 2018 Release
7 participants
@nikomatsakis @rust-highfive @alexcrichton @petrochenkov @bors @nrc @varkor