Separate variant id and variant constructor id.

This commit makes two changes - separating the `NodeId` that identifies
an enum variant from the `NodeId` that identifies the variant's
constructor; and no longer creating a `NodeId` for `Struct`-style enum
variants and structs.

Separation of the variant id and variant constructor id will allow the
rest of RFC 2008 to be implemented by lowering the visibility of the
variant's constructor without lowering the visbility of the variant
itself.

No longer creating a `NodeId` for `Struct`-style enum variants and
structs mostly simplifies logic as previously this `NodeId` wasn't used.
There were various cases where the `NodeId` wouldn't be used unless
there was an unit or tuple struct or enum variant but not all uses of
this `NodeId` had that condition, by removing this `NodeId`, this must
be explicitly dealt with. This change mostly applied cleanly, but there
were one or two cases in name resolution and one case in type check
where the existing logic required a id for `Struct`-style enum variants
and structs.

Author: davidtwco

src/librustc/hir/def.rs

@@ -37,9 +37,13 @@ pub enum NonMacroAttrKind {
 pub enum Def {
     // Type namespace
     Mod(DefId),
-    Struct(DefId), // `DefId` refers to `NodeId` of the struct itself
+    /// `DefId` refers to `NodeId` of the struct. `Def::VariantCtor` represents the constructor of
+    /// a struct.
+    Struct(DefId),
     Union(DefId),
     Enum(DefId),
+    /// `DefId` refers to the `NodeId` of the variant. `Def::VariantCtor` represents the
+    /// constructor of an enum variant.
     Variant(DefId),
     Trait(DefId),
     /// `existential type Foo: Bar;`
@@ -61,8 +65,8 @@ pub enum Def {
     Const(DefId),
     ConstParam(DefId),
     Static(DefId, bool /* is_mutbl */),
-    StructCtor(DefId, CtorKind), // `DefId` refers to `NodeId` of the struct's constructor
-    VariantCtor(DefId, CtorKind), // `DefId` refers to the enum variant
+    /// `DefId` refers to `NodeId` of the struct or enum variant's constructor.
+    Ctor(hir::CtorOf, DefId, CtorKind),
     SelfCtor(DefId /* impl */),  // `DefId` refers to the impl
     Method(DefId),
     AssociatedConst(DefId),
@@ -265,10 +269,9 @@ impl Def {
     pub fn opt_def_id(&self) -> Option<DefId> {
         match *self {
             Def::Fn(id) | Def::Mod(id) | Def::Static(id, _) |
-            Def::Variant(id) | Def::VariantCtor(id, ..) | Def::Enum(id) |
+            Def::Variant(id) | Def::Ctor(_, id, ..) | Def::Enum(id) |
             Def::TyAlias(id) | Def::TraitAlias(id) |
             Def::AssociatedTy(id) | Def::TyParam(id) | Def::ConstParam(id) | Def::Struct(id) |
-            Def::StructCtor(id, ..) |
             Def::Union(id) | Def::Trait(id) | Def::Method(id) | Def::Const(id) |
             Def::AssociatedConst(id) | Def::Macro(id, ..) |
             Def::Existential(id) | Def::AssociatedExistential(id) | Def::ForeignTy(id) => {
@@ -303,20 +306,21 @@ impl Def {
             Def::Fn(..) => "function",
             Def::Mod(..) => "module",
             Def::Static(..) => "static",
-            Def::Variant(..) => "variant",
-            Def::VariantCtor(.., CtorKind::Fn) => "tuple variant",
-            Def::VariantCtor(.., CtorKind::Const) => "unit variant",
-            Def::VariantCtor(.., CtorKind::Fictive) => "struct variant",
             Def::Enum(..) => "enum",
+            Def::Variant(..) => "variant",
+            Def::Ctor(hir::CtorOf::Variant, _, CtorKind::Fn) => "tuple variant",
+            Def::Ctor(hir::CtorOf::Variant, _, CtorKind::Const) => "unit variant",
+            Def::Ctor(hir::CtorOf::Variant, _, CtorKind::Fictive) => "struct variant",
+            Def::Struct(..) => "struct",
+            Def::Ctor(hir::CtorOf::Struct, _, CtorKind::Fn) => "tuple struct",
+            Def::Ctor(hir::CtorOf::Struct, _, CtorKind::Const) => "unit struct",
+            Def::Ctor(hir::CtorOf::Struct, _, CtorKind::Fictive) =>
+                bug!("impossible struct constructor"),
             Def::Existential(..) => "existential type",
             Def::TyAlias(..) => "type alias",
             Def::TraitAlias(..) => "trait alias",
             Def::AssociatedTy(..) => "associated type",
             Def::AssociatedExistential(..) => "associated existential type",
-            Def::Struct(..) => "struct",
-            Def::StructCtor(.., CtorKind::Fn) => "tuple struct",
-            Def::StructCtor(.., CtorKind::Const) => "unit struct",
-            Def::StructCtor(.., CtorKind::Fictive) => bug!("impossible struct constructor"),
             Def::SelfCtor(..) => "self constructor",
             Def::Union(..) => "union",
             Def::Trait(..) => "trait",

src/librustc/hir/intravisit.rs

@@ -559,6 +559,7 @@ pub fn walk_variant<'v, V: Visitor<'v>>(visitor: &mut V,
                                         generics: &'v Generics,
                                         parent_item_id: HirId) {
     visitor.visit_ident(variant.node.ident);
+    visitor.visit_id(variant.node.id);
     visitor.visit_variant_data(&variant.node.data,
                                variant.node.ident.name,
                                generics,
@@ -923,7 +924,9 @@ pub fn walk_impl_item_ref<'v, V: Visitor<'v>>(visitor: &mut V, impl_item_ref: &'
 
 
 pub fn walk_struct_def<'v, V: Visitor<'v>>(visitor: &mut V, struct_definition: &'v VariantData) {
-    visitor.visit_id(struct_definition.hir_id());
+    if let Some(ctor_hir_id) = struct_definition.ctor_hir_id() {
+        visitor.visit_id(ctor_hir_id);
+    }
     walk_list!(visitor, visit_struct_field, struct_definition.fields());
 }
 

src/librustc/hir/lowering.rs

@@ -1615,9 +1615,11 @@ impl<'a> LoweringContext<'a> {
     }
 
     fn lower_variant(&mut self, v: &Variant) -> hir::Variant {
+        let LoweredNodeId { node_id: _, hir_id } = self.lower_node_id(v.node.id);
         Spanned {
             node: hir::VariantKind {
                 ident: v.node.ident,
+                id: hir_id,
                 attrs: self.lower_attrs(&v.node.attrs),
                 data: self.lower_variant_data(&v.node.data),
                 disr_expr: v.node.disr_expr.as_ref().map(|e| self.lower_anon_const(e)),
@@ -2669,19 +2671,10 @@ impl<'a> LoweringContext<'a> {
 
     fn lower_variant_data(&mut self, vdata: &VariantData) -> hir::VariantData {
         match *vdata {
-            VariantData::Struct(ref fields, id, recovered) => {
-                let LoweredNodeId { node_id: _, hir_id } = self.lower_node_id(id);
-
-                hir::VariantData::Struct(
-                    fields
-                        .iter()
-                        .enumerate()
-                        .map(|f| self.lower_struct_field(f))
-                        .collect(),
-                    hir_id,
-                    recovered,
-                )
-            },
+            VariantData::Struct(ref fields, recovered) => hir::VariantData::Struct(
+                fields.iter().enumerate().map(|f| self.lower_struct_field(f)).collect(),
+                recovered,
+            ),
             VariantData::Tuple(ref fields, id) => {
                 let LoweredNodeId { node_id: _, hir_id } = self.lower_node_id(id);
 
@@ -2696,7 +2689,6 @@ impl<'a> LoweringContext<'a> {
             },
             VariantData::Unit(id) => {
                 let LoweredNodeId { node_id: _, hir_id } = self.lower_node_id(id);
-
                 hir::VariantData::Unit(hir_id)
             },
         }

src/librustc/hir/map/collector.rs

@@ -360,9 +360,11 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
             this.insert(i.span, i.hir_id, Node::Item(i));
             this.with_parent(i.hir_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(i.span, struct_def.hir_id(), Node::StructCtor(struct_def));
+                    // If this is a tuple or unit-like struct, register the constructor.
+                    if let Some(ctor_hir_id) = struct_def.ctor_hir_id() {
+                        this.insert(i.span,
+                                    ctor_hir_id,
+                                    Node::Ctor(hir::CtorOf::Struct, struct_def));
                     }
                 }
                 intravisit::walk_item(this, i);
@@ -515,8 +517,12 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
     }
 
     fn visit_variant(&mut self, v: &'hir Variant, g: &'hir Generics, item_id: HirId) {
-        self.insert(v.span, v.node.data.hir_id(), Node::Variant(v));
-        self.with_parent(v.node.data.hir_id(), |this| {
+        self.insert(v.span, v.node.id, Node::Variant(v));
+        self.with_parent(v.node.id, |this| {
+            // Register the constructor of this variant.
+            if let Some(ctor_hir_id) = v.node.data.ctor_hir_id() {
+                this.insert(v.span, ctor_hir_id, Node::Ctor(hir::CtorOf::Variant, &v.node.data));
+            }
             intravisit::walk_variant(this, v, g, item_id);
         });
     }

src/librustc/hir/map/def_collector.rs

@@ -158,9 +158,9 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> {
         self.with_parent(def, |this| {
             match i.node {
                 ItemKind::Struct(ref struct_def, _) | ItemKind::Union(ref struct_def, _) => {
-                    // If this is a tuple-like struct, register the constructor.
-                    if !struct_def.is_struct() {
-                        this.create_def(struct_def.id(),
+                    // If this is a unit or tuple-like struct, register the constructor.
+                    if let Some(ctor_hir_id) = struct_def.ctor_id() {
+                        this.create_def(ctor_hir_id,
                                         DefPathData::StructCtor,
                                         REGULAR_SPACE,
                                         i.span);
@@ -193,11 +193,19 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> {
     }
 
     fn visit_variant(&mut self, v: &'a Variant, g: &'a Generics, item_id: NodeId) {
-        let def = self.create_def(v.node.data.id(),
+        let def = self.create_def(v.node.id,
                                   DefPathData::EnumVariant(v.node.ident.as_interned_str()),
                                   REGULAR_SPACE,
                                   v.span);
-        self.with_parent(def, |this| visit::walk_variant(this, v, g, item_id));
+        self.with_parent(def, |this| {
+            if let Some(ctor_hir_id) = v.node.data.ctor_id() {
+                this.create_def(ctor_hir_id,
+                                DefPathData::VariantCtor,
+                                REGULAR_SPACE,
+                                v.span);
+            }
+            visit::walk_variant(this, v, g, item_id)
+        });
     }
 
     fn visit_variant_data(&mut self, data: &'a VariantData, _: Ident,

src/librustc/hir/map/definitions.rs

@@ -366,8 +366,10 @@ pub enum DefPathData {
     EnumVariant(InternedString),
     /// A struct field
     Field(InternedString),
-    /// Implicit ctor for a tuple-like struct
+    /// Implicit ctor for a unit or tuple-like struct
     StructCtor,
+    /// Implicit ctor for a unit or tuple-like enum variant
+    VariantCtor,
     /// A constant expression (see {ast,hir}::AnonConst).
     AnonConst,
     /// An `impl Trait` type node
@@ -653,6 +655,7 @@ impl DefPathData {
             Misc |
             ClosureExpr |
             StructCtor |
+            VariantCtor |
             AnonConst |
             ImplTrait => None
         }
@@ -683,7 +686,8 @@ impl DefPathData {
             Impl => "{{impl}}",
             Misc => "{{misc}}",
             ClosureExpr => "{{closure}}",
-            StructCtor => "{{constructor}}",
+            StructCtor => "{{struct constructor}}",
+            VariantCtor => "{{variant constructor}}",
             AnonConst => "{{constant}}",
             ImplTrait => "{{opaque}}",
         };

src/librustc/hir/map/mod.rs

@@ -366,12 +366,15 @@ impl<'hir> Map<'hir> {
                 }
             }
             Node::Variant(variant) => {
-                let def_id = self.local_def_id_from_hir_id(variant.node.data.hir_id());
+                let def_id = self.local_def_id_from_hir_id(variant.node.id);
                 Some(Def::Variant(def_id))
             }
-            Node::StructCtor(variant) => {
-                let def_id = self.local_def_id_from_hir_id(variant.hir_id());
-                Some(Def::StructCtor(def_id, def::CtorKind::from_hir(variant)))
+            Node::Ctor(ctor_of, variant_data) => {
+                variant_data.ctor_hir_id()
+                    .map(|hir_id| self.local_def_id_from_hir_id(hir_id))
+                    .map(|def_id| Def::Ctor(
+                        ctor_of, def_id, def::CtorKind::from_hir(variant_data),
+                    ))
             }
             Node::AnonConst(_) |
             Node::Field(_) |
@@ -516,8 +519,7 @@ impl<'hir> Map<'hir> {
             Node::AnonConst(_) => {
                 BodyOwnerKind::Const
             }
-            Node::Variant(&Spanned { node: VariantKind { data: VariantData::Tuple(..), .. }, .. }) |
-            Node::StructCtor(..) |
+            Node::Ctor(..) |
             Node::Item(&Item { node: ItemKind::Fn(..), .. }) |
             Node::TraitItem(&TraitItem { node: TraitItemKind::Method(..), .. }) |
             Node::ImplItem(&ImplItem { node: ImplItemKind::Method(..), .. }) => {
@@ -948,8 +950,8 @@ impl<'hir> Map<'hir> {
                     _ => bug!("struct ID bound to non-struct {}", self.hir_to_string(id))
                 }
             }
-            Some(Node::StructCtor(data)) => data,
             Some(Node::Variant(variant)) => &variant.node.data,
+            Some(Node::Ctor(_, data)) => data,
             _ => bug!("expected struct or variant, found {}", self.hir_to_string(id))
         }
     }
@@ -993,7 +995,7 @@ impl<'hir> Map<'hir> {
             Node::Lifetime(lt) => lt.name.ident().name,
             Node::GenericParam(param) => param.name.ident().name,
             Node::Binding(&Pat { node: PatKind::Binding(_, _, l, _), .. }) => l.name,
-            Node::StructCtor(_) => self.name(self.get_parent(id)),
+            Node::Ctor(..) => self.name(self.get_parent(id)),
             _ => bug!("no name for {}", self.node_to_string(id))
         }
     }
@@ -1019,9 +1021,9 @@ impl<'hir> Map<'hir> {
             Some(Node::Expr(ref e)) => Some(&*e.attrs),
             Some(Node::Stmt(ref s)) => Some(s.node.attrs()),
             Some(Node::GenericParam(param)) => Some(&param.attrs[..]),
-            // unit/tuple structs take the attributes straight from
-            // the struct definition.
-            Some(Node::StructCtor(_)) => return self.attrs(self.get_parent(id)),
+            // Unit/tuple structs/variants take the attributes straight from
+            // the struct/variant definition.
+            Some(Node::Ctor(..)) => return self.attrs(self.get_parent(id)),
             _ => None
         };
         attrs.unwrap_or(&[])
@@ -1068,7 +1070,10 @@ impl<'hir> Map<'hir> {
             Some(Node::Binding(pat)) => pat.span,
             Some(Node::Pat(pat)) => pat.span,
             Some(Node::Block(block)) => block.span,
-            Some(Node::StructCtor(_)) => self.expect_item(self.get_parent(id)).span,
+            Some(Node::Ctor(CtorOf::Struct, _)) =>
+                self.expect_item(self.get_parent(id)).span,
+            Some(Node::Ctor(CtorOf::Variant, _)) =>
+                self.expect_variant(self.node_to_hir_id(self.get_parent_node(id))).span,
             Some(Node::Lifetime(lifetime)) => lifetime.span,
             Some(Node::GenericParam(param)) => param.span,
             Some(Node::Visibility(&Spanned {
@@ -1324,7 +1329,7 @@ impl<'a> print::State<'a> {
             // these cases do not carry enough information in the
             // hir_map to reconstruct their full structure for pretty
             // printing.
-            Node::StructCtor(_)   => bug!("cannot print isolated StructCtor"),
+            Node::Ctor(..)        => bug!("cannot print isolated Ctor"),
             Node::Local(a)        => self.print_local_decl(&a),
             Node::MacroDef(_)     => bug!("cannot print MacroDef"),
             Node::Crate           => bug!("cannot print Crate"),
@@ -1443,8 +1448,8 @@ fn node_id_to_string(map: &Map<'_>, id: NodeId, include_id: bool) -> String {
         Some(Node::Local(_)) => {
             format!("local {}{}", map.node_to_pretty_string(id), id_str)
         }
-        Some(Node::StructCtor(_)) => {
-            format!("struct_ctor {}{}", path_str(), id_str)
+        Some(Node::Ctor(..)) => {
+            format!("ctor {}{}", path_str(), id_str)
         }
         Some(Node::Lifetime(_)) => {
             format!("lifetime {}{}", map.node_to_pretty_string(id), id_str)