serenity
serenity copied to clipboard
Cargo features should be additive
As documented in the Cargo reference here, features that are not mutually exclusive should be additive. In other words, code that compiles with one set of features enabled should also compile when more features are added.
This is not always the case for the serenity
crate, though. For example I've found these publicly exposed parts that may break things when features are added:
- The
EventHandler
trait has a few method signatures that change when thecache
feature is enabled:-
channel_update
,guild_create
,guild_delete
,guild_member_removal
,guild_member_update
,guild_role_delete
,guild_role_update
,guild_update
,message_update
,user_update
, andvoice_state_update
-
- The
CurrentUser
andUser
types have a public fieldaccent_colour
whose type changes fromOption<u32>
toOption<Colour>
if theutils
feature is enabled. TheRole
andEmbed
types have a public fieldcolour
whose type changes fromu32
toColour
if theutils
feature is enabled. - The
ClientBuilder
type fails if theframework
features is enabled, unless the.framework
method is called.
This can introduce issues when using the crate, such as:
- Non-additive features go against Rust conventions, which can lead to the library having surprising behavior.
- Documentation being incomplete and/or wrong for the set of features actually being used, making the library harder to use correctly.
- Library crates cannot reliably use types and traits from the
serenity
crate, since features could be enabled by other crates that would make them unable to compile, which means that code reuse becomes harder.
To resolve these issues, I would suggest that the library is updated such that adding new features can only break code using the crate if mutually exclusive features are enabled (such as rustls_backend
and native_tls_backend
), and not for other features.
Specifically for the EventHandler
trait, I would suggest that either
- the fields that may not be available due to a disabled feature are always present in the signature, but that a
None
value is passed to indicate unavailability, or - that a separate method with an extended signature is added when the
cache
feature is enabled, which will have a default implementation that calls the simpler signature, for exampleasync fn channel_update(&self, _ctx: Context, _new_data: Channel) {} #[cfg(feature = "cache")] async fn channel_update_cached(&self, ctx: Context, _old: Option<Channel>, new: Channel) { self.channel_update(ctx, new).await }
For the fields using the Colour
type, I would suggest always using that type, but only implementing methods on it if the utils
feature is enabled. This means that it would only be a simple wrapper type if it's disabled, but that the code remains compatible if the utils
feature is enabled.
For the ClientBuilder
type, I would suggest removing the restriction that .framework
must be called if the framework
feature is enabled.
A good way to find remaining affected code is to search globally for #[cfg(not(
I cannot find any instance of public API being altered or removed when enabling a feature anymore. I checked by global search for #[cfg(not(feature =