Commands rework - Brigadier backend

[Pesekjak]

This is a continuation of #7032 from the ground up. x)

Problem

The current Skript command system has several drawbacks for both Skript users and maintainers. I think the biggest are:

• There isn't a proper command parser. Instead, syntax patterns matching the command structure are generated and matched against. This can create unexpected behavior for commands such as hello <string> <number>, where the input hello foo 1 2 is considered valid. I also see this implementation as a maintenance burden.
• The command system is limited to the Minecraft environment; addons working with other platforms (cough, Discord) have to come up with their own alternatives.
• It is difficult to structure more complex commands.
• Brigadier features introduced in 1.13 are not supported.

Solution

The solution is to switch to Brigadier as the command backend.

The new implementation is now fully separate from the existing command system, mainly because of the many changes between the two. It is very unlikely this will change. My aim is to integrate most of the features from the old command system, deprecate it, and fully remove it later. This will and should take a lot of time.

Not using Brigadier as a backend would take a lot of extra effort for what I believe is no extra gain, why reinvent the wheel if we don't have to..

Planned Syntax:

brigadier command /foo:
    permission: some.permission
    subcommand help <page: integer from 1>:
        suggestions:
            return integers between 1 and 5
        trigger:
            # help page
    subcommand bar <user: word>
        suggestions:
            loop {users::*}:
                add "<tooltip:This is a suggestion tooltip>%loop-value%" to {_s::*}
            return {_s::*}
        trigger:
            fooUser(player, {_user})

Existing Work:

• New syntax type ArgumentTypeElement. This represents a command argument with additional options; in sense this is very similar to an expression. Some examples can be seen in DefaultArgumentTypes, e.g., integer from %integer% to %integer% or simple ones like word. Right now, you cannot simply use different class infos as arguments, but I plan on supporting this later. I prefer this approach over a validation section because:
  • It can be more closely integrated with existing Brigadier systems.
  • It takes up less space and is easier to read.
• The syntax for the command structure is fully implemented and mostly works (including command/subcommand parsing). The command structure can also be reused by addons. See StructGeneralCommand and StructBrigadierCommand.
• The implementation for the Paper environment mostly works.

The current work is divided into three different packages:

• org.skriptlang.skript.lang.command - general classes related to syntax, parsing, and the new command API.
• org.skriptlang.skript.brigadier - classes that expand on the Brigadier library.
• ch.njol.skript.command.brigadier - platform-specific (Paper) implementation of the new command API.

Work to be Done:

• Arguments must be named, auto generate names for the unnamed.
• Suggestions (tab-completions); they are mostly implemented but are not yet sent to the client.
• Proper command registration, this includes aliases and the Bukkit help page.
• Input handling; a system for handling incorrect user input (not matching the arguments).
• Convert argument types provided by Paper/Adventure to NMS argument types so they can be sent to the client. This should be much easier than it sounds, but I am still not sure if we want to support this, since it requires some interaction with server (Paper) internals. Currently, only Brigadier command parsers are supported.
• Extra logic is needed for interacting via console. (Server sided the commad still uses Bukkit command API)
• Filter out commands for which the player does not meet the requirements from ever reaching the client. Also, executing such commands now results in a syntax error instead of a permission message. (It is up for debate whether a permission message should even be a thing; vanilla commands have no such messages, and the server acts as if those commands do not exist at all).
• possibly support for class infos as arguments, but that is up to debate
• the command usage entry is not fully implemented
• an invalid executor message is missing in the lang file
• optional arguments are not implemented (the last argument in the command tree could be optional)
• plural arguments are not implemented (the last argument in the command tree could be a vararg), but it is not possible to mimic the old behavior of plural arguments
• command cooldowns are completely unimplemented, I plan on copying the implementation from Commands Rework #7032 sovde did
• Checkstyle once this is nearing completion :D

What will almost surely be lost compared to the old command system:

• Plural arguments in their same shape and form.
• Greedy string arguments; quoted strings will need to be used instead. (If they are not the last argument)

Testing Completed

Only local testing has been performed for now. I need to spend more time with this PR before I can start writing proper tests.

This PR is meant for Paper servers running on 1.21 and above, that's why the existing tests on lower versions might fail.

---

Completes: none
Related: #7032

[Fusezion]

I likely won't review this but I'm more concerned about how this will work, since I'm not a huge fan of the implications this makes.

How exactly are you intending to handle multiple arguments and their suggestions?
Since previously we had blah <arg1> <arg2> <arg...>, and the design above makes me feel like it's desired still but there isn't a way to implement that with your suggestion of "return list".

