Option to attempt to detect coremods that apply their patches only on the first call
Barteks2x opened this issue · 2 comments
This is now the second time I've seen a coremod that breaks when used together with mixin, because it's implemented with essentially Map<String, Patch> and the entries are removed after applying.
This mostly silently breaks such coremods with mixins, and if someone isn't aware of it, it can take a really long time to debug such a seemingly simple issue.
I propose adding a mixin.debug option that, when a transformer is first called, calls it a second time and verifies that the output stays the same. Yes, it would slow down classloading, that's why it would be debug only. But it would also allow to easily catch such mistakes. Currently I'm not aware of anything other than mixin applying class transformers twice, and since mixin is generally used in project that change a lot of things, its easy to see it as just an incompatibility with the coremod that uses mixin.
This isn't a bad idea to be honest, the contract of transformers is that calls should be idempotent but as you say a few transformers don't respect this obligation. However it wouldn't be sufficient to check on only the first invocation since the transformer may no-op (and should) if the class in question doesn't have any patches (eg. isn't in the map in the first place). This means it would have to check for every call to every transformer.
This wouldn't be too hard to implement as a standalone mod transformer as well, this might provide a better end-user experience than a debug option since it could simply be activated by placing the jar in the mods folder. This would also allow the check to be done on a particular mod evironment before the mixin-containing mod is added.
Yes, I realize it would involve checking it on every call to all transformers. Which is why it would be debug option, not enabled by default. It would certainly have noticeable performance implications.
As for a standalone mod for that... I think the user experience would be worse here. It would force the user to download a separate mod from somewhere, and add it into mods. With it being a builtin debug feature, it would just involve adding a JVM argument. And while technically a standalone checker to run before adding mixin-containing mod sounds good in theory, 99% of users don't know or care about what mixin is, and only get interested when something goes wrong. In which case, they already have mixin in there.