Enum unit variant changed kind #894
Conversation
There was a problem hiding this comment.
Nicely done!
I have a slightly different opinion on the edge cases where this lint does/doesn't fire. It would be great to make a quick adjustment there and add some more test cases to pin down the behavior.
In general, when we have non-obvious decisions, it'd be great to document them with a detailed comment inline, because in 6 months neither of us will remember the details of why the decision was made. So if you ever feel like a piece of code in your PR would be more obvious if you explained it with a comment on GitHub, write that comment into the file itself to make it easier to find!
| attrs @filter(op: "not_contains", value: ["$non_exhaustive"]) | ||
| public_api_eligible @filter(op: "=", value: ["$true"]) |
There was a problem hiding this comment.
This is definitely a judgment call, but I think enum_tuple_variant_field_added operates under slightly different constraints: the variant existed previously and continues to exist, albeit with extra stuff added. In this lint, we don't have similar continuity in the variant — we have what is essentially a completely new variant which just happens to have the same name as the old one.
I could maybe be convinced about not linting if the variant became #[doc(hidden)]. But adding #[non_exhaustive] in the new version should definitely not prevent this lint, since "the struct isn't constructible" and "also the variant is a different kind" feel very orthogonal.
There was a problem hiding this comment.
Side note: it would be good to have a detailed comment similar to that other lint that lays out the reasoning for why these checks are included or omitted. They are non-obvious and depend on a judgment call, so it's good to be able to remind ourselves in the future why a certain decision made sense.
| "true": true, | ||
| "non_exhaustive": "#[non_exhaustive]", | ||
| }, | ||
| error_message: "A public enum's exhaustive unit variant has changed kind, breaking possible instantiations and patterns.", |
There was a problem hiding this comment.
Using "kind" for variant kinds isn't entirely standard, commonly-used Rust terminology. This line appears only once regardless of how many instances of breakage the lint finds, so let's use a slightly longer description in the interest of clarity.
| error_message: "A public enum's exhaustive unit variant has changed kind, breaking possible instantiations and patterns.", | |
| error_message: "A public enum's exhaustive unit variant has changed to a different kind of enum variant, breaking possible instantiations and patterns.", |
| description: "A public enum unit variant that isn't #[non_exhaustive] changed kind", | ||
| required_update: Major, | ||
| lint_level: Deny, | ||
| reference_link: Some("https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new"), |
There was a problem hiding this comment.
I'm surprised that it really seems like there isn't a great reference link. I couldn't find anything that covers enum variants in detail. The focus here should be on enum variants, not #[non_exhaustive], so let's go with this as the "best we could find":
| reference_link: Some("https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new"), | |
| // TODO: If the Rust reference gains a more detailed explanation of enum *variants*, | |
| // switch the explanation to point there instead. The current link isn't great. | |
| reference_link: Some("https://doc.rust-lang.org/reference/items/enumerations.html"), |
… and adjust tests
solves one lint from #884
Applied the same logic from enum_tuple_variant_field_added and similar lints for whether or not newly doc(hidden) and newly non_exhaustive variants should report for both.
Also, not sure about the reference link. Maybe https://doc.rust-lang.org/reference/attributes/type_system.html#the-non_exhaustive-attribute would be better?