Introduce CommandError Set

[Wunka]

Does what was proposed in this review #3223 (comment)

[Argmaster]

Would it be possible to remove the if (source != .user) return error.InvalidSource; completely by simply changing the signature of execute depending on what is allowed, using comptime metaprogramming?

Both server and user mode:

pub fn execute(args: Args, source: ?*User) !void {

Only user mode:

pub fn execute(args: Args, source: *User) !void {

Only server mode (I don't think there are any cases?)

pub fn execute(args: Args) !void {

[Wunka]

Would it be possible to remove the if (source != .user) return error.InvalidSource; completely by simply changing the signature of execute depending on what is allowed, using comptime metaprogramming?

Maybe its just me but I want a command to actually think about why they don't support console etc, so I like it more if its "harder" to be restrictive.

Also the Source union may not stay at what it currently is. I could see that for example some kind of Entity / Block Source is also added.

Also, some commands may only support console on specifc parts. with the error they can (if entered a "wrong" path) just return that and a message is displayed. I am also thinking about adding a PermissionDenied error for commands who check more explicit permssionPaths etc

[Argmaster]

think about why they don't support console etc, so I like it more if its "harder" to be restrictive.

Excessive boilerplate is not a way to make API less error prone.
What's with Zig programmers wanting to write more code than needed? (Referring to Quantums preference of no auto-serialization).
Approach I proposed on its own should be easy to use right - it requires explicit declaration of intent through function argument type.

Also the Source union may not stay at what it currently is. I could see that for example some kind of Entity / Block Source is also added.

For future extensions, implementation can be adjusted accordingly, changes in the source code of command system will be required anyway, since union will have to change.
Moreover, we cannot prepare for all the funny things future will bring and being ready to switch behavior based on signature is more flexible than using Unions.

Honestly I am puzzled about push for explicit source checks compared to #3336 - so, should it be more automatic or more explicit? Security doesn't require explicit, fine grained control, but function arguments do?

[Wunka]

One nice thing about the union is that it can have functions (like it currently already has 2) so that commands don't really have to change unless they don't support something.
Also this explicit source check should be pretty rare. In my opinion all commands (outside of the worldedit commands) should support console. And for them the code which is the least is if they have the union.

I could see adding support so you can do either: Source or *User (here is though again the question, what if we add a third option? will we add all possible options of the union as options for args?) but I don't think we should go to things like ?*User. And also one of my points still stands. What about commands that are only restricted on specific paths?

At the end I can see that this is boilerplate for the Worldedit commands, and I am fine if we add the option to accept *User for args but I don't see the union going away again or this pr. (I still think for path related things or permission things this pr would be nice)

[IntegratedQuantum]

I thought about this for a bit, and I tend to agree with Argmaster, letting the type system handle the source selection is much safer than relying on the command creator to check their sources correctly (which could lead to a server crash).
This is (going to be) a moddable interface, so we should make it easy for the user to not make mistakes. And in my opinion not crashing beats not having the command available for one possible source.

Extending on argmasters idea I propose the following system:

The command system switches over the source, for each case it looks for a function of matching type and name executeUser(Args, *User) or executeServer(Args, *User), if it doesn't find one it defaults to a default execute(Args) which has no additional parameter. This extra default would make it easy to default to all possibilities, if no requirements for the source exist.

Though honestly I'm not 100% sure if this is the best approach, it seems problematic with all the commands that take an optional user.

---

Another idea could be to put the source inside the parser struct, e.g.

pub const Args = union(enum) {
	@"/tp <biome>": struct { biome: command.BiomeId, source: *User },
	@"/tp <x> <y> <z>": struct {
		x: command.Coordinate,
		y: command.Coordinate,
		z: command.Coordinate,
                source: *User,
	},
	@"/tp <playerIndex>": struct { playerIndex: command.PlayerIndex, source: *User },
	@"/tp <playerIndex>": struct { sourcePlyerIndex: command.PlayerIndex, playerIndex: command.PlayerIndex },
	...
};