Auto merge of #57199 - petrochenkov:ambig, r=estebank

resolve: Simplify treatment of ambiguity errors

If we have a glob conflict like this
```rust
mod m1 { struct S; }
mod m2 { struct S; }

use m1::*;
use m2::*;
```
we treat it as a special "ambiguity item" that's not an error by itself, but produces an error when actually used.
```rust
use m1::*; // primary
use m2::*; // secondary

=>

ambiguity S(m1::S, m2::S);
```

Ambiguity items were *sometimes* treated as their primary items for error recovery, but pretty irregularly.

After this PR they are always treated as their primary items, except that
- If an ambiguity item is marked as used, then it still produces an error.
- Ambiguity items are still filtered away when exported to other crates (which is also a use in some sense).

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,

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<bool>,
     },
-    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<MacroKind> {
-        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);
         }
     }
 

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);
                 }

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,
@@ -498,8 +499,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)) =
@@ -527,13 +528,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,
         })
     }
 
@@ -962,9 +965,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");
@@ -1201,10 +1204,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() {

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 *;
+}

src/test/ui/imports/duplicate.rs

@@ -37,7 +37,7 @@ fn main() {
 }
 
 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::*;

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`
 }

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`.