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

Option to attempt to detect coremods that apply their patches only on the first call #309

Open
Barteks2x opened this issue Mar 10, 2019 · 2 comments

Comments

@Barteks2x
Copy link

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.

@Mumfrey
Copy link
Member

Mumfrey commented Mar 21, 2019

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.

@Barteks2x
Copy link
Author

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.

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

No branches or pull requests

2 participants