serenity-rs / framework Goto Github PK

Having a OneOf (OneOfTwo more specifically) enum to allow a command argument to be one of two types would be very convenient with the new framework.

enum OneOf<T: Parse, U: Parse> {
    VariantOne(T),
    VariantTwo(U),
}

The OneOf enum would implement the Parse trait and try to parse the string as type T first. If that fails, it'd try type U. If both fail, it'd return an error that contains both T's and U's parse errors. Inside a command's function body, a simple match could be used to handle the OneOf argument.

Currently, without such a type, a developer would have to create newtypes and implement the Parse trait for all combinations they wish to support (or create something like this type themselves). I've often had to parse user input as one of two possible types, so having the framework support something like this would be very nice.

I wanted to create this issue before working on a PR for the same to see if such a type would be appropriate for the new framework.

The command arguments will be part of the #[command] macro to serve as a shorthand for parsing arguments manually. They will be provided in the signature of the function after the message parameter, appearing as additional parameters specific to the command. The type of the arguments will have to implement the Argument trait defined in arguments::Argument. If the type implements FromStr, an implementation is automatically generated.

What needs to be done:

• Implement support in the #[command] macro by accepting an arbitrary amount of arguments after the message parameter
• Implement optional arguments
• Implement variadic ("rest") arguments

Code like this can be successfully processed by the #[command] proc-macro:

#[command]
pub async fn some_command(ctx: FrameworkContext<Data>, msg: &Message) {}

However, code like this fails:

type Context = FrameworkContext<Data>;

#[command]
pub async fn some_command(ctx: Context, msg: &Message) {}

Error:

error[E0308]: mismatched types
  --> src/crates.rs:42:1
   |
42 | #[command]
   | ^^^^^^^^^^
   | |
   | expected `()`, found struct `Data`
   | expected `serenity_framework::command::Command` because of return type
   |
   = note: expected struct `serenity_framework::command::Command<()>`
              found struct `serenity_framework::command::Command<Data>`

I think the reason for this failure is that #[command] attempts to deduce the user data type by parsing generic parameters of the ctx function argument. Because of the type alias, it can't find any generic parameters and falsely thinks this function uses the default user data (()).

If this suspicion is correct, the issue can be worked around something like this:

// Inside framework crate
pub struct Context<D = DefaultData, E = DefaultError> {
    // ...
}

#[doc(hidden)]
pub trait _ContextTypes {
    type D;
    type E;
}

impl<D, E> _ContextTypes for Context<D, E> {
    type D = D;
    type E = E;
}

The command_attr crate then uses <ARGUMENTTYPE as _ContextTypes>::D and <ARGUMENTTYPE as _ContextTypes>::E instead of trying to deduce the generic parameters from ARGUMENTTYPE.

Add something like a localisation module for users to easily integrate localised messages with the framework.

The current argument parsing system for commands enforces a strict order of the types of arguments, which proceeds as:

1. Required arguments
2. Optional arguments
3. Variadic argument or #[rest] argument

Each type of argument may be absent, but violating this order will result in a macro error. What this issue proposes, however, is to break the strict order for required and optional arguments. This would allow for backtracking when parsing arguments for a command.

For example, for the following command:

#[command]
async fn boogie(ctx: FrameworkContext, msg: &Message, user2: Option<UserId>, user: UserId) -> CommandResult {
    // ....
}

no error would originate from the macro. Instead, if just one argument is given, user2 will be None, while user will be populated with the argument. And in the case of two arguments, user2 will be the first argument, and user the second argument.

Order between required/optional arguments and a variadic/#[rest] argument will stay the same and will have to be uphold.

By virtue of relying on the std trait FromStr for parsing arguments, the framework cannot implement FromStr on other crates' types, most notably from serenity itself. For that reason, Member, User, Role, Emoji etc. currently cannot be used in the argument parsing system.

I can think of two ways to achieve this:

1. Use a custom trait, along with a blanket implementation impl<T: FromStr> ArgumentParse for T { ... }
2. Implement FromStr on those model types in the main serenity crate itself

2 is probably cleaner, though a new serenity version would have to be released before it can happen, which might take some time. 1 would open the door to potentially support third party types without FromStr as command arguments (chrono, perhaps?)

This is a basic roadmap to get the initial release of the new framework up and running.

What needs to be done:

• Port buckets from the original framework (relevant issues: #9)
• Implement utilities for creating a help command and provide plain-text and embed help commands out of the box (relevant issues: #10)
• Allow more complex types in the argument system that cannot work with just a FromStr implementation (relevant issues: #24)
• Allow mixing the order of required and optional arguments to permit backtracking in argument parsing (relevant issues: #31)

What does not have to be, but would be very nice:

• Implement localisation utilities such that the user can load and use localised text painlessly (relevant issues: #21)

Currently, when a user gives too many arguments to a command, or too few, or their input was malformed, serenity bubbles up the error as a Error::User. However, the error doesn't happen in user code at all; it happens in serenity's dispatch glue code. I think it would be more intuitive and more useful to bubble up argument parsing errors as Error::Dispatch

Edit: we decided that a new designated Error enum variant Error::Argument(ArgumentError) for argument parsing errors is better

Currently, the framework wraps the user data in an Arc<RwLock<_>>. It may be worth considering giving the choice to users which exact type they need, by using &D (or Arc if required) as the data type passed in Context instead of Arc<RwLock>.

Advantages:

• Allows more fine-grained locking about individual parts of the user data
• Allows control about the RwLock implementation used, or whether one is in use at all
  • Simple bots (for example Ferris from the Rust Community Server), that don't need mutability at all, could forego an RwLock or similar in the first place, making their code simpler and more performant

Disadvantages:

• Less familiarity with existing serenity users, who are used to having their user data wrapped in RwLock always

The FrameworkContext docs state: this context type contains every data that's relevant to the command. Would it be possible to move the &Message parameter, that every command function currently had, into FrameworkContext as well?

• it would make command functions shorter and cleaner
• it would be more consistent (since other data relevant to the command already resides in FrameworkContext)
• it encapsulates all useful command context into a single struct that can be passed around easily (for example in my bot using this framework I will have to pass around both FrameworkContext and &Message to a certain utility function, which seems redundant)

Is there an issue with the proposal that I hadn't considered? Otherwise I'd gladly create a PR for this