Store Ident in DefPathData instead of Symbol

src/librustc/hir/map/definitions.rs

@@ -13,7 +13,7 @@ use rustc_hir::def_id::{CrateNum, DefId, DefIndex, CRATE_DEF_INDEX, LOCAL_CRATE}
 use rustc_index::vec::IndexVec;
 use rustc_session::CrateDisambiguator;
 use rustc_span::hygiene::ExpnId;
-use rustc_span::symbol::{sym, Symbol};
+use rustc_span::symbol::{sym, Ident, Symbol};
 use rustc_span::Span;
 
 use std::fmt::Write;
@@ -90,7 +90,9 @@ pub struct Definitions {
     parent_modules_of_macro_defs: FxHashMap<ExpnId, DefId>,
     /// Item with a given `DefIndex` was defined during macro expansion with ID `ExpnId`.
     expansions_that_defined: FxHashMap<DefIndex, ExpnId>,
-    next_disambiguator: FxHashMap<(DefIndex, DefPathData), u32>,
+    // We use a `PlainDefPathData` instead of `DefPathData` so that we don't
+    // consider `Span`s (from `Ident`) when hashing or comparing
+    next_disambiguator: FxHashMap<(DefIndex, PlainDefPathData), u32>,
     def_index_to_span: FxHashMap<DefIndex, Span>,
     /// When collecting definitions from an AST fragment produced by a macro invocation `ExpnId`
     /// we know what parent node that fragment should be attached to thanks to this table.
@@ -102,7 +104,7 @@ pub struct Definitions {
 /// A unique identifier that we can use to lookup a definition
 /// precisely. It combines the index of the definition's parent (if
 /// any) with a `DisambiguatedDefPathData`.
-#[derive(Copy, Clone, PartialEq, Debug, RustcEncodable, RustcDecodable)]
+#[derive(Copy, Clone, Debug, RustcEncodable, RustcDecodable)]
 pub struct DefKey {
     /// The parent path.
     pub parent: Option<DefIndex>,
@@ -153,7 +155,7 @@ impl DefKey {
 /// between them. This introduces some artificial ordering dependency
 /// but means that if you have, e.g., two impls for the same type in
 /// the same module, they do get distinct `DefId`s.
-#[derive(Copy, Clone, PartialEq, Debug, RustcEncodable, RustcDecodable)]
+#[derive(Copy, Clone, Debug, RustcEncodable, RustcDecodable)]
 pub struct DisambiguatedDefPathData {
     pub data: DefPathData,
     pub disambiguator: u32,
@@ -206,7 +208,7 @@ impl DefPath {
         let mut s = String::with_capacity(self.data.len() * 16);
 
         for component in &self.data {
-            write!(s, "::{}[{}]", component.data.as_symbol(), component.disambiguator).unwrap();
+            write!(s, "::{}[{}]", component.data.as_ident().name, component.disambiguator).unwrap();
         }
 
         s
@@ -225,9 +227,10 @@ impl DefPath {
 
         for component in &self.data {
             if component.disambiguator == 0 {
-                write!(s, "::{}", component.data.as_symbol()).unwrap();
+                write!(s, "::{}", component.data.as_ident().name).unwrap();
             } else {
-                write!(s, "{}[{}]", component.data.as_symbol(), component.disambiguator).unwrap();
+                write!(s, "{}[{}]", component.data.as_ident().name, component.disambiguator)
+                    .unwrap();
             }
         }
 
@@ -245,16 +248,49 @@ impl DefPath {
             opt_delimiter.map(|d| s.push(d));
             opt_delimiter = Some('-');
             if component.disambiguator == 0 {
-                write!(s, "{}", component.data.as_symbol()).unwrap();
+                write!(s, "{}", component.data.as_ident().name).unwrap();
             } else {
-                write!(s, "{}[{}]", component.data.as_symbol(), component.disambiguator).unwrap();
+                write!(s, "{}[{}]", component.data.as_ident().name, component.disambiguator)
+                    .unwrap();
             }
         }
         s
     }
 }
 
-#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
+// A copy of `DefPathData`, but with `Symbol` substituted for `Ident`.
+// We want to implement `Hash`, `Eq`, and `PartialEq` for this struct,
+// but not for `DefPathData`. This prevents us from doing something like
+// ```enum BaseDefPathData<T> { ... }, type DefPathData = BaseDefPathData<Ident>```
+//, as `DefPathData` would end up implementing `Hash`, `Eq`, and `PartialEq` due to
+// the `#[derive] macros we would need to place on `BaseDefPathData`.
+//
+// This could be fixed by switching to the `derivative` crate, which allows
+// custom bounds to be applied to parameters in the generated impls.
+// This would allow us to write `trait IsSymbol {}; impl IsSymbol for Symbol {}`.
+// and add a `T: IsSymbol` bound to the `derivative` impls on `PlainDefPathData`.
+//
+// For now, this small amount of duplication (which is invisible outside of this module)
+// isn't worth pulling in an extra dependency.
+#[derive(Eq, PartialEq, Hash, Clone, Copy)]
+enum PlainDefPathData {
+    CrateRoot,
+    Misc,
+    Impl,
+    TypeNs(Symbol),
+    ValueNs(Symbol),
+    MacroNs(Symbol),
+    LifetimeNs(Symbol),
+    ClosureExpr,
+    Ctor,
+    AnonConst,
+    ImplTrait,
+}
+
+// We intentionally do *not* derive `Eq`, `PartialEq`, and `Hash`:
+// this would cause us to hash/compare using the `Span` from the `Ident`,
+// which is almost never correct.
+#[derive(Copy, Clone, Debug, RustcEncodable, RustcDecodable)]
 pub enum DefPathData {
     // Root: these should only be used for the root nodes, because
     // they are treated specially by the `def_path` function.
@@ -267,13 +303,13 @@ pub enum DefPathData {
     /// An impl.
     Impl,
     /// Something in the type namespace.
-    TypeNs(Symbol),
+    TypeNs(Ident),
     /// Something in the value namespace.
-    ValueNs(Symbol),
+    ValueNs(Ident),
     /// Something in the macro namespace.
-    MacroNs(Symbol),
+    MacroNs(Ident),
     /// Something in the lifetime namespace.
-    LifetimeNs(Symbol),
+    LifetimeNs(Ident),
     /// A closure expression.
     ClosureExpr,
 
@@ -428,11 +464,12 @@ impl Definitions {
         );
 
         // The root node must be created with `create_root_def()`.
-        assert!(data != DefPathData::CrateRoot);
+        assert!(!(matches!(data, DefPathData::CrateRoot)));
 
         // Find the next free disambiguator for this key.
         let disambiguator = {
-            let next_disamb = self.next_disambiguator.entry((parent, data)).or_insert(0);
+            let plain_data = data.to_plain();
+            let next_disamb = self.next_disambiguator.entry((parent, plain_data)).or_insert(0);
             let disambiguator = *next_disamb;
             *next_disamb = next_disamb.checked_add(1).expect("disambiguator overflow");
             disambiguator
@@ -522,7 +559,24 @@ impl Definitions {
 }
 
 impl DefPathData {
-    pub fn get_opt_name(&self) -> Option<Symbol> {
+    fn to_plain(&self) -> PlainDefPathData {
+        use self::DefPathData::*;
+        match *self {
+            TypeNs(name) => PlainDefPathData::TypeNs(name.name),
+            ValueNs(name) => PlainDefPathData::ValueNs(name.name),
+            MacroNs(name) => PlainDefPathData::MacroNs(name.name),
+            LifetimeNs(name) => PlainDefPathData::LifetimeNs(name.name),
+            CrateRoot => PlainDefPathData::CrateRoot,
+            Misc => PlainDefPathData::Misc,
+            Impl => PlainDefPathData::Impl,
+            ClosureExpr => PlainDefPathData::ClosureExpr,
+            Ctor => PlainDefPathData::Ctor,
+            AnonConst => PlainDefPathData::AnonConst,
+            ImplTrait => PlainDefPathData::ImplTrait,
+        }
+    }
+
+    pub fn get_opt_name(&self) -> Option<Ident> {
         use self::DefPathData::*;
         match *self {
             TypeNs(name) | ValueNs(name) | MacroNs(name) | LifetimeNs(name) => Some(name),
@@ -531,22 +585,22 @@ impl DefPathData {
         }
     }
 
-    pub fn as_symbol(&self) -> Symbol {
+    pub fn as_ident(&self) -> Ident {
         use self::DefPathData::*;
         match *self {
             TypeNs(name) | ValueNs(name) | MacroNs(name) | LifetimeNs(name) => name,
             // Note that this does not show up in user print-outs.
-            CrateRoot => sym::double_braced_crate,
-            Impl => sym::double_braced_impl,
-            Misc => sym::double_braced_misc,
-            ClosureExpr => sym::double_braced_closure,
-            Ctor => sym::double_braced_constructor,
-            AnonConst => sym::double_braced_constant,
-            ImplTrait => sym::double_braced_opaque,
+            CrateRoot => Ident::with_dummy_span(sym::double_braced_crate),
+            Impl => Ident::with_dummy_span(sym::double_braced_impl),
+            Misc => Ident::with_dummy_span(sym::double_braced_misc),
+            ClosureExpr => Ident::with_dummy_span(sym::double_braced_closure),
+            Ctor => Ident::with_dummy_span(sym::double_braced_constructor),
+            AnonConst => Ident::with_dummy_span(sym::double_braced_constant),
+            ImplTrait => Ident::with_dummy_span(sym::double_braced_opaque),
         }
     }
 
     pub fn to_string(&self) -> String {
-        self.as_symbol().to_string()
+        self.as_ident().name.to_string()
     }
 }

src/librustc/mir/interpret/error.rs

@@ -64,9 +64,10 @@ pub struct FrameInfo<'tcx> {
 impl<'tcx> fmt::Display for FrameInfo<'tcx> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         ty::tls::with(|tcx| {
-            if tcx.def_key(self.instance.def_id()).disambiguated_data.data
-                == DefPathData::ClosureExpr
-            {
+            if matches!(
+                tcx.def_key(self.instance.def_id()).disambiguated_data.data,
+                DefPathData::ClosureExpr
+            ) {
                 write!(f, "inside call to closure")?;
             } else {
                 write!(f, "inside call to `{}`", self.instance)?;

src/librustc/ty/mod.rs

@@ -2996,9 +2996,16 @@ impl<'tcx> TyCtxt<'tcx> {
                 hir_map::DefPathData::Ctor => {
                     self.item_name(DefId { krate: id.krate, index: def_key.parent.unwrap() })
                 }
-                _ => def_key.disambiguated_data.data.get_opt_name().unwrap_or_else(|| {
-                    bug!("item_name: no name for {:?}", self.def_path(id));
-                }),
+                _ => {
+                    def_key
+                        .disambiguated_data
+                        .data
+                        .get_opt_name()
+                        .unwrap_or_else(|| {
+                            bug!("item_name: no name for {:?}", self.def_path(id));
+                        })
+                        .name
+                }
             }
         }
     }

src/librustc/ty/print/obsolete.rs

@@ -186,9 +186,9 @@ impl DefPathBasedNames<'tcx> {
         // foo::bar::ItemName::
         for part in self.tcx.def_path(def_id).data {
             if self.omit_disambiguators {
-                write!(output, "{}::", part.data.as_symbol()).unwrap();
+                write!(output, "{}::", part.data.as_ident().name).unwrap();
             } else {
-                write!(output, "{}[{}]::", part.data.as_symbol(), part.disambiguator).unwrap();
+                write!(output, "{}[{}]::", part.data.as_ident().name, part.disambiguator).unwrap();
             }
         }
 

src/librustc/ty/print/pretty.rs

@@ -13,7 +13,7 @@ use rustc_apfloat::ieee::{Double, Single};
 use rustc_apfloat::Float;
 use rustc_ast::ast;
 use rustc_attr::{SignedInt, UnsignedInt};
-use rustc_span::symbol::{kw, Symbol};
+use rustc_span::symbol::{kw, Ident, Symbol};
 use rustc_target::spec::abi::Abi;
 
 use std::cell::Cell;
@@ -402,12 +402,14 @@ pub trait PrettyPrinter<'tcx>:
                     .find(|child| child.res.def_id() == def_id)
                     .map(|child| child.ident.name);
                 if let Some(reexport) = reexport {
-                    *name = reexport;
+                    *name = Ident::with_dummy_span(reexport);
                 }
             }
             // Re-exported `extern crate` (#43189).
             DefPathData::CrateRoot => {
-                data = DefPathData::TypeNs(self.tcx().original_crate_name(def_id.krate));
+                data = DefPathData::TypeNs(Ident::with_dummy_span(
+                    self.tcx().original_crate_name(def_id.krate),
+                ));
             }
             _ => {}
         }
@@ -1401,7 +1403,7 @@ impl<F: fmt::Write> Printer<'tcx> for FmtPrinter<'_, 'tcx, F> {
 
         // FIXME(eddyb) `name` should never be empty, but it
         // currently is for `extern { ... }` "foreign modules".
-        let name = disambiguated_data.data.as_symbol().as_str();
+        let name = disambiguated_data.data.to_string();
         if !name.is_empty() {
             if !self.empty_path {
                 write!(self, "::")?;

src/librustc/ty/query/profiling_support.rs

@@ -60,12 +60,12 @@ impl<'p, 'c, 'tcx> QueryKeyStringBuilder<'p, 'c, 'tcx> {
 
         match def_key.disambiguated_data.data {
             DefPathData::CrateRoot => {
-                name = self.tcx.original_crate_name(def_id.krate).as_str();
+                name = self.tcx.original_crate_name(def_id.krate).as_str().to_string();
                 dis = "";
                 end_index = 3;
             }
             other => {
-                name = other.as_symbol().as_str();
+                name = other.to_string();
                 if def_key.disambiguated_data.disambiguator == 0 {
                     dis = "";
                     end_index = 3;

src/librustc/ty/util.rs

@@ -448,7 +448,7 @@ impl<'tcx> TyCtxt<'tcx> {
     /// those are not yet phased out). The parent of the closure's
     /// `DefId` will also be the context where it appears.
     pub fn is_closure(self, def_id: DefId) -> bool {
-        self.def_key(def_id).disambiguated_data.data == DefPathData::ClosureExpr
+        matches!(self.def_key(def_id).disambiguated_data.data, DefPathData::ClosureExpr)
     }
 
     /// Returns `true` if `def_id` refers to a trait (i.e., `trait Foo { ... }`).
@@ -465,7 +465,7 @@ impl<'tcx> TyCtxt<'tcx> {
     /// Returns `true` if this `DefId` refers to the implicit constructor for
     /// a tuple struct like `struct Foo(u32)`, and `false` otherwise.
     pub fn is_constructor(self, def_id: DefId) -> bool {
-        self.def_key(def_id).disambiguated_data.data == DefPathData::Ctor
+        matches!(self.def_key(def_id).disambiguated_data.data, DefPathData::Ctor)
     }
 
     /// Given the def-ID of a fn or closure, returns the def-ID of

src/librustc_ast_lowering/lib.rs

@@ -764,17 +764,21 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
         // Get the name we'll use to make the def-path. Note
         // that collisions are ok here and this shouldn't
         // really show up for end-user.
-        let (str_name, kind) = match hir_name {
-            ParamName::Plain(ident) => (ident.name, hir::LifetimeParamKind::InBand),
-            ParamName::Fresh(_) => (kw::UnderscoreLifetime, hir::LifetimeParamKind::Elided),
-            ParamName::Error => (kw::UnderscoreLifetime, hir::LifetimeParamKind::Error),
+        let (name, kind) = match hir_name {
+            ParamName::Plain(ident) => (ident, hir::LifetimeParamKind::InBand),
+            ParamName::Fresh(_) => {
+                (Ident::with_dummy_span(kw::UnderscoreLifetime), hir::LifetimeParamKind::Elided)
+            }
+            ParamName::Error => {
+                (Ident::with_dummy_span(kw::UnderscoreLifetime), hir::LifetimeParamKind::Error)
+            }
         };
 
         // Add a definition for the in-band lifetime def.
         self.resolver.definitions().create_def_with_parent(
             parent_index,
             node_id,
-            DefPathData::LifetimeNs(str_name),
+            DefPathData::LifetimeNs(name),
             ExpnId::root(),
             span,
         );
@@ -1560,7 +1564,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                     self.context.resolver.definitions().create_def_with_parent(
                         self.parent,
                         def_node_id,
-                        DefPathData::LifetimeNs(name.ident().name),
+                        DefPathData::LifetimeNs(name.ident()),
                         ExpnId::root(),
                         lifetime.span,
                     );

src/librustc_codegen_llvm/debuginfo/namespace.rs

@@ -29,7 +29,7 @@ pub fn item_namespace(cx: &CodegenCx<'ll, '_>, def_id: DefId) -> &'ll DIScope {
 
     let namespace_name = match def_key.disambiguated_data.data {
         DefPathData::CrateRoot => cx.tcx.crate_name(def_id.krate),
-        data => data.as_symbol(),
+        data => data.as_ident().name,
     };
     let namespace_name = namespace_name.as_str();
 

src/librustc_codegen_ssa/debuginfo/type_names.rs

@@ -217,7 +217,7 @@ pub fn push_debuginfo_type_name<'tcx>(
             output.push_str(&tcx.crate_name(def_id.krate).as_str());
             for path_element in tcx.def_path(def_id).data {
                 output.push_str("::");
-                output.push_str(&path_element.data.as_symbol().as_str());
+                output.push_str(&path_element.data.to_string());
             }
         } else {
             output.push_str(&tcx.item_name(def_id).as_str());

src/librustc_codegen_utils/symbol_names/legacy.rs

@@ -315,7 +315,7 @@ impl Printer<'tcx> for SymbolPrinter<'tcx> {
             self.path.finalize_pending_component();
         }
 
-        self.write_str(&disambiguated_data.data.as_symbol().as_str())?;
+        self.write_str(&disambiguated_data.data.to_string())?;
         Ok(self)
     }
     fn path_generic_args(

src/librustc_infer/infer/error_reporting/mod.rs

@@ -631,7 +631,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
                 disambiguated_data: &DisambiguatedDefPathData,
             ) -> Result<Self::Path, Self::Error> {
                 let mut path = print_prefix(self)?;
-                path.push(disambiguated_data.data.as_symbol().to_string());
+                path.push(disambiguated_data.data.to_string());
                 Ok(path)
             }
             fn path_generic_args(

src/librustc_lint/context.rs

@@ -793,7 +793,7 @@ impl<'a, 'tcx> LateContext<'a, 'tcx> {
                     _ => {}
                 }
 
-                path.push(disambiguated_data.data.as_symbol());
+                path.push(disambiguated_data.data.as_ident().name);
                 Ok(path)
             }