Fix wrong redirect behavior (MC-256419)

[zly2006]

Overview

Brigadier set the variable foundCommand to true HERE even if no Command<S> was executed, and this cause a serious problem.

An example to this is in minecraft: if you execute the command /xp query Steve, you will not receive an error message from the server. This is because that xp is redirected from experience, and when brigadier found this command is redirected, it set foundCommand to true--even if nothing is actually executed.

Ways to fix

I think the best way is to remove this wrong assignment. I tried in fabric to modify this code like below, and it works.

package com.github.zly2006.testmod.mixin;

import com.mojang.brigadier.CommandDispatcher;
import com.mojang.brigadier.ParseResults;
import com.mojang.brigadier.context.CommandContext;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.injection.At;
import org.spongepowered.asm.mixin.injection.Inject;
import org.spongepowered.asm.mixin.injection.ModifyVariable;
import org.spongepowered.asm.mixin.injection.callback.CallbackInfoReturnable;
import org.spongepowered.asm.mixin.injection.callback.LocalCapture;

import java.util.ArrayList;
import java.util.List;

@Mixin(value = CommandDispatcher.class, remap = false)
public class MixinCommandDispatcher {
    boolean modify = false;
    boolean valueBefore = false;
    @ModifyVariable(method = "execute(Lcom/mojang/brigadier/ParseResults;)I", at = @At(value = "STORE"), ordinal = 1)
    private boolean modify(boolean v) {
        if (modify) {
            modify = false;
            return valueBefore;
        }
        return v;
    }

    @Inject(method = "execute(Lcom/mojang/brigadier/ParseResults;)I", at = @At(value = "INVOKE", target = "Lcom/mojang/brigadier/context/CommandContext;hasNodes()Z"), locals = LocalCapture.CAPTURE_FAILSOFT)
    private void checkFix(ParseResults<?> parse, CallbackInfoReturnable<Integer> cir, int result, int successfulForks, boolean forked, boolean foundCommand, String command, CommandContext<?> original, List<?> contexts, ArrayList<?> next, int size, int i, CommandContext<?> context, CommandContext<?> child) {
        if (child.hasNodes()) {
            modify = true;
            valueBefore = foundCommand;
        }
        else {
            modify = false;
        }
    }
}

The result after my modification was tested in Minecraft 1.19. With this modification, /xp query Steve will throw a CommandSyntaxException as expected and an error message from the server was received.

[zly2006]

This bug is also reported at JIRA.

[Gegy]

Thank you so much for this PR! 😊 Noticed a tiny issue, but should be a small change 🙂

[Gegy]

Looks great to me, thanks again! 🎉

[slicedlime]

Looks good to me - thank you for also adding tests!

[zly2006]

Seems that https://bugs.mojang.com/browse/MC-256419 and https://bugs.mojang.com/browse/MC-165773 can be closed as fixed. @slicedlime @Gegy

[slicedlime]

Not quite - they will be closed when a Minecraft version is released using the updated library.