Transform some model enums back to structs for forwards compatibility

[kangalio]

We currently sometimes use enums to model mutually exclusive fields, which is very good usability.

Examples

serenity/src/model/application/component.rs

Lines 97 to 100 in fd0b573

	pub enum ButtonKind {
	Link { url: String },
	NonLink { custom_id: String, style: ButtonStyle },
	}

serenity/src/model/guild/automod.rs

Lines 82 to 88 in fd0b573

	pub enum Trigger {
	Keyword { strings: Vec<String>, regex_patterns: Vec<String> },
	HarmfulLink,
	Spam,
	KeywordPreset(Vec<KeywordPresetType>),
	Unknown(u8),
	}

serenity/src/model/guild/automod.rs

Lines 274 to 290 in fd0b573

	pub enum Action {
	/// Blocks the content of a message according to the rule.
	BlockMessage,
	/// Logs user content to a specified channel.
	Alert(ChannelId),
	/// Timeout user for a specified duration.
	///
	/// Maximum of 2419200 seconds (4 weeks).
	///
	/// A `Timeout` action can only be setup for [`Keyword`] rules. The [Moderate Members]
	/// permission is required to use the `Timeout` action type.
	///
	/// [`Keyword`]: TriggerType::Keyword
	/// [Moderate Members]: crate::model::Permissions::MODERATE_MEMBERS
	Timeout(Duration),
	Unknown(u8),
	}

serenity/src/model/application/command_interaction.rs

Lines 629 to 643 in fd0b573

	pub enum CommandDataOptionValue {
	Autocomplete { kind: CommandOptionType, value: String },
	Boolean(bool),
	Integer(i64),
	Number(f64),
	String(String),
	SubCommand(Vec<CommandDataOption>),
	SubCommandGroup(Vec<CommandDataOption>),
	Attachment(AttachmentId),
	Channel(ChannelId),
	Mentionable(GenericId),
	Role(RoleId),
	User(UserId),
	Unknown(u8),
	}

However, if Discord adds a new field to one of the variants, that's a breaking change. This has happened with Action::BlockMessage:

serenity/src/model/guild/automod.rs

Lines 274 to 290 in fd0b573

	pub enum Action {
	/// Blocks the content of a message according to the rule.
	BlockMessage,
	/// Logs user content to a specified channel.
	Alert(ChannelId),
	/// Timeout user for a specified duration.
	///
	/// Maximum of 2419200 seconds (4 weeks).
	///
	/// A `Timeout` action can only be setup for [`Keyword`] rules. The [Moderate Members]
	/// permission is required to use the `Timeout` action type.
	///
	/// [`Keyword`]: TriggerType::Keyword
	/// [Moderate Members]: crate::model::Permissions::MODERATE_MEMBERS
	Timeout(Duration),
	Unknown(u8),
	}

https://discord.com/developers/docs/change-log#add-auto-moderation-custommessage-action-metadata-field

To be forward-compatible with new fields, we should change back some of those enums to structs. Some enums won't realistically get new fields in existing variants, like CommandDataOptionValue; we can leave those as enums.

In the case of Action, the equivalent struct would look like

#[non_exhaustive]
pub enum ActionType {
    BlockMessage = 1,
    SendAlertMessage = 2,
    Timeout = 3,
}

#[derive(Default, Serialize, Deserialize)]
#[non_exhaustive]
pub struct ActionData {
    /// For [`ActionType::SendAlertMessage`] (required)
    pub channel_id: Option<ChannelId>,
    /// For [`ActionType::Timeout`] (required)
    pub duration: Option<Duration>,
    /// For [`ActionType::BlockMessage`] (optional)
    pub custom_message: Option<String>,
}

#[derive(Serialize, Deserialize)]
pub struct Action {
    type: ActionType,
    metadata: ActionData,
}

[GnomedDev]

We cannot just deem certain enums as "oh well Discord will never add a variant" and have two different styles of enum based on that assumption. The breaking can be solved with non_exhaustive in the existing system or we should convert all enums to structs (with terrible usablity and massively bloating the "enum" sizes).

