single_match: Lint structs and tuple structs #8395

jubnzv · 2022-02-05T08:41:20Z

These changes allow single_match lint to suggest the simplification of match constructions for structs and tuple structs with the same name, if they form an exhaustive match.

For example:

// lint: S(_, _) forms an exhaustive match, so it could be removed
match s {
    S(42, _a) => {},
    S(_, _) => {},
}

Context: #8322 (comment)

changelog: [single_match]: Lint tuple structs with the same name

These changes allow `single_match` lint to suggest the simplification of `match` constructions for structs with the same name that forms an exhaustive match. For example: ```rust // lint: S(_, _) forms an exhaustive match, so it could be removed match s { S(42, _a) => {}, S(_, _) => {}, } ``` See: rust-lang#8322 (comment)

rust-highfive · 2022-02-05T08:41:23Z

r? @camsteffen

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

camsteffen

I reviewed about 75% so far.

clippy_lints/src/matches.rs

camsteffen · 2022-02-06T00:12:44Z

clippy_lints/src/matches.rs

- });
- acc.push((path, ty));
- true
+ (PatKind::Binding(BindingAnnotation::Unannotated, .., ident, None), _) if contains_only_wilds(right) => {


The BindingAnnotation shouldn't matter here.

Suggested change

(PatKind::Binding(BindingAnnotation::Unannotated, .., ident, None), _) if contains_only_wilds(right) => {

(PatKind::Binding(.., ident, None), _) if contains_only_wilds(right) => {

Also, I think you can treat Binding the same as Wild?

Thirdly, I think we should handle the case with Some at the end. You could do this at the top of this function:

fn peel_subpatterns(pat) { while let PatKind::Binding(.., Some(sub)) = pat.kind { pat = sub; } pat } let left = peel_subpatterns(left); let right = peel_subpatterns(right);

I agree, I added support for bindings as you suggest.

But I have some problems with test suite after the changes.
The error states: '[ui] ui/patterns.rs' panicked at 'failed to apply suggestions for "tests/ui/patterns.rs" with rustfix: Cannot replace slice of data that was already replaced.

As far as I understand, the issue occurs when the test suite tries to apply rustfix on patterns.rs to check if it matches patterns.fixed. According to the stack trace, the problem is in rustfix library, and it is not related with clippy code. I found a few similar issues [1] [2] in clippy and rustfix repositories, but it didn't help me much.

So at the moment I'm not sure how to fix it and I need some time to figure it out. Any help would be helpful.

That error means that rustfix applied your suggestion, reran the lint on the updated code, and then got another suggestion for the same code. Like an infinite loop. Try manually applying the suggestions and then see if you get another suggestion in the same code.

Thank you. I figured it out.

The problem was that after my changes clippy suggests to replace these lines:

match v { Some(x) => (), y @ _ => (), }

To these:

if let Some(x) = x = { () }

But this code itself could be simplified to:

if let Some(x) = x = {}

So we got an infinite loop.

I fixed this file to keep its original logic and not trigger single_match lint.

Hmm but I think the first example should trigger single_match. Which lint fired after the suggestion was applied?

Hmm but I think the first example should trigger single_match

Yes, it seems logical. But for some reasons we have the following check for the els branch. And such cases are explicitly rejected.

According to git blame this check was added here. There are no tests or additional context there, so I'm not sure, what problem this solves. Probably, it is better to leave this logic as is in this MR and come back to it later?

Which lint fired after the suggestion was applied?

First, clippy suggests to replace y @ _ to y with redundant_pattern check. Then we get code that can be simplified with single_match check, and we try to execute rustfix again to fix it.

Yes, it seems logical. But for some reasons we have the following check for the els branch. And such cases are explicitly rejected.

That check is there because we only want to lint if the second arm is .. => () (or {}), so I that shouldn't be a problem for y @ _ => (),?

That check is there because we only want to lint if the second arm is .. => () (or {}), so I that shouldn't be a problem for y @ _ => (),?

Yes, and in that case we'll get the recursion error in rustfix.

clippy_lints/src/matches.rs

camelid · 2022-02-06T21:51:17Z

I'm not sure what you mean by "with the same name". Any other name would result in a type error before clippy ran, right?

jubnzv · 2022-02-07T13:59:39Z

I'm not sure what you mean by "with the same name". Any other name would result in a type error before clippy ran, right?

Yes, that would be a compile error.

bors · 2022-02-07T20:30:02Z

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

…names

clippy_lints/src/matches/single_match.rs

camsteffen · 2022-02-17T02:27:50Z

clippy_lints/src/matches/single_match.rs

- }
- true
- },
+fn contains_single_binding(pat: &Pat<'_>) -> bool {


I don't think this function is needed since wilds and bindings are effectively the same.

We need to handle wildcards only when processing an els branch, because we cannot bind identifier to it.

For example, we don't want to lint the following case:

match s { a => (), a => (), // don't lint, we cannot bind `a` to the `else` branch }

I think we can lint that case. The binding will be unused in the second branch.

It doesn't work in such cases. We don't actually know, which branch will be if and which will be els, so after this change we'll get a lot of false positives.

I think, this out of scope of this PR, because it aims just to add support for Struct and TupleStruct patterns.

We could improve precision of this lint in different PR, right?

clippy_lints/src/matches/single_match.rs

camsteffen · 2022-02-17T02:37:46Z

clippy_lints/src/matches.rs

- });
- acc.push((path, ty));
- true
+ (PatKind::Binding(BindingAnnotation::Unannotated, .., ident, None), _) if contains_only_wilds(right) => {


Yes, it seems logical. But for some reasons we have the following check for the els branch. And such cases are explicitly rejected.

That check is there because we only want to lint if the second arm is .. => () (or {}), so I that shouldn't be a problem for y @ _ => (),?

camsteffen · 2022-02-17T02:40:02Z

clippy_lints/src/matches/single_match.rs

+ let left = peel_subpatterns(left);
+ let right = peel_subpatterns(right);
+ match (&left.kind, &right.kind) {
+ (PatKind::Wild, _) | (_, PatKind::Wild) => true,


Wouldn't this work?

Suggested change

(PatKind::Wild, _) | (_, PatKind::Wild) => true,

(PatKind::Wild | PatKind::Binding(..), _) | (_, PatKind::Wild | PatKind::Binding(..)) => true,

No, because we should lint bindings iff the else branch (right pattern) contains wildcards only.

bors · 2022-02-24T19:00:08Z

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

jubnzv · 2022-02-27T03:49:21Z

@camsteffen friendly ping.
Could you please take a look at latest comments when you have time? I'm not sure we want to tweak any branch processing logic in the PR related to adding Struct and TupleStruct types.

jubnzv · 2022-04-06T03:26:12Z

Closed, since there is no response from maintainers.
Feel free to pick it, this PR is ready to merge.

flip1995 · 2022-04-06T07:58:56Z

Sorry about not getting back to this. We're currently a bit overloaded with PRs and some maintainers (including me) are really busy with their $day_job. Thanks for working on this. I marked it as inactive-closed, so we don't lose track of it and can come back to it once we have a bit more capacity.

jubnzv · 2022-04-06T08:22:23Z

@flip1995 No problem, I understand. I don't have time for any side projects right now either, so I prefer to close it.

rust-highfive assigned camsteffen Feb 5, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 5, 2022

jubnzv mentioned this pull request Feb 5, 2022

single_match: Don't lint non-exhaustive matches; support tuples #8322

Merged

jubnzv changed the title ~~single_match: Lint structs with the same name~~ single_match: Lint tuple structs with the same name Feb 5, 2022

camsteffen suggested changes Feb 6, 2022

View reviewed changes

jubnzv added 4 commits February 6, 2022 11:51

matches: Simplify code; get rid of known_enums module

1907ff5

matches: Clarify the comment. NFC.

22a44b7

matches: Fix clippy warning

2d1546b

matches: Allow lint for Bindings w/ annotations

cd3857a

jubnzv force-pushed the single-match-structs branch from c5cc7d9 to b9dce52 Compare February 6, 2022 11:46

matches: Peel subpatterns in bindings

4873ca1

jubnzv force-pushed the single-match-structs branch from b9dce52 to 4873ca1 Compare February 6, 2022 12:19

matches: Clarify comment

5fc50fe

matches: Support structs

3161389

jubnzv changed the title ~~single_match: Lint tuple structs with the same name~~ single_match: Lint structs and tuple structs Feb 8, 2022

jubnzv added 10 commits February 8, 2022 07:05

Merge remote-tracking branch 'origin/master' into single-match-structs

49cb789

single_match: Update test

159037a

single_match: Simplify code

060957a

single_match: Add test to check we don't lint structs with different …

7fc3545

…names

single_match: Fix naming. NFC.

b6695b0

single_match: Simplify code

cd89b1f

Add a workaround for the rustfix crash

791a74e

single_match: Lint Struct patterns with the same type

5104517

single_match: Don't lint TupleStructs with the same name

4fae6c0

single_match: Improve tests

6686c72

camsteffen reviewed Feb 17, 2022

View reviewed changes

camsteffen suggested changes Feb 17, 2022

View reviewed changes

jubnzv added 2 commits February 17, 2022 07:13

single_match: Handle structs with known enums

cc55a8c

single_match: Simplify code

635bb4d

Merge remote-tracking branch 'origin/master' into single-match-structs

d429906

jubnzv closed this Apr 6, 2022

flip1995 added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Apr 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

single_match: Lint structs and tuple structs #8395

single_match: Lint structs and tuple structs #8395

jubnzv commented Feb 5, 2022 •

edited

Loading

rust-highfive commented Feb 5, 2022

camsteffen left a comment

camsteffen Feb 6, 2022

jubnzv Feb 8, 2022 •

edited

Loading

camsteffen Feb 8, 2022

jubnzv Feb 9, 2022 •

edited

Loading

camsteffen Feb 9, 2022

jubnzv Feb 10, 2022 •

edited

Loading

camsteffen Feb 17, 2022

jubnzv Feb 17, 2022

camelid commented Feb 6, 2022 •

edited

Loading

jubnzv commented Feb 7, 2022

bors commented Feb 7, 2022

camsteffen Feb 17, 2022

jubnzv Feb 17, 2022

camsteffen Feb 17, 2022

jubnzv Feb 19, 2022 •

edited

Loading

camsteffen Feb 17, 2022

camsteffen Feb 17, 2022

jubnzv Feb 17, 2022

bors commented Feb 24, 2022

jubnzv commented Feb 27, 2022

jubnzv commented Apr 6, 2022

flip1995 commented Apr 6, 2022

jubnzv commented Apr 6, 2022

	(PatKind::Binding(BindingAnnotation::Unannotated, .., ident, None), _) if contains_only_wilds(right) => {
	(PatKind::Binding(.., ident, None), _) if contains_only_wilds(right) => {

	(PatKind::Wild, _) \| (_, PatKind::Wild) => true,
	(PatKind::Wild \| PatKind::Binding(..), _) \| (_, PatKind::Wild \| PatKind::Binding(..)) => true,

single_match: Lint structs and tuple structs #8395

single_match: Lint structs and tuple structs #8395

Conversation

jubnzv commented Feb 5, 2022 • edited Loading

rust-highfive commented Feb 5, 2022

camsteffen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubnzv Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubnzv Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubnzv Feb 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelid commented Feb 6, 2022 • edited Loading

jubnzv commented Feb 7, 2022

bors commented Feb 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubnzv Feb 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 24, 2022

jubnzv commented Feb 27, 2022

jubnzv commented Apr 6, 2022

flip1995 commented Apr 6, 2022

jubnzv commented Apr 6, 2022

jubnzv commented Feb 5, 2022 •

edited

Loading

jubnzv Feb 8, 2022 •

edited

Loading

jubnzv Feb 9, 2022 •

edited

Loading

jubnzv Feb 10, 2022 •

edited

Loading

camelid commented Feb 6, 2022 •

edited

Loading

jubnzv Feb 19, 2022 •

edited

Loading