resolve: Make visibility resolution more speculative

petrochenkov · Mark-Simulacrum · commit 8209dbf1fa56 · 2019-12-14T10:51:51.000-05:00
To avoid potential duplicate diagnostics and separate the error reporting logic
diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs
@@ -11,7 +11,7 @@ use crate::resolve_imports::ImportDirectiveSubclass::{self, GlobImport, SingleIm
 use crate::{Module, ModuleData, ModuleKind, NameBinding, NameBindingKind, Segment, ToNameBinding};
 use crate::{ModuleOrUniformRoot, ParentScope, PerNS, Resolver, ResolverArenas, ExternPreludeEntry};
 use crate::Namespace::{self, TypeNS, ValueNS, MacroNS};
-use crate::{ResolutionError, Determinacy, PathResult, CrateLint};
+use crate::{ResolutionError, VisResolutionError, Determinacy, PathResult, CrateLint};
 
 use rustc::bug;
 use rustc::hir::def::{self, *};
@@ -35,7 +35,7 @@ use syntax::ast::{MetaItemKind, StmtKind, TraitItem, TraitItemKind};
 use syntax::feature_gate::is_builtin_attr;
 use syntax::parse::token::{self, Token};
 use syntax::print::pprust;
-use syntax::{span_err, struct_span_err};
+use syntax::span_err;
 use syntax::source_map::{respan, Spanned};
 use syntax::symbol::{kw, sym};
 use syntax::visit::{self, Visitor};
@@ -194,22 +194,25 @@ impl<'a> AsMut<Resolver<'a>> for BuildReducedGraphVisitor<'a, '_> {
 
 impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
     fn resolve_visibility(&mut self, vis: &ast::Visibility) -> ty::Visibility {
-        self.resolve_visibility_speculative(vis, false)
+        self.resolve_visibility_speculative(vis, false).unwrap_or_else(|err| {
+            self.r.report_vis_error(err);
+            ty::Visibility::Public
+        })
     }
 
-    fn resolve_visibility_speculative(
+    fn resolve_visibility_speculative<'ast>(
         &mut self,
-        vis: &ast::Visibility,
+        vis: &'ast ast::Visibility,
         speculative: bool,
-    ) -> ty::Visibility {
+    ) -> Result<ty::Visibility, VisResolutionError<'ast>> {
         let parent_scope = &self.parent_scope;
         match vis.node {
-            ast::VisibilityKind::Public => ty::Visibility::Public,
+            ast::VisibilityKind::Public => Ok(ty::Visibility::Public),
             ast::VisibilityKind::Crate(..) => {
-                ty::Visibility::Restricted(DefId::local(CRATE_DEF_INDEX))
+                Ok(ty::Visibility::Restricted(DefId::local(CRATE_DEF_INDEX)))
             }
             ast::VisibilityKind::Inherited => {
-                ty::Visibility::Restricted(parent_scope.module.normal_ancestor_id)
+                Ok(ty::Visibility::Restricted(parent_scope.module.normal_ancestor_id))
             }
             ast::VisibilityKind::Restricted { ref path, id, .. } => {
                 // For visibilities we are not ready to provide correct implementation of "uniform
@@ -219,32 +222,19 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
                 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.rust_2018() {
-                    let msg = "relative paths are not supported in visibilities on 2018 edition";
-                    self.r.session.struct_span_err(ident.span, msg)
-                        .span_suggestion(
-                            path.span,
-                            "try",
-                            format!("crate::{}", pprust::path_to_string(&path)),
-                            Applicability::MaybeIncorrect,
-                        )
-                        .emit();
-                    return ty::Visibility::Public;
-                } else {
-                    let ctxt = ident.span.ctxt();
+                } else if ident.span.rust_2015() {
                     Some(Segment::from_ident(Ident::new(
-                        kw::PathRoot, path.span.shrink_to_lo().with_ctxt(ctxt)
+                        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 = |this: &Self, res: Res| {
-                    let path_str = Segment::names_to_string(&segments);
-                    struct_span_err!(this.r.session, path.span, E0577,
-                                     "expected module, found {} `{}`", res.descr(), path_str)
-                        .span_label(path.span, "not a module").emit();
-                };
+                let expected_found_error = |res| Err(VisResolutionError::ExpectedFound(
+                    path.span, Segment::names_to_string(&segments), res
+                ));
                 match self.r.resolve_path(
                     &segments,
                     Some(TypeNS),
@@ -260,42 +250,27 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
                         }
                         if module.is_normal() {
                             if res == Res::Err {
-                                ty::Visibility::Public
+                                Ok(ty::Visibility::Public)
                             } else {
                                 let vis = ty::Visibility::Restricted(res.def_id());
                                 if self.r.is_accessible_from(vis, parent_scope.module) {
-                                    vis
+                                    Ok(vis)
                                 } else {
-                                    struct_span_err!(self.r.session, path.span, E0742,
-                                        "visibilities can only be restricted to ancestor modules")
-                                        .emit();
-                                    ty::Visibility::Public
+                                    Err(VisResolutionError::AncestorOnly(path.span))
                                 }
                             }
                         } else {
-                            expected_found_error(self, res);
-                            ty::Visibility::Public
+                            expected_found_error(res)
                         }
                     }
-                    PathResult::Module(..) => {
-                        self.r.session.span_err(path.span, "visibility must resolve to a module");
-                        ty::Visibility::Public
-                    }
-                    PathResult::NonModule(partial_res) => {
-                        expected_found_error(self, partial_res.base_res());
-                        ty::Visibility::Public
-                    }
-                    PathResult::Failed { span, label, suggestion, .. } => {
-                        self.r.report_error(
-                            span, ResolutionError::FailedToResolve { label, suggestion }
-                        );
-                        ty::Visibility::Public
-                    }
-                    PathResult::Indeterminate => {
-                        span_err!(self.r.session, path.span, E0578,
-                                  "cannot determine resolution for the visibility");
-                        ty::Visibility::Public
-                    }
+                    PathResult::Module(..) =>
+                        Err(VisResolutionError::ModuleOnly(path.span)),
+                    PathResult::NonModule(partial_res) =>
+                        expected_found_error(partial_res.base_res()),
+                    PathResult::Failed { span, label, suggestion, .. } =>
+                        Err(VisResolutionError::FailedToResolve(span, label, suggestion)),
+                    PathResult::Indeterminate =>
+                        Err(VisResolutionError::Indeterminate(path.span)),
                 }
             }
         }
@@ -764,9 +739,11 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
                         // NOTE: The field may be an expansion placeholder, but expansion sets
                         // correct visibilities for unnamed field placeholders specifically, so the
                         // constructor visibility should still be determined correctly.
-                        let field_vis = self.resolve_visibility_speculative(&field.vis, true);
-                        if ctor_vis.is_at_least(field_vis, &*self.r) {
-                            ctor_vis = field_vis;
+                        if let Ok(field_vis) =
+                                self.resolve_visibility_speculative(&field.vis, true) {
+                            if ctor_vis.is_at_least(field_vis, &*self.r) {
+                                ctor_vis = field_vis;
+                            }
                         }
                     }
                     let ctor_res = Res::Def(
diff --git a/src/librustc_resolve/diagnostics.rs b/src/librustc_resolve/diagnostics.rs
@@ -11,6 +11,7 @@ use rustc::ty::{self, DefIdTree};
 use rustc::util::nodemap::FxHashSet;
 use syntax::ast::{self, Ident, Path};
 use syntax::feature_gate::BUILTIN_ATTRIBUTES;
+use syntax::print::pprust;
 use syntax::source_map::SourceMap;
 use syntax::struct_span_err;
 use syntax::symbol::{Symbol, kw};
@@ -22,6 +23,7 @@ use crate::resolve_imports::{ImportDirective, ImportDirectiveSubclass, ImportRes
 use crate::{path_names_to_string, KNOWN_TOOLS};
 use crate::{BindingError, CrateLint, HasGenericParams, LegacyScope, Module, ModuleOrUniformRoot};
 use crate::{PathResult, ParentScope, ResolutionError, Resolver, Scope, ScopeSet, Segment};
+use crate::VisResolutionError;
 
 type Res = def::Res<ast::NodeId>;
 
@@ -355,6 +357,44 @@ impl<'a> Resolver<'a> {
         }
     }
 
+    crate fn report_vis_error(&self, vis_resolution_error: VisResolutionError<'_>) {
+        match vis_resolution_error {
+            VisResolutionError::Relative2018(span, path) => {
+                let mut err = self.session.struct_span_err(span,
+                    "relative paths are not supported in visibilities on 2018 edition");
+                err.span_suggestion(
+                    path.span,
+                    "try",
+                    format!("crate::{}", pprust::path_to_string(&path)),
+                    Applicability::MaybeIncorrect,
+                );
+                err
+            }
+            VisResolutionError::AncestorOnly(span) => {
+                struct_span_err!(self.session, span, E0742,
+                    "visibilities can only be restricted to ancestor modules")
+            }
+            VisResolutionError::FailedToResolve(span, label, suggestion) => {
+                self.into_struct_error(
+                    span, ResolutionError::FailedToResolve { label, suggestion }
+                )
+            }
+            VisResolutionError::ExpectedFound(span, path_str, res) => {
+                let mut err = struct_span_err!(self.session, span, E0577,
+                    "expected module, found {} `{}`", res.descr(), path_str);
+                err.span_label(span, "not a module");
+                err
+            }
+            VisResolutionError::Indeterminate(span) => {
+                struct_span_err!(self.session, span, E0578,
+                    "cannot determine resolution for the visibility")
+            }
+            VisResolutionError::ModuleOnly(span) => {
+                self.session.struct_span_err(span, "visibility must resolve to a module")
+            }
+        }.emit()
+    }
+
     /// Lookup typo candidate in scope for a macro or import.
     fn early_lookup_typo_candidate(
         &mut self,
diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs
@@ -218,6 +218,15 @@ enum ResolutionError<'a> {
     SelfInTyParamDefault,
 }
 
+enum VisResolutionError<'a> {
+    Relative2018(Span, &'a ast::Path),
+    AncestorOnly(Span),
+    FailedToResolve(Span, String, Option<Suggestion>),
+    ExpectedFound(Span, String, Res),
+    Indeterminate(Span),
+    ModuleOnly(Span),
+}
+
 // A minimal representation of a path segment. We use this in resolve because
 // we synthesize 'path segments' which don't have the rest of an AST or HIR
 // `PathSegment`.
diff --git a/src/test/ui/attributes/field-attributes-vis-unresolved.rs b/src/test/ui/attributes/field-attributes-vis-unresolved.rs
@@ -20,7 +20,6 @@ struct S {
 struct Z(
     #[rustfmt::skip]
     pub(in nonexistent) u8 //~ ERROR failed to resolve
-                           //~| ERROR cannot determine resolution for the visibility
 );
 
 fn main() {}
diff --git a/src/test/ui/attributes/field-attributes-vis-unresolved.stderr b/src/test/ui/attributes/field-attributes-vis-unresolved.stderr
@@ -1,9 +1,3 @@
-error[E0578]: cannot determine resolution for the visibility
-  --> $DIR/field-attributes-vis-unresolved.rs:22:12
-   |
-LL |     pub(in nonexistent) u8
-   |            ^^^^^^^^^^^
-
 error[E0433]: failed to resolve: maybe a missing crate `nonexistent`?
   --> $DIR/field-attributes-vis-unresolved.rs:17:12
    |
@@ -16,7 +10,6 @@ error[E0433]: failed to resolve: maybe a missing crate `nonexistent`?
 LL |     pub(in nonexistent) u8
    |            ^^^^^^^^^^^ maybe a missing crate `nonexistent`?
 
-error: aborting due to 3 previous errors
+error: aborting due to 2 previous errors
 
-Some errors have detailed explanations: E0433, E0578.
-For more information about an error, try `rustc --explain E0433`.
+For more information about this error, try `rustc --explain E0433`.
