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 [use_self] false negative with on struct and tuple struct patterns #8899

Merged
merged 1 commit into from
May 30, 2022

Conversation

botahamec
Copy link
Contributor

fixes #8845

changelog: Triggered the warning for [`use_self`] on TupleStruct and Struct patterns, whereas currently it's only triggered for Path patterns

@rust-highfive
Copy link

r? @xFrednet

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 26, 2022
@botahamec botahamec marked this pull request as draft May 26, 2022 17:58
@botahamec botahamec marked this pull request as ready for review May 26, 2022 18:17
@xFrednet
Copy link
Member

r? @Alexendoo (It would be cool, if you could leave a comment so that I can assign this PR to you :))

@Alexendoo
Copy link
Member

👋

Comment on lines 560 to 585
impl Something {
const fn get_value(&self) -> u8 {
match self {
Something::Num(n) => *n,
Something::TupleNums(n, _m) => *n,
Something::StructNums { one, two: _ } => *one,
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Using the variants directly trigger the lint, suggesting to replace them all with Self:

fn imported_variants(&self) -> u8 {
    use Something::*;

    match self {
        Num(n) => *n,
        TupleNums(n, _m) => *n,
        StructNums { one, two: _ } => *one,
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

Seems that's uncovering an existing bug, the first path segment is picked unconditionally

enum Enum {
    A,
}

impl Enum {
    fn f(self) -> usize {
        match self {
            crate::Enum::A => 1,
        }
    }
}
warning: unnecessary structure name repetition
  --> t.rs:13:13
   |
13 |             crate::Enum::A => 1,
   |             ^^^^^ help: use the applicable keyword: `Self`
   |

You could check the res field of the PathSegments to pick the right one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was tricky. Using the res field on the PathSegments doesn't seem to work. In the playground, I found that most of the res fields had errors, so I ended up using a workaround similar to what's done in the check_expr method. Luckily, the lint_path_to_variant function already exists. But, it's a bit hacky, and I did find that the name of the enum is correctly shown in the res fields. I made a version of the function which uses the res field, and is a bit less hacky, but it's also a bit less performant. I doubt anyone would notice the difference, but I left it unchanged for now.

@Alexendoo
Copy link
Member

Thanks! Could you also squash the commits and then I think it'll be good to go

@botahamec botahamec force-pushed the use-self-tuple-struct-variants branch from e5d697e to 2aa4569 Compare May 30, 2022 17:41
@botahamec
Copy link
Contributor Author

@Alexendoo Should be good now

@xFrednet xFrednet assigned Alexendoo and unassigned Alexendoo and xFrednet May 30, 2022
@xFrednet
Copy link
Member

@Alexendoo when you're happy with this PR you can r+ it directly. And we no longer need a delegate for that 🥳

@Alexendoo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented May 30, 2022

@Alexendoo: 🔑 Insufficient privileges: Not in reviewers

@Alexendoo
Copy link
Member

Well then. Guess the teams PR needs to happen for that yet 😄

@xFrednet
Copy link
Member

Talking in bors voice

Thank you for the approval

@bors r=Alexendoo

@bors
Copy link
Contributor

bors commented May 30, 2022

📌 Commit 2aa4569 has been approved by Alexendoo

@bors
Copy link
Contributor

bors commented May 30, 2022

⌛ Testing commit 2aa4569 with merge d9f4978...

@bors
Copy link
Contributor

bors commented May 30, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing d9f4978 to master...

@bors bors merged commit d9f4978 into rust-lang:master May 30, 2022
@botahamec botahamec deleted the use-self-tuple-struct-variants branch May 30, 2022 23:46
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.

use_self false negative on tuple or struct enum variants
5 participants