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

Improve suggestion for tuple struct pattern matching errors. #81235

Merged
merged 3 commits into from
Feb 23, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
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: 1 addition & 1 deletion compiler/rustc_parse/src/parser/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -932,7 +932,7 @@ impl<'a> Parser<'a> {
self.bump();
Ok(Ident::new(symbol, self.prev_token.span))
} else {
self.parse_ident_common(false)
self.parse_ident_common(true)
}
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_parse/src/parser/pat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -999,7 +999,7 @@ impl<'a> Parser<'a> {
let boxed_span = self.token.span;
let is_ref = self.eat_keyword(kw::Ref);
let is_mut = self.eat_keyword(kw::Mut);
let fieldname = self.parse_ident()?;
let fieldname = self.parse_field_name()?;
hi = self.prev_token.span;

let bind_type = match (is_ref, is_mut) {
Expand Down
98 changes: 84 additions & 14 deletions compiler/rustc_typeck/src/check/pat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use rustc_span::lev_distance::find_best_match_for_name;
use rustc_span::source_map::{Span, Spanned};
use rustc_span::symbol::Ident;
use rustc_trait_selection::traits::{ObligationCause, Pattern};
use ty::VariantDef;

use std::cmp;
use std::collections::hash_map::Entry::{Occupied, Vacant};
Expand Down Expand Up @@ -1209,14 +1210,64 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
u.emit();
}
}
(None, Some(mut err)) | (Some(mut err), None) => {
(None, Some(mut u)) => {
if let Some(mut e) = self.error_tuple_variant_as_struct_pat(pat, fields, variant) {
u.delay_as_bug();
e.emit();
} else {
u.emit();
}
}
(Some(mut err), None) => {
err.emit();
}
(None, None) => {}
(None, None) => {
if let Some(mut err) =
self.error_tuple_variant_index_shorthand(variant, pat, fields)
{
err.emit();
}
}
}
no_field_errors
}

fn error_tuple_variant_index_shorthand(
&self,
variant: &VariantDef,
pat: &'_ Pat<'_>,
fields: &[hir::FieldPat<'_>],
) -> Option<DiagnosticBuilder<'_>> {
// if this is a tuple struct, then all field names will be numbers
// so if any fields in a struct pattern use shorthand syntax, they will
// be invalid identifiers (for example, Foo { 0, 1 }).
if let (CtorKind::Fn, PatKind::Struct(qpath, field_patterns, ..)) =
(variant.ctor_kind, &pat.kind)
{
let has_shorthand_field_name = field_patterns.iter().any(|field| field.is_shorthand);
if has_shorthand_field_name {
let path = rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| {
s.print_qpath(qpath, false)
});
let mut err = struct_span_err!(
self.tcx.sess,
pat.span,
E0769,
"tuple variant `{}` written as struct variant",
path
);
err.span_suggestion(
qpath.span().shrink_to_hi().until(pat.span),
"use the tuple variant pattern syntax instead",
format!("({})", self.get_suggested_tuple_struct_pattern(fields, variant)),
Applicability::MaybeIncorrect,
);
return Some(err);
}
}
None
}

fn error_foreign_non_exhaustive_spat(&self, pat: &Pat<'_>, descr: &str, no_fields: bool) {
let sess = self.tcx.sess;
let sm = sess.source_map();
Expand Down Expand Up @@ -1356,16 +1407,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
);
let (sugg, appl) = if fields.len() == variant.fields.len() {
(
fields
.iter()
.map(|f| match self.tcx.sess.source_map().span_to_snippet(f.pat.span) {
Ok(f) => f,
Err(_) => rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| {
s.print_pat(f.pat)
}),
})
.collect::<Vec<String>>()
.join(", "),
self.get_suggested_tuple_struct_pattern(fields, variant),
Applicability::MachineApplicable,
)
} else {
Expand All @@ -1375,16 +1417,44 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
)
};
err.span_suggestion(
pat.span,
qpath.span().shrink_to_hi().until(pat.span),
"use the tuple variant pattern syntax instead",
format!("{}({})", path, sugg),
format!("({})", sugg),
appl,
);
return Some(err);
}
None
}

