-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Suggest _
and ..
if a pattern has too few fields
#80017
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
Changes from all commits
16692ab
5fe61a7
f3d9df5
a5e8e6e
fe82cc3
9959d6d
1bce775
d7307a7
e8c8793
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -15,6 +15,7 @@ use rustc_span::hygiene::DesugaringKind; | |||||||
use rustc_span::lev_distance::find_best_match_for_name; | ||||||||
use rustc_span::source_map::{Span, Spanned}; | ||||||||
use rustc_span::symbol::Ident; | ||||||||
use rustc_span::{BytePos, DUMMY_SP}; | ||||||||
use rustc_trait_selection::traits::{ObligationCause, Pattern}; | ||||||||
|
||||||||
use std::cmp; | ||||||||
|
@@ -1001,7 +1002,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||||||
// More generally, the expected type wants a tuple variant with one field of an | ||||||||
// N-arity-tuple, e.g., `V_i((p_0, .., p_N))`. Meanwhile, the user supplied a pattern | ||||||||
// with the subpatterns directly in the tuple variant pattern, e.g., `V_i(p_0, .., p_N)`. | ||||||||
let missing_parenthesis = match (&expected.kind(), fields, had_err) { | ||||||||
let missing_parentheses = match (&expected.kind(), fields, had_err) { | ||||||||
// #67037: only do this if we could successfully type-check the expected type against | ||||||||
// the tuple struct pattern. Otherwise the substs could get out of range on e.g., | ||||||||
// `let P() = U;` where `P != U` with `struct P<T>(T);`. | ||||||||
|
@@ -1014,13 +1015,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||||||
} | ||||||||
_ => false, | ||||||||
}; | ||||||||
if missing_parenthesis { | ||||||||
if missing_parentheses { | ||||||||
let (left, right) = match subpats { | ||||||||
// This is the zero case; we aim to get the "hi" part of the `QPath`'s | ||||||||
// span as the "lo" and then the "hi" part of the pattern's span as the "hi". | ||||||||
// This looks like: | ||||||||
// | ||||||||
// help: missing parenthesis | ||||||||
// help: missing parentheses | ||||||||
// | | ||||||||
// L | let A(()) = A(()); | ||||||||
// | ^ ^ | ||||||||
|
@@ -1029,17 +1030,63 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||||||
// last sub-pattern. In the case of `A(x)` the first and last may coincide. | ||||||||
// This looks like: | ||||||||
// | ||||||||
// help: missing parenthesis | ||||||||
// help: missing parentheses | ||||||||
// | | ||||||||
// L | let A((x, y)) = A((1, 2)); | ||||||||
// | ^ ^ | ||||||||
[first, ..] => (first.span.shrink_to_lo(), subpats.last().unwrap().span), | ||||||||
}; | ||||||||
err.multipart_suggestion( | ||||||||
"missing parenthesis", | ||||||||
"missing parentheses", | ||||||||
camelid marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
vec![(left, "(".to_string()), (right.shrink_to_hi(), ")".to_string())], | ||||||||
Applicability::MachineApplicable, | ||||||||
); | ||||||||
} else if fields.len() > subpats.len() { | ||||||||
let after_fields_span = if pat_span == DUMMY_SP { | ||||||||
pat_span | ||||||||
Comment on lines
+1045
to
+1046
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is to prevent a compiler crash if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if the error rendering system receives a diagnostic that has There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error renderer will ignore |
||||||||
} else { | ||||||||
pat_span.with_hi(pat_span.hi() - BytePos(1)).shrink_to_hi() | ||||||||
camelid marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
}; | ||||||||
let all_fields_span = match subpats { | ||||||||
[] => after_fields_span, | ||||||||
[field] => field.span, | ||||||||
[first, .., last] => first.span.to(last.span), | ||||||||
}; | ||||||||
|
||||||||
// Check if all the fields in the pattern are wildcards. | ||||||||
let all_wildcards = subpats.iter().all(|pat| matches!(pat.kind, PatKind::Wild)); | ||||||||
|
||||||||
let mut wildcard_sugg = vec!["_"; fields.len() - subpats.len()].join(", "); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be good to add a comment for potential improvements here.
Suggested change
|
||||||||
if !subpats.is_empty() { | ||||||||
wildcard_sugg = String::from(", ") + &wildcard_sugg; | ||||||||
} | ||||||||
|
||||||||
err.span_suggestion_verbose( | ||||||||
after_fields_span, | ||||||||
"use `_` to explicitly ignore each field", | ||||||||
wildcard_sugg, | ||||||||
Applicability::MaybeIncorrect, | ||||||||
); | ||||||||
|
||||||||
// Only suggest `..` if more than one field is missing | ||||||||
// or the pattern consists of all wildcards. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Would be better to show an example here for the comment. |
||||||||
if fields.len() - subpats.len() > 1 || all_wildcards { | ||||||||
if subpats.is_empty() || all_wildcards { | ||||||||
err.span_suggestion_verbose( | ||||||||
all_fields_span, | ||||||||
"use `..` to ignore all fields", | ||||||||
String::from(".."), | ||||||||
Applicability::MaybeIncorrect, | ||||||||
); | ||||||||
} else { | ||||||||
err.span_suggestion_verbose( | ||||||||
after_fields_span, | ||||||||
"use `..` to ignore the rest of the fields", | ||||||||
String::from(", .."), | ||||||||
Applicability::MaybeIncorrect, | ||||||||
); | ||||||||
} | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
err.emit(); | ||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,15 @@ LL | struct TupleStruct<S, T>(S, T); | |
... | ||
LL | TupleStruct(_) = TupleStruct(1, 2); | ||
| ^^^^^^^^^^^^^^ expected 2 fields, found 1 | ||
| | ||
help: use `_` to explicitly ignore each field | ||
| | ||
LL | TupleStruct(_, _) = TupleStruct(1, 2); | ||
| ^^^ | ||
Comment on lines
+34
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two cases should also be suggesting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, if someone is already explicitly listing out wildcards, should we suggest that they use I'll change it, but for other things we can always adjust the behavior later; I'm hoping to have this PR finished soon. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds reasonable, I don't foresee any big changes. I think that the code itself looks good but want to nail down the exact behavior we want. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So is the behavior you're suggesting that we also suggest There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think that if it is currently |
||
help: use `..` to ignore all fields | ||
| | ||
LL | TupleStruct(..) = TupleStruct(1, 2); | ||
| ^^ | ||
|
||
error[E0023]: this pattern has 3 fields, but the corresponding tuple variant has 2 fields | ||
--> $DIR/tuple_struct_destructure_fail.rs:34:5 | ||
|
@@ -49,6 +58,15 @@ LL | SingleVariant(S, T) | |
... | ||
LL | Enum::SingleVariant(_) = Enum::SingleVariant(1, 2); | ||
| ^^^^^^^^^^^^^^^^^^^^^^ expected 2 fields, found 1 | ||
| | ||
help: use `_` to explicitly ignore each field | ||
| | ||
LL | Enum::SingleVariant(_, _) = Enum::SingleVariant(1, 2); | ||
| ^^^ | ||
help: use `..` to ignore all fields | ||
| | ||
LL | Enum::SingleVariant(..) = Enum::SingleVariant(1, 2); | ||
| ^^ | ||
|
||
error[E0070]: invalid left-hand side of assignment | ||
--> $DIR/tuple_struct_destructure_fail.rs:40:12 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,15 @@ LL | struct P<T>(T); // 1 type parameter wanted | |
... | ||
LL | let P() = U {}; | ||
| ^^^ expected 1 field, found 0 | ||
| | ||
help: use `_` to explicitly ignore each field | ||
| | ||
LL | let P(_) = U {}; | ||
| ^ | ||
help: use `..` to ignore all fields | ||
| | ||
LL | let P(..) = U {}; | ||
| ^^ | ||
Comment on lines
+21
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that we'll need to special case when there's only one field missing (and all the other preceding fields aren't It also feels like we could deal with language for a single field. My ideal output for this case would be:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I actually think we should suggest And also, as you said, I would like to get this merged soon. I appreciate the desire to make diagnostics as good as possible - I share that desire - but I feel that further changes would have diminishing returns and increase the risk of introducing bugs. Also, this change has taken way longer than I anticipated and I would like to spend my time on some other things :) |
||
|
||
error: aborting due to 2 previous errors | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,15 @@ LL | Rgb(usize, usize, usize), | |
... | ||
LL | Color::Rgb(_, _) => { } | ||
| ^^^^^^^^^^^^^^^^ expected 3 fields, found 2 | ||
| | ||
help: use `_` to explicitly ignore each field | ||
| | ||
LL | Color::Rgb(_, _, _) => { } | ||
| ^^^ | ||
help: use `..` to ignore all fields | ||
| | ||
LL | Color::Rgb(..) => { } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't we suggest There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I consider Edit: let me clarify :)
|
||
| ^^ | ||
|
||
error: aborting due to previous error | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
struct S(i32, f32); | ||
enum E { | ||
S(i32, f32), | ||
} | ||
struct Point4(i32, i32, i32, i32); | ||
|
||
fn main() { | ||
match S(0, 1.0) { | ||
S(x) => {} | ||
//~^ ERROR this pattern has 1 field, but the corresponding tuple struct has 2 fields | ||
//~| HELP use `_` to explicitly ignore each field | ||
} | ||
match S(0, 1.0) { | ||
S(_) => {} | ||
//~^ ERROR this pattern has 1 field, but the corresponding tuple struct has 2 fields | ||
//~| HELP use `_` to explicitly ignore each field | ||
//~| HELP use `..` to ignore all fields | ||
} | ||
match S(0, 1.0) { | ||
S() => {} | ||
//~^ ERROR this pattern has 0 fields, but the corresponding tuple struct has 2 fields | ||
//~| HELP use `_` to explicitly ignore each field | ||
//~| HELP use `..` to ignore all fields | ||
} | ||
|
||
match E::S(0, 1.0) { | ||
E::S(x) => {} | ||
//~^ ERROR this pattern has 1 field, but the corresponding tuple variant has 2 fields | ||
//~| HELP use `_` to explicitly ignore each field | ||
} | ||
match E::S(0, 1.0) { | ||
E::S(_) => {} | ||
//~^ ERROR this pattern has 1 field, but the corresponding tuple variant has 2 fields | ||
//~| HELP use `_` to explicitly ignore each field | ||
//~| HELP use `..` to ignore all fields | ||
} | ||
match E::S(0, 1.0) { | ||
E::S() => {} | ||
//~^ ERROR this pattern has 0 fields, but the corresponding tuple variant has 2 fields | ||
//~| HELP use `_` to explicitly ignore each field | ||
//~| HELP use `..` to ignore all fields | ||
} | ||
match E::S(0, 1.0) { | ||
E::S => {} | ||
//~^ ERROR expected unit struct, unit variant or constant, found tuple variant `E::S` | ||
//~| HELP use the tuple variant pattern syntax instead | ||
} | ||
|
||
match Point4(0, 1, 2, 3) { | ||
Point4( a , _ ) => {} | ||
//~^ ERROR this pattern has 2 fields, but the corresponding tuple struct has 4 fields | ||
//~| HELP use `_` to explicitly ignore each field | ||
//~| HELP use `..` to ignore the rest of the fields | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.