Give better diagnostic when using a private tuple struct constructor

Author: guswynn

compiler/rustc_resolve/src/build_reduced_graph.rs

@@ -796,23 +796,26 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
                         vis
                     };
 
+                    let mut ret_fields = Vec::with_capacity(vdata.fields().len());
+
                     for field in vdata.fields() {
                         // 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.
-                        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 field_vis = self
+                            .resolve_visibility_speculative(&field.vis, true)
+                            .unwrap_or(ty::Visibility::Public);
+                        if ctor_vis.is_at_least(field_vis, &*self.r) {
+                            ctor_vis = field_vis;
                         }
+                        ret_fields.push(field_vis);
                     }
                     let ctor_res = Res::Def(
                         DefKind::Ctor(CtorOf::Struct, CtorKind::from_ast(vdata)),
                         self.r.local_def_id(ctor_node_id).to_def_id(),
                     );
                     self.r.define(parent, ident, ValueNS, (ctor_res, ctor_vis, sp, expansion));
-                    self.r.struct_constructors.insert(def_id, (ctor_res, ctor_vis));
+                    self.r.struct_constructors.insert(def_id, (ctor_res, ctor_vis, ret_fields));
                 }
             }
 
@@ -964,7 +967,7 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
             Res::Def(DefKind::Ctor(CtorOf::Struct, ..), def_id) => {
                 let parent = cstore.def_key(def_id).parent;
                 if let Some(struct_def_id) = parent.map(|index| DefId { index, ..def_id }) {
-                    self.r.struct_constructors.insert(struct_def_id, (res, vis));
+                    self.r.struct_constructors.insert(struct_def_id, (res, vis, vec![]));
                 }
             }
             _ => {}

compiler/rustc_resolve/src/late.rs

@@ -188,7 +188,7 @@ crate enum PathSource<'a> {
     // Paths in struct expressions and patterns `Path { .. }`.
     Struct,
     // Paths in tuple struct patterns `Path(..)`.
-    TupleStruct(Span),
+    TupleStruct(Span, &'a [Span]),
     // `m::A::B` in `<T as m::A>::B::C`.
     TraitItem(Namespace),
 }
@@ -197,7 +197,7 @@ impl<'a> PathSource<'a> {
     fn namespace(self) -> Namespace {
         match self {
             PathSource::Type | PathSource::Trait(_) | PathSource::Struct => TypeNS,
-            PathSource::Expr(..) | PathSource::Pat | PathSource::TupleStruct(_) => ValueNS,
+            PathSource::Expr(..) | PathSource::Pat | PathSource::TupleStruct(..) => ValueNS,
             PathSource::TraitItem(ns) => ns,
         }
     }
@@ -208,7 +208,7 @@ impl<'a> PathSource<'a> {
             | PathSource::Expr(..)
             | PathSource::Pat
             | PathSource::Struct
-            | PathSource::TupleStruct(_) => true,
+            | PathSource::TupleStruct(..) => true,
             PathSource::Trait(_) | PathSource::TraitItem(..) => false,
         }
     }
