Skip to content

Detect struct construction with private field in field with default #135846

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Detect struct construction with private field in field with default #135846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,7 @@ provide! { tcx, def_id, other, cdata,

crate_extern_paths => { cdata.source().paths().cloned().collect() }
expn_that_defined => { cdata.get_expn_that_defined(def_id.index, tcx.sess) }
default_field => { cdata.get_default_field(def_id.index) }
is_doc_hidden => { cdata.get_attr_flags(def_id.index).contains(AttrFlags::IS_DOC_HIDDEN) }
doc_link_resolutions => { tcx.arena.alloc(cdata.get_doc_link_resolutions(def_id.index)) }
doc_link_traits_in_scope => {
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_middle/src/query/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1719,6 +1719,13 @@ rustc_queries! {
feedable
}

/// Returns whether the impl or associated function has the `default` keyword.
query default_field(def_id: DefId) -> Option<DefId> {
desc { |tcx| "looking up the `const` corresponding to the default for `{}`", tcx.def_path_str(def_id) }
separate_provide_extern
feedable
Copy link
Member

Choose a reason for hiding this comment

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

Why is this feedable? Also, why does this query exist at all, do we not already have a way of determining if a field has a default? I would have thought we need some way of doing that in order for type checking/mir building/codegen/that whole set of things to know whether Struct { .. } is valid.

}

query check_well_formed(key: LocalDefId) -> Result<(), ErrorGuaranteed> {
desc { |tcx| "checking that `{}` is well-formed", tcx.def_path_str(key) }
return_result_from_ensure_ok
Expand Down
14 changes: 9 additions & 5 deletions compiler/rustc_resolve/src/build_reduced_graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,14 +396,18 @@ impl<'a, 'ra, 'tcx> BuildReducedGraphVisitor<'a, 'ra, 'tcx> {
// The fields are not expanded yet.
return;
}
let fields = fields
let field_name = |i, field: &ast::FieldDef| {
field.ident.unwrap_or_else(|| Ident::from_str_and_span(&format!("{i}"), field.span))
};
let field_names: Vec<_> =
fields.iter().enumerate().map(|(i, field)| field_name(i, field)).collect();
let defaults = fields
.iter()
.enumerate()
.map(|(i, field)| {
field.ident.unwrap_or_else(|| Ident::from_str_and_span(&format!("{i}"), field.span))
})
.filter_map(|(i, field)| field.default.as_ref().map(|_| field_name(i, field).name))
.collect();
self.r.field_names.insert(def_id, fields);
self.r.field_names.insert(def_id, field_names);
self.r.field_defaults.insert(def_id, defaults);
}

fn insert_field_visibilities_local(&mut self, def_id: DefId, fields: &[ast::FieldDef]) {
Expand Down
60 changes: 58 additions & 2 deletions compiler/rustc_resolve/src/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1750,8 +1750,15 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
}

fn report_privacy_error(&mut self, privacy_error: &PrivacyError<'ra>) {
let PrivacyError { ident, binding, outermost_res, parent_scope, single_nested, dedup_span } =
*privacy_error;
let PrivacyError {
ident,
binding,
outermost_res,
parent_scope,
single_nested,
dedup_span,
ref source,
} = *privacy_error;

let res = binding.res();
let ctor_fields_span = self.ctor_fields_span(binding);
Expand All @@ -1767,6 +1774,55 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
let mut err =
self.dcx().create_err(errors::IsPrivate { span: ident.span, ident_descr, ident });

if let Some(expr) = source
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this whole bit of logic out to a separate function? this function's already massive without another pile of logic for another special case in our diagnostics. At the very least there should be a comment saying what case we're handling here so its obvious you can skip over it if you don't care about this.

&& let ast::ExprKind::Struct(struct_expr) = &expr.kind
&& let Some(Res::Def(_, def_id)) = self.partial_res_map
[&struct_expr.path.segments.iter().last().unwrap().id]
.full_res()
Comment on lines +1779 to +1781
Copy link
Member

Choose a reason for hiding this comment

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

We don't have to handle type-relative paths because they're forbidden in adt exprs. Except I guess they're allowed under feature(more_qualfiied_paths). Pls add a comment about the interactions with that feature here.

&& let Some(default_fields) = self.field_defaults(def_id)
&& !struct_expr.fields.is_empty()
{
let last_span = struct_expr.fields.iter().last().unwrap().span;
let mut iter = struct_expr.fields.iter().peekable();
let mut prev: Option<Span> = None;
while let Some(field) = iter.next() {
if field.expr.span.overlaps(ident.span) {
err.span_label(field.ident.span, "while setting this field");
if default_fields.contains(&field.ident.name) {
let sugg = if last_span == field.span {
vec![(field.span, "..".to_string())]
} else {
vec![
(
// Account for trailing commas and ensure we remove them.
match (prev, iter.peek()) {
(_, Some(next)) => field.span.with_hi(next.span.lo()),
(Some(prev), _) => field.span.with_lo(prev.hi()),
(None, None) => field.span,
},
String::new(),
),
(last_span.shrink_to_hi(), ", ..".to_string()),
]
};
err.multipart_suggestion_verbose(
format!(
"the type `{ident}` of field `{}` is private, but you can \
construct the default value defined for it in `{}` using `..` in \
the struct initializer expression",
field.ident,
self.tcx.item_name(def_id),
),
sugg,
Applicability::MachineApplicable,
);
break;
}
}
prev = Some(field.span);
}
}

let mut not_publicly_reexported = false;
if let Some((this_res, outer_ident)) = outermost_res {
let import_suggestions = self.lookup_import_candidates(
Expand Down
19 changes: 18 additions & 1 deletion compiler/rustc_resolve/src/ident.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,6 +893,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
binding,
dedup_span: path_span,
outermost_res: None,
source: None,
parent_scope: *parent_scope,
single_nested: path_span != root_span,
});
Expand Down Expand Up @@ -1402,7 +1403,16 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
parent_scope: &ParentScope<'ra>,
ignore_import: Option<Import<'ra>>,
) -> PathResult<'ra> {
self.resolve_path_with_ribs(path, opt_ns, parent_scope, None, None, None, ignore_import)
self.resolve_path_with_ribs(
path,
opt_ns,
parent_scope,
None,
None,
None,
None,
ignore_import,
)
}

