From 84609dc046f33fb756362909633db7fcd90d0609 Mon Sep 17 00:00:00 2001 From: Jake Potrebic Date: Sun, 16 Feb 2025 13:55:27 -0800 Subject: [PATCH] Don't auto-create any brig redirects (#11954) --- .../brigadier/CommandRegistrationFlag.java | 5 ++ .../paper/command/brigadier/Commands.java | 5 ++ .../brigadier/tree/CommandNode.java.patch | 3 +- .../minecraft/commands/Commands.java.patch | 78 ++++--------------- .../command/brigadier/ApiMirrorRootNode.java | 4 +- .../command/brigadier/PaperBrigadier.java | 31 +++++--- .../command/brigadier/PaperCommands.java | 59 +++++--------- .../command/brigadier/PluginCommandMeta.java | 26 +++++++ .../command/brigadier/PluginCommandNode.java | 50 ------------ 9 files changed, 95 insertions(+), 166 deletions(-) create mode 100644 paper-server/src/main/java/io/papermc/paper/command/brigadier/PluginCommandMeta.java delete mode 100644 paper-server/src/main/java/io/papermc/paper/command/brigadier/PluginCommandNode.java diff --git a/paper-api/src/main/java/io/papermc/paper/command/brigadier/CommandRegistrationFlag.java b/paper-api/src/main/java/io/papermc/paper/command/brigadier/CommandRegistrationFlag.java index 7e24babf74..7e1d500b18 100644 --- a/paper-api/src/main/java/io/papermc/paper/command/brigadier/CommandRegistrationFlag.java +++ b/paper-api/src/main/java/io/papermc/paper/command/brigadier/CommandRegistrationFlag.java @@ -10,5 +10,10 @@ import org.jetbrains.annotations.ApiStatus; */ @ApiStatus.Internal public enum CommandRegistrationFlag { + + /** + * @deprecated This is the default behavior now. + */ + @Deprecated(since = "1.21.4") FLATTEN_ALIASES } diff --git a/paper-api/src/main/java/io/papermc/paper/command/brigadier/Commands.java b/paper-api/src/main/java/io/papermc/paper/command/brigadier/Commands.java index e32559772a..8664429248 100644 --- a/paper-api/src/main/java/io/papermc/paper/command/brigadier/Commands.java +++ b/paper-api/src/main/java/io/papermc/paper/command/brigadier/Commands.java @@ -113,6 +113,7 @@ public interface Commands extends Registrar { *

Commands have certain overriding behavior: *

