`match_wildcard` improvements #6863

Jarcho · 2021-03-07T14:53:28Z

fixes: #6604
fixes: #5733
fixes: #6862

#5733 is only fixed in the normal case, if different paths are used for the variants then the same problem will occur. It's cause by def_path_str returning an utterly useless result. I haven't dug into why yet.

For #6604 there should be some discussion before accepting this. It's easy enough to change the message rather than disable the lint for Option and Result.

changelog: Attempt to find a common path prefix for match_wildcard_for_single_variants and wildcard_enum_match_arm
changelog: Don't lint op Option and Result for match_wildcard_for_single_variants and wildcard_enum_match_arm
changelog: Consider or patterns and Self prefix for match_wildcard_for_single_variants and wildcard_enum_match_arm

rust-highfive · 2021-03-07T14:53:31Z

r? @llogiq

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

llogiq · 2021-03-08T08:37:52Z

I'm not completely sure about the wording of the warning. Otherwise this looks good.

llogiq · 2021-03-08T08:41:09Z

Also a test with a #[non-exhaustive] enum would probably make sense.

bors · 2021-03-08T16:20:03Z

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

Jarcho · 2021-03-08T19:05:34Z

I left the wording as is, but it could be improved.

The suggestion to change the message instead of ignore the lint for Option and Result was from @camsteffen. I can definitely agree with it for wildcard_enum_match_arm, which is just banning wild matches outright. I'm not really sure about match_wildcard_for_single_variants though. The reason for the lint is the wild match is standing in for a singled variant, but new ones might be added in the future. This is just not true for most std types.

llogiq · 2021-03-09T06:48:43Z

Does the standard library really have that many single-variant enums?

Jarcho · 2021-03-09T13:54:37Z

It's for when the wildcard matches only a single variant, not when the enum has only one.

camsteffen · 2021-03-09T14:31:06Z

Fyi my suggestion for #6604 was intended for all enums. Personally I think that if a _ can be changed to Variant(..) then it should be. It makes the code more clear at little cost.

Jarcho · 2021-03-09T17:11:48Z

I can think of two reasons to use the lint.

Not having code silently break when a variant is added for an enum. Especially when wild match was just a lazy way to write out the last variant. In this case most std types should be ignored, they will never change.
Clarity reasons. In which case you would want it to lint on std types.

I would use the lint for the former, I don't really care about the latter. That's just how I would personally use it.

camsteffen · 2021-03-09T17:27:04Z

I see what you mean. The first reason is generally more important and could be the only important reason for others as well.

Perhaps we need a new pedantic lint wild_single_variant_std to be added some other time.

llogiq · 2021-03-13T15:05:27Z

clippy_lints/src/matches.rs

