Skip to content
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

Jump to bottom

diagnostics: be more careful when suggesting struct fields #116429

Merged
merged 1 commit into from
Oct 5, 2023
Merged

diagnostics: be more careful when suggesting struct fields #116429

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
100 changes: 24 additions & 76 deletions compiler/rustc_hir_typeck/src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ use rustc_infer::infer::DefineOpaqueTypes;
use rustc_infer::infer::InferOk;
use rustc_infer::traits::query::NoSolution;
use rustc_infer::traits::ObligationCause;
use rustc_middle::middle::stability;
use rustc_middle::ty::adjustment::{Adjust, Adjustment, AllowTwoPhase};
use rustc_middle::ty::error::{
ExpectedFound,
Expand Down Expand Up @@ -1585,12 +1584,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
self.check_expr_struct_fields(
adt_ty,
expected,
expr.hir_id,
expr,
qpath.span(),
variant,
fields,
base_expr,
expr.span,
);

self.require_type_is_sized(adt_ty, expr.span, traits::StructInitializerSized);
Expand All @@ -1601,12 +1599,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
&self,
adt_ty: Ty<'tcx>,
expected: Expectation<'tcx>,
expr_id: hir::HirId,
expr: &hir::Expr<'_>,
span: Span,
variant: &'tcx ty::VariantDef,
ast_fields: &'tcx [hir::ExprField<'tcx>],
base_expr: &'tcx Option<&'tcx hir::Expr<'tcx>>,
expr_span: Span,
) {
let tcx = self.tcx;

Expand Down Expand Up @@ -1646,7 +1643,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// struct-like enums (yet...), but it's definitely not
// a bug to have constructed one.
if adt_kind != AdtKind::Enum {
tcx.check_stability(v_field.did, Some(expr_id), field.span, None);
tcx.check_stability(v_field.did, Some(expr.hir_id), field.span, None);
}

self.field_ty(field.span, v_field, args)
Expand All @@ -1662,10 +1659,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
self.report_unknown_field(
adt_ty,
variant,
expr,
field,
ast_fields,
adt.variant_descr(),
expr_span,
)
};

