New lint: exhaustive_enums, exhaustive_structs #6617
Conversation
|
r? @llogiq (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
Manishearth
force-pushed
the
exhaustive_enums
branch
from
ffe5570 to
558cdcb
Compare
|
Thanks @Manishearth! Can we limit the lint to only public enums? And also structs? |
|
Yep. I'd make it two separate lints, since people match on structs less. |
Manishearth
force-pushed
the
exhaustive_enums
branch
from
558cdcb to
09d4d49
Compare
Manishearth
changed the title
|
@sffc updated! |
Manishearth
force-pushed
the
exhaustive_enums
branch
from
1e7a67c to
5bd681b
Compare
| bar: String, | ||
| } | ||
|
|
||
| // no warning, private |
There was a problem hiding this comment.
Can you also make sure that pub(crate) structs and enums don't require #[non_exhaustive]?
If there's a way to exclude pub structs and enums that are inside a non-pub module, that would be nice, too. But at least we should exclude pub(crate).
Basically, the check should only complain about truly public types.
There was a problem hiding this comment.
Yes, that's already how this is implemented, it's for exported types only
Manishearth
force-pushed
the
exhaustive_enums
branch
from
5bd681b to
4736986
Compare
|
cc @camsteffen wanna review this one? |
clippy_lints/src/exhaustive_items.rs
Outdated
| if !item.attrs.iter().any(|a| a.has_name(sym::non_exhaustive)); | ||
| then { | ||
| let lint = if let ItemKind::Enum(..) = item.kind { | ||
| EXHAUSTIVE_STRUCTS |
There was a problem hiding this comment.
| EXHAUSTIVE_STRUCTS | |
| EXHAUSTIVE_ENUMS |
clippy_lints/src/exhaustive_items.rs
Outdated
| EXHAUSTIVE_STRUCTS | ||
| }; | ||
|
|
||
| if let Some(snippet) = snippet_opt(cx, item.span) { |
There was a problem hiding this comment.
Consider using span_lint_and_then and span_suggestion(item.span.shrink_to_lo(), ..) so that you don't need to include the snippet in the suggestion.
There was a problem hiding this comment.
Hmm you didn't do the shrink_to_lo() part... the point of using span_lint_and_then was to use a different Span for the suggestion. The suggestion would be an insertion rather than replacing the entire item. I'm not entirely certain this is possible but it would be cleaner!
There was a problem hiding this comment.
Ahh. I'm not sure that works so well
There was a problem hiding this comment.
Will try, r=you if it doesn't?
clippy_lints/src/exhaustive_items.rs
Outdated
| cx, | ||
| lint, | ||
| item.span, | ||
| "enums should not be exhaustive", |
There was a problem hiding this comment.
"enums" should be a variable
There was a problem hiding this comment.
A couple nits. The message should be about "this enum" instead of "all enums". I think it would be good to mention that the item is "public" or "exported". So maybe: "declared a public, exhaustive enum"
There was a problem hiding this comment.
I picked "Exported foo should not be exhaustive". Lmk if you prefer the other way!
Manishearth
force-pushed
the
exhaustive_enums
branch
from
4736986 to
65d003a
Compare
|
@bors delegate+ |
|
✌️ @Manishearth can now approve this pull request |
|
@bors delegate=camsteffen oops |
|
✌️ @camsteffen can now approve this pull request |
|
This looks good, but as @camsteffen has already done most of the review, I'll leave the r+ to him. 😄 |
|
Nice! @bors r+ |
|
📌 Commit 3e3dff7 has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Fixes #6616
changelog: Added restriction lint:
exhaustive_enums,exhaustive_structs