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

Issue 56128 segment id ice nightly #56143

Merged
merged 10 commits into from
Nov 22, 2018
Merged

Issue 56128 segment id ice nightly #56143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d0a174d
track the span for each id so that we can give a nice ICE
nikomatsakis Nov 21, 2018
40f8094
add some `debug!` into lowering
nikomatsakis Nov 21, 2018
a0a4790
renumber segment ids for visibilities whenever we clone them
nikomatsakis Nov 21, 2018
4687eeb
preserve the original visibility for the "list stem" node
nikomatsakis Nov 21, 2018
2bd2fc9
add regression test
nikomatsakis Nov 21, 2018
4c7ce7c
pass vis by shared reference
nikomatsakis Nov 21, 2018
9cdf491
hack: ignore list-stems for pub lint
nikomatsakis Nov 21, 2018
ebf3c8d
add compile-pass annotation
nikomatsakis Nov 21, 2018
b83150e
only reset non-restricted visibilities
nikomatsakis Nov 22, 2018
5f2a173
explain how this works
nikomatsakis Nov 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 69 additions & 12 deletions src/librustc/hir/lowering.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1866,6 +1866,10 @@ impl<'a> LoweringContext<'a> {
} else {
self.lower_node_id(segment.id)
};
debug!(
"lower_path_segment: ident={:?} original-id={:?} new-id={:?}",
segment.ident, segment.id, id,
);

