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

Fixes MixinIntermediaryDevRemapper.mapMethodName unable to map ambiguous values #751

Merged
merged 1 commit into from
Dec 20, 2022

Conversation

ishland
Copy link
Contributor

@ishland ishland commented Dec 19, 2022

This PR fixes this mixin from lithium unable to be applied cleanly in 1.19.3 devlaunch using 1.19.3+build.2 yarn mappings.

Copy link
Member

@modmuss50 modmuss50 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@modmuss50 modmuss50 added the bug label Dec 19, 2022
Copy link
Contributor

@sfPlayer1 sfPlayer1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

brings it in line with the field remapping code, not 100% sure but plausible

@modmuss50
Copy link
Member

modmuss50 commented Dec 20, 2022

I was unable to reproduce this 🤔 I think the change is still OK, but I wanted to double check.

I downloaded the latest version of lithium for 1.19.3 and was able to to run in dev without issue?. The refmap contains the full desc for the method you pointed out

"stopAllTasks": "Lnet/minecraft/class_4095;method_18900(Lnet/minecraft/class_3218;Lnet/minecraft/class_1309;)V",

I am unsure how you were able to get into this state? A way to reproduce this and/or a crash log would be useful.

@ishland
Copy link
Contributor Author

ishland commented Dec 20, 2022

I can reliably reproduce this using this commit of fabric-example-mod (the current version at this point).

Steps to reproduce:

  • Download the current lithium release into run/mods
  • ./gradlew runClient
  • Go into a singleplayer and run /summon villager
  • Crashes

Note: it may involve another mixin crashing, this patch for lithium fixes that.

Some analysis copied from caffeine discord lithium dev channel:

im pretty sure this is a loader bug and/or a yarn bug
this is how mixin handles devlaunch environment

  1. mixin parses the target and remaps it into intermediary using the refmap provided (success)
    (Lnet/minecraft/entity/ai/brain/task/Task;stop(Lnet/minecraft/server/world/ServerWorld;Lnet/minecraft/entity/LivingEntity;J)V -> Lnet/minecraft/class_7893;method_18925(Lnet/minecraft/class_3218;Lnet/minecraft/class_1309;J)V)
  2. mixin tries to remap the intermediary name into the current devlaunch mapped name (failed)

the issue is remapping method names:

public String mapMethodName(String owner, String name, String desc) {

according to the debugger this method is for remapping intermediary name to the current devlaunch mapped name

  • it attempts to look up in the lookup table but for some reasons the mapping for Lnet/minecraft/class_7893;method_18925(Lnet/minecraft/class_3218;Lnet/minecraft/class_1309;J)V is marked as <ambiguous> (possible mapping bug)
  • then it falls back to "querying with additional owner and/or desc info"
  • but for some reasons it is trying to use intermediary name to look for classes in a yarn environment (line 141), which will definitely find nothing
  • then the remapper thinks the mapping just doesnt exist and gives up checking mappings (line 144)

class names and method descriptors are then remapped successfully
after combining all of them it results in Lnet/minecraft/entity/ai/brain/task/Task;method_18925(Lnet/minecraft/server/world/ServerWorld;Lnet/minecraft/entity/LivingEntity;J)V which is just not valid at all

then mixin just tries to look for methods with this wrong target descriptor and fails

@modmuss50
Copy link
Member

Thanks for getting back quickly I was able to reproduce it 👍

This fix works and intented, many thanks.

@modmuss50 modmuss50 merged commit 0a45612 into FabricMC:master Dec 20, 2022
Juuxel added a commit to architectury/architectury-loom-runtime that referenced this pull request Feb 7, 2023
…ous values

Adapted from Fabric Loader commit 0a456127f8cb69a24d5ddf6a0699dd17785c0ffd,
PR FabricMC/fabric-loader#751.

Co-authored-by: ishland <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants