From 2cdb3911b0081770621af5e0cb2c0e0299bd0a5f Mon Sep 17 00:00:00 2001 From: Spanner Date: Thu, 7 Sep 2023 12:58:34 +0000 Subject: [PATCH] Prioritise errors by chain length, build subcommands first (fix #1934) (#1935) * Fix empty command chain causing NullPointerException * Add test for empty command input * Add test for #1934; subcommand priority issue * Fix ConversionNode; process subcommands first * Fix command error priority, add test for literal suggestions * Test for subcommand priority in graph tests --- .../server/command/CommandParserImpl.java | 10 ++- .../minestom/server/command/GraphImpl.java | 8 +-- .../CommandSuggestionIntegrationTest.java | 65 +++++++++++++++++++ .../server/command/GraphConversionTest.java | 5 ++ 4 files changed, 81 insertions(+), 7 deletions(-) diff --git a/src/main/java/net/minestom/server/command/CommandParserImpl.java b/src/main/java/net/minestom/server/command/CommandParserImpl.java index 925a1a815..0e80492af 100644 --- a/src/main/java/net/minestom/server/command/CommandParserImpl.java +++ b/src/main/java/net/minestom/server/command/CommandParserImpl.java @@ -74,6 +74,10 @@ final class CommandParserImpl implements CommandParser { return nodeResults.stream().map(x -> x.node.argument()).collect(Collectors.toList()); } + int size() { + return nodeResults.size(); + } + Chain() {} Chain(CommandExecutor defaultExecutor, @@ -137,7 +141,7 @@ final class CommandParserImpl implements CommandParser { 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<>") + if (chain.size() == 1) { // If this is the root node (usually "Literal<>") reader.cursor(start); } else { if (!(result instanceof ArgumentResult.Success)) { @@ -174,11 +178,11 @@ final class CommandParserImpl implements CommandParser { // Assume that there is only one successful node for a given chain of arguments return childResult; } else { - if (error == null) { + if (error == null || error.chain.size() < childResult.chain.size()) { // 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)) { + if (!(childResult.chain.size() == 2 && childResult.argumentResult instanceof ArgumentResult.IncompatibleType)) { error = childResult; } } diff --git a/src/main/java/net/minestom/server/command/GraphImpl.java b/src/main/java/net/minestom/server/command/GraphImpl.java index 8ff25a700..544388890 100644 --- a/src/main/java/net/minestom/server/command/GraphImpl.java +++ b/src/main/java/net/minestom/server/command/GraphImpl.java @@ -156,6 +156,10 @@ record GraphImpl(NodeImpl root) implements Graph { static ConversionNode fromCommand(Command command) { ConversionNode root = new ConversionNode(commandToArgument(command), ExecutionImpl.fromCommand(command)); + // Subcommands + for (Command subcommand : command.getSubcommands()) { + root.nextMap.put(commandToArgument(subcommand), fromCommand(subcommand)); + } // Syntaxes for (CommandSyntax syntax : command.getSyntaxes()) { ConversionNode syntaxNode = root; @@ -166,10 +170,6 @@ record GraphImpl(NodeImpl root) implements Graph { if (syntaxNode.execution == null) syntaxNode.execution = ex; } } - // Subcommands - for (Command subcommand : command.getSubcommands()) { - root.nextMap.put(commandToArgument(subcommand), fromCommand(subcommand)); - } return root; } diff --git a/src/test/java/net/minestom/server/command/CommandSuggestionIntegrationTest.java b/src/test/java/net/minestom/server/command/CommandSuggestionIntegrationTest.java index f69b2ed2a..fff2a33d0 100644 --- a/src/test/java/net/minestom/server/command/CommandSuggestionIntegrationTest.java +++ b/src/test/java/net/minestom/server/command/CommandSuggestionIntegrationTest.java @@ -75,4 +75,69 @@ public class CommandSuggestionIntegrationTest { assertEquals(List.of(new TabCompletePacket.Match("suggestion", null)), tabCompletePacket.matches()); }); } + + @Test + public void suggestionWithSubcommand(Env env) { + var instance = env.createFlatInstance(); + var connection = env.createConnection(); + var player = connection.connect(instance, new Pos(0, 42, 0)).join(); + + var command = new Command("foo"); + + var subCommand = new Command("bar"); + + var wordArg1 = Word("wordArg1").setSuggestionCallback((sender, context, suggestion) -> { + suggestion.addEntry(new SuggestionEntry("suggestionA")); + }); + var wordArg2 = Word("wordArg2").setSuggestionCallback((sender, context, suggestion) -> { + suggestion.addEntry(new SuggestionEntry("suggestionB")); + }); + + subCommand.addSyntax((sender, context) -> {}, wordArg1, wordArg2); + + command.addSyntax((sender,context)->{}, Literal("literal"), wordArg2); + + command.addSubcommand(subCommand); + + env.process().command().register(command); + + var listener = connection.trackIncoming(TabCompletePacket.class); + player.addPacketToQueue(new ClientTabCompletePacket(1, "foo bar ")); + player.interpretPacketQueue(); + + listener.assertSingle(tabCompletePacket -> { + assertEquals(List.of(new TabCompletePacket.Match("suggestionA", null)), tabCompletePacket.matches()); + }); + } + + @Test + public void suggestionWithTwoLiterals(Env env) { + var instance = env.createFlatInstance(); + var connection = env.createConnection(); + var player = connection.connect(instance, new Pos(0, 42, 0)).join(); + + var command = new Command("foo"); + + var wordArg1 = Word("wordArg1").setSuggestionCallback((sender, context, suggestion) -> { + suggestion.addEntry(new SuggestionEntry("suggestionA")); + }); + var wordArg2 = Word("wordArg2").setSuggestionCallback((sender, context, suggestion) -> { + suggestion.addEntry(new SuggestionEntry("suggestionB")); + }); + + command.addSyntax((sender,context)->{}, Literal("literal1"), wordArg1); + + command.addSyntax((sender,context)->{}, Literal("literal2"), wordArg2); + + env.process().command().register(command); + + var listener = connection.trackIncoming(TabCompletePacket.class); + player.addPacketToQueue(new ClientTabCompletePacket(1, "foo literal2 ")); + player.interpretPacketQueue(); + + listener.assertSingle(tabCompletePacket -> { + assertEquals(List.of(new TabCompletePacket.Match("suggestionB", null)), tabCompletePacket.matches()); + }); + } + } diff --git a/src/test/java/net/minestom/server/command/GraphConversionTest.java b/src/test/java/net/minestom/server/command/GraphConversionTest.java index 2bea24622..7c294232e 100644 --- a/src/test/java/net/minestom/server/command/GraphConversionTest.java +++ b/src/test/java/net/minestom/server/command/GraphConversionTest.java @@ -85,6 +85,10 @@ public class GraphConversionTest { final Command main = new Command("main"); final Command sub = new Command("sub"); + var baz = Literal("baz"); + + main.addSyntax(GraphConversionTest::dummyExecutor, baz); // Check that subcommands are added to graph first + var bar = Literal("bar"); var number = Integer("number"); @@ -97,6 +101,7 @@ public class GraphConversionTest { var graph = Graph.builder(Literal("main")) .append(Literal("sub"), builder -> builder.append(bar, builder1 -> builder1.append(number))) + .append(Literal("baz")) .build(); assertEqualsGraph(graph, main); }