LlamaLad7 / MixinExtras

Companion library to SpongePowered Mixin with many custom injectors for a more expressive experience.
MIT License
319 stars 17 forks source link

final field accessed via lambda by `@WrapOperation` #53

Closed NicBOMB closed 1 year ago

NicBOMB commented 1 year ago

Source: https://github.com/Vivecraft/VivecraftMod/pull/191/files#diff-27b99287eb2f32e8c6825a339b936ebb9e82785501cfcb57774fd418b2dd813a Production:

    @MixinMerged(
        mixin = "org.vivecraft.mixin.client_vr.OptionsVRMixin",
        priority = 1000,
        sessionId = "9a7dff99-503d-479d-94c9-bc69ee4a339c"
    )
    void wrapOperation$zcn000$vivecraft$processOptionsMixin(class_315 instance, class_304[] keyMappings, Operation original) {
        if (keyMappings != null) {
            if (keyMappings.length > 0) {
                keyMappings = (class_304[])Stream.concat(Arrays.stream(keyMappings), VivecraftVRMod.userKeyBindingSet.stream()).toArray((x$0) -> {
                    return new class_304[x$0];
                });
                VivecraftVRMod.vanillaBindingSet.addAll(Arrays.asList(keyMappings));
                VRSettings.logger.info("keyMappings is: {}", Arrays.toString(keyMappings));
                original.call(new Object[]{instance, keyMappings});
            } else {
                VRSettings.logger.error("keyMappings is empty! {}", Arrays.toString(keyMappings));
                throw new MissingResourceException("keyMappings is empty!", keyMappings.getClass().getName(), Arrays.toString(keyMappings));
            }
        } else {
            VRSettings.logger.error("keyMappings is null!");
            throw new NullPointerException("keyMappings is null!");
        }
    }

Production Mixin:

        class_304[] injectorAllocatedLocal6 = (class_304[])ArrayUtils.addAll(new class_304[]{this.field_1886, this.field_1904, this.field_1894, this.field_1913, this.field_1881, this.field_1849, this.field_1903, this.field_1832, this.field_1867, this.field_1869, this.field_1822, this.field_1890, this.field_1907, this.field_1871, this.field_1845, this.field_26845, this.field_1835, this.field_1824, this.field_1816, this.field_1836, this.field_1906, this.field_1831, this.field_1879, this.field_1874, this.field_1844}, this.field_1852);
        this.wrapOperation$zcn000$vivecraft$processOptionsMixin(this, injectorAllocatedLocal6, (var0) -> {
            WrapOperationRuntime.checkArgumentCount(var0, 2, "[net.minecraft.class_315, net.minecraft.class_304[]]");
            ((class_315)var0[0]).field_1839 = (class_304[])var0[1];
            return (Void)null;
        });

Error:

java.lang.IllegalAccessError: Update to non-static final field net.minecraft.class_315.field_1839 attempted from a different method (mixinextras$bridge$field_1839$213) than the initializer method <init> 
    at net.minecraft.class_315.mixinextras$bridge$field_1839$213(class_315.java)
    at net.minecraft.class_315.wrapOperation$zck000$vivecraft$processOptionsMixin(class_315.java:2308)
    at net.minecraft.class_315.<init>(class_315.java:892)
    at net.minecraft.class_310.<init>(class_310.java:460)
    at net.minecraft.client.main.Main.main(Main.java:211)
    at net.fabricmc.loader.impl.game.minecraft.MinecraftGameProvider.launch(MinecraftGameProvider.java:468)
    at net.fabricmc.loader.impl.launch.knot.Knot.launch(Knot.java:74)
    at net.fabricmc.loader.impl.launch.knot.KnotClient.main(KnotClient.java:23)

I was expecting the mixin to be more like:

        class_304[] injectorAllocatedLocal6 = (class_304[])ArrayUtils.addAll(new class_304[]{this.field_1886, this.field_1904, this.field_1894, this.field_1913, this.field_1881, this.field_1849, this.field_1903, this.field_1832, this.field_1867, this.field_1869, this.field_1822, this.field_1890, this.field_1907, this.field_1871, this.field_1845, this.field_26845, this.field_1835, this.field_1824, this.field_1816, this.field_1836, this.field_1906, this.field_1831, this.field_1879, this.field_1874, this.field_1844}, this.field_1852);
-        this.wrapOperation$zcn000$vivecraft$processOptionsMixin(this, injectorAllocatedLocal6, (var0) -> {
+        ((class_315)var0[0]).field_1839 = this.wrapOperation$zcn000$vivecraft$processOptionsMixin(this, injectorAllocatedLocal6, (var0) -> {
            WrapOperationRuntime.checkArgumentCount(var0, 2, "[net.minecraft.class_315, net.minecraft.class_304[]]");
-            ((class_315)var0[0]).field_1839 = (class_304[])var0[1];
+            return (class_304[])var0[1];
        });

The example from the wiki keeps the left hand side out of the lambda:

- int number = this.expensiveCalculation(flag);
+ int number = this.bypassExpensiveCalculationIfNecessary(this, flag, args -> {
+         return ((TargetClass) args[0]).expensiveCalculation((Boolean) args[1]);
+ });
LlamaLad7 commented 1 year ago

This is intended behavior. The example on the wiki wraps a method call, you're wrapping the field set. If it worked like your "expected code" then you wouldn't be able to prevent the field set happening and it wouldn't be a proper "wrapping". In this particular case you would have to make the field @Mutable via a shadow.

NicBOMB commented 1 year ago

This is intended behavior. The example on the wiki wraps a method call, you're wrapping the field set. If it worked like your "expected code" then you wouldn't be able to prevent the field set happening and it wouldn't be a proper "wrapping". In this particular case you would have to make the field @Mutable via a shadow.

Thanks, that's a great explanation for the behavior.