Skip to content

Commit 0cdc58a

Browse files
alexcrichtonalexcrichton
committed
rustc: Store InternedString in DefPathData
Previously a `Symbol` was stored there, but this ended up causing hash collisions in situations that otherwise shouldn't have a hash collision. Only the symbol's string value was hashed, but it was possible for distinct symbols to have the same string value, fooling various calcuations into thinking that these paths *didn't* need disambiguating data when in fact they did! By storing `InternedString` instead we're hopefully triggering all the exising logic to disambiguate paths with same-name `Symbol` but actually distinct locations.
1 parent 9a23196 commit 0cdc58a

File tree

23 files changed

+77
-75
lines changed

23 files changed

+77
-75
lines changed

‎src/librustc/hir/lowering.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2863,7 +2863,7 @@ impl<'a> LoweringContext<'a> {
28632863
let parent_def = self.parent_def.unwrap();
28642864
let def_id = {
28652865
let defs = self.resolver.definitions();
2866-
let def_path_data = DefPathData::Binding(name);
2866+
let def_path_data = DefPathData::Binding(name.as_str());
28672867
let def_index = defs.create_def_with_parent(parent_def,
28682868
node_id,
28692869
def_path_data,

‎src/librustc/hir/map/def_collector.rs

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -104,14 +104,14 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> {
104104
DefPathData::Impl,
105105
ItemKind::Enum(..) | ItemKind::Struct(..) | ItemKind::Union(..) | ItemKind::Trait(..) |
106106
ItemKind::ExternCrate(..) | ItemKind::ForeignMod(..) | ItemKind::Ty(..) =>
107-
DefPathData::TypeNs(i.ident.name),
107+
DefPathData::TypeNs(i.ident.name.as_str()),
108108
ItemKind::Mod(..) if i.ident == keywords::Invalid.ident() => {
109109
return visit::walk_item(self, i);
110110
}
111-
ItemKind::Mod(..) => DefPathData::Module(i.ident.name),
111+
ItemKind::Mod(..) => DefPathData::Module(i.ident.name.as_str()),
112112
ItemKind::Static(..) | ItemKind::Const(..) | ItemKind::Fn(..) =>
113-
DefPathData::ValueNs(i.ident.name),
114-
ItemKind::MacroDef(..) => DefPathData::MacroDef(i.ident.name),
113+
DefPathData::ValueNs(i.ident.name.as_str()),
114+
ItemKind::MacroDef(..) => DefPathData::MacroDef(i.ident.name.as_str()),
115115
ItemKind::Mac(..) => return self.visit_macro_invoc(i.id, false),
116116
ItemKind::GlobalAsm(..) => DefPathData::Misc,
117117
ItemKind::Use(ref view_path) => {
@@ -139,13 +139,15 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> {
139139
for v in &enum_definition.variants {
140140
let variant_def_index =
141141
this.create_def(v.node.data.id(),
142-
DefPathData::EnumVariant(v.node.name.name),
142+
DefPathData::EnumVariant(v.node.name.name.as_str()),
143143
REGULAR_SPACE);
144144
this.with_parent(variant_def_index, |this| {
145145
for (index, field) in v.node.data.fields().iter().enumerate() {
146146
let name = field.ident.map(|ident| ident.name)
147147
.unwrap_or_else(|| Symbol::intern(&index.to_string()));
148-
this.create_def(field.id, DefPathData::Field(name), REGULAR_SPACE);
148+
this.create_def(field.id,
149+
DefPathData::Field(name.as_str()),
150+
REGULAR_SPACE);
149151
}
150152

151153
if let Some(ref expr) = v.node.disr_expr {
@@ -165,7 +167,7 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> {
165167
for (index, field) in struct_def.fields().iter().enumerate() {
166168
let name = field.ident.map(|ident| ident.name)
167169
.unwrap_or_else(|| Symbol::intern(&index.to_string()));
168-
this.create_def(field.id, DefPathData::Field(name), REGULAR_SPACE);
170+
this.create_def(field.id, DefPathData::Field(name.as_str()), REGULAR_SPACE);
169171
}
170172
}
171173
_ => {}
@@ -176,7 +178,7 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> {
176178

177179
fn visit_foreign_item(&mut self, foreign_item: &'a ForeignItem) {
178180
let def = self.create_def(foreign_item.id,
179-
DefPathData::ValueNs(foreign_item.ident.name),
181+
DefPathData::ValueNs(foreign_item.ident.name.as_str()),
180182
REGULAR_SPACE);
181183

182184
self.with_parent(def, |this| {
@@ -187,7 +189,7 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> {
187189
fn visit_generics(&mut self, generics: &'a Generics) {
188190
for ty_param in generics.ty_params.iter() {
189191
self.create_def(ty_param.id,
190-
DefPathData::TypeParam(ty_param.ident.name),
192+
DefPathData::TypeParam(ty_param.ident.name.as_str()),
191193
REGULAR_SPACE);
192194
}
193195

@@ -197,8 +199,8 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> {
197199
fn visit_trait_item(&mut self, ti: &'a TraitItem) {
198200
let def_data = match ti.node {
199201
TraitItemKind::Method(..) | TraitItemKind::Const(..) =>
200-
DefPathData::ValueNs(ti.ident.name),
201-
TraitItemKind::Type(..) => DefPathData::TypeNs(ti.ident.name),
202+
DefPathData::ValueNs(ti.ident.name.as_str()),
203+
TraitItemKind::Type(..) => DefPathData::TypeNs(ti.ident.name.as_str()),
202204
TraitItemKind::Macro(..) => return self.visit_macro_invoc(ti.id, false),
203205
};
204206

@@ -215,8 +217,8 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> {
215217
fn visit_impl_item(&mut self, ii: &'a ImplItem) {
216218
let def_data = match ii.node {
217219
ImplItemKind::Method(..) | ImplItemKind::Const(..) =>
218-
DefPathData::ValueNs(ii.ident.name),
219-
ImplItemKind::Type(..) => DefPathData::TypeNs(ii.ident.name),
220+
DefPathData::ValueNs(ii.ident.name.as_str()),
221+
ImplItemKind::Type(..) => DefPathData::TypeNs(ii.ident.name.as_str()),
220222
ImplItemKind::Macro(..) => return self.visit_macro_invoc(ii.id, false),
221223
};
222224

@@ -237,7 +239,7 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> {
237239
PatKind::Mac(..) => return self.visit_macro_invoc(pat.id, false),
238240
PatKind::Ident(_, id, _) => {
239241
let def = self.create_def(pat.id,
240-
DefPathData::Binding(id.node.name),
242+
DefPathData::Binding(id.node.name.as_str()),
241243
REGULAR_SPACE);
242244
self.parent_def = Some(def);
243245
}
@@ -282,7 +284,7 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> {
282284

283285
fn visit_lifetime_def(&mut self, def: &'a LifetimeDef) {
284286
self.create_def(def.lifetime.id,
285-
DefPathData::LifetimeDef(def.lifetime.ident.name),
287+
DefPathData::LifetimeDef(def.lifetime.ident.name.as_str()),
286288
REGULAR_SPACE);
287289
}
288290

‎src/librustc/hir/map/definitions.rs

Lines changed: 19 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -80,8 +80,10 @@ impl DefPathTable {
8080

8181
#[inline(always)]
8282
pub fn def_path_hash(&self, index: DefIndex) -> DefPathHash {
83-
self.def_path_hashes[index.address_space().index()]
84-
[index.as_array_index()]
83+
let ret = self.def_path_hashes[index.address_space().index()]
84+
[index.as_array_index()];
85+
debug!("def_path_hash({:?}) = {:?}", index, ret);
86+
return ret
8587
}
8688

8789
pub fn add_def_path_hashes_to(&self,
@@ -213,7 +215,7 @@ impl DefKey {
213215
DefPathData::Binding(name) |
214216
DefPathData::Field(name) |
215217
DefPathData::GlobalMetaData(name) => {
216-
(*name.as_str()).hash(&mut hasher);
218+
name.hash(&mut hasher);
217219
}
218220

219221
DefPathData::Impl |
@@ -347,31 +349,31 @@ pub enum DefPathData {
347349
/// An impl
348350
Impl,
349351
/// Something in the type NS
350-
TypeNs(Symbol),
352+
TypeNs(InternedString),
351353
/// Something in the value NS
352-
ValueNs(Symbol),
354+
ValueNs(InternedString),
353355
/// A module declaration
354-
Module(Symbol),
356+
Module(InternedString),
355357
/// A macro rule
356-
MacroDef(Symbol),
358+
MacroDef(InternedString),
357359
/// A closure expression
358360
ClosureExpr,
359361

360362
// Subportions of items
361363
/// A type parameter (generic parameter)
362-
TypeParam(Symbol),
364+
TypeParam(InternedString),
363365
/// A lifetime definition
364-
LifetimeDef(Symbol),
366+
LifetimeDef(InternedString),
365367
/// A variant of a enum
366-
EnumVariant(Symbol),
368+
EnumVariant(InternedString),
367369
/// A struct field
368-
Field(Symbol),
370+
Field(InternedString),
369371
/// Implicit ctor for a tuple-like struct
370372
StructCtor,
371373
/// Initializer for a const
372374
Initializer,
373375
/// Pattern binding
374-
Binding(Symbol),
376+
Binding(InternedString),
375377
/// An `impl Trait` type node.
376378
ImplTrait,
377379
/// A `typeof` type node.
@@ -380,7 +382,7 @@ pub enum DefPathData {
380382
/// GlobalMetaData identifies a piece of crate metadata that is global to
381383
/// a whole crate (as opposed to just one item). GlobalMetaData components
382384
/// are only supposed to show up right below the crate root.
383-
GlobalMetaData(Symbol)
385+
GlobalMetaData(InternedString)
384386
}
385387

386388
#[derive(Copy, Clone, Hash, PartialEq, Eq, PartialOrd, Ord, Debug,
@@ -601,7 +603,7 @@ impl Definitions {
601603
}
602604

603605
impl DefPathData {
604-
pub fn get_opt_name(&self) -> Option<Symbol> {
606+
pub fn get_opt_name(&self) -> Option<InternedString> {
605607
use self::DefPathData::*;
606608
match *self {
607609
TypeNs(name) |
@@ -639,7 +641,7 @@ impl DefPathData {
639641
Binding(name) |
640642
Field(name) |
641643
GlobalMetaData(name) => {
642-
return name.as_str();
644+
return name
643645
}
644646

645647
// note that this does not show up in user printouts
@@ -684,7 +686,7 @@ macro_rules! define_global_metadata_kind {
684686
definitions.create_def_with_parent(
685687
CRATE_DEF_INDEX,
686688
ast::DUMMY_NODE_ID,
687-
DefPathData::GlobalMetaData(instance.name()),
689+
DefPathData::GlobalMetaData(instance.name().as_str()),
688690
GLOBAL_MD_ADDRESS_SPACE,
689691
Mark::root()
690692
);
@@ -698,7 +700,7 @@ macro_rules! define_global_metadata_kind {
698700
let def_key = DefKey {
699701
parent: Some(CRATE_DEF_INDEX),
700702
disambiguated_data: DisambiguatedDefPathData {
701-
data: DefPathData::GlobalMetaData(self.name()),
703+
data: DefPathData::GlobalMetaData(self.name().as_str()),
702704
disambiguator: 0,
703705
}
704706
};

‎src/librustc/traits/error_reporting.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
348348
//
349349
// Currently I'm leaving it for what I need for `try`.
350350
if self.tcx.trait_of_item(item) == Some(trait_ref.def_id) {
351-
method = self.tcx.item_name(item).as_str();
351+
method = self.tcx.item_name(item);
352352
flags.push(("from_method", None));
353353
flags.push(("from_method", Some(&*method)));
354354
}

‎src/librustc/traits/on_unimplemented.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,7 @@ impl<'a, 'gcx, 'tcx> OnUnimplementedFormatString {
227227
span: Span)
228228
-> Result<(), ErrorReported>
229229
{
230-
let name = tcx.item_name(trait_def_id).as_str();
230+
let name = tcx.item_name(trait_def_id);
231231
let generics = tcx.generics_of(trait_def_id);
232232
let parser = Parser::new(&self.0);
233233
let types = &generics.types;
@@ -272,7 +272,7 @@ impl<'a, 'gcx, 'tcx> OnUnimplementedFormatString {
272272
trait_ref: ty::TraitRef<'tcx>)
273273
-> String
274274
{
275-
let name = tcx.item_name(trait_ref.def_id).as_str();
275+
let name = tcx.item_name(trait_ref.def_id);
276276
let trait_str = tcx.item_path_str(trait_ref.def_id);
277277
let generics = tcx.generics_of(trait_ref.def_id);
278278
let generic_map = generics.types.iter().map(|param| {

‎src/librustc/ty/item_path.rs

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ use hir::def_id::{CrateNum, DefId, CRATE_DEF_INDEX, LOCAL_CRATE};
1313
use ty::{self, Ty, TyCtxt};
1414
use syntax::ast;
1515
use syntax::symbol::Symbol;
16+
use syntax::symbol::InternedString;
1617

1718
use std::cell::Cell;
1819

@@ -130,20 +131,20 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
130131
{
131132
let visible_parent_map = self.visible_parent_map(LOCAL_CRATE);
132133

133-
let (mut cur_def, mut cur_path) = (external_def_id, Vec::<ast::Name>::new());
134+
let (mut cur_def, mut cur_path) = (external_def_id, Vec::<InternedString>::new());
134135
loop {
135136
// If `cur_def` is a direct or injected extern crate, push the path to the crate
136137
// followed by the path to the item within the crate and return.
137138
if cur_def.index == CRATE_DEF_INDEX {
138139
match *self.extern_crate(cur_def) {
139140
Some(ref extern_crate) if extern_crate.direct => {
140141
self.push_item_path(buffer, extern_crate.def_id);
141-
cur_path.iter().rev().map(|segment| buffer.push(&segment.as_str())).count();
142+
cur_path.iter().rev().map(|segment| buffer.push(&segment)).count();
142143
return true;
143144
}
144145
None => {
145146
buffer.push(&self.crate_name(cur_def.krate).as_str());
146-
cur_path.iter().rev().map(|segment| buffer.push(&segment.as_str())).count();
147+
cur_path.iter().rev().map(|segment| buffer.push(&segment)).count();
147148
return true;
148149
}
149150
_ => {},
@@ -152,7 +153,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
152153

153154
cur_path.push(self.sess.cstore.def_key(cur_def)
154155
.disambiguated_data.data.get_opt_name().unwrap_or_else(||
155-
Symbol::intern("<unnamed>")));
156+
Symbol::intern("<unnamed>").as_str()));
156157
match visible_parent_map.get(&cur_def) {
157158
Some(&def) => cur_def = def,
158159
None => return false,

‎src/librustc/ty/maps.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ use middle::cstore::{ExternCrate, LinkagePreference, NativeLibrary};
2020
use middle::cstore::{NativeLibraryKind, DepKind, CrateSource};
2121
use middle::privacy::AccessLevels;
2222
use middle::region;
23-
use middle::region::RegionMaps;
2423
use middle::resolve_lifetime::{Region, ObjectLifetimeDefault};
2524
use middle::stability::{self, DeprecationEntry};
2625
use middle::lang_items::{LanguageItems, LangItem};

‎src/librustc/ty/mod.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2206,11 +2206,11 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
22062206
}
22072207
}
22082208

2209-
pub fn item_name(self, id: DefId) -> ast::Name {
2209+
pub fn item_name(self, id: DefId) -> InternedString {
22102210
if let Some(id) = self.hir.as_local_node_id(id) {
2211-
self.hir.name(id)
2211+
self.hir.name(id).as_str()
22122212
} else if id.index == CRATE_DEF_INDEX {
2213-
self.original_crate_name(id.krate)
2213+
self.original_crate_name(id.krate).as_str()
22142214
} else {
22152215
let def_key = self.sess.cstore.def_key(id);
22162216
// The name of a StructCtor is that of its struct parent.

‎src/librustc_const_eval/eval.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -327,7 +327,7 @@ fn eval_const_expr_partial<'a, 'tcx>(cx: &ConstContext<'a, 'tcx>,
327327
ConstEvalErr { span: e.span, kind: LayoutError(err) }
328328
})
329329
};
330-
match &tcx.item_name(def_id).as_str()[..] {
330+
match &tcx.item_name(def_id)[..] {
331331
"size_of" => {
332332
let size = layout_of(substs.type_at(0))?.size(tcx);
333333
return Ok(Integral(Usize(ConstUsize::new(size.bytes(),

‎src/librustc_driver/test.rs

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ use rustc_trans;
1818
use rustc::middle::free_region::FreeRegionMap;
1919
use rustc::middle::region;
2020
use rustc::middle::resolve_lifetime;
21-
use rustc::middle::stability;
2221
use rustc::ty::subst::{Kind, Subst};
2322
use rustc::traits::{ObligationCause, Reveal};
2423
use rustc::ty::{self, Ty, TyCtxt, TypeFoldable};
@@ -140,7 +139,6 @@ fn test_env<F>(source_string: &str,
140139

141140
// run just enough stuff to build a tcx:
142141
let named_region_map = resolve_lifetime::krate(&sess, &hir_map);
143-
let index = stability::Index::new(&sess);
144142
TyCtxt::create_and_enter(&sess,
145143
ty::maps::Providers::default(),
146144
ty::maps::Providers::default(),
@@ -150,7 +148,6 @@ fn test_env<F>(source_string: &str,
150148
resolutions,
151149
named_region_map.unwrap(),
152150
hir_map,
153-
index,
154151
"test_crate",
155152
|tcx| {
156153
tcx.infer_ctxt().enter(|infcx| {

‎src/librustc_metadata/cstore_impl.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@ use syntax::parse::filemap_to_stream;
4040
use syntax::symbol::Symbol;
4141
use syntax_pos::{Span, NO_EXPANSION};
4242
use rustc_data_structures::indexed_set::IdxSetBuf;
43-
use rustc::hir::svh::Svh;
4443
use rustc::hir;
4544

4645
macro_rules! provide {
@@ -469,7 +468,7 @@ impl CrateStore for cstore::CStore {
469468
.insert(local_span, (name.to_string(), data.get_span(id.index, sess)));
470469

471470
LoadedMacro::MacroDef(ast::Item {
472-
ident: ast::Ident::with_empty_ctxt(name),
471+
ident: ast::Ident::from_str(&name),
473472
id: ast::DUMMY_NODE_ID,
474473
span: local_span,
475474
attrs: attrs.iter().cloned().collect(),

0 commit comments

Comments
 (0)