Add merged form of @Inject and @ModifyVariable

[Chocohead]

Player had mentioned this a few weeks ago, right now local capture with @Inject is a liability whenever the target code changes. @ModifyVariable has a more ideal local targeting system, but only works one at a time, and doesn't allow the target method to be cancelled if desired. This PR adds an injector which merges the two systems together into a more ergonomic one.

Take this rather artificial scenario:

class A {
	public static String method(int a, long b, Object c) {
		long d = a + b;
		String e = c.toString();
		String f = e; //Save for later
		e += " with sum " + d;
		return e + " and " + f;
	}
}

If I wanted to inspect the values of e and f just before the return concatenation, I could do so with an @Inject:

@Mixin(A.class)
abstract class AMixin {
	@Inject(method = "method", at = @At(value = "CONSTANT", args = "stringValue= and "), locals = LocalCapture.CAPTURE_FAILSOFT)
	private static void onMethod(int a, long b, Object c, CallbackInfoReturnable<String> call, long d, String e, String f) {
		//Now I have e and f
	}
}

This is fine if I want to inspect them, but if I want to change them things start getting difficult

@Mixin(A.class)
abstract class AMixin {
	@Unique
	private String capturedE;

	@ModifyVariable(method = "method", at = @At(value = "CONSTANT", args = "stringValue= and "), ordinal = 0)
	private static String onMethodE(String e) {
		return capturedE = e;
	}

	@ModifyVariable(method = "method", at = @At(value = "CONSTANT", args = "stringValue= and "), ordinal = 1)
	private static String onMethodF(String f) {
		//Now I have e (in capturedE) and f, and can change f
		return f;
	}
}

Now that works if I know the code isn't threaded, otherwise my capturedE might not be the right value. It also only lets me change f, if I want to change e too...

@Mixin(A.class)
abstract class AMixin {
	@Unique
	private String capturedE;

	@ModifyVariable(method = "method", at = @At(value = "CONSTANT", args = "stringValue= and "), ordinal = 0)
	private static String onMethodE(String e) {
		return capturedE = e;
	}

	@ModifyVariable(method = "method", at = @At(value = "CONSTANT", args = "stringValue= and "), ordinal = 1)
	private static String onMethodF(String f) {
		//Time to calculate something which needs e and f
		capturedE = ...;
		return ...;
	}

	@ModifyVariable(method = "method", at = @At(value = "CONSTANT", args = "stringValue= and "), ordinal = 0)
	private static String onMethodE(String e) {
		return capturedE;
	}
}

All getting a bit hairy now. Say I might want to change the return value of the method altogether, otherwise change the value of e and f.

@Mixin(A.class)
abstract class AMixin {
	@Unique
	private String computedE, computedF;

	@Inject(method = "method", at = @At(value = "CONSTANT", args = "stringValue= and "), locals = LocalCapture.CAPTURE_FAILSOFT, cancellable = true)
	private static void onMethod(int a, long b, Object c, CallbackInfoReturnable<String> call, long d, String e, String f) {
		if (...) {//Some decision whether to change the return completely
			call.setReturnValue("something else");
		} else {//Don't need to, just calculate something which needs e and f
			computedE = ...;
			computedF = ...;
		}
	}

	@ModifyVariable(method = "method", at = @At(value = "CONSTANT", args = "stringValue= and "), ordinal = 1)
	private static String onMethodF(String f) {
		return computedF;
	}

	@ModifyVariable(method = "method", at = @At(value = "CONSTANT", args = "stringValue= and "), ordinal = 0)
	private static String onMethodE(String e) {
		return computedE;
	}
}

Starting to look like a wobbling tower of maintainability issues. If another local is added before e for example, the @Inject goes wrong and needs some help:

@Mixin(A.class)
abstract class AMixin {
	@Inject(method = "method", at = @At(value = "CONSTANT", args = "stringValue= and "), locals = LocalCapture.CAPTURE_FAILSOFT, cancellable = true)
	private static void onMethod(int a, long b, Object c, CallbackInfoReturnable<String> call, long d, String e, String f) {
		...
	}

	@Surrogate
	private static void onMethod(int a, long b, Object c, CallbackInfoReturnable<String> call, long d, long dsFriend, String e, String f) {
		onMethod(a, b, call, d, e, f);
	}

	...
}

Of course this is all a forced example, but you see people who need to do one part of these situations every so often.

---

The top most problem is local capture being brittle. Needing to capture all the locals before the one(s) you want is no fun when that means a dozen or more extra parameters to your handler. Nor is the accompanying local capture related crash when trying to update your mixin to a newer Minecraft version. What we need is something more like @ModifyVariable:

