CallbackInfoReturnable suddenly null with Fabric Loader 0.12 / Mixin 0.8.4
Fourmisain opened this issue · 11 comments
Not quite sure if this is the right place to report this, let's go with it anyway.
RightClickHarvest started crashing with the new loader when right clicking crops:
java.lang.NullPointerException: Cannot invoke "org.spongepowered.asm.mixin.injection.callback.CallbackInfoReturnable.setReturnValue(Object)" because "info" is null
at net.minecraft.class_2302.handler$zho000$onUseMixin(class_2302.java:558)
The relevant code is found here:
https://github.com/JamCoreModding/RightClickHarvestFabric/blob/e1dc1fc264b2576270c8cf6547b2960f19ee7f9c/src/main/java/io/github/jamalam360/mixin/AbstractBlockMixin.java#L43-L45
which is then overridden e.g. by
https://github.com/JamCoreModding/RightClickHarvestFabric/blob/e1dc1fc264b2576270c8cf6547b2960f19ee7f9c/src/main/java/io/github/jamalam360/mixin/CropBlockMixin.java#L47
So it's simple Mixin inheritance, the CropBlockMixin is supposed to cancel the AbstractBlock.onUse() method but it fails because the CallbackInfoReturnable is null.
The interesting part is that when you add a use of info inside AbstractBlockMixin.onUseMixin(), e.g. something like
info.getId(); or maybe throw new AssertionError(info); if you expect it never being called directly
then info won't be null, despite this code never being called.
This almost looks like some failing optimization that eliminates the CallbackInfoReturnable because it didn't think it was necessary.
The decompilation of AbstractBlock pretty much looks like expected given this information:
Broken when AbstractBlockMixin.onUseMixin() doesn't refer to info:
Working when AbstractBlockMixin.onUseMixin() does refer to info:
I have no idea if this happens with base Mixin or if it's just the Fabric fork, hence I reported it here.
Ah yeah, that'd do it!
So the issue seems to be that it looks at each mixin (or rather CallbackInjector) in isolation, and eliminates the CallBackInfo if it isn't being used, but that assumes there cannot be another mixin overriding that injected method which does use the CallBackInfo.
This optimization could still be safely done if it checks the mixin hierachy - if there's no inheriting mixin using the CallBackInfo, it can be eliminated.
I have no clue how easy that is to implement, since it needs to know/access all possible mixins at this point.
At least there's an easy workaround, so if things go awry, it could e.g. be documented in the fabric mixin tutorial, maybe also pinned in the #mod-dev-mixin channel.
I guess I'll leave a ping and cross-reference here @Chocohead #52
I think it should be fine to only try to avoid creating the callback info when the mod is targetting loader 0.12+. (Same as some of the other new fixes)
This is an odd pattern for a mixin IMO, and the work around throwing the AssertionError is something I'd prob do anyway. Making this work nicely for a case like this looks like a right pita/large changes to mixin.
This is an odd pattern for a mixin IMO
It's just mixin inheritance though, from what I heard, Sponge uses it a ton and it's been regularly recommended by SpongePowered's #mixin channel and mentioned a non-0 number of times on fabricord too.
It's arguably cleaner than the alternative of doing a bunch of instanceof checks in the super class since it's directly making use of Java's native virtual function calls - should have slightly better performance too, though it's also slightly less mod compatible since no 2 mods can add the same method without it being overwritten.
the work around throwing the AssertionError is something I'd prob do anyway
Yeah, throwing an AssertionError in methods that should never be called is good practive, the small but important difference is knowing to put the info object in there.
Making this work nicely for a case like this looks like a right pita/large changes to mixin.
I think it should be fine to only try to avoid creating the callback info when the mod is targetting loader 0.12+
Sounds good!
People need to check their mixins due to the changed/fixed locals detection anyway, so they should stumble upon this issue too, just need to tell them how to fix it.
P.S.
I just realized that in the specific case of RightClickHarvest, throw new AssertionError(info); wouldn't work since it'd cause a crash for every non-crop block, since it's a mixin into AbstractBlock.
A safer general workaround would be info.getId();.
It is definitely fixable properly, shouldn't be too bad.
The local detection change is btw largely worked around by some backwards compat facilities, any mod targeting a min loader version < 0.12 will see the old behavior.
The logic is just whether the handler method loads the callback at all, an assert info != null or even if (false) info.toString() would make it appear used and so wouldn't be passed as null.
The issue should get fixed universally (as opposed to leaving the optimisation for 0.12+ mods) given Mixin loads so much information about present mixins anyway there's not a whole lot of redundant processing needed to account for inheritance of the handling method itself.
I just tested the new loader 0.12.3 with RightClickHarvest in the dev env and a fresh installation but the callback info object is still null.
Is there an easy way to view the debug output of the "mixin" logger being used in #69?
I can't seem to get -Dlog4j.configurationFile=... to work in the dev env or in the vanilla launcher for some reason.
Was able to get logs from a server instance, though they don't say much (simplified the log a bit):
(FabricLoader/Mixin): AbstractBlockMixin.onUseMixin from mod rightclickharvest doesn't use it's CallbackInfoReturnable
(FabricLoader/Mixin): AbstractBlockMixin.onUseMixin from mod rightclickharvest has 0 override(s) in child classes
(FabricLoader/Mixin): AbstractBlockMixin.onUseMixin from mod rightclickharvest won't be passed a CallbackInfoReturnable as a result
Relevant part is here, so it's something in or before the findOverrides logic.
Just a guess but could it be that that parentMixins isn't fully initialized when findOverrides is called, meaning it fails because it doesn't have all the needed information yet?
Specifically the question is if MixinInheritanceTracker.onInit() is executed for every mixin (of a given mixin config) before findOverrides is called for the first time.
I'd love to do some debugging on this, but I fear setting up the loader with a custom Mixin build (or setting up a debug Mixin project) might be beyond me.
Did some testing with 0.12.4 and can confirm that the general issue is fixed! 🎉
Not to rain on the parade, but I did find one case where it can fail, namely for package level overrides.
This means something in this check fails:
I'm not exactly sure but I think owner and child.getName() have a different format because the former is ClassInfo.getName() while the latter is MixinInfo.getName().
ClassInfo.getName() is the same as ClassNode.name, which is the same as org.objectweb.asm.Type.getInternalName, which according to the docs
Returns the internal name of the class corresponding to this object or array type. The internal name of a class is its fully qualified name (as returned by Class.getName(), where '.' are replaced by '/').
If the Mixin package is project.mixin and there's a sub.TestMixin Mixin in there, this probably looks like sub/TestMixin, which makes sense given the code.
Looking at where MixinInfo.getName() is set ultimately leads to
Mixin/src/main/java/org/spongepowered/asm/mixin/transformer/MixinConfig.java
Lines 260 to 264 in 20d9184
From this description the names probably look like project.mixin.sub.TestMixin instead,
or maybe project/mixin/sub/TestMixin - both would explain why it fails.
It's not only that they have a different format, it's not even a sensible comparison.
When I first sketched the design out child was a ClassInfo, so getName returned the binary name of the (child) mixin class. Later down the line it was switched to be a MixinInfo (as the method bytecode was needed to check for CallbackInfo use which ClassInfo doesn't keep) which also has a getName method. That method is not getting the binary name however, it gets the Java name minus the root mixin package.
So not only is one using slashes and the other dots, the parent is its full package name but the child only partial. Annoyingly (or maybe not) it's another single line fix