SpongePowered
Support 'hybrid' redirectors that take a CallbackInfo
When using Mixin, it's often necessary to prevent a method from executing based on the return value of a (modified) method call. For example, a mixin might want to redirect a call toWorld#spawnEntity or World#setBlockState, and cause the method to return early if some checks fail.
Currently, this requires writing two separate injections - a @Redirect targeting the method, and an @Inject immediately after the method call. A new field must also be introduced to store the result of the checks performed in the @Redirect.
It would be very useful if Mixin allowed an @Redirect method to take a CallbackInfo/CallbackInfoReturnable, just as an @Inject method. This CallbackInfo would function just as it does for @Injects. This would allow Mixin users to modify a method call and cancel the containing method, all from a single @Redirect.
I'm considering that the way this would have to work is where you'd point an injected CallbackInfo/CallbackInfoReturnable and then point the possible return point as well, so something along the lines of:
@Redirect(method = "addTileEntity",
at = @At(value = "INVOKE", target = "Ljava/util/List;add(Ljava/lang/Object;)Z", remap = false),
slice = @Slice(from = @At(value = "FIELD", target = "Lnet/minecraft/world/World;tickableTileEntities:Ljava/util/List;"),
to = @At(value = "FIELD", target = "Lnet/minecraft/world/World;isRemote:Z")),
cancellable = @Cancellable(
from = @At(value = "INVOKE", target = "Lnet/minecraft/tileentity/TileEntity:getPos()Lnet/minecraft/util/math/BlockPos;"),
earlyReturn = @At(value = "INVOKE", target = "Ljava/util/List;add(Ljava/lang/Object;)Z", remap = false)
)
)
private boolean onAddTileEntity(List<net.minecraft.tileentity.TileEntity> list, Object tile, CallbackInfoReturnable<Boolean> ci) {
if (!this.isFake() && !canTileUpdate((net.minecraft.tileentity.TileEntity) tile)) {
return false;
}
ci.setReturnValue(true);
return list.add((net.minecraft.tileentity.TileEntity) tile);
}
Where the target looks like this:
public boolean addTileEntity(TileEntity tile)
{
boolean flag = this.loadedTileEntityList.add(tile);
BlockPos blockpos1 = tile.getPos();
if (flag && tile instanceof ITickable)
{
this.tickableTileEntities.add(tile);
}
if (this.isRemote)
{
IBlockState iblockstate1 = this.getBlockState(blockpos1);
this.notifyBlockUpdate(blockpos1, iblockstate1, iblockstate1, 2);
}
return flag;
}
And the mixed in result being:
public boolean addTileEntity(TileEntity tile)
{
boolean flag = this.loadedTileEntityList.add(tile);
BlockPos blockpos1 = tile.getPos();
CallbackInfoReturnable cb = new CallbackInfoReturnable("onAddTileEntity", true, false);
if (flag && tile instanceof ITickable)
{
onAddTileEntity$redirect$Z001(this.tickableTileEntities, tile, cb);
if (cb.isCancelled()) {
return cb.getReturnValueZ();
}
}
if (this.isRemote)
{
IBlockState iblockstate1 = this.getBlockState(blockpos1);
this.notifyBlockUpdate(blockpos1, iblockstate1, iblockstate1, 2);
}
return flag;
}
And this actually makes it even easier to exemplify using the callback info to potentially even retrieve the local variable of that target method with some local captures.
Edit: Actually, with this you could even go as far as telling the return value to "absorb" a local variable for local capture if you wanted to simply cancel and "short return" the captured local variable, which you'd know the type of at that point (since the local variable could be dictated by the @Cancellable annotation. And the generic typing of the method declaration for the enhanced redirector.
And yes, I know that this has great potential for being destructive as it changes some fundamental structures of the method that previously would potentially not be considered.
Sounds like a neat idea. I'm currently heading out to Germany for a week for Gamescom and all my time on Mixin has been focused on the Java 9/ModLauncher update anyway. I'll have a look at this more when I get back (or if I have some spare time in Cologne).
I don't see any issues with this from a paradigm perspective, but I've always resisted overloading the functionality of @Redirect and similar too much because they are quite well-defined and have a simple, understandable contract as they are. However I see the desirability of the functionality iself so some kind of cancellable redirect is definately do-able, it's just very unlikely that I'll do this by extending @Redirect itself. It will either be a separate annotation on a redirect method or a separate annotation altogether. But again, I need more time to think about it and I'm heading out the door right now.
