Skip to content

Suggest making private tuple struct field public #106579

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions compiler/rustc_resolve/src/build_reduced_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,15 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
self.r.field_names.insert(def_id, field_names);
}

fn insert_field_visibilities_local(&mut self, def_id: DefId, vdata: &ast::VariantData) {
let field_vis = vdata
.fields()
.iter()
.map(|field| field.vis.span.until(field.ident.map_or(field.ty.span, |i| i.span)))
.collect();
self.r.field_visibility_spans.insert(def_id, field_vis);
}

fn insert_field_names_extern(&mut self, def_id: DefId) {
let field_names =
self.r.cstore().struct_field_names_untracked(def_id, self.r.session).collect();
Expand Down Expand Up @@ -737,6 +746,7 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {

// Record field names for error reporting.
self.insert_field_names_local(def_id, vdata);
self.insert_field_visibilities_local(def_id, vdata);

// If this is a tuple or unit struct, define a name
// in the value namespace as well.
Expand Down Expand Up @@ -770,6 +780,8 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
Res::Def(DefKind::Ctor(CtorOf::Struct, ctor_kind), ctor_def_id.to_def_id());
self.r.define(parent, ident, ValueNS, (ctor_res, ctor_vis, sp, expansion));
self.r.visibilities.insert(ctor_def_id, ctor_vis);
// We need the field visibility spans also for the constructor for E0603.
self.insert_field_visibilities_local(ctor_def_id.to_def_id(), vdata);

self.r
.struct_constructors
Expand All @@ -783,6 +795,7 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {

// Record field names for error reporting.
self.insert_field_names_local(def_id, vdata);
self.insert_field_visibilities_local(def_id, vdata);
}

ItemKind::Trait(..) => {
Expand Down Expand Up @@ -1510,6 +1523,7 @@ impl<'a, 'b> Visitor<'b> for BuildReducedGraphVisitor<'a, 'b> {

// Record field names for error reporting.
self.insert_field_names_local(def_id.to_def_id(), &variant.data);
self.insert_field_visibilities_local(def_id.to_def_id(), &variant.data);

visit::walk_variant(self, variant);
}
Expand Down
14 changes: 13 additions & 1 deletion compiler/rustc_resolve/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ use rustc_ast::{self as ast, Crate, ItemKind, ModKind, NodeId, Path, CRATE_NODE_
use rustc_ast_pretty::pprust;
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::struct_span_err;
use rustc_errors::{Applicability, Diagnostic, DiagnosticBuilder, ErrorGuaranteed, MultiSpan};
use rustc_errors::{
pluralize, Applicability, Diagnostic, DiagnosticBuilder, ErrorGuaranteed, MultiSpan,
};
use rustc_feature::BUILTIN_ATTRIBUTES;
use rustc_hir::def::Namespace::{self, *};
use rustc_hir::def::{self, CtorKind, CtorOf, DefKind, NonMacroAttrKind, PerNS};
Expand Down Expand Up @@ -1604,6 +1606,16 @@ impl<'a> Resolver<'a> {
err.span_label(ident.span, &format!("private {}", descr));
if let Some(span) = ctor_fields_span {
err.span_label(span, "a constructor is private if any of the fields is private");
if let Res::Def(_, d) = res && let Some(fields) = self.field_visibility_spans.get(&d) {
err.multipart_suggestion_verbose(
&format!(
"consider making the field{} publicly accessible",
pluralize!(fields.len())
),
fields.iter().map(|span| (*span, "pub ".to_string())).collect(),
Applicability::MaybeIncorrect,
);
}
}

// Print the whole import chain to make it easier to see what happens.
Expand Down
11 changes: 11 additions & 0 deletions compiler/rustc_resolve/src/late/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1451,6 +1451,17 @@ impl<'a: 'ast, 'ast> LateResolutionVisitor<'a, '_, 'ast> {
.collect();

if non_visible_spans.len() > 0 {
if let Some(fields) = self.r.field_visibility_spans.get(&def_id) {
err.multipart_suggestion_verbose(
&format!(
"consider making the field{} publicly accessible",
pluralize!(fields.len())
),
fields.iter().map(|span| (*span, "pub ".to_string())).collect(),
Applicability::MaybeIncorrect,
);
}

let mut m: MultiSpan = non_visible_spans.clone().into();
non_visible_spans
.into_iter()
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_resolve/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -881,6 +881,10 @@ pub struct Resolver<'a> {
/// Used for hints during error reporting.
field_names: FxHashMap<DefId, Vec<Spanned<Symbol>>>,

/// Span of the privacy modifier in fields of an item `DefId` accessible with dot syntax.
/// Used for hints during error reporting.
field_visibility_spans: FxHashMap<DefId, Vec<Span>>,

Comment on lines +884 to +887
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to extend and reuse the field_names field for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

field_names only had the Symbol and I wasn't sure if there was any place that relied on it being always a Vec, causing perf impact if needing to map/collect it every time. But yes, we could do that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only difference I see is that this approach will only store the privacy span vec for tuple structs and nothing else.

/// All imports known to succeed or fail.
determined_imports: Vec<&'a Import<'a>>,

Expand Down Expand Up @@ -1268,6 +1272,7 @@ impl<'a> Resolver<'a> {

has_self: FxHashSet::default(),
field_names: FxHashMap::default(),
field_visibility_spans: FxHashMap::default(),

determined_imports: Vec::new(),
indeterminate_imports: Vec::new(),
Expand Down
4 changes: 4 additions & 0 deletions tests/ui/privacy/issue-75906.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ note: constructor is not visible here due to private fields
|
LL | pub struct Bar(u8);
| ^^ private field
help: consider making the field publicly accessible
|
LL | pub struct Bar(pub u8);
| +++

error: aborting due to previous error

Expand Down
2 changes: 1 addition & 1 deletion tests/ui/privacy/issue-75907.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

mod foo {
pub(crate) struct Foo(u8);
pub(crate) struct Bar(pub u8, u8, Foo);
pub(crate) struct Bar(pub u8, pub(in crate::foo) u8, Foo);

pub(crate) fn make_bar() -> Bar {
Bar(1, 12, Foo(10))
Expand Down
8 changes: 8 additions & 0 deletions tests/ui/privacy/issue-75907.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ LL | let Bar(x, y, Foo(z)) = make_bar();
| ^ ^^^^^^ private field
| |
| private field
help: consider making the fields publicly accessible
|
LL | pub(crate) struct Bar(pub u8, pub u8, pub Foo);
| ~~~ ~~~ +++

error[E0532]: cannot match against a tuple struct which contains private fields
--> $DIR/issue-75907.rs:15:19
Expand All @@ -23,6 +27,10 @@ note: constructor is not visible here due to private fields
|
LL | let Bar(x, y, Foo(z)) = make_bar();
| ^ private field
help: consider making the field publicly accessible
|
LL | pub(crate) struct Foo(pub u8);
| +++

error: aborting due to 2 previous errors

Expand Down
Loading