-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Conversation
r? @estebank (rust-highfive has picked a reviewer for you, use r? to override) |
src/test/ui/destructuring-assignment/tuple_struct_destructure_fail.stderr
Outdated
Show resolved
Hide resolved
..
pattern suggestion_
and ..
patterns when a pattern has too few fields
_
and ..
patterns when a pattern has too few fields_
and ..
if a pattern has too few fields
64b3db4
to
eb66b98
Compare
let after_fields_span = if pat_span == DUMMY_SP { | ||
pat_span |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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_SP
s 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.
src/test/ui/issues/issue-67037-pat-tup-scrut-ty-diff-less-fields.stderr
Outdated
Show resolved
Hide resolved
src/test/ui/destructuring-assignment/tuple_struct_destructure_fail.stderr
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
Sorry for the delay in reviewing. I left a few nitpicks that require changing the logic slightly. Would you mind incorporating those changes and pinging me once that's done? |
That's okay; it seems most of the diagnostics PRs get assigned to you :)
👍 |
@estebank here's your ping! I didn't make one of the changes because of #80017 (comment). |
help: use `..` to ignore the rest of the fields | ||
| | ||
LL | Point4( a , _ , ..) => {} | ||
| ^^^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this one be the following?
help: use `..` to ignore the rest of the fields
|
LL | Point4( a , ..) => {}
| ^^^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally yes, but I think that would make the code much more complicated. This is the current behavior:
- If it's of the form
(_, _)
then suggest(..)
- Otherwise, suggest
(/* ... */, ..)
Of course, there's different behavior for the case when only one field is missing :)
For example, this code: struct S(i32, f32); let S(x) = S(0, 1.0); will make the compiler suggest either: let S(x, _) = S(0, 1.0); or: let S(x, ..) = S(0, 1.0);
Co-authored-by: Esteban Kuber <estebank@users.noreply.github.com>
It's 'parentheses', not 'parenthesis', when you have more than one.
4e7a271
to
d7307a7
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
ad6b7d8
to
e8c8793
Compare
| ^^^ | ||
help: use `..` to ignore all fields | ||
| | ||
LL | Color::Rgb(..) => { } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
); | ||
|
||
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
// 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.
help: use `_` to explicitly ignore each field | ||
| | ||
LL | let P(_) = U {}; | ||
| ^ | ||
help: use `..` to ignore all fields | ||
| | ||
LL | let P(..) = U {}; | ||
| ^^ |
There was a problem hiding this comment.
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 {};
| ^
There was a problem hiding this comment.
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 :)
👍 @bors r+ |
📌 Commit e8c8793 has been approved by |
Suggest `_` and `..` if a pattern has too few fields Fixes rust-lang#80010.
Rollup of 17 pull requests Successful merges: - rust-lang#79982 (Add missing methods to unix ExitStatusExt) - rust-lang#80017 (Suggest `_` and `..` if a pattern has too few fields) - rust-lang#80169 (Recommend panic::resume_unwind instead of panicking.) - rust-lang#80217 (Add a `std::io::read_to_string` function) - rust-lang#80444 (Add as_ref and as_mut methods for Bound) - rust-lang#80567 (Add Iterator::intersperse_with) - rust-lang#80829 (Get rid of `DepConstructor`) - rust-lang#80895 (Fix handling of malicious Readers in read_to_end) - rust-lang#80966 (Deprecate atomic::spin_loop_hint in favour of hint::spin_loop) - rust-lang#80969 (Use better ICE message when no MIR is available) - rust-lang#80972 (Remove unstable deprecated Vec::remove_item) - rust-lang#80973 (Update books) - rust-lang#80980 (Fixed incorrect doc comment) - rust-lang#80981 (Fix -Cpasses=list and llvm version print with -vV) - rust-lang#80985 (Fix stabilisation version of slice_strip) - rust-lang#80990 (llvm: Remove the unused context from CreateDebugLocation) - rust-lang#80991 (Fix formatting specifiers doc links) Failed merges: - rust-lang#80944 (Use Option::map_or instead of `.map(..).unwrap_or(..)`) r? `@ghost` `@rustbot` modify labels: rollup
Account for existing `_` field pattern when suggesting `..` Follow up to rust-lang#80017.
Fixes #80010.