From c44f72430c7bf0c254fdb8f8a600c1825984e5a3 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Mon, 16 Jul 2018 21:25:52 +0300 Subject: [PATCH 1/8] resolve: Remove unused parameter from `resolve_ident_in_module` --- src/librustc_resolve/build_reduced_graph.rs | 3 +-- src/librustc_resolve/lib.rs | 11 +++++------ src/librustc_resolve/resolve_imports.rs | 5 ++--- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index e00919547fc43..88641011335fd 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -704,8 +704,7 @@ impl<'a> Resolver<'a> { } else { for (name, span) in legacy_imports.imports { let ident = Ident::with_empty_ctxt(name); - let result = self.resolve_ident_in_module(module, ident, MacroNS, - false, false, span); + let result = self.resolve_ident_in_module(module, ident, MacroNS, false, span); if let Ok(binding) = result { let directive = macro_use_directive(span); self.potentially_unused_imports.push(directive); diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index d98434796d5bd..9cf179b880cb2 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -2002,7 +2002,6 @@ impl<'a> Resolver<'a> { module: Module<'a>, mut ident: Ident, ns: Namespace, - ignore_unresolved_invocations: bool, record_used: bool, span: Span) -> Result<&'a NameBinding<'a>, Determinacy> { @@ -2012,7 +2011,7 @@ impl<'a> Resolver<'a> { self.current_module = self.macro_def_scope(def); } let result = self.resolve_ident_in_module_unadjusted( - module, ident, ns, ignore_unresolved_invocations, record_used, span, + module, ident, ns, false, record_used, span, ); self.current_module = orig_current_module; result @@ -2518,7 +2517,7 @@ impl<'a> Resolver<'a> { // If there is a TraitRef in scope for an impl, then the method must be in the // trait. if let Some((module, _)) = self.current_trait_ref { - if self.resolve_ident_in_module(module, ident, ns, false, false, span).is_err() { + if self.resolve_ident_in_module(module, ident, ns, false, span).is_err() { let path = &self.current_trait_ref.as_ref().unwrap().1.path; resolve_error(self, span, err(ident.name, &path_names_to_string(path))); } @@ -3468,7 +3467,7 @@ impl<'a> Resolver<'a> { } let binding = if let Some(module) = module { - self.resolve_ident_in_module(module, ident, ns, false, record_used, path_span) + self.resolve_ident_in_module(module, ident, ns, record_used, path_span) } else if opt_ns == Some(MacroNS) { self.resolve_lexical_macro_path_segment(ident, ns, record_used, path_span) .map(MacroBinding::binding) @@ -3762,7 +3761,7 @@ impl<'a> Resolver<'a> { // Look for associated items in the current trait. if let Some((module, _)) = self.current_trait_ref { if let Ok(binding) = - self.resolve_ident_in_module(module, ident, ns, false, false, module.span) { + self.resolve_ident_in_module(module, ident, ns, false, module.span) { let def = binding.def(); if filter_fn(def) { return Some(if self.has_self.contains(&def.def_id()) { @@ -4075,7 +4074,7 @@ impl<'a> Resolver<'a> { let mut found_traits = Vec::new(); // Look for the current trait. if let Some((module, _)) = self.current_trait_ref { - if self.resolve_ident_in_module(module, ident, ns, false, false, module.span).is_ok() { + if self.resolve_ident_in_module(module, ident, ns, false, module.span).is_ok() { let def_id = module.def_id().unwrap(); found_traits.push(TraitCandidate { def_id: def_id, import_id: None }); } diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 0ee17ebc48704..329926389a8a6 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -244,7 +244,7 @@ impl<'a> Resolver<'a> { SingleImport { source, .. } => source, _ => unreachable!(), }; - match this.resolve_ident_in_module(module, ident, ns, false, false, path_span) { + match this.resolve_ident_in_module(module, ident, ns, false, path_span) { Err(Determined) => {} _ => return false, } @@ -630,7 +630,6 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { source, ns, false, - false, directive.span)); } else { return @@ -803,7 +802,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { if all_ns_err { let mut all_ns_failed = true; self.per_ns(|this, ns| if !type_ns_only || ns == TypeNS { - match this.resolve_ident_in_module(module, ident, ns, false, true, span) { + match this.resolve_ident_in_module(module, ident, ns, true, span) { Ok(_) => all_ns_failed = false, _ => {} } From 22143491bc44287b42b6dfa22e9fd702a9d76a0c Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Mon, 16 Jul 2018 21:36:13 +0300 Subject: [PATCH 2/8] resolve: Rename `global_macros` to `macro_prelude` Rename `shadows_glob` to `shadowed_glob` --- src/librustc_resolve/build_reduced_graph.rs | 2 +- src/librustc_resolve/lib.rs | 6 +++--- src/librustc_resolve/macros.rs | 14 +++++++------- src/librustc_resolve/resolve_imports.rs | 8 ++++---- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 88641011335fd..da2847dc55793 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -651,7 +651,7 @@ impl<'a> Resolver<'a> { binding: &'a NameBinding<'a>, span: Span, allow_shadowing: bool) { - if self.global_macros.insert(name, binding).is_some() && !allow_shadowing { + if self.macro_prelude.insert(name, binding).is_some() && !allow_shadowing { let msg = format!("`{}` is already in scope", name); let note = "macro-expanded `#[macro_use]`s may not shadow existing macros (see RFC 1560)"; diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 9cf179b880cb2..9fe25aaa6c01f 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -1393,7 +1393,7 @@ pub struct Resolver<'a> { crate_loader: &'a mut dyn CrateLoader, macro_names: FxHashSet, - global_macros: FxHashMap>, + macro_prelude: FxHashMap>, pub all_macros: FxHashMap, lexical_macro_resolutions: Vec<(Ident, &'a Cell>)>, macro_map: FxHashMap>, @@ -1715,7 +1715,7 @@ impl<'a> Resolver<'a> { crate_loader, macro_names: FxHashSet(), - global_macros: FxHashMap(), + macro_prelude: FxHashMap(), all_macros: FxHashMap(), lexical_macro_resolutions: Vec::new(), macro_map: FxHashMap(), @@ -3224,7 +3224,7 @@ impl<'a> Resolver<'a> { }; } } - let is_global = self.global_macros.get(&path[0].name).cloned() + let is_global = self.macro_prelude.get(&path[0].name).cloned() .map(|binding| binding.get_macro(self).kind() == MacroKind::Bang).unwrap_or(false); if primary_ns != MacroNS && (is_global || self.macro_names.contains(&path[0].modern())) { diff --git a/src/librustc_resolve/macros.rs b/src/librustc_resolve/macros.rs index f076d884f6099..c85115c62f8eb 100644 --- a/src/librustc_resolve/macros.rs +++ b/src/librustc_resolve/macros.rs @@ -220,7 +220,7 @@ impl<'a> base::Resolver for Resolver<'a> { vis: ty::Visibility::Invisible, expansion: Mark::root(), }); - self.global_macros.insert(ident.name, binding); + self.macro_prelude.insert(ident.name, binding); } fn resolve_imports(&mut self) { @@ -238,7 +238,7 @@ impl<'a> base::Resolver for Resolver<'a> { attr::mark_known(&attrs[i]); } - match self.global_macros.get(&name).cloned() { + match self.macro_prelude.get(&name).cloned() { Some(binding) => match *binding.get_macro(self) { MultiModifier(..) | MultiDecorator(..) | SyntaxExtension::AttrProcMacro(..) => { return Some(attrs.remove(i)) @@ -274,7 +274,7 @@ impl<'a> base::Resolver for Resolver<'a> { } let trait_name = traits[j].segments[0].ident.name; let legacy_name = Symbol::intern(&format!("derive_{}", trait_name)); - if !self.global_macros.contains_key(&legacy_name) { + if !self.macro_prelude.contains_key(&legacy_name) { continue } let span = traits.remove(j).span; @@ -565,7 +565,7 @@ impl<'a> Resolver<'a> { module, ident, ns, true, record_used, path_span, ).map(MacroBinding::Modern) } else { - self.global_macros.get(&ident.name).cloned().ok_or(determinacy) + self.macro_prelude.get(&ident.name).cloned().ok_or(determinacy) .map(MacroBinding::Global) }; self.current_module = orig_current_module; @@ -652,7 +652,7 @@ impl<'a> Resolver<'a> { let binding = if let Some(binding) = binding { MacroBinding::Legacy(binding) - } else if let Some(binding) = self.global_macros.get(&ident.name).cloned() { + } else if let Some(binding) = self.macro_prelude.get(&ident.name).cloned() { if !self.use_extern_macros { self.record_use(ident, MacroNS, binding, DUMMY_SP); } @@ -762,8 +762,8 @@ impl<'a> Resolver<'a> { // Then check global macros. }.or_else(|| { // FIXME: get_macro needs an &mut Resolver, can we do it without cloning? - let global_macros = self.global_macros.clone(); - let names = global_macros.iter().filter_map(|(name, binding)| { + let macro_prelude = self.macro_prelude.clone(); + let names = macro_prelude.iter().filter_map(|(name, binding)| { if binding.get_macro(self).kind() == kind { Some(name) } else { diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 329926389a8a6..ed85105d19f8b 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -108,7 +108,7 @@ pub struct NameResolution<'a> { single_imports: SingleImports<'a>, /// The least shadowable known binding for this name, or None if there are no known bindings. pub binding: Option<&'a NameBinding<'a>>, - shadows_glob: Option<&'a NameBinding<'a>>, + shadowed_glob: Option<&'a NameBinding<'a>>, } #[derive(Clone, Debug)] @@ -194,7 +194,7 @@ impl<'a> Resolver<'a> { if record_used { if let Some(binding) = resolution.binding { - if let Some(shadowed_glob) = resolution.shadows_glob { + if let Some(shadowed_glob) = resolution.shadowed_glob { let name = ident.name; // Forbid expanded shadowing to avoid time travel. if restricted_shadowing && @@ -401,7 +401,7 @@ impl<'a> Resolver<'a> { if binding.is_glob_import() { if !old_binding.is_glob_import() && !(ns == MacroNS && old_binding.expansion != Mark::root()) { - resolution.shadows_glob = Some(binding); + resolution.shadowed_glob = Some(binding); } else if binding.def() != old_binding.def() { resolution.binding = Some(this.ambiguity(old_binding, binding)); } else if !old_binding.vis.is_at_least(binding.vis, &*this) { @@ -414,7 +414,7 @@ impl<'a> Resolver<'a> { resolution.binding = Some(this.ambiguity(binding, old_binding)); } else { resolution.binding = Some(binding); - resolution.shadows_glob = Some(old_binding); + resolution.shadowed_glob = Some(old_binding); } } else { return Err(old_binding); From c2533b64a32b44dfde9b6a8a2301403a6568c313 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Tue, 17 Jul 2018 04:12:48 +0300 Subject: [PATCH 3/8] resolve: Remove `SingleImports` in favor of a simple set --- src/librustc_resolve/resolve_imports.rs | 125 +++++------------------- 1 file changed, 26 insertions(+), 99 deletions(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index ed85105d19f8b..1b8d1e849b329 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -33,7 +33,7 @@ use syntax::util::lev_distance::find_best_match_for_name; use syntax_pos::Span; use std::cell::{Cell, RefCell}; -use std::{mem, ptr}; +use std::mem; /// Contains data for specific types of import directives. #[derive(Clone, Debug)] @@ -104,67 +104,20 @@ impl<'a> ImportDirective<'a> { #[derive(Clone, Default, Debug)] /// Records information about the resolution of a name in a namespace of a module. pub struct NameResolution<'a> { - /// The single imports that define the name in the namespace. - single_imports: SingleImports<'a>, + /// Single imports that may define the name in the namespace. + /// Import directives are arena-allocated, so it's ok to use pointers as keys, they are stable. + single_imports: FxHashSet<*const ImportDirective<'a>>, /// The least shadowable known binding for this name, or None if there are no known bindings. pub binding: Option<&'a NameBinding<'a>>, shadowed_glob: Option<&'a NameBinding<'a>>, } -#[derive(Clone, Debug)] -enum SingleImports<'a> { - /// No single imports can define the name in the namespace. - None, - /// Only the given single import can define the name in the namespace. - MaybeOne(&'a ImportDirective<'a>), - /// Only one of these two single imports can define the name in the namespace. - MaybeTwo(&'a ImportDirective<'a>, &'a ImportDirective<'a>), - /// At least one single import will define the name in the namespace. - AtLeastOne, -} - -impl<'a> Default for SingleImports<'a> { - /// Creates a `SingleImports<'a>` of None type. - fn default() -> Self { - SingleImports::None - } -} - -impl<'a> SingleImports<'a> { - fn add_directive(&mut self, directive: &'a ImportDirective<'a>, use_extern_macros: bool) { - match *self { - SingleImports::None => *self = SingleImports::MaybeOne(directive), - SingleImports::MaybeOne(directive_one) => *self = if use_extern_macros { - SingleImports::MaybeTwo(directive_one, directive) - } else { - SingleImports::AtLeastOne - }, - // If three single imports can define the name in the namespace, we can assume that at - // least one of them will define it since otherwise we'd get duplicate errors in one of - // other namespaces. - SingleImports::MaybeTwo(..) => *self = SingleImports::AtLeastOne, - SingleImports::AtLeastOne => {} - }; - } - - fn directive_failed(&mut self, dir: &'a ImportDirective<'a>) { - match *self { - SingleImports::None => unreachable!(), - SingleImports::MaybeOne(_) => *self = SingleImports::None, - SingleImports::MaybeTwo(dir1, dir2) => - *self = SingleImports::MaybeOne(if ptr::eq(dir1, dir) { dir1 } else { dir2 }), - SingleImports::AtLeastOne => {} - } - } -} - impl<'a> NameResolution<'a> { // Returns the binding for the name if it is known or None if it not known. fn binding(&self) -> Option<&'a NameBinding<'a>> { - self.binding.and_then(|binding| match self.single_imports { - SingleImports::None => Some(binding), - _ if !binding.is_glob_import() => Some(binding), - _ => None, // The binding could be shadowed by a single import, so it is not known. + self.binding.and_then(|binding| { + if !binding.is_glob_import() || + self.single_imports.is_empty() { Some(binding) } else { None } }) } } @@ -227,58 +180,31 @@ impl<'a> Resolver<'a> { if usable { Ok(binding) } else { Err(Determined) } }; - // Items and single imports are not shadowable. + // Items and single imports are not shadowable, if we have one, then it's determined. if let Some(binding) = resolution.binding { if !binding.is_glob_import() { return check_usable(self, binding); } } - // Check if a single import can still define the name. - let resolve_single_import = |this: &mut Self, directive: &'a ImportDirective<'a>| { - let module = match directive.imported_module.get() { - Some(module) => module, - None => return false, - }; - let ident = match directive.subclass { + // From now on we either have a glob resolution or no resolution. + + // Check if one of single imports can still define the name, + // if it can then our result is not determined and can be invalidated. + for single_import in &resolution.single_imports { + let single_import = unsafe { &**single_import }; + if !self.is_accessible(single_import.vis.get()) { + continue; + } + let module = unwrap_or!(single_import.imported_module.get(), return Err(Undetermined)); + let ident = match single_import.subclass { SingleImport { source, .. } => source, _ => unreachable!(), }; - match this.resolve_ident_in_module(module, ident, ns, false, path_span) { - Err(Determined) => {} - _ => return false, - } - true - }; - match resolution.single_imports { - SingleImports::AtLeastOne => return Err(Undetermined), - SingleImports::MaybeOne(directive) => { - let accessible = self.is_accessible(directive.vis.get()); - if accessible { - if !resolve_single_import(self, directive) { - return Err(Undetermined) - } - } - } - SingleImports::MaybeTwo(directive1, directive2) => { - let accessible1 = self.is_accessible(directive1.vis.get()); - let accessible2 = self.is_accessible(directive2.vis.get()); - if accessible1 && accessible2 { - if !resolve_single_import(self, directive1) && - !resolve_single_import(self, directive2) { - return Err(Undetermined) - } - } else if accessible1 { - if !resolve_single_import(self, directive1) { - return Err(Undetermined) - } - } else { - if !resolve_single_import(self, directive2) { - return Err(Undetermined) - } - } + match self.resolve_ident_in_module(module, ident, ns, false, path_span) { + Err(Determined) => continue, + Ok(_) | Err(Undetermined) => return Err(Undetermined), } - SingleImports::None => {}, } let no_unresolved_invocations = @@ -348,7 +274,7 @@ impl<'a> Resolver<'a> { SingleImport { target, type_ns_only, .. } => { self.per_ns(|this, ns| if !type_ns_only || ns == TypeNS { let mut resolution = this.resolution(current_module, target, ns).borrow_mut(); - resolution.single_imports.add_directive(directive, this.use_extern_macros); + resolution.single_imports.insert(directive); }); } // We don't add prelude imports to the globs since they only affect lexical scopes, @@ -640,7 +566,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { Err(Undetermined) => indeterminate = true, Err(Determined) => { this.update_resolution(parent, target, ns, |_, resolution| { - resolution.single_imports.directive_failed(directive) + resolution.single_imports.remove(&(directive as *const _)); }); } Ok(binding) if !binding.is_importable() => { @@ -826,7 +752,8 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { _ => Some(&i.name), } }, - NameResolution { single_imports: SingleImports::None, .. } => None, + NameResolution { ref single_imports, .. } + if single_imports.is_empty() => None, _ => Some(&i.name), } }); From 414a86e7598b339f8b2681d0aa090d9fb4b6f0e1 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Wed, 18 Jul 2018 03:08:49 +0300 Subject: [PATCH 4/8] resolve: Add some comments to in-module resolution --- src/librustc_resolve/resolve_imports.rs | 45 +++++++++++++++++-------- 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 1b8d1e849b329..c242b9c7f2f4d 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -130,7 +130,7 @@ impl<'a> Resolver<'a> { } /// Attempts to resolve `ident` in namespaces `ns` of `module`. - /// Invariant: if `record_used` is `Some`, import resolution must be complete. + /// Invariant: if `record_used` is `Some`, expansion and import resolution must be complete. pub fn resolve_ident_in_module_unadjusted(&mut self, module: Module<'a>, ident: Ident, @@ -187,7 +187,7 @@ impl<'a> Resolver<'a> { } } - // From now on we either have a glob resolution or no resolution. + // --- From now on we either have a glob resolution or no resolution. --- // Check if one of single imports can still define the name, // if it can then our result is not determined and can be invalidated. @@ -207,27 +207,43 @@ impl<'a> Resolver<'a> { } } - let no_unresolved_invocations = - restricted_shadowing || module.unresolved_invocations.borrow().is_empty(); + let no_unexpanded_macros = module.unresolved_invocations.borrow().is_empty(); match resolution.binding { - // In `MacroNS`, expanded bindings do not shadow (enforced in `try_define`). - Some(binding) if no_unresolved_invocations || ns == MacroNS => + // So we have a resolution that's from a glob import. This resolution is determined + // if it cannot be shadowed by some new item/import expanded from a macro. + // This happens either if there are no unexpanded macros, or expanded names cannot + // shadow globs (that happens in macro namespace or with restricted shadowing). + Some(binding) if no_unexpanded_macros || ns == MacroNS || restricted_shadowing => return check_usable(self, binding), - None if no_unresolved_invocations => {} + // If we have no resolution, then it's a determined error it some new item/import + // cannot appear from a macro expansion or an undetermined glob. + None if no_unexpanded_macros => {} // go check for globs below + // This is actually an undetermined error, but we need to return determinate error + // due to subtle interactions with `resolve_lexical_macro_path_segment` + // that are going to be removed in the next commit. + None if restricted_shadowing => {} // go check for globs below _ => return Err(Undetermined), } - // Check if the globs are determined + // --- From now on we have no resolution. --- + + // Check if one of glob imports can still define the name, + // if it can then our "no resolution" result is not determined and can be invalidated. + + // What on earth is this? + // Apparently one more subtle interaction with `resolve_lexical_macro_path_segment` + // that are going to be removed in the next commit. if restricted_shadowing && module.def().is_some() { return Err(Determined); } - for directive in module.globs.borrow().iter() { - if !self.is_accessible(directive.vis.get()) { + + for glob_import in module.globs.borrow().iter() { + if !self.is_accessible(glob_import.vis.get()) { continue } - let module = unwrap_or!(directive.imported_module.get(), return Err(Undetermined)); + let module = unwrap_or!(glob_import.imported_module.get(), return Err(Undetermined)); let (orig_current_module, mut ident) = (self.current_module, ident.modern()); - match ident.span.glob_adjust(module.expansion, directive.span.ctxt().modern()) { + match ident.span.glob_adjust(module.expansion, glob_import.span.ctxt().modern()) { Some(Some(def)) => self.current_module = self.macro_def_scope(def), Some(None) => {} None => continue, @@ -236,8 +252,9 @@ impl<'a> Resolver<'a> { module, ident, ns, false, false, path_span, ); self.current_module = orig_current_module; - if let Err(Undetermined) = result { - return Err(Undetermined); + match result { + Err(Determined) => continue, + Ok(_) | Err(Undetermined) => return Err(Undetermined), } } From 2eb83ee527fdb4bcd740ea41c43e9d2d52f99c1c Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Wed, 18 Jul 2018 21:53:54 +0300 Subject: [PATCH 5/8] data_structures: Add a reference wrapper for pointer-indexed maps/sets Use `ptr::eq` for comparing pointers --- src/liballoc/collections/btree/node.rs | 2 +- src/librustc/lint/mod.rs | 4 +-- src/librustc/ty/mod.rs | 9 +++-- src/librustc/ty/query/job.rs | 9 ++--- src/librustc_data_structures/lib.rs | 21 ++++++------ src/librustc_data_structures/ptr_key.rs | 45 +++++++++++++++++++++++++ src/librustc_resolve/resolve_imports.rs | 16 ++++----- 7 files changed, 76 insertions(+), 30 deletions(-) create mode 100644 src/librustc_data_structures/ptr_key.rs diff --git a/src/liballoc/collections/btree/node.rs b/src/liballoc/collections/btree/node.rs index 19bdcbc6ad63e..0ae45b3123259 100644 --- a/src/liballoc/collections/btree/node.rs +++ b/src/liballoc/collections/btree/node.rs @@ -103,7 +103,7 @@ impl LeafNode { } fn is_shared_root(&self) -> bool { - self as *const _ == &EMPTY_ROOT_NODE as *const _ as *const LeafNode + ptr::eq(self, &EMPTY_ROOT_NODE as *const _ as *const _) } } diff --git a/src/librustc/lint/mod.rs b/src/librustc/lint/mod.rs index e3d35a7c105ea..dc1c9f7c10844 100644 --- a/src/librustc/lint/mod.rs +++ b/src/librustc/lint/mod.rs @@ -39,7 +39,7 @@ use hir::intravisit; use hir; use lint::builtin::BuiltinLintDiagnostics; use session::{Session, DiagnosticMessageId}; -use std::hash; +use std::{hash, ptr}; use syntax::ast; use syntax::codemap::MultiSpan; use syntax::edition::Edition; @@ -354,7 +354,7 @@ pub struct LintId { impl PartialEq for LintId { fn eq(&self, other: &LintId) -> bool { - (self.lint as *const Lint) == (other.lint as *const Lint) + ptr::eq(self.lint, other.lint) } } diff --git a/src/librustc/ty/mod.rs b/src/librustc/ty/mod.rs index 178f0d3cdcbc1..bd24b93f0293f 100644 --- a/src/librustc/ty/mod.rs +++ b/src/librustc/ty/mod.rs @@ -47,7 +47,7 @@ use std::ops::Deref; use rustc_data_structures::sync::{self, Lrc, ParallelIterator, par_iter}; use std::slice; use std::vec::IntoIter; -use std::mem; +use std::{mem, ptr}; use syntax::ast::{self, DUMMY_NODE_ID, Name, Ident, NodeId}; use syntax::attr; use syntax::ext::hygiene::Mark; @@ -527,8 +527,7 @@ impl<'tcx> PartialOrd for TyS<'tcx> { impl<'tcx> PartialEq for TyS<'tcx> { #[inline] fn eq(&self, other: &TyS<'tcx>) -> bool { - // (self as *const _) == (other as *const _) - (self as *const TyS<'tcx>) == (other as *const TyS<'tcx>) + ptr::eq(self, other) } } impl<'tcx> Eq for TyS<'tcx> {} @@ -678,7 +677,7 @@ impl PartialOrd for Slice where T: PartialOrd { impl PartialEq for Slice { #[inline] fn eq(&self, other: &Slice) -> bool { - (self as *const _) == (other as *const _) + ptr::eq(self, other) } } impl Eq for Slice {} @@ -1730,7 +1729,7 @@ impl Ord for AdtDef { impl PartialEq for AdtDef { // AdtDef are always interned and this is part of TyS equality #[inline] - fn eq(&self, other: &Self) -> bool { self as *const _ == other as *const _ } + fn eq(&self, other: &Self) -> bool { ptr::eq(self, other) } } impl Eq for AdtDef {} diff --git a/src/librustc/ty/query/job.rs b/src/librustc/ty/query/job.rs index a54deeca293d8..6cc71642c42ae 100644 --- a/src/librustc/ty/query/job.rs +++ b/src/librustc/ty/query/job.rs @@ -11,6 +11,7 @@ #![allow(warnings)] use std::mem; +use rustc_data_structures::ptr_key::PtrKey; use rustc_data_structures::sync::{Lock, LockGuard, Lrc, Weak}; use rustc_data_structures::OnDrop; use syntax_pos::Span; @@ -20,7 +21,7 @@ use ty::query::plumbing::CycleError; use ty::context::TyCtxt; use errors::Diagnostic; use std::process; -use std::fmt; +use std::{fmt, ptr}; use std::collections::HashSet; #[cfg(parallel_queries)] use { @@ -124,7 +125,7 @@ impl<'tcx> QueryJob<'tcx> { while let Some(job) = current_job { cycle.insert(0, job.info.clone()); - if &*job as *const _ == self as *const _ { + if ptr::eq(&*job, self) { // This is the end of the cycle // The span entry we included was for the usage // of the cycle itself, and not part of the cycle @@ -282,7 +283,7 @@ where fn cycle_check<'tcx>(query: Lrc>, span: Span, stack: &mut Vec<(Span, Lrc>)>, - visited: &mut HashSet<*const QueryJob<'tcx>> + visited: &mut HashSet>> ) -> Option>> { if visited.contains(&query.as_ptr()) { return if let Some(p) = stack.iter().position(|q| q.1.as_ptr() == query.as_ptr()) { @@ -321,7 +322,7 @@ fn cycle_check<'tcx>(query: Lrc>, #[cfg(parallel_queries)] fn connected_to_root<'tcx>( query: Lrc>, - visited: &mut HashSet<*const QueryJob<'tcx>> + visited: &mut HashSet>> ) -> bool { // We already visited this or we're deliberately ignoring it if visited.contains(&query.as_ptr()) { diff --git a/src/librustc_data_structures/lib.rs b/src/librustc_data_structures/lib.rs index b386f887d77f1..ef0d57c7b7ce7 100644 --- a/src/librustc_data_structures/lib.rs +++ b/src/librustc_data_structures/lib.rs @@ -56,29 +56,30 @@ extern crate rustc_cratesio_shim; pub use rustc_serialize::hex::ToHex; -pub mod array_vec; pub mod accumulate_vec; -pub mod small_vec; +pub mod array_vec; pub mod base_n; pub mod bitslice; pub mod bitvec; +pub mod flock; +pub mod fx; +pub mod graph; pub mod indexed_set; pub mod indexed_vec; pub mod obligation_forest; +pub mod owning_ref; +pub mod ptr_key; pub mod sip128; +pub mod small_vec; pub mod snapshot_map; pub use ena::snapshot_vec; +pub mod sorted_map; pub mod stable_hasher; -pub mod transitive_relation; -pub use ena::unify; -pub mod fx; -pub mod tuple_slice; -pub mod graph; -pub mod flock; pub mod sync; -pub mod owning_ref; pub mod tiny_list; -pub mod sorted_map; +pub mod transitive_relation; +pub mod tuple_slice; +pub use ena::unify; pub mod work_queue; pub struct OnDrop(pub F); diff --git a/src/librustc_data_structures/ptr_key.rs b/src/librustc_data_structures/ptr_key.rs new file mode 100644 index 0000000000000..6835dab38df0a --- /dev/null +++ b/src/librustc_data_structures/ptr_key.rs @@ -0,0 +1,45 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use std::{hash, ptr}; +use std::ops::Deref; + +/// A wrapper around reference that compares and hashes like a pointer. +/// Can be used as a key in sets/maps indexed by pointers to avoid `unsafe`. +#[derive(Debug)] +pub struct PtrKey<'a, T: 'a>(pub &'a T); + +impl<'a, T> Clone for PtrKey<'a, T> { + fn clone(&self) -> Self { *self } +} + +impl<'a, T> Copy for PtrKey<'a, T> {} + +impl<'a, T> PartialEq for PtrKey<'a, T> { + fn eq(&self, rhs: &Self) -> bool { + ptr::eq(self.0, rhs.0) + } +} + +impl<'a, T> Eq for PtrKey<'a, T> {} + +impl<'a, T> hash::Hash for PtrKey<'a, T> { + fn hash(&self, hasher: &mut H) { + (self.0 as *const T).hash(hasher) + } +} + +impl<'a, T> Deref for PtrKey<'a, T> { + type Target = T; + + fn deref(&self) -> &Self::Target { + self.0 + } +} diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index c242b9c7f2f4d..97b9a38528770 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -17,6 +17,7 @@ use Resolver; use {names_to_string, module_to_string}; use {resolve_error, ResolutionError}; +use rustc_data_structures::ptr_key::PtrKey; use rustc::ty; use rustc::lint::builtin::BuiltinLintDiagnostics; use rustc::lint::builtin::{DUPLICATE_MACRO_EXPORTS, PUB_USE_OF_PRIVATE_EXTERN_CRATE}; @@ -33,7 +34,7 @@ use syntax::util::lev_distance::find_best_match_for_name; use syntax_pos::Span; use std::cell::{Cell, RefCell}; -use std::mem; +use std::{mem, ptr}; /// Contains data for specific types of import directives. #[derive(Clone, Debug)] @@ -105,8 +106,8 @@ impl<'a> ImportDirective<'a> { /// Records information about the resolution of a name in a namespace of a module. pub struct NameResolution<'a> { /// Single imports that may define the name in the namespace. - /// Import directives are arena-allocated, so it's ok to use pointers as keys, they are stable. - single_imports: FxHashSet<*const ImportDirective<'a>>, + /// Import directives are arena-allocated, so it's ok to use pointers as keys. + single_imports: FxHashSet>>, /// The least shadowable known binding for this name, or None if there are no known bindings. pub binding: Option<&'a NameBinding<'a>>, shadowed_glob: Option<&'a NameBinding<'a>>, @@ -192,7 +193,6 @@ impl<'a> Resolver<'a> { // Check if one of single imports can still define the name, // if it can then our result is not determined and can be invalidated. for single_import in &resolution.single_imports { - let single_import = unsafe { &**single_import }; if !self.is_accessible(single_import.vis.get()) { continue; } @@ -291,7 +291,7 @@ impl<'a> Resolver<'a> { SingleImport { target, type_ns_only, .. } => { self.per_ns(|this, ns| if !type_ns_only || ns == TypeNS { let mut resolution = this.resolution(current_module, target, ns).borrow_mut(); - resolution.single_imports.insert(directive); + resolution.single_imports.insert(PtrKey(directive)); }); } // We don't add prelude imports to the globs since they only affect lexical scopes, @@ -398,7 +398,7 @@ impl<'a> Resolver<'a> { _ if old_binding.is_some() => return t, None => return t, Some(binding) => match old_binding { - Some(old_binding) if old_binding as *const _ == binding as *const _ => return t, + Some(old_binding) if ptr::eq(old_binding, binding) => return t, _ => (binding, t), } } @@ -583,7 +583,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { Err(Undetermined) => indeterminate = true, Err(Determined) => { this.update_resolution(parent, target, ns, |_, resolution| { - resolution.single_imports.remove(&(directive as *const _)); + resolution.single_imports.remove(&PtrKey(directive)); }); } Ok(binding) if !binding.is_importable() => { @@ -916,7 +916,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { let mut reexports = Vec::new(); let mut exported_macro_names = FxHashMap(); - if module as *const _ == self.graph_root as *const _ { + if ptr::eq(module, self.graph_root) { let macro_exports = mem::replace(&mut self.macro_exports, Vec::new()); for export in macro_exports.into_iter().rev() { if let Some(later_span) = exported_macro_names.insert(export.ident.modern(), From 32453db3326a9e8331695dda549cfb794feb656a Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Fri, 20 Jul 2018 01:11:30 +0300 Subject: [PATCH 6/8] resolve: Fully prohibit shadowing of in-scope names by globs in macro paths --- src/librustc_resolve/macros.rs | 3 +- src/librustc_resolve/resolve_imports.rs | 2 +- src/test/ui/imports/glob-shadowing.rs | 44 ++++++++++++++++++++ src/test/ui/imports/glob-shadowing.stderr | 49 +++++++++++++++++++++++ 4 files changed, 95 insertions(+), 3 deletions(-) create mode 100644 src/test/ui/imports/glob-shadowing.rs create mode 100644 src/test/ui/imports/glob-shadowing.stderr diff --git a/src/librustc_resolve/macros.rs b/src/librustc_resolve/macros.rs index c85115c62f8eb..0ad652b4710ce 100644 --- a/src/librustc_resolve/macros.rs +++ b/src/librustc_resolve/macros.rs @@ -588,8 +588,7 @@ impl<'a> Resolver<'a> { return potential_illegal_shadower; } } - if binding.expansion != Mark::root() || - (binding.is_glob_import() && module.unwrap().def().is_some()) { + if binding.is_glob_import() || binding.expansion != Mark::root() { potential_illegal_shadower = result; } else { return result; diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 97b9a38528770..ba66305299819 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -233,7 +233,7 @@ impl<'a> Resolver<'a> { // What on earth is this? // Apparently one more subtle interaction with `resolve_lexical_macro_path_segment` // that are going to be removed in the next commit. - if restricted_shadowing && module.def().is_some() { + if restricted_shadowing { return Err(Determined); } diff --git a/src/test/ui/imports/glob-shadowing.rs b/src/test/ui/imports/glob-shadowing.rs new file mode 100644 index 0000000000000..e4f55137e660f --- /dev/null +++ b/src/test/ui/imports/glob-shadowing.rs @@ -0,0 +1,44 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(decl_macro)] + +mod m { + pub macro env($e: expr) { $e } + pub macro fenv() { 0 } +} + +mod glob_in_normal_module { + use m::*; + fn check() { + let x = env!("PATH"); //~ ERROR `env` is ambiguous + } +} + +mod glob_in_block_module { + fn block() { + use m::*; + fn check() { + let x = env!("PATH"); //~ ERROR `env` is ambiguous + } + } +} + +mod glob_shadows_item { + pub macro fenv($e: expr) { $e } + fn block() { + use m::*; + fn check() { + let x = fenv!(); //~ ERROR `fenv` is ambiguous + } + } +} + +fn main() {} diff --git a/src/test/ui/imports/glob-shadowing.stderr b/src/test/ui/imports/glob-shadowing.stderr new file mode 100644 index 0000000000000..7f61cd6c76d6a --- /dev/null +++ b/src/test/ui/imports/glob-shadowing.stderr @@ -0,0 +1,49 @@ +error[E0659]: `env` is ambiguous + --> $DIR/glob-shadowing.rs:21:17 + | +LL | let x = env!("PATH"); //~ ERROR `env` is ambiguous + | ^^^ + | +note: `env` could refer to the name imported here + --> $DIR/glob-shadowing.rs:19:9 + | +LL | use m::*; + | ^^^^ + = note: `env` is also a builtin macro + = note: consider adding an explicit import of `env` to disambiguate + +error[E0659]: `env` is ambiguous + --> $DIR/glob-shadowing.rs:29:21 + | +LL | let x = env!("PATH"); //~ ERROR `env` is ambiguous + | ^^^ + | +note: `env` could refer to the name imported here + --> $DIR/glob-shadowing.rs:27:13 + | +LL | use m::*; + | ^^^^ + = note: `env` is also a builtin macro + = note: consider adding an explicit import of `env` to disambiguate + +error[E0659]: `fenv` is ambiguous + --> $DIR/glob-shadowing.rs:39:21 + | +LL | let x = fenv!(); //~ ERROR `fenv` is ambiguous + | ^^^^ + | +note: `fenv` could refer to the name imported here + --> $DIR/glob-shadowing.rs:37:13 + | +LL | use m::*; + | ^^^^ +note: `fenv` could also refer to the name defined here + --> $DIR/glob-shadowing.rs:35:5 + | +LL | pub macro fenv($e: expr) { $e } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: consider adding an explicit import of `fenv` to disambiguate + +error: aborting due to 3 previous errors + +For more information about this error, try `rustc --explain E0659`. From e7aeb2b1c7e9776a6771f8d345390c2ab1ceb947 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Fri, 20 Jul 2018 01:45:08 +0300 Subject: [PATCH 7/8] resolve: Add more comments to in-module resolution --- src/librustc_resolve/resolve_imports.rs | 48 +++++++++++++------------ 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index ba66305299819..50eb89be69011 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -207,36 +207,39 @@ impl<'a> Resolver<'a> { } } - let no_unexpanded_macros = module.unresolved_invocations.borrow().is_empty(); - match resolution.binding { - // So we have a resolution that's from a glob import. This resolution is determined - // if it cannot be shadowed by some new item/import expanded from a macro. - // This happens either if there are no unexpanded macros, or expanded names cannot - // shadow globs (that happens in macro namespace or with restricted shadowing). - Some(binding) if no_unexpanded_macros || ns == MacroNS || restricted_shadowing => - return check_usable(self, binding), - // If we have no resolution, then it's a determined error it some new item/import - // cannot appear from a macro expansion or an undetermined glob. - None if no_unexpanded_macros => {} // go check for globs below - // This is actually an undetermined error, but we need to return determinate error - // due to subtle interactions with `resolve_lexical_macro_path_segment` - // that are going to be removed in the next commit. - None if restricted_shadowing => {} // go check for globs below - _ => return Err(Undetermined), + // So we have a resolution that's from a glob import. This resolution is determined + // if it cannot be shadowed by some new item/import expanded from a macro. + // This happens either if there are no unexpanded macros, or expanded names cannot + // shadow globs (that happens in macro namespace or with restricted shadowing). + let unexpanded_macros = !module.unresolved_invocations.borrow().is_empty(); + if let Some(binding) = resolution.binding { + if !unexpanded_macros || ns == MacroNS || restricted_shadowing { + return check_usable(self, binding); + } else { + return Err(Undetermined); + } } // --- From now on we have no resolution. --- - // Check if one of glob imports can still define the name, - // if it can then our "no resolution" result is not determined and can be invalidated. - - // What on earth is this? - // Apparently one more subtle interaction with `resolve_lexical_macro_path_segment` - // that are going to be removed in the next commit. + // Now we are in situation when new item/import can appear only from a glob or a macro + // expansion. With restricted shadowing names from globs and macro expansions cannot + // shadow names from outer scopes, so we can freely fallback from module search to search + // in outer scopes. To continue search in outer scopes we have to lie a bit and return + // `Determined` to `resolve_lexical_macro_path_segment` even if the correct answer + // for in-module resolution could be `Undetermined`. if restricted_shadowing { return Err(Determined); } + // Check if one of unexpanded macros can still define the name, + // if it can then our "no resolution" result is not determined and can be invalidated. + if unexpanded_macros { + return Err(Undetermined); + } + + // Check if one of glob imports can still define the name, + // if it can then our "no resolution" result is not determined and can be invalidated. for glob_import in module.globs.borrow().iter() { if !self.is_accessible(glob_import.vis.get()) { continue @@ -258,6 +261,7 @@ impl<'a> Resolver<'a> { } } + // No resolution and no one else can define the name - determinate error. Err(Determined) } From 382285a01de393ecc9dd2c78c2518299d8749c12 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Fri, 20 Jul 2018 12:45:07 +0300 Subject: [PATCH 8/8] Revert some use of `PtrKey` to fix parallel_queries build --- src/librustc/ty/query/job.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/librustc/ty/query/job.rs b/src/librustc/ty/query/job.rs index 6cc71642c42ae..56a8c13a8d3b8 100644 --- a/src/librustc/ty/query/job.rs +++ b/src/librustc/ty/query/job.rs @@ -11,7 +11,6 @@ #![allow(warnings)] use std::mem; -use rustc_data_structures::ptr_key::PtrKey; use rustc_data_structures::sync::{Lock, LockGuard, Lrc, Weak}; use rustc_data_structures::OnDrop; use syntax_pos::Span; @@ -283,7 +282,7 @@ where fn cycle_check<'tcx>(query: Lrc>, span: Span, stack: &mut Vec<(Span, Lrc>)>, - visited: &mut HashSet>> + visited: &mut HashSet<*const QueryJob<'tcx>> ) -> Option>> { if visited.contains(&query.as_ptr()) { return if let Some(p) = stack.iter().position(|q| q.1.as_ptr() == query.as_ptr()) { @@ -322,7 +321,7 @@ fn cycle_check<'tcx>(query: Lrc>, #[cfg(parallel_queries)] fn connected_to_root<'tcx>( query: Lrc>, - visited: &mut HashSet>> + visited: &mut HashSet<*const QueryJob<'tcx>> ) -> bool { // We already visited this or we're deliberately ignoring it if visited.contains(&query.as_ptr()) {