SpongePowered / Mixin

Mixin is a trait/mixin and bytecode weaving framework for Java using ASM
MIT License
1.41k stars 194 forks source link

Replace naive class version check with heuristic #496

Closed zml2008 closed 3 years ago

zml2008 commented 3 years ago

With Minecraft moving to Java 16, the natural choice for target compatibility is compiling Java 16 Mixin classes, which is not yet supported.

A potential workaround is to compile Mixin classes with Java 11 compatibility while the rest of the mod is compiled for Java 16, which is possible with Gradle source sets (as done for now in Sponge). However, the IDE support for this is limited -- Eclipse, for one, compiles all source sets at once, with a single target version (in this case Java 16). This causes in-IDE run configurations fail, since Mixin classes are all compiled with Java 16 rather than 11.

(this is also ignoring that Eclipse doesn't support Java 16 until 2021-06, which has not reached a final release yet -- that will be solved by the Eclipse team soon enough though)

The workaround of separate target versions for Mixins vs the mod itself will work in the short term, but it makes development awkward, and will only become more of an issue when a Java 16-based Minecraft becomes our primary development target.

Mumfrey commented 3 years ago

I don't understand what this issue is requesting?

compatibilityLevel in the mixin config is a declaration of the minimum level required by a particular mixin config to be able to successfully apply its mixins. So if you don't need any post-java6 features you can leave it alone, otherwise you'd increment it to the minimum level required by your mixins.

There are two types of support in mixin, "I have features which work on version x", for which compatibilityLevel as declared is required to elevate the level, the other is provided by ASM but I don't control that. Mixin tracks the version of ASM in the primary target (ModLauncher) but there's nothing stopping you using a newer version.

What are you actually asking for? There's no features in mixin that are gated behind anything higher than JAVA_8 at present.

Mumfrey commented 3 years ago

Ok I'm hijacking this issue now I understand (from our discord discussions) what the issue is.

So fundamentally the issue seems to be that compatibilityLevel is now required to be higher than it ideally should be. The values available in the enum are not meant to be an exhaustive list of java versions (and the ASM dependency is abstracted for this reason) they are effectively meant to be a declaration of features required in mixins.

A check currently in the code violates this however since compiling with a newer target class version actually leads to mixin being unable to load the class, even if no unsupported features are present.

Therefore I think the best way to deal with this is two fold:

To clarify the purpose of compatibilityLevel, it might also be desirable to actually allow an enumerable list of required features instead, because that was the original intention of the setting anyway, which seems to have been lost.

Changing this to enhancement since this is behaviour that needs to change, but isn't fundamentally a bug or new feature.