@Mixin(A.class)
abstract class AMixin {
	@InjectWithLocals(method = "method", at = @At(value = "CONSTANT", args = "stringValue= and "), behaviour = LocalCapture.CAPTURE_FAILSOFT,
			locals = {@Local(ordinal = 0), @Local(ordinal = 1)})
	private static void onMethod(int a, long b, Object c, CallbackInfoReturnable<String> call, String e, String f) {
		//Now I have e and f, without having to worry what is before them
	}

That's all well and good, but how do I know which @Local I need to do. For that printing will give you a helping hand:

@Mixin(A.class)
abstract class AMixin {
	@InjectWithLocals(method = "method", at = @At(value = "CONSTANT", args = "stringValue= and "), behaviour = LocalCapture.PRINT)
	private static void onMethod(int a, long b, Object c, CallbackInfoReturnable<String> call) {
	}
}

Running that gets us the following in the log:

/******************************************************************************************************************/
/*       Target Class : A                                                                                         */
/*      Target Method : public String method(int a, long b, Object c)                                             */
/*  Target Max LOCALS : 8                                                                                         */
/* Initial Frame Size : 4                                                                                         */
/*      Callback Name : onMethod                                                                                  */
/*        Instruction : InjectionNode LDC                                                                         */
/******************************************************************************************************************/
/*   INDEX  ORDINAL                            TYPE  NAME                                                         */
/*   PARAM   [ x ]                              int  a                                                            */
/*   PARAM   [ x ]                             long  b                                                            */
/*   PARAM                                    <top>                                                               */
/*   PARAM   [ x ]                           Object  c                                                            */
/* > [  0]   [  0]                             long  d                                                  <skipped> */
/*   [  1]                                    <top>                                                               */
/*   [  2]   [  0]                           String  e                                                  <skipped> */
/*   [  3]   [  1]                           String  f                                                  <skipped> */
/******************************************************************************************************************/

Now I know e is the first String (ordinal 0), and f is the second. I could also use the LVT index, then they'd be 2 and 3. I could even do them by name (although for Minecraft you wouldn't want to).

@Mixin(A.class)
abstract class AMixin {
	@InjectWithLocals(method = "method", at = @At(value = "CONSTANT", args = "stringValue= and "), behaviour = LocalCapture.CAPTURE_FAILSOFT,
			locals = {@Local(ordinal = 0), @Local(index = 2), @Local(name = "e")})
	private static void onMethod(int a, long b, Object c, CallbackInfoReturnable<String> call, String e, String eToo, String eAsWell) {
	}
}

What if I wanted d? Well that would be ordinal 0 for long, or index 0, or name d. As there is only one long local however, like @ModifyVariable, we can use an implicit declaration:

@Mixin(A.class)
abstract class AMixin {
	@InjectWithLocals(method = "method", at = @At(value = "CONSTANT", args = "stringValue= and "), behaviour = LocalCapture.CAPTURE_FAILSOFT,
			locals = {@Local})
	private static void onMethod(int a, long b, Object c, CallbackInfoReturnable<String> call, long d) {
	}

	@InjectWithLocals(method = "method", at = @At(value = "CONSTANT", args = "stringValue= and "), behaviour = LocalCapture.CAPTURE_FAILSOFT)
	private static void onMethodAlternative(int a, long b, Object c, CallbackInfoReturnable<String> call, long d) {
		//Don't even need to specify a @Local, it will be inferred
	}

	@InjectWithLocals(method = "method", at = @At(value = "CONSTANT", args = "stringValue= and "), behaviour = LocalCapture.CAPTURE_FAILSOFT,
			locals = {@Local, @Local(ordinal = 0)})
	private static void onMethod(int a, long b, Object c, CallbackInfoReturnable<String> call, long d, String e) {
		//Now I have d and e
	}

