-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Warn about trait bounds on struct and enum type parameters #1689
Comments
@oli-obk I think this would help a lot with people trying to write Deserialize<'de> bounds on their types. Almost all the struggles I've seen have been on structs, not so much on functions, and the answer is always just don't put a trait bound there. |
Hm, this is an interesting lint. Unsure if it should be warn by default; probably should. |
I believe it should warn by default as long as we can reliably detect the use of an associated type. |
Fun fact: a few libstd types could benefit from these changes. |
For the longest time |
For the longest time before 2013 rust-lang/rust@6ce7446#diff-76432ae7ccf0520cf36aac7c006809a3 😉 |
Won't the error messages quality decrease if those bounds are as late as possible? Especially for iterator adaptors? Don't know about any of the conversations on this topic, so I might be mixing some. A specialized version for a whitelist of bounds would probably be a good start. Things like debug, partialeq, deserialize... |
I'd also like to see how such a lint would fare against typenum. |
Perfectly fine. I tried removing all trait bounds from structs in typenum and it still compiles and passes all tests. |
@oli-obk could you put together an example with before and after code? I don't think this could happen. Errors are triggered when you rely on behavior, and we would be keeping trait bounds on behavior. |
cc @paholg |
It's beyond the scope of clippy, but I would like to see the opposite; that putting a constraint on a struct implicitly adds that constraint to all impls of that struct. This would mean that removing constraints would become a potentially breaking change, though. As is, removing constraints from structs shouldn't break things, but it moves information on how the struct should be used from the type signature into documentation. |
Constraints on a struct implying the same constraints on all impls is an orthogonal discussion. Even with that feature, I would strongly disagree with putting trait bounds on data and I think this lint would still be valuable. Consider Vec<T>. You can clone a vec if you can clone T. You can serialize a vec if you can serialize T. You can deserialize a vec if you can deserialize T. Look at the code I linked to in the top comment. I see this all the time. It is the equivalent of writing: #[derive(Clone, Serialize, Deserialize)]
struct Vec<T> where T: Clone + Serialize + Deserialize { /* ... */ } Yes your crate may compile either way. Yes "information on how the struct should be used [is in] the type signature [not] documentation." But no it's not valuable in any way. |
It's not valuable for a general purpose struct like I think this lint could be valuable, too, and I should have expressed that in the previous comment. It's easy enough to disable in the rare cases when you don't want it. |
Makes sense. On a spectrum between Vec<T> and typenum::PInt<U>, where do you think the Compact<T, H> type stands? And as a fraction of types how common do you think the typenum situation of opting out of this lint would be? |
I think the typenum case isn't so common – indeed I hope it goes away sooner than later, once Rust's type system can express integers directly. Another crate I know that uses generic constraints a lot is diesel. Perhaps @killercup can tell us about their uses? |
I haven't read the whole issue thread, but let me chime in regardless :) I'm pretty sure that since most public data structures contain some private fields (and they typically should for forwards compat reasons), they also have at least one associated constructor method (usually As far as I can As for private structs, it's possible/easier to just use the type constructors, so you need to keep track of the intended constraints yourself, or add them to your struct. |
This lint would be pointless if (big if) implied bounds is solved. |
As discussed in https://github.com/Manishearth/rust-clippy/issues/1689#issuecomment-296396580, this lint would be valuable whether or not implied bounds are solved. The typenum-like cases where you would want to opt out are rare. |
@dtolnay It think it makes sense for structures like |
I would disagree with allow-by-default even after implied bounds have landed. Let's look at the motivating example again. Keep in mind that this is real code written by a real live Rust user of the sort that would rely on Clippy for guidance in navigating the complicated world of Rust types. #[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
enum Compact<T: CompactPart, H: Serialize + Deserialize + Clone + 'static> {
Decrypted {
header: Header<H>,
payload: T,
},
Encrypted(Encrypted),
} Oh but we'd like to add a PartialOrd impl because it is needed for some new code. Easy enough: - #[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
- enum Compact<T: CompactPart, H: Serialize + Deserialize + Clone + 'static> {
+ #[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize, PartialOrd)]
+ enum Compact<T: CompactPart, H: Serialize + Deserialize + Clone + PartialOrd + 'static> { Boom, breaking change. Whether or not there are implied bounds. In general this pattern conflicts with the ways we like to think about adding functionality to data structures. The HashMap case is what we mean by the rare exceptions. Everyday code is much more like the code in this comment. |
Sounds good to me :) |
Derive `Serialize` and `Deserialize` for messages instead of `RustcEncodable` and `RustcDecodable`. Remove unnecessary trait bounds from types. These tend to transitively 'infect' any usage and require documenting the same bounds even if not used in a given function, type or implementation. The reason I decided to do this in this commit was because the change to Deserialize would require a lifetime bound, which isn't even currently used in structs deriving Deserialize. This led to a lot of unnecessary boilerplate showing up all over the code base. Removing the trait bounds altogether and only using them in functions and impl blocks cleaned this up. See this issue for more information: serde-rs/serde#890 For further details: https://github.com/Manishearth/rust-clippy/issues/1689#issuecomment-296396580 Simplify the Process definition by just using a single parameter instead of an associated type.
It is not useful to set trait bounds in struct. Refer: rust-lang/rust-clippy#1689
Following this has generally made my code nicer, but I have found one little hiccup that I'm not quite sure what to do about. Including trait bounds in the data definition allows access to associated items inside traits. This is often done with traits such as In my case I'm writing a stream adaptor that contains a buffer of items, but I think Peekable is a good enough analogy: pub struct Peekable<S: Stream> {
stream: Fuse<S>,
peeked: Option<S::Item>,
} This is taken from peek.rs in futures. If we want to write this without the pub struct Peekable<S, T> {
stream: Fuse<S>,
peeked: Option<T>,
} and then to include a However, including the |
This way they don't have to be repeated everywhere the type is used. See also rust-lang/rust-clippy#1689 .
We already provide `serde::{Serialize, Deserialize}` implementations for every implementation of `prio::field::FieldElement`, but we didn't `express that trait bound on `FieldElement` itself. This makes it tricky to serialize or deserialize `FieldElement` values in generic functions. To make this possible, we also change the declaration of various structs so that the `F: FieldElement` trait bound is on the `impl` and not the `struct`. See rust-lang/rust-clippy#1689 for explanation.
We already provide `serde::{Serialize, Deserialize}` implementations for every implementation of `prio::field::FieldElement`, but we didn't `express that trait bound on `FieldElement` itself. This makes it tricky to serialize or deserialize `FieldElement` values in generic functions. To make this possible, we also change the declaration of various structs so that the `F: FieldElement` trait bound is on the `impl` and not the `struct`. See [the Rust API guidelines][rust-api] or [this clippy issue](clippy) for justification. [rust-api]: https://rust-lang.github.io/api-guidelines/future-proofing.html#data-structures-do-not-duplicate-derived-trait-bounds-c-struct-bounds [clippy]: rust-lang/rust-clippy#1689
Traits are for behavior.
The only exception is things like
Cow
that use associated types to define data.My current understanding is that any data structure not using associated types should not have trait bounds (?Sized doesn't count). Redundant bounds like this suck because they transitively infect anything that names the type in any way. Consider this enum.
Without trait bounds on the data structure:
The text was updated successfully, but these errors were encountered: