Skip to content

diagnostic: properly deal with hygienic names on unresolved fields and imports #116575

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

Closed
wants to merge 2 commits into from
Closed

diagnostic: properly deal with hygienic names on unresolved fields and imports #116575

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
449ff6b
diagnostics: further improve handling of hygienic struct fields
fmease Oct 7, 2023
f0c83e4
diagnostics: unresolved imports: don't suggest hygienic names
fmease Oct 9, 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
28 changes: 16 additions & 12 deletions compiler/rustc_hir_typeck/src/expr.rs
View file
Original file line number Diff line number Diff line change
@@ -1659,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,
)
};

@@ -2046,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;
}
@@ -2099,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,
@@ -2117,16 +2117,17 @@ 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,
);
}
},
_ => {
// prevent all specified fields from being suggested
let available_field_names = self.available_field_names(variant, expr, skip_fields);
// Prevent all specified fields from being suggested with `skip_fields`.
let available_field_names =
self.available_field_names(variant, field.ident.span, skip_fields);
if let Some(field_name) =
find_best_match_for_name(&available_field_names, field.ident.name, None)
{
@@ -2170,15 +2171,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
fn available_field_names(
&self,
variant: &'tcx ty::VariantDef,
expr: &hir::Expr<'_>,
span: Span,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you specify which span this is?

skip_fields: &[hir::ExprField<'_>],
) -> Vec<Symbol> {
let block = self.tcx.hir().local_def_id_to_hir_id(self.body_id);
let (span, def_scope) = self.tcx.adjust_span_and_get_scope(span, variant.def_id, block);

variant
.fields
.iter()
.filter(|field| {
skip_fields.iter().all(|&skip| skip.ident.name != field.name)
&& self.is_field_suggestable(field, expr.hir_id, expr.span)
&& self.is_field_suggestable(field, def_scope, span)
})
.map(|field| field.name)
.collect()
@@ -2409,7 +2413,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, expr, def, ident);
self.suggest_fields_on_recordish(&mut err, def, ident);
}
ty::Param(param_ty) => {
self.point_at_param_definition(&mut err, param_ty);
@@ -2571,11 +2575,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
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, &[]);
let available_field_names =
self.available_field_names(def.non_enum_variant(), field.span, &[]);
if let Some(suggested_field_name) =
find_best_match_for_name(&available_field_names, field.name, None)
{
11 changes: 8 additions & 3 deletions compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs
View file
Original file line number Diff line number Diff line change
@@ -7,6 +7,7 @@ use rustc_ast::util::parser::{ExprPrecedence, PREC_POSTFIX};
use rustc_errors::{Applicability, Diagnostic, MultiSpan};
use rustc_hir as hir;
use rustc_hir::def::{CtorKind, CtorOf, DefKind};
use rustc_hir::def_id::DefId;
use rustc_hir::lang_items::LangItem;
use rustc_hir::{
AsyncGeneratorKind, Expr, ExprKind, GeneratorKind, GenericBound, HirId, Node, Path, QPath,
@@ -1686,14 +1687,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
}

/// Returns `true` if the given `field` can be suggested in diagnostics.
///
/// The `span` represents the precise expected usage site, normalized to
/// macros 2.0 and adjusted to the expansion that defined the scope.
pub(crate) fn is_field_suggestable(
&self,
field: &ty::FieldDef,
hir_id: HirId,
def_scope: DefId,
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 be visible in the containing scope.
field.vis.is_accessible_from(def_scope, self.tcx)
// The field must not be unstable.
&& !matches!(
self.tcx.eval_stability(field.did, None, rustc_span::DUMMY_SP, None),
29 changes: 20 additions & 9 deletions compiler/rustc_hir_typeck/src/pat.rs
View file
Original file line number Diff line number Diff line change
@@ -1407,7 +1407,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
adt.variant_descr(),
&inexistent_fields,
&mut unmentioned_fields,
pat,
variant,
args,
))
@@ -1431,10 +1430,19 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
tcx.sess.emit_err(errors::UnionPatDotDot { span: pat.span });
}
} else if !unmentioned_fields.is_empty() {
let block = self.tcx.hir().local_def_id_to_hir_id(self.body_id);
// It's hard to pinpoint who is to blame for the unmentioned fields and for whom those
// fields are actually inaccessible since the fields that *are* mentioned may come from
// different expansions. Instead of employing a heuristic that tries to find the most
// relevant expansion, just blame the struct pattern `pat` which isn't great but
// at least it's technically correct.
let (span, def_scope) =
self.tcx.adjust_span_and_get_scope(pat.span, variant.def_id, block);

let accessible_unmentioned_fields: Vec<_> = unmentioned_fields
.iter()
.copied()
.filter(|(field, _)| self.is_field_suggestable(field, pat.hir_id, pat.span))
.filter(|(field, _)| self.is_field_suggestable(field, def_scope, span))
.collect();

if !has_rest_pat {
@@ -1570,7 +1578,6 @@ 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> {
@@ -1614,7 +1621,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
);

if let [(field_def, field)] = unmentioned_fields.as_slice()
&& self.is_field_suggestable(field_def, pat.hir_id, pat.span)
&& let block = self.tcx.hir().local_def_id_to_hir_id(self.body_id)
&& let (span, def_scope) =
self.tcx.adjust_span_and_get_scope(pat_field.ident.span, variant.def_id, block)
&& self.is_field_suggestable(field_def, def_scope, span)
{
let suggested_name =
find_best_match_for_name(&[field.name], pat_field.ident.name, None);
@@ -1881,17 +1891,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let len = unmentioned_fields.len();
let (prefix, postfix, sp) = match fields {
[] => match &pat.kind {
PatKind::Struct(path, [], false) => {
PatKind::Struct(path, [], false) if path.span().eq_ctxt(pat.span) => {
(" { ", " }", path.span().shrink_to_hi().until(pat.span.shrink_to_hi()))
Comment on lines +1894 to 1895
Copy link
Contributor

Choose a reason for hiding this comment

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

You can generalize this by looking at path.span().ancestor_within_span(pat.span).

}
_ => return err,
},
[.., field] => {
// Account for last field having a trailing comma or parse recovery at the tail of
// the pattern to avoid invalid suggestion (#78511).
let tail = field.span.shrink_to_hi().with_hi(pat.span.hi());
match &pat.kind {
PatKind::Struct(..) => (", ", " }", tail),
// Account for last field having a trailing comma or parse recovery at the tail of
// the pattern to avoid invalid suggestion (#78511).
PatKind::Struct(..) if field.span.eq_ctxt(pat.span) => {
(", ", " }", field.span.shrink_to_hi().with_hi(pat.span.hi()))
Comment on lines +1903 to +1904
Copy link
Contributor

Choose a reason for hiding this comment

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

You can generalize this by looking at field.span.ancestor_within_span(pat.span).

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've already tried using find_ancestor_inside in some earlier version of this patch before opening a PR without a satisfying result if I remember correctly. However, let me check again tomorrow, I might've missed something.

Copy link
Member Author

@fmease fmease Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, so I'm almost certain that pat.span.FIND_ANCESTOR(field.span) with FIND_ANCESTOR ∊ {find_ancestor_inside, find_ancestor_in_same_ctxt} or just field.span isn't what we want here, eq_ctxt is conservative and correct (see next comment).

If the field and the struct patterns aren't in the same syntax context, there's not much we can do here structured-suggestion-wise without resorting to elaborate span_to_snippet hacks. If the field comes from a different expansion, then the struct pattern has to be of the form S { … $field … } or S { … $( … )… … }, meaning the field has to be a metavariable or a repetition but notably not a macro call like S { … m!(…) … } (ungrammatical). Therefore, there are two location were we could theoretically suggest the unmentioned fields:

  1. At the end of the struct pattern (~pat.span): However, we don't know if we should add a leading comma or not since we'd need to look at def-site (consider S { $( $fields/*,*/ )* }) and use-site (consider m!(field/*,*/)) which requires span_to_snippet I think and is ugly. The suggestion is marked MachineApplicable btw (we could ofc tweak that in this case to be MaybeIncorrect).
  2. At the end of the last field (~field.span)
    • However, the other fields (other than the last one) might come from a more "general" expansion.
    • The suggestion might be incorrect since we don't know the shape of the metavariable $field (etc.). Consider m!(field: _) where we don't know if the suggestion m!(field: _, missing: _) would be valid input for macro m

Copy link
Member Author

@fmease fmease Oct 12, 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 noticed that eq_ctxt is not sufficient anyway for preventing invalid suggestions. For example:

struct S {
    a: (),
    b: (),
}

macro_rules! n {
    ($( $f:ident )?) => {
        let S { $( $f )? } = loop {};
    }
}

fn main() {
    n!(a);
}

Here, we suggest garbage:

error[E0027]: pattern does not mention field `b`
  --> krash.rs:17:13
   |
17 |         let S { $( $f )? } = loop {};
   |             ^^^^^^^^^^^^^^ missing field `b`
...
23 |     n!(a);
   |     ----- in this macro invocation
   |
   = note: this error originates in the macro `n` (in Nightly builds, run with -Z macro-backtrace for more info)
help: include the missing field in the pattern
   |
17 |         let S { $( $f, b } = loop {};
   |                      ~~~~~
help: if you don't care about this missing field, you can explicitly ignore it
   |
17 |         let S { $( $f, .. } = loop {};
   |                      ~~~~~~

On master, we don't but on the other hand we misclassifiy a and b to be inaccessible:

error: pattern requires `..` due to inaccessible fields
  --> krash.rs:17:13
   |
17 |         let S { $( $f )? } = loop {};
   |             ^^^^^^^^^^^^^^
...
23 |     n!(a);
   |     ----- in this macro invocation
   |
   = note: this error originates in the macro `n` (in Nightly builds, run with -Z macro-backtrace for more info)
help: ignore the inaccessible and unused fields
   |
17 |         let S { $( $f, .. )? } = loop {};
   |                      ++++

Seems like I still need to tweak some things :|.

}
_ => return err,
}
}
18 changes: 14 additions & 4 deletions compiler/rustc_middle/src/ty/mod.rs
View file
Original file line number Diff line number Diff line change
@@ -2510,16 +2510,26 @@ impl<'tcx> TyCtxt<'tcx> {
// FIXME(vincenzopalazzo): move the HirId to a LocalDefId
pub fn adjust_ident_and_get_scope(
self,
mut ident: Ident,
ident: Ident,
scope: DefId,
block: hir::HirId,
) -> (Ident, DefId) {
let scope = ident
.span
let (span, scope) = self.adjust_span_and_get_scope(ident.span, scope, block);
(Ident { span, ..ident }, scope)
}

// FIXME(vincenzopalazzo): move the HirId to a LocalDefId
pub fn adjust_span_and_get_scope(
self,
mut span: Span,
scope: DefId,
block: hir::HirId,
) -> (Span, DefId) {
let scope = span
.normalize_to_macros_2_0_and_adjust(self.expn_that_defined(scope))
.and_then(|actual_expansion| actual_expansion.expn_data().parent_module)
.unwrap_or_else(|| self.parent_module(block).to_def_id());
(ident, scope)
(span, scope)
}

/// Returns `true` if the debuginfo for `span` should be collapsed to the outermost expansion
66 changes: 36 additions & 30 deletions compiler/rustc_resolve/src/imports.rs
View file
Original file line number Diff line number Diff line change
@@ -1102,39 +1102,45 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
});

return if all_ns_failed {
let resolutions = match module {
ModuleOrUniformRoot::Module(module) => Some(self.resolutions(module).borrow()),
_ => None,
};
let resolutions = resolutions.as_ref().into_iter().flat_map(|r| r.iter());
let names = resolutions
.filter_map(|(BindingKey { ident: i, .. }, resolution)| {
if i.name == ident.name {
return None;
} // Never suggest the same name
match *resolution.borrow() {
NameResolution { binding: Some(name_binding), .. } => {
match name_binding.kind {
NameBindingKind::Import { binding, .. } => {
match binding.kind {
// Never suggest the name that has binding error
// i.e., the name that cannot be previously resolved
NameBindingKind::Res(Res::Err) => None,
_ => Some(i.name),
}
let names = if let ModuleOrUniformRoot::Module(module) = module {
let mut adjusted_sp = ident.span;
adjusted_sp.normalize_to_macros_2_0_and_adjust(module.expansion);

self.resolutions(module)
.borrow()
.iter()
.filter_map(|(BindingKey { ident: i, .. }, resolution)| {
// Never suggest the same name
if i.name == ident.name {
return None;
}
// Don't suggest hygienic names from different syntax contexts.
if !i.span.normalize_to_macros_2_0().eq_ctxt(adjusted_sp) {
return None;
}
match *resolution.borrow() {
NameResolution { binding: Some(name_binding), .. } => {
// Never suggest the name that has binding error
// i.e., the name that cannot be previously resolved
if let NameBindingKind::Import { binding, .. } = name_binding.kind
&& let NameBindingKind::Res(Res::Err) = binding.kind
{
return None;
}
_ => Some(i.name),
}
NameResolution { ref single_imports, .. }
if single_imports.is_empty() =>
{
return None;
}
_ => {}
}
NameResolution { ref single_imports, .. }
if single_imports.is_empty() =>
{
None
}
_ => Some(i.name),
}
})
.collect::<Vec<Symbol>>();
Some(i.name)
})
.collect()
} else {
Vec::new()
};

let lev_suggestion =
find_best_match_for_name(&names, ident.name, None).map(|suggestion| {
Loading