Skip to content

Suggest field typo through derefs #117110

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

Merged
merged 6 commits into from
Nov 18, 2023
Merged

Suggest field typo through derefs #117110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dfa7539
Suggest field typo through derefs
estebank Oct 24, 2023
8bd8f3b
Suggest `unwrap()` on field not found for `Result`/`Option`
estebank Oct 24, 2023
4f7dddd
recover primary span label
estebank Nov 9, 2023
6a2d9b4
address review comment
estebank Nov 15, 2023
1ee37bf
let-chain fmt
estebank Nov 16, 2023
289ce57
tweak logic of "unknown field" label
estebank Nov 18, 2023
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
215 changes: 112 additions & 103 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 @@ -2191,7 +2191,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
if let Some(field_name) =
find_best_match_for_name(&available_field_names, field.ident.name, None)
{
err.span_suggestion(
err.span_label(field.ident.span, "unknown field");
err.span_suggestion_verbose(
field.ident.span,
"a field with a similar name exists",
field_name,
Expand Down Expand Up @@ -2420,35 +2421,32 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
ty: Ty<'tcx>,
) {
let Some(output_ty) = self.get_impl_future_output_ty(ty) else {
err.span_label(field_ident.span, "unknown field");
return;
};
let mut add_label = true;
if let ty::Adt(def, _) = output_ty.kind() {
// no field access on enum type
if !def.is_enum() {
if def
.non_enum_variant()
.fields
.iter()
.any(|field| field.ident(self.tcx) == field_ident)
{
add_label = false;
err.span_label(
field_ident.span,
"field not available in `impl Future`, but it is available in its `Output`",
);
err.span_suggestion_verbose(
base.span.shrink_to_hi(),
"consider `await`ing on the `Future` and access the field of its `Output`",
".await",
Applicability::MaybeIncorrect,
);
}
}
let ty::Adt(def, _) = output_ty.kind() else {
err.span_label(field_ident.span, "unknown field");
return;
};
// no field access on enum type
if def.is_enum() {
err.span_label(field_ident.span, "unknown field");
return;
}
if add_label {
err.span_label(field_ident.span, format!("field not found in `{ty}`"));
if !def.non_enum_variant().fields.iter().any(|field| field.ident(self.tcx) == field_ident) {
err.span_label(field_ident.span, "unknown field");
return;
}
err.span_label(
field_ident.span,
"field not available in `impl Future`, but it is available in its `Output`",
);
err.span_suggestion_verbose(
base.span.shrink_to_hi(),
"consider `await`ing on the `Future` and access the field of its `Output`",
".await",
Applicability::MaybeIncorrect,
);
}

fn ban_nonexisting_field(
Expand All @@ -2471,16 +2469,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
ty::RawPtr(..) => {
self.suggest_first_deref_field(&mut err, expr, base, ident);
}
ty::Adt(def, _) if !def.is_enum() => {
self.suggest_fields_on_recordish(&mut err, expr, def, ident);
}
ty::Param(param_ty) => {
err.span_label(ident.span, "unknown field");
self.point_at_param_definition(&mut err, param_ty);
}
ty::Alias(ty::Opaque, _) => {
self.suggest_await_on_field_access(&mut err, ident, base, base_ty.peel_refs());
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you please extract this whole block to a function and not make suggest_await_on_field_access return a value, but instead put the functionality in suggest_await_on_field_access, that is responsible for the return value, in this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't mean that you should get rid of the has_label block, just that you either move the functionality that calculates has_label to its own function. As an alternative you could also just put the logic that was responsible for calculating the return value of suggest_await_on_field_access directly into the "has_label block" instead of making suggest_await_on_field_access have a return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of the last commit instead? There's some duplication of the setting of the label, but less state to keep track of.

}
_ => {}
_ => {
err.span_label(ident.span, "unknown field");
}
}

self.suggest_fn_call(&mut err, base, base_ty, |output_ty| {
Expand Down Expand Up @@ -2633,34 +2631,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
err.span_label(param_span, format!("type parameter '{param_name}' declared here"));
}

fn suggest_fields_on_recordish(
&self,
err: &mut Diagnostic,
expr: &hir::Expr<'_>,
def: ty::AdtDef<'tcx>,
field: Ident,
) {
let available_field_names = self.available_field_names(def.non_enum_variant(), expr, &[]);
if let Some(suggested_field_name) =
find_best_match_for_name(&available_field_names, field.name, None)
{
err.span_suggestion(
field.span,
"a field with a similar name exists",
suggested_field_name,
Applicability::MaybeIncorrect,
);
} else {
err.span_label(field.span, "unknown field");
if !available_field_names.is_empty() {
err.note(format!(
"available fields are: {}",
self.name_series_display(available_field_names),
));
}
}
}

fn maybe_suggest_array_indexing(
&self,
err: &mut Diagnostic,
Expand All @@ -2669,6 +2639,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
field: Ident,
len: ty::Const<'tcx>,
) {
err.span_label(field.span, "unknown field");
if let (Some(len), Ok(user_index)) =
(len.try_eval_target_usize(self.tcx, self.param_env), field.as_str().parse::<u64>())
&& let Ok(base) = self.tcx.sess.source_map().span_to_snippet(base.span)
Expand All @@ -2691,6 +2662,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
base: &hir::Expr<'_>,
field: Ident,
) {
err.span_label(field.span, "unknown field");
if let Ok(base) = self.tcx.sess.source_map().span_to_snippet(base.span) {
let msg = format!("`{base}` is a raw pointer; try dereferencing it");
let suggestion = format!("(*{base}).{field}");
Expand All @@ -2709,18 +2681,30 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

let mut err = type_error_struct!(
self.tcx().sess,
field.span,
span,
expr_t,
E0609,
"no field `{field}` on type `{expr_t}`",
);

// try to add a suggestion in case the field is a nested field of a field of the Adt
let mod_id = self.tcx.parent_module(id).to_def_id();
if let Some((fields, args)) =
self.get_field_candidates_considering_privacy(span, expr_t, mod_id)
let (ty, unwrap) = if let ty::Adt(def, args) = expr_t.kind()
&& (self.tcx.is_diagnostic_item(sym::Result, def.did())
|| self.tcx.is_diagnostic_item(sym::Option, def.did()))
&& let Some(arg) = args.get(0)
&& let Some(ty) = arg.as_type()
{
let candidate_fields: Vec<_> = fields
(ty, "unwrap().")
} else {
(expr_t, "")
};
for (found_fields, args) in
self.get_field_candidates_considering_privacy(span, ty, mod_id, id)
{
let field_names = found_fields.iter().map(|field| field.name).collect::<Vec<_>>();
let candidate_fields: Vec<_> = found_fields
.into_iter()
.filter_map(|candidate_field| {
self.check_for_nested_field_satisfying(
span,
Expand All @@ -2729,15 +2713,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
args,
vec![],
mod_id,
id,
)
})
.map(|mut field_path| {
field_path.pop();
field_path
.iter()
.map(|id| id.name.to_ident_string())
.collect::<Vec<String>>()
.join(".")
.map(|id| format!("{}.", id.name.to_ident_string()))
.collect::<String>()
})
.collect::<Vec<_>>();

Expand All @@ -2750,9 +2734,24 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
if len > 1 { "some" } else { "one" },
if len > 1 { "have" } else { "has" },
),
candidate_fields.iter().map(|path| format!("{path}.")),
candidate_fields.iter().map(|path| format!("{unwrap}{path}")),
Applicability::MaybeIncorrect,
);
} else {
if let Some(field_name) = find_best_match_for_name(&field_names, field.name, None) {
err.span_suggestion_verbose(
field.span,
"a field with a similar name exists",
format!("{unwrap}{}", field_name),
Applicability::MaybeIncorrect,
);
} else if !field_names.is_empty() {
let is = if field_names.len() == 1 { " is" } else { "s are" };
err.note(format!(
"available field{is}: {}",
self.name_series_display(field_names),
));
}
}
}
err
Expand Down Expand Up @@ -2781,33 +2780,39 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
span: Span,
base_ty: Ty<'tcx>,
mod_id: DefId,
) -> Option<(impl Iterator<Item = &'tcx ty::FieldDef> + 'tcx, GenericArgsRef<'tcx>)> {
hir_id: hir::HirId,
) -> Vec<(Vec<&'tcx ty::FieldDef>, GenericArgsRef<'tcx>)> {
debug!("get_field_candidates(span: {:?}, base_t: {:?}", span, base_ty);

for (base_t, _) in self.autoderef(span, base_ty) {
match base_t.kind() {
ty::Adt(base_def, args) if !base_def.is_enum() => {
let tcx = self.tcx;
let fields = &base_def.non_enum_variant().fields;
// Some struct, e.g. some that impl `Deref`, have all private fields
// because you're expected to deref them to access the _real_ fields.
// This, for example, will help us suggest accessing a field through a `Box<T>`.
if fields.iter().all(|field| !field.vis.is_accessible_from(mod_id, tcx)) {
continue;
self.autoderef(span, base_ty)
.filter_map(move |(base_t, _)| {
match base_t.kind() {
ty::Adt(base_def, args) if !base_def.is_enum() => {
let tcx = self.tcx;
let fields = &base_def.non_enum_variant().fields;
// Some struct, e.g. some that impl `Deref`, have all private fields
// because you're expected to deref them to access the _real_ fields.
// This, for example, will help us suggest accessing a field through a `Box<T>`.
if fields.iter().all(|field| !field.vis.is_accessible_from(mod_id, tcx)) {
return None;
}
return Some((
fields
.iter()
.filter(move |field| {
field.vis.is_accessible_from(mod_id, tcx)
&& self.is_field_suggestable(field, hir_id, span)
})
// For compile-time reasons put a limit on number of fields we search
.take(100)
.collect::<Vec<_>>(),
*args,
));
}
return Some((
fields
.iter()
.filter(move |field| field.vis.is_accessible_from(mod_id, tcx))
// For compile-time reasons put a limit on number of fields we search
.take(100),
args,
));
_ => None,
}
_ => {}
}
}
None
})
.collect()
}

/// This method is called after we have encountered a missing field error to recursively
Expand All @@ -2820,6 +2825,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
subst: GenericArgsRef<'tcx>,
mut field_path: Vec<Ident>,
mod_id: DefId,
hir_id: HirId,
) -> Option<Vec<Ident>> {
debug!(
"check_for_nested_field_satisfying(span: {:?}, candidate_field: {:?}, field_path: {:?}",
Expand All @@ -2835,20 +2841,23 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let field_ty = candidate_field.ty(self.tcx, subst);
if matches(candidate_field, field_ty) {
return Some(field_path);
} else if let Some((nested_fields, subst)) =
self.get_field_candidates_considering_privacy(span, field_ty, mod_id)
{
// recursively search fields of `candidate_field` if it's a ty::Adt
for field in nested_fields {
if let Some(field_path) = self.check_for_nested_field_satisfying(
span,
matches,
field,
subst,
field_path.clone(),
mod_id,
) {
return Some(field_path);
} else {
for (nested_fields, subst) in
self.get_field_candidates_considering_privacy(span, field_ty, mod_id, hir_id)
{
// recursively search fields of `candidate_field` if it's a ty::Adt
for field in nested_fields {
if let Some(field_path) = self.check_for_nested_field_satisfying(
span,
matches,
field,
subst,
field_path.clone(),
mod_id,
hir_id,
) {
return Some(field_path);
}
}
}
}
Expand Down
Loading