* @@ -129,6 +130,7 @@ public interface Commands extends Registrar { *

Commands have certain overriding behavior: *

* @@ -146,6 +148,7 @@ public interface Commands extends Registrar { *

Commands have certain overriding behavior: *

* @@ -163,6 +166,7 @@ public interface Commands extends Registrar { *

Commands have certain overriding behavior: *

* @@ -179,6 +183,7 @@ public interface Commands extends Registrar { *

Commands have certain overriding behavior: *

* diff --git a/paper-server/patches/sources/com/mojang/brigadier/tree/CommandNode.java.patch b/paper-server/patches/sources/com/mojang/brigadier/tree/CommandNode.java.patch index bb97c0d52c..f892ceeba5 100644 --- a/paper-server/patches/sources/com/mojang/brigadier/tree/CommandNode.java.patch +++ b/paper-server/patches/sources/com/mojang/brigadier/tree/CommandNode.java.patch @@ -1,6 +1,6 @@ --- a/com/mojang/brigadier/tree/CommandNode.java +++ b/com/mojang/brigadier/tree/CommandNode.java -@@ -27,11 +_,21 @@ +@@ -27,11 +_,22 @@ private final Map> children = new LinkedHashMap<>(); private final Map> literals = new LinkedHashMap<>(); private final Map> arguments = new LinkedHashMap<>(); @@ -13,6 +13,7 @@ + public CommandNode clientNode; // Paper - Brigadier API + public CommandNode unwrappedCached = null; // Paper - Brigadier Command API + public CommandNode wrappedCached = null; // Paper - Brigadier Command API ++ public io.papermc.paper.command.brigadier.PluginCommandMeta pluginCommandMeta; // Paper - Brigadier Command API + // CraftBukkit start + public void removeCommand(String name) { + this.children.remove(name); diff --git a/paper-server/patches/sources/net/minecraft/commands/Commands.java.patch b/paper-server/patches/sources/net/minecraft/commands/Commands.java.patch index 0c4203d037..b5fdc04cca 100644 --- a/paper-server/patches/sources/net/minecraft/commands/Commands.java.patch +++ b/paper-server/patches/sources/net/minecraft/commands/Commands.java.patch @@ -1,6 +1,6 @@ --- a/net/minecraft/commands/Commands.java +++ b/net/minecraft/commands/Commands.java -@@ -251,6 +_,30 @@ +@@ -251,6 +_,24 @@ PublishCommand.register(this.dispatcher); } @@ -14,17 +14,11 @@ + // Paper start - Brigadier Command API + // Create legacy minecraft namespace commands + for (final CommandNode node : new java.util.ArrayList<>(this.dispatcher.getRoot().getChildren())) { -+ // The brigadier dispatcher is not able to resolve nested redirects. -+ // E.g. registering the alias minecraft:tp cannot redirect to tp, as tp itself redirects to teleport. -+ // Instead, target the first none redirecting node. -+ CommandNode flattenedAliasTarget = node; -+ while (flattenedAliasTarget.getRedirect() != null) flattenedAliasTarget = flattenedAliasTarget.getRedirect(); -+ -+ this.dispatcher.register( -+ com.mojang.brigadier.builder.LiteralArgumentBuilder.literal("minecraft:" + node.getName()) -+ .executes(flattenedAliasTarget.getCommand()) -+ .requires(flattenedAliasTarget.getRequirement()) -+ .redirect(flattenedAliasTarget) ++ this.dispatcher.getRoot().addChild( ++ io.papermc.paper.command.brigadier.PaperBrigadier.copyLiteral( ++ "minecraft:" + node.getName(), ++ (com.mojang.brigadier.tree.LiteralCommandNode) node ++ ) + ); + } + // Paper end - Brigadier Command API @@ -150,11 +144,10 @@ } return null; -@@ -360,25 +_,130 @@ +@@ -360,26 +_,85 @@ } public void sendCommands(ServerPlayer player) { -- Map, CommandNode> map = Maps.newHashMap(); + // Paper start - Send empty commands if tab completion is disabled + if (org.spigotmc.SpigotConfig.tabComplete < 0) { + player.connection.send(new ClientboundCommandsPacket(new RootCommandNode<>())); @@ -182,7 +175,7 @@ + + private void sendAsync(ServerPlayer player, java.util.Collection> dispatcherRootChildren) { + // Paper end - Perf: Async command map building -+ Map, CommandNode> map = Maps.newIdentityHashMap(); // Use identity to prevent aliasing issues + Map, CommandNode> map = Maps.newHashMap(); RootCommandNode rootCommandNode = new RootCommandNode<>(); map.put(this.dispatcher.getRoot(), rootCommandNode); - this.fillUsableCommands(this.dispatcher.getRoot(), rootCommandNode, player.createCommandSourceStack(), map); @@ -224,7 +217,6 @@ Map, CommandNode> commandNodeToSuggestionNode ) { - for (CommandNode commandNode : rootCommandSource.getChildren()) { -+ commandNodeToSuggestionNode.keySet().removeIf((node) -> !org.spigotmc.SpigotConfig.sendNamespaced && node.getName().contains(":")); // Paper - Remove namedspaced from result nodes to prevent redirect trimming ~ see comment below + for (CommandNode commandNode : children) { // Paper - Perf: Async command map building; pass copy of children + // Paper start - Brigadier API + if (commandNode.clientNode != null) { @@ -234,58 +226,16 @@ + if (!org.spigotmc.SpigotConfig.sendNamespaced && commandNode.getName().contains(":")) continue; // Spigot if (commandNode.canUse(source)) { ArgumentBuilder argumentBuilder = (ArgumentBuilder) commandNode.createBuilder(); -+ // Paper start -+ /* -+ Because of how commands can be yeeted right left and center due to bad bukkit practices -+ we need to be able to ensure that ALL commands are registered (even redirects). -+ -+ What this will do is IF the redirect seems to be "dead" it will create a builder and essentially populate (flatten) -+ all the children from the dead redirect to the node. -+ -+ So, if minecraft:msg redirects to msg but the original msg node has been overriden minecraft:msg will now act as msg and will explicilty inherit its children. -+ -+ The only way to fix this is to either: -+ - Send EVERYTHING flattened, don't use redirects -+ - Don't allow command nodes to be deleted -+ - Do this :) -+ */ -+ -+ // Is there an invalid command redirect? -+ if (argumentBuilder.getRedirect() != null && commandNodeToSuggestionNode.get(argumentBuilder.getRedirect()) == null) { -+ // Create the argument builder with the same values as the specified node, but with a different literal and populated children -+ -+ CommandNode redirect = argumentBuilder.getRedirect(); -+ // Diff copied from LiteralCommand#createBuilder -+ final com.mojang.brigadier.builder.LiteralArgumentBuilder builder = com.mojang.brigadier.builder.LiteralArgumentBuilder.literal(commandNode.getName()); -+ builder.requires(redirect.getRequirement()); -+ // builder.forward(redirect.getRedirect(), redirect.getRedirectModifier(), redirect.isFork()); We don't want to migrate the forward, since it's invalid. -+ if (redirect.getCommand() != null) { -+ builder.executes(redirect.getCommand()); -+ } -+ // Diff copied from LiteralCommand#createBuilder -+ for (CommandNode child : redirect.getChildren()) { -+ builder.then(child); -+ } -+ -+ argumentBuilder = builder; -+ } -+ // Paper end argumentBuilder.requires(suggestions -> true); - if (argumentBuilder.getCommand() != null) { +- if (argumentBuilder.getCommand() != null) { - argumentBuilder.executes(commandContext -> 0); -+ // Paper start - fix suggestions due to falsely equal nodes -+ // Always create a new instance -+ //noinspection Convert2Lambda -+ argumentBuilder.executes(new com.mojang.brigadier.Command<>() { -+ @Override -+ public int run(com.mojang.brigadier.context.CommandContext commandContext) { -+ return 0; -+ } -+ }); -+ // Paper end - fix suggestions due to falsely equal nodes - } +- } ++ // Paper - don't replace Command instance on suggestion node ++ // we want the exact command instance to be used for equality checks ++ // when assigning serialization ids to each command node if (argumentBuilder instanceof RequiredArgumentBuilder) { + RequiredArgumentBuilder requiredArgumentBuilder = (RequiredArgumentBuilder)argumentBuilder; @@ -396,7 +_,7 @@ commandNodeToSuggestionNode.put(commandNode, commandNode1); rootSuggestion.addChild(commandNode1); diff --git a/paper-server/src/main/java/io/papermc/paper/command/brigadier/ApiMirrorRootNode.java b/paper-server/src/main/java/io/papermc/paper/command/brigadier/ApiMirrorRootNode.java index 74d7b19627..51cd385821 100644 --- a/paper-server/src/main/java/io/papermc/paper/command/brigadier/ApiMirrorRootNode.java +++ b/paper-server/src/main/java/io/papermc/paper/command/brigadier/ApiMirrorRootNode.java @@ -124,7 +124,7 @@ public abstract class ApiMirrorRootNode extends RootCommandNode nativeWrapperArgumentType) { + } else if (pureArgumentType instanceof final VanillaArgumentProviderImpl.NativeWrapperArgumentType nativeWrapperArgumentType) { converted = this.unwrapArgumentWrapper(pureArgumentNode, nativeWrapperArgumentType, nativeWrapperArgumentType, null); // "null" for suggestion provider so it uses the argument type's suggestion provider /* @@ -140,6 +140,8 @@ public abstract class ApiMirrorRootNode extends RootCommandNode argumentCommandNode = node; @@ -49,8 +45,8 @@ public final class PaperBrigadier { } Map, String> map = PaperCommands.INSTANCE.getDispatcherInternal().getSmartUsage(argumentCommandNode, DUMMY); - String usage = map.isEmpty() ? pluginCommandNode.getUsageText() : pluginCommandNode.getUsageText() + " " + String.join("\n" + pluginCommandNode.getUsageText() + " ", map.values()); - return new PluginVanillaCommandWrapper(pluginCommandNode.getName(), pluginCommandNode.getDescription(), usage, pluginCommandNode.getAliases(), node, pluginCommandNode.getPlugin()); + String usage = map.isEmpty() ? node.getUsageText() : node.getUsageText() + " " + String.join("\n" + node.getUsageText() + " ", map.values()); + return new PluginVanillaCommandWrapper(node.getName(), meta.description(), usage, meta.aliases(), node, meta.plugin()); } /* @@ -70,4 +66,19 @@ public final class PaperBrigadier { } } } + + public static LiteralCommandNode copyLiteral(final String newLiteral, final LiteralCommandNode source) { + // logic copied from LiteralCommandNode#createBuilder + final LiteralArgumentBuilder copyBuilder = LiteralArgumentBuilder.literal(newLiteral) + .requires(source.getRequirement()) + .forward(source.getRedirect(), source.getRedirectModifier(), source.isFork()); + if (source.getCommand() != null) { + copyBuilder.executes(source.getCommand()); + } + + for (final CommandNode child : source.getChildren()) { + copyBuilder.then(child); + } + return copyBuilder.build(); + } } diff --git a/paper-server/src/main/java/io/papermc/paper/command/brigadier/PaperCommands.java b/paper-server/src/main/java/io/papermc/paper/command/brigadier/PaperCommands.java index 95d3b42cbe..f9a370433d 100644 --- a/paper-server/src/main/java/io/papermc/paper/command/brigadier/PaperCommands.java +++ b/paper-server/src/main/java/io/papermc/paper/command/brigadier/PaperCommands.java @@ -21,23 +21,20 @@ import java.util.Set; import net.minecraft.commands.CommandBuildContext; import org.apache.commons.lang3.ArrayUtils; import org.apache.commons.lang3.StringUtils; -import org.checkerframework.checker.nullness.qual.MonotonicNonNull; -import org.checkerframework.checker.nullness.qual.NonNull; -import org.checkerframework.checker.nullness.qual.Nullable; -import org.checkerframework.framework.qual.DefaultQualifier; -import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Unmodifiable; +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; import static java.util.Objects.requireNonNull; -@DefaultQualifier(NonNull.class) +@NullMarked public class PaperCommands implements Commands, PaperRegistrar { public static final PaperCommands INSTANCE = new PaperCommands(); private @Nullable LifecycleEventOwner currentContext; - private @MonotonicNonNull CommandDispatcher dispatcher; - private @MonotonicNonNull CommandBuildContext buildContext; + private @Nullable CommandDispatcher dispatcher; + private @Nullable CommandBuildContext buildContext; private boolean invalid = false; @Override @@ -93,65 +90,47 @@ public class PaperCommands implements Commands, PaperRegistrar registerWithFlags(@NotNull final PluginMeta pluginMeta, @NotNull final LiteralCommandNode node, @org.jetbrains.annotations.Nullable final String description, @NotNull final Collection aliases, @NotNull final Set flags) { - final boolean hasFlattenRedirectFlag = flags.contains(CommandRegistrationFlag.FLATTEN_ALIASES); + public @Unmodifiable Set registerWithFlags(final PluginMeta pluginMeta, final LiteralCommandNode node, final @Nullable String description, final Collection aliases, final Set flags) { + final PluginCommandMeta meta = new PluginCommandMeta(pluginMeta, description); final String identifier = pluginMeta.getName().toLowerCase(Locale.ROOT); final String literal = node.getLiteral(); - final PluginCommandNode pluginLiteral = new PluginCommandNode(identifier + ":" + literal, pluginMeta, node, description); // Treat the keyed version of the command as the root + final LiteralCommandNode pluginLiteral = PaperBrigadier.copyLiteral(identifier + ":" + literal, node); final Set registeredLabels = new HashSet<>(aliases.size() * 2 + 2); if (this.registerIntoDispatcher(pluginLiteral, true)) { registeredLabels.add(pluginLiteral.getLiteral()); } - if (this.registerRedirect(literal, pluginMeta, pluginLiteral, description, true, hasFlattenRedirectFlag)) { // Plugin commands should override vanilla commands + if (this.registerIntoDispatcher(node, true)) { // Plugin commands should override vanilla commands registeredLabels.add(literal); } // Add aliases final List registeredAliases = new ArrayList<>(aliases.size() * 2); for (final String alias : aliases) { - if (this.registerRedirect(alias, pluginMeta, pluginLiteral, description, false, hasFlattenRedirectFlag)) { + if (this.registerCopy(alias, pluginLiteral, meta)) { registeredAliases.add(alias); } - if (this.registerRedirect(identifier + ":" + alias, pluginMeta, pluginLiteral, description, false, hasFlattenRedirectFlag)) { + if (this.registerCopy(identifier + ":" + alias, pluginLiteral, meta)) { registeredAliases.add(identifier + ":" + alias); } } - if (!registeredAliases.isEmpty()) { - pluginLiteral.setAliases(registeredAliases); - } + pluginLiteral.pluginCommandMeta = new PluginCommandMeta(pluginMeta, description, registeredAliases); registeredLabels.addAll(registeredAliases); return registeredLabels.isEmpty() ? Collections.emptySet() : Collections.unmodifiableSet(registeredLabels); } - private boolean registerRedirect(final String aliasLiteral, final PluginMeta plugin, final PluginCommandNode redirectTo, final @Nullable String description, final boolean override, boolean hasFlattenRedirectFlag) { - final LiteralCommandNode redirect; - if (redirectTo.getChildren().isEmpty() || hasFlattenRedirectFlag) { - redirect = Commands.literal(aliasLiteral) - .executes(redirectTo.getCommand()) - .requires(redirectTo.getRequirement()) - .build(); - - for (final CommandNode child : redirectTo.getChildren()) { - redirect.addChild(child); - } - } else { - redirect = Commands.literal(aliasLiteral) - .executes(redirectTo.getCommand()) - .redirect(redirectTo) - .requires(redirectTo.getRequirement()) - .build(); - } - - return this.registerIntoDispatcher(new PluginCommandNode(aliasLiteral, plugin, redirect, description), override); + private boolean registerCopy(final String aliasLiteral, final LiteralCommandNode redirectTo, final PluginCommandMeta meta) { + final LiteralCommandNode node = PaperBrigadier.copyLiteral(aliasLiteral, redirectTo); + node.pluginCommandMeta = meta; + return this.registerIntoDispatcher(node, false); } - private boolean registerIntoDispatcher(final PluginCommandNode node, boolean override) { - final @Nullable CommandNode existingChild = this.getDispatcher().getRoot().getChild(node.getLiteral()); - if (existingChild != null && !(existingChild instanceof PluginCommandNode) && !(existingChild instanceof BukkitCommandNode)) { + private boolean registerIntoDispatcher(final LiteralCommandNode node, boolean override) { + final CommandNode existingChild = this.getDispatcher().getRoot().getChild(node.getLiteral()); + if (existingChild != null && existingChild.pluginCommandMeta == null && !(existingChild instanceof BukkitCommandNode)) { override = true; // override vanilla commands } if (existingChild == null || override) { // Avoid merging behavior. Maybe something to look into in the future diff --git a/paper-server/src/main/java/io/papermc/paper/command/brigadier/PluginCommandMeta.java b/paper-server/src/main/java/io/papermc/paper/command/brigadier/PluginCommandMeta.java new file mode 100644 index 0000000000..45701bcca0 --- /dev/null +++ b/paper-server/src/main/java/io/papermc/paper/command/brigadier/PluginCommandMeta.java @@ -0,0 +1,26 @@ +package io.papermc.paper.command.brigadier; + +import io.papermc.paper.plugin.configuration.PluginMeta; +import java.util.Collections; +import java.util.List; +import java.util.Objects; +import org.bukkit.Bukkit; +import org.bukkit.plugin.Plugin; +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; + +@NullMarked +public record PluginCommandMeta(PluginMeta pluginMeta, @Nullable String description, List aliases) { + + public PluginCommandMeta(final PluginMeta pluginMeta, final @Nullable String description) { + this(pluginMeta, description, Collections.emptyList()); + } + + public PluginCommandMeta { + aliases = List.copyOf(aliases); + } + + public Plugin plugin() { + return Objects.requireNonNull(Bukkit.getPluginManager().getPlugin(this.pluginMeta.getName())); + } +} diff --git a/paper-server/src/main/java/io/papermc/paper/command/brigadier/PluginCommandNode.java b/paper-server/src/main/java/io/papermc/paper/command/brigadier/PluginCommandNode.java deleted file mode 100644 index 3a9f58873b..0000000000 --- a/paper-server/src/main/java/io/papermc/paper/command/brigadier/PluginCommandNode.java +++ /dev/null @@ -1,50 +0,0 @@ -package io.papermc.paper.command.brigadier; - -import com.mojang.brigadier.tree.CommandNode; -import com.mojang.brigadier.tree.LiteralCommandNode; -import java.util.Collections; -import java.util.List; -import java.util.Objects; -import io.papermc.paper.plugin.configuration.PluginMeta; -import org.bukkit.Bukkit; -import org.bukkit.plugin.Plugin; -import org.jetbrains.annotations.NotNull; -import org.jetbrains.annotations.Nullable; - -public class PluginCommandNode extends LiteralCommandNode { - - private final PluginMeta plugin; - private final String description; - private List aliases = Collections.emptyList(); - - public PluginCommandNode(final @NotNull String literal, final @NotNull PluginMeta plugin, final @NotNull LiteralCommandNode rootLiteral, final @Nullable String description) { - super( - literal, rootLiteral.getCommand(), rootLiteral.getRequirement(), - rootLiteral.getRedirect(), rootLiteral.getRedirectModifier(), rootLiteral.isFork() - ); - this.plugin = plugin; - this.description = description; - - for (CommandNode argument : rootLiteral.getChildren()) { - this.addChild(argument); - } - } - - @NotNull - public Plugin getPlugin() { - return Objects.requireNonNull(Bukkit.getPluginManager().getPlugin(this.plugin.getName())); - } - - @NotNull - public String getDescription() { - return this.description; - } - - public void setAliases(List aliases) { - this.aliases = aliases; - } - - public List getAliases() { - return this.aliases; - } -}