[kangalio]

We cannot just deem certain enums as "oh well Discord will never add a variant"

I said "Some enums won't realistically get new fields in existing variants".

The breaking can be solved with non_exhaustive in the existing system

The breaking of adding new variants can be solved with #[non_exhaustive], but the breaking of adding new fields to existing variants can't, really.

It is possible to annotate individual enum variants with #[non_exhaustive] to mean that the individual variants may gain new fields, however the variants become unconstructable then. Actually, this is probably okay for CommandDataOptionValue. But it's not okay for ButtonKind, Action, and Trigger.

[GnomedDev]

Ah okay, would it not be fixed by splitting the enum struct fields into their own structs, then referencing them in the enums? Then a new function or something could be used instead of literal syntax

[GnomedDev]

Thinking about it, that could be automated with a macro

[kangalio]

Ah okay, would it not be fixed by splitting the enum struct fields into their own structs, then referencing them in the enums? Then a new function or something could be used instead of literal syntax

That would indeed be another solution. I'm kinda opposed to that because it would add a big pile of overhead on top of the existing overhead of the enum approach.

Using Trigger as an example; compare

#[non_exhaustive]
pub enum TriggerType { 
    Keyword
    HarmfulLink, 
    Spam, 
    KeywordPreset,
    Unknown(u8), 
}

#[non_exhaustive]
pub struct TriggerData { 
    pub strings: Option<Vec<String>>,
    pub regex_patterns: Option<Vec<String>>,
    pub presets: Option<Vec<KeywordPresetType>>,
}

with

#[non_exhaustive]
pub enum Trigger {
    Keyword(TriggerKeyword),
    HarmfulLink(TriggerHarmfulLink),
    Spam(TriggerSpam),
    KeywordPreset(TriggerKeywordPreset),
    Unknown(u8),
}

impl Trigger {
    pub fn trigger_type(&self) -> TriggerType {
        pub fn kind(&self) -> TriggerType {
            match *self {
                Self::Keyword(_) => TriggerType::Keyword,
                Self::Spam(_) => TriggerType::Spam,
                Self::KeywordPreset(_) => TriggerType::KeywordPreset,
                Self::MentionSpam(_) => TriggerType::MentionSpam,
                Self::Unknown(unknown) => TriggerType::Unknown(unknown),
            }
        }
    }
}

#[non_exhaustive]
pub struct TriggerKeyword {
    pub strings: Vec<String>,
    pub regex_patterns: Vec<String>,
}

impl TriggerKeyword {
    pub fn new(strings: Vec<String>, regex_patterns: Vec<String>) -> Self {
        Self {
            strings,
            regex_patterns,
        }
    }
}

#[non_exhaustive]
pub struct TriggerHarmfulLink {}

impl TriggerHarmfulLink {
    pub fn new() -> Self {
        Self {}
    }
}

#[non_exhaustive]
pub struct TriggerSpam {}

impl TriggerSpam {
    pub fn new() -> Self {
        Self {}
    }
}

#[non_exhaustive]
pub struct TriggerKeywordPreset {
    pub presets: Vec<KeywordPresetType>,
}

impl TriggerKeywordPreset {
    pub fn new(presets: Vec<KeywordPresetType>) -> Self {
        Self {
            presets,
        }
    }
}

enum_number! {
    #[non_exhaustive]
    #[derive(Serialize, Deserialize)]
    pub enum TriggerType {
        Keyword = 1,
        HarmfulLink = 2,
        Spam = 3,
        KeywordPreset = 4,
        _ => Unknown(u8),
    }
}

#[derive(Deserialize, Serialize)]
#[serde(rename = "Trigger")]
struct RawTrigger<'a> {
    trigger_type: TriggerType,
    trigger_metadata: RawTriggerMetadata,
}

