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

Constructor redirect incorrectly handles nested constructor invocations #659

Open
Barteks2x opened this issue Feb 2, 2024 · 0 comments
Open

Comments

@Barteks2x
Copy link

This to the best of my knowledge doesn't occur anywhere in Minecraft.

Consider the following target:

public class Test {
    public Test(String v, Test t) {
    }

    public Test(Test test) {
    }

    public static void test() {
        Test t = new Test(new Test(null, null));
    }
}

Then, the bytecode of the test() method looks like this (package names removed in all of the following examples)

   L0
    LINENUMBER 11 L0
    NEW Test
    DUP
    NEW Test
    DUP
    ACONST_NULL
    ACONST_NULL
    INVOKESPECIAL Test.<init> (Ljava/lang/String;LTest;)V
    INVOKESPECIAL Test.<init> (LTest;)V
    ASTORE 0
   L1
    LINENUMBER 12 L1
    RETURN

In this case, Mixin code that attempts to match NEW with a corresponding INVOKESPECIAL fails, and mixin provides seemingly "impossible" error messages.

This redirect:

    @Redirect(method = "test", at = @At(value = "NEW", target = "(Ljava/lang/String;LTest;)LTest;"))
    private static Test modifiedTest(String s, Test test) {
        System.out.println("Constructing " + test);
        return null;
    }

Produces the following error:

Caused by: org.spongepowered.asm.mixin.injection.throwables.InvalidInjectionException: @Redirect factory method Test::modifiedTest from mixinconfig.mixins.json:common.TestMixin2 has an invalid signature. Found unexpected argument type java.lang.String at index 0, expected Test. Handler signature: (Ljava/lang/String;LTest;)LTest; Expected signature: (LTest;)LTest; [INJECT Applicator Phase -> mixinconfig.mixins.json:common.TestMixin2 -> Apply Injections ->  -> Inject -> cubicchunks.mixins.core.json:common.TestMixin2->@FactoryRedirectWrapper::modifiedTest(Ljava/lang/String;LTest;)LTest;]

So it seems like it's trying to use the one-argument constructor. But when I target the one argument constructor like this:

    @Redirect(method = "test", at = @At(value = "NEW", target = "(LTest;)LTest;"))
    private static Test modifiedTest(Test test) {
        System.out.println("Constructing " + test);
        return null;
    }

Results in this error:

Caused by: org.spongepowered.asm.mixin.injection.throwables.InvalidInjectionException: @Redirect factory method Test::modifiedTest from mixinconfig.mixins.json:common.TestMixin2 has an invalid signature. Found unexpected argument type Test at index 0, expected java.lang.String. Handler signature: (LTest;)LTest; Expected signature: (Ljava/lang/String;LTest;)LTest; [INJECT Applicator Phase -> mixinconfig.mixins.json:common.TestMixin2 -> Apply Injections ->  -> Inject -> mixinconfig.mixins.json:common.TestMixin2->@FactoryRedirectWrapper::modifiedTest(LTest;)LTest;]

So in this case, it's expecting the 2-argument constructor.

This is caused by the use of this method:

public MethodInsnNode findInitNodeFor(TypeInsnNode newNode) {
int start = this.indexOf(newNode);
for (Iterator<AbstractInsnNode> iter = this.insns.iterator(start); iter.hasNext();) {
AbstractInsnNode insn = iter.next();
if (insn instanceof MethodInsnNode && insn.getOpcode() == Opcodes.INVOKESPECIAL) {
MethodInsnNode methodNode = (MethodInsnNode)insn;
if (Constants.CTOR.equals(methodNode.name) && methodNode.owner.equals(newNode.desc)) {
return methodNode;
}
}
}
return null;
}

Which assumes that between a NEW and its corresponding INVOKESPECIAL, there cannot be another NEW and INVOKESPECIAL for the same class.

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

No branches or pull requests

1 participant