hollow-cube/command-parser-fixes (#54)

* Rewrite CommandParserImpl recursively (fix #1327)

* Fix for tests: CommandManagerTest, CommandParseTest

* Make attributes final inside Chain

* fix #1295 with argument type priority

* Don't include command name in list of arguments

* Add test for #1327

* Add test for #1295

* Fix suggestions with bad syntax

* Fix #1916

* Add test for #1916

* add failing test

* mess with arg order

* Fix `GraphImpl` causing syntax order issues

---------

Co-authored-by: Spanner <spanner77@protonmail.com>
This commit is contained in:
Matt Worzala 2023-08-30 19:04:11 -04:00 committed by GitHub
parent 0e8151150a
commit e9d0098418
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 291 additions and 92 deletions

View File

@ -58,6 +58,7 @@ public class Main {
commandManager.register(new RedirectTestCommand());
commandManager.register(new DisplayCommand());
commandManager.register(new NotificationCommand());
commandManager.register(new TestCommand2());
commandManager.setUnknownCommandCallback((sender, command) -> sender.sendMessage(Component.text("Unknown command", NamedTextColor.RED)));

View File

@ -136,6 +136,14 @@ public class PlayerInit {
})
.addListener(PlayerBlockPlaceEvent.class, event -> {
// event.setDoBlockUpdates(false);
})
.addListener(PlayerBlockInteractEvent.class, event -> {
var block = event.getBlock();
var rawOpenProp = block.getProperty("open");
if (rawOpenProp == null) return;
block = block.withProperty("open", String.valueOf(!Boolean.parseBoolean(rawOpenProp)));
event.getInstance().setBlock(event.getBlockPosition(), block);
});
static {

View File

@ -0,0 +1,20 @@
package net.minestom.demo.commands;
import net.minestom.server.command.builder.Command;
import net.minestom.server.command.builder.arguments.ArgumentType;
public class TestCommand2 extends Command {
public TestCommand2() {
super("test2");
var argA = ArgumentType.String("a");
var argB = ArgumentType.String("b");
addSyntax((sender, context) -> {
sender.sendMessage("a only");
}, argA);
addSyntax((sender, context) -> {
sender.sendMessage("a and b");
}, argB, argA);
}
}

View File

@ -18,7 +18,6 @@ import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.function.Function;
import java.util.function.Supplier;
import java.util.stream.Collectors;
final class CommandParserImpl implements CommandParser {
@ -27,6 +26,7 @@ final class CommandParserImpl implements CommandParser {
static final class Chain {
CommandExecutor defaultExecutor = null;
SuggestionCallback suggestionCallback = null;
final ArrayDeque<NodeResult> nodeResults = new ArrayDeque<>();
final List<CommandCondition> conditions = new ArrayList<>();
final List<CommandExecutor> globalListeners = new ArrayList<>();
@ -60,81 +60,60 @@ final class CommandParserImpl implements CommandParser {
return (sender, context) -> globalListeners.forEach(x -> x.apply(sender, context));
}
SuggestionCallback extractSuggestionCallback() {
return nodeResults.peekLast().callback;
}
Map<String, ArgumentResult<Object>> collectArguments() {
return nodeResults.stream()
.skip(1) // skip root
.skip(2) // skip root node and command
.collect(Collectors.toUnmodifiableMap(NodeResult::name, NodeResult::argumentResult));
}
List<Argument<?>> getArgs() {
return nodeResults.stream().map(x -> x.node.argument()).collect(Collectors.toList());
}
Chain() {}
Chain(CommandExecutor defaultExecutor,
SuggestionCallback suggestionCallback,
ArrayDeque<NodeResult> nodeResults,
List<CommandCondition> conditions,
List<CommandExecutor> globalListeners) {
this.defaultExecutor = defaultExecutor;
this.suggestionCallback = suggestionCallback;
this.nodeResults.addAll(nodeResults);
this.conditions.addAll(conditions);
this.globalListeners.addAll(globalListeners);
}
Chain fork() {
return new Chain(defaultExecutor, suggestionCallback, nodeResults, conditions, globalListeners);
}
}
@Override
public @NotNull CommandParser.Result parse(@NotNull CommandSender sender, @NotNull Graph graph, @NotNull String input) {
final CommandStringReader reader = new CommandStringReader(input);
final Chain chain = new Chain();
// Read from input
NodeResult result;
Chain chain = new Chain();
Node parent = graph.root();
while ((result = parseChild(sender, parent, reader)) != null) {
chain.append(result);
if (result.argumentResult instanceof ArgumentResult.SyntaxError<?> e) {
// Syntax error stop at this arg
final ArgumentCallback argumentCallback = parent.argument().getCallback();
if (argumentCallback == null && chain.defaultExecutor != null) {
return ValidCommand.defaultExecutor(input, chain);
} else {
return new InvalidCommand(input, chain.mergedConditions(),
argumentCallback, e, chain.collectArguments(), chain.mergedGlobalExecutors(),
chain.extractSuggestionCallback(), chain.getArgs());
}
}
parent = result.node;
NodeResult result = parseNode(sender, parent, chain, reader);
chain = result.chain;
if (result.argumentResult instanceof ArgumentResult.Success<?>) {
NodeResult lastNodeResult = chain.nodeResults.peekLast();
Node lastNode = lastNodeResult.node;
CommandExecutor executor = nullSafeGetter(lastNode.execution(), Graph.Execution::executor);
if (executor != null) return ValidCommand.executor(input, chain, executor);
}
// Check children for arguments with default values
do {
Node tmp = parent;
parent = null;
for (Node child : tmp.next()) {
final Argument<?> argument = child.argument();
final Function<CommandSender, ?> defaultSupplier = argument.getDefaultValue();
if (defaultSupplier != null) {
final Object value = defaultSupplier.apply(sender);
final ArgumentResult<Object> argumentResult = new ArgumentResult.Success<>(value, "");
chain.append(new NodeResult(child, argumentResult, argument.getSuggestionCallback()));
parent = child;
break;
}
}
} while (parent != null);
// Check if any syntax has been found
final NodeResult lastNode = chain.nodeResults.peekLast();
if (lastNode == null) return UnknownCommandResult.INSTANCE;
// Verify syntax(s)
final CommandExecutor executor = nullSafeGetter(lastNode.node().execution(), Graph.Execution::executor);
if (executor == null) {
// Syntax error
if (chain.defaultExecutor != null) {
return ValidCommand.defaultExecutor(input, chain);
} else {
return InvalidCommand.invalid(input, chain);
}
// If here, then the command failed or didn't have an executor
// Look for a default executor, or give up if we got nowhere
NodeResult lastNode = chain.nodeResults.peekLast();
if (lastNode.node.equals(parent)) return UnknownCommandResult.INSTANCE;
if (chain.defaultExecutor != null) {
return ValidCommand.defaultExecutor(input, chain);
}
if (reader.hasRemaining()) {
// Command had trailing data
if (chain.defaultExecutor != null) {
return ValidCommand.defaultExecutor(input, chain);
} else {
return InvalidCommand.invalid(input, chain);
}
}
return ValidCommand.executor(input, chain, executor);
return InvalidCommand.invalid(input, chain);
}
@Contract("null, _ -> null; !null, null -> fail; !null, !null -> _")
@ -142,32 +121,90 @@ final class CommandParserImpl implements CommandParser {
return obj == null ? null : getter.apply(obj);
}
private static NodeResult parseChild(@NotNull CommandSender sender, Node parent, CommandStringReader reader) {
if (!reader.hasRemaining()) return null;
for (Node child : parent.next()) {
final Argument<?> argument = child.argument();
final int start = reader.cursor();
final ArgumentResult<?> parse = parse(sender, argument, reader);
if (parse instanceof ArgumentResult.Success<?> success) {
return new NodeResult(child, (ArgumentResult<Object>) success,
argument.getSuggestionCallback());
} else if (parse instanceof ArgumentResult.SyntaxError<?> syntaxError) {
return new NodeResult(child, (ArgumentResult<Object>) syntaxError,
argument.getSuggestionCallback());
private static NodeResult parseNode(@NotNull CommandSender sender, Node node, Chain chain, CommandStringReader reader) {
chain = chain.fork();
Argument<?> argument = node.argument();
int start = reader.cursor();
if (reader.hasRemaining()) {
ArgumentResult<?> result = parseArgument(sender, argument, reader);
SuggestionCallback suggestionCallback = argument.getSuggestionCallback();
NodeResult nodeResult = new NodeResult(node, chain, (ArgumentResult<Object>) result, suggestionCallback);
chain.append(nodeResult);
if (suggestionCallback != null) chain.suggestionCallback = suggestionCallback;
if (chain.nodeResults.size() == 1) { // If this is the root node (usually "Literal<>")
reader.cursor(start);
} else {
// Reset cursor & try next
if (!(result instanceof ArgumentResult.Success<?>)) {
reader.cursor(start);
return nodeResult;
}
}
} else {
// Nothing left, yet we're still being asked to parse? There must be defaults then
Function<CommandSender, ?> defaultSupplier = node.argument().getDefaultValue();
if (defaultSupplier != null) {
Object value = defaultSupplier.apply(sender);
ArgumentResult<Object> argumentResult = new ArgumentResult.Success<>(value, "");
chain.append(new NodeResult(node, chain, argumentResult, argument.getSuggestionCallback()));
// Add the default to the chain, and then carry on dealing with this node
} else {
// Still being asked to parse yet there's nothing left, syntax error.
return new NodeResult(
node,
chain,
new ArgumentResult.SyntaxError<>("Not enough arguments","",-1),
argument.getSuggestionCallback()
);
}
}
// Successfully matched this node's argument
start = reader.cursor();
if (!reader.hasRemaining()) start--; // This is needed otherwise the reader throws an AssertionError
NodeResult error = null;
for (Node child : node.next()) {
NodeResult childResult = parseNode(sender, child, chain, reader);
if (childResult.argumentResult instanceof ArgumentResult.Success<Object>) {
// Assume that there is only one successful node for a given chain of arguments
return childResult;
} else {
if (error == null) {
// If this is the base argument (e.g. "teleport" in /teleport) then
// do not report an argument to be incompatible, since the more
// correct thing would be to say that the command is unknown.
if (!(childResult.chain.nodeResults.size() == 2 && childResult.argumentResult instanceof ArgumentResult.IncompatibleType<?>)) {
error = childResult;
}
}
reader.cursor(start);
}
}
for (Node node : parent.next()) {
final SuggestionCallback suggestionCallback = node.argument().getSuggestionCallback();
if (suggestionCallback != null) {
return new NodeResult(parent,
new ArgumentResult.SyntaxError<>("None of the arguments were compatible, but a suggestion callback was found.", "", -1),
suggestionCallback);
// None were successful. Either incompatible types, or syntax error. It doesn't matter to us, though
// Try to execute this node
CommandExecutor executor = nullSafeGetter(node.execution(), Graph.Execution::executor);
if (executor == null) {
// Stuck here with no executor
if (error != null) {
return error;
} else {
return chain.nodeResults.peekLast();
}
}
return null;
if (reader.hasRemaining()) {
// Trailing data is a syntax error
return new NodeResult(
node,
chain,
new ArgumentResult.SyntaxError<>("Command has trailing data", "", -1),
argument.getSuggestionCallback()
);
}
// Command was successful!
return chain.nodeResults.peekLast();
}
record UnknownCommandResult() implements Result.UnknownCommand {
@ -222,7 +259,7 @@ final class CommandParserImpl implements CommandParser {
return new InvalidCommand(input, chain.mergedConditions(),
null/*todo command syntax callback*/,
new ArgumentResult.SyntaxError<>("Command has trailing data.", null, -1),
chain.collectArguments(), chain.mergedGlobalExecutors(), chain.extractSuggestionCallback(), chain.getArgs());
chain.collectArguments(), chain.mergedGlobalExecutors(), chain.suggestionCallback, chain.getArgs());
}
@Override
@ -238,12 +275,12 @@ final class CommandParserImpl implements CommandParser {
static ValidCommand defaultExecutor(String input, Chain chain) {
return new ValidCommand(input, chain.mergedConditions(), chain.defaultExecutor, chain.collectArguments(),
chain.mergedGlobalExecutors(), chain.extractSuggestionCallback(), chain.getArgs());
chain.mergedGlobalExecutors(), chain.suggestionCallback, chain.getArgs());
}
static ValidCommand executor(String input, Chain chain, CommandExecutor executor) {
return new ValidCommand(input, chain.mergedConditions(), executor, chain.collectArguments(), chain.mergedGlobalExecutors(),
chain.extractSuggestionCallback(), chain.getArgs());
chain.suggestionCallback, chain.getArgs());
}
@Override
@ -321,7 +358,7 @@ final class CommandParserImpl implements CommandParser {
static final ExecutableCommand.Result INVALID_SYNTAX = new ExecutionResultImpl(Type.INVALID_SYNTAX, null);
}
private record NodeResult(Node node, ArgumentResult<Object> argumentResult, SuggestionCallback callback) {
private record NodeResult(Node node, Chain chain, ArgumentResult<Object> argumentResult, SuggestionCallback callback) {
public String name() {
return node.argument().getId();
}
@ -372,7 +409,7 @@ final class CommandParserImpl implements CommandParser {
// ARGUMENT
private static <T> ArgumentResult<T> parse(@NotNull CommandSender sender, Argument<T> argument, CommandStringReader reader) {
private static <T> ArgumentResult<T> parseArgument(@NotNull CommandSender sender, Argument<T> argument, CommandStringReader reader) {
// Handle specific type without loop
try {
// Single word argument

View File

@ -4,6 +4,8 @@ import net.minestom.server.command.builder.Command;
import net.minestom.server.command.builder.CommandExecutor;
import net.minestom.server.command.builder.CommandSyntax;
import net.minestom.server.command.builder.arguments.Argument;
import net.minestom.server.command.builder.arguments.ArgumentCommand;
import net.minestom.server.command.builder.arguments.ArgumentLiteral;
import net.minestom.server.command.builder.condition.CommandCondition;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
@ -62,6 +64,12 @@ record GraphImpl(NodeImpl root) implements Graph {
}
record NodeImpl(Argument<?> argument, ExecutionImpl execution, List<Graph.Node> next) implements Graph.Node {
NodeImpl(Argument<?> argument, ExecutionImpl execution, List<Graph.Node> next) {
this.argument = argument;
this.execution = execution;
this.next = next.stream().sorted(nodePriority).toList();
}
static NodeImpl fromBuilder(BuilderImpl builder) {
final List<BuilderImpl> children = builder.children;
Node[] nodes = new NodeImpl[children.size()];
@ -76,6 +84,17 @@ record GraphImpl(NodeImpl root) implements Graph {
static NodeImpl rootCommands(Collection<Command> commands) {
return ConversionNode.rootConv(commands).toNode();
}
private static final java.util.Comparator<Node> nodePriority = (node1, node2) -> {
int node1Value = argumentValue(node1.argument());
int node2Value = argumentValue(node2.argument());
return Integer.compare(node1Value, node2Value);
};
private static int argumentValue(Argument<?> argument) {
if (argument.getClass() == ArgumentCommand.class) return -3000;
if (argument.getClass() == ArgumentLiteral.class) return -2000;
return -1000;
}
}
record ExecutionImpl(Predicate<CommandSender> predicate,
@ -113,8 +132,17 @@ record GraphImpl(NodeImpl root) implements Graph {
}
}
private record ConversionNode(Argument<?> argument, ExecutionImpl execution,
Map<Argument<?>, ConversionNode> nextMap) {
private static final class ConversionNode {
final Argument<?> argument;
ExecutionImpl execution;
final Map<Argument<?>, ConversionNode> nextMap;
public ConversionNode(Argument<?> argument, ExecutionImpl execution, Map<Argument<?>, ConversionNode> nextMap) {
this.argument = argument;
this.execution = execution;
this.nextMap = nextMap;
}
ConversionNode(Argument<?> argument, ExecutionImpl execution) {
this(argument, execution, new LinkedHashMap<>());
}
@ -133,10 +161,9 @@ record GraphImpl(NodeImpl root) implements Graph {
ConversionNode syntaxNode = root;
for (Argument<?> arg : syntax.getArguments()) {
boolean last = arg == syntax.getArguments()[syntax.getArguments().length - 1];
syntaxNode = syntaxNode.nextMap.computeIfAbsent(arg, argument -> {
var ex = last ? ExecutionImpl.fromSyntax(syntax) : null;
return new ConversionNode(argument, ex);
});
var ex = last ? ExecutionImpl.fromSyntax(syntax) : null;
syntaxNode = syntaxNode.nextMap.computeIfAbsent(arg, argument -> new ConversionNode(argument, ex));
if (syntaxNode.execution == null) syntaxNode.execution = ex;
}
}
// Subcommands
@ -154,6 +181,7 @@ record GraphImpl(NodeImpl root) implements Graph {
}
return new ConversionNode(Literal(""), null, next);
}
}
static Argument<String> commandToArgument(Command command) {

View File

@ -21,7 +21,6 @@ public class EntityActionListener {
private static void setSneaking(Player player, boolean sneaking) {
boolean oldState = player.isSneaking();
player.setSneaking(sneaking);
if (oldState != sneaking) {

View File

@ -1,7 +1,10 @@
package net.minestom.server.command;
import net.minestom.server.command.builder.Command;
import net.minestom.server.command.builder.CommandResult;
import net.minestom.server.command.builder.arguments.ArgumentType;
import net.minestom.server.network.packet.server.play.DeclareCommandsPacket;
import org.jetbrains.annotations.NotNull;
import org.junit.jupiter.api.Test;
import java.util.concurrent.atomic.AtomicBoolean;
@ -45,6 +48,62 @@ public class CommandManagerTest {
assertTrue(check.get());
}
@Test
public void testSharedArgumentSyntaxABFirst() {
var manager = new CommandManager();
var checkA = new AtomicBoolean(false);
var checkAB = new AtomicBoolean(false);
var cmd = new Command("cmd");
var argA = ArgumentType.String("a");
var argB = ArgumentType.String("b");
cmd.addSyntax((sender, context) -> checkAB.set(true), argA, argB);
cmd.addSyntax((sender, context) -> checkA.set(true), argA);
manager.register(cmd);
var result = manager.executeServerCommand("cmd a");
assertEquals(CommandResult.Type.SUCCESS, result.getType());
assertTrue(checkA.get());
assertFalse(checkAB.get());
checkA.set(false); // these should be different tests
checkAB.set(false);
result = manager.executeServerCommand("cmd a b");
assertEquals(CommandResult.Type.SUCCESS, result.getType());
assertFalse(checkA.get());
assertTrue(checkAB.get());
}
@Test
public void testSharedArgumentSyntaxAFirst() {
var manager = new CommandManager();
var checkA = new AtomicBoolean(false);
var checkAB = new AtomicBoolean(false);
var cmd = new Command("cmd");
var argA = ArgumentType.String("a");
var argB = ArgumentType.String("b");
cmd.addSyntax((sender, context) -> checkA.set(true), argA);
cmd.addSyntax((sender, context) -> checkAB.set(true), argA, argB);
manager.register(cmd);
var result = manager.executeServerCommand("cmd a");
assertEquals(CommandResult.Type.SUCCESS, result.getType());
assertTrue(checkA.get());
assertFalse(checkAB.get());
checkA.set(false); // these should be different tests
checkAB.set(false);
result = manager.executeServerCommand("cmd a b");
assertEquals(CommandResult.Type.SUCCESS, result.getType());
assertFalse(checkA.get());
assertTrue(checkAB.get());
}
private static void assertNodeEquals(DeclareCommandsPacket.Node node, byte flags, int[] children, int redirectedNode,
String name, String parser, byte[] properties, String suggestionsType) {
assertEquals(flags, node.flags);

View File

@ -12,6 +12,8 @@ import org.junit.jupiter.api.Test;
import java.util.List;
import static net.minestom.server.command.builder.arguments.ArgumentType.Literal;
import static net.minestom.server.command.builder.arguments.ArgumentType.Word;
import static net.minestom.server.command.builder.arguments.ArgumentType.Integer;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNull;
@ -48,4 +50,29 @@ public class CommandSuggestionIntegrationTest {
assertEquals(List.of(new TabCompletePacket.Match("test1", null)), tabCompletePacket.matches());
});
}
@Test
public void suggestionWithDefaults(Env env) {
var instance = env.createFlatInstance();
var connection = env.createConnection();
var player = connection.connect(instance, new Pos(0, 42, 0)).join();
var suggestArg = Word("suggestArg").setSuggestionCallback(
(sender, context, suggestion) -> suggestion.addEntry(new SuggestionEntry("suggestion"))
);
var defaultArg = Integer("defaultArg").setDefaultValue(123);
var command = new Command("foo");
command.addSyntax((sender,context)->{}, suggestArg, defaultArg);
env.process().command().register(command);
var listener = connection.trackIncoming(TabCompletePacket.class);
player.addPacketToQueue(new ClientTabCompletePacket(1, "foo 1"));
player.interpretPacketQueue();
listener.assertSingle(tabCompletePacket -> {
assertEquals(List.of(new TabCompletePacket.Match("suggestion", null)), tabCompletePacket.matches());
});
}
}

View File

@ -7,6 +7,7 @@ import org.junit.jupiter.api.Test;
import java.lang.String;
import java.util.List;
import java.util.Map;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicReference;
import static net.minestom.server.command.builder.arguments.ArgumentType.Float;
@ -27,6 +28,25 @@ public class CommandSyntaxMultiTest {
assertSyntax(args, "float 5.5", ExpectedExecution.SECOND_SYNTAX, Map.of("float", "float", "number", 5.5f));
}
@Test
public void argPriority() {
List<List<Argument<?>>> args = List.of(
List.of(Word("word")),
List.of(Literal("literal"))
);
assertSyntax(args, "literal", ExpectedExecution.SECOND_SYNTAX);
}
@Test
public void similarArgs() {
List<List<Argument<?>>> args = List.of(
List.of(Word("a")),
List.of(Word("b"), Word("a"))
);
assertSyntax(args, "baz", ExpectedExecution.FIRST_SYNTAX);
assertSyntax(args, "bar baz", ExpectedExecution.SECOND_SYNTAX);
}
private static void assertSyntax(List<List<Argument<?>>> args, String input, ExpectedExecution expectedExecution, Map<String, Object> expectedValues) {
final String commandName = "name";