#[derive(Deserialize, Serialize)]
#[serde(rename = "TriggerMetadata")]
struct RawTriggerMetadata<'a> {
    keyword_filter: Option<Vec<String>>,
    regex_patterns: Option<Vec<String>>,
    presets: Option<Vec<KeywordPresetType>>,
}

impl<'de> Deserialize<'de> for Trigger {
    fn deserialize<D: Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
        let trigger = RawTrigger::deserialize(deserializer)?;
        let trigger = match trigger.trigger_type {
            TriggerType::Keyword => Self::Keyword(TriggerKeyword {
                strings: trigger
                    .trigger_metadata
                    .keyword_filter
                    .ok_or_else(|| Error::missing_field("keyword_filter"))?,
                regex_patterns: trigger
                    .trigger_metadata
                    .regex_patterns
                    .ok_or_else(|| Error::missing_field("regex_patterns"))?,
            }),
            TriggerType::HarmfulLink => Self::HarmfulLink(TriggerHarmfulLink {}),
            TriggerType::Spam => Self::Spam(TriggerSpam {}),
            TriggerType::KeywordPreset => Self::KeywordPreset(TriggerKeywordPreset {
                presets: trigger
                    .trigger_metadata
                    .presets
                    .ok_or_else(|| Error::missing_field("presets"))?,
            }),
            TriggerType::Unknown(unknown) => Self::Unknown(unknown),
        };
        Ok(trigger)
    }
}

impl Serialize for Trigger {
    fn serialize<S: Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
        let mut trigger = RawTrigger {
            trigger_type: self.trigger_type(),
            trigger_metadata: RawTriggerMetadata {
                keyword_filter: None,
                regex_patterns: None,
                presets: None,
            },
        };
        match self {
            Self::Keyword(TriggerKeyword {
                strings,
                regex_patterns,
            }) => {
                trigger.trigger_metadata.keyword_filter = Some(strings.into());
                trigger.trigger_metadata.regex_patterns = Some(regex_patterns.into());
            },
            Self::HarmfulLink(TriggerHarmfulLink {}) => {},
            Self::Spam(TriggerSpam {}) => {},
            Self::KeywordPreset(TriggerKeywordPreset {
                presets,
            }) => trigger.trigger_metadata.presets = Some(presets.into()),
            Self::Unknown(_) => {},
        }
        trigger.serialize(serializer)
    }
}

Thinking about it, that could be automated with a macro

True, at least the new overhead from TriggerKeyword, TriggerHarmfulLink, TriggerSpam, and TriggerKeywordPreset. I'll try something

By the way, the manual Serialize/Deserialize could also be automated with serde-derive, if dtolnay would just stop ignoring and finally merge serde-rs/serde#2056 😭

[kangalio]

I'll try something

The following macro works for the current state of Trigger (all fields required)

Code

macro_rules! forward_compatible_enum {
    (
        pub enum $enum_name:ident {
            $( $variant_name:ident($struct_name:ident {
                $( pub $field_name:ident: $field_type:ty, )*
            } ), )*
        }
    ) => {
        #[non_exhaustive]
        pub enum $enum_name {
            $( pub $variant_name($struct_name), )*
            Unknown(u8),
        }

        $(
            #[non_exhaustive]
            pub struct $struct_name {
                $( pub $field_name: $field_type, )*
            }

            impl $struct_name {
                pub fn new($( $field_name: $field_type ),*) -> Self {
                    Self { $( $field_name ),* }
                }
            }
        )*
    }
}

forward_compatible_enum! {
    pub enum Trigger {
        Keyword(TriggerKeyword {
            strings: Vec<String>,
            regex_patterns: Vec<String>,
        }),
        HarmfulLink(TriggerHarmfulLink {}),
        Spam(TriggerSpam {}),
        KeywordPreset(TriggerKeywordPreset {
            presets: Vec<KeywordPresetType>,
        }),
    }
}

However, if Discord adds a new optional field, the macro would add it to the new() constructor fields automatically. So the macro would need to differentiate between required and optional fields. This is messy; I think it's only possible with tt-munching or a proc macro