fn get_suggested_tuple_struct_pattern(
&self,
fields: &[hir::FieldPat<'_>],
variant: &VariantDef,
) -> String {
let variant_field_idents = variant.fields.iter().map(|f| f.ident).collect::<Vec<Ident>>();
fields
.iter()
.map(|field| {
match self.tcx.sess.source_map().span_to_snippet(field.pat.span) {
Ok(f) => {
// Field names are numbers, but numbers
// are not valid identifiers
if variant_field_idents.contains(&field.ident) {
String::from("_")
} else {
f
}
}
Err(_) => rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| {
s.print_pat(field.pat)
}),
}
Comment on lines +1439 to +1452
Copy link
Contributor

@estebank estebank Jan 27, 2021

Choose a reason for hiding this comment

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

This code is fair (as you're just copying from existing logic), but ideally we would not be using span_to_snippet here. (No need to change this now.)

})
.collect::<Vec<String>>()
.join(", ")
}

/// Returns a diagnostic reporting a struct pattern which is missing an `..` due to
/// inaccessible fields.
///
Expand Down
4 changes: 3 additions & 1 deletion src/test/ui/issues/issue-17800.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ error[E0769]: tuple variant `MyOption::MySome` written as struct variant
--> $DIR/issue-17800.rs:8:9
|
LL | MyOption::MySome { x: 42 } => (),
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the tuple variant pattern syntax instead: `MyOption::MySome(42)`
| ----------------^^^^^^^^^^
| |
| help: use the tuple variant pattern syntax instead: `(42)`

error: aborting due to previous error

Expand Down
4 changes: 3 additions & 1 deletion src/test/ui/missing/missing-fields-in-struct-pattern.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ error[E0769]: tuple variant `S` written as struct variant
--> $DIR/missing-fields-in-struct-pattern.rs:4:12
|
LL | if let S { a, b, c, d } = S(1, 2, 3, 4) {
| ^^^^^^^^^^^^^^^^ help: use the tuple variant pattern syntax instead: `S(a, b, c, d)`
| -^^^^^^^^^^^^^^^
| |
| help: use the tuple variant pattern syntax instead: `(a, b, c, d)`
Copy link
Contributor

Choose a reason for hiding this comment

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

The span in all of the tests' suggestions seems to now be wrong. The resulting code would be (a, b, c, d) instead of the desired S(a, b, c, d).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The suggestion here seemed like you were suggesting to remove the path from the help text. It seems like I misunderstood the intent there, what were you suggesting to use instead of the path?

Copy link
Contributor

Choose a reason for hiding this comment

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

My proposal is to use span_suggestion_verbose to have a separate suggestion subwindow, but for the suggestion to be correctly displayed you need to make a span trimming the S from the beginning. The suggestion has to have the correct span for it to be rendered correctly.

In the case highlighted above, you had a span pointing at S { a, b, c, d }. You now have a span pointing at S. Creating a new span that starts at the end of S's span, you'll have a span pointing at { a, b, c, d } which would do then render correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, having that suggestion with span_suggestion_verbose makes a lot of sense. Thanks for the clarification, still figuring out all the options for these diagnostics 😅


error: aborting due to previous error

Expand Down
4 changes: 3 additions & 1 deletion src/test/ui/parser/recover-from-bad-variant.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ error[E0769]: tuple variant `Enum::Bar` written as struct variant
--> $DIR/recover-from-bad-variant.rs:12:9
|
LL | Enum::Bar { a, b } => {}
| ^^^^^^^^^^^^^^^^^^ help: use the tuple variant pattern syntax instead: `Enum::Bar(a, b)`
| ---------^^^^^^^^^
| |
| help: use the tuple variant pattern syntax instead: `(a, b)`

error: aborting due to 3 previous errors

Expand Down
15 changes: 15 additions & 0 deletions src/test/ui/structs/struct-tuple-field-names.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
struct S(i32, f32);
enum E {
S(i32, f32),
}
fn main() {
let x = E::S(1, 2.2);
match x {
E::S { 0, 1 } => {}
//~^ ERROR tuple variant `E::S` written as struct variant [E0769]
}
let y = S(1, 2.2);
match y {
S { } => {} //~ ERROR: tuple variant `S` written as struct variant [E0769]
}
}
19 changes: 19 additions & 0 deletions src/test/ui/structs/struct-tuple-field-names.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
error[E0769]: tuple variant `E::S` written as struct variant
--> $DIR/struct-tuple-field-names.rs:8:9
|
LL | E::S { 0, 1 } => {}
| ----^^^^^^^^^
| |
| help: use the tuple variant pattern syntax instead: `(_, _)`

error[E0769]: tuple variant `S` written as struct variant
--> $DIR/struct-tuple-field-names.rs:13:9
|
LL | S { } => {}
| -^^^^
| |
| help: use the tuple variant pattern syntax instead: `(_, _)`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0769`.
4 changes: 3 additions & 1 deletion src/test/ui/type/type-check/issue-41314.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ error[E0769]: tuple variant `X::Y` written as struct variant
--> $DIR/issue-41314.rs:7:9
|
LL | X::Y { number } => {}
| ^^^^^^^^^^^^^^^ help: use the tuple variant pattern syntax instead: `X::Y(number)`
| ----^^^^^^^^^^^
| |
| help: use the tuple variant pattern syntax instead: `(number)`

error: aborting due to previous error

Expand Down