Mixin: Illegal ctor redirect raises NPE instead of a useful error

Wait, what?!?!!?!

The initial example is trying to redirect a SUPER CONSTRUCTOR CALL?

I can’t even.

…

Still doesn’t explain the actual bug but seriously this is some dumb crap.

Just making a point, as my second line says.

Can’t see what the point is…

Field initialization guarantees

As I stated in my previous comment, Mixin could check that an overwrite/injection into a constructor initializes all final fields once and only once.

field initialization ordering

As I stated in my previous comment, this is not a problem since the constructor always initializes the fields in the class. But some parts of Mixin such as local variable capture require you to know the bytecode of the target method, so determining an injection point for the constructor is much easier compared to this.

super and primary constructor calls

The mixin class should extend the target’s super class. The overwritten constructor will necessarily include a super call to a correct constructor.

etc. There are concrete examples (and cases I’m not aware of) that I’m sure will be provided, but as this isn’t a project I work on, don’t have any at hand.

That’s my point. You assume that injecting/overwriting constructors would cause problems without actually being aware of any problems.

And how would that affect injecting into a constructor?

It doesn’t, simply making a point.

How does that have anything to do with this?

Just making a point, as my second line says.

You say that, and yet you haven’t been able to give a single example of a problem that might be caused by injecting into a constructor.

Field initialization guarantees, field initialization ordering, super and primary constructor calls, etc. There are concrete examples (and cases I’m not aware of) that I’m sure will be provided, but as this isn’t a project I work on, don’t have any at hand.

And here is a link that shows you how to reflectively assign a new value to a private static final variable.

It’s almost as if “able to do something” and “should do something” and “will always work” are 3 entirely separate categories of things.

@DemonWav

This sentence screams such ignorance it’s frightening.

No, it is just based on the method’s name being <init>:

    if (Constants.CTOR.equals(target.method.name)) { // Constants.CTOR is "<init>"
        for (InjectionPoint injectionPoint : injectionPoints) {
            if (!injectionPoint.getClass().equals(BeforeReturn.class)) {
                throw new InvalidInjectionException(this.info, "Found injection point type " + injectionPoint.getClass().getSimpleName()
                        + " targetting a ctor in " + this + ". Only RETURN allowed for a ctor target");
            }
        }
    }

Removing that check makes it work, I tested it.

but just because the method I want to inject into is named <init> rather than init, Mixin refuses to do so.

This sentence screams such ignorance it’s frightening.

But they are. Once compiled to bytecode, a constructor is just a method named <init> that happens to be called after every object creation.

Oh my god. Even just the fact that ctors are called by invokespecial rather than invokevirtual, that alone should tell you that maybe they are at least a little different than a normal public method? ctors have specific semantics and rules that normal methods just don’t have, treating them like any other method will cause problems.

redirecting constructors is reckless and 100% unnecessary

Yes, it is unnecessary in this case. I was just redirecting the constructor as a workaround for not being able to overwrite/inject in it.

I don’t even see anything in your example that you couldn’t do without your proposed solution of redirecting the ctor anyway. If you just want to modify the setting of properties then do so by redirecting the field setter.

I am trying to completely remove this from the constructor, and only create instances of StateImplementation as they are needed, to optimize memory usage:

this.properties = ImmutableSortedMap.copyOf(map);
Map<Map<IProperty<?>, Comparable<?>>, BlockStateContainer.StateImplementation> map2 = Maps.newLinkedHashMap();
List<BlockStateContainer.StateImplementation> list1 = Lists.newArrayList();

for (List<Comparable<?>> list : Cartesian.cartesianProduct(this.getAllowedValues())) {
    Map<IProperty<?>, Comparable<?>> map1 = MapPopulator.createMap(this.properties.values(), list);
    BlockStateContainer.StateImplementation blockstatecontainer$stateimplementation = createState(blockIn, ImmutableMap.copyOf(map1), unlistedProperties);
    map2.put(map1, blockstatecontainer$stateimplementation);
    list1.add(blockstatecontainer$stateimplementation);
}

for (BlockStateContainer.StateImplementation blockstatecontainer$stateimplementation1 : list1) {
    blockstatecontainer$stateimplementation1.buildPropertyValueTable(map2);
}

this.validStates = ImmutableList.<IBlockState>copyOf(list1);

If you can’t do what you need to with the tools in Mixin then you probably shouldn’t be doing it.

The only reason I can’t do what I need to do is that Mojang decided to put the initialization code for the BlockStateContainer in the constructor. If they had simply put it in a separate method named init which was called by the constructor, I would be able to overwrite it, but just because the method I want to inject into is named <init> rather than init, Mixin refuses to do so.

