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

Analyze jdk classes through reflection instead of asm #158

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sfPlayer1
Copy link

This already seems to be enough to make MC 1.21.1 run with Java 23, Fabric API and an ASM version incompatible with the active JDK.

It doesn't look like Mixin needs any of the non-populated data, I probably already fill in more than necessary. It is assumed that mixins can't target jdk classes, but Mixin doesn't seem to give good early errors for this, I think it'd end up silently ignoring those targets as they wouldn't go through Knot in the first place. Improving that is out of scope for this change.

@LlamaLad7
Copy link
Collaborator

Could this at least be a fallback, used only if the current JDK is newer than ASM supports?

@modmuss50
Copy link
Member

Could this at least be a fallback, used only if the current JDK is newer than ASM supports?

My only worry with that is it that it would only be exercised in the rare situation when a newer Java version is used, thus its unlikely it will be regularly tested. What would be the argument against using this all the time, are you worried about peformance?

@LlamaLad7
Copy link
Collaborator

Mainly just worried about the unpopulated data. It may well not be used by Mixin itself but other mods are perfectly free to use ClassInfo and regularly do so. If the data is available via reflection I'd prefer it to be populated.

@sfPlayer1
Copy link
Author

It should perform better than analyzing the class, I think if mixin accesses any of the missing data there's a bug elsewhere. I noticed that Mixin is not too eager to initialize everything everywhere itself, imo it's fair to change the expectations a little unless we find actual breakage in the field.

@Oregano1963
Copy link

Any updates on this situation?

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

Successfully merging this pull request may close these issues.

4 participants