Expand Down Expand Up @@ -1731,7 +1728,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
.iter()
.map(|f| {
let fru_ty = self
.normalize(expr_span, self.field_ty(base_expr.span, f, fresh_args));
.normalize(expr.span, self.field_ty(base_expr.span, f, fresh_args));
let ident = self.tcx.adjust_ident(f.ident(self.tcx), variant.def_id);
if let Some(_) = remaining_fields.remove(&ident) {
let target_ty = self.field_ty(base_expr.span, f, args);
Expand Down Expand Up @@ -1814,7 +1811,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
ty::Adt(adt, args) if adt.is_struct() => variant
.fields
.iter()
.map(|f| self.normalize(expr_span, f.ty(self.tcx, args)))
.map(|f| self.normalize(expr.span, f.ty(self.tcx, args)))
.collect(),
_ => {
self.tcx
Expand All @@ -1824,13 +1821,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
}
};
self.typeck_results.borrow_mut().fru_field_types_mut().insert(expr_id, fru_tys);
self.typeck_results.borrow_mut().fru_field_types_mut().insert(expr.hir_id, fru_tys);
} else if adt_kind != AdtKind::Union && !remaining_fields.is_empty() {
debug!(?remaining_fields);
let private_fields: Vec<&ty::FieldDef> = variant
.fields
.iter()
.filter(|field| !field.vis.is_accessible_from(tcx.parent_module(expr_id), tcx))
.filter(|field| !field.vis.is_accessible_from(tcx.parent_module(expr.hir_id), tcx))
.collect();

if !private_fields.is_empty() {
Expand Down Expand Up @@ -2049,16 +2046,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
&self,
ty: Ty<'tcx>,
variant: &'tcx ty::VariantDef,
expr: &hir::Expr<'_>,
field: &hir::ExprField<'_>,
skip_fields: &[hir::ExprField<'_>],
kind_name: &str,
expr_span: Span,
) -> ErrorGuaranteed {
if variant.is_recovered() {
let guar = self
.tcx
.sess
.delay_span_bug(expr_span, "parser recovered but no error was emitted");
.delay_span_bug(expr.span, "parser recovered but no error was emitted");
self.set_tainted_by_errors(guar);
return guar;
}
Expand Down Expand Up @@ -2102,7 +2099,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
);
err.span_label(field.ident.span, "field does not exist");
err.span_suggestion_verbose(
expr_span,
expr.span,
format!(
"`{adt}::{variant}` is a tuple {kind_name}, use the appropriate syntax",
adt = ty,
Expand All @@ -2120,7 +2117,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
err.span_label(variant_ident_span, format!("`{ty}` defined here"));
err.span_label(field.ident.span, "field does not exist");
err.span_suggestion_verbose(
expr_span,
expr.span,
format!("`{ty}` is a tuple {kind_name}, use the appropriate syntax",),
format!("{ty}(/* fields */)"),
Applicability::HasPlaceholders,
Expand All @@ -2129,9 +2126,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
},
_ => {
// prevent all specified fields from being suggested
let skip_fields: Vec<_> = skip_fields.iter().map(|x| x.ident.name).collect();
let available_field_names = self.available_field_names(variant, expr, skip_fields);
if let Some(field_name) =
self.suggest_field_name(variant, field.ident.name, &skip_fields, expr_span)
find_best_match_for_name(&available_field_names, field.ident.name, None)
{
err.span_suggestion(
field.ident.span,
Expand All @@ -2153,10 +2150,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
format!("`{ty}` does not have this field"),
);
}
let mut available_field_names =
self.available_field_names(variant, expr_span);
available_field_names
.retain(|name| skip_fields.iter().all(|skip| name != skip));
if available_field_names.is_empty() {
err.note("all struct fields are already assigned");
} else {
Expand All @@ -2174,63 +2167,19 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
err.emit()
}

// Return a hint about the closest match in field names
fn suggest_field_name(
&self,
variant: &'tcx ty::VariantDef,
field: Symbol,
skip: &[Symbol],
// The span where stability will be checked
span: Span,
) -> Option<Symbol> {
let names = variant
.fields
.iter()
.filter_map(|field| {
// ignore already set fields and private fields from non-local crates
// and unstable fields.
if skip.iter().any(|&x| x == field.name)
|| (!variant.def_id.is_local() && !field.vis.is_public())
|| matches!(
self.tcx.eval_stability(field.did, None, span, None),
stability::EvalResult::Deny { .. }
)
{
None
} else {
Some(field.name)
}
})
.collect::<Vec<Symbol>>();

find_best_match_for_name(&names, field, None)
}

fn available_field_names(
&self,
variant: &'tcx ty::VariantDef,
access_span: Span,
expr: &hir::Expr<'_>,
skip_fields: &[hir::ExprField<'_>],
) -> Vec<Symbol> {
let body_owner_hir_id = self.tcx.hir().local_def_id_to_hir_id(self.body_id);
variant
.fields
.iter()
.filter(|field| {
let def_scope = self
.tcx
.adjust_ident_and_get_scope(
field.ident(self.tcx),
variant.def_id,
body_owner_hir_id,
)
.1;
field.vis.is_accessible_from(def_scope, self.tcx)
&& !matches!(
self.tcx.eval_stability(field.did, None, access_span, None),
stability::EvalResult::Deny { .. }
)
skip_fields.iter().all(|&skip| skip.ident.name != field.name)
&& self.is_field_suggestable(field, expr.hir_id, expr.span)
})
.filter(|field| !self.tcx.is_doc_hidden(field.did))
.map(|field| field.name)
.collect()
}
Expand Down Expand Up @@ -2460,7 +2409,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
self.suggest_first_deref_field(&mut err, expr, base, ident);
}
ty::Adt(def, _) if !def.is_enum() => {
self.suggest_fields_on_recordish(&mut err, def, ident, expr.span);
self.suggest_fields_on_recordish(&mut err, expr, def, ident);
}
ty::Param(param_ty) => {
self.point_at_param_definition(&mut err, param_ty);
Expand Down Expand Up @@ -2622,12 +2571,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
fn suggest_fields_on_recordish(
&self,
err: &mut Diagnostic,
expr: &hir::Expr<'_>,
def: ty::AdtDef<'tcx>,
field: Ident,
access_span: Span,
) {
let available_field_names = self.available_field_names(def.non_enum_variant(), expr, &[]);
if let Some(suggested_field_name) =
self.suggest_field_name(def.non_enum_variant(), field.name, &[], access_span)
find_best_match_for_name(&available_field_names, field.name, None)
{
err.span_suggestion(
field.span,
Expand All @@ -2637,12 +2587,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
);
} else {
err.span_label(field.span, "unknown field");
let struct_variant_def = def.non_enum_variant();
let field_names = self.available_field_names(struct_variant_def, access_span);
if !field_names.is_empty() {
if !available_field_names.is_empty() {
err.note(format!(
"available fields are: {}",
self.name_series_display(field_names),
self.name_series_display(available_field_names),
));
}
}
Expand Down
19 changes: 19 additions & 0 deletions compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1687,4 +1687,23 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
false
}
}

pub(crate) fn is_field_suggestable(
&self,
field: &ty::FieldDef,
hir_id: HirId,
span: Span,
) -> bool {
// The field must be visible in the containing module.
field.vis.is_accessible_from(self.tcx.parent_module(hir_id), self.tcx)
Copy link
Member Author

@fmease fmease Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I've taken is_accessible_from(parent_module()) from one of the places I've removed but another one tried to obtain the DefScope instead of the parent module (via adjust_ident_and_get_scope). Which one is more correct / general?

Copy link
Member

Choose a reason for hiding this comment

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

I think the latter, but I doubt it matters. If something is reachable from an item then it's reachable from its parent module, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if there are some edge cases like structss defined inside of a fn instead of a module. Not sure.

// The field must not be unstable.
&& !matches!(
self.tcx.eval_stability(field.did, None, rustc_span::DUMMY_SP, None),
rustc_middle::middle::stability::EvalResult::Deny { .. }
)
// If the field is from an external crate it must not be `doc(hidden)`.
&& (field.did.is_local() || !self.tcx.is_doc_hidden(field.did))
// If the field is hygienic it must come from the same syntax context.
&& self.tcx.def_ident_span(field.did).unwrap().normalize_to_macros_2_0().eq_ctxt(span)
}
}
41 changes: 15 additions & 26 deletions compiler/rustc_hir_typeck/src/pat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ use rustc_hir::pat_util::EnumerateAndAdjustIterator;
use rustc_hir::{HirId, Pat, PatKind};
use rustc_infer::infer;
use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
use rustc_middle::middle::stability::EvalResult;
use rustc_middle::ty::{self, Adt, BindingMode, Ty, TypeVisitableExt};
use rustc_session::lint::builtin::NON_EXHAUSTIVE_OMITTED_PATTERNS;
use rustc_span::edit_distance::find_best_match_for_name;
Expand Down Expand Up @@ -1408,6 +1407,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
adt.variant_descr(),
&inexistent_fields,
&mut unmentioned_fields,
pat,
variant,
args,
))
Expand All @@ -1434,15 +1434,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let accessible_unmentioned_fields: Vec<_> = unmentioned_fields
.iter()
.copied()
.filter(|(field, _)| {
field.vis.is_accessible_from(tcx.parent_module(pat.hir_id), tcx)
&& !matches!(
tcx.eval_stability(field.did, None, DUMMY_SP, None),
EvalResult::Deny { .. }
)
// We only want to report the error if it is hidden and not local
&& !(tcx.is_doc_hidden(field.did) && !field.did.is_local())
})
.filter(|(field, _)| self.is_field_suggestable(field, pat.hir_id, pat.span))
.collect();

if !has_rest_pat {
Expand Down Expand Up @@ -1578,12 +1570,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
kind_name: &str,
inexistent_fields: &[&hir::PatField<'tcx>],
unmentioned_fields: &mut Vec<(&'tcx ty::FieldDef, Ident)>,
pat: &'tcx Pat<'tcx>,
variant: &ty::VariantDef,
args: &'tcx ty::List<ty::GenericArg<'tcx>>,
) -> DiagnosticBuilder<'tcx, ErrorGuaranteed> {
let tcx = self.tcx;
let (field_names, t, plural) = if inexistent_fields.len() == 1 {
(format!("a field named `{}`", inexistent_fields[0].ident), "this", "")
let (field_names, t, plural) = if let [field] = inexistent_fields {
(format!("a field named `{}`", field.ident), "this", "")
} else {
(
format!(
Expand Down Expand Up @@ -1620,10 +1613,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
),
);

if unmentioned_fields.len() == 1 {
let input =
unmentioned_fields.iter().map(|(_, field)| field.name).collect::<Vec<_>>();
let suggested_name = find_best_match_for_name(&input, pat_field.ident.name, None);
if let [(field_def, field)] = unmentioned_fields.as_slice()
&& self.is_field_suggestable(field_def, pat.hir_id, pat.span)
{
let suggested_name =
find_best_match_for_name(&[field.name], pat_field.ident.name, None);
if let Some(suggested_name) = suggested_name {
err.span_suggestion(
pat_field.ident.span,
Expand All @@ -1646,22 +1640,17 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
PatKind::Lit(expr)
if !self.can_coerce(
self.typeck_results.borrow().expr_ty(expr),
self.field_ty(
unmentioned_fields[0].1.span,
unmentioned_fields[0].0,
args,
),
self.field_ty(field.span, field_def, args),
) => {}
_ => {
let unmentioned_field = unmentioned_fields[0].1.name;
err.span_suggestion_short(
pat_field.ident.span,
format!(
"`{}` has a field named `{}`",
tcx.def_path_str(variant.def_id),
unmentioned_field
field.name,
),
unmentioned_field.to_string(),
field.name,
Applicability::MaybeIncorrect,
);
}
Expand Down Expand Up @@ -1871,8 +1860,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
fields: &'tcx [hir::PatField<'tcx>],
) -> DiagnosticBuilder<'tcx, ErrorGuaranteed> {
let inaccessible = if have_inaccessible_fields { " and inaccessible fields" } else { "" };
let field_names = if unmentioned_fields.len() == 1 {
format!("field `{}`{}", unmentioned_fields[0].1, inaccessible)
let field_names = if let [(_, field)] = unmentioned_fields {
format!("field `{field}`{inaccessible}")
} else {
let fields = unmentioned_fields
.iter()
Expand Down
6 changes: 6 additions & 0 deletions tests/ui/did_you_mean/auxiliary/doc-hidden-fields.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#[derive(Default)]
pub struct B {
#[doc(hidden)]
pub hello: i32,
pub bye: i32,
}
Loading
Loading