✨ Allow for literals to be combined with a variable arg(#181)

Co-authored-by: Irmo van den Berge <irmo.vandenberge@ziggo.nl>

cloud-core/src/main/java/cloud/commandframework/CommandTree.java

@@ -49,13 +49,16 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Queue;
+import java.util.Set;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 /**
  * Tree containing all commands and command paths.
@@ -268,9 +271,35 @@ public final class CommandTree<C> {
     ) {
         CommandPermission permission;
         final List<Node<CommandArgument<C, ?>>> children = root.getChildren();
-        if (children.size() == 1 && !(children.get(0).getValue() instanceof StaticArgument)) {
+
+        // Check whether it matches any of the static arguments
+        // If so, do not attempt parsing as a dynamic argument
+        if (!commandQueue.isEmpty()) {
+            final String literal = commandQueue.peek();
+            final boolean matchesLiteral = children.stream()
+                .filter(n -> n.getValue() instanceof StaticArgument)
+                .map(n -> (StaticArgument<?>) n.getValue())
+                .flatMap(arg -> Stream.concat(Stream.of(arg.getName()), arg.getAliases().stream()))
+                .anyMatch(arg -> arg.equals(literal));
+
+            if (matchesLiteral) {
+                return Pair.of(null, null);
+            }
+        }
+
+        // If it does not match a literal, try to find the one argument node, if it exists
+        // The ambiguity check guarantees that only one will be present
+        final List<Node<CommandArgument<C, ?>>> argumentNodes = children.stream()
+                .filter(n -> (n.getValue() != null && !(n.getValue() instanceof StaticArgument)))
+                .collect(Collectors.toList());
+
+        if (argumentNodes.size() > 1) {
+            throw new IllegalStateException("Unexpected ambiguity detected, number of "
+                    + "dynamic child nodes should not exceed 1");
+        } else if (!argumentNodes.isEmpty()) {
+            final Node<CommandArgument<C, ?>> child = argumentNodes.get(0);
+
             // The value has to be a variable
-            final Node<CommandArgument<C, ?>> child = children.get(0);
             permission = this.isPermitted(commandContext.getSender(), child);
             if (!commandQueue.isEmpty() && permission != null) {
                 return Pair.of(null, new NoPermissionException(
@@ -419,6 +448,7 @@ public final class CommandTree<C> {
                 }
             }
         }
+
         return Pair.of(null, null);
     }
 
@@ -442,20 +472,24 @@ public final class CommandTree<C> {
             final @NonNull Queue<@NonNull String> commandQueue,
             final @NonNull Node<@Nullable CommandArgument<C, ?>> root
     ) {
-
         /* If the sender isn't allowed to access the root node, no suggestions are needed */
         if (this.isPermitted(commandContext.getSender(), root) != null) {
             return Collections.emptyList();
         }
         final List<Node<CommandArgument<C, ?>>> children = root.getChildren();
-        if (children.size() == 1 && !(children.get(0).getValue() instanceof StaticArgument)) {
+
-            return this.suggestionsForDynamicArgument(commandContext, commandQueue, children.get(0));
+        /* Calculate a list of arguments that are static literals */
-        }
+        final List<Node<CommandArgument<C, ?>>> staticArguments = children.stream()
-        /* There are 0 or more static arguments as children. No variable child arguments are present */
+                .filter(n -> n.getValue() instanceof StaticArgument)
-        if (children.isEmpty() || commandQueue.isEmpty()) {
+                .collect(Collectors.toList());
-            return Collections.emptyList();
+
-        } else {
+        /*
-            final Iterator<Node<CommandArgument<C, ?>>> childIterator = root.getChildren().iterator();
+         * Try to see if any of the static literals can be parsed (matches exactly)
+         * If so, enter that node of the command tree for deeper suggestions
+         */
+        if (!staticArguments.isEmpty() && !commandQueue.isEmpty()) {
+            final Queue<String> commandQueueCopy = new LinkedList<String>(commandQueue);
+            final Iterator<Node<CommandArgument<C, ?>>> childIterator = staticArguments.iterator();
             if (childIterator.hasNext()) {
                 while (childIterator.hasNext()) {
                     final Node<CommandArgument<C, ?>> child = childIterator.next();
@@ -466,31 +500,51 @@ public final class CommandTree<C> {
                                 commandQueue
                         );
                         if (result.getParsedValue().isPresent()) {
-                            return this.getSuggestions(commandContext, commandQueue, child);
+                            // If further arguments are specified, dive into this literal
+                            if (!commandQueue.isEmpty()) {
+                                return this.getSuggestions(commandContext, commandQueue, child);
+                            }
+
+                            // We've already matched one exactly, no use looking further
+                            break;
                         }
                     }
                 }
-                if (commandQueue.size() > 1) {
-                    /*
-                     * In this case we were unable to match any of the literals, and so we cannot
-                     * possibly attempt to match any of its children (which is what we want, according
-                     * to the input queue). Because of this, we terminate immediately
-                     */
-                    return Collections.emptyList();
-                }
             }
-            final List<String> suggestions = new LinkedList<>();
+
-            for (final Node<CommandArgument<C, ?>> argument : root.getChildren()) {
+            // Restore original queue
-                if (argument.getValue() == null || this.isPermitted(commandContext.getSender(), argument) != null) {
+            commandQueue.clear();
+            commandQueue.addAll(commandQueueCopy);
+        }
+
+        /* Calculate suggestions for the literal arguments */
+        final List<String> suggestions = new LinkedList<>();
+        if (commandQueue.size() <= 1) {
+            final String literalValue = stringOrEmpty(commandQueue.peek());
+            for (final Node<CommandArgument<C, ?>> argument : staticArguments) {
+                if (this.isPermitted(commandContext.getSender(), argument) != null) {
                     continue;
                 }
                 commandContext.setCurrentArgument(argument.getValue());
                 final List<String> suggestionsToAdd = argument.getValue().getSuggestionsProvider()
-                        .apply(commandContext, stringOrEmpty(commandQueue.peek()));
+                        .apply(commandContext, literalValue);
-                suggestions.addAll(suggestionsToAdd);
+                for (String suggestion : suggestionsToAdd) {
+                    if (suggestion.equals(literalValue) || !suggestion.startsWith(literalValue)) {
+                        continue;
+                    }
+                    suggestions.add(suggestion);
+                }
             }
-            return suggestions;
         }
+
+        /* Calculate suggestions for the variable argument, if one exists */
+        for (final Node<CommandArgument<C, ?>> child : root.getChildren()) {
+            if (child.getValue() != null && !(child.getValue() instanceof StaticArgument)) {
+                suggestions.addAll(this.suggestionsForDynamicArgument(commandContext, commandQueue, child));
+            }
+        }
+
+        return suggestions;
     }
 
     private @NonNull List<@NonNull String> suggestionsForDynamicArgument(
@@ -740,21 +794,52 @@ public final class CommandTree<C> {
         if (node.isLeaf()) {
             return;
         }
-        final int size = node.children.size();
+
-        for (final Node<CommandArgument<C, ?>> child : node.children) {
+        // List of child nodes that are not static arguments, but (parsed) variable ones
-            if (child.getValue() != null
+        final List<Node<CommandArgument<C, ?>>> childVariableArguments = node.children.stream()
-                    && !(child.getValue() instanceof StaticArgument)
+                .filter(n -> (n.getValue() != null && !(n.getValue() instanceof StaticArgument)))
-                    && size > 1) {
+                .collect(Collectors.toList());
-                throw new AmbiguousNodeException(
+
-                        node.getValue(),
+        // If more than one child node exists with a variable argument, fail
-                        child.getValue(),
+        if (childVariableArguments.size() > 1) {
-                        node.getChildren()
+            Node<CommandArgument<C, ?>> child = childVariableArguments.get(0);
-                                .stream()
+            throw new AmbiguousNodeException(
-                                .filter(n -> n.getValue() != null)
+                    node.getValue(),
-                                .map(Node::getValue).collect(Collectors.toList())
+                    child.getValue(),
-                );
+                    node.getChildren()
+                            .stream()
+                            .filter(n -> n.getValue() != null)
+                            .map(Node::getValue).collect(Collectors.toList())
+            );
+        }
+
+        // List of child nodes that are static arguments, with fixed values
+        @SuppressWarnings({ "rawtypes", "unchecked" })
+        final List<Node<StaticArgument<?>>> childStaticArguments = node.children.stream()
+                .filter(n -> n.getValue() instanceof StaticArgument)
+                .map(n -> (Node<StaticArgument<?>>) ((Node) n))
+                .collect(Collectors.toList());
+
+        // Check none of the static arguments are equal to another one
+        // This is done by filling a set and checking there are no duplicates
+        final Set<String> checkedLiterals = new HashSet<>();
+        for (final Node<StaticArgument<?>> child : childStaticArguments) {
+            for (final String nameOrAlias : child.getValue().getAliases()) {
+                if (!checkedLiterals.add(nameOrAlias)) {
+                    // Same literal value, ambiguity detected
+                    throw new AmbiguousNodeException(
+                            node.getValue(),
+                            child.getValue(),
+                            node.getChildren()
+                                    .stream()
+                                    .filter(n -> n.getValue() != null)
+                                    .map(Node::getValue).collect(Collectors.toList())
+                    );
+                }
             }
         }
+
+        // Recursively check child nodes as well
         node.children.forEach(this::checkAmbiguity);
     }
 

cloud-core/src/main/java/cloud/commandframework/arguments/StandardCommandSyntaxFormatter.java

@@ -82,7 +82,15 @@ public class StandardCommandSyntaxFormatter<C> implements CommandSyntaxFormatter
                 final Iterator<CommandTree.Node<CommandArgument<C, ?>>> childIterator = tail.getChildren().iterator();
                 while (childIterator.hasNext()) {
                     final CommandTree.Node<CommandArgument<C, ?>> child = childIterator.next();
-                    formattingInstance.appendName(child.getValue().getName());
+
+                    if (child.getValue() instanceof StaticArgument) {
+                        formattingInstance.appendName(child.getValue().getName());
+                    } else if (child.getValue().isRequired()) {
+                        formattingInstance.appendRequired(child.getValue());
+                    } else {
+                        formattingInstance.appendOptional(child.getValue());
+                    }
+
                     if (childIterator.hasNext()) {
                         formattingInstance.appendPipe();
                     }

cloud-core/src/test/java/cloud/commandframework/CommandHelpHandlerTest.java

@@ -24,6 +24,7 @@
 package cloud.commandframework;
 
 import cloud.commandframework.arguments.standard.IntegerArgument;
+import cloud.commandframework.arguments.standard.StringArgument;
 import cloud.commandframework.meta.CommandMeta;
 import cloud.commandframework.meta.SimpleCommandMeta;
 import cloud.commandframework.types.tuples.Pair;
@@ -48,6 +49,7 @@ class CommandHelpHandlerTest {
         final SimpleCommandMeta meta2 = SimpleCommandMeta.builder().with(CommandMeta.DESCRIPTION, "Command with variables").build();
         manager.command(manager.commandBuilder("test", meta2).literal("int").
                 argument(IntegerArgument.of("int"), Description.of("A number")).build());
+        manager.command(manager.commandBuilder("test").argument(StringArgument.of("potato")));
 
         manager.command(manager.commandBuilder("vec")
                 .meta(CommandMeta.DESCRIPTION, "Takes in a vector")
@@ -61,16 +63,18 @@ class CommandHelpHandlerTest {
     void testVerboseHelp() {
         final List<CommandHelpHandler.VerboseHelpEntry<TestCommandSender>> syntaxHints
                 = manager.getCommandHelpHandler().getAllCommands();
-        final CommandHelpHandler.VerboseHelpEntry<TestCommandSender> entry1 = syntaxHints.get(0);
+        final CommandHelpHandler.VerboseHelpEntry<TestCommandSender> entry0 = syntaxHints.get(0);
+        Assertions.assertEquals("test <potato>", entry0.getSyntaxString());
+        final CommandHelpHandler.VerboseHelpEntry<TestCommandSender> entry1 = syntaxHints.get(1);
         Assertions.assertEquals("test int <int>", entry1.getSyntaxString());
-        final CommandHelpHandler.VerboseHelpEntry<TestCommandSender> entry2 = syntaxHints.get(1);
+        final CommandHelpHandler.VerboseHelpEntry<TestCommandSender> entry2 = syntaxHints.get(2);
         Assertions.assertEquals("test this thing", entry2.getSyntaxString());
     }
 
     @Test
     void testLongestChains() {
         final List<String> longestChains = manager.getCommandHelpHandler().getLongestSharedChains();
-        Assertions.assertEquals(Arrays.asList("test int|this", "vec <<x> <y>>"), longestChains);
+        Assertions.assertEquals(Arrays.asList("test int|this|<potato>", "vec <<x> <y>>"), longestChains);
     }
 
     @Test

cloud-core/src/test/java/cloud/commandframework/CommandSuggestionsTest.java

@@ -99,6 +99,15 @@ public class CommandSuggestionsTest {
                 }))
                 .literal("literal")
                 .build());
+
+        manager.command(manager.commandBuilder("literal_with_variable")
+                .argument(StringArgument.<TestCommandSender>newBuilder("arg").withSuggestionsProvider((context, input) -> {
+                    return Arrays.asList("veni", "vidi");
+                }).build())
+                .literal("now"));
+        manager.command(manager.commandBuilder("literal_with_variable")
+                .literal("vici")
+                .literal("later"));
     }
 
     @Test
@@ -300,6 +309,27 @@ public class CommandSuggestionsTest {
         Assertions.assertEquals(Collections.singletonList("literal"), suggestions9);
     }
 
+    void testLiteralWithVariable() {
+        final String input = "literal_with_variable ";
+        final List<String> suggestions = manager.suggest(new TestCommandSender(), input);
+        Assertions.assertEquals(Arrays.asList("vici", "veni", "vidi"), suggestions);
+        final String input2 = "literal_with_variable v";
+        final List<String> suggestions2 = manager.suggest(new TestCommandSender(), input2);
+        Assertions.assertEquals(Arrays.asList("vici", "veni", "vidi"), suggestions2);
+        final String input3 = "literal_with_variable vi";
+        final List<String> suggestions3 = manager.suggest(new TestCommandSender(), input3);
+        Assertions.assertEquals(Arrays.asList("vici", "vidi"), suggestions3);
+        final String input4 = "literal_with_variable vidi";
+        final List<String> suggestions4 = manager.suggest(new TestCommandSender(), input4);
+        Assertions.assertEquals(Collections.emptyList(), suggestions4);
+        final String input5 = "literal_with_variable vidi ";
+        final List<String> suggestions5 = manager.suggest(new TestCommandSender(), input5);
+        Assertions.assertEquals(Collections.singletonList("now"), suggestions5);
+        final String input6 = "literal_with_variable vici ";
+        final List<String> suggestions6 = manager.suggest(new TestCommandSender(), input6);
+        Assertions.assertEquals(Collections.singletonList("later"), suggestions6);
+    }
+
     public enum TestEnum {
         FOO,
         BAR

cloud-core/src/test/java/cloud/commandframework/CommandTreeTest.java

@@ -267,7 +267,7 @@ class CommandTreeTest {
         Assertions.assertFalse(
                 manager.getCommandTree().getSuggestions(
                         new CommandContext<>(new TestCommandSender(), manager),
-                        new LinkedList<>(Collections.singletonList("test "))
+                        new LinkedList<>(Arrays.asList("test", ""))
                 ).isEmpty());
     }
 
@@ -342,22 +342,30 @@ class CommandTreeTest {
                         .argument(IntegerArgument.of("integer"))));
         newTree();
 
+        // Literal and argument can co-exist, not ambiguous
         manager.command(manager.commandBuilder("ambiguous")
                 .argument(StringArgument.of("string"))
         );
-        Assertions.assertThrows(AmbiguousNodeException.class, () ->
+        manager.command(manager.commandBuilder("ambiguous")
-                manager.command(manager.commandBuilder("ambiguous")
+                .literal("literal"));
-                        .literal("literal")));
         newTree();
 
+        // Two literals (different names) and argument can co-exist, not ambiguous
         manager.command(manager.commandBuilder("ambiguous")
-                .literal("literal")
+                .literal("literal"));
-        );
         manager.command(manager.commandBuilder("ambiguous")
                 .literal("literal2"));
-        Assertions.assertThrows(AmbiguousNodeException.class, () ->
+
+        manager.command(manager.commandBuilder("ambiguous")
+                .argument(IntegerArgument.of("integer")));
+        newTree();
+
+        // Two literals with the same name can not co-exist, causes 'duplicate command chains' error
+        manager.command(manager.commandBuilder("ambiguous")
+                .literal("literal"));
+        Assertions.assertThrows(IllegalStateException.class, () ->
                 manager.command(manager.commandBuilder("ambiguous")
-                        .argument(IntegerArgument.of("integer"))));
+                        .literal("literal")));
         newTree();
     }
 
@@ -391,6 +399,53 @@ class CommandTreeTest {
         );
     }
 
+    @Test
+    void testAmbiguousLiteralOverridingArgument() {
+        /* Build two commands for testing literals overriding variable arguments */
+        manager.command(
+                manager.commandBuilder("literalwithvariable")
+                       .argument(StringArgument.of("variable"))
+        );
+
+        manager.command(
+                manager.commandBuilder("literalwithvariable")
+                       .literal("literal", "literalalias")
+        );
+
+        /* Try parsing as a variable, which should match the variable command */
+        final Pair<Command<TestCommandSender>, Exception> variableResult = manager.getCommandTree().parse(
+                        new CommandContext<>(new TestCommandSender(), manager),
+                        new LinkedList<>(Arrays.asList("literalwithvariable", "argthatdoesnotmatch"))
+                );
+        Assertions.assertNull(variableResult.getSecond());
+        Assertions.assertEquals("literalwithvariable",
+                variableResult.getFirst().getArguments().get(0).getName());
+        Assertions.assertEquals("variable",
+                variableResult.getFirst().getArguments().get(1).getName());
+
+        /* Try parsing with the main name literal, which should match the literal command */
+        final Pair<Command<TestCommandSender>, Exception> literalResult = manager.getCommandTree().parse(
+                new CommandContext<>(new TestCommandSender(), manager),
+                new LinkedList<>(Arrays.asList("literalwithvariable", "literal"))
+        );
+        Assertions.assertNull(literalResult.getSecond());
+        Assertions.assertEquals("literalwithvariable",
+                literalResult.getFirst().getArguments().get(0).getName());
+        Assertions.assertEquals("literal",
+                literalResult.getFirst().getArguments().get(1).getName());
+
+        /* Try parsing with the alias of the literal, which should match the literal command */
+        final Pair<Command<TestCommandSender>, Exception> literalAliasResult = manager.getCommandTree().parse(
+                new CommandContext<>(new TestCommandSender(), manager),
+                new LinkedList<>(Arrays.asList("literalwithvariable", "literalalias"))
+        );
+        Assertions.assertNull(literalAliasResult.getSecond());
+        Assertions.assertEquals("literalwithvariable",
+                literalAliasResult.getFirst().getArguments().get(0).getName());
+        Assertions.assertEquals("literal",
+                literalAliasResult.getFirst().getArguments().get(1).getName());
+    }
+
     @Test
     void testDuplicateArgument() {
         final CommandArgument<TestCommandSender, String> argument = StringArgument.of("test");

cloud-minecraft/cloud-brigadier/src/main/java/cloud/commandframework/brigadier/CloudBrigadierManager.java

@@ -62,13 +62,17 @@ import io.leangen.geantyref.TypeToken;
 import org.checkerframework.checker.nullness.qual.NonNull;
 import org.checkerframework.checker.nullness.qual.Nullable;
 
+import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.CompletableFuture;
 import java.util.function.BiPredicate;
 import java.util.function.Function;
 import java.util.function.Supplier;
+import java.util.stream.Collectors;
 
 /**
  * Manager used to map cloud {@link Command}
@@ -372,8 +376,10 @@ public final class CloudBrigadierManager<C, S> {
         final SuggestionProvider<S> provider = (context, builder) -> this.buildSuggestions(
                 context,
                 node.getValue(),
+                Collections.emptySet(),
                 builder
         );
+
         final LiteralArgumentBuilder<S> literalArgumentBuilder = LiteralArgumentBuilder
                 .<S>literal(label)
                 .requires(sender -> permissionChecker.test(sender, (CommandPermission) node.getNodeMeta()
@@ -497,6 +503,18 @@ public final class CloudBrigadierManager<C, S> {
                             )))
                     .executes(executor);
         } else {
+            // Check for sibling literals (StaticArgument)
+            // These are important when providing suggestions
+            final Set<String> siblingLiterals = (root.getParent() == null) ? Collections.emptySet()
+                    : root.getParent().getChildren().stream()
+                        .filter(n -> n.getValue() instanceof StaticArgument)
+                        .map(n -> (StaticArgument<C>) ((CommandArgument) n.getValue()))
+                        .flatMap(s -> s.getAliases().stream())
+                        .sorted()
+                        .distinct()
+                        .collect(Collectors.toSet());
+
+            // Register argument
             final Pair<ArgumentType<?>, Boolean> pair = this.getArgument(
                     root.getValue().getValueType(),
                     TypeToken.get(root.getValue().getParser().getClass()),
@@ -507,6 +525,7 @@ public final class CloudBrigadierManager<C, S> {
                     : (context, builder) -> this.buildSuggestions(
                             context,
                             root.getValue(),
+                            siblingLiterals,
                             builder
                     );
             argumentBuilder = RequiredArgumentBuilder
@@ -536,6 +555,7 @@ public final class CloudBrigadierManager<C, S> {
     private @NonNull CompletableFuture<Suggestions> buildSuggestions(
             final com.mojang.brigadier.context.@Nullable CommandContext<S> senderContext,
             final @NonNull CommandArgument<C, ?> argument,
+            final @NonNull Set<String> siblingLiterals,
             final @NonNull SuggestionsBuilder builder
     ) {
         final CommandContext<C> commandContext;
@@ -561,11 +581,15 @@ public final class CloudBrigadierManager<C, S> {
             command = command.substring(leading.split(":")[0].length() + 1);
         }
 
-        final List<String> suggestions = this.commandManager.suggest(
+        final List<String> suggestionsUnfiltered = this.commandManager.suggest(
                 commandContext.getSender(),
                 command
         );
 
+        /* Filter suggetions that are literal arguments to avoid duplicates */
+        final List<String> suggestions = new ArrayList<>(suggestionsUnfiltered);
+        suggestions.removeIf(siblingLiterals::contains);
+
         SuggestionsBuilder suggestionsBuilder = builder;
 
         final int lastIndexOfSpaceInRemainingString = builder.getRemaining().lastIndexOf(' ');