Brigadier: Incorrect serialization of same-name command nodes causing client to suggest invalid literals

[Rickyboy320]

Expected behavior

With the following snippet:

this.getLifecycleManager().registerEventHandler(LifecycleEvents.COMMANDS, event -> {
            event.registrar().register(
                    literal("hide")
                            .executes(this::hide)
                            .then(literal("something-else")
                                    .executes(this::hideSomethingElse)).build());

            event.registrar().register(
                    literal("chat")
                            .then(literal("hide")
                                    .executes(this::chatHide)).build());
        });

One would expect the client to suggest the following commands, and it does:
hide
hide something-else
chat hide

I expect no other hide/chat related commands to be suggested.

Observed/Actual behavior

Interestingly, I found that the client also suggests

chat hide something-else

which is of course not a valid command:

So why does the client suggest it? (See analysis below)

Steps/models to reproduce

this.getLifecycleManager().registerEventHandler(LifecycleEvents.COMMANDS, event -> {
            event.registrar().register(
                    literal("hide")
                            .executes(this::hide)
                            .then(literal("something-else")
                                    .executes(this::hideSomethingElse)).build());

            event.registrar().register(
                    literal("chat")
                            .then(literal("hide")
                                    .executes(this::chatHide)).build());
        });

Plugin and Datapack List

Only the example plugin to enable the above snippet.

Paper version

This server is running Paper version 1.21.3-48-master@817550c (2024-11-22T12:02:23Z) (Implementing API version 1.21.3-R0.1-SNAPSHOT)
You are running the latest version
Previous version: 1.21.1-132-b48403b (MC: 1.21.1)

Also on 1.21 build 132 (previous version)

Other

The direct cause of the problem is that ClientboundCommandsPacket maps the two hide nodes into one, because it thinks they are equal (equals() of both hide nodes returns true! and hashcode is probably equivalent as well). This is usually normal and correct behaviour, but these nodes really should not be considered equal.

Looking into the equals method of (Literal)CommandNode, we see that children and command are checked for equality. But from our example, command should be different for both. At serialization time, however, it is equal because Commands#fillUsableCommands overwrites the to-be-serialized nodes with a local lambda.

Chat-hide node:

Hide node:

Note the command field being Commands$lambda@29095 for both nodes. Since our command structure does not add any children to the hides, the equals method returns true, because the command is equal.

My suggestion is altering the Commands#fillUsableCommands method from

argumentbuilder.executes((commandcontext) -> {
  return 0;
});

to, for example, return a unique Command every time:

argumentbuilder.executes(new Command<CommandSourceStack>() {
    @Override
    public int run(CommandContext<CommandSourceStack> commandContext) throws CommandSyntaxException {
        return 0;
    }
});

Example_patch.patch

This could prevent reuse of nodes in the serialized version at all because equals() will now differ for all command nodes. So maybe a more proper solution is to track a mapping from original command -> new command (or just keep the original command in? Why would it be overwritten?)

There likely is also a related issue with redirect not being used in the check for equality nor the hashcode, causing two otherwise identical nodes that redirect to other nodes to be serialized as one and the same - thus potentially redirecting incorrectly on the client. At time of writing I'm unable to make a proof of concept though, because there seems to be a bit of magic going on around redirects.

If this is deemed a suitable fix, I can make a PR (or someone else can with the attached patch, I don't mind)