neoforged / NeoForge

Neo Modding API for Minecraft, based on Forge
https://projects.neoforged.net/neoforged/neoforge
Other
1.1k stars 161 forks source link

Mod transformers are applied inconsistently if loading fails #43

Closed embeddedt closed 9 months ago

embeddedt commented 1 year ago

If early mod loading fails (e.g. due to a missing dependency), mod access transformers are not applied (since only the ATs from system mods are loaded) but their mixins are still applied. If the mod mixes into a class that is used within the code path to display the dependency error screen, and that mixin references a field in a manner that requires the mod's access transformer to also have been applied, the game will crash.

To see this in the wild, install Rubidium 0.6.5 and Sophisticated Backpacks 3.18.56.890 on Forge 47.1.0. Be sure not to install Sophisticated Core. Rather than displaying the missing dependency screen, the game will crash with an IllegalAccessError, as Rubidium includes a mixin that depends on its access transformer.

In my opinion this design is confusing and inconsistent. It also creates misleading crash reports for users that show the IllegalAccessError instead of the actual problem (which is that a a dependency is installed). Both access transformers and mixins are modifying core game code, so it seems prudent to either not apply mod mixins in this scenario, or also apply mod ATs. This current design works in the case that the mod uses only ATs and Forge events (as its event handlers are skipped) but does not work in a Mixin world.

It's possible to work around the issue by checking ModLoader.isLoadingStateValid() (after constructors fire) or LoadingModList.get().getErrors().isEmpty() (before constructors fire) in critical mixins, but I don't think modders should have to do this.

MehVahdJukaar commented 1 year ago

This would prevent so many headaches to us modders as people don't install dependencies and get hard crashes like this all the times. I did point this out before in MCF and answer was that it didn't matter as mixins/coremods shouldn't be used

Technici4n commented 9 months ago

Fixed by https://github.com/neoforged/FancyModLoader/pull/31.