Auto merge of #56143 - nikomatsakis:issue-56128-segment-id-ice-nightly, 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

4 files changed, 123 insertions, 41 deletions

diff --git a/src/librustc/hir/lowering.rs b/src/librustc/hir/lowering.rs
index 7ac3b033437..b3ba2968c9f 100644
--- a/src/librustc/hir/lowering.rs
+++ b/src/librustc/hir/lowering.rs
@@ -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,
@@ -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
@@ -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,
@@ -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,
@@ -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,
                                 }
@@ -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);
                 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);
@@ -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 {
diff --git a/src/librustc/hir/map/collector.rs b/src/librustc/hir/map/collector.rs
index 4fbcd83adb5..2917fd7457a 100644
--- a/src/librustc/hir/map/collector.rs
+++ b/src/librustc/hir/map/collector.rs
@@ -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
@@ -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);
 
@@ -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,
@@ -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)
     {
@@ -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)
@@ -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 {
@@ -216,8 +222,11 @@ 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)
@@ -225,7 +234,8 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {
                     self.current_dep_node_owner,
                     self.definitions.def_path(hir_id.owner).to_string_no_crate(),
                     hir_id.owner,
-                    forgot_str)
+                    forgot_str,
+                )
             }
         }
 
@@ -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);
@@ -323,7 +333,7 @@ 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);
@@ -331,7 +341,7 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
     }
 
     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);
     }
 
@@ -339,7 +349,7 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
         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);
@@ -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);
@@ -365,7 +375,7 @@ 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);
@@ -373,7 +383,7 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
     }
 
     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);
@@ -381,7 +391,7 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
     }
 
     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);
@@ -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);
@@ -399,13 +409,13 @@ 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);
@@ -413,7 +423,7 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
     }
 
     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);
@@ -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) {
@@ -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);
                 });
@@ -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);
         });
diff --git a/src/librustc/hir/map/mod.rs b/src/librustc/hir/map/mod.rs
index cf7a7abf95a..ef777abfbc4 100644
--- a/src/librustc/hir/map/mod.rs
+++ b/src/librustc/hir/map/mod.rs
@@ -1032,14 +1032,14 @@ pub fn map_crate<'hir>(sess: &::session::Session,
         let mut collector = NodeCollector::root(&forest.krate,
                                                 &forest.dep_graph,
                                                 &definitions,
-                                                hcx);
+                                                hcx,
+                                                sess.source_map());
         intravisit::walk_crate(&mut collector, &forest.krate);
 
         let crate_disambiguator = sess.local_crate_disambiguator();
         let cmdline_args = sess.opts.dep_tracking_hash();
         collector.finalize_and_compute_crate_hash(crate_disambiguator,
                                                   cstore,
-                                                  sess.source_map(),
                                                   cmdline_args)
     };
 
diff --git a/src/test/ui/issues/issue-56128.rs b/src/test/ui/issues/issue-56128.rs
new file mode 100644
index 00000000000..3a3eccdc33c
--- /dev/null
+++ b/src/test/ui/issues/issue-56128.rs
@@ -0,0 +1,15 @@
+// Regression test for #56128. When this `pub(super) use...` gets
+// exploded in the HIR, we were not handling ids correctly.
+//
+// compile-pass
+
+mod bar {
+    pub(super) use self::baz::{x, y};
+
+    mod baz {
+        pub fn x() { }
+        pub fn y() { }
+    }
+}
+
+fn main() { }