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

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<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<>")
+            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;
                     }
                 }

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;
         }
 

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());
+        });
+    }
+
 }

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);
     }