[API-8] Error "types" for CommandResults

[ejm]

Right now, CommandResults can either be successful or have a Component error message, but if another plugin would like to understand why a command resulted in an error, the Component would have to be parsed by the interested plugin.

This parsing by hand is fine for the Sponge-specific error messages that take on a consistent format, but if other plugins deviate and write their own error messages, a plugin attempting to capture those errors would have a hard time doing so in a consistent way.

My proposal are having different error "types" that can be attached to a CommandResult.error() that fall under common error categories. These categories are things like "not having permission" or "command couldn't be parsed," along with a more general "catch all" category for errors that can't otherwise be categorized. There likely wouldn't be too many of these categories.

As an example use-case, let's take a plugin like WorldEdit which has navigation commands. Using /ascend as an example command, I might want to change the error message that the command gives off if a player does not have permission to be something like "If you'd like to use /ascend, you need to have the Squid rank!" without changing other error messages by mistake. Those messages would of course need to be modified by another plugin, but that second plugin wouldn't need to know anything about the error message it is modifying other than its "type."

There may be better ways to do this, so I'd love to hear other thoughts, thanks!

[dualspiral]

Before I delve into this issue proper, I want to explain a little bit about how commands are dispatched at runtime in Sponge. Note that I'm simply going to talk about this in the context of parameterised commands, but most registrars will do things the same way.

There are effectively five steps to command execution, so for a command /command a b c:

1. Select the command (/command) - handled by the CommandManager and is common to all commands - and includes a "canUse" check.
2. Pass off control to the appropriate CommandRegistrar
3. For the Command.Parameterized registrar, parse the string (a b c), which includes a "canUse" check
4. If a successful parse is made, pass control to the plugin that owns the command to execute it.
5. Return control from the CommandRegistrar to interpret the result and throw an event.

Point 1 is easy for us to control (in the Sponge domain), and point 4 is not too contraversial because that'll normally be handled as part of execute(...) and the plugin that is executing a command and choose the appropriate reason.

However, point 3 is a problem - parsing. Because of the way our parsers work, there may be a multitude of reasons why a single command invocation failed. Take, for example, this command tree:

command -----> parse_a
          |
          |--> parse_b ---> parse_c (permission failure) --> parse_d
          |             |
          |             |-> parse_e (fails if not console)
          |
          |--> parse_x --> parse_y --> parse_z --> parse_ohno

If a player runs the command above, the parser will try all available routes before failing. The reasons for failure are:

• Too many arguments
• Did not pass the canUse predicate (both permission checks and console check - and we don't know exactly why it failed)
• Not enough arguments

What do we report due to this parse time failure? The reasons for failure are varied and multiple, and all of them are valid errors.

I'm loathed to make the API more complicated to add such error reasons in. For execution time, that's fine, we'd just error(Type, Component), I don't really object to something like that. However, this issue predominantly concerns itself with wanting to report these parse time failures and, frankly, because we integrate with Brig, the way it works does put some constraints on how we can report these parse time errors - and it would be inconsistent at best.

Your specific comment on using a WorldEdit command is interesting, simply because that's a good example of where this might fall down further. It's quite possible that, as a cross platform system, they assume the official Sponge impl and use the Brigadier registrar to register their commands. This is a vanilla like registrar and we basically don't touch it other than to register alises to the command manager. We won't have a clue why a command fails, we only know the result of an invocation request - that is, if it worked or not.

I understand the use case of customising the error message when the command is not in the manager. That would be easy. Beyond that, the manager does not have control and any reason would be at the mercy of the registrar or plugin - and even for our internal registrars, particularly the Vanilla one, this would be a challenge. I'm not even comfortable just providing the step where it failed (which would be something like SELECT, PARSE, EXECUTE) because beyond select, the steps are going to be registrar dependent.

Finally, in our implementation, we throw any exceptions that occurred back further to Minecraft, beyond our command manager, to let Minecraft handle the command exceptions, which it does by reading the message in the exception. Customising this would be expensive as we would need to create another exception for the message. Truth be told this is a minor issue as I could just stop the exception bubbling (I don't think we show the error anyway) but that then indicates a success to Minecraft and we might not want to do that.

Basically, this is a complicated problem, far more complicated than you're proposing, and I think it's likely to be inconsistent at best, particularly around parse time errors.

[ejm]

Wow. Yeah, I was not expecting this but greatly appreciate the full explanation!