Fix tests and assertions; add some comments

Author: Nick Cameron

src/librustc/hir/intravisit.rs

@@ -298,6 +298,9 @@ pub trait Visitor<'v> : Sized {
     fn visit_fn(&mut self, fk: FnKind<'v>, fd: &'v FnDecl, b: BodyId, s: Span, id: NodeId) {
         walk_fn(self, fk, fd, b, s, id)
     }
+    fn visit_use(&mut self, path: &'v Path, id: NodeId, hir_id: HirId) {
+        walk_use(self, path, id, hir_id)
+    }
     fn visit_trait_item(&mut self, ti: &'v TraitItem) {
         walk_trait_item(self, ti)
     }
@@ -471,8 +474,7 @@ pub fn walk_item<'v, V: Visitor<'v>>(visitor: &mut V, item: &'v Item) {
             }
         }
         ItemKind::Use(ref path, _) => {
-            visitor.visit_id(item.id);
-            visitor.visit_path(path, item.hir_id);
+            visitor.visit_use(path, item.id, item.hir_id);
         }
         ItemKind::Static(ref typ, _, body) |
         ItemKind::Const(ref typ, body) => {
@@ -554,6 +556,14 @@ pub fn walk_item<'v, V: Visitor<'v>>(visitor: &mut V, item: &'v Item) {
     walk_list!(visitor, visit_attribute, &item.attrs);
 }
 
+pub fn walk_use<'v, V: Visitor<'v>>(visitor: &mut V,
+                                    path: &'v Path,
+                                    item_id: NodeId,
+                                    hir_id: HirId) {
+    visitor.visit_id(item_id);
+    visitor.visit_path(path, hir_id);
+}
+
 pub fn walk_enum_def<'v, V: Visitor<'v>>(visitor: &mut V,
                                          enum_definition: &'v EnumDef,
                                          generics: &'v Generics,
@@ -652,6 +662,9 @@ pub fn walk_path_segment<'v, V: Visitor<'v>>(visitor: &mut V,
                                              path_span: Span,
                                              segment: &'v PathSegment) {
     visitor.visit_ident(segment.ident);
+    if let Some(id) = segment.id {
+        visitor.visit_id(id);
+    }
     if let Some(ref args) = segment.args {
         visitor.visit_generic_args(path_span, args);
     }

src/librustc/hir/lowering.rs

@@ -1069,6 +1069,9 @@ impl<'a> LoweringContext<'a> {
     }
 
     fn lower_attr(&mut self, attr: &Attribute) -> Attribute {
+        // Note that we explicitly do not walk the path. Since we don't really
+        // lower attributes (we use the AST version) there is nowhere to keep
+        // the HirIds. We don't actually need HIR version of attributes anyway.
         Attribute {
             id: attr.id,
             style: attr.style,
@@ -1682,6 +1685,7 @@ impl<'a> LoweringContext<'a> {
                         num_lifetimes,
                         parenthesized_generic_args,
                         itctx.reborrow(),
+                        None,
                     )
                 })
                 .collect(),
@@ -1725,6 +1729,7 @@ impl<'a> LoweringContext<'a> {
                 0,
                 ParenthesizedGenericArgs::Warn,
                 itctx.reborrow(),
+                None,
             ));
             let qpath = hir::QPath::TypeRelative(ty, segment);
 
@@ -1753,6 +1758,7 @@ impl<'a> LoweringContext<'a> {
         p: &Path,
         ident: Option<Ident>,
         param_mode: ParamMode,
+        explicit_owner: Option<NodeId>,
     ) -> hir::Path {
         hir::Path {
             def,
@@ -1766,6 +1772,7 @@ impl<'a> LoweringContext<'a> {
                         0,
                         ParenthesizedGenericArgs::Err,
                         ImplTraitContext::disallowed(),
+                        explicit_owner,
                     )
                 })
                 .chain(ident.map(|ident| hir::PathSegment::from_ident(ident)))
@@ -1776,7 +1783,7 @@ impl<'a> LoweringContext<'a> {
 
     fn lower_path(&mut self, id: NodeId, p: &Path, param_mode: ParamMode) -> hir::Path {
         let def = self.expect_full_def(id);
-        self.lower_path_extra(def, p, None, param_mode)
+        self.lower_path_extra(def, p, None, param_mode, None)
     }
 
     fn lower_path_segment(
@@ -1787,6 +1794,7 @@ impl<'a> LoweringContext<'a> {
         expected_lifetimes: usize,
         parenthesized_generic_args: ParenthesizedGenericArgs,
         itctx: ImplTraitContext<'_>,
+        explicit_owner: Option<NodeId>,
     ) -> hir::PathSegment {
         let (mut generic_args, infer_types) = if let Some(ref generic_args) = segment.args {
             let msg = "parenthesized parameters may only be used with a trait";
@@ -1858,9 +1866,15 @@ impl<'a> LoweringContext<'a> {
         }
 
         let def = self.expect_full_def(segment.id);
+        let id = if let Some(owner) = explicit_owner {
+            self.lower_node_id_with_owner(segment.id, owner)
+        } else {
+            self.lower_node_id(segment.id)
+        };
+
         hir::PathSegment::new(
             segment.ident,
-            Some(segment.id),
+            Some(id.node_id),
             Some(def),
             generic_args,
             infer_types,
@@ -2944,19 +2958,20 @@ impl<'a> LoweringContext<'a> {
         attrs: &hir::HirVec<Attribute>,
     ) -> hir::ItemKind {
         let path = &tree.prefix;
+        let segments = prefix
+            .segments
+            .iter()
+            .chain(path.segments.iter())
+            .cloned()
+            .collect();
 
         match tree.kind {
             UseTreeKind::Simple(rename, id1, id2) => {
                 *name = tree.ident().name;
 
                 // First apply the prefix to the path
                 let mut path = Path {
-                    segments: prefix
-                        .segments
-                        .iter()
-                        .chain(path.segments.iter())
-                        .cloned()
-                        .collect(),
+                    segments,
                     span: path.span,
                 };
 
@@ -2976,9 +2991,18 @@ impl<'a> LoweringContext<'a> {
                 // for later
                 let ret_def = defs.next().unwrap_or(Def::Err);
 
+                // Here, we are looping over namespaces, if they exist for the definition
+                // being imported. We only handle type and value namespaces because we
+                // won't be dealing with macros in the rest of the compiler.
+                // Essentially a single `use` which imports two names is desugared into
+                // two imports.
                 for (def, &new_node_id) in defs.zip([id1, id2].iter()) {
                     let vis = vis.clone();
                     let name = name.clone();
+                    let mut path = path.clone();
+                    for seg in &mut path.segments {
+                        seg.id = self.sess.next_node_id();
+                    }
                     let span = path.span;
                     self.resolver.definitions().create_def_with_parent(
                         parent_def_index,
@@ -2991,7 +3015,8 @@ impl<'a> LoweringContext<'a> {
 
                     self.with_hir_id_owner(new_node_id, |this| {
                         let new_id = this.lower_node_id(new_node_id);
-                        let path = this.lower_path_extra(def, &path, None, ParamMode::Explicit);
+                        let path =
+                            this.lower_path_extra(def, &path, None, ParamMode::Explicit, None);
                         let item = hir::ItemKind::Use(P(path), hir::UseKind::Single);
                         let vis_kind = match vis.node {
                             hir::VisibilityKind::Public => hir::VisibilityKind::Public,
@@ -3001,7 +3026,6 @@ impl<'a> LoweringContext<'a> {
                                 let id = this.next_id();
                                 hir::VisibilityKind::Restricted {
                                     path: path.clone(),
-                                    // We are allocating a new NodeId here
                                     id: id.node_id,
                                     hir_id: id.hir_id,
                                 }
@@ -3024,50 +3048,60 @@ impl<'a> LoweringContext<'a> {
                     });
                 }
 
-                let path = P(self.lower_path_extra(ret_def, &path, None, ParamMode::Explicit));
+                let path =
+                    P(self.lower_path_extra(ret_def, &path, None, ParamMode::Explicit, None));
                 hir::ItemKind::Use(path, hir::UseKind::Single)
             }
             UseTreeKind::Glob => {
                 let path = P(self.lower_path(
                     id,
                     &Path {
-                        segments: prefix
-                            .segments
-                            .iter()
-                            .chain(path.segments.iter())
-                            .cloned()
-                            .collect(),
+                        segments,
                         span: path.span,
                     },
                     ParamMode::Explicit,
                 ));
                 hir::ItemKind::Use(path, hir::UseKind::Glob)
             }
             UseTreeKind::Nested(ref trees) => {
+                // Nested imports are desugared into simple imports.
+
                 let prefix = Path {
-                    segments: prefix
-                        .segments
-                        .iter()
-                        .chain(path.segments.iter())
-                        .cloned()
-                        .collect(),
+                    segments,
                     span: prefix.span.to(path.span),
                 };
 
-                // Add all the nested PathListItems in the HIR
+                // Add all the nested PathListItems to the HIR.
                 for &(ref use_tree, id) in trees {
                     self.allocate_hir_id_counter(id, &use_tree);
+
                     let LoweredNodeId {
                         node_id: new_id,
                         hir_id: new_hir_id,
                     } = self.lower_node_id(id);
 
                     let mut vis = vis.clone();
                     let mut name = name.clone();
-                    let item =
-                        self.lower_use_tree(use_tree, &prefix, new_id, &mut vis, &mut name, &attrs);
+                    let mut prefix = prefix.clone();
 
+                    // Give the segments new ids since they are being cloned.
+                    for seg in &mut prefix.segments {
+                        seg.id = self.sess.next_node_id();
+                    }
+
+                    // Each `use` import is an item and thus are owners of the
+                    // names in the path. Up to this point the nested import is
+                    // the current owner, since we want each desugared import to
+                    // own its own names, we have to adjust the owner before
+                    // lowering the rest of the import.
                     self.with_hir_id_owner(new_id, |this| {
+                        let item = this.lower_use_tree(use_tree,
+                                                       &prefix,
+                                                       new_id,
+                                                       &mut vis,
+                                                       &mut name,
+                                                       attrs);
+
                         let vis_kind = match vis.node {
                             hir::VisibilityKind::Public => hir::VisibilityKind::Public,
                             hir::VisibilityKind::Crate(sugar) => hir::VisibilityKind::Crate(sugar),
@@ -3076,7 +3110,6 @@ impl<'a> LoweringContext<'a> {
                                 let id = this.next_id();
                                 hir::VisibilityKind::Restricted {
                                     path: path.clone(),
-                                    // We are allocating a new NodeId here
                                     id: id.node_id,
                                     hir_id: id.hir_id,
                                 }
@@ -3089,7 +3122,7 @@ impl<'a> LoweringContext<'a> {
                             hir::Item {
                                 id: new_id,
                                 hir_id: new_hir_id,
-                                name: name,
+                                name,
                                 attrs: attrs.clone(),
                                 node: item,
                                 vis,
@@ -3653,6 +3686,7 @@ impl<'a> LoweringContext<'a> {
                     0,
                     ParenthesizedGenericArgs::Err,
                     ImplTraitContext::disallowed(),
+                    None,
                 );
                 let args = args.iter().map(|x| self.lower_expr(x)).collect();
                 hir::ExprKind::MethodCall(hir_seg, seg.ident.span, args)
@@ -4506,8 +4540,15 @@ impl<'a> LoweringContext<'a> {
                 } else {
                     self.lower_node_id(id)
                 };
+                let def = self.expect_full_def(id);
                 hir::VisibilityKind::Restricted {
-                    path: P(self.lower_path(id, path, ParamMode::Explicit)),
+                    path: P(self.lower_path_extra(
+                        def,
+                        path,
+                        None,
+                        ParamMode::Explicit,
+                        explicit_owner,
+                    )),
                     id: lowered_id.node_id,
                     hir_id: lowered_id.hir_id,
                 }
@@ -4814,8 +4855,15 @@ impl<'a> LoweringContext<'a> {
         params: Option<P<hir::GenericArgs>>,
         is_value: bool
     ) -> hir::Path {
-        self.resolver
-            .resolve_str_path(span, self.crate_root, components, params, is_value)
+        let mut path = self.resolver
+            .resolve_str_path(span, self.crate_root, components, params, is_value);
+
+        for seg in path.segments.iter_mut() {
+            if let Some(id) = seg.id {
+                seg.id = Some(self.lower_node_id(id).node_id);
+            }
+        }
+        path
     }
 
     fn ty_path(&mut self, id: LoweredNodeId, span: Span, qpath: hir::QPath) -> hir::Ty {

src/librustc/hir/map/collector.rs

@@ -210,17 +210,22 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {
                     None => format!("{:?}", node)
                 };
 
-                if hir_id == ::hir::DUMMY_HIR_ID {
-                    debug!("Maybe you forgot to lower the node id {:?}?", id);
-                }
+                let forgot_str = if hir_id == ::hir::DUMMY_HIR_ID {
+                    format!("\nMaybe you forgot to lower the node id {:?}?", id)
+                } else {
+                    String::new()
+                };
 
                 bug!("inconsistent DepNode for `{}`: \
-                      current_dep_node_owner={}, hir_id.owner={}",
+                      current_dep_node_owner={} ({:?}), hir_id.owner={} ({:?}) {}",
                     node_str,
                     self.definitions
                         .def_path(self.current_dep_node_owner)
                         .to_string_no_crate(),
-                    self.definitions.def_path(hir_id.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)
             }
         }
 

src/librustc/hir/map/hir_id_validator.rs

@@ -88,7 +88,7 @@ impl<'a, 'hir: 'a> HirIdValidator<'a, 'hir> {
         walk(self);
 
         if owner_def_index == CRATE_DEF_INDEX {
-            return
+            return;
         }
 
         // There's always at least one entry for the owning item itself
@@ -129,13 +129,16 @@ impl<'a, 'hir: 'a> HirIdValidator<'a, 'hir> {
                                            local_id,
                                            self.hir_map.node_to_string(node_id)));
             }
-
             self.errors.push(format!(
                 "ItemLocalIds not assigned densely in {}. \
-                Max ItemLocalId = {}, missing IDs = {:?}",
+                Max ItemLocalId = {}, missing IDs = {:?}; seens IDs = {:?}",
                 self.hir_map.def_path(DefId::local(owner_def_index)).to_string_no_crate(),
                 max,
-                missing_items));
+                missing_items,
+                self.hir_ids_seen
+                    .values()
+                    .map(|n| format!("({:?} {})", n, self.hir_map.node_to_string(*n)))
+                    .collect::<Vec<_>>()));
         }
     }
 }
@@ -155,6 +158,7 @@ impl<'a, 'hir: 'a> intravisit::Visitor<'hir> for HirIdValidator<'a, 'hir> {
             self.errors.push(format!("HirIdValidator: No HirId assigned for NodeId {}: {:?}",
                                      node_id,
                                      self.hir_map.node_to_string(node_id)));
+            return;
         }
 
         if owner != stable_id.owner {

src/librustc/hir/map/mod.rs

@@ -1100,7 +1100,7 @@ impl<'a> print::State<'a> {
             Node::AnonConst(a)    => self.print_anon_const(&a),
             Node::Expr(a)         => self.print_expr(&a),
             Node::Stmt(a)         => self.print_stmt(&a),
-            Node::PathSegment(_)  => bug!("cannot print PathSegment"),
+            Node::PathSegment(a)  => self.print_path_segment(&a),
             Node::Ty(a)           => self.print_type(&a),
             Node::TraitRef(a)     => self.print_trait_ref(&a),
             Node::Binding(a)      |