In dev env, @ModifyVariable doesn't check the entire method descriptor

[mega12345mega]

In the development environment, @ModifyVariable doesn't check that the arguments of the target method matches. (The return value may also not be checked, I haven't tested that.) For example:

@Mixin(ClickSlotC2SPacket.class)
public class ClickSlotC2SPacketMixin {
  @ModifyVariable(method = "<init>(IIIILnet/minecraft/screen/slot/SlotActionType;Lnet/minecraft/item/ItemStack;Lit/unimi/dsi/fastutil/ints/Int2ObjectMap;)V", at = @At("HEAD"), ordinal = 3)
  @Group(name = "<init>", min = 1)
  private static int init_new(int button) {
    System.out.println("New called");
    return button;
  }
  // Note III instead of IIII
  @ModifyVariable(method = "<init>(IIILnet/minecraft/class_1713;Lnet/minecraft/class_1799;Lit/unimi/dsi/fastutil/ints/Int2ObjectMap;)V", at = @At("HEAD"), ordinal = 2, remap = false)
  @Group(name = "<init>", min = 1)
  @SuppressWarnings("target")
  private static int init_old(int button) {
    System.out.println("Old called");
    return button;
  }
}

When running in Minecraft 1.20.6 with Fabric and Mixin version 0.8.5, both print statements are called, even though there isn't a constructor with 3 int arguments. This doesn't happen when running in either 1.17 or 1.20.6 outside of the development environment.

Since I haven't encountered any similar issues before, I assume that the fact that both named and intermediary yarn mappings use the same method name is what allows this to happen (or more specifically, both are <init>). Otherwise, the second @ModifyVariable would fail to find a target method.

[Earthcomputer]

Can you check if this still happens on 0.8.6? It reminds me of #515 which was fixed a few months ago.

[mega12345mega]

How do you change the Mixin version in Fabric? I tried adding 0.8.6 to the build.gradle with modImplementation (I'm guessing that is incorrect) and removing the auto generated 0.8.5 from the library list, but that made it crash due to "this.env" is null. I also assume that users of the mod would still be running it with the older Mixin version?

My loader version is 0.15.11, my Fabric version is 0.100.8+1.20.6, and my Fabric Loom version is 1.7-SNAPSHOT, which I believe is the most up to date for 1.20.6. (Or do I just need to update to newer versions of Minecraft?)

Looking through the commit that fixed the issue you mentioned, it seems like it only changes RedirectInjector and BeforeNew. The Target changes seem to be for NEW & INVOKESPECIAL specifically, assuming the Javadoc is still accurate (while INVOKESPECIAL could refer to a super() call, I don't believe NEW is called inside the constructor). This leads me to think that it wouldn't fix this, since this is the constructor itself rather than the caller of the constructor being modified, but I obviously can't be sure.

[Geolykt]

Also do note that fabric is running their own fork of mixin, meaning that the issues you are experiencing are potentially regressions in fabric's variant.

[LlamaLad7]

This behaviour is deliberate, if the descriptor doesn't match anything then mixin does a separate pass with just the name.

[mega12345mega]

This behaviour is deliberate, if the descriptor doesn't match anything then mixin does a separate pass with just the name.

Doesn't that mean that it should have the same problem outside of the development environment in 1.20.6? Because it doesn't happen in that case.

[Earthcomputer]

This behaviour is deliberate, if the descriptor doesn't match anything then mixin does a separate pass with just the name.

Wait what? That's very silly imo. Is there a way to make it not do that?

[mega12345mega]

I've just had the same issue with a @Inject, which unlike the @ModifyVariable case where I can simply check the Minecraft version, I will need to instead setup a mixin plugin to avoid a crash. If this really is intended behavior, a option on the annotations should definitely be added to disable it as it seems excessive to have to use a mixin plugin to avoid this. That said, as I mentioned above, it seems strange then that the @ModifyVariable example doesn't occur outside of the development environment, which implies the behavior may not be fully deliberate. (That said, maybe Fabric changed something as @Geolykt mentioned. @LlamaLad7 presumably you're specifically talking about normal mixin, not Fabric's mixin fork?)

@Mixin(HorseScreenHandler.class)
public class HorseScreenHandlerMixin {
    @Inject(method = "<init>(ILnet/minecraft/entity/player/PlayerInventory;Lnet/minecraft/inventory/Inventory;Lnet/minecraft/entity/passive/AbstractHorseEntity;I)V", at = @At("HEAD"))
    @Group(name = "initHead", min = 1)
    private static void initHead(int syncId, PlayerInventory playerInventory, Inventory inventory, AbstractHorseEntity horse, int slotColumnCount, CallbackInfo info) {
        ...
    }
    @Inject(method = "<init>(ILnet/minecraft/class_1661;Lnet/minecraft/class_1263;Lnet/minecraft/class_1496;)V", at = @At("HEAD"), remap = false)
    @Group(name = "initHead", min = 1)
    @SuppressWarnings("target")
    private static void initHead_old(int syncId, PlayerInventory playerInventory, Inventory inventory, AbstractHorseEntity horse, CallbackInfo info) {
        ...
    }

    ...
}

Causes:

org.spongepowered.asm.mixin.injection.throwables.InvalidInjectionException: Invalid descriptor on nbteditor.mixins.json:HorseScreenHandlerMixin from mod nbteditor->@Inject::initHead_old(ILnet/minecraft/entity/player/PlayerInventory;Lnet/minecraft/inventory/Inventory;Lnet/minecraft/entity/passive/AbstractHorseEntity;Lorg/spongepowered/asm/mixin/injection/callback/CallbackInfo;)V! Expected (ILnet/minecraft/entity/player/PlayerInventory;Lnet/minecraft/inventory/Inventory;Lnet/minecraft/entity/passive/AbstractHorseEntity;ILorg/spongepowered/asm/mixin/injection/callback/CallbackInfo;)V but found (ILnet/minecraft/entity/player/PlayerInventory;Lnet/minecraft/inventory/Inventory;Lnet/minecraft/entity/passive/AbstractHorseEntity;Lorg/spongepowered/asm/mixin/injection/callback/CallbackInfo;)V [INJECT_APPLY Applicator Phase -> nbteditor.mixins.json:HorseScreenHandlerMixin from mod nbteditor -> Apply Injections ->  -> Inject -> nbteditor.mixins.json:HorseScreenHandlerMixin from mod nbteditor->@Inject::initHead_old(ILnet/minecraft/entity/player/PlayerInventory;Lnet/minecraft/inventory/Inventory;Lnet/minecraft/entity/passive/AbstractHorseEntity;Lorg/spongepowered/asm/mixin/injection/callback/CallbackInfo;)V]

(Note that I haven't tested this case outside of the development environment yet)