diff --git a/compiler/rustc_middle/src/middle/privacy.rs b/compiler/rustc_middle/src/middle/privacy.rs index 0be4c8243d632..906d927665a55 100644 --- a/compiler/rustc_middle/src/middle/privacy.rs +++ b/compiler/rustc_middle/src/middle/privacy.rs @@ -7,6 +7,7 @@ use std::hash::Hash; use rustc_data_structures::fx::{FxIndexMap, IndexEntry}; use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; use rustc_hir::def::DefKind; +use rustc_hir::{ItemKind, Node, UseKind}; use rustc_macros::HashStable; use rustc_span::def_id::{CRATE_DEF_ID, LocalDefId}; @@ -184,13 +185,20 @@ impl EffectiveVisibilities { if !is_impl && tcx.trait_impl_of_assoc(def_id.to_def_id()).is_none() { let nominal_vis = tcx.visibility(def_id); if !nominal_vis.is_at_least(ev.reachable, tcx) { - span_bug!( - span, - "{:?}: reachable {:?} > nominal {:?}", - def_id, - ev.reachable, - nominal_vis, - ); + if let Node::Item(item) = tcx.hir_node_by_def_id(def_id) + && let ItemKind::Use(_, UseKind::Glob) = item.kind + { + // Glob import visibilities can be increased by other + // more public glob imports in cases of ambiguity. + } else { + span_bug!( + span, + "{:?}: reachable {:?} > nominal {:?}", + def_id, + ev.reachable, + nominal_vis, + ); + } } } } diff --git a/compiler/rustc_resolve/src/build_reduced_graph.rs b/compiler/rustc_resolve/src/build_reduced_graph.rs index 4148ec1b7abd7..8cbc0adfaac24 100644 --- a/compiler/rustc_resolve/src/build_reduced_graph.rs +++ b/compiler/rustc_resolve/src/build_reduced_graph.rs @@ -93,7 +93,9 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { ambiguity: CmCell::new(ambiguity), // External ambiguities always report the `AMBIGUOUS_GLOB_IMPORTS` lint at the moment. warn_ambiguity: CmCell::new(true), - vis: CmCell::new(vis), + initial_vis: vis, + ambiguity_vis_max: CmCell::new(None), + ambiguity_vis_min: CmCell::new(None), span, expansion, parent_module: Some(parent.to_module()), diff --git a/compiler/rustc_resolve/src/effective_visibilities.rs b/compiler/rustc_resolve/src/effective_visibilities.rs index cd7790f27b5f5..b5614106fe997 100644 --- a/compiler/rustc_resolve/src/effective_visibilities.rs +++ b/compiler/rustc_resolve/src/effective_visibilities.rs @@ -182,6 +182,10 @@ impl<'a, 'ra, 'tcx> EffectiveVisibilitiesVisitor<'a, 'ra, 'tcx> { parent_id.level(), tcx, ); + if let Some(max_vis_decl) = decl.ambiguity_vis_max.get() { + // Avoid the most visible import in an ambiguous glob set being reported as unused. + self.update_import(max_vis_decl, parent_id); + } } fn update_def( diff --git a/compiler/rustc_resolve/src/ident.rs b/compiler/rustc_resolve/src/ident.rs index 33e9ac1d430a4..f9be7edbab00e 100644 --- a/compiler/rustc_resolve/src/ident.rs +++ b/compiler/rustc_resolve/src/ident.rs @@ -490,6 +490,12 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { } Some(Finalize { import, .. }) => import, }; + this.get_mut().maybe_push_glob_vs_glob_vis_ambiguity( + ident, + orig_ident_span, + decl, + import, + ); if let Some(&(innermost_decl, _)) = innermost_results.first() { // Found another solution, if the first one was "weak", report an error. @@ -779,6 +785,30 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { ret.map_err(ControlFlow::Continue) } + fn maybe_push_glob_vs_glob_vis_ambiguity( + &mut self, + ident: IdentKey, + orig_ident_span: Span, + decl: Decl<'ra>, + import: Option, + ) { + let Some(import) = import else { return }; + let vis1 = self.import_decl_vis(decl, import); + let vis2 = self.import_decl_vis_ext(decl, import, true); + if vis1 != vis2 { + self.ambiguity_errors.push(AmbiguityError { + kind: AmbiguityKind::GlobVsGlob, + ambig_vis: Some((vis1, vis2)), + ident: ident.orig(orig_ident_span), + b1: decl.ambiguity_vis_max.get().unwrap_or(decl), + b2: decl.ambiguity_vis_min.get().unwrap_or(decl), + scope1: Scope::ModuleGlobs(decl.parent_module.unwrap(), None), + scope2: Scope::ModuleGlobs(decl.parent_module.unwrap(), None), + warning: Some(AmbiguityWarning::GlobImport), + }); + } + } + fn maybe_push_ambiguity( &mut self, ident: IdentKey, diff --git a/compiler/rustc_resolve/src/imports.rs b/compiler/rustc_resolve/src/imports.rs index 9df075561b3f2..2926549e6824a 100644 --- a/compiler/rustc_resolve/src/imports.rs +++ b/compiler/rustc_resolve/src/imports.rs @@ -371,8 +371,17 @@ fn remove_same_import<'ra>(d1: Decl<'ra>, d2: Decl<'ra>) -> (Decl<'ra>, Decl<'ra impl<'ra, 'tcx> Resolver<'ra, 'tcx> { pub(crate) fn import_decl_vis(&self, decl: Decl<'ra>, import: ImportSummary) -> Visibility { + self.import_decl_vis_ext(decl, import, false) + } + + pub(crate) fn import_decl_vis_ext( + &self, + decl: Decl<'ra>, + import: ImportSummary, + min: bool, + ) -> Visibility { assert!(import.vis.is_accessible_from(import.nearest_parent_mod, self.tcx)); - let decl_vis = decl.vis(); + let decl_vis = if min { decl.min_vis() } else { decl.vis() }; if decl_vis.is_at_least(import.vis, self.tcx) { // Ordered, import is less visible than the imported declaration, or the same, // use the import's visibility. @@ -413,7 +422,9 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { ambiguity: CmCell::new(None), warn_ambiguity: CmCell::new(false), span: import.span, - vis: CmCell::new(vis.to_def_id()), + initial_vis: vis.to_def_id(), + ambiguity_vis_max: CmCell::new(None), + ambiguity_vis_min: CmCell::new(None), expansion: import.parent_scope.expansion, parent_module: Some(import.parent_scope.module), }) @@ -438,8 +449,6 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { // - A glob decl is overwritten by its clone after setting ambiguity in it. // FIXME: avoid this by removing `warn_ambiguity`, or by triggering glob re-fetch // with the same decl in some way. - // - A glob decl is overwritten by a glob decl with larger visibility. - // FIXME: avoid this by updating this visibility in place. // - A glob decl is overwritten by a glob decl re-fetching an // overwritten decl from other module (the recursive case). // Here we are detecting all such re-fetches and overwrite old decls @@ -453,8 +462,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { // FIXME: reenable the asserts when `warn_ambiguity` is removed (#149195). // assert_ne!(old_deep_decl, deep_decl); // assert!(old_deep_decl.is_glob_import()); - // FIXME: reenable the assert when visibility is updated in place. - // assert!(!deep_decl.is_glob_import()); + assert!(!deep_decl.is_glob_import()); if old_glob_decl.ambiguity.get().is_some() && glob_decl.ambiguity.get().is_none() { // Do not lose glob ambiguities when re-fetching the glob. glob_decl.ambiguity.set_unchecked(old_glob_decl.ambiguity.get()); @@ -474,12 +482,21 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { // FIXME: remove this when `warn_ambiguity` is removed (#149195). self.arenas.alloc_decl((*old_glob_decl).clone()) } - } else if !old_glob_decl.vis().is_at_least(glob_decl.vis(), self.tcx) { - // We are glob-importing the same item but with greater visibility. + } else if let old_vis = old_glob_decl.vis() + && let vis = glob_decl.vis() + && old_vis != vis + { + // We are glob-importing the same item but with a different visibility. // All visibilities here are ordered because all of them are ancestors of `module`. - // FIXME: Update visibility in place, but without regressions - // (#152004, #151124, #152347). - glob_decl + if vis.is_at_least(old_vis, self.tcx) { + old_glob_decl.ambiguity_vis_max.set_unchecked(Some(glob_decl)); + } else if let old_min_vis = old_glob_decl.min_vis() + && old_min_vis != vis + && old_min_vis.is_at_least(vis, self.tcx) + { + old_glob_decl.ambiguity_vis_min.set_unchecked(Some(glob_decl)); + } + old_glob_decl } else if glob_decl.is_ambiguity_recursive() && !old_glob_decl.is_ambiguity_recursive() { // Overwriting a non-ambiguous glob import with an ambiguous glob import. old_glob_decl.ambiguity.set_unchecked(Some(glob_decl)); @@ -502,6 +519,8 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { ) -> Result<(), Decl<'ra>> { assert!(!decl.warn_ambiguity.get()); assert!(decl.ambiguity.get().is_none()); + assert!(decl.ambiguity_vis_max.get().is_none()); + assert!(decl.ambiguity_vis_min.get().is_none()); let module = decl.parent_module.unwrap().expect_local(); assert!(self.is_accessible_from(decl.vis(), module.to_module())); let res = decl.res(); @@ -560,11 +579,12 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { .resolution_or_default(module.to_module(), key, orig_ident_span) .borrow_mut_unchecked(); let old_decl = resolution.determined_decl(); + let old_vis = old_decl.map(|d| d.vis()); let t = f(self, resolution); if let Some(binding) = resolution.determined_decl() - && old_decl != Some(binding) + && (old_decl != Some(binding) || old_vis != Some(binding.vis())) { (binding, t, warn_ambiguity || old_decl.is_some()) } else { diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index a44a7b30b4e60..5f0dfbc4fb90c 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/compiler/rustc_resolve/src/lib.rs @@ -924,7 +924,13 @@ struct DeclData<'ra> { warn_ambiguity: CmCell, expansion: LocalExpnId, span: Span, - vis: CmCell>, + initial_vis: Visibility, + /// If the declaration refers to an ambiguous glob set, then this is the most visible + /// declaration from the set, if its visibility is different from `initial_vis`. + ambiguity_vis_max: CmCell>>, + /// If the declaration refers to an ambiguous glob set, then this is the least visible + /// declaration from the set, if its visibility is different from `initial_vis`. + ambiguity_vis_min: CmCell>>, parent_module: Option>, } @@ -1044,7 +1050,13 @@ struct AmbiguityError<'ra> { impl<'ra> DeclData<'ra> { fn vis(&self) -> Visibility { - self.vis.get() + // Select the maximum visibility if there are multiple ambiguous glob imports. + self.ambiguity_vis_max.get().map(|d| d.vis()).unwrap_or_else(|| self.initial_vis) + } + + fn min_vis(&self) -> Visibility { + // Select the minimum visibility if there are multiple ambiguous glob imports. + self.ambiguity_vis_min.get().map(|d| d.vis()).unwrap_or_else(|| self.initial_vis) } fn res(&self) -> Res { @@ -1500,7 +1512,9 @@ impl<'ra> ResolverArenas<'ra> { kind: DeclKind::Def(res), ambiguity: CmCell::new(None), warn_ambiguity: CmCell::new(false), - vis: CmCell::new(vis), + initial_vis: vis, + ambiguity_vis_max: CmCell::new(None), + ambiguity_vis_min: CmCell::new(None), span, expansion, parent_module, diff --git a/tests/ui/imports/ambiguous-import-visibility-globglob-priv.stderr b/tests/ui/imports/ambiguous-import-visibility-globglob-priv.stderr index 6e77808855a6c..4485f5ac96481 100644 --- a/tests/ui/imports/ambiguous-import-visibility-globglob-priv.stderr +++ b/tests/ui/imports/ambiguous-import-visibility-globglob-priv.stderr @@ -5,10 +5,10 @@ LL | use crate::both::private::S; | ^ private struct import | note: the struct import `S` is defined here... - --> $DIR/ambiguous-import-visibility-globglob-priv.rs:8:24 + --> $DIR/ambiguous-import-visibility-globglob-priv.rs:7:13 | -LL | pub(super) use crate::m::*; - | ^^^^^^^^^^^ +LL | use crate::m::*; + | ^^^^^^^^^^^ note: ...and refers to the struct `S` which is defined here --> $DIR/ambiguous-import-visibility-globglob-priv.rs:2:5 | @@ -27,10 +27,10 @@ LL | use crate::both::private::S; | ^ private struct import | note: the struct import `S` is defined here... - --> $DIR/ambiguous-import-visibility-globglob-priv.rs:8:24 + --> $DIR/ambiguous-import-visibility-globglob-priv.rs:7:13 | -LL | pub(super) use crate::m::*; - | ^^^^^^^^^^^ +LL | use crate::m::*; + | ^^^^^^^^^^^ note: ...and refers to the struct `S` which is defined here --> $DIR/ambiguous-import-visibility-globglob-priv.rs:2:5 | diff --git a/tests/ui/imports/ambiguous-import-visibility-globglob.rs b/tests/ui/imports/ambiguous-import-visibility-globglob.rs index 5af32abd1c600..5c8b6f4766835 100644 --- a/tests/ui/imports/ambiguous-import-visibility-globglob.rs +++ b/tests/ui/imports/ambiguous-import-visibility-globglob.rs @@ -1,7 +1,5 @@ //@ check-pass -// FIXME: should report "ambiguous import visibility" in all the cases below. - mod m { pub struct S {} } @@ -12,7 +10,11 @@ mod min_vis_first { pub use crate::m::*; pub use self::S as S1; + //~^ WARN ambiguous import visibility + //~| WARN this was previously accepted pub(crate) use self::S as S2; + //~^ WARN ambiguous import visibility + //~| WARN this was previously accepted use self::S as S3; // OK } @@ -22,7 +24,11 @@ mod mid_vis_first { pub use crate::m::*; pub use self::S as S1; + //~^ WARN ambiguous import visibility + //~| WARN this was previously accepted pub(crate) use self::S as S2; + //~^ WARN ambiguous import visibility + //~| WARN this was previously accepted use self::S as S3; // OK } @@ -32,7 +38,11 @@ mod max_vis_first { pub(crate) use crate::m::*; pub use self::S as S1; + //~^ WARN ambiguous import visibility + //~| WARN this was previously accepted pub(crate) use self::S as S2; + //~^ WARN ambiguous import visibility + //~| WARN this was previously accepted use self::S as S3; // OK } diff --git a/tests/ui/imports/ambiguous-import-visibility-globglob.stderr b/tests/ui/imports/ambiguous-import-visibility-globglob.stderr new file mode 100644 index 0000000000000..d15282cc436cd --- /dev/null +++ b/tests/ui/imports/ambiguous-import-visibility-globglob.stderr @@ -0,0 +1,135 @@ +warning: ambiguous import visibility: pub or pub(in crate::min_vis_first) + --> $DIR/ambiguous-import-visibility-globglob.rs:12:19 + | +LL | pub use self::S as S1; + | ^ + | + = note: ambiguous because of multiple glob imports of a name in the same module +note: `S` could refer to the struct imported here + --> $DIR/ambiguous-import-visibility-globglob.rs:10:13 + | +LL | pub use crate::m::*; + | ^^^^^^^^^^^ + = help: consider adding an explicit import of `S` to disambiguate +note: `S` could also refer to the struct imported here + --> $DIR/ambiguous-import-visibility-globglob.rs:8:9 + | +LL | use crate::m::*; + | ^^^^^^^^^^^ + = help: consider adding an explicit import of `S` to disambiguate + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #149145 + = note: `#[warn(ambiguous_import_visibilities)]` (part of `#[warn(future_incompatible)]`) on by default + +warning: ambiguous import visibility: pub(crate) or pub(in crate::min_vis_first) + --> $DIR/ambiguous-import-visibility-globglob.rs:15:26 + | +LL | pub(crate) use self::S as S2; + | ^ + | + = note: ambiguous because of multiple glob imports of a name in the same module +note: `S` could refer to the struct imported here + --> $DIR/ambiguous-import-visibility-globglob.rs:10:13 + | +LL | pub use crate::m::*; + | ^^^^^^^^^^^ + = help: consider adding an explicit import of `S` to disambiguate +note: `S` could also refer to the struct imported here + --> $DIR/ambiguous-import-visibility-globglob.rs:8:9 + | +LL | use crate::m::*; + | ^^^^^^^^^^^ + = help: consider adding an explicit import of `S` to disambiguate + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #149145 + +warning: ambiguous import visibility: pub or pub(in crate::mid_vis_first) + --> $DIR/ambiguous-import-visibility-globglob.rs:26:19 + | +LL | pub use self::S as S1; + | ^ + | + = note: ambiguous because of multiple glob imports of a name in the same module +note: `S` could refer to the struct imported here + --> $DIR/ambiguous-import-visibility-globglob.rs:24:13 + | +LL | pub use crate::m::*; + | ^^^^^^^^^^^ + = help: consider adding an explicit import of `S` to disambiguate +note: `S` could also refer to the struct imported here + --> $DIR/ambiguous-import-visibility-globglob.rs:23:9 + | +LL | use crate::m::*; + | ^^^^^^^^^^^ + = help: consider adding an explicit import of `S` to disambiguate + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #149145 + +warning: ambiguous import visibility: pub(crate) or pub(in crate::mid_vis_first) + --> $DIR/ambiguous-import-visibility-globglob.rs:29:26 + | +LL | pub(crate) use self::S as S2; + | ^ + | + = note: ambiguous because of multiple glob imports of a name in the same module +note: `S` could refer to the struct imported here + --> $DIR/ambiguous-import-visibility-globglob.rs:24:13 + | +LL | pub use crate::m::*; + | ^^^^^^^^^^^ + = help: consider adding an explicit import of `S` to disambiguate +note: `S` could also refer to the struct imported here + --> $DIR/ambiguous-import-visibility-globglob.rs:23:9 + | +LL | use crate::m::*; + | ^^^^^^^^^^^ + = help: consider adding an explicit import of `S` to disambiguate + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #149145 + +warning: ambiguous import visibility: pub or pub(in crate::max_vis_first) + --> $DIR/ambiguous-import-visibility-globglob.rs:40:19 + | +LL | pub use self::S as S1; + | ^ + | + = note: ambiguous because of multiple glob imports of a name in the same module +note: `S` could refer to the struct imported here + --> $DIR/ambiguous-import-visibility-globglob.rs:36:13 + | +LL | pub use crate::m::*; + | ^^^^^^^^^^^ + = help: consider adding an explicit import of `S` to disambiguate +note: `S` could also refer to the struct imported here + --> $DIR/ambiguous-import-visibility-globglob.rs:37:9 + | +LL | use crate::m::*; + | ^^^^^^^^^^^ + = help: consider adding an explicit import of `S` to disambiguate + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #149145 + +warning: ambiguous import visibility: pub(crate) or pub(in crate::max_vis_first) + --> $DIR/ambiguous-import-visibility-globglob.rs:43:26 + | +LL | pub(crate) use self::S as S2; + | ^ + | + = note: ambiguous because of multiple glob imports of a name in the same module +note: `S` could refer to the struct imported here + --> $DIR/ambiguous-import-visibility-globglob.rs:36:13 + | +LL | pub use crate::m::*; + | ^^^^^^^^^^^ + = help: consider adding an explicit import of `S` to disambiguate +note: `S` could also refer to the struct imported here + --> $DIR/ambiguous-import-visibility-globglob.rs:37:9 + | +LL | use crate::m::*; + | ^^^^^^^^^^^ + = help: consider adding an explicit import of `S` to disambiguate + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #149145 + +warning: 6 warnings emitted + diff --git a/tests/ui/imports/overwrite-vis-unused.rs b/tests/ui/imports/overwrite-vis-unused.rs index 0217fb6250837..6361c71f6639e 100644 --- a/tests/ui/imports/overwrite-vis-unused.rs +++ b/tests/ui/imports/overwrite-vis-unused.rs @@ -1,12 +1,12 @@ // Regression test for issues #152004 and #151124. - +//@ check-pass #![deny(unused)] mod m { pub struct S {} } -use m::*; //~ ERROR unused import: `m::*` +use m::*; pub use m::*; fn main() {} diff --git a/tests/ui/imports/overwrite-vis-unused.stderr b/tests/ui/imports/overwrite-vis-unused.stderr deleted file mode 100644 index a38aa4d5f070a..0000000000000 --- a/tests/ui/imports/overwrite-vis-unused.stderr +++ /dev/null @@ -1,15 +0,0 @@ -error: unused import: `m::*` - --> $DIR/overwrite-vis-unused.rs:9:5 - | -LL | use m::*; - | ^^^^ - | -note: the lint level is defined here - --> $DIR/overwrite-vis-unused.rs:3:9 - | -LL | #![deny(unused)] - | ^^^^^^ - = note: `#[deny(unused_imports)]` implied by `#[deny(unused)]` - -error: aborting due to 1 previous error -