Arguments in a `CommandContext` disappear when a node redirects #137

willkroboth · 2023-08-31T11:58:47Z

This may well be the intended behavior, but I wanted to check because it feels like a bug. If this is intended, I was wondering what the workaround would be?

I encountered this problem when working generally with this sort of command structure:

(base command) = A node at the end of some CommandNode structure
(final arguments) = Some CommandNode structure

(intermediate branch) = Some CommandNode structure
(branches) = List of (intermediate branch)

(base command) -> (branches) -> (final arguments)

I have some base command path, and I want to add a bunch of branches after that base command node. These branches should converge onto the same final arguments node structure. Abstractly, this is how I achieved that:

baseCommand.addChild(firstBranch)

for the rest of the branches:
    branch.redirect(firstBranch)
    baseCommand.addChild(branch)

// Actually build finalArguments
firstBranch.addChild(finalArguments)

I setup (base command) -> (first branch), then setup (base command) -> (branch) -> redirect to (firstBranch). Then, I add the final arguments onto the first branch. Since the other branches redirect to first branch, their parsing correctly continues on to the final arguments.

It is important to note that the final arguments structure is built after the branches are built. That's why I redirect the other branches to the first branch. With that, I only need to add the final arguments to the first branch, and therefore I only need to keep track of this "representative" branch in my building code.

I only introduce this as context for the bug. If you think my real problem is that I'm building my command wrong, fair enough. I'm happy to go into more depth about my specific situation if the bug is actually intended behavior.

To show this problem, I set up this simple example:

CommandDispatcher<Object> dispatcher = new CommandDispatcher<>();

CommandNode<Object> baseCommand = literal("command").build();

// Add first branch
CommandNode<Object> firstBranchTermination = argument("number", IntegerArgumentType.integer(10, 20)).build();
baseCommand.addChild(literal("high").then(firstBranchTermination).build());

// Redirect other branches to first branch
CommandNode<Object> otherBranch = argument("number", IntegerArgumentType.integer(0, 10)).redirect(firstBranchTermination).build();
baseCommand.addChild(literal("low").then(otherBranch).build());

// Actually build final arguments
CommandNode<Object> finalArgument = argument("string", StringArgumentType.greedyString())
        .executes(context -> {
            int number = context.getArgument("number", Integer.class);
            System.out.println("The given number is " + number);

            String stringArgument = context.getArgument("string", String.class);
            System.out.println("The string was given as " + stringArgument);
            return 1;
        })
        .build();

// Add final arguments at the convergence of the branches
firstBranchTermination.addChild(finalArgument);

// Register command
dispatcher.getRoot().addChild(baseCommand);

// Test cases
dispatcher.execute("command high 11 some text", new Object());
dispatcher.execute("command low 5 some text", new Object());

Here's how that connects to my abstract structure before

(base command) = literal("command")
(final arguments) = argument("string", GreedyString) with execution defined

(branches):
    (first branch) = literal("high").then(argument("number", Integer from 10 to 20))
    (other branch) = literal("low").then(argument("number", Integer from 0 to 10))

There is a low branch that only accepts integers from 0 to 10, and a high branch that only accepts integers from 10 to 20. Both of these branches converge on greedy string, which has an executor that uses the given "number" and "string" argument.

So, the outcome I expect from running this code is that the two test cases at the end execute without failure

dispatcher.execute("command high 11 some text", new Object());
dispatcher.execute("command low 5 some text", new Object());

The first case that uses the high branch works as expected. However, the low branch case throws this exception:

java.lang.IllegalArgumentException: No such argument 'number' exists on this command
	at com.mojang.brigadier.context.CommandContext.getArgument(CommandContext.java:92)
	at com.mojang.brigadier.CustomTest.lambda$test$0(CustomTest.java:34) // This is the `int number = context.getArgument("number", Integer.class);` line
	at com.mojang.brigadier.CommandDispatcher.execute(CommandDispatcher.java:264)
	at com.mojang.brigadier.CommandDispatcher.execute(CommandDispatcher.java:177)
	at com.mojang.brigadier.CommandDispatcher.execute(CommandDispatcher.java:142)
	at com.mojang.brigadier.CustomTest.test(CustomTest.java:49)

