serenity icon indicating copy to clipboard operation
serenity copied to clipboard

Cargo features should be additive

Open frxstrem opened this issue 2 years ago • 2 comments

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 the cache 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, and voice_state_update
  • The CurrentUser and User types have a public field accent_colour whose type changes from Option<u32> to Option<Colour> if the utils feature is enabled. The Role and Embed types have a public field colour whose type changes from u32 to Colour if the utils feature is enabled.
  • The ClientBuilder type fails if the framework features is enabled, unless the .framework method is called.

This can introduce issues when using the crate, such as:

  1. Non-additive features go against Rust conventions, which can lead to the library having surprising behavior.
  2. Documentation being incomplete and/or wrong for the set of features actually being used, making the library harder to use correctly.
  3. 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.

frxstrem avatar Apr 16 '22 22:04 frxstrem

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

  1. 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
  2. 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 example
    async 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.

frxstrem avatar Apr 16 '22 22:04 frxstrem

A good way to find remaining affected code is to search globally for #[cfg(not(

kangalio avatar Sep 18 '22 05:09 kangalio

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 =

kangalio avatar Nov 05 '22 20:11 kangalio