+            let mut suggestions: Vec<_> = variants.iter().cloned().map(format_suggestion).collect();
+            let message = if adt_def.is_variant_list_non_exhaustive() {
+                suggestions.push("_".into());
+                "match on non-exhaustive enum doesn't explicitly match all known variants"


I still don't like that this message references a "non-exhaustive enum" where the lint doesn't even trigger on enums that have been marked #[non-exhaustive]. How about "match on the single remaining variant by wildcard" instead?

Looks like I used the wrong message there.

llogiq · 2021-03-17T08:47:42Z

I'm still not completely sure about the message: "will miss any" reads like variants added in the future would not be matched when the opposite is the case. How about "will also match any" instead?

bors · 2021-03-17T14:56:34Z

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

…um_match_arm` lints Don't lint on `Result` and `Option` types. Considers `or` patterns. Considers variants prefixed with `Self` Suggestions will try to find a common prefix rather than just using the full path

…ariant`

Jarcho · 2021-03-17T16:40:56Z

Changed. Should the match on non-exhaustive enums be separated into it's own lint? The whole purpose of them is to not necessarily match every variant.

llogiq · 2021-03-18T10:39:24Z

Good question. But not a blocker, so in the interest of avoiding more rebasing work for you, we can address this in a followup. @bors r+

bors · 2021-03-18T10:39:25Z

📌 Commit d5a7941 has been approved by llogiq

bors · 2021-03-18T10:39:32Z

⌛ Testing commit d5a7941 with merge 4d68619...

bors · 2021-03-18T10:54:31Z

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

rust-highfive assigned llogiq Mar 7, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 7, 2021

Jarcho force-pushed the wild_enum_match branch 3 times, most recently from 45c41db to 0523137 Compare March 8, 2021 05:49

Jarcho force-pushed the wild_enum_match branch from 7b9bc87 to 456a541 Compare March 8, 2021 16:15

llogiq changed the title ~~match_wildcard imporvements~~ match_wildcard improvements Mar 9, 2021

llogiq reviewed Mar 13, 2021

View reviewed changes

Jarcho force-pushed the wild_enum_match branch 2 times, most recently from 3b25ecf to 76fbe92 Compare March 16, 2021 17:28

Jarcho added 2 commits March 17, 2021 12:04

Improvements to match_wildcard_for_single_variants and `wildcard_en…

0b7ab90

…um_match_arm` lints Don't lint on `Result` and `Option` types. Considers `or` patterns. Considers variants prefixed with `Self` Suggestions will try to find a common prefix rather than just using the full path

Add test for #[non_exhaustive] enum in `match_wildcard_for_single-v…

6cc9cac

…ariant`

Jarcho force-pushed the wild_enum_match branch from 76fbe92 to 575ec7d Compare March 17, 2021 16:39

Fix message for match_wildcard_for_single_variant

d5a7941

Jarcho force-pushed the wild_enum_match branch from 575ec7d to d5a7941 Compare March 17, 2021 16:42

bors merged commit 4d68619 into rust-lang:master Mar 18, 2021

dtolnay mentioned this pull request Mar 27, 2021

False positive match_wildcard_for_single_variants on enum with hidden variant #6984

Closed

Jarcho deleted the wild_enum_match branch April 6, 2021 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`match_wildcard` improvements #6863

`match_wildcard` improvements #6863

Jarcho commented Mar 7, 2021 •

edited

Loading

rust-highfive commented Mar 7, 2021

llogiq commented Mar 8, 2021

llogiq commented Mar 8, 2021

bors commented Mar 8, 2021

Jarcho commented Mar 8, 2021

llogiq commented Mar 9, 2021

Jarcho commented Mar 9, 2021

camsteffen commented Mar 9, 2021

Jarcho commented Mar 9, 2021

camsteffen commented Mar 9, 2021

llogiq Mar 13, 2021

Jarcho Mar 16, 2021

llogiq commented Mar 17, 2021

bors commented Mar 17, 2021

Jarcho commented Mar 17, 2021 •

edited

Loading

llogiq commented Mar 18, 2021

bors commented Mar 18, 2021

bors commented Mar 18, 2021

bors commented Mar 18, 2021

match_wildcard improvements #6863

match_wildcard improvements #6863

Conversation

Jarcho commented Mar 7, 2021 • edited Loading

rust-highfive commented Mar 7, 2021

llogiq commented Mar 8, 2021

llogiq commented Mar 8, 2021

bors commented Mar 8, 2021

Jarcho commented Mar 8, 2021

llogiq commented Mar 9, 2021

Jarcho commented Mar 9, 2021

camsteffen commented Mar 9, 2021

Jarcho commented Mar 9, 2021

camsteffen commented Mar 9, 2021

llogiq Mar 13, 2021

Choose a reason for hiding this comment

Jarcho Mar 16, 2021

Choose a reason for hiding this comment

llogiq commented Mar 17, 2021

bors commented Mar 17, 2021

Jarcho commented Mar 17, 2021 • edited Loading

llogiq commented Mar 18, 2021

bors commented Mar 18, 2021

bors commented Mar 18, 2021

bors commented Mar 18, 2021

`match_wildcard` improvements #6863

`match_wildcard` improvements #6863

Jarcho commented Mar 7, 2021 •

edited

Loading

Jarcho commented Mar 17, 2021 •

edited

Loading