Rollup merge of rust-lang#155948 - SynapLink:fix/pub-visibility-order, r=petrochenkov,mu001999

Fix order-dependent visibility diagnostics

Fixes rust-lang#40066.
Fixes rust-lang#109657.
Delay visibility path diagnostics until module collection has finished, so paths to later non-ancestor modules report E0742 instead of an unresolved path error.

Author: jhpratt

compiler/rustc_resolve/src/build_reduced_graph.rs

@@ -35,9 +35,10 @@ use crate::imports::{ImportData, ImportKind, OnUnknownData};
 use crate::macros::{MacroRulesDecl, MacroRulesScope, MacroRulesScopeRef};
 use crate::ref_mut::CmCell;
 use crate::{
-    BindingKey, Decl, DeclData, DeclKind, ExternModule, ExternPreludeEntry, Finalize, IdentKey,
-    LocalModule, MacroData, Module, ModuleKind, ModuleOrUniformRoot, ParentScope, PathResult, Res,
-    Resolver, Segment, Used, VisResolutionError, errors,
+    BindingKey, Decl, DeclData, DeclKind, DelayedVisResolutionError, ExternModule,
+    ExternPreludeEntry, Finalize, IdentKey, LocalModule, MacroData, Module, ModuleKind,
+    ModuleOrUniformRoot, ParentScope, PathResult, Res, Resolver, Segment, Used, VisResolutionError,
+    errors,
 };
 
 impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
@@ -237,6 +238,111 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         }
     }
 
