[WIP] resolve: Make ambiguity checking stricter

Author: petrochenkov

compiler/rustc_resolve/src/ident.rs

@@ -5,7 +5,8 @@ use Namespace::*;
 use rustc_ast::{self as ast, NodeId};
 use rustc_errors::ErrorGuaranteed;
 use rustc_hir::def::{DefKind, MacroKinds, Namespace, NonMacroAttrKind, PartialRes, PerNS};
-use rustc_middle::{bug, span_bug};
+use rustc_hir::def_id::DefId;
+use rustc_middle::{bug, span_bug, ty};
 use rustc_session::lint::builtin::PROC_MACRO_DERIVE_RESOLUTION_FALLBACK;
 use rustc_session::parse::feature_err;
 use rustc_span::hygiene::{ExpnId, ExpnKind, LocalExpnId, MacroKind, SyntaxContext};
@@ -465,13 +466,34 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                     Ok((binding, flags))
                         if sub_namespace_match(binding.macro_kinds(), macro_kind) =>
                     {
+                        if !matches!(scope, Scope::MacroUsePrelude) {
+                            let adjusted_module = if let Scope::Module(module, _) = scope
+                                && !matches!(scope_set, ScopeSet::ModuleAndExternPrelude(..))
+                            {
+                                module
+                            } else {
+                                parent_scope.module
+                            };
+                            if !this.is_accessible_from(binding.vis, adjusted_module) {
+                                this.dcx()
+                                    .struct_span_err(binding.span, "binding")
+                                    .with_span_label(adjusted_module.span, "module")
+                                    .emit();
+                            }
+                        }
+
                         // Below we report various ambiguity errors.
                         // We do not need to report them if we are either in speculative resolution,
                         // or in late resolution when everything is already imported and expanded
                         // and no ambiguities exist.
-                        if matches!(finalize, None | Some(Finalize { stage: Stage::Late, .. })) {
-                            return ControlFlow::Break(Ok(binding));
-                        }
+                        let (significant_visibility, import_vis) = match finalize {
+                            None | Some(Finalize { stage: Stage::Late, .. }) => {
+                                return ControlFlow::Break(Ok(binding));
+                            }
+                            Some(Finalize { significant_visibility, import_vis, .. }) => {
+                                (significant_visibility, import_vis)
+                            }
+                        };
 
                         if let Some((innermost_binding, innermost_flags)) = innermost_result {
                             // Found another solution, if the first one was "weak", report an error.
@@ -482,6 +504,8 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                                 innermost_binding,
                                 flags,
                                 innermost_flags,
+                                significant_visibility,
+                                import_vis,
                                 extern_prelude_item_binding,
                                 extern_prelude_flag_binding,
                             ) {
@@ -730,11 +754,21 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         innermost_binding: NameBinding<'ra>,
         flags: Flags,
         innermost_flags: Flags,
+        significant_visibility: bool,
+        import_vis: Option<ty::Visibility>,
         extern_prelude_item_binding: Option<NameBinding<'ra>>,
         extern_prelude_flag_binding: Option<NameBinding<'ra>>,
     ) -> bool {
         let (res, innermost_res) = (binding.res(), innermost_binding.res());
-        if res == innermost_res {
+
+        let vis_min = |v1: ty::Visibility<DefId>, v2: ty::Visibility<DefId>| {
+            if v1.is_at_least(v2, self.tcx) { v2 } else { v1 }
+        };
+        let bad_vis = significant_visibility
+            && vis_min(binding.vis, import_vis.unwrap().to_def_id())
+                != vis_min(innermost_binding.vis, import_vis.unwrap().to_def_id());
+
+        if res == innermost_res && !bad_vis {
             return false;
         }
 
@@ -1207,7 +1241,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             && shadowing == Shadowing::Restricted
             && finalize.stage == Stage::Early
             && binding.expansion != LocalExpnId::ROOT
-            && binding.res() != shadowed_glob.res()
+            && (binding.res() != shadowed_glob.res() || finalize.significant_visibility)
         {
             self.ambiguity_errors.push(AmbiguityError {
                 kind: AmbiguityKind::GlobVsExpanded,

compiler/rustc_resolve/src/imports.rs

@@ -394,6 +394,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                             && non_glob_binding.expansion != LocalExpnId::ROOT
                             && glob_binding.res() != non_glob_binding.res()
                         {
+                            // FIXME: record vis mismatches as well, but breakage in practice.
                             resolution.non_glob_binding = Some(this.new_ambiguity_binding(
                                 AmbiguityKind::GlobVsExpanded,
                                 non_glob_binding,
@@ -413,7 +414,8 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                                     glob_binding,
                                     false,
                                 ));
-                            } else if !old_glob_binding.vis.is_at_least(binding.vis, this.tcx) {
+                            } else if !old_glob_binding.vis.is_at_least(glob_binding.vis, this.tcx)
+                            {
                                 resolution.glob_binding = Some(glob_binding);
                             }
                         } else {
@@ -957,7 +959,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             &import.module_path,
             None,
             &import.parent_scope,
-            Some(finalize),
+            Some(Finalize { significant_visibility: false, ..finalize }),
             ignore_binding,
             Some(import),
         );
@@ -1135,7 +1137,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                     ident,
                     ns,
                     &import.parent_scope,
-                    Some(Finalize { report_private: false, ..finalize }),
+                    Some(Finalize {
+                        report_private: false,
+                        import_vis: Some(import.vis),
+                        ..finalize
+                    }),
                     bindings[ns].get().binding(),
                     Some(import),
                 );

compiler/rustc_resolve/src/late.rs

@@ -1498,7 +1498,7 @@ impl<'a, 'ast, 'ra, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {
             opt_ns,
             &self.parent_scope,
             Some(source),
-            finalize,
+            finalize.map(|finalize| Finalize { significant_visibility: false, ..finalize }),
             Some(&self.ribs),
             None,
             None,

compiler/rustc_resolve/src/lib.rs

@@ -2031,6 +2031,8 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         warn_ambiguity: bool,
     ) {
         if let Some((b2, kind)) = used_binding.ambiguity {
+            // FIXME: Need to check if the `AmbiguityKind::GlobVsGlob`s caused by visibility
+            // mismatch ever passed through an import, but it's too breaking.
             let ambiguity_error = AmbiguityError {
                 kind,
                 ident,
@@ -2503,6 +2505,11 @@ struct Finalize {
     used: Used = Used::Other,
     /// Finalizing early or late resolution.
     stage: Stage = Stage::Early,
+    /// Ambiguous bindings with different visibilities need to be reported
+    /// even if they have the same `Res`olutions.
+    significant_visibility: bool = true,
+    /// Tralala
+    import_vis: Option<Visibility> = None,
 }
 
 impl Finalize {

compiler/rustc_resolve/src/macros.rs

@@ -868,11 +868,13 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             for seg in &mut path {
                 seg.id = None;
             }
+
+            let finalize = Finalize::new(ast::CRATE_NODE_ID, path_span);
             match self.cm().resolve_path(
                 &path,
                 Some(ns),
                 &parent_scope,
-                Some(Finalize::new(ast::CRATE_NODE_ID, path_span)),
+                Some(Finalize { significant_visibility: false, ..finalize }),
                 None,
                 None,
             ) {
@@ -941,11 +943,12 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
 
         let macro_resolutions = self.single_segment_macro_resolutions.take(self);
         for (ident, kind, parent_scope, initial_binding, sugg_span) in macro_resolutions {
+            let finalize = Finalize::new(ast::CRATE_NODE_ID, ident.span);
             match self.cm().resolve_ident_in_scope_set(
                 ident,
                 ScopeSet::Macro(kind),
                 &parent_scope,
-                Some(Finalize::new(ast::CRATE_NODE_ID, ident.span)),
+                Some(Finalize { significant_visibility: false, ..finalize }),
                 true,
                 None,
                 None,
@@ -996,11 +999,12 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
 
         let builtin_attrs = mem::take(&mut self.builtin_attrs);
         for (ident, parent_scope) in builtin_attrs {
+            let finalize = Finalize::new(ast::CRATE_NODE_ID, ident.span);
             let _ = self.cm().resolve_ident_in_scope_set(
                 ident,
                 ScopeSet::Macro(MacroKind::Attr),
                 &parent_scope,
-                Some(Finalize::new(ast::CRATE_NODE_ID, ident.span)),
+                Some(Finalize { significant_visibility: false, ..finalize }),
                 true,
                 None,
                 None,

tests/ui/imports/auxiliary/same-res-ambigious-extern-fail.rs

@@ -13,4 +13,4 @@ globbing! {} // this imports the same `RustEmbed` macro with `pub` visibility
 
 pub trait RustEmbed {}
 
-pub use RustEmbed as Embed;
+pub use self::RustEmbed as Embed;

tests/ui/imports/auxiliary/same-res-ambigious-extern.rs

@@ -8,4 +8,4 @@ pub use same_res_ambigious_extern_macro::*;
 
 pub trait RustEmbed {}
 
-pub use RustEmbed as Embed;
+pub use self::RustEmbed as Embed;

tests/ui/imports/same-res-ambigious.fail.stderr

@@ -1,11 +1,11 @@
 //@ edition: 2018
 //@ revisions: fail pass
-//@[pass] check-pass
+//@ check-pass
 //@[pass] aux-crate: ambigious_extern=same-res-ambigious-extern.rs
 //@[fail] aux-crate: ambigious_extern=same-res-ambigious-extern-fail.rs
 // see https://github.com/rust-lang/rust/pull/147196
 
-#[derive(ambigious_extern::Embed)] //[fail]~ ERROR: derive macro `Embed` is private
+#[derive(ambigious_extern::Embed)]
 struct Foo{}
 
 fn main(){}