When I use the redirected low branch, the "number" argument is not present in the CommandContext, hence the title of this issue. Even though the "number" argument was parsed as 5, it is not available when the command executes. When a node redirects, the arguments defined before the redirected node disappear.

I think the bug comes from this line in CommandDispatcher (The same problem likely occurs when commands fork due to similar code on line 248):

brigadier/src/main/java/com/mojang/brigadier/CommandDispatcher.java

Line 239 in f20bede

next.add(child.copyFor(context.getSource()));

I don't know the reasoning behind the parsing and execution logic here, but I can comment on what I see happening here. I don't know why, but I can tell you how this bug happens.

When "command low 5 some text" is parsed, the resulting command context looks something like this:

Input: "command low 5 some text"
CommandContext:
    Range 0 to 13 ("command low 5")
    Arguments:
        "number" -> Range 12 to 13 ("5"), result: 5
    Forks: false
    child CommandContext:
        Range 14 to 23 ("some text")
        Arguments:
            "string" -> Range 14 to 23 ("some text"), result: "some text"
        Command: Yes, I have a command to execute

The original context parsed everything up to the redirected node. It maps the "number" argument to its value, 5. When parsing reached the redirect, it started a new child context to parse everything after the redirected node. This context maps the "string" argument to its value, "some text". The child context also knows that it has a command to execute. Again, I don't know why, but this seems reasonable.

Anyway, the line I linked to above is part of code responsible for using this CommandContext to actually execute a command. It looks through all the child contexts and handles all the redirects, especially if the context forks and the executor needs to be multiplied among many sources. The line I specifically linked to is responsible for figuring out the next context in this case with a child context that does not fork. Remember, that line is:

next.add(child.copyFor(context.getSource()));

So, the next context that will be evaluated is the child context, but copied to have the original context's source. So, now the context looks like this:

CommandContext:
    Range 14 to 23 ("some text")
    Arguments:
        "string" -> Range 14 to 23 ("some text"), result: "some text"
    Command: Yes, I have a command to execute

This context ends the chain, and it is executable, so the command is run using the current context. However, the current context only contains the "string" argument, not the "number" argument. The "number" argument only existed in the original context, and it was not copied to the child context. Hence, the "number" argument does not exist while running the command, and I get the IllegalArgumentException.

The main reason why I think this is a bug is because it can be easily fixed. Instead of the current code that just copies the command source:

next.add(child.copyFor(context.getSource()));

Using this line solves my issue:

next.add(child.copyFor(context.getSource()).copyArguments(context));

Where CommandContext#copyArguments is defined like so:

public CommandContext<S> copyArguments(CommandContext<S> context) {
    Map<String, ParsedArgument<S, ?>> newArguments = new HashMap<>();
    newArguments.putAll(context.arguments);
    newArguments.putAll(this.arguments);
    return new CommandContext<>(source, input, newArguments, command, rootNode, nodes, range, child, modifier, forks);
}

It seems likely to me that not copying the parent context's arguments is just a semantics error and not intended behavior.

Due to the lack of documentation for the redirect and fork methods, I'm not sure what behavior to expect. It makes sense to me that prior arguments would be preserved after a redirect, so I assume this is a bug. Looking at the current uses of redirect and fork, I don't think my changes would cause different behavior. For example, Minecraft's /execute command uses redirects to have a looping argument command structure. That means I could reuse the same arguments like so:

/execute as @a as @e[type=!player,limit=1] ...

Here, the "targets" argument is used twice, once as @a and once as @e[type=!player,limit=1]. I assume the CommandContext in this situation would like the following:

CommandContext:
    Source: original sender
    Arguments:
        "entities" -> result: @a
    Forks: true
    child CommandContext:
        Arguments:
            "entities" -> result: @e[type=!player,limit=1]
        Forks: true
        child CommandContext:
            ...

When the original context's fork is being evaluated, "entities" is @a as expected. So, the redirect modifier should correctly select all the players as the next sources for the command. When merging the arguments for the new CommandContext, the child context's arguments take precedence. So, the next context will look like this:

