diff --git a/cloud-core/src/main/java/cloud/commandframework/CommandTree.java b/cloud-core/src/main/java/cloud/commandframework/CommandTree.java index a2e7651a..c6306265 100644 --- a/cloud-core/src/main/java/cloud/commandframework/CommandTree.java +++ b/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 { ) { CommandPermission permission; final List>> 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>> 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> child = argumentNodes.get(0); + // The value has to be a variable - final Node> 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 { } } } + return Pair.of(null, null); } @@ -442,20 +472,24 @@ public final class CommandTree { final @NonNull Queue<@NonNull String> commandQueue, final @NonNull Node<@Nullable CommandArgument> 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>> children = root.getChildren(); - if (children.size() == 1 && !(children.get(0).getValue() instanceof StaticArgument)) { - return this.suggestionsForDynamicArgument(commandContext, commandQueue, children.get(0)); - } - /* There are 0 or more static arguments as children. No variable child arguments are present */ - if (children.isEmpty() || commandQueue.isEmpty()) { - return Collections.emptyList(); - } else { - final Iterator>> childIterator = root.getChildren().iterator(); + + /* Calculate a list of arguments that are static literals */ + final List>> staticArguments = children.stream() + .filter(n -> n.getValue() instanceof StaticArgument) + .collect(Collectors.toList()); + + /* + * 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 commandQueueCopy = new LinkedList(commandQueue); + final Iterator>> childIterator = staticArguments.iterator(); if (childIterator.hasNext()) { while (childIterator.hasNext()) { final Node> child = childIterator.next(); @@ -466,31 +500,51 @@ public final class CommandTree { 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 suggestions = new LinkedList<>(); - for (final Node> argument : root.getChildren()) { - if (argument.getValue() == null || this.isPermitted(commandContext.getSender(), argument) != null) { + + // Restore original queue + commandQueue.clear(); + commandQueue.addAll(commandQueueCopy); + } + + /* Calculate suggestions for the literal arguments */ + final List suggestions = new LinkedList<>(); + if (commandQueue.size() <= 1) { + final String literalValue = stringOrEmpty(commandQueue.peek()); + for (final Node> argument : staticArguments) { + if (this.isPermitted(commandContext.getSender(), argument) != null) { continue; } commandContext.setCurrentArgument(argument.getValue()); final List suggestionsToAdd = argument.getValue().getSuggestionsProvider() - .apply(commandContext, stringOrEmpty(commandQueue.peek())); - suggestions.addAll(suggestionsToAdd); + .apply(commandContext, literalValue); + 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> 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 { if (node.isLeaf()) { return; } - final int size = node.children.size(); - for (final Node> child : node.children) { - if (child.getValue() != null - && !(child.getValue() instanceof StaticArgument) - && size > 1) { - throw new AmbiguousNodeException( - node.getValue(), - child.getValue(), - node.getChildren() - .stream() - .filter(n -> n.getValue() != null) - .map(Node::getValue).collect(Collectors.toList()) - ); + + // List of child nodes that are not static arguments, but (parsed) variable ones + final List>> childVariableArguments = node.children.stream() + .filter(n -> (n.getValue() != null && !(n.getValue() instanceof StaticArgument))) + .collect(Collectors.toList()); + + // If more than one child node exists with a variable argument, fail + if (childVariableArguments.size() > 1) { + Node> child = childVariableArguments.get(0); + throw new AmbiguousNodeException( + node.getValue(), + 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>> childStaticArguments = node.children.stream() + .filter(n -> n.getValue() instanceof StaticArgument) + .map(n -> (Node>) ((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 checkedLiterals = new HashSet<>(); + for (final Node> 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); } diff --git a/cloud-core/src/main/java/cloud/commandframework/arguments/StandardCommandSyntaxFormatter.java b/cloud-core/src/main/java/cloud/commandframework/arguments/StandardCommandSyntaxFormatter.java index f87878b1..a5626be2 100644 --- a/cloud-core/src/main/java/cloud/commandframework/arguments/StandardCommandSyntaxFormatter.java +++ b/cloud-core/src/main/java/cloud/commandframework/arguments/StandardCommandSyntaxFormatter.java @@ -82,7 +82,15 @@ public class StandardCommandSyntaxFormatter implements CommandSyntaxFormatter final Iterator>> childIterator = tail.getChildren().iterator(); while (childIterator.hasNext()) { final CommandTree.Node> 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(); } diff --git a/cloud-core/src/test/java/cloud/commandframework/CommandHelpHandlerTest.java b/cloud-core/src/test/java/cloud/commandframework/CommandHelpHandlerTest.java index 0f096763..ffec9679 100644 --- a/cloud-core/src/test/java/cloud/commandframework/CommandHelpHandlerTest.java +++ b/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> syntaxHints = manager.getCommandHelpHandler().getAllCommands(); - final CommandHelpHandler.VerboseHelpEntry entry1 = syntaxHints.get(0); + final CommandHelpHandler.VerboseHelpEntry entry0 = syntaxHints.get(0); + Assertions.assertEquals("test ", entry0.getSyntaxString()); + final CommandHelpHandler.VerboseHelpEntry entry1 = syntaxHints.get(1); Assertions.assertEquals("test int ", entry1.getSyntaxString()); - final CommandHelpHandler.VerboseHelpEntry entry2 = syntaxHints.get(1); + final CommandHelpHandler.VerboseHelpEntry entry2 = syntaxHints.get(2); Assertions.assertEquals("test this thing", entry2.getSyntaxString()); } @Test void testLongestChains() { final List longestChains = manager.getCommandHelpHandler().getLongestSharedChains(); - Assertions.assertEquals(Arrays.asList("test int|this", "vec < >"), longestChains); + Assertions.assertEquals(Arrays.asList("test int|this|", "vec < >"), longestChains); } @Test diff --git a/cloud-core/src/test/java/cloud/commandframework/CommandSuggestionsTest.java b/cloud-core/src/test/java/cloud/commandframework/CommandSuggestionsTest.java index 0f814375..9f2173ea 100644 --- a/cloud-core/src/test/java/cloud/commandframework/CommandSuggestionsTest.java +++ b/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.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 suggestions = manager.suggest(new TestCommandSender(), input); + Assertions.assertEquals(Arrays.asList("vici", "veni", "vidi"), suggestions); + final String input2 = "literal_with_variable v"; + final List suggestions2 = manager.suggest(new TestCommandSender(), input2); + Assertions.assertEquals(Arrays.asList("vici", "veni", "vidi"), suggestions2); + final String input3 = "literal_with_variable vi"; + final List suggestions3 = manager.suggest(new TestCommandSender(), input3); + Assertions.assertEquals(Arrays.asList("vici", "vidi"), suggestions3); + final String input4 = "literal_with_variable vidi"; + final List suggestions4 = manager.suggest(new TestCommandSender(), input4); + Assertions.assertEquals(Collections.emptyList(), suggestions4); + final String input5 = "literal_with_variable vidi "; + final List suggestions5 = manager.suggest(new TestCommandSender(), input5); + Assertions.assertEquals(Collections.singletonList("now"), suggestions5); + final String input6 = "literal_with_variable vici "; + final List suggestions6 = manager.suggest(new TestCommandSender(), input6); + Assertions.assertEquals(Collections.singletonList("later"), suggestions6); + } + public enum TestEnum { FOO, BAR diff --git a/cloud-core/src/test/java/cloud/commandframework/CommandTreeTest.java b/cloud-core/src/test/java/cloud/commandframework/CommandTreeTest.java index 236a421f..99548498 100644 --- a/cloud-core/src/test/java/cloud/commandframework/CommandTreeTest.java +++ b/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") - .literal("literal"))); + manager.command(manager.commandBuilder("ambiguous") + .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, 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, 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, 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 argument = StringArgument.of("test"); diff --git a/cloud-minecraft/cloud-brigadier/src/main/java/cloud/commandframework/brigadier/CloudBrigadierManager.java b/cloud-minecraft/cloud-brigadier/src/main/java/cloud/commandframework/brigadier/CloudBrigadierManager.java index 2a2b406e..7dbba791 100644 --- a/cloud-minecraft/cloud-brigadier/src/main/java/cloud/commandframework/brigadier/CloudBrigadierManager.java +++ b/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 { final SuggestionProvider provider = (context, builder) -> this.buildSuggestions( context, node.getValue(), + Collections.emptySet(), builder ); + final LiteralArgumentBuilder literalArgumentBuilder = LiteralArgumentBuilder .literal(label) .requires(sender -> permissionChecker.test(sender, (CommandPermission) node.getNodeMeta() @@ -497,6 +503,18 @@ public final class CloudBrigadierManager { ))) .executes(executor); } else { + // Check for sibling literals (StaticArgument) + // These are important when providing suggestions + final Set siblingLiterals = (root.getParent() == null) ? Collections.emptySet() + : root.getParent().getChildren().stream() + .filter(n -> n.getValue() instanceof StaticArgument) + .map(n -> (StaticArgument) ((CommandArgument) n.getValue())) + .flatMap(s -> s.getAliases().stream()) + .sorted() + .distinct() + .collect(Collectors.toSet()); + + // Register argument final Pair, Boolean> pair = this.getArgument( root.getValue().getValueType(), TypeToken.get(root.getValue().getParser().getClass()), @@ -507,6 +525,7 @@ public final class CloudBrigadierManager { : (context, builder) -> this.buildSuggestions( context, root.getValue(), + siblingLiterals, builder ); argumentBuilder = RequiredArgumentBuilder @@ -536,6 +555,7 @@ public final class CloudBrigadierManager { private @NonNull CompletableFuture buildSuggestions( final com.mojang.brigadier.context.@Nullable CommandContext senderContext, final @NonNull CommandArgument argument, + final @NonNull Set siblingLiterals, final @NonNull SuggestionsBuilder builder ) { final CommandContext commandContext; @@ -561,11 +581,15 @@ public final class CloudBrigadierManager { command = command.substring(leading.split(":")[0].length() + 1); } - final List suggestions = this.commandManager.suggest( + final List suggestionsUnfiltered = this.commandManager.suggest( commandContext.getSender(), command ); + /* Filter suggetions that are literal arguments to avoid duplicates */ + final List suggestions = new ArrayList<>(suggestionsUnfiltered); + suggestions.removeIf(siblingLiterals::contains); + SuggestionsBuilder suggestionsBuilder = builder; final int lastIndexOfSpaceInRemainingString = builder.getRemaining().lastIndexOf(' ');