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.

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

Keep track of patterns that could have introduced a binding, but didn't #134284

Merged
merged 3 commits into from
Dec 16, 2024
Merged
Changes from 1 commit
Commits
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
2 changes: 2 additions & 0 deletions compiler/rustc_ast/src/ast.rs
Original file line number Diff line number Diff line change
@@ -859,6 +859,8 @@ pub enum PatKind {
pub enum PatFieldsRest {
/// `module::StructName { field, ..}`
Rest,
/// `module::StructName { field, syntax error }`
Recovered(ErrorGuaranteed),
/// `module::StructName { field }`
None,
}
9 changes: 8 additions & 1 deletion compiler/rustc_ast_lowering/src/pat.rs
Original file line number Diff line number Diff line change
@@ -92,7 +92,14 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
span: self.lower_span(f.span),
}
}));
break hir::PatKind::Struct(qpath, fs, *etc == ast::PatFieldsRest::Rest);
break hir::PatKind::Struct(
qpath,
fs,
matches!(
etc,
ast::PatFieldsRest::Rest | ast::PatFieldsRest::Recovered(_)
),
);
}
PatKind::Tuple(pats) => {
let (pats, ddpos) = self.lower_pat_tuple(pats, "tuple");
5 changes: 4 additions & 1 deletion compiler/rustc_ast_pretty/src/pprust/state.rs
Original file line number Diff line number Diff line change
@@ -1653,11 +1653,14 @@ impl<'a> State<'a> {
},
|f| f.pat.span,
);
if *etc == ast::PatFieldsRest::Rest {
if let ast::PatFieldsRest::Rest | ast::PatFieldsRest::Recovered(_) = etc {
if !fields.is_empty() {
self.word_space(",");
}
self.word("..");
if let ast::PatFieldsRest::Recovered(_) = etc {
self.word("/* recovered parse error */");
}
}
if !empty {
self.space();
4 changes: 2 additions & 2 deletions compiler/rustc_parse/src/parser/pat.rs
Original file line number Diff line number Diff line change
@@ -1371,10 +1371,10 @@ impl<'a> Parser<'a> {
self.bump();
let (fields, etc) = self.parse_pat_fields().unwrap_or_else(|mut e| {
e.span_label(path.span, "while parsing the fields for this pattern");
e.emit();
let guar = e.emit();
self.recover_stmt();
// When recovering, pretend we had `Foo { .. }`, to avoid cascading errors.
(ThinVec::new(), PatFieldsRest::Rest)
(ThinVec::new(), PatFieldsRest::Recovered(guar))
});
self.bump();
Ok(PatKind::Struct(qself, path, fields, etc))
33 changes: 30 additions & 3 deletions compiler/rustc_resolve/src/late.rs
Original file line number Diff line number Diff line change
@@ -264,12 +264,17 @@ impl RibKind<'_> {
#[derive(Debug)]
pub(crate) struct Rib<'ra, R = Res> {
pub bindings: IdentMap<R>,
pub patterns_with_skipped_bindings: FxHashMap<DefId, Vec<(Span, bool /* recovered error */)>>,
Copy link
Member

Choose a reason for hiding this comment

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

why use a bool for recovered error when we have Result<(), ErrorGuaranteed>?

pub kind: RibKind<'ra>,
}

impl<'ra, R> Rib<'ra, R> {
fn new(kind: RibKind<'ra>) -> Rib<'ra, R> {
Rib { bindings: Default::default(), kind }
Rib {
bindings: Default::default(),
patterns_with_skipped_bindings: Default::default(),
kind,
}
}
}

@@ -3753,6 +3758,7 @@ impl<'a, 'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {
/// When a whole or-pattern has been dealt with, the thing happens.
///
/// See the implementation and `fresh_binding` for more details.
#[tracing::instrument(skip(self, bindings), level = "debug")]
fn resolve_pattern_inner(
&mut self,
pat: &Pat,
@@ -3761,7 +3767,6 @@ impl<'a, 'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {
) {
// Visit all direct subpatterns of this pattern.
pat.walk(&mut |pat| {
debug!("resolve_pattern pat={:?} node={:?}", pat, pat.kind);
match pat.kind {
PatKind::Ident(bmode, ident, ref sub) => {
// First try to resolve the identifier as some existing entity,
@@ -3787,8 +3792,9 @@ impl<'a, 'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {
PatKind::Path(ref qself, ref path) => {
self.smart_resolve_path(pat.id, qself, path, PathSource::Pat);
}
PatKind::Struct(ref qself, ref path, ..) => {
PatKind::Struct(ref qself, ref path, ref _fields, ref rest) => {
self.smart_resolve_path(pat.id, qself, path, PathSource::Struct);
self.record_patterns_with_skipped_bindings(pat, rest);
}
PatKind::Or(ref ps) => {
// Add a new set of bindings to the stack. `Or` here records that when a
@@ -3821,6 +3827,27 @@ impl<'a, 'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {
});
}

fn record_patterns_with_skipped_bindings(&mut self, pat: &Pat, rest: &ast::PatFieldsRest) {
match rest {
ast::PatFieldsRest::Rest | ast::PatFieldsRest::Recovered(_) => {
// Record that the pattern doesn't introduce all the bindings it could.
if let Some(partial_res) = self.r.partial_res_map.get(&pat.id)
&& let Some(res) = partial_res.full_res()
&& let Some(def_id) = res.opt_def_id()
{
self.ribs[ValueNS]
.last_mut()
.unwrap()
.patterns_with_skipped_bindings
.entry(def_id)
.or_default()
.push((pat.span, matches!(rest, ast::PatFieldsRest::Recovered(_))));
}
}
ast::PatFieldsRest::None => {}
}
}

fn fresh_binding(
&mut self,
ident: Ident,
60 changes: 60 additions & 0 deletions compiler/rustc_resolve/src/late/diagnostics.rs
Original file line number Diff line number Diff line change
@@ -430,6 +430,7 @@ impl<'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'_, 'ast, 'ra, 'tcx> {
let mut err = self.r.dcx().struct_span_err(base_error.span, base_error.msg.clone());
err.code(code);

self.detect_missing_binding_available_from_pattern(&mut err, path, following_seg);
self.suggest_at_operator_in_slice_pat_with_range(&mut err, path);
self.suggest_swapping_misplaced_self_ty_and_trait(&mut err, source, res, base_error.span);

@@ -1120,6 +1121,65 @@ impl<'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'_, 'ast, 'ra, 'tcx> {
true
}

fn detect_missing_binding_available_from_pattern(
&mut self,
err: &mut Diag<'_>,
path: &[Segment],
following_seg: Option<&Segment>,
) {
let [segment] = path else { return };
let None = following_seg else { return };
'outer: for rib in self.ribs[ValueNS].iter().rev() {
for (def_id, spans) in &rib.patterns_with_skipped_bindings {
if let Some(fields) = self.r.field_idents(*def_id) {
for field in fields {
if field.name == segment.ident.name {
if spans.iter().all(|(_, was_recovered)| *was_recovered) {
// This resolution error will likely be fixed by fixing a
// syntax error in a pattern, so it is irrelevant to the user.
let multispan: MultiSpan =
spans.iter().map(|(s, _)| *s).collect::<Vec<_>>().into();
err.span_note(
multispan,
"this pattern had a recovered parse error which likely \
lost the expected fields",
);
err.downgrade_to_delayed_bug();
}
let mut multispan: MultiSpan = spans
.iter()
.filter(|(_, was_recovered)| !was_recovered)
.map(|(sp, _)| *sp)
.collect::<Vec<_>>()
.into();
let def_span = self.r.def_span(*def_id);
let ty = self.r.tcx.item_name(*def_id);
multispan.push_span_label(def_span, String::new());
multispan.push_span_label(field.span, "defined here".to_string());
for (span, _) in spans.iter().filter(|(_, r)| !r) {
multispan.push_span_label(
*span,
format!(
"this pattern doesn't include `{field}`, which is \
available in `{ty}`",
),
);
}
err.span_note(
multispan,
format!(
"`{ty}` has a field `{field}` which could have been included \
in this pattern, but it wasn't",
),
);
break 'outer;
}
}
}
}
}
}

fn suggest_at_operator_in_slice_pat_with_range(
&mut self,
err: &mut Diag<'_>,
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
struct Website {
url: String,
title: Option<String> ,//~ NOTE defined here
}

fn main() {
let website = Website {
url: "http://www.example.com".into(),
title: Some("Example Domain".into()),
};

if let Website { url, Some(title) } = website { //~ ERROR expected `,`
//~^ NOTE while parsing the fields for this pattern
println!("[{}]({})", title, url); // we hide the errors for `title` and `url`
}

if let Website { url, .. } = website { //~ NOTE `Website` has a field `title`
//~^ NOTE this pattern
println!("[{}]({})", title, url); //~ ERROR cannot find value `title` in this scope
//~^ NOTE not found in this scope
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
error: expected `,`
--> $DIR/struct-pattern-with-missing-fields-resolve-error.rs:12:31
|
LL | if let Website { url, Some(title) } = website {
| ------- ^
| |
| while parsing the fields for this pattern

error[E0425]: cannot find value `title` in this scope
--> $DIR/struct-pattern-with-missing-fields-resolve-error.rs:19:30
|
LL | println!("[{}]({})", title, url);
| ^^^^^ not found in this scope
|
note: `Website` has a field `title` which could have been included in this pattern, but it wasn't
--> $DIR/struct-pattern-with-missing-fields-resolve-error.rs:17:12
|
LL | / struct Website {
LL | | url: String,
LL | | title: Option<String> ,
| | ----- defined here
LL | | }
| |_-
...
LL | if let Website { url, .. } = website {
| ^^^^^^^^^^^^^^^^^^^ this pattern doesn't include `title`, which is available in `Website`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0425`.
Loading