Skip to content

Commit

Permalink
Fix MultiLiteralArguments - give nodes multiple parents instead of us…
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@willkroboth
willkroboth committed Feb 28, 2024
1 parent 1ed9e96 commit 7dc59ff
Show file tree
Hide file tree
Showing 8 changed files with 226 additions and 149 deletions.
11 changes: 7 additions & 4 deletions commandapi-core/src/main/java/dev/jorel/commandapi/AbstractArgumentTree.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,15 @@ public List<List<String>> getBranchesAsStrings() {
/**
* Builds the Brigadier {@link CommandNode} structure for this argument tree.
*
* @param previousNode The {@link CommandNode} to add this argument tree onto.
* @param previousNodes A List of {@link CommandNode}s to add this argument onto.
* @param previousArguments A List of CommandAPI arguments that came before this argument tree.
* @param previousNonLiteralArgumentNames A List of Strings containing the node names that came before this argument.
* @param <Source> The Brigadier Source object for running commands.
*/
public <Source> void buildBrigadierNode(CommandNode<Source> previousNode, List<Argument> previousArguments, List<String> previousNonLiteralArgumentNames) {
public <Source> void buildBrigadierNode(
List<CommandNode<Source>> previousNodes,
List<Argument> previousArguments, List<String> previousNonLiteralArgumentNames
) {
// Check preconditions
if (argument instanceof GreedyArgument && !arguments.isEmpty()) {
// Argument is followed by at least some arguments
Expand All @@ -142,15 +145,15 @@ public <Source> void buildBrigadierNode(CommandNode<Source> previousNode, List<A
}

// Create node for this argument
CommandNode<Source> rootNode = argument.addArgumentNodes(previousNode, previousArguments, previousNonLiteralArgumentNames, executor);
previousNodes = argument.addArgumentNodes(previousNodes, previousArguments, previousNonLiteralArgumentNames, executor);

// Add our branches as children to the node
for (AbstractArgumentTree<?, Argument, CommandSender> child : arguments) {
// We need a new list for each branch of the tree
List<Argument> newPreviousArguments = new ArrayList<>(previousArguments);
List<String> newPreviousArgumentNames = new ArrayList<>(previousNonLiteralArgumentNames);

child.buildBrigadierNode(rootNode, newPreviousArguments, newPreviousArgumentNames);
child.buildBrigadierNode(previousNodes, newPreviousArguments, newPreviousArgumentNames);
}
}

Expand Down
6 changes: 3 additions & 3 deletions commandapi-core/src/main/java/dev/jorel/commandapi/AbstractCommandAPICommand.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ protected <Source> void createArgumentNodes(LiteralCommandNode<Source> rootNode)

// Create arguments
if (hasAnyArguments()) {
CommandNode<Source> previousNode = rootNode;
List<CommandNode<Source>> previousNodes = List.of(rootNode);
List<Argument> previousArguments = new ArrayList<>();
List<String> previousArgumentNames = new ArrayList<>();

Expand All @@ -319,15 +319,15 @@ protected <Source> void createArgumentNodes(LiteralCommandNode<Source> rootNode)
// Add required arguments
for (int i = 0; i < requiredArguments.size(); i++) {
Argument argument = requiredArguments.get(i);
previousNode = argument.addArgumentNodes(previousNode, previousArguments, previousArgumentNames,
previousNodes = argument.addArgumentNodes(previousNodes, previousArguments, previousArgumentNames,
// Only the last required argument is executable
i == requiredArguments.size() - 1 ? executor : null);
}

// Add optional arguments
for (Argument argument : optionalArguments) {
// All optional arguments are executable
previousNode = argument.addArgumentNodes(previousNode, previousArguments, previousArgumentNames, executor);
previousNodes = argument.addArgumentNodes(previousNodes, previousArguments, previousArgumentNames, executor);
}

// Check greedy argument constraint
Expand Down
2 changes: 1 addition & 1 deletion commandapi-core/src/main/java/dev/jorel/commandapi/AbstractCommandTree.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ protected <Source> void createArgumentNodes(LiteralCommandNode<Source> rootNode)
List<String> previousNonLiteralArgumentNames = new ArrayList<>();
previousArguments.add(commandNames);

argument.buildBrigadierNode(rootNode, previousArguments, previousNonLiteralArgumentNames);
argument.buildBrigadierNode(List.of(rootNode), previousArguments, previousNonLiteralArgumentNames);
}
}
}
50 changes: 29 additions & 21 deletions commandapi-core/src/main/java/dev/jorel/commandapi/arguments/AbstractArgument.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -356,20 +356,23 @@ public void appendToCommandPaths(List<List<String>> argumentStrings) {
* <li>{@link #checkPreconditions(List, List)}</li>
* <li>{@link #createArgumentBuilder(List, List)}</li>
* <li>{@link #finishBuildingNode(ArgumentBuilder, List, CommandAPIExecutor)}</li>
* <li>{@link #linkNode(CommandNode, CommandNode, List, List, CommandAPIExecutor)}</li>
* <li>{@link #linkNode(List, CommandNode, List, List, CommandAPIExecutor)}</li>
* </ul>
*
* @param previousNode The {@link CommandNode} to add this argument onto.
* @param previousNodes A List of {@link CommandNode}s to add this argument onto.
* @param previousArguments A List of CommandAPI arguments that came before this argument.
* @param previousNonLiteralArgumentNames A List of Strings containing the node names that came before this argument.
* @param terminalExecutor The {@link CommandAPIExecutor} to apply at the end of the node structure.
* This parameter can be null to indicate that this argument should not be
* executable.
* @param <Source> The Brigadier Source object for running commands.
* @return The last node in the Brigadier node structure for this argument.
* @return The list of last nodes in the Brigadier node structure for this argument.
*/
public <Source> CommandNode<Source> addArgumentNodes(CommandNode<Source> previousNode, List<Argument> previousArguments, List<String> previousNonLiteralArgumentNames,
CommandAPIExecutor<CommandSender, AbstractCommandSender<? extends CommandSender>> terminalExecutor) {
public <Source> List<CommandNode<Source>> addArgumentNodes(
List<CommandNode<Source>> previousNodes,
List<Argument> previousArguments, List<String> previousNonLiteralArgumentNames,
CommandAPIExecutor<CommandSender, AbstractCommandSender<? extends CommandSender>> terminalExecutor
) {
CommandAPIHandler<Argument, CommandSender, Source> handler = CommandAPIHandler.getInstance();

// Check preconditions
Expand All @@ -387,10 +390,10 @@ public <Source> CommandNode<Source> addArgumentNodes(CommandNode<Source> previou
CommandNode<Source> rootNode = finishBuildingNode(rootBuilder, previousArguments, terminalExecutor);

// Link node to previous
previousNode = linkNode(previousNode, rootNode, previousArguments, previousNonLiteralArgumentNames, terminalExecutor);
previousNodes = linkNode(previousNodes, rootNode, previousArguments, previousNonLiteralArgumentNames, terminalExecutor);

// Return last node
return previousNode;
// Return last nodes
return previousNodes;
}

/**
Expand Down Expand Up @@ -460,34 +463,39 @@ public <Source> CommandNode<Source> finishBuildingNode(ArgumentBuilder<Source, ?
/**
* Links this argument into the Brigadier {@link CommandNode} structure.
*
* @param previousNode The {@link CommandNode} to add this argument onto.
* @param previousNodes A List of {@link CommandNode}s to add this argument onto.
* @param rootNode The {@link CommandNode} representing this argument.
* @param previousArguments A List of CommandAPI arguments that came before this argument.
* @param previousNonLiteralArgumentNames A List of Strings containing the node names that came before this argument.
* @param terminalExecutor The {@link CommandAPIExecutor} to apply at the end of the node structure.
* This parameter can be null to indicate that this argument should not be
* executable.
* @param <Source> The Brigadier Source object for running commands.
* @return The last node in the Brigadier {@link CommandNode} structure representing this Argument. Note that this
* is not necessarily the {@code rootNode} for this argument, since the Brigadier node structure may contain multiple
* nodes. This might happen if {@link #combineWith(AbstractArgument[])} was called for this argument to merge it with
* other arguments.
* @return The list of last nodes in the Brigadier {@link CommandNode} structure representing this Argument. Note that
* this is not necessarily the {@code rootNode} for this argument, since the Brigadier node structure may contain multiple
* nodes in a chain. This might happen if {@link #combineWith(AbstractArgument[])} was called for this argument to merge
* it with other arguments.
*/
public <Source> CommandNode<Source> linkNode(CommandNode<Source> previousNode, CommandNode<Source> rootNode, List<Argument> previousArguments, List<String> previousNonLiteralArgumentNames,
CommandAPIExecutor<CommandSender, AbstractCommandSender<? extends CommandSender>> terminalExecutor) {
// Add rootNode to the previous
previousNode.addChild(rootNode);
public <Source> List<CommandNode<Source>> linkNode(
List<CommandNode<Source>> previousNodes, CommandNode<Source> rootNode,
List<Argument> previousArguments, List<String> previousNonLiteralArgumentNames,
CommandAPIExecutor<CommandSender, AbstractCommandSender<? extends CommandSender>> terminalExecutor
) {
// Add rootNode to the previous nodes
for(CommandNode<Source> previousNode : previousNodes) {
previousNode.addChild(rootNode);
}

// Add combined arguments
previousNode = rootNode;
previousNodes = List.of(rootNode);
for (int i = 0; i < combinedArguments.size(); i++) {
Argument subArgument = combinedArguments.get(i);
previousNode = subArgument.addArgumentNodes(previousNode, previousArguments, previousNonLiteralArgumentNames,
previousNodes = subArgument.addArgumentNodes(previousNodes, previousArguments, previousNonLiteralArgumentNames,
// Only apply the `terminalExecutor` to the last argument
i == combinedArguments.size() - 1 ? terminalExecutor : null);
}

// Return last node
return previousNode;
// Return last nodes
return previousNodes;
}
}
37 changes: 22 additions & 15 deletions commandapi-core/src/main/java/dev/jorel/commandapi/arguments/MultiLiteral.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,44 +112,51 @@ default void appendToCommandPaths(List<List<String>> argumentStrings) {
}

/**
* Overrides {@link AbstractArgument#linkNode(CommandNode, CommandNode, List, List, CommandAPIExecutor)}.
* Overrides {@link AbstractArgument#linkNode(List, CommandNode, List, List, CommandAPIExecutor)}.
* <p>
* Normally, Arguments are only represented by a single node, and so only need to link one node. However, a MultiLiteral
* represents multiple literal node paths, which also need to be generated and inserted into the node structure.
*/
default <Source> CommandNode<Source> linkNode(CommandNode<Source> previousNode, CommandNode<Source> rootNode, List<Argument> previousArguments, List<String> previousNonLiteralArgumentNames,
CommandAPIExecutor<CommandSender, AbstractCommandSender<? extends CommandSender>> terminalExecutor) {
default <Source> List<CommandNode<Source>> linkNode(
List<CommandNode<Source>> previousNodes, CommandNode<Source> rootNode,
List<Argument> previousArguments, List<String> previousNonLiteralArgumentNames,
CommandAPIExecutor<CommandSender, AbstractCommandSender<? extends CommandSender>> terminalExecutor
) {
List<CommandNode<Source>> newNodes = new ArrayList<>();
// Add root node to previous
for(CommandNode<Source> previousNode : previousNodes) {
previousNode.addChild(rootNode);
}
newNodes.add(rootNode);

// Generate nodes for other literals
Iterator<String> literals = Arrays.asList(getLiterals()).iterator();
literals.next(); // Skip first literal; that was handled by `#createArgumentBuilder`
while (literals.hasNext()) {
// Create node
MultiLiteralArgumentBuilder<Source> literalBuilder = MultiLiteralArgumentBuilder.multiLiteral(getNodeName(), literals.next());

// Redirect to root node so all its arguments come after this
literalBuilder.redirect(rootNode);

// Finish building node
CommandNode<Source> literalNode = finishBuildingNode(literalBuilder, previousArguments, terminalExecutor);

// Link node to previous
previousNode.addChild(literalNode);
// Add node to previous
for(CommandNode<Source> previousNode : previousNodes) {
previousNode.addChild(literalNode);
}
newNodes.add(literalNode);
}

// Add root node to previous
previousNode.addChild(rootNode);

// Add combined arguments
previousNode = rootNode;
previousNodes = newNodes;
List<Argument> combinedArguments = getCombinedArguments();
for (int i = 0; i < combinedArguments.size(); i++) {
Argument subArgument = combinedArguments.get(i);
previousNode = subArgument.addArgumentNodes(previousNode, previousArguments, previousNonLiteralArgumentNames,
previousNodes = subArgument.addArgumentNodes(previousNodes, previousArguments, previousNonLiteralArgumentNames,
// Only apply the `terminalExecutor` to the last argument
i == combinedArguments.size() - 1 ? terminalExecutor : null);
}

// Return last node
return previousNode;
// Return last nodes
return previousNodes;
}
}
4 changes: 2 additions & 2 deletions ...andapi-bukkit-core/src/main/java/dev/jorel/commandapi/arguments/MultiLiteralArgument.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ public void appendToCommandPaths(List<List<String>> argumentStrings) {
}

@Override
public <Source> CommandNode<Source> linkNode(CommandNode<Source> previousNode, CommandNode<Source> rootNode, List<Argument<?>> previousArguments, List<String> previousNonLiteralArgumentNames, CommandAPIExecutor<CommandSender, AbstractCommandSender<? extends CommandSender>> terminalExecutor) {
return MultiLiteral.super.linkNode(previousNode, rootNode, previousArguments, previousNonLiteralArgumentNames, terminalExecutor);
public <Source> List<CommandNode<Source>> linkNode(List<CommandNode<Source>> previousNodes, CommandNode<Source> rootNode, List<Argument<?>> previousArguments, List<String> previousNonLiteralArgumentNames, CommandAPIExecutor<CommandSender, AbstractCommandSender<? extends CommandSender>> terminalExecutor) {
return MultiLiteral.super.linkNode(previousNodes, rootNode, previousArguments, previousNonLiteralArgumentNames, terminalExecutor);
}
}
Loading

0 comments on commit 7dc59ff

Please sign in to comment.