CommandContext:
    Source: All players
    Arguments:
        "entities" -> result: @e[type=!player,limit=1]
    child CommandContext:
        ...

Since the child context already had a value for "entities", merging arguments from the parent CommandContext doesn't do anything. Therefore, the redirect modifier correctly still processes @e[type=!player,limit=1] as the next selector. Overall, merging the arguments doesn't affect old command structures, since if they expect an argument, it will always override the value possibly given to the parent context.

So, I hope the fact that arguments are not copied to child contexts is a bug, because I think it would solve my problem. If it is a bug, I think it can be easily fixed without affecting old commands. It seems intuitive to me that arguments would be preserved after redirected nodes, but I can't find any documentation that suggests one way or the other.

If this is a bug, I can definitely make a PR for this. I'm not familiar with the code style of this repo yet, but I think I can figure it out with time. The changes shouldn't be too large; I mostly just need to figure out how to create a test for this.

The text was updated successfully, but these errors were encountered:

Precursor work for #483 Notable changes: - No more flattening everything into a CommandAPICommand - CommandTrees are not flattened - CommandMetaData and Execution removed (only used to flatten CommandTrees) - MultiLiteralArgument and subcommands are no longer converted into LiteralArguments - Optional arguments are not chopped up into multiple arrays - In general, a single call to `ExecutableCommand#register` now creates the entire Brigadier tree for that command instance - Should only be converting Arguments to Brigadier nodes once instead of multiple times - CommandAPIHandler no longer creates Brigadier nodes - Delegated to AbstractCommandAPICommand, AbstractCommandTree, AbstractArgumentTree.java, and AbstractArgument - Problems that prevent a command from being registered have been unified under CommandRegistrationException - Exception messages tweaked - Bukkit-specific features removed from `commandapi-core` - Removed `isConverted` property of CommandAPICommand - Moved logic for flattening EntitySelectorArgument from `CommandAPIHandler#generateBrigadierCommand` to Converter - Moved special Bukkit command permission stuff into CommandAPIBukkit - Taking advantage of Brigadier redirects - Command alias nodes redirect to main command name - MultiLiteralArgument literal nodes use redirects - Instead of duplicating the argument node structure after these arguments, they can reference the same path TODO: - The checks performed by `CommandAPIHandler#hasCommandConflict` have not been reimplemented (I'm not sure if they did anything?) - MultiLiteralArgument is currently broken. See Mojang/brigadier#137 - Many changes not covered by tests (could be sneaky behavioral differences)

willkroboth · 2023-12-24T21:16:18Z

Ah, I found my workaround.

I didn't realize before that the CommandDispatcher nodes don't have to form a tree. A child can have multiple parents. So, my test example can work like so:

CommandDispatcher<Object> dispatcher = new CommandDispatcher<>();

CommandNode<Object> baseCommand = literal("command").build();

// Add first branch
CommandNode<Object> firstBranchTermination = argument("number", IntegerArgumentType.integer(10, 20)).build();
baseCommand.addChild(literal("high").then(firstBranchTermination).build());

// Redirect other branches to first branch
CommandNode<Object> otherBranchTermination = argument("number", IntegerArgumentType.integer(0, 10)).build();
baseCommand.addChild(literal("low").then(otherBranchTermination).build());

// Actually build final arguments
CommandNode<Object> finalArgument = argument("string", StringArgumentType.greedyString())
        .executes(context -> {
            int number = context.getArgument("number", Integer.class);
            System.out.println("The given number is " + number);

            String stringArgument = context.getArgument("string", String.class);
            System.out.println("The string was given as " + stringArgument);
            return 1;
        })
        .build();

// Add final arguments to both branches
firstBranchTermination.addChild(finalArgument);
otherBranchTermination.addChild(finalArgument);

// Register command
dispatcher.getRoot().addChild(baseCommand);

// Test cases
dispatcher.execute("command high 11 some text", new Object());
dispatcher.execute("command low 5 some text", new Object());

This method can also work with cycles. At least, suggestions and executing a CommandDispatcher with nodes that cycle seem to work fine. For example, this setup:

LiteralCommandNode<Source> command = LiteralArgumentBuilder.<Source>literal("command").build();

