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

fix: Support nested type on replace if let with match #10491

Merged
merged 7 commits into from
Oct 14, 2021
Merged

fix: Support nested type on replace if let with match #10491

merged 7 commits into from
Oct 14, 2021

Conversation

k-nasa
Copy link
Contributor

@k-nasa k-nasa commented Oct 9, 2021

Why

close: #8690

Now, Replacing if-let with match cant't output exhaustive patterns code.
This was because the else conversion used specific types (ex. Option, Result) instead of wildcards.

I thought it was more of a problem to generate non-exhaustive patterns than the benefits of using the concrete one.

How about using wildcards in else?
Is this change policy acceptable?

What

  • using wildcards on make_else_arm
  • Change test cases

@k-nasa k-nasa changed the title K nasa/support nested type on replace if let with match Support nested type on replace if let with match Oct 9, 2021
@Veykril
Copy link
Member

Veykril commented Oct 10, 2021

Ideally we will be able to generate exhaustive patterns for any patterns properly(the compilers exhaustiveness checks are currently being lifted out into its own crate which would allow us to properly do this soon), so I do prefer having these special cases in place for now.
We should be able to fix this particular problem heuristically though by looking at the pattern in the then arm, if it looks non-exhaustive(it contains a slice pattern for example) then we should emit a wildcard pattern in the else arm instead.

@k-nasa
Copy link
Contributor Author

k-nasa commented Oct 13, 2021 •
edited
Loading

o I do prefer having these special cases in place for now.

Hmm. Currently I thought it is a good solution.

it contains a slice pattern for example

I don't know how to do this, so please let me know if already have this feature 🙏

Looking at this seems to be a solution.
https://github.com/rust-analyzer/rust-analyzer/blob/137ac67f5dd10d8a5e83e9eeb7600993e9886c8a/crates/syntax/src/ast/generated/nodes.rs#L1537

@k-nasa
Copy link
Contributor Author

k-nasa commented Oct 13, 2021

I made a simple implementation. I hope you will see it! @Veykril

@Veykril
Copy link
Member

Veykril commented Oct 14, 2021

Ye checking whether a pattern is deeper than one level should be good enough for our usecase here for the time being 👍
bors d+

@bors
Copy link
Contributor

bors bot commented Oct 14, 2021

✌️ k-nasa can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@k-nasa
Copy link
Contributor Author

k-nasa commented Oct 14, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 14, 2021

Build succeeded:

@bors bors bot merged commit 3a79af7 into rust-lang:master Oct 14, 2021
@k-nasa k-nasa deleted the k-nasa/support_nested_type_on_replace_if_let_with_match branch October 14, 2021 22:48
@Veykril Veykril changed the title Support nested type on replace if let with match fix: Support nested type on replace if let with match Oct 15, 2021
@Veykril Veykril mentioned this pull request Oct 19, 2021
Closed
@profetia profetia mentioned this pull request Dec 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replacing if-let with match can lead to match expression with non-exhaustive patterns
2 participants
@k-nasa @Veykril