A @ModifyReturnValue feature

[Barteks2x]

Explanation

A @ModifyReturnValue would target method calls and take the return value of the method as an argument, and return the modified return value. Additional parameters as with other injectors could also be allowed.

In the case of only one mixin modifying a given method, it's already possible to do the same thing with @Redirect that just calls the original method, and then applies it's changes to the return value. And this is a common use case for redirects that I can see. The main issue with this approach is that multiple mixins can't modify return value of the same method. Sometimes similar cases can be solved with very specific use of ModifyArg and ModifyVariable but they are generally more fragile, less direct about the intent and not always possible.

@ModifyReturnValue could allow multiple mixins to target the same method call, as the original method call remains there, and the order in which return value changes are applied would depend on mixin application order (mixin priority).

Example

Consider the following code

// a lot of code here
if (someMethod(a, b, c)) {
  // a lot of code here
}
// more code here

If we consider 2 mods, one of which wants to change the code to effectively do this:

if (someMethod(a, b, c) || myCustomFeatureEnabled)

And another that wants to change it to

if (someMethod(a, b, c) && thingIsAllowed)

both mods using ModifyReturnValue with appropriate priorities would result in transformed code like this

if (verifyThingAllowed(checkCustomFeatureEnabled(someMethod(a, b, c)))

This result is, to my knowledge, currently impossible without close cooperation of both mods.

---

As explained on discord, II also thought about extending it all the way to "modify top of the stack" as it seemed like quite a natural way to extend it, with what is targeted being specified just by the injection point, but it did seem like something way too powerful, difficult to verify correctness of, and prone to misuse. But I can also see this being extended for field access, and maybe other specific cases.

This could still open the possibility of modders using it incorrectly, and just completely replacing the return value, as if it was just a redirect, simply because of being told that it's better to use this instead of a redirect, and blindly applying it.

[yyny]

I don't see much wrong with modifying the stack directly, perhaps even multiple stack elements at once, this is useful for adding data to builders:

public static Foo createFoo();
 Code:
  invokestatic ...  // Foo.createBuilder
  getstatic ..      // TargetClass.VALUE_X
  invokestatic ..   // FooBuilder.valueX
  getstatic ..      // TargetClass.VALUE_Y
  invokestatic ..   // FooBuilder.valueY
  getstatic ..      // TargetClass.VALUE_Z
  invokestatic ..   // FooBuilder.valueZ
  <modify the FooBuilder here>
  invokestatic ..   // FooBuilder.build
  areturn

[Aizistral]

Forgive my ignorance if I am missing out on something, but isn't this achievable with @Inject(at = @At("RETURN"), cancelable = true) and CallbackInfoReturnable?

[yyny]

Forgive my ignorance if I am missing out on something, but isn't this achievable with @Inject(at = @At("RETURN"), cancelable = true) and CallbackInfoReturnable?

It appears so, CallbackInfoReturnable.getReturnValue contains the old return value, although this does create bloated CallbackInfoReturnable infrastructure that we never use. It also does not quite handle my use case shown above.

Thinking about it a bit, both of these use cases just seems like more general use of ModifyArg/ModifyArgs, applied to injection points other than invokes (not currently possible). I'm still in favor of a new @Decorator (ModifyStack?) just for documentation purposes.

[yyny]

Another thought: I'm not quite certain how (or even if) we can type check if we allow multiple stack elements, since the JVM doesn't seem to expose this, but for most opcodes finding the type of the item at the top of the stack right before and/or after an opcode is trivial (example: if we inject before dreturn, then the top of the stack should always be a double, if we inject after newarray, then the top of the stack should always be an array of some known type).

We could simply disallow ambiguous injection points. And if we know the item of the top of the stack is T, we can invokestatic a function static T changeStack(T item);, and then the type at the top of the stack will remain unchanged, all while injecting just a single opcode. I think that handles the majority of use cases in a very simple way.

For more natural injection using a member method T changeStack(T item);, we could also:

<any opcode>  # oldItem
aload_0       # oldItem this
swap          # this oldItem
invokevirtual # newItem

For long/double, we would still have have to store/load into a temporary local variable, since there is no swap for those.

Or, if the opcode does not pop any elements (example: LOAD_CONSTANT/LOAD/INVOKE with 0 arguments):

aload_0       # this
<opcode*>     # this oldItem
invokevirtual # newItem

Either way, this is only a couple of opcodes, and no local variable usage, much better than @ModifyArg and definitely better than CallbackInfoReturnable.

Also, it seems theoretically feasible to use static callgraph analysis to resolve ambiguous stack types so we can resolve ambiguous stack elements and even allow modification of multiple arbitrary stack elements at once just like @ModifyArgs, but the best use case for that I can think of is to modify arguments, and we already have an injector for that.

EDIT: Also, there is StackMapTable, but that is usually incomplete.

Usually, when you want to change the stack, it is right before a pop or right after a push. I see no reason to restrict the usage of that to only apply to return values.

Another example I ran across where something like @ModifyStackTop is necessary, in a 400 opcode function that can't be easily redirected:

// I want to transform this:
double a = b + x * CONSTANT;
// into this
double a = b + modify(x) * CONSTANT;
// but currently, I have to do this:
double a = b + x * CONSTANT;
a = modify(a);
// and then find+subtract b and divide by CONSTANT again, hoping I don't lose precision.

[yyny]

Update: I decided to take a spin at implementing this feature like the theoretical implementation outlined above. It didn't turn out to be very difficult, as expected.

I also had to implement three new injection points for the feature to be useful:

• BeforeOpcode (OPCODE)
• AfterOpcode (OPCODE_RESULT)
• AfterInvokeResult (INVOKE_RESULT)

I would like to know if there is interest to add this to the library so I can create a pull request, otherwise I will just turn it into a library.

Some refactoring of injection points might also be in order, since most injection points just look for instructions matching a predicate, which is a lot of duplicated code.

Example usage:

@Mixin(TitleScreen.class)
public class TitleScreenMixin {

    @Inject(method="init()V", at=@At("HEAD"))
    private void onInject(CallbackInfo ci) {
        System.out.println("Normal injection");
    }

    // This is the equivalent of the CallbackInfoReturnable method mentioned above, but much cleaner
    @ModifyStackTop(method="areRealmsNotificationsEnabled()Z", at=@At(value="TAIL"), require=1, allow=1)
    private static boolean changeReturnValue(boolean b) {
        return false;
    }

    // This seems to be the actual feature requested in the issue
    // Changes `if (this.client.isDemo()) { ... }` to
    // `if (changeIsDemoResult(this.client.isDemo())) { ... }`
    @ModifyStackTop(method="init()V",
                    at=@At(value="INVOKE_RESULT", target="Lnet/minecraft/client/MinecraftClient;isDemo()Z", ordinal=0),
                    require=1, allow=1)
    private static boolean changeIsDemoResult(boolean isDemo) {
        System.out.println("Is this a demo version? " + isDemo);
        return true; // We are now!
    }

    // Change the result of a non-function call.
    @ModifyStackTop(method="initWidgetsDemo(II)V",
                    at=@At(value="OPCODE_RESULT", opcode=Opcodes.IDIV, ordinal=0),
                    require=1, allow=1)
    private static int onDivision(int halfWidth) {
        return halfWidth / 2;
    }

    // or, equivalently:
    /*
    @ModifyStackTop(method="initWidgetsDemo(II)V",
                    at=@At(value="OPCODE", opcode=Opcodes.IDIV, ordinal=0),
                    require=1, allow=1)
    private static int onDivision(int divisor) {
        return divisor * 2;
    }
    */
}

[Aizistral]

// This is the equivalent of the CallbackInfoReturnable method mentioned above, but much cleaner
@ModifyStackTop(method="areRealmsNotificationsEnabled()Z", at=@At(value="TAIL"), require=1, allow=1)
private static boolean changeReturnValue(boolean b) {
    return false;
}

But how would this behave if there are multiple return points within the same method?

[yyny]

@Aizistral

The callback will be automatically injected into every single injection point, just like any other injector, so if you specify @At("RETURN"), your callback will get called for every single return point.

[yyny]

The ModifyStackTop doesn't decide where to inject the callback, if that's what you're concerned about.

[Aizistral]

To be honest, I don't see how this is better than CallbackInfoReturnable for the particular purpose of modifying returned value, knowing CallbackInfoReturnable was designed specifically to handle this. Your ModifyStackTop thing apparently has uses other than this, but since this issue was initially written as request for @ModifyReturnValue feature, I wish you could explain more clearly what exactly isn't good with way of achieving such functionality we already have at our disposal.

[yyny]

The CallbackInfoReturnable method only works for my use case, it doesn't actually do what's requested in the issue (which is not possible at all right now). Being able to avoid 20/30 opcodes including memory allocations caused by CallbackInfoReturnable is just a nice side effect I liked to show off.

The showcase for the actual feature request is the changeIsDemoResult function, which changes the result of the this.client.isDemo() function inside of TitleScreen#init() before it is processed by the if statement.

[Aizistral]

Ok, makes sense now. Somehow I've just been missing out on the key point.

[LoganDark]

I just ran into this missing feature myself, and would also greatly appreciate the addition of an annotation that allows me to modify the return value of a function call without completely redirecting it.

[LlamaLad7]

You could consider using this in the meantime https://github.com/LlamaLad7/MixinExtras/wiki/ModifyExpressionValue

[LoganDark]

You could consider using this in the meantime https://github.com/LlamaLad7/MixinExtras/wiki/ModifyExpressionValue

That looks like exactly what I need. Thank you! :D

Does it support capturing locals?

[LlamaLad7]

Does it support capturing locals?

Not currently, as with all stock injectors except Inject itself. Depending on your specific situation there's often ways around it, and in many cases relying on local capture isn't a great idea anyway, though discussions of specific situations like that would be best placed in the appropriate channels in the forge, fabric, or sponge discord.