	@InjectWithLocals(method = "method", at = @At(value = "CONSTANT", args = "stringValue= and "), behaviour = LocalCapture.CAPTURE_FAILSOFT,
			locals = {@Local(ordinal = 0)})
	private static void onMethod(int a, long b, Object c, CallbackInfoReturnable<String> call, String e, long d) {
		//Now I have d and e, inferring d would be found with an implicit @Local
	}

This is mainly to save typing out situations where the only local/s of it's/their type is/are being used. If an implicit search is used where there are multiple types, an error will be thrown (based on the behaviour):

@Mixin(A.class)
abstract class AMixin {
	@InjectWithLocals(method = "method", at = @At(value = "CONSTANT", args = "stringValue= and "), behaviour = LocalCapture.CAPTURE_FAILSOFT,
			locals = {@Local})
	private static void onMethod(int a, long b, Object c, CallbackInfoReturnable<String> call, String e) {
	}

Running that gets us the following in the log:

[WARN] [FabricLoader/Mixin]: Injection warning: Failed to capture all locals:
	[ 0] String - Expected one local with type but found 2

The errors try to be useful where they can, each local is being found according to rules so it's easier to say what went wrong compared to @Inject's local capture which just knows the handler signature isn't what it wants.

---

Back to our scenario, how could we write that now?

@Mixin(A.class)
abstract class AMixin {
	@InjectWithLocals(method = "method", at = @At(value = "CONSTANT", args = "stringValue= and "), behaviour = LocalCapture.CAPTURE_FAILSOFT,
			locals = {@Local(ordinal = 0), @Local(ordinal = 1)})
	private static void onMethod(int a, long b, Object c, CallbackInfoReturnable<String> call, @Modify String e, @Modify String f) {
		if (...) {//Some decision whether to change the return completely
			call.setReturnValue("something else");
		} else {//Don't need to, just calculate something which needs e and f
			e = ...;
			f = ...;
		}
	}
}

One handler is best, no threading problems now. But the direct changes to e and f, how are they propagated back? That's where the final trick comes in, @InjectWithLocals.Modify. That acts as a marker for any local which the changes should be applied to the target method. Mixin magic takes hold to make this happen, generating a special CallbackInfo(Returnable) which takes the locals in just before any return in the handler, then applying them to the target (if the call isn't cancelled of course). This process means minimal overhead as there is no boxing/putting into an array, and if the Callback is used anyway there will be no new objects created at all compared to a normal injector.

To demonstrate if that description is a little too abstract:

class A {
	public static String method(int a, long b, Object c) {
		long d = a + b;
		String e = c.toString();
		String f = e;
		e += " with sum " + d;
		StringBuilder var10000 = new StringBuilder(e);
		CallbackInfoWithLocals$2 callbackInfo = new CallbackInfoWithLocals$2("method", false);
		handler$zzz000$onMethod(a, b, c, callbackInfo, e, f);
		//You can see here how we inject too late for the changes to e to apply to the returned value ;)
		e = callbackInfo.getLocal$0();
		f = callbackInfo.getLocal$1();
		return var10000.append(" and ").append(f).toString();
	}

	@MixinMerged(...)
	private static void handler$zzz000$onMethod(int a, long b, Object c, CallbackInfoReturnable<String> call, @Modify String e, @Modify String f) {
		e = ...;
		f = ...;
		((CallbackInfoWithLocals$2)call).setLocals(e, f);
	}
}

// $FF: synthetic class
public class CallbackInfoWithLocals$2 extends CallbackInfoReturnable {
	private String local$0, local$1;

	public CallbackInfoWithLocals$2(String var1, boolean var2) {
		super(var1, var2);
	}

	public void setLocals(String var1, String var2) {
		this.local$0 = var1;
		this.local$1 = var2;
	}

	public String getLocal$0() {
		return this.local$0;
	}

	public String getLocal$1() {
		return this.local$1;
	}
}

As many (or as few) captured locals as desired can be annotated with @Modify, this means changes to locals which you don't want to carry over don't have to.

---

This PR isn't quite done; the docs have to be finished on @InjectWithLocals, the AP remapping logic needs doing (i.e. copy what @Inject does), and there's a bug or two to fix with when the handler is wrong, but I thought it best check this is what you imagined.

[Earthcomputer]

How is this likely to interact with Mumfrey's planned local variable capturing changes in 0.9?

[Chocohead]

As an implementation it's really only tied to how CallbackInjector injects it callbacks.

• Finding the locals at a given @At target goes through the normal Locals route, with Fabric already having combability handling more changes shouldn't be an issue.
• The local filtering mirrors how @ModifyVariable works conceptually, but is implemented independently to LocalVariableDiscriminator, because it designed to handle all the locals at once.
• Loading the locals for the callback is just adding some VarInsnNodes and is by no means obscure in CallbackLocalInjector#invokeCallback.
• @Modify has very little attachment to how the locals are originally caught at all.

I don't have a crystal ball for how 0.9 will pan out, but using a new annotation rather than adding to @Inject will cover most of what could go wrong with any update. In an ideal world all injectors could capture locals similar to this, then both @ModifyWithLocals and @ModifyVariable would be (redundant) alternatives to @Inject. If not, this still has a part to play alongside existing options.