+    pub(crate) fn try_resolve_visibility(
+        &mut self,
+        parent_scope: &ParentScope<'ra>,
+        vis: &ast::Visibility,
+        finalize: bool,
+    ) -> Result<Visibility, VisResolutionError> {
+        match vis.kind {
+            ast::VisibilityKind::Public => Ok(Visibility::Public),
+            ast::VisibilityKind::Inherited => {
+                Ok(match parent_scope.module.kind {
+                    // Any inherited visibility resolved directly inside an enum or trait
+                    // (i.e. variants, fields, and trait items) inherits from the visibility
+                    // of the enum or trait.
+                    ModuleKind::Def(DefKind::Enum | DefKind::Trait, def_id, _) => {
+                        self.tcx.visibility(def_id).expect_local()
+                    }
+                    // Otherwise, the visibility is restricted to the nearest parent `mod` item.
+                    _ => Visibility::Restricted(
+                        parent_scope.module.nearest_parent_mod().expect_local(),
+                    ),
+                })
+            }
+            ast::VisibilityKind::Restricted { ref path, id, .. } => {
+                // For visibilities we are not ready to provide correct implementation of "uniform
+                // paths" right now, so on 2018 edition we only allow module-relative paths for now.
+                // On 2015 edition visibilities are resolved as crate-relative by default,
+                // so we are prepending a root segment if necessary.
+                let ident = path.segments.get(0).expect("empty path in visibility").ident;
+                let crate_root = if ident.is_path_segment_keyword() {
+                    None
+                } else if ident.span.is_rust_2015() {
+                    Some(Segment::from_ident(Ident::new(
+                        kw::PathRoot,
+                        path.span.shrink_to_lo().with_ctxt(ident.span.ctxt()),
+                    )))
+                } else {
+                    return Err(VisResolutionError::Relative2018(
+                        ident.span,
+                        path.as_ref().clone(),
+                    ));
+                };
+                let segments = crate_root
+                    .into_iter()
+                    .chain(path.segments.iter().map(|seg| seg.into()))
+                    .collect::<Vec<_>>();
+                let expected_found_error = |res| {
+                    Err(VisResolutionError::ExpectedFound(
+                        path.span,
+                        Segment::names_to_string(&segments),
+                        res,
+                    ))
+                };
+                match self.cm().resolve_path(
+                    &segments,
+                    None,
+                    parent_scope,
+                    finalize.then(|| Finalize::new(id, path.span)),
+                    None,
+                    None,
+                ) {
+                    PathResult::Module(ModuleOrUniformRoot::Module(module)) => {
+                        let res = module.res().expect("visibility resolved to unnamed block");
+                        if module.is_normal() {
+                            match res {
+                                Res::Err => {
+                                    if finalize {
+                                        self.record_partial_res(id, PartialRes::new(res));
+                                    }
+                                    Ok(Visibility::Public)
+                                }
+                                _ => {
+                                    let vis = Visibility::Restricted(res.def_id());
+                                    if self.is_accessible_from(vis, parent_scope.module) {
+                                        if finalize {
+                                            self.record_partial_res(id, PartialRes::new(res));
+                                        }
+                                        Ok(vis.expect_local())
+                                    } else {
+                                        Err(VisResolutionError::AncestorOnly(path.span))
+                                    }
+                                }
+                            }
+                        } else {
+                            expected_found_error(res)
+                        }
+                    }
+                    PathResult::Module(..) => Err(VisResolutionError::ModuleOnly(path.span)),
+                    PathResult::NonModule(partial_res) => {
+                        expected_found_error(partial_res.expect_full_res())
+                    }
+                    PathResult::Failed {
+                        span, label, suggestion, message, segment_name, ..
+                    } => Err(VisResolutionError::FailedToResolve(
+                        span,
+                        segment_name,
+                        label,
+                        suggestion,
+                        message,
+                    )),
+                    PathResult::Indeterminate => Err(VisResolutionError::Indeterminate(path.span)),
+                }
+            }
+        }
+    }
+
     pub(crate) fn build_reduced_graph_external(&self, module: ExternModule<'ra>) {
         let def_id = module.def_id();
         let children = self.tcx.module_children(def_id);
@@ -364,106 +470,15 @@ impl<'a, 'ra, 'tcx> DefCollector<'a, 'ra, 'tcx> {
     }
 
     fn resolve_visibility(&mut self, vis: &ast::Visibility) -> Visibility {
-        self.try_resolve_visibility(vis, true).unwrap_or_else(|err| {
-            self.r.report_vis_error(err);
-            Visibility::Public
-        })
-    }
-
-    fn try_resolve_visibility<'ast>(
-        &mut self,
-        vis: &'ast ast::Visibility,
-        finalize: bool,
-    ) -> Result<Visibility, VisResolutionError<'ast>> {
-        let parent_scope = &self.parent_scope;
-        match vis.kind {
-            ast::VisibilityKind::Public => Ok(Visibility::Public),
-            ast::VisibilityKind::Inherited => {
-                Ok(match self.parent_scope.module.kind {
-                    // Any inherited visibility resolved directly inside an enum or trait
-                    // (i.e. variants, fields, and trait items) inherits from the visibility
-                    // of the enum or trait.
-                    ModuleKind::Def(DefKind::Enum | DefKind::Trait, def_id, _) => {
-                        self.r.tcx.visibility(def_id).expect_local()
-                    }
-                    // Otherwise, the visibility is restricted to the nearest parent `mod` item.
-                    _ => Visibility::Restricted(
-                        self.parent_scope.module.nearest_parent_mod().expect_local(),
-                    ),
-                })
-            }
-            ast::VisibilityKind::Restricted { ref path, id, .. } => {
-                // For visibilities we are not ready to provide correct implementation of "uniform
-                // paths" right now, so on 2018 edition we only allow module-relative paths for now.
-                // On 2015 edition visibilities are resolved as crate-relative by default,
-                // so we are prepending a root segment if necessary.
-                let ident = path.segments.get(0).expect("empty path in visibility").ident;
-                let crate_root = if ident.is_path_segment_keyword() {
-                    None
-                } else if ident.span.is_rust_2015() {
-                    Some(Segment::from_ident(Ident::new(
-                        kw::PathRoot,
-                        path.span.shrink_to_lo().with_ctxt(ident.span.ctxt()),
-                    )))
-                } else {
-                    return Err(VisResolutionError::Relative2018(ident.span, path));
-                };
-
-                let segments = crate_root
-                    .into_iter()
-                    .chain(path.segments.iter().map(|seg| seg.into()))
-                    .collect::<Vec<_>>();
-                let expected_found_error = |res| {
-                    Err(VisResolutionError::ExpectedFound(
-                        path.span,
-                        Segment::names_to_string(&segments),
-                        res,
-                    ))
-                };
-                match self.r.cm().resolve_path(
-                    &segments,
-                    None,
-                    parent_scope,
-                    finalize.then(|| Finalize::new(id, path.span)),
-                    None,
-                    None,
-                ) {
-                    PathResult::Module(ModuleOrUniformRoot::Module(module)) => {
-                        let res = module.res().expect("visibility resolved to unnamed block");
-                        if finalize {
-                            self.r.record_partial_res(id, PartialRes::new(res));
-                        }
-                        if module.is_normal() {
-                            match res {
-                                Res::Err => Ok(Visibility::Public),
-                                _ => {
-                                    let vis = Visibility::Restricted(res.def_id());
-                                    if self.r.is_accessible_from(vis, parent_scope.module) {
-                                        Ok(vis.expect_local())
-                                    } else {
-                                        Err(VisResolutionError::AncestorOnly(path.span))
-                                    }
-                                }
-                            }
-                        } else {
-                            expected_found_error(res)
-                        }
-                    }
-                    PathResult::Module(..) => Err(VisResolutionError::ModuleOnly(path.span)),
-                    PathResult::NonModule(partial_res) => {
-                        expected_found_error(partial_res.expect_full_res())
-                    }
-                    PathResult::Failed {
-                        span, label, suggestion, message, segment_name, ..
-                    } => Err(VisResolutionError::FailedToResolve(
-                        span,
-                        segment_name,
-                        label,
-                        suggestion,
-                        message,
-                    )),
-                    PathResult::Indeterminate => Err(VisResolutionError::Indeterminate(path.span)),
-                }
+        match self.r.try_resolve_visibility(&self.parent_scope, vis, true) {
+            Ok(vis) => vis,
+            Err(error) => {
+                self.r.delayed_vis_resolution_errors.push(DelayedVisResolutionError {
+                    vis: vis.clone(),
+                    parent_scope: self.parent_scope,
+                    error,
+                });
+                Visibility::Public
             }
         }
     }
@@ -906,7 +921,8 @@ impl<'a, 'ra, 'tcx> DefCollector<'a, 'ra, 'tcx> {
                         // correct visibilities for unnamed field placeholders specifically, so the
                         // constructor visibility should still be determined correctly.
                         let field_vis = self
-                            .try_resolve_visibility(&field.vis, false)
+                            .r
+                            .try_resolve_visibility(&self.parent_scope, &field.vis, false)
                             .unwrap_or(Visibility::Public);
                         if ctor_vis.greater_than(field_vis, self.r.tcx) {
                             ctor_vis = field_vis;
@@ -1341,9 +1357,10 @@ impl<'a, 'ra, 'tcx> DefCollector<'a, 'ra, 'tcx> {
             let vis = match item.kind {
                 // Visibilities must not be resolved non-speculatively twice
                 // and we already resolved this one as a `fn` item visibility.
-                ItemKind::Fn(..) => {
-                    self.try_resolve_visibility(&item.vis, false).unwrap_or(Visibility::Public)
-                }
+                ItemKind::Fn(..) => self
+                    .r
+                    .try_resolve_visibility(&self.parent_scope, &item.vis, false)
+                    .unwrap_or(Visibility::Public),
                 _ => self.resolve_visibility(&item.vis),
             };
             if !vis.is_public() {

compiler/rustc_resolve/src/diagnostics.rs

@@ -1,4 +1,5 @@
 // ignore-tidy-filelength
+use std::mem;
 use std::ops::ControlFlow;
 
 use itertools::Itertools as _;
@@ -48,10 +49,10 @@ use crate::imports::{Import, ImportKind};
 use crate::late::{DiagMetadata, PatternSource, Rib};
 use crate::{
     AmbiguityError, AmbiguityKind, AmbiguityWarning, BindingError, BindingKey, Decl, DeclKind,
-    Finalize, ForwardGenericParamBanReason, HasGenericParams, IdentKey, LateDecl, MacroRulesScope,
-    Module, ModuleKind, ModuleOrUniformRoot, ParentScope, PathResult, PrivacyError, Res,
-    ResolutionError, Resolver, Scope, ScopeSet, Segment, UseError, Used, VisResolutionError,
-    errors as errs, path_names_to_string,
+    DelayedVisResolutionError, Finalize, ForwardGenericParamBanReason, HasGenericParams, IdentKey,
+    LateDecl, MacroRulesScope, Module, ModuleKind, ModuleOrUniformRoot, ParentScope, PathResult,
+    PrivacyError, Res, ResolutionError, Resolver, Scope, ScopeSet, Segment, UseError, Used,
+    VisResolutionError, errors as errs, path_names_to_string,
 };
 
 /// A vector of spans and replacements, a message and applicability.
@@ -137,6 +138,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
     }
 
     pub(crate) fn report_errors(&mut self, krate: &Crate) {
+        self.report_delayed_vis_resolution_errors();
         self.report_with_use_injections(krate);
 
         for &(span_use, span_def) in &self.macro_expanded_macro_export_errors {
@@ -171,16 +173,27 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         }
 
         let mut reported_spans = FxHashSet::default();
-        for error in std::mem::take(&mut self.privacy_errors) {
+        for error in mem::take(&mut self.privacy_errors) {
             if reported_spans.insert(error.dedup_span) {
                 self.report_privacy_error(&error);
             }
         }
     }
 
+    fn report_delayed_vis_resolution_errors(&mut self) {
+        for DelayedVisResolutionError { vis, parent_scope, error } in
+            mem::take(&mut self.delayed_vis_resolution_errors)
+        {
+            match self.try_resolve_visibility(&parent_scope, &vis, true) {
+                Ok(_) => self.report_vis_error(error),
+                Err(error) => self.report_vis_error(error),
+            };
+        }
+    }
+
     fn report_with_use_injections(&mut self, krate: &Crate) {
         for UseError { mut err, candidates, def_id, instead, suggestion, path, is_call } in
-            std::mem::take(&mut self.use_injections)
+            mem::take(&mut self.use_injections)
         {
             let (span, found_use) = if let Some(def_id) = def_id.as_local() {
                 UsePlacementFinder::check(krate, self.def_id_to_node_id(def_id))
@@ -1137,7 +1150,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
 
     pub(crate) fn report_vis_error(
         &mut self,
-        vis_resolution_error: VisResolutionError<'_>,
+        vis_resolution_error: VisResolutionError,
     ) -> ErrorGuaranteed {
         match vis_resolution_error {
             VisResolutionError::Relative2018(span, path) => {
@@ -1146,7 +1159,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                     path_span: path.span,
                     // intentionally converting to String, as the text would also be used as
                     // in suggestion context
-                    path_str: pprust::path_to_string(path),
+                    path_str: pprust::path_to_string(&path),
                 })
             }
             VisResolutionError::AncestorOnly(span) => {

compiler/rustc_resolve/src/lib.rs

@@ -337,8 +337,9 @@ enum ResolutionError<'ra> {
     BindingInNeverPattern,
 }
 
-enum VisResolutionError<'a> {
-    Relative2018(Span, &'a ast::Path),
+#[derive(Debug)]
+enum VisResolutionError {
+    Relative2018(Span, ast::Path),
     AncestorOnly(Span),
     FailedToResolve(Span, Symbol, String, Option<Suggestion>, String),
     ExpectedFound(Span, String, Res),
@@ -1004,6 +1005,13 @@ struct UseError<'a> {
     is_call: bool,
 }
 
+#[derive(Debug)]
+struct DelayedVisResolutionError<'ra> {
+    vis: ast::Visibility,
+    parent_scope: ParentScope<'ra>,
+    error: VisResolutionError,
+}
+
 #[derive(Clone, Copy, PartialEq, Debug)]
 enum AmbiguityKind {
     BuiltinAttr,
@@ -1364,6 +1372,8 @@ pub struct Resolver<'ra, 'tcx> {
     issue_145575_hack_applied: bool = false,
     /// `use` injections are delayed for better placement and deduplication.
     use_injections: Vec<UseError<'tcx>> = Vec::new(),
+    /// Visibility path resolution failures are delayed until all modules are collected.
+    delayed_vis_resolution_errors: Vec<DelayedVisResolutionError<'ra>> = Vec::new(),
     /// Crate-local macro expanded `macro_export` referred to by a module-relative path.
     macro_expanded_macro_export_errors: BTreeSet<(Span, Span)> = BTreeSet::new(),