Warn on mixin plugin classloads from the game layer#410
Draft
lukebemish wants to merge 3 commits into
Draft
Conversation
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Long term, it would be a good idea to limit mixin plugins to be in
FMLModType=LIBRARYjars and not be pulled up from the game layer during transformation.I've mentioned this before but the fact that this works this way is the last major footgun in the transformation system and leads to a lot of wacky behaviour. Most notably -- it's generally intended that part of the contract of a
ClassProcessoris that its actions will only affect processors after it. This is not the case for mixin -- aClassProcessor's actions, even if that processor is after mixin, can affect what mixin does if it processes a class loaded (indirectly or directly) from a mixin plugin.IMO it's a reasonable solution to limit mixin plugins (and anything mixin loads, for that matter) to LIBRARY -- after all, everything else out there that could be involved in transforming classes, that someone might write, would be loaded above the game layer (Mostly just
ClassProcessors now).Last this was brought up there were worried about it causing extra complexity for people writing their own mixin plugins. My take is that this is (and ought to be!) a minority of modders -- even a minority of modders with mixins -- and is rarely needed for the things people say it is:
mixinsblock of yourneoforge.mods.toml, no need for a mixin config plugin at all (and I've opened a PR to make sure that is documented).Even a case that does require moving the plugin to a LIBRARY jar shouldn't be that difficult -- in fact, doing so is already documented for MDG.
This PR (a draft for now as I work out some details) would be the first step in this process, making mixin plugins loaded from the TCL during load log errors but allowing load to continue.
For additional justification as to why this change should be made: currently, there is no entrypoint during which a classprocessor running after mixin may, before it needs to decide whether it is processing any class, look at the mixin-transformed bytecode (which it has available to it, as it is running after mixin) of a particular vanilla class. Were you to do so in
handlesClass(at the last possible moment that is), the first times that is queried would be for mixin plugin classes and you would be unable to see the mixin-transformed bytecode for anything as it is recursively hidden inside mixin transformation. This is bad; classprocessors should be independent from one another. They're well-ordered for a reason.