Command overloading, & automatic command argument parsing & validation system

[dktapps]

Description

This has been discussed many times over the years, but it's never been done in a satisfying way.

Currently, all commands receive their arguments as string[] and have to do boilerplate validation themselves, including argument count checking, conversion to appropriate types (e.g. targets), checking if the targets exist/are valid/etc, checking if the parameters are part of an accepted set of values, etc.

You need only look at this nasty code to see why we need this. I remember spending hours and hours to get to those few lines of code.

Before I get into this, I'll mention that it's possible there are general-purpose libraries for supporting this kind of feature. Processing commands is not unique to Minecraft, as it's also a common need of applications to parse CLI arguments, which would have pretty much the same requirements.

It should be possible to implement this system without breaking BC, even just within a custom Command::execute(), as this is essentially a subcommands system combined with automatic argument validation & parsing. However, it may be desirable to introduce an OverloadedCommand to allow overload information to be exported for use in features like #6517.

This issue proposes the following:

• Commands should be allowed to have multiple executors (or overloads), which each declare the types and constraints of arguments that they want in their function signature (and perhaps attributes if needed), like so:
  • tp to player: function(Player $sender, Player $target)
  • tp to xyz: function(Player $sender, Vector3 $target)
  • tp to xyz+rotation: function(Player $sender, Vector3 $target, float $yaw, float $pitch)
  • tp player1 to player2: function(CommandSender $sender, Player $subject, Player $target)
  • tp player1 to xyz: function(CommandSender $sender, Player $subject, Vector3 $target)
  • tp player1 to xyz+rotation: function(CommandSender $sender, Player $subject, Vector3 $target, float $yaw, float $pitch)
• Requirements for each overload are extracted from the function signature using reflection. It may be necessary to support attributes for certain things, like dynamic enums.
• Overload is selected based on the number of args given, and whether the given args can be parsed according to the requirements of the overload. e.g. in tp player1 player2, player2 cannot be parsed as a Vector3, so overload 2 would be skipped. Overloads should be checked from most arguments to least arguments.
• Sender is treated like an argument for overload selection. In the example above, ConsoleCommandSender would automatically be prevented from using the first three overloads, as ConsoleCommandSender can't satisfy the type Player. This fixes the age-old issue of needing to check if($sender instanceof Player).
• Each overload has a permission associated. In the example above, the first three overloads would have pocketmine.command.tp.self and the remaining overloads would have pocketmine.command.tp.other. This would avoid 99% of explicit permission checking and also be used to prevent sending overloads to clients that they can't use (see #6517)
• If no available overload can accept the given args, an invalid syntax error will be returned to the user. (Possibly commands could provide a fallback overload which accepts string[], but not sure how this would work.)

Justification

The current command system is annoying to work with for anything but the simplest of use-cases, due to the writer of the command needing to know how to parse the given arguments.

This also leads to many inconsistencies in the way arguments are parsed and handled. For example, some people will just use (int) $args[0] to convert to an int instead of using getInteger() which has bounds checks. It's also easy to forget to check permissions for subcommands.

Implementing this would also make it trivial to support client-side command hints as per #6517, with no extra work by the developer. I'm of the opinion that if we're going to break BC to support command hints, we should do so in a way that maximizes the benefit gained for the extra work the developer has to do.

Alternative methods

[ShockedPlot7560]

What about using attribute in function signature to define custom validator, dynamic/hard enums to validate string or values ? In some case we probably want to have only a set of Player or whatever from a default validator/parser based on an enum. I think we need to include it to provide a better API

[dktapps]

I'm thinking a brigadier style API would allow more flexibility. Attributes wouldn't be able to include context info, so it would be difficult to do stuff like dynamic integer bounds. Might be OK for the simple cases to infer the arg types from the function though. Similar to how we have registerEvents() (uses magic & attributes) and registerEvent() (everything explicit & can do weird stuff not possible with magic & attributes, but harder to work with).

[dktapps]

Make no mistake though, this will probably be very hard to implement. As I rambled on about on #team the other day, the process of parsing args and selecting overloads would be non-trivial.