Don't suggest doc(hidden) or unstable variants in wildcard lint #7407
Conversation
Every time we add something to this enum in std, this test breaks.
|
r? @phansch (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
m-ou-se
force-pushed
the
doc-hidden-variants
branch
from
94470ff to
38569c0
Compare
There was a problem hiding this comment.
Could you update the changelog message to mention the specific lints that are effected by this change? That makes writing the change log easier in the end. In this case, it should be the lint wildcard_enum_match_arm
The rest looks good to me.
Hey @flip1995, I would take over the review of this PR if that works for you :)
| @@ -1033,7 +1033,7 @@ fn check_wild_enum_match(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>]) | |||
|
|
|||
| // Accumulate the variants which should be put in place of the wildcard because they're not | |||
| // already covered. | |||
| let mut missing_variants: Vec<_> = adt_def.variants.iter().collect(); | |||
| let mut missing_variants: Vec<_> = adt_def.variants.iter().filter(|x| !is_hidden(cx, x)).collect(); | |||
There was a problem hiding this comment.
I think here it would be good to differentiate between the enums origin. It would make sense to me to also cover hidden/unstable variations if they originate from the same crate. (I couldn't find a check for that in the current implementation)
I'm also debating if that should be configurable, but I believe there is no real reason for that.
There was a problem hiding this comment.
I don't think this has to be done in this PR. I like this idea though. Can you open a good-first-issue issue for this, please?
|
(Un-assigning from phansch, since he's taking a break, thanks for taking over the review @xFrednet!) |
|
@rustbot label -S-waiting-on-review +S-waiting-on-author |
rustbot
added
S-waiting-on-author
This fixes a bug where match_wildcard_for_single_variants produced a bad suggestion where besides the missing variant, one or more hidden variants were left. This also adds tests to the ui-tests match_wildcard_for_single_variants and wildcard_enum_match_arm to make sure that the correct suggestion is produced.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Don't suggest doc(hidden) or unstable variants in wildcard lint Clippy's wildcard lint would suggest doc(hidden) and unstable variants for non_exhaustive enums, even though those aren't part of the public interface (yet) and should only be matched on using a `_`, just like potential future additions to the enum. There was already some logic to exclude a *single* doc(hidden) variant. This extends that to all hidden variants, and also hides `#[unstable]` variants. See rust-lang/rust#85746 (comment) This PR includes #7406 as the first commit. Here's the diff that this PR adds on top of that PR: m-ou-se/fork@std-errorkind...m-ou-se:doc-hidden-variants --- *Please write a short comment explaining your change (or "none" for internal only changes)* changelog: - No longer suggest unstable and doc(hidden) variants in wildcard lint.
This comment has been minimized.
This comment has been minimized.
|
@bors retry |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
| --> $DIR/wildcard_enum_match_arm.rs:101:13 | ||
| | | ||
| LL | _ => (), | ||
| | ^ help: try this: `Enum::B | _` |
There was a problem hiding this comment.
I find it confusing that the lint suggests here to use the enum value in the same arm as the wildcard. That would mean that future additions would also run into this branch.
It would be good to either add a note to explain why a wildcard has to be present here or to make the suggestion to have two arms like this:
Enum::B => {},
_ => { /* Hidden values */ }Adding a note that the enum includes hidden, or unstable values would still be good.
There was a problem hiding this comment.
A X | _ pattern is already caught by another lint, so this shouldn't be an issue.
Clippy's wildcard lint would suggest doc(hidden) and unstable variants for non_exhaustive enums, even though those aren't part of the public interface (yet) and should only be matched on using a
_, just like potential future additions to the enum. There was already some logic to exclude a single doc(hidden) variant. This extends that to all hidden variants, and also hides#[unstable]variants.See rust-lang/rust#85746 (comment)
This PR includes #7406 as the first commit.
Here's the diff that this PR adds on top of that PR: m-ou-se/fork@std-errorkind...m-ou-se:doc-hidden-variants
Please write a short comment explaining your change (or "none" for internal only changes)
changelog: No longer suggest unstable and doc(hidden) variants in wildcard lint. wildcard_enum_match_arm, match_wildcard_for_single_variants