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

jubnzv · 2022-01-20T14:30:04Z

single_match lint:

Don't lint exhaustive enum patterns without a wild.
Rationale: The definition of the enum could be changed, so the user can get non-exhaustive match after applying the suggested lint (see single_match should not lint exhaustive match #8282 (comment) for context).
Lint match constructions with tuples (as suggested at single_match should not lint exhaustive match #8282 (comment))

changelog: [single_match]: Don't lint exhaustive enum patterns without a wild.
changelog: [single_match]: Lint match constructions with tuples

This commit changes the behavior of `single_match` lint. After that, we won't lint non-exhaustive matches like this: ```rust match Some(v) { Some(a) => println!("${:?}", a), None => {}, } ``` The rationale is that, because the type of `a` could be changed, so the user can get non-exhaustive match after applying the suggested lint (see rust-lang#8282 (comment) for context). We also will lint `match` constructions with tuples. When we see the tuples on the both arms, we will check them both at the same time, and if they form exhaustive match, we could display the warning. Closes rust-lang#8282

rust-highfive · 2022-01-20T14:30:07Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon.

Please see the contribution instructions for more information.

jubnzv · 2022-01-20T14:34:43Z

This is not finished yet, because I need some time to restore logic that calls match_types in the condition. This is required to handle cases, when the user has aliases named as standard structures (Err, Ok, etc.), I'd like to write tests for this.

I'm publishing this PR now to discuss overall design of changes, because there was some discussion in #8282. I'm open to ideas if there are better suggestions to implementing this.

llogiq

I commented where I found shorter ways or was befuddled with the implementation. In general, this should be mostly merge-worthy already apart from the nitpick comments I added.

clippy_lints/src/matches.rs

llogiq · 2022-01-20T20:14:12Z

clippy_lints/src/matches.rs

+    match (&left.kind, &right.kind) {
+        (PatKind::Wild, _) | (_, PatKind::Wild) => true,
+        (PatKind::Tuple(left_in, left_pos), PatKind::Tuple(right_in, right_pos)) => {
+            // We don't actually know the position and presence of the `..` (dotdot) operator in


This puzzles me. Why don't we know at which argument #no is the ..? And why do we need the span for this? It seems like a very roundabout way to implement this.

The point is that we don't know the actual number of the patterns replaced by ...

For example:

match (Some(E::V), Some(E::V), Some(E::V), Some(E::V)) { (Some(E::V), .., None) => todo!(), // `..` replaces [1, 2] (.., None) => todo!(), // `..` replaces [0, 1, 2] }

We want to iterate to the both arms at the same time, to make sure that the elements with the same index form the exhaustive match. So, the logic implemented here basically evaluates the maximum possible length of the patterns and traverses both arms, considering entries in .. as wildcards (_).

Probably, the most simple solution would be to run the lint iff the second arm contains only wildcards. But this will make the lint a bit less accurate, because we won't be able to generate warnings in some cases.

In that case shouldn't, position- and rpositioning the .. pattern in the subpatterns be enough? Also "span" has a different meaning in most lints, so the naming could be improved (if needed at all).

I fixed the naming to make it differ from rustc's Spans.

I'm not sure if we want to remove these variables at all, because this would complicate the loop in places when we need to evaluate the relative offset from the current pattern to the .. position.

tests/ui/single_match.rs

camsteffen · 2022-01-24T15:49:45Z

Just a note that the term "exhaustive match" doesn't really make sense like I thought it did when I posted the issue. All matches are exhaustive by necessity. The problem is more specific to enums, like "exhaustive enum patterns without a wild".

llogiq · 2022-01-30T19:51:38Z

Thank you! @bors r+

bors · 2022-01-30T19:51:39Z

📌 Commit a8fdf5c has been approved by llogiq

bors · 2022-01-30T19:51:46Z

⌛ Testing commit a8fdf5c with merge 7b89fa9...

single_match: Don't lint non-exhaustive matches; support tuples `single_match` lint: * Don't lint exhaustive enum patterns without a wild. Rationale: The definition of the enum could be changed, so the user can get non-exhaustive match after applying the suggested lint (see #8282 (comment) for context). * Lint `match` constructions with tuples (as suggested at #8282 (comment)) Closes #8282

bors · 2022-01-30T19:52:14Z

💔 Test failed - checks-action_test

xFrednet · 2022-01-30T22:00:32Z

The changelog entries were missing in the PR description, I added them now based on the current description 🙃

@bors retry

bors · 2022-01-30T22:00:38Z

⌛ Testing commit a8fdf5c with merge 0ed8ca4...

bors · 2022-01-30T22:17:49Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 0ed8ca4 to master...

camelid · 2022-01-31T02:14:17Z

Btw, does this work with tuple structs (S(_, _)) too?

jubnzv · 2022-01-31T13:19:01Z

Btw, does this work with tuple structs (S(_, _)) too?

No, this PR doesn't implement any additional logic with structs.

I think, we could process them just like tuples, i.e.:

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

// lint: S(..) forms exhaustive match, so it could be removed
match s {
  S(42, a) => dummy(a),
  S(..) => {},
}

Am I right?

camsteffen · 2022-01-31T19:00:47Z

Yes. It should generally work with all PatKind variants.

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)

jubnzv · 2022-02-05T08:42:09Z

I added support for tuple structs in #8395.

rust-highfive assigned llogiq Jan 20, 2022

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

llogiq reviewed Jan 20, 2022

View reviewed changes

jubnzv force-pushed the 8282-single-match branch 2 times, most recently from 404e262 to a0c5087 Compare January 21, 2022 04:25

jubnzv added 2 commits January 21, 2022 07:28

single_match: Clarify the don't lint test case

a0c5087

matches: Simplify code

49ae73b

jubnzv added 4 commits January 26, 2022 14:16

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

5416a71

matches: Clarify the behavior of exhaustive check

4aee3b1

matches: Improve naming. NFC.

8101587

matches: Restore match_type logic; add tests for these cases

467a0bf

jubnzv force-pushed the 8282-single-match branch 3 times, most recently from 8c320be to a8fdf5c Compare January 26, 2022 16:47

matches: Remove extra comment

a8fdf5c

jubnzv marked this pull request as ready for review January 26, 2022 17:20

bors merged commit 0ed8ca4 into rust-lang:master Jan 30, 2022

bors mentioned this pull request Jan 30, 2022

match_same_arms fix #8232

Merged

jubnzv mentioned this pull request Feb 5, 2022

single_match: Lint structs and tuple structs #8395

Closed

yeputons mentioned this pull request Jun 1, 2022

single_match should lint exhaustive match on standard types #8928

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jubnzv commented Jan 20, 2022 •

edited by xFrednet

Loading

rust-highfive commented Jan 20, 2022

jubnzv commented Jan 20, 2022

llogiq left a comment

llogiq Jan 20, 2022

jubnzv Jan 21, 2022 •

edited

Loading

llogiq Jan 21, 2022

jubnzv Jan 26, 2022 •

edited

Loading

camsteffen commented Jan 24, 2022

llogiq commented Jan 30, 2022

bors commented Jan 30, 2022

bors commented Jan 30, 2022

bors commented Jan 30, 2022

xFrednet commented Jan 30, 2022 •

edited

Loading

bors commented Jan 30, 2022

bors commented Jan 30, 2022

camelid commented Jan 31, 2022

jubnzv commented Jan 31, 2022

camsteffen commented Jan 31, 2022

jubnzv commented Feb 5, 2022

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

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

Conversation

jubnzv commented Jan 20, 2022 • edited by xFrednet Loading

rust-highfive commented Jan 20, 2022

jubnzv commented Jan 20, 2022

llogiq left a comment

Choose a reason for hiding this comment

llogiq Jan 20, 2022

Choose a reason for hiding this comment

jubnzv Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

llogiq Jan 21, 2022

Choose a reason for hiding this comment

jubnzv Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

camsteffen commented Jan 24, 2022

llogiq commented Jan 30, 2022

bors commented Jan 30, 2022

bors commented Jan 30, 2022

bors commented Jan 30, 2022

xFrednet commented Jan 30, 2022 • edited Loading

bors commented Jan 30, 2022

bors commented Jan 30, 2022

camelid commented Jan 31, 2022

jubnzv commented Jan 31, 2022

camsteffen commented Jan 31, 2022

jubnzv commented Feb 5, 2022

jubnzv commented Jan 20, 2022 •

edited by xFrednet

Loading

jubnzv Jan 21, 2022 •

edited

Loading

jubnzv Jan 26, 2022 •

edited

Loading

xFrednet commented Jan 30, 2022 •

edited

Loading