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

Suggest _ and .. if a pattern has too few fields #80017

Merged
merged 9 commits into from
Jan 14, 2021
57 changes: 52 additions & 5 deletions compiler/rustc_typeck/src/check/pat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);`.
Expand All @@ -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(());
// | ^ ^
Expand All @@ -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
camelid marked this conversation as resolved.
Show resolved Hide resolved
// |
// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the DUMMY_SP for and this branch for? I never seen this before.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to prevent a compiler crash if pat_span == DUMMY_SP since we then subtract 1 from pat_span and DUMMY_SP is basically equal to 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

If pat_span is DUMMY_SP then you shouldn't be giving suggestions at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

What happens if the error rendering system receives a diagnostic that has DUMMY_SP? Will it just ignore it?

Copy link
Contributor

Choose a reason for hiding this comment

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

The error renderer will ignore DUMMY_SPs and not show labels associated to it, while still showing messages, but they do have the potential to break suggestions and certainly break everything when synthesizing a new span from them.

} 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(", ");
Copy link
Contributor

Choose a reason for hiding this comment

The 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
let mut wildcard_sugg = vec!["_"; fields.len() - subpats.len()].join(", ");
// FIXME: heuristic-based suggestion to check current types for where to add `_`.
let mut wildcard_sugg = vec!["_"; fields.len() - subpats.len()].join(", ");

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// or the pattern consists of all wildcards.
// or the pattern consists of all wildcards `(_, _)`.

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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

These two cases should also be suggesting .. as an alternative, no?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 ..? On the other hand, someone may not know that .. is an option and would rather that over listing out wildcards manually. So I think overall I agree with you that we should be giving it as an alternative :)

I'll change it, but for other things we can always adjust the behavior later; I'm hoping to have this PR finished soon.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

So is the behavior you're suggesting that we also suggest S(..) if it's currently of the form S(_, _, _)?

Copy link
Contributor

@estebank estebank Jan 13, 2021

Choose a reason for hiding this comment

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

Yes, I think that if it is currently S(_, _) and we suggest S(_, _, _) we should also suggest S(..). Having said that, if you find that it will be too much work, only create a follow-up ticket and r=me.

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
Expand All @@ -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
Expand Down
9 changes: 7 additions & 2 deletions src/test/ui/error-codes/E0023.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ LL | Apple(String, String),
...
LL | Fruit::Apple(a) => {},
| ^^^^^^^^^^^^^^^ expected 2 fields, found 1
|
help: use `_` to explicitly ignore each field
|
LL | Fruit::Apple(a, _) => {},
| ^^^

error[E0023]: this pattern has 3 fields, but the corresponding tuple variant has 2 fields
--> $DIR/E0023.rs:12:9
Expand Down Expand Up @@ -34,7 +39,7 @@ LL | Orange((String, String)),
LL | Fruit::Orange(a, b) => {},
| ^^^^^^^^^^^^^^^^^^^ expected 1 field, found 2
|
help: missing parenthesis
help: missing parentheses
|
LL | Fruit::Orange((a, b)) => {},
| ^ ^
Expand All @@ -48,7 +53,7 @@ LL | Banana(()),
LL | Fruit::Banana() => {},
| ^^^^^^^^^^^^^^^ expected 1 field, found 0
|
help: missing parenthesis
help: missing parentheses
|
LL | Fruit::Banana(()) => {},
| ^ ^
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 _) and _only suggest _. To clarify, I think that in this case we should only provide the first suggestion, not the .. one. That being said, it is something I can look at myself if you're itching to merge asap.

It also feels like we could deal with language for a single field. My ideal output for this case would be:

help: use `_` to explicitly ignore the missing field
   |
LL |     let P(_) = U {};
   |           ^

Copy link
Member Author

@camelid camelid Jan 13, 2021

Choose a reason for hiding this comment

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

To clarify, I think that in this case we should only provide the first suggestion, not the .. one.

I actually think we should suggest .. in this case. It's very possible that someone didn't list any fields because they want to match on the outer structure, and so they just want to ignore all of them. We should definitely suggest .. in that case.

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

Expand Down
5 changes: 5 additions & 0 deletions src/test/ui/issues/issue-72574-2.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ LL | struct Binder(i32, i32, i32);
...
LL | Binder(_a, _x @ ..) => {}
| ^^^^^^^^^^^^^^^^^^^ expected 3 fields, found 2
|
help: use `_` to explicitly ignore each field
|
LL | Binder(_a, _x @ .., _) => {}
| ^^^

error: aborting due to 3 previous errors

Expand Down
9 changes: 9 additions & 0 deletions src/test/ui/match/match-pattern-field-mismatch.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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(..) => { }
Copy link
Contributor

@pickfire pickfire Jan 13, 2021

Choose a reason for hiding this comment

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

Why don't we suggest Color::Rgb(_, _, ..) as well? I think the above is enough but we could show the other possible options.

Copy link
Contributor

@estebank estebank Jan 13, 2021

Choose a reason for hiding this comment

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

I consider Color::Rgb(_, _, ..) is bad style. :)

Edit: let me clarify :)

Rgb(_, _, _) is useful because you're saying "I care if Rgb changes and adds fields", while Rgb(..) is saying "I don't care about the values, I just care about the tagged variant". Rgb(_, _, ..) seems to be saying "I only care to change this pattern if the variant has less than 3 fields", which seems very special cased to me.

| ^^

error: aborting due to previous error

Expand Down
5 changes: 5 additions & 0 deletions src/test/ui/pattern/issue-74539.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ LL | A(u8, u8),
...
LL | E::A(x @ ..) => {
| ^^^^^^^^^^^^ expected 2 fields, found 1
|
help: use `_` to explicitly ignore each field
|
LL | E::A(x @ .., _) => {
| ^^^

error: aborting due to 3 previous errors

Expand Down
55 changes: 55 additions & 0 deletions src/test/ui/pattern/pat-tuple-underfield.rs
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
}
}
Loading