hir::PathSegment::new(
segment.ident,
Expand Down Expand Up @@ -2955,6 +2959,9 @@ impl<'a> LoweringContext<'a> {
name: &mut Name,
attrs: &hir::HirVec<Attribute>,
) -> hir::ItemKind {
debug!("lower_use_tree(tree={:?})", tree);
debug!("lower_use_tree: vis = {:?}", vis);

let path = &tree.prefix;
let segments = prefix
.segments
Expand Down Expand Up @@ -3022,12 +3029,7 @@ impl<'a> LoweringContext<'a> {
hir::VisibilityKind::Inherited => hir::VisibilityKind::Inherited,
hir::VisibilityKind::Restricted { ref path, id: _, hir_id: _ } => {
let id = this.next_id();
let mut path = path.clone();
for seg in path.segments.iter_mut() {
if seg.id.is_some() {
seg.id = Some(this.next_id().node_id);
}
}
let path = this.renumber_segment_ids(path);
hir::VisibilityKind::Restricted {
path,
id: id.node_id,
Expand Down Expand Up @@ -3068,7 +3070,29 @@ impl<'a> LoweringContext<'a> {
hir::ItemKind::Use(path, hir::UseKind::Glob)
}
UseTreeKind::Nested(ref trees) => {
// Nested imports are desugared into simple imports.
// Nested imports are desugared into simple
// imports. So if we start with
//
// ```
// pub(x) use foo::{a, b};
// ```
//
// we will create three items:
//
// ```
// pub(x) use foo::a;
// pub(x) use foo::b;
// pub(x) use foo::{}; // <-- this is called the `ListStem`
// ```
//
// The first two are produced by recursively invoking
// `lower_use_tree` (and indeed there may be things
// like `use foo::{a::{b, c}}` and so forth). They
// wind up being directly added to
// `self.items`. However, the structure of this
// function also requires us to return one item, and
// for that we return the `{}` import (called the
// "`ListStem`").

let prefix = Path {
segments,
Expand Down Expand Up @@ -3112,8 +3136,9 @@ impl<'a> LoweringContext<'a> {
hir::VisibilityKind::Inherited => hir::VisibilityKind::Inherited,
hir::VisibilityKind::Restricted { ref path, id: _, hir_id: _ } => {
let id = this.next_id();
let path = this.renumber_segment_ids(path);
hir::VisibilityKind::Restricted {
path: path.clone(),
path: path,
id: id.node_id,
hir_id: id.hir_id,
}
Expand All @@ -3136,17 +3161,48 @@ impl<'a> LoweringContext<'a> {
});
}

// Privatize the degenerate import base, used only to check
// the stability of `use a::{};`, to avoid it showing up as
// a re-export by accident when `pub`, e.g. in documentation.
// Subtle and a bit hacky: we lower the privacy level
// of the list stem to "private" most of the time, but
// not for "restricted" paths. The key thing is that
// we don't want it to stay as `pub` (with no caveats)
// because that affects rustdoc and also the lints
// about `pub` items. But we can't *always* make it
// private -- particularly not for restricted paths --
// because it contains node-ids that would then be
// unused, failing the check that HirIds are "densely
// assigned".
match vis.node {
hir::VisibilityKind::Public |
hir::VisibilityKind::Crate(_) |
hir::VisibilityKind::Inherited => {
*vis = respan(prefix.span.shrink_to_lo(), hir::VisibilityKind::Inherited);
}
hir::VisibilityKind::Restricted { .. } => {
// do nothing here, as described in the comment on the match
}
}

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.

hir::ItemKind::Use(path, hir::UseKind::ListStem)
}
}
}

/// Paths like the visibility path in `pub(super) use foo::{bar, baz}` are repeated
/// many times in the HIR tree; for each occurrence, we need to assign distinct
/// node-ids. (See e.g. #56128.)
fn renumber_segment_ids(&mut self, path: &P<hir::Path>) -> P<hir::Path> {
debug!("renumber_segment_ids(path = {:?})", path);
let mut path = path.clone();
for seg in path.segments.iter_mut() {
if seg.id.is_some() {
seg.id = Some(self.next_id().node_id);
}
}
path
}

fn lower_trait_item(&mut self, i: &TraitItem) -> hir::TraitItem {
let LoweredNodeId { node_id, hir_id } = self.lower_node_id(i.id);
let trait_item_def_id = self.resolver.definitions().local_def_id(node_id);
Expand Down Expand Up @@ -4540,6 +4596,7 @@ impl<'a> LoweringContext<'a> {
VisibilityKind::Public => hir::VisibilityKind::Public,
VisibilityKind::Crate(sugar) => hir::VisibilityKind::Crate(sugar),
VisibilityKind::Restricted { ref path, id } => {
debug!("lower_visibility: restricted path id = {:?}", id);
let lowered_id = if let Some(owner) = explicit_owner {
self.lower_node_id_with_owner(id, owner)
} else {
Expand Down
64 changes: 37 additions & 27 deletions src/librustc/hir/map/collector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ use rustc_data_structures::stable_hasher::{HashStable, StableHasher, StableHashe
pub(super) struct NodeCollector<'a, 'hir> {
/// The crate
krate: &'hir Crate,

/// Source map
source_map: &'a SourceMap,

/// The node map
map: Vec<Option<Entry<'hir>>>,
/// The parent of this node
Expand All @@ -54,7 +58,8 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {
pub(super) fn root(krate: &'hir Crate,
dep_graph: &'a DepGraph,
definitions: &'a definitions::Definitions,
hcx: StableHashingContext<'a>)
hcx: StableHashingContext<'a>,
source_map: &'a SourceMap)
-> NodeCollector<'a, 'hir> {
let root_mod_def_path_hash = definitions.def_path_hash(CRATE_DEF_INDEX);

Expand Down Expand Up @@ -102,6 +107,7 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {

let mut collector = NodeCollector {
krate,
source_map,
map: vec![],
parent_node: CRATE_NODE_ID,
current_signature_dep_index: root_mod_sig_dep_index,
Expand All @@ -125,7 +131,6 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {
pub(super) fn finalize_and_compute_crate_hash(mut self,
crate_disambiguator: CrateDisambiguator,
cstore: &dyn CrateStore,
source_map: &SourceMap,
commandline_args_hash: u64)
-> (Vec<Option<Entry<'hir>>>, Svh)
{
Expand Down Expand Up @@ -154,7 +159,8 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {
// If we included the full mapping in the SVH, we could only have
// reproducible builds by compiling from the same directory. So we just
// hash the result of the mapping instead of the mapping itself.
let mut source_file_names: Vec<_> = source_map
let mut source_file_names: Vec<_> = self
.source_map
.files()
.iter()
.filter(|source_file| CrateNum::from_u32(source_file.crate_of_origin) == LOCAL_CRATE)
Expand Down Expand Up @@ -186,7 +192,7 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {
self.map[id.as_usize()] = Some(entry);
}

fn insert(&mut self, id: NodeId, node: Node<'hir>) {
fn insert(&mut self, span: Span, id: NodeId, node: Node<'hir>) {
let entry = Entry {
parent: self.parent_node,
dep_node: if self.currently_in_body {
Expand Down Expand Up @@ -216,16 +222,20 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {
String::new()
};

bug!("inconsistent DepNode for `{}`: \
current_dep_node_owner={} ({:?}), hir_id.owner={} ({:?}){}",
span_bug!(
span,
"inconsistent DepNode at `{:?}` for `{}`: \
current_dep_node_owner={} ({:?}), hir_id.owner={} ({:?}){}",
self.source_map.span_to_string(span),
node_str,
self.definitions
.def_path(self.current_dep_node_owner)
.to_string_no_crate(),
self.current_dep_node_owner,
self.definitions.def_path(hir_id.owner).to_string_no_crate(),
hir_id.owner,
forgot_str)
forgot_str,
)
}
}

Expand Down Expand Up @@ -309,12 +319,12 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
debug_assert_eq!(i.hir_id.owner,
self.definitions.opt_def_index(i.id).unwrap());
self.with_dep_node_owner(i.hir_id.owner, i, |this| {
this.insert(i.id, Node::Item(i));
this.insert(i.span, i.id, Node::Item(i));
this.with_parent(i.id, |this| {
if let ItemKind::Struct(ref struct_def, _) = i.node {
// If this is a tuple-like struct, register the constructor.
if !struct_def.is_struct() {
this.insert(struct_def.id(), Node::StructCtor(struct_def));
this.insert(i.span, struct_def.id(), Node::StructCtor(struct_def));
}
}
intravisit::walk_item(this, i);
Expand All @@ -323,23 +333,23 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
}

fn visit_foreign_item(&mut self, foreign_item: &'hir ForeignItem) {
self.insert(foreign_item.id, Node::ForeignItem(foreign_item));
self.insert(foreign_item.span, foreign_item.id, Node::ForeignItem(foreign_item));

self.with_parent(foreign_item.id, |this| {
intravisit::walk_foreign_item(this, foreign_item);
});
}

fn visit_generic_param(&mut self, param: &'hir GenericParam) {
self.insert(param.id, Node::GenericParam(param));
self.insert(param.span, param.id, Node::GenericParam(param));
intravisit::walk_generic_param(self, param);
}

fn visit_trait_item(&mut self, ti: &'hir TraitItem) {
debug_assert_eq!(ti.hir_id.owner,
self.definitions.opt_def_index(ti.id).unwrap());
self.with_dep_node_owner(ti.hir_id.owner, ti, |this| {
this.insert(ti.id, Node::TraitItem(ti));
this.insert(ti.span, ti.id, Node::TraitItem(ti));

this.with_parent(ti.id, |this| {
intravisit::walk_trait_item(this, ti);
Expand All @@ -351,7 +361,7 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
debug_assert_eq!(ii.hir_id.owner,
self.definitions.opt_def_index(ii.id).unwrap());
self.with_dep_node_owner(ii.hir_id.owner, ii, |this| {
this.insert(ii.id, Node::ImplItem(ii));
this.insert(ii.span, ii.id, Node::ImplItem(ii));

this.with_parent(ii.id, |this| {
intravisit::walk_impl_item(this, ii);
Expand All @@ -365,23 +375,23 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
} else {
Node::Pat(pat)
};
self.insert(pat.id, node);
self.insert(pat.span, pat.id, node);

self.with_parent(pat.id, |this| {
intravisit::walk_pat(this, pat);
});
}

fn visit_anon_const(&mut self, constant: &'hir AnonConst) {
self.insert(constant.id, Node::AnonConst(constant));
self.insert(DUMMY_SP, constant.id, Node::AnonConst(constant));

self.with_parent(constant.id, |this| {
intravisit::walk_anon_const(this, constant);
});
}

fn visit_expr(&mut self, expr: &'hir Expr) {
self.insert(expr.id, Node::Expr(expr));
self.insert(expr.span, expr.id, Node::Expr(expr));

self.with_parent(expr.id, |this| {
intravisit::walk_expr(this, expr);
Expand All @@ -390,7 +400,7 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {

fn visit_stmt(&mut self, stmt: &'hir Stmt) {
let id = stmt.node.id();
self.insert(id, Node::Stmt(stmt));
self.insert(stmt.span, id, Node::Stmt(stmt));

self.with_parent(id, |this| {
intravisit::walk_stmt(this, stmt);
Expand All @@ -399,21 +409,21 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {

fn visit_path_segment(&mut self, path_span: Span, path_segment: &'hir PathSegment) {
if let Some(id) = path_segment.id {
self.insert(id, Node::PathSegment(path_segment));
self.insert(path_span, id, Node::PathSegment(path_segment));
}
intravisit::walk_path_segment(self, path_span, path_segment);
}

fn visit_ty(&mut self, ty: &'hir Ty) {
self.insert(ty.id, Node::Ty(ty));
self.insert(ty.span, ty.id, Node::Ty(ty));

self.with_parent(ty.id, |this| {
intravisit::walk_ty(this, ty);
});
}

fn visit_trait_ref(&mut self, tr: &'hir TraitRef) {
self.insert(tr.ref_id, Node::TraitRef(tr));
self.insert(tr.path.span, tr.ref_id, Node::TraitRef(tr));

self.with_parent(tr.ref_id, |this| {
intravisit::walk_trait_ref(this, tr);
Expand All @@ -427,21 +437,21 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
}

fn visit_block(&mut self, block: &'hir Block) {
self.insert(block.id, Node::Block(block));
self.insert(block.span, block.id, Node::Block(block));
self.with_parent(block.id, |this| {
intravisit::walk_block(this, block);
});
}

fn visit_local(&mut self, l: &'hir Local) {
self.insert(l.id, Node::Local(l));
self.insert(l.span, l.id, Node::Local(l));
self.with_parent(l.id, |this| {
intravisit::walk_local(this, l)
})
}

fn visit_lifetime(&mut self, lifetime: &'hir Lifetime) {
self.insert(lifetime.id, Node::Lifetime(lifetime));
self.insert(lifetime.span, lifetime.id, Node::Lifetime(lifetime));
}

fn visit_vis(&mut self, visibility: &'hir Visibility) {
Expand All @@ -450,7 +460,7 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
VisibilityKind::Crate(_) |
VisibilityKind::Inherited => {}
VisibilityKind::Restricted { id, .. } => {
self.insert(id, Node::Visibility(visibility));
self.insert(visibility.span, id, Node::Visibility(visibility));
self.with_parent(id, |this| {
intravisit::walk_vis(this, visibility);
});
Expand All @@ -462,20 +472,20 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
let def_index = self.definitions.opt_def_index(macro_def.id).unwrap();

self.with_dep_node_owner(def_index, macro_def, |this| {
this.insert(macro_def.id, Node::MacroDef(macro_def));
this.insert(macro_def.span, macro_def.id, Node::MacroDef(macro_def));
});
}

fn visit_variant(&mut self, v: &'hir Variant, g: &'hir Generics, item_id: NodeId) {
let id = v.node.data.id();
self.insert(id, Node::Variant(v));
self.insert(v.span, id, Node::Variant(v));
self.with_parent(id, |this| {
intravisit::walk_variant(this, v, g, item_id);
});
}

fn visit_struct_field(&mut self, field: &'hir StructField) {
self.insert(field.id, Node::Field(field));
self.insert(field.span, field.id, Node::Field(field));
self.with_parent(field.id, |this| {
intravisit::walk_struct_field(this, field);
});
Expand Down
Loading