Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
aromaa opened this issue Jul 4, 2024 · 1 comment

Comments

@aromaa
Copy link
Member

aromaa commented Jul 4, 2024

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
Copy link
Member

Mumfrey commented Jul 4, 2024

So having investigated this, this appears to be a result of a13f0c0 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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants