From 06f22ba427660589c543d6f37c8c45de88d28093 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Sat, 29 Dec 2018 18:15:29 +0300 Subject: [PATCH] resolve: Simplify treatment of ambiguity errors --- src/librustc_resolve/build_reduced_graph.rs | 3 + src/librustc_resolve/lib.rs | 63 ++++++++----------- src/librustc_resolve/macros.rs | 7 ++- src/librustc_resolve/resolve_imports.rs | 29 +++++---- .../ui/imports/auxiliary/glob-conflict.rs | 4 ++ src/test/ui/imports/duplicate.rs | 4 +- src/test/ui/imports/duplicate.stderr | 21 +------ .../ui/imports/glob-conflict-cross-crate.rs | 1 + .../imports/glob-conflict-cross-crate.stderr | 8 ++- 9 files changed, 65 insertions(+), 75 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 21fb29974c8..a452bbf0c9d 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -42,6 +42,7 @@ impl<'a> ToNameBinding<'a> for (Module<'a>, ty::Visibility, Span, Mark) { fn to_name_binding(self, arenas: &'a ResolverArenas<'a>) -> &'a NameBinding<'a> { arenas.alloc_name_binding(NameBinding { kind: NameBindingKind::Module(self.0), + ambiguity: None, vis: self.1, span: self.2, expansion: self.3, @@ -53,6 +54,7 @@ impl<'a> ToNameBinding<'a> for (Def, ty::Visibility, Span, Mark) { fn to_name_binding(self, arenas: &'a ResolverArenas<'a>) -> &'a NameBinding<'a> { arenas.alloc_name_binding(NameBinding { kind: NameBindingKind::Def(self.0, false), + ambiguity: None, vis: self.1, span: self.2, expansion: self.3, @@ -66,6 +68,7 @@ impl<'a> ToNameBinding<'a> for (Def, ty::Visibility, Span, Mark, IsMacroExport) fn to_name_binding(self, arenas: &'a ResolverArenas<'a>) -> &'a NameBinding<'a> { arenas.alloc_name_binding(NameBinding { kind: NameBindingKind::Def(self.0, true), + ambiguity: None, vis: self.1, span: self.2, expansion: self.3, diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 0db6dbafb6c..dd19fcfc66a 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -1191,6 +1191,7 @@ impl<'a> fmt::Debug for ModuleData<'a> { #[derive(Clone, Debug)] pub struct NameBinding<'a> { kind: NameBindingKind<'a>, + ambiguity: Option<(&'a NameBinding<'a>, AmbiguityKind)>, expansion: Mark, span: Span, vis: ty::Visibility, @@ -1215,11 +1216,6 @@ enum NameBindingKind<'a> { directive: &'a ImportDirective<'a>, used: Cell, }, - Ambiguity { - kind: AmbiguityKind, - b1: &'a NameBinding<'a>, - b2: &'a NameBinding<'a>, - } } struct PrivacyError<'a>(Span, Ident, &'a NameBinding<'a>); @@ -1309,15 +1305,13 @@ impl<'a> NameBinding<'a> { NameBindingKind::Def(def, _) => def, NameBindingKind::Module(module) => module.def().unwrap(), NameBindingKind::Import { binding, .. } => binding.def(), - NameBindingKind::Ambiguity { .. } => Def::Err, } } - fn def_ignoring_ambiguity(&self) -> Def { - match self.kind { - NameBindingKind::Import { binding, .. } => binding.def_ignoring_ambiguity(), - NameBindingKind::Ambiguity { b1, .. } => b1.def_ignoring_ambiguity(), - _ => self.def(), + fn is_ambiguity(&self) -> bool { + self.ambiguity.is_some() || match self.kind { + NameBindingKind::Import { binding, .. } => binding.is_ambiguity(), + _ => false, } } @@ -1362,7 +1356,6 @@ impl<'a> NameBinding<'a> { fn is_glob_import(&self) -> bool { match self.kind { NameBindingKind::Import { directive, .. } => directive.is_glob(), - NameBindingKind::Ambiguity { b1, .. } => b1.is_glob_import(), _ => false, } } @@ -1382,7 +1375,7 @@ impl<'a> NameBinding<'a> { } fn macro_kind(&self) -> Option { - match self.def_ignoring_ambiguity() { + match self.def() { Def::Macro(_, kind) => Some(kind), Def::NonMacroAttr(..) => Some(MacroKind::Attr), _ => None, @@ -1893,6 +1886,7 @@ impl<'a> Resolver<'a> { arenas, dummy_binding: arenas.alloc_name_binding(NameBinding { kind: NameBindingKind::Def(Def::Err, false), + ambiguity: None, expansion: Mark::root(), span: DUMMY_SP, vis: ty::Visibility::Public, @@ -1963,33 +1957,30 @@ impl<'a> Resolver<'a> { fn record_use(&mut self, ident: Ident, ns: Namespace, used_binding: &'a NameBinding<'a>, is_lexical_scope: bool) { - match used_binding.kind { - NameBindingKind::Import { directive, binding, ref used } if !used.get() => { - // Avoid marking `extern crate` items that refer to a name from extern prelude, - // but not introduce it, as used if they are accessed from lexical scope. - if is_lexical_scope { - if let Some(entry) = self.extern_prelude.get(&ident.modern()) { - if let Some(crate_item) = entry.extern_crate_item { - if ptr::eq(used_binding, crate_item) && !entry.introduced_by_item { - return; - } + if let Some((b2, kind)) = used_binding.ambiguity { + self.ambiguity_errors.push(AmbiguityError { + kind, ident, b1: used_binding, b2, + misc1: AmbiguityErrorMisc::None, + misc2: AmbiguityErrorMisc::None, + }); + } + if let NameBindingKind::Import { directive, binding, ref used } = used_binding.kind { + // Avoid marking `extern crate` items that refer to a name from extern prelude, + // but not introduce it, as used if they are accessed from lexical scope. + if is_lexical_scope { + if let Some(entry) = self.extern_prelude.get(&ident.modern()) { + if let Some(crate_item) = entry.extern_crate_item { + if ptr::eq(used_binding, crate_item) && !entry.introduced_by_item { + return; } } } - used.set(true); - directive.used.set(true); - self.used_imports.insert((directive.id, ns)); - self.add_to_glob_map(directive.id, ident); - self.record_use(ident, ns, binding, false); } - NameBindingKind::Ambiguity { kind, b1, b2 } => { - self.ambiguity_errors.push(AmbiguityError { - kind, ident, b1, b2, - misc1: AmbiguityErrorMisc::None, - misc2: AmbiguityErrorMisc::None, - }); - } - _ => {} + used.set(true); + directive.used.set(true); + self.used_imports.insert((directive.id, ns)); + self.add_to_glob_map(directive.id, ident); + self.record_use(ident, ns, binding, false); } } diff --git a/src/librustc_resolve/macros.rs b/src/librustc_resolve/macros.rs index 5f6ef934c1a..dd1e82f467f 100644 --- a/src/librustc_resolve/macros.rs +++ b/src/librustc_resolve/macros.rs @@ -175,6 +175,7 @@ impl<'a> base::Resolver for Resolver<'a> { self.macro_map.insert(def_id, ext); let binding = self.arenas.alloc_name_binding(NameBinding { kind: NameBindingKind::Def(Def::Macro(def_id, kind), false), + ambiguity: None, span: DUMMY_SP, vis: ty::Visibility::Public, expansion: Mark::root(), @@ -389,7 +390,7 @@ impl<'a> Resolver<'a> { .push((path[0].ident, kind, parent_scope.clone(), binding.ok())); } - binding.map(|binding| binding.def_ignoring_ambiguity()) + binding.map(|binding| binding.def()) } } @@ -950,9 +951,9 @@ impl<'a> Resolver<'a> { Ok(binding) => { let initial_def = initial_binding.map(|initial_binding| { self.record_use(ident, MacroNS, initial_binding, false); - initial_binding.def_ignoring_ambiguity() + initial_binding.def() }); - let def = binding.def_ignoring_ambiguity(); + let def = binding.def(); let seg = Segment::from_ident(ident); check_consistency(self, &[seg], ident.span, kind, initial_def, def); } diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 5830aa3713d..853d63a5c63 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -445,6 +445,7 @@ impl<'a> Resolver<'a> { directive, used: Cell::new(false), }, + ambiguity: None, span: directive.span, vis, expansion: directive.parent_scope.expansion, @@ -494,8 +495,8 @@ impl<'a> Resolver<'a> { nonglob_binding, glob_binding)); } else { resolution.binding = Some(nonglob_binding); - resolution.shadowed_glob = Some(glob_binding); } + resolution.shadowed_glob = Some(glob_binding); } (false, false) => { if let (&NameBindingKind::Def(_, true), &NameBindingKind::Def(_, true)) = @@ -523,13 +524,15 @@ impl<'a> Resolver<'a> { }) } - fn ambiguity(&self, kind: AmbiguityKind, b1: &'a NameBinding<'a>, b2: &'a NameBinding<'a>) - -> &'a NameBinding<'a> { + fn ambiguity(&self, kind: AmbiguityKind, + primary_binding: &'a NameBinding<'a>, secondary_binding: &'a NameBinding<'a>) + -> &'a NameBinding<'a> { self.arenas.alloc_name_binding(NameBinding { - kind: NameBindingKind::Ambiguity { kind, b1, b2 }, - vis: if b1.vis.is_at_least(b2.vis, self) { b1.vis } else { b2.vis }, - span: b1.span, - expansion: Mark::root(), + kind: primary_binding.kind.clone(), + ambiguity: Some((secondary_binding, kind)), + vis: primary_binding.vis, + span: primary_binding.span, + expansion: primary_binding.expansion, }) } @@ -958,9 +961,9 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { directive.module_path.is_empty()); } } - initial_binding.def_ignoring_ambiguity() + initial_binding.def() }); - let def = binding.def_ignoring_ambiguity(); + let def = binding.def(); if let Ok(initial_def) = initial_def { if def != initial_def && this.ambiguity_errors.is_empty() { span_bug!(directive.span, "inconsistent resolution for an import"); @@ -1197,10 +1200,10 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { None => continue, }; - // Filter away "empty import canaries". - let is_non_canary_import = - binding.is_import() && binding.vis != ty::Visibility::Invisible; - if is_non_canary_import || binding.is_macro_def() { + // Filter away "empty import canaries" and ambiguous imports. + let is_good_import = binding.is_import() && !binding.is_ambiguity() && + binding.vis != ty::Visibility::Invisible; + if is_good_import || binding.is_macro_def() { let def = binding.def(); if def != Def::Err { if let Some(def_id) = def.opt_def_id() { diff --git a/src/test/ui/imports/auxiliary/glob-conflict.rs b/src/test/ui/imports/auxiliary/glob-conflict.rs index ac12ed9c81c..c83db64c643 100644 --- a/src/test/ui/imports/auxiliary/glob-conflict.rs +++ b/src/test/ui/imports/auxiliary/glob-conflict.rs @@ -7,3 +7,7 @@ mod m2 { pub use m1::*; pub use m2::*; + +pub mod glob { + pub use *; +} diff --git a/src/test/ui/imports/duplicate.rs b/src/test/ui/imports/duplicate.rs index 95bb69043e9..db6538969ec 100644 --- a/src/test/ui/imports/duplicate.rs +++ b/src/test/ui/imports/duplicate.rs @@ -33,11 +33,11 @@ mod g { fn main() { e::foo(); f::foo(); //~ ERROR `foo` is ambiguous - g::foo(); //~ ERROR `foo` is ambiguous + g::foo(); } mod ambiguous_module_errors { - pub mod m1 { pub use super::m1 as foo; } + pub mod m1 { pub use super::m1 as foo; pub fn bar() {} } pub mod m2 { pub use super::m2 as foo; } use self::m1::*; diff --git a/src/test/ui/imports/duplicate.stderr b/src/test/ui/imports/duplicate.stderr index 3df2c5930e8..acd66826fdf 100644 --- a/src/test/ui/imports/duplicate.stderr +++ b/src/test/ui/imports/duplicate.stderr @@ -50,25 +50,6 @@ LL | pub use b::*; | ^^^^ = help: consider adding an explicit import of `foo` to disambiguate -error[E0659]: `foo` is ambiguous (glob import vs glob import in the same module) - --> $DIR/duplicate.rs:36:8 - | -LL | g::foo(); //~ ERROR `foo` is ambiguous - | ^^^ ambiguous name - | -note: `foo` could refer to the function imported here - --> $DIR/duplicate.rs:29:13 - | -LL | pub use a::*; - | ^^^^ - = help: consider adding an explicit import of `foo` to disambiguate -note: `foo` could also refer to the unresolved item imported here - --> $DIR/duplicate.rs:30:13 - | -LL | pub use f::*; - | ^^^^ - = help: consider adding an explicit import of `foo` to disambiguate - error[E0659]: `foo` is ambiguous (glob import vs glob import in the same module) --> $DIR/duplicate.rs:49:9 | @@ -88,7 +69,7 @@ LL | use self::m2::*; | ^^^^^^^^^^^ = help: consider adding an explicit import of `foo` to disambiguate -error: aborting due to 5 previous errors +error: aborting due to 4 previous errors Some errors occurred: E0252, E0659. For more information about an error, try `rustc --explain E0252`. diff --git a/src/test/ui/imports/glob-conflict-cross-crate.rs b/src/test/ui/imports/glob-conflict-cross-crate.rs index e02148b19f7..c8b18525d80 100644 --- a/src/test/ui/imports/glob-conflict-cross-crate.rs +++ b/src/test/ui/imports/glob-conflict-cross-crate.rs @@ -4,4 +4,5 @@ extern crate glob_conflict; fn main() { glob_conflict::f(); //~ ERROR cannot find function `f` in module `glob_conflict` + glob_conflict::glob::f(); //~ ERROR cannot find function `f` in module `glob_conflict::glob` } diff --git a/src/test/ui/imports/glob-conflict-cross-crate.stderr b/src/test/ui/imports/glob-conflict-cross-crate.stderr index f64637fd6f6..f5a82ef1b3b 100644 --- a/src/test/ui/imports/glob-conflict-cross-crate.stderr +++ b/src/test/ui/imports/glob-conflict-cross-crate.stderr @@ -4,6 +4,12 @@ error[E0425]: cannot find function `f` in module `glob_conflict` LL | glob_conflict::f(); //~ ERROR cannot find function `f` in module `glob_conflict` | ^ not found in `glob_conflict` -error: aborting due to previous error +error[E0425]: cannot find function `f` in module `glob_conflict::glob` + --> $DIR/glob-conflict-cross-crate.rs:7:26 + | +LL | glob_conflict::glob::f(); //~ ERROR cannot find function `f` in module `glob_conflict::glob` + | ^ not found in `glob_conflict::glob` + +error: aborting due to 2 previous errors For more information about this error, try `rustc --explain E0425`.