Skip to content

Commit 394c717

Browse files
petrochenkovpetrochenkov
committed
resolve: Make sure visibilities of import declarations make sense
That they are all ordered inside the module and not more private than the module itself The `import_decl_vis` logic is reused when reporting `ambiguous_import_visibilities` lint Some asserts are hardened Some relevant tests are added
1 parent 38c14f3 commit 394c717

15 files changed

Lines changed: 348 additions & 68 deletions

‎compiler/rustc_resolve/src/ident.rs‎

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ use Namespace::*;
55
use rustc_ast::{self as ast, NodeId};
66
use rustc_errors::ErrorGuaranteed;
77
use rustc_hir::def::{DefKind, MacroKinds, Namespace, NonMacroAttrKind, PartialRes, PerNS};
8-
use rustc_middle::ty::Visibility;
98
use rustc_middle::{bug, span_bug};
109
use rustc_session::lint::builtin::PROC_MACRO_DERIVE_RESOLUTION_FALLBACK;
1110
use rustc_session::parse::feature_err;
@@ -24,9 +23,9 @@ use crate::late::{
2423
use crate::macros::{MacroRulesScope, sub_namespace_match};
2524
use crate::{
2625
AmbiguityError, AmbiguityKind, AmbiguityWarning, BindingKey, CmResolver, Decl, DeclKind,
27-
Determinacy, Finalize, IdentKey, ImportKind, LateDecl, LocalModule, Module, ModuleKind,
28-
ModuleOrUniformRoot, ParentScope, PathResult, PrivacyError, Res, ResolutionError, Resolver,
29-
Scope, ScopeSet, Segment, Stage, Symbol, Used, errors,
26+
Determinacy, Finalize, IdentKey, ImportKind, ImportSummary, LateDecl, LocalModule, Module,
27+
ModuleKind, ModuleOrUniformRoot, ParentScope, PathResult, PrivacyError, Res, ResolutionError,
28+
Resolver, Scope, ScopeSet, Segment, Stage, Symbol, Used, errors,
3029
};
3130

3231
#[derive(Copy, Clone)]
@@ -485,11 +484,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
485484
// We do not need to report them if we are either in speculative resolution,
486485
// or in late resolution when everything is already imported and expanded
487486
// and no ambiguities exist.
488-
let import_vis = match finalize {
487+
let import = match finalize {
489488
None | Some(Finalize { stage: Stage::Late, .. }) => {
490489
return ControlFlow::Break(Ok(decl));
491490
}
492-
Some(Finalize { import_vis, .. }) => import_vis,
491+
Some(Finalize { import, .. }) => import,
493492
};
494493

495494
if let Some(&(innermost_decl, _)) = innermost_results.first() {
@@ -503,7 +502,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
503502
decl,
504503
scope,
505504
&innermost_results,
506-
import_vis,
505+
import,
507506
) {
508507
// No need to search for more potential ambiguities, one is enough.
509508
return ControlFlow::Break(Ok(innermost_decl));
@@ -790,19 +789,18 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
790789
decl: Decl<'ra>,
791790
scope: Scope<'ra>,
792791
innermost_results: &[(Decl<'ra>, Scope<'ra>)],
793-
import_vis: Option<Visibility>,
792+
import: Option<ImportSummary>,
794793
) -> bool {
795794
let (innermost_decl, innermost_scope) = innermost_results[0];
796795
let (res, innermost_res) = (decl.res(), innermost_decl.res());
797796
let ambig_vis = if res != innermost_res {
798797
None
799-
} else if let Some(import_vis) = import_vis
800-
&& let min =
801-
(|d: Decl<'_>| d.vis().min(import_vis.to_def_id(), self.tcx).expect_local())
802-
&& let (min1, min2) = (min(decl), min(innermost_decl))
803-
&& min1 != min2
798+
} else if let Some(import) = import
799+
&& let vis1 = self.import_decl_vis(decl, import)
800+
&& let vis2 = self.import_decl_vis(innermost_decl, import)
801+
&& vis1 != vis2
804802
{
805-
Some((min1, min2))
803+
Some((vis1, vis2))
806804
} else {
807805
return false;
808806
};

‎compiler/rustc_resolve/src/imports.rs‎

Lines changed: 52 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,9 @@ use crate::errors::{
3737
use crate::ref_mut::CmCell;
3838
use crate::{
3939
AmbiguityError, BindingKey, CmResolver, Decl, DeclData, DeclKind, Determinacy, Finalize,
40-
IdentKey, ImportSuggestion, LocalModule, ModuleOrUniformRoot, ParentScope, PathResult, PerNS,
41-
Res, ResolutionError, Resolver, ScopeSet, Segment, Used, module_to_string, names_to_string,
40+
IdentKey, ImportSuggestion, ImportSummary, LocalModule, ModuleOrUniformRoot, ParentScope,
41+
PathResult, PerNS, Res, ResolutionError, Resolver, ScopeSet, Segment, Used, module_to_string,
42+
names_to_string,
4243
};
4344

4445
/// A potential import declaration in the process of being planted into a module.
@@ -267,6 +268,14 @@ impl<'ra> ImportData<'ra> {
267268
ImportKind::MacroExport => Reexport::MacroExport,
268269
}
269270
}
271+
272+
fn summary(&self) -> ImportSummary {
273+
ImportSummary {
274+
vis: self.vis,
275+
nearest_parent_mod: self.parent_scope.module.nearest_parent_mod().expect_local(),
276+
is_single: matches!(self.kind, ImportKind::Single { .. }),
277+
}
278+
}
270279
}
271280

272281
/// Records information about the resolution of a name in a namespace of a module.
@@ -327,9 +336,9 @@ struct UnresolvedImportError {
327336

328337
// Reexports of the form `pub use foo as bar;` where `foo` is `extern crate foo;`
329338
// are permitted for backward-compatibility under a deprecation lint.
330-
fn pub_use_of_private_extern_crate_hack(import: Import<'_>, decl: Decl<'_>) -> Option<NodeId> {
331-
match (&import.kind, &decl.kind) {
332-
(ImportKind::Single { .. }, DeclKind::Import { import: decl_import, .. })
339+
fn pub_use_of_private_extern_crate_hack(import: ImportSummary, decl: Decl<'_>) -> Option<NodeId> {
340+
match (import.is_single, decl.kind) {
341+
(true, DeclKind::Import { import: decl_import, .. })
333342
if let ImportKind::ExternCrate { id, .. } = decl_import.kind
334343
&& import.vis.is_public() =>
335344
{
@@ -361,31 +370,50 @@ fn remove_same_import<'ra>(d1: Decl<'ra>, d2: Decl<'ra>) -> (Decl<'ra>, Decl<'ra
361370
}
362371

363372
impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
373+
pub(crate) fn import_decl_vis(&self, decl: Decl<'ra>, import: ImportSummary) -> Visibility {
374+
assert!(import.vis.is_accessible_from(import.nearest_parent_mod, self.tcx));
375+
let decl_vis = decl.vis();
376+
if decl_vis.is_at_least(import.vis, self.tcx) {
377+
// Ordered, import is less visible than the imported declaration, or the same,
378+
// use the import's visibility.
379+
import.vis
380+
} else if decl_vis.is_accessible_from(import.nearest_parent_mod, self.tcx) {
381+
// Ordered, imported declaration is less visible than the import, but is still visible
382+
// from the current module, use the declaration's visibility.
383+
assert!(import.vis.is_at_least(decl_vis, self.tcx));
384+
if pub_use_of_private_extern_crate_hack(import, decl).is_some() {
385+
import.vis
386+
} else {
387+
decl_vis.expect_local()
388+
}
389+
} else {
390+
// Ordered or not, the imported declaration is too private for the current module.
391+
// It doesn't matter what visibility we choose here (except in the `PRIVATE_MACRO_USE`
392+
// case), because either some error will be reported, or the import declaration
393+
// will be thrown away (unfortunately cannot use delayed bug here for this reason).
394+
// Use import visibility to keep the all declaration visibilities in a module ordered.
395+
import.vis
396+
}
397+
}
398+
364399
/// Given an import and the declaration that it points to,
365400
/// create the corresponding import declaration.
366401
pub(crate) fn new_import_decl(&self, decl: Decl<'ra>, import: Import<'ra>) -> Decl<'ra> {
367-
let import_vis = import.vis.to_def_id();
368-
let vis = if decl.vis().is_at_least(import_vis, self.tcx)
369-
|| pub_use_of_private_extern_crate_hack(import, decl).is_some()
370-
{
371-
import_vis
372-
} else {
373-
decl.vis()
374-
};
402+
let vis = self.import_decl_vis(decl, import.summary());
375403

376404
if let ImportKind::Glob { ref max_vis, .. } = import.kind
377-
&& (vis == import_vis
405+
&& (vis == import.vis
378406
|| max_vis.get().is_none_or(|max_vis| vis.is_at_least(max_vis, self.tcx)))
379407
{
380-
max_vis.set_unchecked(Some(vis.expect_local()))
408+
max_vis.set_unchecked(Some(vis))
381409
}
382410

383411
self.arenas.alloc_decl(DeclData {
384412
kind: DeclKind::Import { source_decl: decl, import },
385413
ambiguity: CmCell::new(None),
386414
warn_ambiguity: CmCell::new(false),
387415
span: import.span,
388-
vis: CmCell::new(vis),
416+
vis: CmCell::new(vis.to_def_id()),
389417
expansion: import.parent_scope.expansion,
390418
parent_module: Some(import.parent_scope.module),
391419
})
@@ -448,6 +476,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
448476
}
449477
} else if !old_glob_decl.vis().is_at_least(glob_decl.vis(), self.tcx) {
450478
// We are glob-importing the same item but with greater visibility.
479+
// All visibilities here are ordered because all of them are ancestors of `module`.
451480
// FIXME: Update visibility in place, but without regressions
452481
// (#152004, #151124, #152347).
453482
glob_decl
@@ -471,7 +500,10 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
471500
decl: Decl<'ra>,
472501
warn_ambiguity: bool,
473502
) -> Result<(), Decl<'ra>> {
503+
assert!(!decl.warn_ambiguity.get());
504+
assert!(decl.ambiguity.get().is_none());
474505
let module = decl.parent_module.unwrap().expect_local();
506+
assert!(self.is_accessible_from(decl.vis(), module.to_module()));
475507
let res = decl.res();
476508
self.check_reserved_macro_name(ident.name, orig_ident_span, res);
477509
// Even if underscore names cannot be looked up, we still need to add them to modules,
@@ -487,7 +519,6 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
487519
orig_ident_span,
488520
warn_ambiguity,
489521
|this, resolution| {
490-
assert!(!decl.warn_ambiguity.get());
491522
if decl.is_glob_import() {
492523
resolution.glob_decl = Some(match resolution.glob_decl {
493524
Some(old_decl) => this.select_glob_decl(
@@ -1261,7 +1292,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
12611292
&import.parent_scope,
12621293
Some(Finalize {
12631294
report_private: false,
1264-
import_vis: Some(import.vis),
1295+
import: Some(import.summary()),
12651296
..finalize
12661297
}),
12671298
bindings[ns].get().decl(),
@@ -1461,7 +1492,9 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
14611492
// All namespaces must be re-exported with extra visibility for an error to occur.
14621493
if !any_successful_reexport {
14631494
let (ns, binding) = reexport_error.unwrap();
1464-
if let Some(extern_crate_id) = pub_use_of_private_extern_crate_hack(import, binding) {
1495+
if let Some(extern_crate_id) =
1496+
pub_use_of_private_extern_crate_hack(import.summary(), binding)
1497+
{
14651498
let extern_crate_sp = self.tcx.source_span(self.local_def_id(extern_crate_id));
14661499
self.lint_buffer.buffer_lint(
14671500
PUB_USE_OF_PRIVATE_EXTERN_CRATE,

‎compiler/rustc_resolve/src/lib.rs‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2722,6 +2722,15 @@ enum Stage {
27222722
Late,
27232723
}
27242724

2725+
/// Parts of import data required for finalizing import resolution.
2726+
/// Does not carry a lifetime, so it can be stored in `Finalize`.
2727+
#[derive(Copy, Clone, Debug)]
2728+
struct ImportSummary {
2729+
vis: Visibility,
2730+
nearest_parent_mod: LocalDefId,
2731+
is_single: bool,
2732+
}
2733+
27252734
/// Invariant: if `Finalize` is used, expansion and import resolution must be complete.
27262735
#[derive(Copy, Clone, Debug)]
27272736
struct Finalize {
@@ -2740,8 +2749,8 @@ struct Finalize {
27402749
used: Used = Used::Other,
27412750
/// Finalizing early or late resolution.
27422751
stage: Stage = Stage::Early,
2743-
/// Nominal visibility of the import item, in case we are resolving an import's final segment.
2744-
import_vis: Option<Visibility> = None,
2752+
/// Some import data, in case we are resolving an import's final segment.
2753+
import: Option<ImportSummary> = None,
27452754
}
27462755

27472756
impl Finalize {

‎tests/ui/imports/ambiguous-import-visibility-globglob-priv-pass.rs‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
//@ check-pass
2+
3+
mod m {
4+
pub struct S {}
5+
}
6+
7+
mod one_private {
8+
use crate::m::*;
9+
pub use crate::m::*;
10+
}
11+
12+
// One of the ambiguous imports is not visible from here,
13+
// and does not contribute to the ambiguity.
14+
use crate::one_private::S;
15+
16+
// Separate module to make visibilities `in crate::inner` and `in crate::one_private` unordered.
17+
mod inner {
18+
// One of the ambiguous imports is not visible from here,
19+
// and does not contribute to the ambiguity.
20+
use crate::one_private::S;
21+
}
22+
23+
fn main() {}

‎tests/ui/imports/ambiguous-import-visibility-globglob-priv.rs‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
mod m {
2+
pub struct S {}
3+
}
4+
5+
mod both {
6+
pub mod private {
7+
use crate::m::*;
8+
pub(super) use crate::m::*;
9+
}
10+
}
11+
12+
use crate::both::private::S;
13+
//~^ ERROR struct import `S` is private
14+
15+
// Separate module to make visibilities `in crate::inner` and `in crate::both(::private)` unordered.
16+
mod inner {
17+
use crate::both::private::S;
18+
//~^ ERROR struct import `S` is private
19+
}
20+
21+
fn main() {}

‎tests/ui/imports/ambiguous-import-visibility-globglob-priv.stderr‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
error[E0603]: struct import `S` is private
2+
--> $DIR/ambiguous-import-visibility-globglob-priv.rs:12:27
3+
|
4+
LL | use crate::both::private::S;
5+
| ^ private struct import
6+
|
7+
note: the struct import `S` is defined here...
8+
--> $DIR/ambiguous-import-visibility-globglob-priv.rs:8:24
9+
|
10+
LL | pub(super) use crate::m::*;
11+
| ^^^^^^^^^^^
12+
note: ...and refers to the struct `S` which is defined here
13+
--> $DIR/ambiguous-import-visibility-globglob-priv.rs:2:5
14+
|
15+
LL | pub struct S {}
16+
| ^^^^^^^^^^^^ you could import this directly
17+
help: import `S` through the re-export
18+
|
19+
LL - use crate::both::private::S;
20+
LL + use m::S;
21+
|
22+
23+
error[E0603]: struct import `S` is private
24+
--> $DIR/ambiguous-import-visibility-globglob-priv.rs:17:31
25+
|
26+
LL | use crate::both::private::S;
27+
| ^ private struct import
28+
|
29+
note: the struct import `S` is defined here...
30+
--> $DIR/ambiguous-import-visibility-globglob-priv.rs:8:24
31+
|
32+
LL | pub(super) use crate::m::*;
33+
| ^^^^^^^^^^^
34+
note: ...and refers to the struct `S` which is defined here
35+
--> $DIR/ambiguous-import-visibility-globglob-priv.rs:2:5
36+
|
37+
LL | pub struct S {}
38+
| ^^^^^^^^^^^^ you could import this directly
39+
help: import `S` through the re-export
40+
|
41+
LL - use crate::both::private::S;
42+
LL + use m::S;
43+
|
44+
45+
error: aborting due to 2 previous errors
46+
47+
For more information about this error, try `rustc --explain E0603`.

‎tests/ui/imports/ambiguous-import-visibility-globglob.rs‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
//@ check-pass
2+
3+
// FIXME: should report "ambiguous import visibility" in all the cases below.
4+
5+
mod m {
6+
pub struct S {}
7+
}
8+
9+
mod min_vis_first {
10+
use crate::m::*;
11+
pub(crate) use crate::m::*;
12+
pub use crate::m::*;
13+
14+
pub use self::S as S1;
15+
pub(crate) use self::S as S2;
16+
use self::S as S3; // OK
17+
}
18+
19+
mod mid_vis_first {
20+
pub(crate) use crate::m::*;
21+
use crate::m::*;
22+
pub use crate::m::*;
23+
24+
pub use self::S as S1;
25+
pub(crate) use self::S as S2;
26+
use self::S as S3; // OK
27+
}
28+
29+
mod max_vis_first {
30+
pub use crate::m::*;
31+
use crate::m::*;
32+
pub(crate) use crate::m::*;
33+
34+
pub use self::S as S1;
35+
pub(crate) use self::S as S2;
36+
use self::S as S3; // OK
37+
}
38+
39+
fn main() {}

0 commit comments

Comments
 (0)