derive common traits for structs

[crumplecup]

This pull request stems from an issue I had as a user of the library. When I added a field of type GeoPoint2d to my struct, I discovered the struct could no longer by serialized or deserialized. However, I can add a Point2d field to the struct without issue. Why can Point2d be serialized and not GeoPoint2d, especially knowing both contain fields of f64? My preference would be for both to be serializable.

The set of common traits that I like to see implemented on public structs in a library, when appropriate for the data type, are Debug, Default, Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Hash, Serialize and Deserialize. Since you already have serde as a dependency, it makes sense to me to derive Serialize and Deserialize where possible, though I also see lots of libraries putting these derives behind a feature gate, and that approach has some appeal to me. Another good candidate for a feature gate are the strum and and strum_macros libraries which allow you to derive EnumIter on your enums, which can come in handy.

Where the data structure does not have a natural Default or cannot be Copy, I did not try to get fancy, I just stripped it from the derive, so the only additions are derives that do not make the compiler complain. The test suite still passes and clippy has no comment the changes. Once I got started on GeoPoint2d, I kept going and touched up the structs where I could, including a couple structs that look like internal library housekeeping, where I can't imagine you actually need Hash, like ContourPointsIterator... maybe that is going too far.

There are two somewhat ancient threads I referred to for recommendations on what traits to derive for libraries. User vadixidav makes the pro case on reddit:

I would recommend deriving Clone, Debug, PartialEq, Eq, PartialOrd, Ord, and Hash (all where possible) if you are making a library. As pointed out by other posters, there are some exceptions. If you make a binary, I would only derive what you need and no more. If you make a library I highly recommend adding serde as an optional dependency and deriving Serialize and Deserialize as well, where applicable.

If you make a library, you may want to explicitly not expose certain traits. I would always consider "will this prevent a totally valid usage, and is it strictly incorrect for a user to use this trait". As time has gone on, I have typically become more lenient, and I tend to only disallow API usage that could break invariants expected in my structs, so I typically always derive things like Ord if I can to support putting objects into BTreeMap, as it usually has no negative side-effects. It can be annoying when you have a float in your struct and then you cannot derive it, but you just have to accept that.

User caleblbaker adds the following observation that i find helpful:

When you implement a trait you are making a statement about your type like "this type can be copied" or "this type can be converted into some serialized format like json". So don't implement a trait unless the statement that that trait represents is true for your type. In the case of derivable traits, the common situation is that if your type is able to derive the trait then the statement that the trait is making is probably true for the type. So in most cases there's nothing wrong with deriving traits that you might not be taking advantage of since you're just making true statements about your type.

On the conservative, or against side, I can quote the venerable Burnt Sushi from the rust users forum:

Note that this advice is assuming you're developing a library and that the type in question is a public type exposed to users of your library.

I think some good general advice is be conservative because the traits you derive on your types are part of its public interface. For example, if you derive Copy on an enum but later decide to, say, add a variant with a String in it, then you're forced to remove the impl for Copy on your type. This will be a breaking change for downstream users. This is true for any other trait, but Copy can be especially tricky because it's easy to change the definition of your type such that it can no longer be Copy.

Being conservative is a good thing here because adding an impl to your type can never be a breaking change. (Is this true? Can someone check me on that? I think it is.) IMO, the process of adding impls "because the compiler says you needed them" is not a bad place to start. It keeps your impls to the minimum possible. If your types need more impls, then someone will hopefully come along and say, "Hey you should derive this trait on your type because such and such..." And then you can evaluate whether it's worthwhile to do so, or whether the user should just create a wrapper type and derive the impl themselves (which may not be possible if the impl requires details of the type that aren't exposed).

Deriving Debug is probably close to a universally good thing. It's just plain convenient to allow users of your library to dump a quick 'n' dirty representation of a value. Moreover, the name of the trait itself won't give any false impressions about whether that representation should be shown to the user.

Generally I would categorize ignoring Burnt Sushi's advice to be foolish, or out on a limb. Note that his primary concern is creating breaking changes for downstream users in the future if these derives need to be removed to accommodate additional constraints introduced by, for example, adding a new field with a more constrictive type. As one of the users described who is coming along saying "Hey you should derive this trait", I kind of see it the other way around. The fact that removing these derives creates fear of breaking changes implies that users are not only taking advantage of these traits, but in fact may rely on them to the extent that removing them could break their code. This is a good thing because it enables us to use to the code in the first place to accomplish the goals of our project.

[Maximkaaa]

Thank for very detailed PR comment :-D