ctor calls should never be treated like a normal method call

But they are. Once compiled to bytecode, a constructor is just a method named <init> that happens to be called after every object creation. The following code:

public class TestClass extends SuperClass {
    private int x;
    private int y;
    private String s1 = "String 1";
    private String s2;

    {
        s2 = "String 2";
    }

    public TestClass(int x, int y) {
        this.x = x;
        this.y = y;
    }

    public void init(int x, int y) {
        super.init();
        s1 = "String 1";
        s2 = "String 2";
        this.x = x;
        this.y = y;
    }
}

compiles to the following bytecode:

  public void <init>(int x, int y);
    Flags: PUBLIC
    Code:
      stack=2, locals=3, arguments=2
           0: aload_0         /* this */
           1: invokespecial   SuperClass.<init>:()V
           4: aload_0         /* this */
           5: ldc             "String 1"
           7: putfield        TestClass.s1:Ljava/lang/String;
          10: aload_0         /* this */
          11: ldc             "String 2"
          13: putfield        TestClass.s2:Ljava/lang/String;
          16: aload_0         /* this */
          17: iload_1         /* x */
          18: putfield        TestClass.x:I
          21: aload_0         /* this */
          22: iload_2         /* y */
          23: putfield        TestClass.y:I
          26: return         
    LocalVariableTable:
      Start  Length  Slot  Name  Signature
      -----  ------  ----  ----  -----------
      0      27      0     this  LTestClass;
      0      27      1     x     I
      0      27      2     y     I
  
  public void init(int x, int y);
    Flags: PUBLIC
    Code:
      stack=2, locals=3, arguments=2
           0: aload_0         /* this */
           1: invokespecial   SuperClass.init:()V
           4: aload_0         /* this */
           5: ldc             "String 1"
           7: putfield        TestClass.s1:Ljava/lang/String;
          10: aload_0         /* this */
          11: ldc             "String 2"
          13: putfield        TestClass.s2:Ljava/lang/String;
          16: aload_0         /* this */
          17: iload_1         /* x */
          18: putfield        TestClass.x:I
          21: aload_0         /* this */
          22: iload_2         /* y */
          23: putfield        TestClass.y:I
          26: return         
    LocalVariableTable:
      Start  Length  Slot  Name  Signature
      -----  ------  ----  ----  -----------
      0      27      0     this  LTestClass;
      0      27      1     x     I
      0      27      2     y     I

The methods are exactly the same, apart from one of them being named <init> rather than init.

Here are the possible “problems” when injecting into/overwriting a constructor:

1. It’s difficult to know what instruction the injection point will target exactly because the compiled constructor includes field and initializers at the top. -> Yes, it’s a bit more difficult than a regular method, but the order of the field initializer is preserved, making it much easier than something like local variable capture, which mixin supports.
2. An overwrite might leave a final field uninitialized -> Mixin could check this by making sure all @Final fields are assigned to in the overwriting method.
3. Injecting before the super constructor call -> Either require the mixin method to be static, or disallow doing that.

Ok so my superctor lookup is failing because this is actually an overloaded ctor. Makes sense, that can be fixed.

The inability to modify constructors is a major flaw in Mixin, in my opinion.

You’re entitled to your opinion, but I directly reject your assertion. The inability to modify constructors is a deliberate feature of Mixin.

Consider the things that you can do with Mixin to interact with object construction:

• Use modifyargs and modifyarg to modify ctor args inline
• Redirect any field or method call in a ctor, just like other methods
• Modify constants (“redirect” them if you will) in a ctor
• Modify any initialiser and override initialisers with your own
• Append callbacks to the end of ctors for custom construction logic.

Now consider the things I don’t let you do:

• Inject at arbitrary points in the ctor - ctors are delicate enough as it is, this is pure sanity stuff
• Redirect the a ctor call - this is just utterly stupid because ctor calls should never be treated like a normal method call

It’s forcing me to use ASM directly to patch some of the constructors I’m not able to edit using Mixin.

Doing stupid shit with ASM is your prerogative, but I’m not going to endorse or support it. If you can’t do what you need to with the tools in Mixin then you probably shouldn’t be doing it. I don’t even see anything in your example that you couldn’t do without your proposed solution of redirecting the ctor anyway. If you just want to modify the setting of properties then do so by redirecting the field setter.

I accept that the bug here is that I didn’t take into account delegating (overloaded) ctors, and will fix it accordingly. However redirecting constructors is reckless and 100% unnecessary, I wouldn’t recommend doing it in ASM and it’s certainly never going to happen in Mixin.