Skip to content

Commit

Permalink
Rollup merge of #116429 - fmease:clean-up-struct-field-suggs, r=compi…
Browse files Browse the repository at this point in the history 
…ler-errors

Diagnostics: Be more careful when suggesting struct fields

Consolidate the various places which filter out struct fields that shouldn't be suggested into a single function.

Previously, each of those code paths had slightly different and incomplete metrics for no good reason. Now, there's only a single 'complete' metric (namely `is_field_suggestable`) which also filters out hygienic fields that come from different syntax contexts.

Fixes #116334.
  • Loading branch information
@workingjubilee
workingjubilee authored Oct 5, 2023
2 parents cfce3a9 + 867cc41 commit a9a389c
Show file tree
Hide file tree
Showing 10 changed files with 226 additions and 145 deletions.
100 changes: 24 additions & 76 deletions compiler/rustc_hir_typeck/src/expr.rs
View file
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
Original file line number Diff line number Diff line change
Expand Up @@ -1685,4 +1685,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)
// 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
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
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

0 comments on commit a9a389c

Please sign in to comment.