Fix serialization of arguments node, aliases are now redirects...

The vanilla client doesn't know how to serialize the ArgumentTypes used
by the invocation factories (see StringArrayArgumentType).
ArgumentsCommandNode looks like an ArgumentCommandNode<S, String> but
works like an ArgumentCommandNode<S, T>, where T is the type expected by
each Command implementation. This allows for efficient parsing (just one
arguments parse per execution/suggestion).

PaperMC/velocity-brigadier#7 allows us to create
"cheap" hinting nodes, i.e. they have a dummy #requires that returns
false. Before, we needed to set the Command, requirements,
suggestions... This allows for separation of concerns. The clone is
still needed to set this requirement, however.

PaperMC/velocity-brigadier#8 is perhaps the
biggest change: we now use Brigadier redirects for command aliases. This
means we no longer have to perform a deep clone of all hints and the
arguments node, which can save lots of memory for hint-heavy commands
and commands with lots of aliases (e.g. LuckPerms). This comes at a cost
of a reference in each CommandContextBuilder and CommandContext object,
which get allocated for each suggestion to test the context-aware
predicate.

proxy/src/main/java/com/velocitypowered/proxy/command/SuggestionsProvider.java

@@ -109,7 +109,7 @@ private CompletableFuture<Suggestions> provideArgumentsSuggestions(
           final StringReader reader, final CommandContextBuilder<S> context,
           final CommandNode<S> aliasNode) {
     final CommandNode<S> argumentsNode = aliasNode.getChild(VelocityCommands.ARGS_NODE_NAME);
-    if (argumentsNode == null) {
+    if (!VelocityCommands.isArgumentsNode(argumentsNode)) {
       // This is a BrigadierCommand
       reader.setCursor(0);
       return this.provideDefaultSuggestions(reader, context.getSource());

proxy/src/main/java/com/velocitypowered/proxy/command/VelocityCommands.java

@@ -19,6 +19,7 @@
 
 import com.google.common.base.Preconditions;
 import com.mojang.brigadier.ImmutableStringReader;
+import com.mojang.brigadier.RedirectModifier;
 import com.mojang.brigadier.StringReader;
 import com.mojang.brigadier.arguments.ArgumentType;
 import com.mojang.brigadier.arguments.StringArgumentType;
@@ -54,7 +55,7 @@
  * <p>A <i>fully-built</i> alias node is a node returned by
  * {@link CommandNodeFactory#create(Command, String)}.
  */
-final class VelocityCommands {
+public final class VelocityCommands {
 
   static final String ARGS_NODE_NAME = "arguments";
 
@@ -67,6 +68,11 @@ final class VelocityCommands {
    * @return the command alias
    */
   static String readAlias(final CommandContext<?> context) {
+    if (context.getParent() != null) {
+      // This is the child context of an alias redirect. Command implementations are interested
+      // in knowing the used alias, i.e. the name of the first node in the root context.
+      return readAlias(context.getParent());
+    }
     return readAlias(context.getNodes());
   }
 
@@ -77,6 +83,10 @@ static String readAlias(final CommandContext<?> context) {
    * @return the command alias
    */
   static String readAlias(final CommandContextBuilder<?> context) {
+    if (context.getParent() != null) {
+      return readAlias(context.getParent());
+    }
+
     return readAlias(context.getNodes());
   }
 
@@ -143,20 +153,13 @@ private static <V> V readArguments(final Map<String, ? extends ParsedArgument<?,
    */
   static LiteralCommandNode<CommandSource> newAliasRedirect(
           final LiteralCommandNode<CommandSource> target, final String alias) {
-    // Brigadier parses command input within different contexts. When a node with a redirect is
-    // reached, a child context builder with the redirect target set as its root node is created.
-    // Command implementations are interested in knowing the used alias, i.e. the name of the
-    // redirect origin. A child context does not know about its origin, so we cannot use
-    // the redirect system as is.
-    // We could use a redirect modifier to inject the origin node into the child context, but
-    // that would leave the context in an invalid state.
     Preconditions.checkNotNull(target, "target");
     Preconditions.checkNotNull(alias, "alias");
     return LiteralArgumentBuilder
             .<CommandSource>literal(alias)
             .requires(target.getRequirement())
             .requiresWithContext(target.getContextRequirement())
-            .executes(target.getCommand())
+            .redirect(target)
             .build();
   }
 
@@ -172,12 +175,24 @@ static LiteralCommandNode<CommandSource> newAliasRedirect(
    */
   static <V> ArgumentCommandNode<CommandSource, String> newArgumentsNode(
           final LiteralCommandNode<CommandSource> aliasNode, final ArgumentType<V> type,
-          final BiPredicate<CommandContextBuilder<CommandSource>, ImmutableStringReader> contextRequirement,
-          final SuggestionProvider<CommandSource> customSuggestions) {
+          final BiPredicate<CommandContextBuilder<CommandSource>, ImmutableStringReader>
+                  contextRequirement, final SuggestionProvider<CommandSource> customSuggestions) {
     return new ArgumentsCommandNode<>(type, aliasNode.getCommand(), source -> true,
             contextRequirement, customSuggestions);
   }
 
+  /**
+   * Returns whether the given node is an arguments node.
+   *
+   * @param node the node to check
+   * @return {@code true} if the node is an arguments node
+   * @see #newArgumentsNode(LiteralCommandNode, ArgumentType, BiPredicate, SuggestionProvider) to
+   *      create an arguments node
+   */
+  public static boolean isArgumentsNode(final CommandNode<?> node) {
+    return node instanceof ArgumentsCommandNode;
+  }
+
   // The Vanilla client doesn't know how to serialize the ArgumentTypes used by the invocation
   // factories. The data sent to the client look like it comes from a StringArgumentType, but
   // is generated by the given parsingType.
@@ -190,8 +205,8 @@ private static final class ArgumentsCommandNode<T>
             final ArgumentType<T> parsingType,
             final com.mojang.brigadier.Command<CommandSource> command,
             final Predicate<CommandSource> requirement,
-            final BiPredicate<CommandContextBuilder<CommandSource>, ImmutableStringReader> contextRequirement,
-            final SuggestionProvider<CommandSource> customSuggestions) {
+            final BiPredicate<CommandContextBuilder<CommandSource>, ImmutableStringReader>
+                    contextRequirement, final SuggestionProvider<CommandSource> customSuggestions) {
       super(ARGS_NODE_NAME, StringArgumentType.greedyString(), command, requirement,
               contextRequirement, null, null, false,
               Preconditions.checkNotNull(customSuggestions, "customSuggestions"));
@@ -236,12 +251,14 @@ public void addChild(final CommandNode<CommandSource> node) {
 
     @Override
     public boolean equals(final Object o) {
-      if (this == o) return true;
-      if (!(o instanceof ArgumentsCommandNode)) return false;
-      if (!super.equals(o)) return false;
+      if (this == o) {
+        return true;
+      }
+      if (o == null || getClass() != o.getClass()) {
+        return false;
+      }
 
       final ArgumentsCommandNode<?> that = (ArgumentsCommandNode<?>) o;
-
       return parsingType.equals(that.parsingType);
     }
 

proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BackendPlaySessionHandler.java

@@ -20,7 +20,11 @@
 import static com.velocitypowered.proxy.connection.backend.BungeeCordMessageResponder.getBungeeCordChannel;
 
 import com.google.common.collect.ImmutableList;
+import com.mojang.brigadier.CommandDispatcher;
+import com.mojang.brigadier.StringReader;
 import com.mojang.brigadier.builder.ArgumentBuilder;
+import com.mojang.brigadier.context.CommandContextBuilder;
+import com.mojang.brigadier.context.StringRange;
 import com.mojang.brigadier.tree.CommandNode;
 import com.mojang.brigadier.tree.RootCommandNode;
 import com.velocitypowered.api.command.CommandSource;
@@ -29,6 +33,7 @@
 import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.api.proxy.messages.ChannelIdentifier;
 import com.velocitypowered.proxy.VelocityServer;
+import com.velocitypowered.proxy.command.VelocityCommands;
 import com.velocitypowered.proxy.connection.MinecraftConnection;
 import com.velocitypowered.proxy.connection.MinecraftSessionHandler;
 import com.velocitypowered.proxy.connection.client.ClientPlaySessionHandler;
@@ -197,16 +202,11 @@ public boolean handle(AvailableCommands commands) {
     RootCommandNode<CommandSource> rootNode = commands.getRootNode();
     if (server.getConfiguration().isAnnounceProxyCommands()) {
       // Inject commands from the proxy.
-      RootCommandNode<CommandSource> dispatcherRootNode =
-          (RootCommandNode<CommandSource>)
-              filterNode(server.getCommandManager().getDispatcher().getRoot());
+      RootCommandNode<CommandSource> dispatcherRootNode = this.filterProxyNodes();
       assert dispatcherRootNode != null : "Filtering root node returned null.";
       Collection<CommandNode<CommandSource>> proxyNodes = dispatcherRootNode.getChildren();
       for (CommandNode<CommandSource> node : proxyNodes) {
-        CommandNode<CommandSource> existingServerChild = rootNode.getChild(node.getName());
-        if (existingServerChild != null) {
-          rootNode.getChildren().remove(existingServerChild);
-        }
+        rootNode.removeChildByName(node.getName());
         rootNode.addChild(node);
       }
     }
@@ -221,41 +221,69 @@ public boolean handle(AvailableCommands commands) {
     return true;
   }
 
+  // We don't know the real range as there's no contents
+  private static final StringRange FILTERING_RANGE = StringRange.at(0);
+  private static final StringReader FILTERING_READER = new StringReader("");
+
+  private RootCommandNode<CommandSource> filterProxyNodes() {
+    final CommandDispatcher<CommandSource> dispatcher = server.getCommandManager().getDispatcher();
+    final CommandContextBuilder<CommandSource> context = new CommandContextBuilder<>(
+            dispatcher, serverConn.getPlayer(), dispatcher.getRoot(), 0);
+    return (RootCommandNode<CommandSource>) filterNode(dispatcher.getRoot(), context);
+  }
+
   /**
-   * Creates a deep copy of the provided command node, but removes any node that are not accessible
-   * by the player (respecting the requirement of the node).
+   * Creates a deep copy of the provided command node, but removes any node that is not accessible
+   * by the source (respecting both requirements of the node).
    *
-   * @param source source node
-   * @return filtered node
+   * @param source the node to filter
+   * @param parentContext the context builder for the parent node
+   * @return the filtered node
    */
-  private CommandNode<CommandSource> filterNode(CommandNode<CommandSource> source) {
+  private CommandNode<CommandSource> filterNode(
+          final CommandNode<CommandSource> source,
+          final CommandContextBuilder<CommandSource> parentContext) {
+    if (VelocityCommands.isArgumentsNode(source)) {
+      return source;
+    }
+
+    CommandContextBuilder<CommandSource> context;
     CommandNode<CommandSource> dest;
     if (source instanceof RootCommandNode) {
       dest = new RootCommandNode<>();
+      context = parentContext;
     } else {
-      if (source.getRequirement() != null) {
-        try {
-          if (!source.getRequirement().test(serverConn.getPlayer())) {
-            return null;
-          }
-        } catch (Throwable e) {
-          // swallow everything because plugins
-          logger.error(
-              "Requirement test for command node " + source + " encountered an exception", e);
+      try {
+        if (!source.canUse(serverConn.getPlayer())) {
+          return null;
         }
-      }
 
-      ArgumentBuilder<CommandSource, ?> destChildBuilder = source.createBuilder();
-      destChildBuilder.requires((commandSource) -> true);
-      if (destChildBuilder.getRedirect() != null) {
-        destChildBuilder.redirect(filterNode(destChildBuilder.getRedirect()));
+        context = parentContext.copy();
+        context.withNode(source, FILTERING_RANGE);
+        if (!source.canUse(context, FILTERING_READER)) {
+          logger.info("Cannot use " + source + ", context: " + context);
+          return null;
+        }
+      } catch (final Throwable e) {
+        // swallow everything because plugins
+        logger.error(
+                "Requirement test for command node " + source + " encountered an exception", e);
+        return null;
       }
 
-      dest = destChildBuilder.build();
+      final ArgumentBuilder<CommandSource, ?> destBuilder = source.createBuilder()
+              .requires(source1 -> true)
+              .requiresWithContext((context1, reader) -> true);
+      if (destBuilder.getRedirect() != null) {
+        final CommandContextBuilder<CommandSource> targetContext = new CommandContextBuilder<>(
+                context.getDispatcher(), context.getSource(), source.getRedirect(), 0);
+        destBuilder.redirect(filterNode(destBuilder.getRedirect(), targetContext));
+      }
+      dest = destBuilder.build();
     }
 
     for (CommandNode<CommandSource> sourceChild : source.getChildren()) {
-      CommandNode<CommandSource> destChild = filterNode(sourceChild);
+      CommandNode<CommandSource> destChild = filterNode(sourceChild, context);
       if (destChild == null) {
         continue;
       }

proxy/src/test/java/com/velocitypowered/proxy/command/CommandTests.java

@@ -210,6 +210,8 @@ public List<String> suggest(final Invocation invocation) {
 
   @Test
   void testExecutedWithAlias() {
+    final AtomicInteger callCount = new AtomicInteger();
+
     final CommandMeta meta = manager.metaBuilder("foo")
             .aliases("bar")
             .build();
@@ -218,23 +220,29 @@ void testExecutedWithAlias() {
       public void execute(final Invocation invocation) {
         assertEquals("bar", invocation.alias());
         assertEquals("", invocation.arguments());
+        callCount.incrementAndGet();
       }
     });
 
     assertNotForwarded("bar");
+    assertEquals(1, callCount.get());
   }
 
   @Test
   void testExecutedWithAliasAndArguments() {
+    final AtomicInteger callCount = new AtomicInteger();
+
     final CommandMeta meta = manager.metaBuilder("foo")
             .aliases("bar")
             .build();
     manager.register(meta, (SimpleCommand) invocation -> {
       assertEquals("bar", invocation.alias());
       assertArrayEquals(new String[] { "baz" }, invocation.arguments());
+      callCount.incrementAndGet();
     });
 
     assertNotForwarded("bar baz");
+    assertEquals(1, callCount.get());
   }
 
   @Test
@@ -291,6 +299,26 @@ void testAllAliasesAreSuggested() {
     assertSuggestions("", "bar", "baz", "foo");
   }
 
+  @Test
+  void voidTestOnlyPermissibleAliasesAreSuggested() {
+    final CommandMeta meta = manager.metaBuilder("hello")
+            .aliases("greetings", "howdy", "bonjour")
+            .build();
+    manager.register(meta, new RawCommand() {
+      @Override
+      public void execute(final Invocation invocation) {
+        fail();
+      }
+
+      @Override
+      public boolean hasPermission(final Invocation invocation) {
+        return invocation.alias().equals("greetings") || invocation.alias().equals("howdy");
+      }
+    });
+
+    assertSuggestions("", "greetings", "howdy");
+  }
+
   // Hinting
 
   @Test