SpongePowered / Mixin

Mixin is a trait/mixin and bytecode weaving framework for Java using ASM
MIT License
1.37k stars 185 forks source link

LocalVariable capture isn't matching the injection point in 0.8.7 snapshot #671

Open aromaa opened 3 weeks ago

aromaa commented 3 weeks ago

Testing out the 0.8.7 snapshot on Sponge seems to cause some new failures on existing Mixins around the LocalVariable capture. The following Mixin here decompiles to:

private static boolean burn(RegistryAccess $$0, @Nullable RecipeHolder $$1, NonNullList $$2, int $$3) {
    if ($$1 != null && canBurn($$0, $$1, $$2, $$3)) {
        ItemStack $$4 = (ItemStack)$$2.get(0);
        ItemStack $$5 = $$1.value().getResultItem($$0);
        ItemStack $$6 = (ItemStack)$$2.get(2);
        if ($$6.isEmpty()) {
            $$2.set(2, $$5.copy());
        } else if (ItemStack.isSameItemSameComponents($$6, $$5)) {
            $$6.grow(1);
        }

        if ($$4.is(Blocks.WET_SPONGE.asItem()) && !((ItemStack)$$2.get(1)).isEmpty() && ((ItemStack)$$2.get(1)).is(Items.BUCKET)) {
            $$2.set(1, new ItemStack(Items.WATER_BUCKET));
        }

        $$4.shrink(1);
        handler$bcf000$vanillaImpl$afterSmeltItem($$0, $$1, $$2, $$3, new CallbackInfoReturnable("burn", false));
        return true;
    } else {
        return false;
    }
}

but the LocalVariable print seems to mismatch on what's actually in the stack:

/*****************************************************************************************************************************************************************/
/*       Target Class : net.minecraft.world.level.block.entity.AbstractFurnaceBlockEntity                                                                        */
/*      Target Method : private boolean burn(RegistryAccess $$0, RecipeHolder $$1, NonNullList $$2, int $$3)                                                     */
/*  Target Max LOCALS : 7                                                                                                                                        */
/* Initial Frame Size : 4                                                                                                                                        */
/*      Callback Name : vanillaImpl$afterSmeltItem                                                                                                               */
/*        Instruction : InjectionNode UNKNOWN                                                                                                                    */
/*****************************************************************************************************************************************************************/
/*   LOCAL                  TYPE  NAME                                                                                                                           */
/*   [  0]        RegistryAccess  $$0                                                                                                                            */
/*   [  1]          RecipeHolder  $$1                                                                                                                            */
/*   [  2]           NonNullList  $$2                                                                                                                            */
/*   [  3]                   int  $$3                                                                                                                            */
/* > [  4]             ItemStack  $$4                                                <capture>                                                                   */
/*   [  5]                     -                                                                                                                                 */
/*   [  6]                     -                                                                                                                                 */
/*****************************************************************************************************************************************************************/
/*                                                                                                                                                               */
/* /**                                                                                                                                                           */
/*  * Expected callback signature                                                                                                                                */
/*  * /                                                                                                                                                          */
/* private void vanillaImpl$afterSmeltItem(RegistryAccess $$0, RecipeHolder $$1, NonNullList $$2, int $$3, CallbackInfoReturnable<Boolean> cir, ItemStack $$4) { */
/*     // Method body                                                                                                                                            */
/* }                                                                                                                                                             */
/*                                                                                                                                                               */
/*****************************************************************************************************************************************************************/
Mumfrey commented 3 weeks ago

So having investigated this, this appears to be a result of a13f0c0db0be4f6ff691b78b498e928a6d7504bf which is intended to bring the ClassReader flags in line with the flags used when classes are read for transformation in ModLauncher itself. However there are cases where EXPAND_FRAMES yields slightly different results when generating Locals, despite my best efforts to reduce those cases.

In this case, the two additional ItemStacks are technically still in scope at the injection point, but have achieved "zombie" status because they have also been technically removed by a frame CHOP which has taken them out of the frame from the JVM's point of view, even though they are still technically available in those frame positions.

For the moment, I've extended the capability of the new property mixin.tunable.classReaderExpandFrames to also affect metadata class nodes, so enabling that option with the new snapshot should correct the issue here (at least it does in my testing), but it's probably worth considering whether the flag should default to the old value and the tunable's behaviour be inverted (so that for example Forge can deassert it).