#[instrument(level = "debug", skip(self))]
Expand All @@ -1419,6 +1429,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
path,
opt_ns,
parent_scope,
None,
finalize,
None,
ignore_binding,
Expand All @@ -1431,6 +1442,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
path: &[Segment],
opt_ns: Option<Namespace>, // `None` indicates a module path in import
parent_scope: &ParentScope<'ra>,
source: Option<PathSource<'_>>,
finalize: Option<Finalize>,
ribs: Option<&PerNS<Vec<Rib<'ra>>>>,
ignore_binding: Option<NameBinding<'ra>>,
Expand Down Expand Up @@ -1605,6 +1617,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
// the user it is not accessible.
for error in &mut self.privacy_errors[privacy_errors_len..] {
error.outermost_res = Some((res, ident));
error.source = match source {
Some(PathSource::Struct(Some(expr)))
| Some(PathSource::Expr(Some(expr))) => Some(expr.clone()),
_ => None,
};
}

let maybe_assoc = opt_ns != Some(MacroNS) && PathSource::Type.is_expected(res);
Expand Down
31 changes: 18 additions & 13 deletions compiler/rustc_resolve/src/late.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ pub(crate) enum PathSource<'a> {
// Paths in path patterns `Path`.
Pat,
// Paths in struct expressions and patterns `Path { .. }`.
Struct,
Struct(Option<&'a Expr>),
// Paths in tuple struct patterns `Path(..)`.
TupleStruct(Span, &'a [Span]),
// `m::A::B` in `<T as m::A>::B::C`.
Expand All @@ -419,7 +419,7 @@ pub(crate) enum PathSource<'a> {
impl<'a> PathSource<'a> {
fn namespace(self) -> Namespace {
match self {
PathSource::Type | PathSource::Trait(_) | PathSource::Struct => TypeNS,
PathSource::Type | PathSource::Trait(_) | PathSource::Struct(_) => TypeNS,
PathSource::Expr(..)
| PathSource::Pat
| PathSource::TupleStruct(..)
Expand All @@ -435,7 +435,7 @@ impl<'a> PathSource<'a> {
PathSource::Type
| PathSource::Expr(..)
| PathSource::Pat
| PathSource::Struct
| PathSource::Struct(_)
| PathSource::TupleStruct(..)
| PathSource::ReturnTypeNotation => true,
PathSource::Trait(_)
Expand All @@ -450,7 +450,7 @@ impl<'a> PathSource<'a> {
PathSource::Type => "type",
PathSource::Trait(_) => "trait",
PathSource::Pat => "unit struct, unit variant or constant",
PathSource::Struct => "struct, variant or union type",
PathSource::Struct(_) => "struct, variant or union type",
PathSource::TupleStruct(..) => "tuple struct or tuple variant",
PathSource::TraitItem(ns) => match ns {
TypeNS => "associated type",
Expand Down Expand Up @@ -531,7 +531,7 @@ impl<'a> PathSource<'a> {
|| matches!(res, Res::Def(DefKind::Const | DefKind::AssocConst, _))
}
PathSource::TupleStruct(..) => res.expected_in_tuple_struct_pat(),
PathSource::Struct => matches!(
PathSource::Struct(_) => matches!(
res,
Res::Def(
DefKind::Struct
Expand Down Expand Up @@ -571,8 +571,8 @@ impl<'a> PathSource<'a> {
(PathSource::Trait(_), false) => E0405,
(PathSource::Type, true) => E0573,
(PathSource::Type, false) => E0412,
(PathSource::Struct, true) => E0574,
(PathSource::Struct, false) => E0422,
(PathSource::Struct(_), true) => E0574,
(PathSource::Struct(_), false) => E0422,
(PathSource::Expr(..), true) | (PathSource::Delegation, true) => E0423,
(PathSource::Expr(..), false) | (PathSource::Delegation, false) => E0425,
(PathSource::Pat | PathSource::TupleStruct(..), true) => E0532,
Expand Down Expand Up @@ -1458,11 +1458,13 @@ impl<'a, 'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {
path: &[Segment],
opt_ns: Option<Namespace>, // `None` indicates a module path in import
finalize: Option<Finalize>,
source: PathSource<'ast>,
) -> PathResult<'ra> {
self.r.resolve_path_with_ribs(
path,
opt_ns,
&self.parent_scope,
Some(source),
finalize,
Some(&self.ribs),
None,
Expand Down Expand Up @@ -1967,7 +1969,7 @@ impl<'a, 'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {
| PathSource::ReturnTypeNotation => false,
PathSource::Expr(..)
| PathSource::Pat
| PathSource::Struct
| PathSource::Struct(_)
| PathSource::TupleStruct(..)
| PathSource::Delegation => true,
};
Expand Down Expand Up @@ -3805,7 +3807,7 @@ impl<'a, 'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {
self.smart_resolve_path(pat.id, qself, path, PathSource::Pat);
}
PatKind::Struct(ref qself, ref path, ref _fields, ref rest) => {
self.smart_resolve_path(pat.id, qself, path, PathSource::Struct);
self.smart_resolve_path(pat.id, qself, path, PathSource::Struct(None));
self.record_patterns_with_skipped_bindings(pat, rest);
}
PatKind::Or(ref ps) => {
Expand Down Expand Up @@ -4214,6 +4216,7 @@ impl<'a, 'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {
qself,
path,
ns,
source,
path_span,
source.defer_to_typeck(),
finalize,
Expand Down Expand Up @@ -4259,7 +4262,7 @@ impl<'a, 'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {
std_path.push(Segment::from_ident(Ident::with_dummy_span(sym::std)));
std_path.extend(path);
if let PathResult::Module(_) | PathResult::NonModule(_) =
self.resolve_path(&std_path, Some(ns), None)
self.resolve_path(&std_path, Some(ns), None, source)
{
// Check if we wrote `str::from_utf8` instead of `std::str::from_utf8`
let item_span =
Expand Down Expand Up @@ -4330,6 +4333,7 @@ impl<'a, 'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {
qself: &Option<P<QSelf>>,
path: &[Segment],
primary_ns: Namespace,
source: PathSource<'ast>,
span: Span,
defer_to_typeck: bool,
finalize: Finalize,
Expand All @@ -4338,7 +4342,7 @@ impl<'a, 'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {

for (i, &ns) in [primary_ns, TypeNS, ValueNS].iter().enumerate() {
if i == 0 || ns != primary_ns {
match self.resolve_qpath(qself, path, ns, finalize)? {
match self.resolve_qpath(qself, path, ns, source, finalize)? {
Some(partial_res)
if partial_res.unresolved_segments() == 0 || defer_to_typeck =>
{
Expand Down Expand Up @@ -4374,6 +4378,7 @@ impl<'a, 'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {
qself: &Option<P<QSelf>>,
path: &[Segment],
ns: Namespace,
source: PathSource<'ast>,
finalize: Finalize,
) -> Result<Option<PartialRes>, Spanned<ResolutionError<'ra>>> {
debug!(
Expand Down Expand Up @@ -4435,7 +4440,7 @@ impl<'a, 'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {
)));
}

let result = match self.resolve_path(path, Some(ns), Some(finalize)) {
let result = match self.resolve_path(path, Some(ns), Some(finalize), source) {
PathResult::NonModule(path_res) => path_res,
PathResult::Module(ModuleOrUniformRoot::Module(module)) if !module.is_normal() => {
PartialRes::new(module.res().unwrap())
Expand Down Expand Up @@ -4659,7 +4664,7 @@ impl<'a, 'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {
}

ExprKind::Struct(ref se) => {
self.smart_resolve_path(expr.id, &se.qself, &se.path, PathSource::Struct);
self.smart_resolve_path(expr.id, &se.qself, &se.path, PathSource::Struct(parent));
// This is the same as `visit::walk_expr(self, expr);`, but we want to pass the
// parent in for accurate suggestions when encountering `Foo { bar }` that should
// have been `Foo { bar: self.bar }`.
Expand Down
Loading
Loading