ArgumentCommandNode<Source, String> flagName = RequiredArgumentBuilder
	.<Source, String>argument("flag", StringArgumentType.word()).build();
ArgumentCommandNode<Source, Integer> flagValue = RequiredArgumentBuilder
	.<Source, Integer>argument("value", IntegerArgumentType.integer(0, 255))
	.executes(context -> {
		// Use the flags...
	}).build();

flagName.addChild(flagValue);
flagValue.addChild(flagName);

command.addChild(flagName);

dispatcher.getRoot().addChild(command);

// command looks like `/command <flag> <value> <flag> <value> ...`

In this command, the user can input any number of flag-value pairs. Executing the command is possible whenever they have typed in a value. Brigadier seems to handle this case fine, so nothing is lost from using cycling children rather than redirects.

However, as #105 mentions, Minecraft does not support cycles when it sends commands to the player in a packet. At least on a Paper server, trying to send a commands packet containing cycling children causes a stack overflow. While the format of the commands packet does not seem to prohibit a structure like this, the server code and I assume the Vanilla Minecraft client reject this setup. So, if you want to include argument cycles in your Minecraft command (and I do :P), you can't use redirects since the arguments disappear, and you can't use cycling children because the Vanilla client says no.

However, I have found a hacky workaround. The trick is that when Minecraft builds the Commands packet, it uses the map CommandNode.children. However, when Brigadier parses a command, it uses the map CommandNode.arguments. Usually, these maps are synced up, but the magic of reflection allows you to do (almost) whatever you want. This is what I did:

LiteralCommandNode<Source> command = LiteralArgumentBuilder.<Source>literal("command").build();

ArgumentCommandNode<Source, String> flagName = RequiredArgumentBuilder
	.<Source, String>argument("flag", StringArgumentType.word()).build();
ArgumentCommandNode<Source, Integer> flagValue = RequiredArgumentBuilder
	.<Source, Integer>argument("value", IntegerArgumentType.integer(0, 255))
		// Use the flags...
	})
	.redirect(command)
	.build();

ArgumentCommandNode<Source, Integer> flagValueCopy = flagValue.createBuilder().redirect(null).build();
flagValueCopy.addChild(flagName);

Map<String, CommandNode<Source>> children;
Map<String, ArgumentCommandNode<Source, ?>> arguments;
try {
	Field commandNodeChildren = CommandNode.class.getDeclaredField("children");
	commandNodeChildren.setAccessible(true);
	children = (Map<String, CommandNode<Source>>) commandNodeChildren.get(flagName);

	Field commandNodeArguments = CommandNode.class.getDeclaredField("arguments");
	commandNodeArguments.setAccessible(true);
	arguments = (Map<String, ArgumentCommandNode<Source, ?>>) commandNodeArguments.get(flagName);
} catch (ReflectiveOperationException exception) {
	throw new RuntimeException(exception);
}

children.put(flagValue.getName(), flagValue);
arguments.put(flagValueCopy.getName(), flagValueCopy);

command.addChild(flagName);

platform.getBrigadierDispatcher().getRoot().addChild(command);

Based on the children map, the command looks like: /command <name> <value> -> command. So, when the server sends the command to a client, it gives them a redirect cycle. The client asks the server to execute commands, so it doesn't need to worry about commands disappearing after redirects, avoiding the issue I've presented here.

However, using reflection, I put a different node into the arguments map. This way, the command looks like /command <name> <value> <name> <value>.... So, when the server executes the command for the player, it uses a child cycle. That avoids the issue here where arguments disappear after redirects, and so a command can access all the arguments given (with just a bit more finagling that I've omitted).

So, it is possible to work around this issue. However, I don't think any solution involving reflection should be official :P. It would still be great to solve this issue via #142, or something else like that.

…ing redirects The Brigadier CommandDispatcher CommandNode structure does not need to be a tree. This means one node can have multiple parents. In this case, the node after a MultiLiteralArgument has parent node for each literal in the MultiLiteral. This works around the issue documented in Mojang/brigadier#137, where arguments disappear from the CommandContext used when executing an argument when the command encounters a redirect. This way, we can remember which literal was chosen no matter which branch was taken, yet still converge on the same path.