Mixin icon indicating copy to clipboard operation
Mixin copied to clipboard

MixinPreProcessorStandard incorrectly checks STATIC flag on @Unique fields, a crash happens

Open saharNooby opened this issue 3 years ago • 1 comments

Steps to reproduce:

  1. Create a Mixin like this (1.18.2, Mojang names, Fabric):
import net.minecraft.server.packs.repository.Pack;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.Unique;

@Mixin(Pack.class)
public final class TestMixin {

	// Matches name of instance field "id" in the target class
	@Unique
	private static String id;

}
  1. Launch the client

Actual behavior: crash at start

Caused by: org.spongepowered.asm.mixin.transformer.throwables.InvalidMixinException: STATIC modifier of @Shadow field id in ***.TestMixin does not match the target
	at org.spongepowered.asm.mixin.transformer.MixinPreProcessorStandard.attachFields(MixinPreProcessorStandard.java:632)
	at org.spongepowered.asm.mixin.transformer.MixinPreProcessorStandard.attach(MixinPreProcessorStandard.java:302)
	at org.spongepowered.asm.mixin.transformer.MixinPreProcessorStandard.createContextFor(MixinPreProcessorStandard.java:277)
	at org.spongepowered.asm.mixin.transformer.MixinInfo.createContextFor(MixinInfo.java:1289)
	at org.spongepowered.asm.mixin.transformer.MixinApplicatorStandard.apply(MixinApplicatorStandard.java:292)
	at org.spongepowered.asm.mixin.transformer.TargetClassContext.apply(TargetClassContext.java:421)
	at org.spongepowered.asm.mixin.transformer.TargetClassContext.applyMixins(TargetClassContext.java:403)
	at org.spongepowered.asm.mixin.transformer.MixinProcessor.applyMixins(MixinProcessor.java:363)
	... 16 more

Expected behavior: the static field id gets renamed to something unuque and launch proceeds

This is a rare edge case (it occured when I was obfuscating a mod with some custom techiques), but I beleive that this is an incorrect behavior -- what's the point in checking static modifier of a field, if it gets renamed to something unuqie anyway and will not conflict with anything?

I propose that compareFlags should be called only for @Shadow fields, like this:

            if (isShadow && !Bytecode.compareFlags(mixinField, target, Opcodes.ACC_STATIC)) {
                throw new InvalidMixinException(this.mixin, String.format("STATIC modifier of @Shadow field %s in %s does not match the target",
                        mixinField.name, this.mixin));
            }

Or, at least the message could be more clear -- the field is not @Shadow after all. Something like "STATIC modifier of @Unique fields should match modifier of the shadowed field in the target class". After writing it down it sounds really strange :)

Context: line 636 of MixinPreProcessorStandard

saharNooby avatar Apr 28 '22 07:04 saharNooby

Yeah the entire point of @Unique is "I am adding this and don't intend to accidentall clobber an existing member, so needing it to match doesn't make sense, it's just a bug plain and simple.

Mumfrey avatar Apr 28 '22 12:04 Mumfrey