Argument parsing errors should have designated Error enum variant

[kangalio]

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

[arqunis]

Currently, when a user gives too many arguments to a command, or too few, or their input was malformed, serenity the framework bubbles up the error as a Error::User.

This is intended. Argument parsing is part of the command's code that's first executed before running the actual body of the command. Failure to parse arguments constitutes as a user error, since it is the user's responsibility to parse arguments (the #[command] macro just provides syntax sugar to generate code the user might have written themselves).

To clarify, dispatch errors originate from the framework, user errors from the commands.

[kangalio]

Yeah I can see why it happens. I can't quite interpret what you think of the proposal though - do you agree that this behavior is confusing and a designated error enum variant might be beneficial, or is this a "working as intended" situation?

In other words, would a PR about this issue be accepted?

[arqunis]

The latter, "working as intended." The current error enum makes the origin of the errors from either the framework or commands clear, and allows the two to be handled separately:

fn handle_dispatch_error(err: DispatchError) {
    // ....
}

// Assuming the user defined a custom error type called `MyError`
fn handle_my_error(err: MyError) {
    // ...
}

let error = ...;

match error {
    Error::Dispatch(err) => handle_dispatch_error(err),
    Error::User(err) => handle_my_error(err)
}

If the user error was merged into DispatchError, the above match would still be possible, but more ugly, among other ways to handle errors.

Therefore, I don't believe that this is an issue that ought to be fixed. Besides that, how would we distinguish argument errors from other errors returned from commands? By default, the error type for commands is Box<dyn std::error::Error>, but the user can change that to something else. We could perhaps add a wrapper error type for argument errors and other errors for commands. However, that would be an ugly solution for the user and the framework, asserting my point that argument errors should stay as user errors.

[kangalio]

I see your concern about error handling being more difficult with the Error enum variants merged. I think there is a misconception here. My idea was not to merge the variants, but to add a new variant.

Either to Error:

pub enum Error {
    Dispatch(DispatchError),
    Argument(ArgumentError<Box<dyn std::error::Error>>), // <--
    User(E),
}

or to DispatchError:

pub enum DispatchError {
    NormalMessage,
    PrefixOnly(String),
    InvalidCommandName(String),
    CheckFailed(String, Reason),
    ArgumentError(ArgumentError<Box<dyn std::error::Error>>), // <--
}

(Not sure yet which one of the two is more ergonomic for the user. Also, Box<Error> because parse implementations may return all kinds of different error types).

Funnily enough, the motivation behind this change is of the same kind as the concern you expressed: merging argument parsing errors and user-thrown errors into the same Error::User variant makes precise error handling more difficult.

Have I understood and addressed your concern?

[arqunis]

I understood your request to separate argument errors from Error::User, and to place them into Error::Dispatch, but

or to DispatchError:

That is what I meant by merging into DispatchError, that should belong entirely to the framework. Argument errors from commands are not framework errors. Adding a new variant to Error would be fine, but how would argument errors be differentiated from other errors returned from commands? The current definition of CommandResult is type CommandResult<E = DefaultError> = Result<(), E>;, that doesn't leave room to separate argument errors from other errors when a user defines their own error type. As I said, we could have a wrapper like so:

enum CommandError<E> {
    Argument(ArgumentError<Box<dyn std::error::Error>>),
    Normal(E),
} 

and use it instead of a plain E in Result<(), E> from the type alias above. This will work, but again, I don't like it. This wrapping should be done by the user with their own error type. Even if a user just depends on the default error type (Box<dyn std::error::Error>), they can downcast to ArgumentError and handle argument errors separately. I wouldn't have written ArgumentError otherwise if I hadn't considered this scenario.

[kangalio]

This wrapping should be done by the user with their own error type. Even if a user just depends on the default error type (Box), they can downcast to ArgumentError and handle argument errors separately.

Ah, now I understand how you envisioned users to handle argument parsing errors. Either call .downcast::<ArgumentError> on the Box<dyn Error> error, or swap in a custom error type with a From<ArgumentError> implementation, that allows later retrieval of the stored ArgumentError instance. That makes sense.

Adding a new variant to Error would be fine, but how would argument errors be differentiated from other errors returned from commands?

The generated function would yield a Result<(), FrameworkError<USERERROR>> instead of Result<(), USERERROR>. The function contents would need to be adjusted accordingly to correctly wrap argument parse errors in FrameworkError::Argument and user-thrown errors in FrameworkError::User. I have created a quick mock-up function to test this approach and it successfully compiled.

What I personally like about this approach in comparison to the other two you presented (downcasting and custom error type):

• it's easily discoverable: it's clear to users how they are supposed to handle argument parsing errors, which is the main problem I had. Not everyone knows about downcasting, and it is not at all obvious to use downcasting or custom error types to handle ArgumentErrors, particularly to people who don't know how the proc-macro generated code works
• it doesn't require users to adapt their error type to fit serenity's needs (read: provide a From<ArgumentError> implementation for their custom error type). This point is quite subjective but it feels unfortunate to me that users need to be aware of hidden code injected by serenity possibly throwing an ArgumentException, just in order to design their own error type. That feels like serenity is shifting responsibility over to the user when it should really handle its responsibilities itself.

By the way, to be clear: It is not my intention to force through my subjective opinion on this topic. I merely want to ensure that my idea and its (dis)advantages are fully understood and considered before the accept/reject decision is made.

[arqunis]

The generated function would yield a Result<(), FrameworkError<USERERROR>> instead of Result<(), USERERROR>. The function contents would need to be adjusted accordingly to correctly wrap argument parse errors in FrameworkError::Argument and user-thrown errors in FrameworkError::User. I have created a quick mock-up function to test this approach and it successfully compiled.

Looking from that angle, I like it, actually. I disagreed with your original proposal a lot, but reusing Error rather than creating a wrapper error type or integrating argument errors into DispatchError can simplify things. There's a slight problem, though. We can't add a From<std::error::Error> implementation for Error to return Error::User, because it would conflict with the From<T> for T impl from the standard library (as Error implements std::error::Error).

[kangalio]

We can't add a Fromstd::error::Error implementation for Error to return Error::User, because it would conflict with the From for T impl from the standard library (as Error implements std::error::Error).

I don't think such a trait impl is needed. The generated command code can embed the user code like this:

let result: Result<(), USERERROR> = async {
    // USER CODE HERE
}.await;
result.map_err(FrameworkError::User)

[arqunis]

Ah, then I have no more issues with this. You're free to open a pull request.