Dynamic fix for ModifyExpressionValue mixins

Sinytra · Jul 12, 2024 · b09c98e · b09c98e
1 parent adcc207
commit b09c98e
Show file tree

Hide file tree

Showing 7 changed files with 108 additions and 14 deletions.
diff --git a/definition/src/main/java/org/sinytra/adapter/patch/analysis/MethodCallAnalyzer.java b/definition/src/main/java/org/sinytra/adapter/patch/analysis/MethodCallAnalyzer.java
@@ -54,6 +54,12 @@ public static InstructionMatcher findForwardInstructions(AbstractInsnNode insn,
         return new InstructionMatcher(insn, List.of(), nextInsns);
     }
 
+    public static InstructionMatcher findForwardInstructionsDirect(AbstractInsnNode insn, int range) {
+        List<AbstractInsnNode> nextInsns = getInsns(insn, range, FORWARD);
+
+        return new InstructionMatcher(insn, List.of(), nextInsns);
+    }
+
     private static List<AbstractInsnNode> getInsns(AbstractInsnNode root, int range, UnaryOperator<AbstractInsnNode> operator) {
         return Stream.iterate(root, Objects::nonNull, operator)
             .filter(insn -> !(insn instanceof FrameNode) && !(insn instanceof LineNumberNode))

diff --git a/...main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixArbitraryInjectionPoint.java b/...main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixArbitraryInjectionPoint.java
@@ -2,10 +2,7 @@
 
 import org.jetbrains.annotations.Nullable;
 import org.objectweb.asm.Type;
-import org.objectweb.asm.tree.AbstractInsnNode;
-import org.objectweb.asm.tree.ClassNode;
-import org.objectweb.asm.tree.MethodInsnNode;
-import org.objectweb.asm.tree.MethodNode;
+import org.objectweb.asm.tree.*;
 import org.sinytra.adapter.patch.analysis.InsnComparator;
 import org.sinytra.adapter.patch.analysis.InstructionMatcher;
 import org.sinytra.adapter.patch.analysis.MethodCallAnalyzer;
@@ -15,16 +12,20 @@
 import org.sinytra.adapter.patch.transformer.ModifyInjectionPoint;
 import org.sinytra.adapter.patch.util.AdapterUtil;
 
+import java.util.ArrayList;
 import java.util.List;
+import java.util.Set;
 
 public class DynFixArbitraryInjectionPoint implements DynamicFixer<DynFixArbitraryInjectionPoint.Data> {
+    private static final Set<String> ACCEPTED_ANNOTATIONS = Set.of(MixinConstants.INJECT, MixinConstants.MODIFY_EXPR_VAL);
+
     public record Data(MethodContext.TargetPair dirtyTarget, AbstractInsnNode cleanInjectionInsn) {
     }
 
     @Nullable
     @Override
     public Data prepare(MethodContext methodContext) {
-        if (methodContext.methodAnnotation().matchesDesc(MixinConstants.INJECT)) {
+        if (methodContext.methodAnnotation().matchesAny(ACCEPTED_ANNOTATIONS)) {
             MethodContext.TargetPair cleanInjectionTarget = methodContext.findCleanInjectionTarget();
             List<AbstractInsnNode> cleanInsns = methodContext.findInjectionTargetInsns(cleanInjectionTarget);
             if (cleanInsns.size() == 1 && methodContext.failsDirtyInjectionCheck()) {
@@ -47,24 +48,41 @@ public Patch.Result apply(ClassNode classNode, MethodNode methodNode, MethodCont
         }
         int firstOpcode = cleanMatcher.after().getFirst().getOpcode();
 
+        List<AbstractInsnNode> candidates = new ArrayList<>();
+
         for (int i = 0; i < dirtyTargetMethod.instructions.size(); i++) {
             AbstractInsnNode insn = dirtyTargetMethod.instructions.get(i);
-            if (insn.getOpcode() != firstOpcode) {
+            if (insn instanceof FrameNode || insn instanceof LineNumberNode || insn.getOpcode() != firstOpcode) {
                 continue;
             }
 
-            InstructionMatcher dirtyMatcher = MethodCallAnalyzer.findForwardInstructions(insn, 5);
+            InstructionMatcher dirtyMatcher = MethodCallAnalyzer.findForwardInstructionsDirect(insn, 5);
             if (cleanMatcher.test(dirtyMatcher, InsnComparator.IGNORE_VAR_INDEX)) {
                 // Find first method call past matched instruction
                 AbstractInsnNode lastInsn = dirtyMatcher.after().getLast();
                 if (lastInsn != null) {
-                    MethodInsnNode nextMethodCall = (MethodInsnNode) AdapterUtil.iterateInsns(lastInsn, AbstractInsnNode::getNext, v -> v instanceof MethodInsnNode);
-                    String newInjectionPoint = Type.getObjectType(nextMethodCall.owner).getDescriptor() + nextMethodCall.name + nextMethodCall.desc;
-                    return new ModifyInjectionPoint("INVOKE", newInjectionPoint, true, false).apply(classNode, methodNode, methodContext);
+                    candidates.add(lastInsn);
                 }
             }
         }
 
+        if (candidates.size() == 1) {
+            AbstractInsnNode lastInsn = candidates.getFirst();
+            MethodInsnNode nextMethodCall = findReplacementInjectionPoint(lastInsn, methodContext);
+            String newInjectionPoint = Type.getObjectType(nextMethodCall.owner).getDescriptor() + nextMethodCall.name + nextMethodCall.desc;
+            return new ModifyInjectionPoint("INVOKE", newInjectionPoint, true, false).apply(classNode, methodNode, methodContext);
+        }
+
         return Patch.Result.PASS;
     }
+
+    private static MethodInsnNode findReplacementInjectionPoint(AbstractInsnNode lastInsn, MethodContext methodContext) {
+        // Require matching return types for ModifyExpressionValue mixins
+        if (methodContext.methodAnnotation().matchesDesc(MixinConstants.MODIFY_EXPR_VAL)) {
+            Type desiredReturnType = Type.getReturnType(methodContext.getInjectionPointMethodQualifier().desc());
+            return (MethodInsnNode) AdapterUtil.iterateInsns(lastInsn, AbstractInsnNode::getNext, v -> v instanceof MethodInsnNode minsn && Type.getReturnType(minsn.desc).equals(desiredReturnType));
+        } else {
+            return (MethodInsnNode) AdapterUtil.iterateInsns(lastInsn, AbstractInsnNode::getNext, v -> v instanceof MethodInsnNode);
+        }
+    }
 }
diff --git a/...c/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixAtVariableAssignStore.java b/...c/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixAtVariableAssignStore.java
@@ -14,8 +14,6 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Set;
-import java.util.function.Predicate;
-import java.util.function.UnaryOperator;
 
 /**
  * Find our new injection point with relation to variable assignments

diff --git a/test/src/test/java/org/sinytra/adapter/patch/test/mixin/DynamicMixinPatchTest.java b/test/src/test/java/org/sinytra/adapter/patch/test/mixin/DynamicMixinPatchTest.java
@@ -58,7 +58,26 @@ void testUpdatedInjectionTargetSamePoint() throws Exception {
     void testUpdatedInjectionPointFieldToMethod() throws Exception {
         assertSameCode(
             "org/sinytra/adapter/test/mixin/HoeItemMixin",
-            "injectUseOn"
+            "injectUseOn",
+            assertInjectionPoint()
+        );
+    }
+
+    @Test
+    void testUpdatedInjectionPoint2() throws Exception {
+        assertSameCode(
+            "org/sinytra/adapter/test/mixin/MilkBucketItemMixin",
+            "onClearStatusEffect",
+            assertInjectionPoint()
+        );
+    }
+
+    @Test
+    void testUpdatedInjectionPointModifyExprVal() throws Exception {
+        assertSameCode(
+            "org/sinytra/adapter/test/mixin/FarmLandBlockMixin",
+            "isFarmlandNearWater",
+            assertInjectionPoint()
         );
     }
 

diff --git a/test/src/testClasses/java/org/sinytra/adapter/test/mixin/FarmLandBlockMixin.java b/test/src/testClasses/java/org/sinytra/adapter/test/mixin/FarmLandBlockMixin.java
@@ -0,0 +1,29 @@
+package org.sinytra.adapter.test.mixin;
+
+import com.llamalad7.mixinextras.injector.ModifyExpressionValue;
+import com.llamalad7.mixinextras.sugar.Local;
+import net.minecraft.core.BlockPos;
+import net.minecraft.world.level.LevelReader;
+import net.minecraft.world.level.block.FarmBlock;
+import org.spongepowered.asm.mixin.Mixin;
+import org.spongepowered.asm.mixin.injection.At;
+
+@Mixin(FarmBlock.class)
+public class FarmLandBlockMixin {
+    // https://github.com/warior456/Sculk-Depths/blob/06870f1ab93b8d500bbbea285eebbfa7db6a5f89/src/main/java/net/ugi/sculk_depths/mixin/FarmLandBlockMixin.java#L23
+    @ModifyExpressionValue(
+        method = "isNearWater",
+        at = @At(value = "INVOKE", target = "Lnet/minecraft/world/level/material/FluidState;is(Lnet/minecraft/tags/TagKey;)Z")
+    )
+    private static boolean isFarmlandNearWater(boolean original, @Local(ordinal = 0) LevelReader world, @Local(ordinal = 1) BlockPos blockPos) {
+        return original;
+    }
+
+    @ModifyExpressionValue(
+        method = "isNearWater",
+        at = @At(value = "INVOKE", target = "Lnet/minecraft/world/level/block/state/BlockState;canBeHydrated(Lnet/minecraft/world/level/BlockGetter;Lnet/minecraft/core/BlockPos;Lnet/minecraft/world/level/material/FluidState;Lnet/minecraft/core/BlockPos;)Z")
+    )
+    private static boolean isFarmlandNearWaterExpected(boolean original, @Local(ordinal = 0) LevelReader world, @Local(ordinal = 1) BlockPos blockPos) {
+        return original;
+    }
+}
diff --git a/test/src/testClasses/java/org/sinytra/adapter/test/mixin/HoeItemMixin.java b/test/src/testClasses/java/org/sinytra/adapter/test/mixin/HoeItemMixin.java
@@ -26,7 +26,7 @@ public void injectUseOn(UseOnContext itemUsageContext, CallbackInfoReturnable<In
     }
 
     @Inject(
-        at = @At(value = "INVOKE", target = "Lnet/minecraft/world/level/block/state/BlockState;getToolModifiedState(Lnet/minecraft/world/item/context/UseOnContext;Lnet/minecraftforge/common/ToolAction;Z)Lnet/minecraft/world/level/block/state/BlockState;"),
+        at = @At(value = "INVOKE", target = "Lnet/minecraft/world/level/Level;getBlockState(Lnet/minecraft/core/BlockPos;)Lnet/minecraft/world/level/block/state/BlockState;"),
         method = "useOn",
         cancellable = true,
         locals = LocalCapture.CAPTURE_FAILSOFT

diff --git a/test/src/testClasses/java/org/sinytra/adapter/test/mixin/MilkBucketItemMixin.java b/test/src/testClasses/java/org/sinytra/adapter/test/mixin/MilkBucketItemMixin.java
@@ -0,0 +1,24 @@
+package org.sinytra.adapter.test.mixin;
+
+import net.minecraft.world.entity.LivingEntity;
+import net.minecraft.world.item.ItemStack;
+import net.minecraft.world.item.MilkBucketItem;
+import net.minecraft.world.level.Level;
+import org.spongepowered.asm.mixin.Mixin;
+import org.spongepowered.asm.mixin.injection.At;
+import org.spongepowered.asm.mixin.injection.Inject;
+import org.spongepowered.asm.mixin.injection.callback.CallbackInfoReturnable;
+
+@Mixin(MilkBucketItem.class)
+public class MilkBucketItemMixin {
+    // https://github.com/Patbox/brewery/blob/ffdaf1f5298e7ef4f82447546d2afb68286b6acd/src/main/java/eu/pb4/brewery/mixin/MilkBucketItemMixin.java#L30
+    @Inject(method = "finishUsingItem", at = @At(value = "INVOKE", target = "Lnet/minecraft/world/entity/LivingEntity;removeAllEffects()Z", shift = At.Shift.BEFORE))
+    private void onClearStatusEffect(ItemStack stack, Level level, LivingEntity user, CallbackInfoReturnable<ItemStack> cir) {
+        // Noop
+    }
+
+    @Inject(method = "finishUsingItem", at = @At(value = "INVOKE", target = "Lnet/minecraft/world/entity/LivingEntity;removeEffectsCuredBy(Lnet/neoforged/neoforge/common/EffectCure;)Z"))
+    private void onClearStatusEffectExpected(ItemStack stack, Level level, LivingEntity user, CallbackInfoReturnable<ItemStack> cir) {
+        // Noop
+    }
+}