Auto merge of #76499 - guswynn:priv_des, r=petrochenkov

Give better diagnostic when using a private tuple struct constructor

Fixes #75907

Some notes:
1. This required some deep changes, including removing a Copy impl for PatKind. If some tests fail, I would still appreciate review on the overall approach
2. this only works with basic patterns (no wildcards for example), and fails if there is any problems getting the visibility of the fields (i am not sure what the failure that can happen in resolve_visibility_speculative, but we check the length of our fields in both cases against each other, so if anything goes wrong, we fall back to the worse error. This could be extended to more patterns
3. this does not yet deal with #75906, but I believe it will be similar
4. let me know if you want more tests
5. doesn't yet at the suggestion that `@yoshuawuyts` suggested at the end of their issue, but that could be added relatively easily (i believe)

Author: bors

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

@@ -90,7 +90,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),
@@ -623,12 +623,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);
@@ -640,7 +640,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),
@@ -705,7 +705,7 @@ impl<'a> LateResolutionVisitor<'a, '_, '_> {
             }
             (
                 Res::Def(DefKind::Enum, def_id),
-                PathSource::TupleStruct(_) | PathSource::Expr(..),
+                PathSource::TupleStruct(..) | PathSource::Expr(..),
             ) => {
                 if self
                     .diagnostic_metadata
@@ -745,15 +745,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

@@ -1005,7 +1005,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>,
@@ -1042,6 +1043,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> {
@@ -1073,6 +1075,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`.