Skip to content

manual_let_else: support struct patterns #10866

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

Merged
merged 3 commits into from
Jun 3, 2023

Conversation

est31
Copy link
Member

@est31 est31 commented Jun 1, 2023

This adds upon the improvements of #10797 and:

  • Only prints () around Or patterns at the top level (fixing a regression of Improve pattern printing for manual_let_else #10797)
  • Supports multi-binding patterns: let (u, v) = if let (Some(u_i), Ok(v_i)) = ex { (u_i, v_i) } else ...
  • Traverses through tuple patterns: let v = if let (Some(v), None) = ex { v } else ...
  • Supports struct patterns: let v = if let S { v, w, } = ex { (v, w) } else ...
changelog: [`manual_let_else`]: improve pattern printing to support struct patterns

fixes #10708
fixes #10424

At the top level, () are required, but on the levels below they are not.
@rustbot
Copy link
Collaborator

rustbot commented Jun 1, 2023

r? @Manishearth

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 1, 2023
@Manishearth
Copy link
Member

@Centri3 this is an interesting PR; would you like to review it as well? No worries if not.

(trying to point contributors at interesting PRs to review to get more folks who know how to review things)

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Mostly looks good. I want to have a closer look later, and perhaps wait for @Centri3's review if she is interested.


// There is some length mismatch, which indicates usage of .. in the patterns above e.g.:
// let (a, ..) = if let [a, b, _c] = ex { (a, b) } else { ... };
// For now, bail in these cases.
Copy link
Member

Choose a reason for hiding this comment

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

thought: seems reasonable; i can't think of any big reason to do this beyond WIP code.

pat_bindings.insert(ident);
});
if pat_bindings.len() < paths.len() {
return false;
// This rebinds some bindings from the outer scope, or it repeats some copy-able bindings multiple
// times. We don't support these cases so we bail here. E.g.:
Copy link
Member

Choose a reason for hiding this comment

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

thought: probably possible to split into two lets, probably not worth it.

(Might be interesting to design a different lint that detects when let (a, b) = ... is splittable)

@est31 est31 force-pushed the manual_let_else_pattern branch 3 times, most recently from 6211a3a to 71140d8 Compare June 2, 2023 11:54
@est31 est31 force-pushed the manual_let_else_pattern branch from 71140d8 to f538402 Compare June 2, 2023 12:46
@Centri3
Copy link
Member

Centri3 commented Jun 2, 2023

Looks good overall, I have nothing to add past what was commented on already

@Manishearth
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 3, 2023

📌 Commit f538402 has been approved by Manishearth

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jun 3, 2023

⌛ Testing commit f538402 with merge 2490de4...

@bors
Copy link
Contributor

bors commented Jun 3, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Manishearth
Pushing 2490de4 to master...

@bors bors merged commit 2490de4 into rust-lang:master Jun 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
5 participants