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

fix: unnested_or_patterns suggests wrongly in let #14401

Merged
merged 1 commit into from
Mar 29, 2025

Conversation

profetia
Copy link
Contributor

Closes #9952

changelog: [unnested_or_patterns] fix wrong suggestion in let

@rustbot
Copy link
Collaborator

rustbot commented Mar 13, 2025

r? @y21

rustbot has assigned @y21.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 13, 2025
Copy link
Contributor

@samueltardieu samueltardieu left a comment

Choose a reason for hiding this comment

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

This is one way to fix it up, but there might be a simpler one. By fixing the visitor in insert_necessary_parens() to not remove parens (if any) on the outermost pattern, you get the same result without having to introduce PatternSource. This could be something like (with minimal diffs to the existing visitor):

fn remove_all_parens(pat: &mut P<Pat>) {
    #[derive(Default)]
    struct Visitor {
        is_inner: bool,
    }
    impl MutVisitor for Visitor {
        fn visit_pat(&mut self, pat: &mut P<Pat>) {
            let is_inner = mem::replace(&mut self.is_inner, true);
            walk_pat(self, pat);
            pat.kind = match &mut pat.kind {
                Paren(i) if is_inner => mem::replace(&mut i.kind, Wild),
                _ => return,
            };
        }
    }
    Visitor::default().visit_pat(pat);
}

The only difference in output with what you are proposing is that this wouldn't "fix" the outermost parens if the user chose to use them. For example, this would transform:

if let (0 | (1 | 2)) = …

into

if let (0 | 1 | 2) = …

whereas your change also drops the outermost parens. Even if dropping them is correct, this is not what the lint advertises, and this might be considered redundant with rustc's unused_parens lint.

@profetia
Copy link
Contributor Author

profetia commented Mar 14, 2025

Good advice! Thanks!

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Thanks, just a NIT.

Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

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

Also looks good to me, just one small suggestion

@profetia profetia requested a review from y21 March 29, 2025 12:53
@y21 y21 added this pull request to the merge queue Mar 29, 2025
Merged via the queue into rust-lang:master with commit 4c610e3 Mar 29, 2025
11 checks passed
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
Development

Successfully merging this pull request may close these issues.

unnested_or_patterns removes parens: top-level or-patterns are not allowed in let bindings
5 participants