If anything I would prefer them handled in a way similar to set tab completions of (argument) to <suggestions>
along side implementation of the the suggestion class itself. I believe that should also help with implementing them as paper components later on when that's planned if ever. Versus the existing MessageComponent -> SpigotComponent -> PaperComponnt if I read the code correctly.

I also don't see anything like sub arguments/commands of sub commands
Using existing brigadier api from paper or command api I have
/booster (start|end|give) (personalglobal) (booster type)
personal and global are literal arguments as it prevents usage of any other type of string
Are you planning to implement additional sub command support to existing sub commands?

[Pesekjak]

Thanks for the feedback, let me clear up some things. :)

How exactly are you intending to handle multiple arguments and their suggestions? Since previously we had blah <arg1> <arg2> <arg...>, and the design above makes me feel like it's desired still but there isn't a way to implement that with your suggestion of "return list".

Correct, if you structure the command as

command /foo <first: word> <second: word> <third: word>:
    suggestions:
        # something

the suggestions will apply only for the last node (argument third). If it is desired to add suggestions for each of the arguments, it can be done as so:

command /foo:
    suggestions:
            # suggestions for first
    subcommand <first: word>:
        suggestions:
            # suggestions for second
        subcommand <second: word>:
            suggestions:
                # suggestions for third
            subcommand <third: word>:

But it is good to note that the command nodes themselves have own suggestion logic, for command such as

command /foo:
    subcommand bar
    subcommand world

both bar and world get suggested without any extra code. I agree the design is now a bit bloated, maybe introduction of custom argument types with suggestions providers would be good.

If anything I would prefer them handled in a way similar to set tab completions of (argument) to <suggestions> along side implementation of the the suggestion class itself. I believe that should also help with implementing them as paper components later on when that's planned if ever. Versus the existing MessageComponent -> SpigotComponent -> PaperComponnt if I read the code correctly.

Yes, MessageComponent -> SpigotComponent -> PaperComponent -> Suggestion. Suggestions can only be plain text with a component tooltip, I felt like using existing MessageComponent fits well here. Possible improvement would be to convert MessageComponent directly to Suggestion, but I think introducing Suggestion as a separate type is too specific.

I also don't see anything like sub arguments/commands of sub commands Using existing brigadier api from paper or command api I have /booster (start|end|give) (personalglobal) (booster type) personal and global are literal arguments as it prevents usage of any other type of string Are you planning to implement additional sub command support to existing sub commands?

Subcommands can already have other subcommands if that is the question, SubCommandEntryData can be nested.

[sovdeeth]

looks like a great start!
I'm sure it's in progress but just a reminder to add a bunch more javadocs to stuff, too.

[sovdeeth]

Any reason these files are still in ch/njo/skript?

[Pesekjak]

I wanted to keep the platform implementation separate and thought putting it next to the current command system was better choice. I will change it to the org/skriptlang/skript package then

[sovdeeth]

Yeah you can just make a bukkit/paper package for it (like the previous pr did iirc)

[sovdeeth]

Suggested change

	"[single] (word|string)", "[quotable|quoted] string", "greedy string");
	"[single] (word|string)", "(quotable|quoted) string", "greedy string");

[sovdeeth]

i'd like to see some more friendly versions like integer greater than x or integer less than or equal to y
It's also unsafe to get the values of expressions without events, so these would need to be literals or parsed/evaluated with a proper event.

I get why you prefer this over validation sections and it does seem more convenient, but I feel it also adds unnecessary duplication when we could instead leverage existing syntaxes and give users more control using validation sections. I've mixed feelings on it.

[Pesekjak]

i'd like to see some more friendly versions like integer greater than x or integer less than or equal to y

I agree, the syntax was more for demonstration; I will change this later

It's also unsafe to get the values of expressions without events, so these would need to be literals or parsed/evaluated with a proper event.

The argument parser does not parse expressions, only literals. I assume that should be safe, but correct me if I am wrong.

I get why you prefer this over validation sections and it does seem more convenient, but I feel it also adds unnecessary duplication when we could instead leverage existing syntaxes and give users more control using validation sections. I've mixed feelings on it.

One of the strong points for this was the integration with brigadier (e.g., client can predict when the user input is outside the integer range before the command is sent). I suppose we could have a mix of both, but I am really for the current syntax and I am not sure if having both wouldn't be too bloated.

[sovdeeth]

The argument parser does not parse expressions, only literals. I assume that should be safe, but correct me if I am wrong.

ok yeah if it's only literals you should cast it to literal and just use get() instead of get(Event) imo

And I'm fine with having a mix of both, but validation sections/similar ideas don't need to be a priority, since users can just write them themselves in the trigger.

[sovdeeth]

Actually, for future-proofing for contextless expr, just evaluate with ContextlessEvent.get(), don't cast

[sovdeeth]

i'm not really sure about this being in the Skript class.