From 2b8beadd3ef196ed1e9a291af87f33f69300b877 Mon Sep 17 00:00:00 2001 From: Spanner Date: Wed, 30 Aug 2023 23:11:17 +0000 Subject: [PATCH] Rewrite CommandParserImpl recursively (fix #1327) (#1893) * 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 * Fix `GraphImpl` causing syntax order issues --- .../server/command/CommandParserImpl.java | 206 +++++++++++------- .../minestom/server/command/GraphImpl.java | 40 +++- .../CommandSuggestionIntegrationTest.java | 27 +++ .../command/CommandSyntaxMultiTest.java | 20 ++ 4 files changed, 203 insertions(+), 90 deletions(-) diff --git a/src/main/java/net/minestom/server/command/CommandParserImpl.java b/src/main/java/net/minestom/server/command/CommandParserImpl.java index 007ebd65d..0af20c062 100644 --- a/src/main/java/net/minestom/server/command/CommandParserImpl.java +++ b/src/main/java/net/minestom/server/command/CommandParserImpl.java @@ -30,6 +30,7 @@ final class CommandParserImpl implements CommandParser { static final class Chain { CommandExecutor defaultExecutor = null; + SuggestionCallback suggestionCallback = null; final ArrayDeque nodeResults = new ArrayDeque<>(); final List conditions = new ArrayList<>(); final List globalListeners = new ArrayList<>(); @@ -63,81 +64,60 @@ final class CommandParserImpl implements CommandParser { return (sender, context) -> globalListeners.forEach(x -> x.apply(sender, context)); } - SuggestionCallback extractSuggestionCallback() { - return nodeResults.peekLast().callback; - } - Map> collectArguments() { return nodeResults.stream() - .skip(1) // skip root + .skip(2) // skip root node and command .collect(Collectors.toUnmodifiableMap(NodeResult::name, NodeResult::argumentResult)); } List> getArgs() { return nodeResults.stream().map(x -> x.node.argument()).collect(Collectors.toList()); } + + Chain() {} + + Chain(CommandExecutor defaultExecutor, + SuggestionCallback suggestionCallback, + ArrayDeque nodeResults, + List conditions, + List 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 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(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(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 Supplier defaultSupplier = argument.getDefaultValue(); - if (defaultSupplier != null) { - final Object value = defaultSupplier.get(); - final ArgumentResult 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 -> _") @@ -145,32 +125,90 @@ final class CommandParserImpl implements CommandParser { return obj == null ? null : getter.apply(obj); } - private static NodeResult parseChild(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(argument, reader); - if (parse instanceof ArgumentResult.Success success) { - return new NodeResult(child, (ArgumentResult) success, - argument.getSuggestionCallback()); - } else if (parse instanceof ArgumentResult.SyntaxError syntaxError) { - return new NodeResult(child, (ArgumentResult) syntaxError, - argument.getSuggestionCallback()); + private static NodeResult parseNode(Node node, Chain chain, CommandStringReader reader) { + chain = chain.fork(); + Argument argument = node.argument(); + int start = reader.cursor(); + + if (reader.hasRemaining()) { + ArgumentResult result = parseArgument(argument, reader); + SuggestionCallback suggestionCallback = argument.getSuggestionCallback(); + NodeResult nodeResult = new NodeResult(node, chain, (ArgumentResult) 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 + Supplier defaultSupplier = node.argument().getDefaultValue(); + if (defaultSupplier != null) { + Object value = defaultSupplier.get(); + ArgumentResult 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(child, chain, reader); + if (childResult.argumentResult instanceof ArgumentResult.Success) { + // 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 { @@ -225,7 +263,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 @@ -241,12 +279,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 @@ -324,7 +362,7 @@ final class CommandParserImpl implements CommandParser { static final ExecutableCommand.Result INVALID_SYNTAX = new ExecutionResultImpl(Type.INVALID_SYNTAX, null); } - private record NodeResult(Node node, ArgumentResult argumentResult, SuggestionCallback callback) { + private record NodeResult(Node node, Chain chain, ArgumentResult argumentResult, SuggestionCallback callback) { public String name() { return node.argument().getId(); } @@ -375,7 +413,7 @@ final class CommandParserImpl implements CommandParser { // ARGUMENT - private static ArgumentResult parse(Argument argument, CommandStringReader reader) { + private static ArgumentResult parseArgument(Argument argument, CommandStringReader reader) { // Handle specific type without loop try { // Single word argument diff --git a/src/main/java/net/minestom/server/command/GraphImpl.java b/src/main/java/net/minestom/server/command/GraphImpl.java index df1733126..8ff25a700 100644 --- a/src/main/java/net/minestom/server/command/GraphImpl.java +++ b/src/main/java/net/minestom/server/command/GraphImpl.java @@ -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 next) implements Graph.Node { + NodeImpl(Argument argument, ExecutionImpl execution, List next) { + this.argument = argument; + this.execution = execution; + this.next = next.stream().sorted(nodePriority).toList(); + } + static NodeImpl fromBuilder(BuilderImpl builder) { final List children = builder.children; Node[] nodes = new NodeImpl[children.size()]; @@ -76,6 +84,17 @@ record GraphImpl(NodeImpl root) implements Graph { static NodeImpl rootCommands(Collection commands) { return ConversionNode.rootConv(commands).toNode(); } + + private static final java.util.Comparator 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 predicate, @@ -113,8 +132,17 @@ record GraphImpl(NodeImpl root) implements Graph { } } - private record ConversionNode(Argument argument, ExecutionImpl execution, - Map, ConversionNode> nextMap) { + private static final class ConversionNode { + final Argument argument; + ExecutionImpl execution; + final Map, ConversionNode> nextMap; + + public ConversionNode(Argument argument, ExecutionImpl execution, Map, 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 commandToArgument(Command command) { diff --git a/src/test/java/net/minestom/server/command/CommandSuggestionIntegrationTest.java b/src/test/java/net/minestom/server/command/CommandSuggestionIntegrationTest.java index 90fa13402..f69b2ed2a 100644 --- a/src/test/java/net/minestom/server/command/CommandSuggestionIntegrationTest.java +++ b/src/test/java/net/minestom/server/command/CommandSuggestionIntegrationTest.java @@ -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()); + }); + } } diff --git a/src/test/java/net/minestom/server/command/CommandSyntaxMultiTest.java b/src/test/java/net/minestom/server/command/CommandSyntaxMultiTest.java index 8a5c3f18d..ca0fec626 100644 --- a/src/test/java/net/minestom/server/command/CommandSyntaxMultiTest.java +++ b/src/test/java/net/minestom/server/command/CommandSyntaxMultiTest.java @@ -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>> args = List.of( + List.of(Word("word")), + List.of(Literal("literal")) + ); + assertSyntax(args, "literal", ExpectedExecution.SECOND_SYNTAX); + } + + @Test + public void similarArgs() { + List>> 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>> args, String input, ExpectedExecution expectedExecution, Map expectedValues) { final String commandName = "name";