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.

Sign up for GitHub

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

Jump to bottom

Migrate rustc_mir_build's non_exhaustive_match diagnostic #108278

Closed
wants to merge 11 commits into from
Closed

Migrate rustc_mir_build's non_exhaustive_match diagnostic #108278

wants to merge 11 commits into from

Conversation

mejrs
Copy link
Contributor

@mejrs mejrs commented Feb 20, 2023 •
edited
Loading

This also changes the diagnostic somewhat, I hope that's OK

@rustbot
Copy link
Collaborator

rustbot commented Feb 20, 2023

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 20, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 20, 2023

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@compiler-errors compiler-errors self-assigned this Feb 21, 2023
@compiler-errors
Copy link
Member

compiler-errors commented Feb 21, 2023 •
edited
Loading

I'm a fan of the primary message being tweaked along these lines -- but could you perhaps split that out into a separate PR that edits the message without doing the diagnostics migration (i.e. using the old diagnostics machinery), so others can chime in on the wording and presentation?

Then the diff of this PR would be much smaller, so it's easier for a reviewer to notice more subtle changes due to the refactoring you're doing here?

@mejrs
Copy link
Contributor Author

mejrs commented Feb 21, 2023

I realized this PR would be hard to follow, which is why I did this change in multiple commits. Each of those makes sense on its own, which I hope makes the reviewing easier.

@compiler-errors
Copy link
Member

Well it still needs fixing regardless -- was just trying to make it possible to get some of these changes (at least their presentation) landed before you were able to debug the rest of it.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 21, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@est31
Copy link
Member

est31 commented Feb 22, 2023

FTR this is what rust-analyzer prints. Like the current error it's straight to the point, which I prefer:

Screenshot_20230222_171006

From what I can tell though the rust-analyzer error is native to them though, so this PR won't disrupt that IDE error.

I think if the set of patterns not handled becomes very complex then it makes sense to switch to the suggested error scheme and only explain the missing patterns in the report below, but otherwise I prefer the older way where the missing patterns are right at the top.

est31
est31 reviewed Feb 22, 2023
View reviewed changes
tests/ui/closures/2229_closure_analysis/match/issue-88331.stderr Outdated
|
LL ~ Opcode::OP1 => unimplemented!(),
LL ~ Opcode(0_u8) | Opcode(2_u8..=u8::MAX) => todo!(),
LL + Opcode(0_u8) => { todo!() },
Copy link
Member

Choose a reason for hiding this comment

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

Note that while this is also my personal preference, to put a , after blocks in match arms, rustfmt's defaults are to omit it.

Copy link
Member

@compiler-errors compiler-errors Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think this should be reverted. If you want to make this change separately, break it out into another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted the commas.

@bors
Copy link
Contributor

bors commented Feb 22, 2023

☔ The latest upstream changes (presumably #103042) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cjgillot cjgillot removed their assignment Feb 23, 2023
@bors
Copy link
Contributor

bors commented Mar 2, 2023

☔ The latest upstream changes (presumably #108640) made this pull request unmergeable. Please resolve the merge conflicts.

@mejrs
Copy link
Contributor Author

mejrs commented Mar 30, 2023

oops i think i forgot this:

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 30, 2023
@bors
Copy link
Contributor

bors commented Apr 6, 2023

☔ The latest upstream changes (presumably #108504) made this pull request unmergeable. Please resolve the merge conflicts.

@compiler-errors
Copy link
Member

@rustbot author -- needs a rebase

feel free to ping me when this is ready for a review, sorry for just seeing this now

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 9, 2023
@JohnCSimon
Copy link
Member

JohnCSimon commented May 28, 2023 •
edited
Loading

Ping from triage:
@mejrs can you please address the merge conflicts, rebase etc?

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

@mejrs
Copy link
Contributor Author

mejrs commented Jun 3, 2023

I will, my apologies for the delay :)

@JohnCSimon
Copy link
Member

@mejrs
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Jul 30, 2023
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jul 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@est31 est31 est31 left review comments

@compiler-errors compiler-errors compiler-errors left review comments

Assignees

@compiler-errors compiler-errors

Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@mejrs @rustbot @rust-log-analyzer @compiler-errors @est31 @bors @JohnCSimon @cjgillot