@@ -219,7 +219,7 @@ impl<'a> PathSource<'a> {
             PathSource::Trait(_) => "trait",
             PathSource::Pat => "unit struct, unit variant or constant",
             PathSource::Struct => "struct, variant or union type",
-            PathSource::TupleStruct(_) => "tuple struct or tuple variant",
+            PathSource::TupleStruct(..) => "tuple struct or tuple variant",
             PathSource::TraitItem(ns) => match ns {
                 TypeNS => "associated type",
                 ValueNS => "method or associated constant",
@@ -305,7 +305,7 @@ impl<'a> PathSource<'a> {
                 | Res::SelfCtor(..) => true,
                 _ => false,
             },
-            PathSource::TupleStruct(_) => match res {
+            PathSource::TupleStruct(..) => match res {
                 Res::Def(DefKind::Ctor(_, CtorKind::Fn), _) | Res::SelfCtor(..) => true,
                 _ => false,
             },
@@ -340,8 +340,8 @@ impl<'a> PathSource<'a> {
             (PathSource::Struct, false) => error_code!(E0422),
             (PathSource::Expr(..), true) => error_code!(E0423),
             (PathSource::Expr(..), false) => error_code!(E0425),
-            (PathSource::Pat | PathSource::TupleStruct(_), true) => error_code!(E0532),
-            (PathSource::Pat | PathSource::TupleStruct(_), false) => error_code!(E0531),
+            (PathSource::Pat | PathSource::TupleStruct(..), true) => error_code!(E0532),
+            (PathSource::Pat | PathSource::TupleStruct(..), false) => error_code!(E0531),
             (PathSource::TraitItem(..), true) => error_code!(E0575),
             (PathSource::TraitItem(..), false) => error_code!(E0576),
         }
@@ -411,7 +411,7 @@ struct LateResolutionVisitor<'a, 'b, 'ast> {
 }
 
 /// Walks the whole crate in DFS order, visiting each item, resolving names as it goes.
-impl<'a, 'ast> Visitor<'ast> for LateResolutionVisitor<'a, '_, 'ast> {
+impl<'a: 'ast, 'ast> Visitor<'ast> for LateResolutionVisitor<'a, '_, 'ast> {
     fn visit_item(&mut self, item: &'ast Item) {
         let prev = replace(&mut self.diagnostic_metadata.current_item, Some(item));
         // Always report errors in items we just entered.
@@ -659,7 +659,7 @@ impl<'a, 'ast> Visitor<'ast> for LateResolutionVisitor<'a, '_, 'ast> {
     }
 }
 
-impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
+impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
     fn new(resolver: &'b mut Resolver<'a>) -> LateResolutionVisitor<'a, 'b, 'ast> {
         // During late resolution we only track the module component of the parent scope,
         // although it may be useful to track other components as well for diagnostics.
@@ -1539,8 +1539,16 @@ impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
                         .unwrap_or_else(|| self.fresh_binding(ident, pat.id, pat_src, bindings));
                     self.r.record_partial_res(pat.id, PartialRes::new(res));
                 }
-                PatKind::TupleStruct(ref path, ..) => {
-                    self.smart_resolve_path(pat.id, None, path, PathSource::TupleStruct(pat.span));
+                PatKind::TupleStruct(ref path, ref sub_patterns) => {
+                    self.smart_resolve_path(
+                        pat.id,
+                        None,
+                        path,
+                        PathSource::TupleStruct(
+                            pat.span,
+                            self.r.arenas.alloc_pattern_spans(sub_patterns.iter().map(|p| p.span)),
+                        ),
+                    );
                 }
                 PatKind::Path(ref qself, ref path) => {
                     self.smart_resolve_path(pat.id, qself.as_ref(), path, PathSource::Pat);

compiler/rustc_resolve/src/late/diagnostics.rs

@@ -89,7 +89,7 @@ fn import_candidate_to_enum_paths(suggestion: &ImportSuggestion) -> (String, Str
     (variant_path_string, enum_path_string)
 }
 
-impl<'a> LateResolutionVisitor<'a, '_, '_> {
+impl<'a: 'ast, 'ast> LateResolutionVisitor<'a, '_, 'ast> {
     fn def_span(&self, def_id: DefId) -> Option<Span> {
         match def_id.krate {
             LOCAL_CRATE => self.r.opt_span(def_id),
@@ -622,12 +622,12 @@ impl<'a> LateResolutionVisitor<'a, '_, '_> {
                         );
                     }
                 }
-                PathSource::Expr(_) | PathSource::TupleStruct(_) | PathSource::Pat => {
+                PathSource::Expr(_) | PathSource::TupleStruct(..) | PathSource::Pat => {
                     let span = match &source {
                         PathSource::Expr(Some(Expr {
                             span, kind: ExprKind::Call(_, _), ..
                         }))
-                        | PathSource::TupleStruct(span) => {
+                        | PathSource::TupleStruct(span, _) => {
                             // We want the main underline to cover the suggested code as well for
                             // cleaner output.
                             err.set_span(*span);
@@ -639,7 +639,7 @@ impl<'a> LateResolutionVisitor<'a, '_, '_> {
                         err.span_label(span, &format!("`{}` defined here", path_str));
                     }
                     let (tail, descr, applicability) = match source {
-                        PathSource::Pat | PathSource::TupleStruct(_) => {
+                        PathSource::Pat | PathSource::TupleStruct(..) => {
                             ("", "pattern", Applicability::MachineApplicable)
                         }
                         _ => (": val", "literal", Applicability::HasPlaceholders),
@@ -704,7 +704,7 @@ impl<'a> LateResolutionVisitor<'a, '_, '_> {
             }
             (
                 Res::Def(DefKind::Enum, def_id),
-                PathSource::TupleStruct(_) | PathSource::Expr(..),
+                PathSource::TupleStruct(..) | PathSource::Expr(..),
             ) => {
                 if self
                     .diagnostic_metadata
@@ -744,15 +744,50 @@ impl<'a> LateResolutionVisitor<'a, '_, '_> {
                 }
             }
             (Res::Def(DefKind::Struct, def_id), _) if ns == ValueNS => {
-                if let Some((ctor_def, ctor_vis)) = self.r.struct_constructors.get(&def_id).cloned()
+                if let Some((ctor_def, ctor_vis, fields)) =
+                    self.r.struct_constructors.get(&def_id).cloned()
                 {
                     let accessible_ctor =
                         self.r.is_accessible_from(ctor_vis, self.parent_scope.module);
                     if is_expected(ctor_def) && !accessible_ctor {
-                        err.span_label(
-                            span,
-                            "constructor is not visible here due to private fields".to_string(),
-                        );
+                        let mut better_diag = false;
+                        if let PathSource::TupleStruct(_, pattern_spans) = source {
+                            if pattern_spans.len() > 0 && fields.len() == pattern_spans.len() {
+                                let non_visible_spans: Vec<Span> = fields
+                                    .iter()
+                                    .zip(pattern_spans.iter())
+                                    .filter_map(|(vis, span)| {
+                                        match self
+                                            .r
+                                            .is_accessible_from(*vis, self.parent_scope.module)
+                                        {
+                                            true => None,
+                                            false => Some(*span),
+                                        }
+                                    })
+                                    .collect();
+                                // Extra check to be sure
+                                if non_visible_spans.len() > 0 {
+                                    let mut m: rustc_span::MultiSpan =
+                                        non_visible_spans.clone().into();
+                                    non_visible_spans.into_iter().for_each(|s| {
+                                        m.push_span_label(s, "private field".to_string())
+                                    });
+                                    err.span_note(
+                                        m,
+                                        "constructor is not visible here due to private fields",
+                                    );
+                                    better_diag = true;
+                                }
+                            }
+                        }
+
+                        if !better_diag {
+                            err.span_label(
+                                span,
+                                "constructor is not visible here due to private fields".to_string(),
+                            );
+                        }
                     }
                 } else {
                     bad_struct_syntax_suggestion(def_id);

compiler/rustc_resolve/src/lib.rs

@@ -999,7 +999,8 @@ pub struct Resolver<'a> {
 
     /// Table for mapping struct IDs into struct constructor IDs,
     /// it's not used during normal resolution, only for better error reporting.
-    struct_constructors: DefIdMap<(Res, ty::Visibility)>,
+    /// Also includes of list of each fields visibility
+    struct_constructors: DefIdMap<(Res, ty::Visibility, Vec<ty::Visibility>)>,
 
     /// Features enabled for this crate.
     active_features: FxHashSet<Symbol>,
@@ -1036,6 +1037,7 @@ pub struct ResolverArenas<'a> {
     name_resolutions: TypedArena<RefCell<NameResolution<'a>>>,
     macro_rules_bindings: TypedArena<MacroRulesBinding<'a>>,
     ast_paths: TypedArena<ast::Path>,
+    pattern_spans: TypedArena<Span>,
 }
 
 impl<'a> ResolverArenas<'a> {
@@ -1067,6 +1069,9 @@ impl<'a> ResolverArenas<'a> {
     fn alloc_ast_paths(&'a self, paths: &[ast::Path]) -> &'a [ast::Path] {
         self.ast_paths.alloc_from_iter(paths.iter().cloned())
     }
+    fn alloc_pattern_spans(&'a self, spans: impl Iterator<Item = Span>) -> &'a [Span] {
+        self.pattern_spans.alloc_from_iter(spans)
+    }
 }
 
 impl<'a> AsMut<Resolver<'a>> for Resolver<'a> {

src/test/ui/issues/auxiliary/issue-75907.rs

@@ -0,0 +1,5 @@
+pub struct Bar(pub u8, u8, u8);
+
+pub fn make_bar() -> Bar {
+    Bar(1, 12, 10)
+}

src/test/ui/issues/issue-75907.rs

@@ -0,0 +1,18 @@
+// Test for for diagnostic improvement issue #75907
+
+mod foo {
+    pub(crate) struct Foo(u8);
+    pub(crate) struct Bar(pub u8, u8, Foo);
+
+    pub(crate) fn make_bar() -> Bar {
+        Bar(1, 12, Foo(10))
+    }
+}
+
+use foo::{make_bar, Bar, Foo};
+
+fn main() {
+    let Bar(x, y, Foo(z)) = make_bar();
+    //~^ ERROR expected tuple struct
+    //~| ERROR expected tuple struct
+}

src/test/ui/issues/issue-75907.stderr

@@ -0,0 +1,29 @@
+error[E0532]: expected tuple struct or tuple variant, found struct `Bar`
+  --> $DIR/issue-75907.rs:15:9
+   |
+LL |     let Bar(x, y, Foo(z)) = make_bar();
+   |         ^^^
+   |
+note: constructor is not visible here due to private fields
+  --> $DIR/issue-75907.rs:15:16
+   |
+LL |     let Bar(x, y, Foo(z)) = make_bar();
+   |                ^  ^^^^^^ private field
+   |                |
+   |                private field
+
+error[E0532]: expected tuple struct or tuple variant, found struct `Foo`
+  --> $DIR/issue-75907.rs:15:19
+   |
+LL |     let Bar(x, y, Foo(z)) = make_bar();
+   |                   ^^^
+   |
+note: constructor is not visible here due to private fields
+  --> $DIR/issue-75907.rs:15:23
+   |
+LL |     let Bar(x, y, Foo(z)) = make_bar();
+   |                       ^ private field
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0532`.

src/test/ui/issues/issue-75907_b.rs

@@ -0,0 +1,11 @@
+// Test for for diagnostic improvement issue #75907, extern crate
+// aux-build:issue-75907.rs
+
+extern crate issue_75907 as a;
+
+use a::{make_bar, Bar};
+
+fn main() {
+    let Bar(x, y, z) = make_bar();
+    //~^ ERROR expected tuple struct
+}

src/test/ui/issues/issue-75907_b.stderr

@@ -0,0 +1,9 @@
+error[E0532]: expected tuple struct or tuple variant, found struct `Bar`
+  --> $DIR/issue-75907_b.rs:9:9
+   |
+LL |     let Bar(x, y, z) = make_bar();
+   |         ^^^ constructor is not visible here due to private fields
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0532`.