From b002b1ea8f5e15d6f78cbaa45ca56835b33d4f88 Mon Sep 17 00:00:00 2001 From: Tong317 Date: Thu, 1 Oct 2026 18:23:32 +0800 Subject: [PATCH 01/16] Assorted mixin patch fixes --- .../sinytra/adapter/env/MockMixinRuntime.java | 10 ++-- .../sinytra/adapter/env/ctx/MethodHelper.java | 50 ++++++++++++++++++- .../adapter/env/ctx/PatchEnvironmentImpl.java | 17 +++++-- .../patch/mixin/ModifyVariableMixin.java | 3 +- .../ArbitraryInjectionPointSubResolver.java | 12 ++++- .../special/SliceBoundaryResolver.java | 2 +- .../param/ReplaceParametersTransformer.java | 4 +- .../types/BytecodeFixerJarGenerator.java | 13 ++++- 8 files changed, 97 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/org/sinytra/adapter/env/MockMixinRuntime.java b/core/src/main/java/org/sinytra/adapter/env/MockMixinRuntime.java index b39b443..0e0457e 100644 --- a/core/src/main/java/org/sinytra/adapter/env/MockMixinRuntime.java +++ b/core/src/main/java/org/sinytra/adapter/env/MockMixinRuntime.java @@ -72,8 +72,8 @@ public static IMixinContext forClass(String className, String targetClass, Patch return new ClassMixinContext(className, targetClass, environment); } - public static ISliceContext forSlice(IMixinContext context, MethodNode methodNode) { - return new MethodSliceContext(context, methodNode); + public static ISliceContext forSlice(IMixinContext context, MethodNode methodNode, AnnotationNode annotation) { + return new MethodSliceContext(context, methodNode, annotation); } public static InjectionInfo forInjectionInfo(String className, String targetClass, PatchEnvironment environment) { @@ -102,7 +102,7 @@ public static InjectionInfo createModifyVariableInjectionInfo(IMixinContext mixi return info; } - private record MethodSliceContext(IMixinContext context, MethodNode methodNode) implements ISliceContext { + private record MethodSliceContext(IMixinContext context, MethodNode methodNode, AnnotationNode annotation) implements ISliceContext { @Override public IMixinContext getMixin() { return this.context; @@ -117,7 +117,9 @@ public String remap(String reference) { @Override public String getElementDescription() {return "dummy";} @Override public MethodSlice getSlice(String id) {throw new UnsupportedOperationException();} @Override public MethodNode getMethod() {return this.methodNode;} - @Override public AnnotationNode getAnnotationNode() {throw new UnsupportedOperationException();} + // NOTE: the remaining stubs below deliberately still throw. A loud failure is preferable to a + // silent wrong answer: if Mixin reaches another of these entry points we want to see it. + @Override public AnnotationNode getAnnotationNode() {return this.annotation;} @Override public ISelectorContext getParent() {throw new UnsupportedOperationException();} @Override public IAnnotationHandle getAnnotation() {throw new UnsupportedOperationException();} @Override public IAnnotationHandle getSelectorAnnotation() {throw new UnsupportedOperationException();} diff --git a/core/src/main/java/org/sinytra/adapter/env/ctx/MethodHelper.java b/core/src/main/java/org/sinytra/adapter/env/ctx/MethodHelper.java index 4bb6ddc..06ac18f 100644 --- a/core/src/main/java/org/sinytra/adapter/env/ctx/MethodHelper.java +++ b/core/src/main/java/org/sinytra/adapter/env/ctx/MethodHelper.java @@ -216,7 +216,7 @@ private InsnList getSlicedInsns(AnnotationHandle parentAnnotation, ClassNode cla }) .map(sliceAnn -> { IMixinContext mixinContext = MockMixinRuntime.forClass(classNode.name, targetClass.name, context.environment()); - ISliceContext sliceContext = MockMixinRuntime.forSlice(mixinContext, injectorMethod); + ISliceContext sliceContext = MockMixinRuntime.forSlice(mixinContext, injectorMethod, sliceAnn); return computeSlicedInsns(sliceContext, sliceAnn, mixinTarget); }) .orElse(targetMethod.instructions); @@ -244,7 +244,53 @@ public List getTargetMethodLocals(TargetPair target, int startPos @Nullable public List getTargetMethodLocals(TargetPair target, int startPos, int lvtCompatLevel) { - List targetInsns = findInjectionTargetInsns(target); + return getTargetMethodLocals(target, findInjectionTargetInsns(target), startPos, lvtCompatLevel); + } + + /** + * Like {@link #findInjectionTargetInsns(TargetPair)}, but locates the injection point from the given + * {@code @At} data rather than from the annotation as originally written. + *

+ * Adaptation can rewrite {@code @At}, after which the raw annotation points at an injection site that + * no longer exists. Anything derived from that site - notably the locals captured by a + * {@code locals = LocalCapture...} handler - would then be computed from the wrong place, or not at + * all. + */ + public List findInjectionTargetInsns(@Nullable TargetPair target, @Nullable AtData atData) { + if (target == null) return List.of(); + if (atData == null) return findInjectionTargetInsns(target); + + return computeInjectionTargetInsns( + target, + this.context::injectionPointAnnotation, + (ctx, h) -> { + AnnotationNode atCopy = atData.toAnnotationNode(); + AnnotationHandle annCopy = this.context.methodAnnotation().copy(); + annCopy.setOrAppendNonNull(PROPERTY_AT, atCopy); + + return InjectionPoint.parse(ctx, this.context.methodNode(), annCopy.unwrap(), atCopy); + }, + true + ); + } + + /** + * Variant of {@link #getTargetMethodLocals(TargetPair)} that uses the effective {@code @At} data. + * See {@link #findInjectionTargetInsns(TargetPair, AtData)}. + */ + @Nullable + public List getTargetMethodLocals(TargetPair target, @Nullable AtData atData) { + Type[] targetParams = Type.getArgumentTypes(target.methodNode().desc); + boolean isStatic = isStatic(this.context.methodNode()); + int lvtOffset = isStatic ? 0 : 1; + // The starting LVT index is of the first var after all method parameters. Offset by 1 for instance methods to skip 'this' + int targetLocalPos = targetParams.length + lvtOffset; + return getTargetMethodLocals(target, findInjectionTargetInsns(target, atData), targetLocalPos, + this.context.environment().fabricLVTCompatibility()); + } + + @Nullable + private List getTargetMethodLocals(TargetPair target, List targetInsns, int startPos, int lvtCompatLevel) { if (targetInsns.isEmpty()) { LOGGER.debug("Skipping LVT patch, no target instructions found"); return null; diff --git a/core/src/main/java/org/sinytra/adapter/env/ctx/PatchEnvironmentImpl.java b/core/src/main/java/org/sinytra/adapter/env/ctx/PatchEnvironmentImpl.java index f959a0e..4d883fb 100644 --- a/core/src/main/java/org/sinytra/adapter/env/ctx/PatchEnvironmentImpl.java +++ b/core/src/main/java/org/sinytra/adapter/env/ctx/PatchEnvironmentImpl.java @@ -8,8 +8,8 @@ import org.sinytra.adapter.util.provider.MixinClassLookup; import java.util.Collection; -import java.util.HashMap; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; public final class PatchEnvironmentImpl implements PatchEnvironment { private final RefmapHolder refmapHolder; @@ -21,7 +21,10 @@ public final class PatchEnvironmentImpl implements PatchEnvironment { private final AuditTrail auditTrail; private final @Nullable Collection pkgNamespaces; - private final Map inheritanceHandlers = new HashMap<>(); + // The transformer processes classes in parallel (ForkJoinPool via AsyncHelper), so this cache is + // touched from multiple threads. A plain HashMap.computeIfAbsent throws ConcurrentModificationException + // under concurrent modification (and also when the mapping function re-enters this method). + private final Map inheritanceHandlers = new ConcurrentHashMap<>(); public PatchEnvironmentImpl( RefmapHolder refmapHolder, @@ -77,7 +80,15 @@ public MixinClassGenerator classGenerator() { @Override public InheritanceHandler inheritanceHandler(ClassLookup lookup) { - return this.inheritanceHandlers.computeIfAbsent(lookup, InheritanceHandler::new); + // get-then-putIfAbsent instead of computeIfAbsent: the handler constructor can re-enter this + // method, which ConcurrentHashMap.computeIfAbsent would reject as a recursive update. + InheritanceHandler existing = this.inheritanceHandlers.get(lookup); + if (existing != null) { + return existing; + } + InheritanceHandler created = new InheritanceHandler(lookup); + InheritanceHandler prev = this.inheritanceHandlers.putIfAbsent(lookup, created); + return prev != null ? prev : created; } @Override diff --git a/core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyVariableMixin.java b/core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyVariableMixin.java index ebf41de..9d4dd5d 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyVariableMixin.java +++ b/core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyVariableMixin.java @@ -63,7 +63,8 @@ public TxResult preProcess(MixinContext context, MutableConfiguration clean, Res @Override public TxResult postProcess(MixinContext context, Configuration clean, MutableConfiguration dirty, Recipe recipe) { - if (dirty.getTargetMethod() == null || dirty.getTargetMethod().desc() == null) { + if (dirty.getTargetMethod() == null || dirty.getTargetMethod().desc() == null + || clean.getTargetMethod() == null || clean.getTargetMethod().desc() == null) { return TxResult.FAIL; } diff --git a/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/ArbitraryInjectionPointSubResolver.java b/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/ArbitraryInjectionPointSubResolver.java index 762f95e..856e470 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/ArbitraryInjectionPointSubResolver.java +++ b/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/ArbitraryInjectionPointSubResolver.java @@ -93,11 +93,21 @@ private static AbstractInsnNode findCandidates(InstructionMatcher cleanMatcher, private static MethodInsnNode findReplacementInjectionPoint(AbstractInsnNode lastInsn, UnaryOperator flow, MixinContext context, @Nullable String injectionPointTarget) { // Require matching return types for ModifyExpressionValue mixins - if (context.hasFlag(MixinFlag.RETURN_TYPE_SENSITIVE) && injectionPointTarget != null) { + if (context.hasFlag(MixinFlag.RETURN_TYPE_SENSITIVE) && injectionPointTarget != null + && isCompleteMethodDescriptor(injectionPointTarget)) { Type desiredReturnType = Type.getReturnType(injectionPointTarget); return (MethodInsnNode) AdapterUtil.iterateInsns(lastInsn, flow, v -> v instanceof MethodInsnNode minsn && Type.getReturnType(minsn.desc).equals(desiredReturnType)); } return (MethodInsnNode) AdapterUtil.iterateInsns(lastInsn, flow, v -> v instanceof MethodInsnNode); } + + /** + * ASM's {@code Type.getReturnType} computes {@code desc.indexOf(')') + 1} and immediately reads that + * character, so a descriptor that ends right after ')' throws StringIndexOutOfBoundsException. + */ + private static boolean isCompleteMethodDescriptor(String desc) { + int paren = desc.indexOf(')'); + return paren >= 0 && paren + 1 < desc.length(); + } } diff --git a/core/src/main/java/org/sinytra/adapter/patch/resolver/special/SliceBoundaryResolver.java b/core/src/main/java/org/sinytra/adapter/patch/resolver/special/SliceBoundaryResolver.java index 6e2a372..48b7a58 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/resolver/special/SliceBoundaryResolver.java +++ b/core/src/main/java/org/sinytra/adapter/patch/resolver/special/SliceBoundaryResolver.java @@ -88,7 +88,7 @@ private static Optional fixSlideInjectionPoint(AtData boundary, MethodNo private static boolean passesSliceCheck(SliceData slice, MixinContext context, TargetPair dirtyTarget) { Target mixinTarget = MockMixinRuntime.createMixinTarget(dirtyTarget); IMixinContext mixinContext = MockMixinRuntime.forClass(context.classNode().name, dirtyTarget.classNode().name, context.patchContext().environment()); - ISliceContext sliceContext = MockMixinRuntime.forSlice(mixinContext, context.methodNode()); + ISliceContext sliceContext = MockMixinRuntime.forSlice(mixinContext, context.methodNode(), slice.toAnnotationNode()); MethodSlice methodSlice = MethodSlice.parse(sliceContext, slice.toAnnotationNode()); InsnList insns = methodSlice.getSlice(mixinTarget); diff --git a/core/src/main/java/org/sinytra/adapter/transform/param/ReplaceParametersTransformer.java b/core/src/main/java/org/sinytra/adapter/transform/param/ReplaceParametersTransformer.java index 50845bf..0294b04 100644 --- a/core/src/main/java/org/sinytra/adapter/transform/param/ReplaceParametersTransformer.java +++ b/core/src/main/java/org/sinytra/adapter/transform/param/ReplaceParametersTransformer.java @@ -81,9 +81,11 @@ public PatchResult apply(ClassNode classNode, MethodNode methodNode, MixinContex } if (insn instanceof MethodInsnNode minsn && minsn.owner.equals(originalType.getInternalName())) { + // getMethodCallInsns is @Nullable: it returns null when the call source cannot be + // determined statically. Skip the rewrite instead of killing the whole jar transform. List insns = MethodCallAnalyzer.getMethodCallInsns(methodNode, minsn); // Find var load instruction - for (AbstractInsnNode callInsn : insns) { + for (AbstractInsnNode callInsn : insns == null ? List.of() : insns) { if (callInsn instanceof VarInsnNode varInsn && varInsn.var == localVar.index) { minsn.owner = this.type.getInternalName(); } diff --git a/core/src/main/java/org/sinytra/adapter/types/BytecodeFixerJarGenerator.java b/core/src/main/java/org/sinytra/adapter/types/BytecodeFixerJarGenerator.java index 5fb85fb..850b134 100644 --- a/core/src/main/java/org/sinytra/adapter/types/BytecodeFixerJarGenerator.java +++ b/core/src/main/java/org/sinytra/adapter/types/BytecodeFixerJarGenerator.java @@ -61,7 +61,18 @@ public boolean save(Path path, Attributes additionalAttributes) { jarEntry.setTime(ZIP_TIME); jos.putNextEntry(jarEntry); - ClassWriter cw = new ClassWriter(ClassWriter.COMPUTE_MAXS | ClassWriter.COMPUTE_FRAMES); + ClassWriter cw = new ClassWriter(ClassWriter.COMPUTE_MAXS | ClassWriter.COMPUTE_FRAMES) { + @Override + protected String getCommonSuperClass(String type1, String type2) { + try { + return super.getCommonSuperClass(type1, type2); + } catch (Throwable t) { + // The default implementation resolves types through this class loader, which cannot + // see Minecraft classes. Degrade to Object so jar generation still succeeds. + return "java/lang/Object"; + } + } + }; entry.getValue().accept(cw); byte[] bytes = cw.toByteArray(); jos.write(bytes); From 4a8c222671aa196ca5ab4b7660f83f437289653f Mon Sep 17 00:00:00 2001 From: Tong317 Date: Thu, 1 Oct 2026 18:23:32 +0800 Subject: [PATCH 02/16] Record audit results at every early exit --- .../org/sinytra/adapter/patch/Patcher.java | 19 ++++++++- .../transform/PipelineMethodTransformer.java | 39 ++++++++++++++++--- 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/org/sinytra/adapter/patch/Patcher.java b/core/src/main/java/org/sinytra/adapter/patch/Patcher.java index 8a1a9b1..43963d7 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/Patcher.java +++ b/core/src/main/java/org/sinytra/adapter/patch/Patcher.java @@ -72,6 +72,21 @@ public PatchResult process(ClassNode classNode) { return result; } + /** + * See PipelineMethodTransformer#skip: every "give up quietly" exit must record the mixin, + * otherwise it disappears from the audit trail and the jar is reported as compatible. + */ + private static PatchResult skip(MixinContext context, org.sinytra.adapter.patch.config.Configuration config, String reason) { + context.environment().auditTrail().recordResult(context, config, org.sinytra.adapter.env.ctx.AuditTrail.Match.NONE); + try { + context.recordCtxAudit("Skipped mixin: %s", reason); + } catch (RuntimeException ignored) { + // Not every exit runs inside an audit frame (pushAudit/popAudit). recordResult above is + // what actually makes the skip visible; the reason is best-effort. + } + return PatchResult.PASS; + } + private PatchResult processMixin(ClassNode classNode, ClassTarget classTarget, PatchContext patchContext, MixinParser.MixinMethodHandle mixin) { MethodQualifier target = mixin.properties().getProperty(MixinKeys.TARGET_METHOD).orElse(null); if (target == null) return PatchResult.PASS; @@ -104,7 +119,7 @@ private PatchResult processMixin(ClassNode classNode, ClassTarget classTarget, P TxResult preResult = mixinType.preProcess(mixinContext, configuration, mixinContext.getResolvers(), mixinContext.getProcessors()); if (preResult == TxResult.FAIL) { LOGGER.debug(MIXINPATCH, "Skipping mixin {} due to failed preProcess", mixinId); - return PatchResult.PASS; + return skip(mixinContext, configuration, "failed preProcess"); } // << RUN LOADED PHASE @@ -118,7 +133,7 @@ private PatchResult processMixin(ClassNode classNode, ClassTarget classTarget, P // Validate clean config if (!configuration.validate()) { LOGGER.debug(MIXINPATCH, "Skipping mixin {} due to invalid CLEAN config", mixinId); - return PatchResult.PASS; + return skip(mixinContext, configuration, "invalid CLEAN config"); } // Temporarily set this to a high number for frame analysis to work diff --git a/core/src/main/java/org/sinytra/adapter/transform/PipelineMethodTransformer.java b/core/src/main/java/org/sinytra/adapter/transform/PipelineMethodTransformer.java index 769dbae..0835f55 100644 --- a/core/src/main/java/org/sinytra/adapter/transform/PipelineMethodTransformer.java +++ b/core/src/main/java/org/sinytra/adapter/transform/PipelineMethodTransformer.java @@ -47,11 +47,21 @@ public PipelineMethodTransformer(List methodPatches, boolean patche @Override public PatchResult apply(MixinContext context, Configuration config) { TargetPair cleanTarget = context.methods().findOwnMethodPair(context.cleanLookup(), config.getTargetMethod()); - if (cleanTarget == null) return PatchResult.PASS; + if (cleanTarget == null) return skip(context, config, "target method not found in the clean (vanilla) class"); TargetPair dirtyTarget = context.methods().findOwnMethodPair(context.dirtyLookup(), config.getTargetMethod()); - if (!this.patchResolver.matches(config) && !failsDirtyInjectionCheck(context, config, dirtyTarget) && hasValidSlice(context, config, dirtyTarget)) + if (!this.patchResolver.matches(config) && !failsDirtyInjectionCheck(context, config, dirtyTarget) && hasValidSlice(context, config, dirtyTarget)) { + // NOT a failure: failsDirtyInjectionCheck() returning false means the mixin can already inject + // into the patched target as-is, i.e. no adaptation is required. Record it as a success so it + // is visible as "considered and fine" rather than never-considered. + context.environment().auditTrail().recordResult(context, config, AuditTrail.Match.FULL); + try { + context.recordCtxAudit("No adaptation required: injection point already resolves in the patched target"); + } catch (RuntimeException ignored) { + // no audit frame at this exit + } return PatchResult.PASS; + } LOGGER.debug(MIXINPATCH, "Considering method {}", context.getMixinId()); @@ -67,6 +77,23 @@ public PatchResult apply(MixinContext context, Configuration config) { return PatchResult.PASS; } + /** + * The pipeline has several "give up quietly" exits. Each of them used to return PASS without + * recording anything, so the mixin vanished from the audit trail and the jar was reported as + * compatible. Record the skip so it shows up in the report. AuditTrailImpl downgrades it to + * IGNORED when the mixin is not required, so non-applicable mixins do not fail the mod. + */ + private static PatchResult skip(MixinContext context, Configuration config, String reason, Object... args) { + context.environment().auditTrail().recordResult(context, config, AuditTrail.Match.NONE); + try { + context.recordCtxAudit("Skipped mixin: " + reason, args); + } catch (RuntimeException ignored) { + // Not every exit runs inside an audit frame (pushAudit/popAudit). recordResult above is + // what actually makes the skip visible; the reason is best-effort. + } + return PatchResult.PASS; + } + private PatchResult execute(MixinContext context, Configuration config) { String mixinId = context.getMixinId(); Resolvers resolvers = this.patchesOnly ? new Resolvers(false) : context.getResolvers(); @@ -102,7 +129,7 @@ private PatchResult execute(MixinContext context, Configuration config) { } } else if (res.type() == Resolver.ResultType.FAIL) { LOGGER.debug(MIXINPATCH, "Skipping mixin {} due to failed RESOLVER {}", mixinId, resolver.getClass().getSimpleName()); - return PatchResult.PASS; + return skip(context, config, "failed resolver %s", resolver.getClass().getSimpleName()); } } @@ -114,7 +141,7 @@ private PatchResult execute(MixinContext context, Configuration config) { TxResult postResult = lateMixinType.postProcess(context, cleanConfig, dirtyConfig, recipe); if (postResult == TxResult.FAIL) { LOGGER.debug(MIXINPATCH, "Skipping mixin {} due to failed postProcess", mixinId); - return PatchResult.PASS; + return skip(context, config, "failed postProcess"); } } } @@ -122,7 +149,7 @@ private PatchResult execute(MixinContext context, Configuration config) { // 3.1. Validate dirty config if (!dirtyConfig.validate()) { LOGGER.debug(MIXINPATCH, "Skipping mixin {} due to invalid DIRTY config", mixinId); - return PatchResult.PASS; + return skip(context, config, "invalid DIRTY config"); } // 4. Run Processors @@ -136,7 +163,7 @@ private PatchResult execute(MixinContext context, Configuration config) { } if (res == TxResult.FAIL) { LOGGER.debug(MIXINPATCH, "Skipping mixin {} due to failed PROCESSOR {}", mixinId, processor.getClass().getSimpleName()); - return PatchResult.PASS; + return skip(context, config, "failed processor %s", processor.getClass().getSimpleName()); } } From a42bf097b5dbc96454bbeccb2d35b19c3de67d76 Mon Sep 17 00:00:00 2001 From: Tong317 Date: Thu, 1 Oct 2026 18:23:32 +0800 Subject: [PATCH 03/16] Add Accessor, Invoker, ModifyArgs, ModifyConstant and Overwrite support --- .../adapter/env/util/MixinAnnotations.java | 5 + .../sinytra/adapter/patch/MixinParser.java | 24 ++++ .../adapter/patch/config/key/MixinKeys.java | 20 +++- .../adapter/patch/mixin/AccessorMixin.java | 90 +++++++++++++++ .../adapter/patch/mixin/InvokerMixin.java | 98 ++++++++++++++++ .../adapter/patch/mixin/MixinType.java | 38 ++++++ .../adapter/patch/mixin/MixinTypes.java | 18 ++- .../adapter/patch/mixin/ModifyArgsMixin.java | 61 ++++++++++ .../patch/mixin/ModifyConstantMixin.java | 108 ++++++++++++++++++ .../adapter/patch/mixin/OverwriteMixin.java | 80 +++++++++++++ .../adapter/patch/mixin/RedirectMixin.java | 85 +++++++++++++- 11 files changed, 618 insertions(+), 9 deletions(-) create mode 100644 core/src/main/java/org/sinytra/adapter/patch/mixin/AccessorMixin.java create mode 100644 core/src/main/java/org/sinytra/adapter/patch/mixin/InvokerMixin.java create mode 100644 core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyArgsMixin.java create mode 100644 core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyConstantMixin.java create mode 100644 core/src/main/java/org/sinytra/adapter/patch/mixin/OverwriteMixin.java diff --git a/core/src/main/java/org/sinytra/adapter/env/util/MixinAnnotations.java b/core/src/main/java/org/sinytra/adapter/env/util/MixinAnnotations.java index 0f10121..88804d2 100644 --- a/core/src/main/java/org/sinytra/adapter/env/util/MixinAnnotations.java +++ b/core/src/main/java/org/sinytra/adapter/env/util/MixinAnnotations.java @@ -6,12 +6,17 @@ public class MixinAnnotations { public static final String REDIRECT = "Lorg/spongepowered/asm/mixin/injection/Redirect;"; public static final String MODIFY_ARG = "Lorg/spongepowered/asm/mixin/injection/ModifyArg;"; public static final String MODIFY_ARGS = "Lorg/spongepowered/asm/mixin/injection/ModifyArgs;"; + public static final String MODIFY_ARGS_INTERNAL_NAME = "org/spongepowered/asm/mixin/injection/ModifyArgs"; public static final String MODIFY_VAR = "Lorg/spongepowered/asm/mixin/injection/ModifyVariable;"; public static final String MODIFY_CONST = "Lorg/spongepowered/asm/mixin/injection/ModifyConstant;"; + public static final String MODIFY_CONST_INTERNAL_NAME = "org/spongepowered/asm/mixin/injection/ModifyConstant"; public static final String OVERWRITE = "Lorg/spongepowered/asm/mixin/Overwrite;"; + public static final String OVERWRITE_INTERNAL_NAME = "org/spongepowered/asm/mixin/Overwrite"; // Interface mixins public static final String ACCESSOR = "Lorg/spongepowered/asm/mixin/gen/Accessor;"; + public static final String ACCESSOR_INTERNAL_NAME = "org/spongepowered/asm/mixin/gen/Accessor"; public static final String INVOKER = "Lorg/spongepowered/asm/mixin/gen/Invoker;"; + public static final String INVOKER_INTERNAL_NAME = "org/spongepowered/asm/mixin/gen/Invoker"; // Mixinextras annotations public static final String MODIFY_EXPR_VAL = "Lcom/llamalad7/mixinextras/injector/ModifyExpressionValue;"; public static final String MODIFY_EXPR_VAL_INTERNAL_NAME = "com/llamalad7/mixinextras/injector/ModifyExpressionValue"; diff --git a/core/src/main/java/org/sinytra/adapter/patch/MixinParser.java b/core/src/main/java/org/sinytra/adapter/patch/MixinParser.java index b07dc0d..579b1cf 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/MixinParser.java +++ b/core/src/main/java/org/sinytra/adapter/patch/MixinParser.java @@ -65,6 +65,15 @@ public static MixinMethodHandle parseMixin(ClassTarget cls, MethodNode method, R properties.setProperty(ControlKeys.TARGET_CLASS, targetClass); properties.setProperty(ControlKeys.RETURN_TYPE, Type.getReturnType(method.desc)); properties.setProperty(SpecialKeys.STATIC, MethodHelper.isStatic(method)); + // @Overwrite / @Invoker name no "method": their target is derived from the mixin method + // itself. Set the qualifier so postProcessMixin below can complete the descriptor from the + // dirty target class, exactly like an annotation that omitted its descriptor. + if (!properties.hasProperty(MixinKeys.TARGET_METHOD)) { + MethodQualifier derived = mixinType.deriveTarget(properties, method); + if (derived != null) { + properties.setProperty(MixinKeys.TARGET_METHOD, derived); + } + } if (!postProcessMixin(properties, environment)) { continue; } @@ -102,6 +111,21 @@ private static boolean postProcessMixin(MutablePropertyContainer properties, Pat List targets = dirtyTarget.methods.stream() .filter(m -> target.matches(MethodQualifier.create(m))) .toList(); + + // Fall back to the superclass chain. An injector may name a method the mixin's target class only + // INHERITS - carve's @Redirect(method = "causeExtraKnockback") on @Mixin(Player.class), where the + // method is declared on LivingEntity, is the reference case. Mixin itself resolves the target + // through the hierarchy, so mirroring that here is what lets the descriptor be completed (and the + // mixin stop being reported as an unexplained failure). + for (ClassNode cls = dirtyTarget; targets.isEmpty() && cls.superName != null; ) { + ClassNode parent = environment.dirtyClassLookup().getClass(cls.superName).orElse(null); + if (parent == null) break; + targets = parent.methods.stream() + .filter(m -> target.matches(MethodQualifier.create(m))) + .toList(); + cls = parent; + } + if (targets.size() == 1) { properties.setProperty(MixinKeys.TARGET_METHOD, MethodQualifier.create(targets.getFirst())); } diff --git a/core/src/main/java/org/sinytra/adapter/patch/config/key/MixinKeys.java b/core/src/main/java/org/sinytra/adapter/patch/config/key/MixinKeys.java index f4c37de..5778fac 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/config/key/MixinKeys.java +++ b/core/src/main/java/org/sinytra/adapter/patch/config/key/MixinKeys.java @@ -43,7 +43,13 @@ public class MixinKeys { .build(); public static final PropertyKey TARGET_CONSTANT = PropertyKey.builder("constant") .parser((value, mapper) -> { + // Same null guard as TARGET_AT above. Without it, a @ModifyConstant whose "constant" is not a single + // annotation (a repeated/array form, or a plain int) reached new AnnotationHandle(null) and blew up + // with a NullPointerException inside MixinParser - which aborted the whole transform for that mod + // instead of simply skipping the property. Reference case: shuttfup's DefaultLANPort. AnnotationNode node = ControlKeys.parseSingle(value, AnnotationNode.class); + if (node == null) return null; + AnnotationHandle handle = new AnnotationHandle(node); return ConstantData.parse(handle).orElse(null); }) @@ -51,11 +57,22 @@ public class MixinKeys { .build(); // Mixin data public static final PropertyKey CANCELLABLE = PropertyKey.create("cancellable", Boolean.class); + /** {@code @Accessor}/{@code @Invoker} field or method name. */ + public static final PropertyKey ACCESSOR_VALUE = PropertyKey.create("value", String.class); public static final PropertyKey INDEX = PropertyKey.create("index", Integer.class); public static final PropertyKey ORDINAL = PropertyKey.create("ordinal", Integer.class); public static final PropertyKey ARGS_ONLY = PropertyKey.create("argsOnly", Boolean.class); public static final PropertyKey SLICE = PropertyKey.builder("slice") - .parser((value, mapper) -> SliceData.parse(new AnnotationHandle((AnnotationNode) value), mapper)) + // ASM stores a repeated annotation as a List, so the bare cast (AnnotationNode) value threw + // ClassCastException: ArrayList cannot be cast to AnnotationNode, aborting the transform for the whole mod. + // parseSingle unwraps the single-element case and yields null otherwise ("no slice"; the multi-element + // form is handled by SLICES above). A/B verified: this does NOT change carpet's outcome - the + // onBlockBroken/onExplosionDone strip count stayed identical with and without this change. + // Reference case: cookeymod (CRASH -> 61/7). + .parser((value, mapper) -> { + AnnotationNode node = ControlKeys.parseSingle(value, AnnotationNode.class); + return node == null ? null : SliceData.parse(new AnnotationHandle(node), mapper); + }) .serializer(SliceData::toAnnotationNode) .build(); public static final PropertyKey> SLICES = PropertyKey.>builder("slice") @@ -68,3 +85,4 @@ public class MixinKeys { public static final PropertyKey LOCALS = PropertyKey.create("locals", LocalCapture.class); public static final PropertyKey REQUIRE = PropertyKey.create("require", Integer.class); } + diff --git a/core/src/main/java/org/sinytra/adapter/patch/mixin/AccessorMixin.java b/core/src/main/java/org/sinytra/adapter/patch/mixin/AccessorMixin.java new file mode 100644 index 0000000..6d1aadb --- /dev/null +++ b/core/src/main/java/org/sinytra/adapter/patch/mixin/AccessorMixin.java @@ -0,0 +1,90 @@ +package org.sinytra.adapter.patch.mixin; + +import org.objectweb.asm.tree.ClassNode; +import org.sinytra.adapter.env.ctx.AuditTrail; +import org.sinytra.adapter.env.ctx.MixinContext; +import org.sinytra.adapter.patch.Recipe; +import org.sinytra.adapter.patch.TxResult; +import org.sinytra.adapter.patch.config.Configuration; +import org.sinytra.adapter.patch.config.MutableConfiguration; +import org.sinytra.adapter.patch.config.PropertyContainerTemplate; +import org.sinytra.adapter.patch.config.key.ControlKeys; +import org.sinytra.adapter.patch.config.key.MixinKeys; +import org.sinytra.adapter.patch.processor.Processors; +import org.sinytra.adapter.patch.resolver.Resolvers; + +/** + * Handles {@code @Accessor} mixins. + *

+ * Accessors are the one handler that does not target a method at all: they expose a field of + * the target class. The usual pipeline is method-first - {@code PipelineMethodTransformer.apply} + * resolves a target method before doing anything else and records a failure when it cannot - so + * simply registering this type would have marked every accessor as unadapted + * ({@code @Accessor} appears in 280 classes across 31 mods of the reference corpus) and made + * Connector's mixin safeguard reject all of them. + *

+ * That is why this type opts out with {@link #targetsMethod()} and validates itself instead: the + * field is looked up in the patched target class and only a genuinely missing field is reported. + */ +public class AccessorMixin implements MixinType { + private static final PropertyContainerTemplate TEMPLATE = PropertyContainerTemplate.builder() + .require(ControlKeys.MIXIN_TYPE, ControlKeys.TARGET_CLASS, ControlKeys.RETURN_TYPE, ControlKeys.PARAMETERS) + .keys(MixinKeys.ACCESSOR_VALUE, MixinKeys.REQUIRE) + .build(); + + @Override + public PropertyContainerTemplate getConfigurationTemplate() { + return TEMPLATE; + } + + @Override + public boolean targetsMethod() { + return false; + } + + @Override + public TxResult preProcess(MixinContext context, MutableConfiguration clean, Resolvers resolvers, Processors processors) { + return TxResult.SUCCESS; + } + + @Override + public TxResult postProcess(MixinContext context, Configuration clean, MutableConfiguration dirty, Recipe recipe) { + return TxResult.SUCCESS; + } + + @Override + public void validateNonMethodTarget(MixinContext context, Configuration config) { + String declared = config.getProperty(MixinKeys.ACCESSOR_VALUE).orElse(null); + final String fieldName = (declared == null || declared.isEmpty()) + ? inferFieldName(context.methodNode().name) + : declared; + + String targetClass = config.getTargetClass(); + ClassNode target = targetClass == null ? null : context.dirtyLookup().getClass(targetClass).orElse(null); + + boolean found = target != null && fieldName != null + && target.fields.stream().anyMatch(f -> f.name.equals(fieldName)); + + context.environment().auditTrail().recordResult(context, config, + found ? AuditTrail.Match.FULL : AuditTrail.Match.NONE); + } + + /** + * Mixin infers the field for {@code @Accessor} from the method name when no value is given: + * {@code getFoo}/{@code setFoo}/{@code isFoo} access the field {@code foo}. + */ + private static String inferFieldName(String methodName) { + String base; + if (methodName.startsWith("get") || methodName.startsWith("set")) { + base = methodName.substring(3); + } else if (methodName.startsWith("is")) { + base = methodName.substring(2); + } else { + return null; + } + if (base.isEmpty()) { + return null; + } + return Character.toLowerCase(base.charAt(0)) + base.substring(1); + } +} diff --git a/core/src/main/java/org/sinytra/adapter/patch/mixin/InvokerMixin.java b/core/src/main/java/org/sinytra/adapter/patch/mixin/InvokerMixin.java new file mode 100644 index 0000000..da8951d --- /dev/null +++ b/core/src/main/java/org/sinytra/adapter/patch/mixin/InvokerMixin.java @@ -0,0 +1,98 @@ +package org.sinytra.adapter.patch.mixin; + +import org.jetbrains.annotations.Nullable; +import org.objectweb.asm.tree.ClassNode; +import org.objectweb.asm.tree.MethodNode; +import org.sinytra.adapter.env.ctx.AuditTrail; +import org.sinytra.adapter.env.ctx.MixinContext; +import org.sinytra.adapter.patch.Recipe; +import org.sinytra.adapter.patch.TxResult; +import org.sinytra.adapter.patch.config.Configuration; +import org.sinytra.adapter.patch.config.MutableConfiguration; +import org.sinytra.adapter.patch.config.PropertyContainer; +import org.sinytra.adapter.patch.config.PropertyContainerTemplate; +import org.sinytra.adapter.patch.config.key.ControlKeys; +import org.sinytra.adapter.patch.config.key.MixinKeys; +import org.sinytra.adapter.patch.processor.Processors; +import org.sinytra.adapter.patch.resolver.Resolvers; +import org.sinytra.adapter.util.MethodQualifier; + +/** + * Handles {@code @Invoker} mixins. + *

+ * Like {@code @Accessor}, an invoker is an interface mixin: Mixin generates a + * method that calls the target, it does not inject into an existing method body. Running the + * method-target injection pipeline on it therefore always finds no injection point and reports a + * failure, which is why this type opts out with {@link #targetsMethod()} and validates itself + * instead - the target method named by {@code value} (or {@code callXxx} -> {@code xxx}) is looked + * up in the patched target class, and only a genuinely missing method is reported. + *

+ * Registering the type matters for the same reason as the others: before this, {@code MixinParser} + * skipped invokers entirely, so an invoker whose target had moved stayed in the jar and failed at + * runtime. {@code @Invoker} appears in 23 of the 46 mods of the reference corpus. + */ +public class InvokerMixin implements MixinType { + private static final PropertyContainerTemplate TEMPLATE = PropertyContainerTemplate.builder() + .require(ControlKeys.MIXIN_TYPE, ControlKeys.TARGET_CLASS, ControlKeys.RETURN_TYPE, ControlKeys.PARAMETERS) + .keys(MixinKeys.ACCESSOR_VALUE, MixinKeys.REQUIRE) + .build(); + + @Override + public PropertyContainerTemplate getConfigurationTemplate() { + return TEMPLATE; + } + + @Override + public boolean targetsMethod() { + return false; + } + + @Override + public TxResult preProcess(MixinContext context, MutableConfiguration clean, Resolvers resolvers, Processors processors) { + return TxResult.SUCCESS; + } + + @Override + public TxResult postProcess(MixinContext context, Configuration clean, MutableConfiguration dirty, Recipe recipe) { + return TxResult.SUCCESS; + } + + @Override + public void validateNonMethodTarget(MixinContext context, Configuration config) { + MethodQualifier qualifier = deriveTarget(config, context.methodNode()); + String name = qualifier == null ? null : qualifier.name(); + + String targetClass = config.getTargetClass(); + ClassNode target = targetClass == null ? null : context.dirtyLookup().getClass(targetClass).orElse(null); + + boolean found = target != null && name != null + && target.methods.stream().anyMatch(m -> m.name.equals(name)); + + context.environment().auditTrail().recordResult(context, config, + found ? AuditTrail.Match.FULL : AuditTrail.Match.NONE); + } + + /** + * The invoker names its target with {@code value}; without one Mixin falls back to the handler's + * own name with a {@code call}/{@code invoke}/{@code invoker} prefix removed. + */ + @Nullable + @Override + public MethodQualifier deriveTarget(PropertyContainer properties, MethodNode method) { + String name = properties.getProperty(MixinKeys.ACCESSOR_VALUE).orElse(null); + if (name == null || name.isEmpty()) { + name = method.name; + for (String prefix : new String[]{"invoker", "invoke", "call"}) { + if (name.startsWith(prefix) && name.length() > prefix.length()) { + name = name.substring(prefix.length()); + break; + } + } + if (name.isEmpty()) { + return null; + } + name = Character.toLowerCase(name.charAt(0)) + name.substring(1); + } + return new MethodQualifier(name, null); + } +} diff --git a/core/src/main/java/org/sinytra/adapter/patch/mixin/MixinType.java b/core/src/main/java/org/sinytra/adapter/patch/mixin/MixinType.java index cd71d1b..66c26db 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/mixin/MixinType.java +++ b/core/src/main/java/org/sinytra/adapter/patch/mixin/MixinType.java @@ -1,13 +1,17 @@ package org.sinytra.adapter.patch.mixin; +import org.jetbrains.annotations.Nullable; +import org.objectweb.asm.tree.MethodNode; import org.sinytra.adapter.env.ctx.MixinContext; import org.sinytra.adapter.patch.Recipe; import org.sinytra.adapter.patch.TxResult; import org.sinytra.adapter.patch.config.Configuration; import org.sinytra.adapter.patch.config.MutableConfiguration; +import org.sinytra.adapter.patch.config.PropertyContainer; import org.sinytra.adapter.patch.config.PropertyContainerTemplate; import org.sinytra.adapter.patch.processor.Processors; import org.sinytra.adapter.patch.resolver.Resolvers; +import org.sinytra.adapter.util.MethodQualifier; import java.util.EnumSet; import java.util.Set; @@ -26,6 +30,40 @@ default boolean canInject(MixinContext context, Configuration config) { return true; } + /** + * Types whose target is not named by a {@code method} property derive it from the mixin method + * itself: {@code @Overwrite} replaces the method of the same name, while {@code @Invoker} calls + * the method named by its {@code value} (falling back to {@code callXxx} -> {@code xxx}). + *

+ * Parsers use this to fill in the target qualifier before the usual resolution runs. + * + * @return the qualifier to use as {@code TARGET_METHOD}, or {@code null} when this type does not + * derive one + */ + @Nullable + default MethodQualifier deriveTarget(PropertyContainer properties, MethodNode method) { + return null; + } + + /** + * Whether this handler targets a method (and therefore belongs in the method-target pipeline). + *

+ * {@code @Accessor} returns {@code false}: it exposes a field, so the method-first pipeline would + * report every accessor as unadapted. Such types instead implement + * {@link #validateNonMethodTarget(MixinContext, Configuration)} and are dispatched there. + */ + default boolean targetsMethod() { + return true; + } + + /** + * Validation for types that opted out of the method pipeline via {@link #targetsMethod()}. + * Implementations must record an audit result themselves - the pipeline does not run for them. + * The default does nothing, so no result is recorded and the mixin stays invisible. + */ + default void validateNonMethodTarget(MixinContext context, Configuration config) { + } + TxResult preProcess(MixinContext context, MutableConfiguration clean, Resolvers resolvers, Processors processors); TxResult postProcess(MixinContext context, Configuration clean, MutableConfiguration dirty, Recipe recipe); diff --git a/core/src/main/java/org/sinytra/adapter/patch/mixin/MixinTypes.java b/core/src/main/java/org/sinytra/adapter/patch/mixin/MixinTypes.java index 840f9fa..b6801ef 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/mixin/MixinTypes.java +++ b/core/src/main/java/org/sinytra/adapter/patch/mixin/MixinTypes.java @@ -14,25 +14,37 @@ public class MixinTypes { private static final Map MIXIN_TYPES = new HashMap<>(); // TODO: - // MixinConstants.MODIFY_ARGS, MixinConstants.MODIFY_CONST, MixinConstants.WRAP_WITH_CONDITION, - // MixinConstants.OVERWRITE (dont forget special modifyTarget handling) - // MixinConstants.ACCESSOR + // MixinConstants.WRAP_WITH_CONDITION + // Measured 2026-09: @WrapWithCondition appears in 0 of the 46 mods of the reference corpus + // (it is a MixinExtras annotation and rare in practice), so it is deliberately left out. public static final MixinType INJECT = new InjectMixin(); public static final MixinType MODIFY_VAR = new ModifyVariableMixin(); public static final MixinType MODIFY_ARG = new ModifyArgMixin(); + public static final MixinType MODIFY_ARGS = new ModifyArgsMixin(); + public static final MixinType MODIFY_CONST = new ModifyConstantMixin(); public static final MixinType MODIFY_RET = new ModifyReturnValueMixin(); public static final MixinType REDIRECT = new RedirectMixin(); public static final MixinType WRAP_OP = new WrapOperationMixin(); public static final MixinType MODIFY_EXPR_VAL = new ModifyExpressionValueMixin(); + public static final MixinType OVERWRITE = new OverwriteMixin(); + public static final MixinType ACCESSOR = new AccessorMixin(); + public static final MixinType INVOKER = new InvokerMixin(); static { registerMixinType(Inject.class, INJECT); registerMixinType(ModifyVariable.class, MODIFY_VAR); registerMixinType(ModifyArg.class, MODIFY_ARG); + // NOTE: this map is keyed by annotation *internal names* (MixinParser passes + // Type.getType(desc).getInternalName()), so the "L...;" descriptor form would never match. + registerMixinType(MixinAnnotations.MODIFY_ARGS_INTERNAL_NAME, MODIFY_ARGS); + registerMixinType(MixinAnnotations.MODIFY_CONST_INTERNAL_NAME, MODIFY_CONST); registerMixinType(MixinAnnotations.MODIFY_RETURN_VAL_INTERNAL_NAME, MODIFY_RET); registerMixinType(Redirect.class, REDIRECT); registerMixinType(MixinAnnotations.WRAP_OPERATION_INTERNAL_NAME, WRAP_OP); registerMixinType(MixinAnnotations.MODIFY_EXPR_VAL_INTERNAL_NAME, MODIFY_EXPR_VAL); + registerMixinType(MixinAnnotations.OVERWRITE_INTERNAL_NAME, OVERWRITE); + registerMixinType(MixinAnnotations.ACCESSOR_INTERNAL_NAME, ACCESSOR); + registerMixinType(MixinAnnotations.INVOKER_INTERNAL_NAME, INVOKER); } @Nullable diff --git a/core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyArgsMixin.java b/core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyArgsMixin.java new file mode 100644 index 0000000..cd1b5f2 --- /dev/null +++ b/core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyArgsMixin.java @@ -0,0 +1,61 @@ +package org.sinytra.adapter.patch.mixin; + +import org.sinytra.adapter.env.ctx.MixinContext; +import org.sinytra.adapter.env.param.MethodParameters; +import org.sinytra.adapter.patch.Recipe; +import org.sinytra.adapter.patch.TxResult; +import org.sinytra.adapter.patch.config.Configuration; +import org.sinytra.adapter.patch.config.ConfigurationTemplates; +import org.sinytra.adapter.patch.config.MutableConfiguration; +import org.sinytra.adapter.patch.config.PropertyContainerTemplate; +import org.sinytra.adapter.patch.config.key.MixinKeys; +import org.sinytra.adapter.patch.processor.Processors; +import org.sinytra.adapter.patch.resolver.Resolvers; +import org.sinytra.adapter.patch.resolver.injection.ArbitraryInjectionPointSubResolver; +import org.sinytra.adapter.patch.resolver.injection.InjectionPointResolver; + +import java.util.List; + +import static org.sinytra.adapter.env.param.MethodParameters.ParamGroup.SINGLE_ANY; + +/** + * Handles {@code @ModifyArgs} mixins. + *

+ * The handler signature is fixed - {@code void handler(Args args)} - no matter what the injection + * point looks like, so unlike {@link ModifyArgMixin} there is no argument type to re-derive. The + * work here is registering the type at all: previously {@code MixinParser} skipped it, so a + * {@code @ModifyArgs} mixin that could not be injected was neither adapted nor recorded and only + * blew up at runtime. + */ +public class ModifyArgsMixin implements MixinType { + private static final PropertyContainerTemplate TEMPLATE = ConfigurationTemplates.MIXIN_AT.extend() + .pluralKeys(MixinKeys.TARGET_METHOD) + .build(); + + @Override + public PropertyContainerTemplate getConfigurationTemplate() { + return TEMPLATE; + } + + @Override + public TxResult preProcess(MixinContext context, MutableConfiguration clean, Resolvers resolvers, Processors processors) { + resolvers.getOrThrow(InjectionPointResolver.class) + .addSubResolver(new ArbitraryInjectionPointSubResolver()); + + clean.setParameters(MethodParameters.create(context.methodNode(), List.of(SINGLE_ANY))); + + return TxResult.SUCCESS; + } + + @Override + public TxResult postProcess(MixinContext context, Configuration clean, MutableConfiguration dirty, Recipe recipe) { + dirty.inheritProperyIfAbsent(MixinKeys.ORDINAL); + + // The handler always takes the argument list and returns void, so its signature never has to + // follow the (possibly re-targeted) injection point. + dirty.inheritParameters(); + dirty.inheritReturnType(); + + return TxResult.SUCCESS; + } +} diff --git a/core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyConstantMixin.java b/core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyConstantMixin.java new file mode 100644 index 0000000..401b771 --- /dev/null +++ b/core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyConstantMixin.java @@ -0,0 +1,108 @@ +package org.sinytra.adapter.patch.mixin; + +import org.objectweb.asm.Type; +import org.sinytra.adapter.env.ann.ConstantData; +import org.objectweb.asm.tree.AbstractInsnNode; +import org.sinytra.adapter.env.ctx.MixinContext; +import org.sinytra.adapter.env.ctx.TargetPair; +import org.sinytra.adapter.env.param.MethodParameters; +import org.sinytra.adapter.patch.Recipe; +import org.sinytra.adapter.patch.TxResult; +import org.sinytra.adapter.patch.config.Configuration; +import org.sinytra.adapter.patch.config.ConfigurationTemplates; +import org.sinytra.adapter.patch.config.MutableConfiguration; +import org.sinytra.adapter.patch.config.PropertyContainerTemplate; +import org.sinytra.adapter.patch.config.key.MixinKeys; +import org.sinytra.adapter.patch.processor.Processors; +import org.sinytra.adapter.patch.resolver.Resolvers; +import org.spongepowered.asm.mixin.injection.points.BeforeConstant; + +import java.util.List; + +import static org.sinytra.adapter.env.param.MethodParameters.ParamGroup.SINGLE_ANY; + +/** + * Handles {@code @ModifyConstant} mixins. + *

+ * Unlike the other handlers, {@code @ModifyConstant} does not carry an {@code @At}: the injection + * point is derived from the {@code @Constant} annotation itself. The pipeline already knows how to + * locate such constants ({@link PipelineMethodTransformer#computeConstantTargetInsns}), but until + * this type existed the annotation was not registered in {@link MixinTypes}, so + * {@code MixinParser} skipped the method outright: it was never resolved, never validated and never + * recorded. A handler whose target constant had moved therefore stayed in the transformed jar and + * blew up at runtime with {@code InjectionError: ... failed injection check, (0/1) succeeded}. + *

+ * Registering the type fixes two things at once: + *

    + *
  • mixins whose constant still resolves take the pipeline's normal path, and
    • + *
  • mixins whose constant no longer exists fail in {@link #postProcess} - which both drops + * them from the jar and, crucially, records the failure so Connector's mixin safeguard can + * refuse the mod instead of letting the game crash later.
    • + *
+ */ +public class ModifyConstantMixin implements MixinType { + private static final PropertyContainerTemplate TEMPLATE = ConfigurationTemplates.MIXIN_BASE.extend() + .keys(MixinKeys.TARGET_CONSTANT, MixinKeys.ORDINAL) + .pluralKeys(MixinKeys.SLICES) + .build(); + + @Override + public PropertyContainerTemplate getConfigurationTemplate() { + return TEMPLATE; + } + + @Override + public TxResult preProcess(MixinContext context, MutableConfiguration clean, Resolvers resolvers, Processors processors) { + // The handler receives the constant it replaces and returns the replacement value. + clean.setParameters(MethodParameters.create(context.methodNode(), List.of(SINGLE_ANY))); + + return TxResult.SUCCESS; + } + + @Override + public TxResult postProcess(MixinContext context, Configuration clean, MutableConfiguration dirty, Recipe recipe) { + dirty.inheritProperyIfAbsent(MixinKeys.ORDINAL); + + // The template declares TARGET_CONSTANT, but nothing ever put a value in it: PropertyProcessor only + // forwards keys that are ALREADY set, it does not read them off the annotation. A dirty config with + // "constant" missing therefore failed validation ("invalid DIRTY config") and the mixin was dropped -- + // even after its target had been successfully retargeted to the method the constant moved into. + if (!dirty.hasProperty(MixinKeys.TARGET_CONSTANT)) { + context.methodAnnotation().getNested("constant") + .flatMap(ConstantData::parse) + .ifPresent(constant -> dirty.setProperty(MixinKeys.TARGET_CONSTANT, constant)); + } + + // MIXIN_BASE.require() mandates PARAMETERS and RETURN_TYPE, and every other mixin type populates both + // here. This one did not, so its dirty config never validated ("invalid DIRTY config") and the handler + // was dropped - even once its target had been retargeted and its constant resolved. + dirty.setParameters(MethodParameters.create(context.methodNode(), List.of(SINGLE_ANY))); + dirty.setReturnType(Type.getReturnType(context.methodNode().desc)); + + if (dirty.getTargetMethod() == null) { + return TxResult.FAIL; + } + + TargetPair dirtyTarget = context.methods().findOwnMethodPair(context.dirtyLookup(), dirty.getTargetMethod()); + if (dirtyTarget == null) { + return TxResult.FAIL; + } + + // Ask the same machinery the pipeline uses, so that "can this constant be found?" is answered + // exactly the way Mixin will answer it at runtime. + List constantInsns = context.methods().computeInjectionTargetInsns( + dirtyTarget, + () -> context.methodAnnotation().getNested("constant").orElse(null), + (ctx, handle) -> new BeforeConstant(ctx, handle.unwrap(), Type.getReturnType(context.methodNode().desc).getDescriptor()), + false + ); + + if (constantInsns.isEmpty()) { + return TxResult.FAIL; + } + + return TxResult.SUCCESS; + } +} + + diff --git a/core/src/main/java/org/sinytra/adapter/patch/mixin/OverwriteMixin.java b/core/src/main/java/org/sinytra/adapter/patch/mixin/OverwriteMixin.java new file mode 100644 index 0000000..4df5100 --- /dev/null +++ b/core/src/main/java/org/sinytra/adapter/patch/mixin/OverwriteMixin.java @@ -0,0 +1,80 @@ +package org.sinytra.adapter.patch.mixin; + +import org.objectweb.asm.tree.MethodNode; +import org.sinytra.adapter.env.ctx.MixinContext; +import org.sinytra.adapter.env.ctx.TargetPair; +import org.sinytra.adapter.env.param.MethodParameters; +import org.sinytra.adapter.patch.Recipe; +import org.sinytra.adapter.patch.TxResult; +import org.sinytra.adapter.patch.config.Configuration; +import org.sinytra.adapter.patch.config.MutableConfiguration; +import org.sinytra.adapter.patch.config.PropertyContainer; +import org.sinytra.adapter.patch.config.PropertyContainerTemplate; +import org.sinytra.adapter.patch.config.key.ControlKeys; +import org.sinytra.adapter.patch.config.key.MixinKeys; +import org.sinytra.adapter.patch.processor.Processors; +import org.sinytra.adapter.patch.resolver.Resolvers; +import org.sinytra.adapter.util.MethodQualifier; + +import java.util.List; + +import static org.sinytra.adapter.env.param.MethodParameters.ParamGroup.METHOD_PARAMS; + +/** + * Handles {@code @Overwrite} mixins. + *

+ * {@code @Overwrite} is the one handler that carries no target information at all: it replaces the + * target method wholesale, and the target is identified by the handler's own name. That is + * why it needs {@link MixinType#derivesTargetFromMixinMethod()} and a template that does not require + * {@code method} - see {@code MixinParser}, which fills in the qualifier (name only) so the usual + * "resolve by name inside the target class" step can complete it. + *

+ * Registering the type is what makes an unadaptable overwrite visible: previously + * {@code MixinParser} skipped it entirely, so a handler whose target had moved stayed in the jar + * and failed only at runtime. + */ +public class OverwriteMixin implements MixinType { + /** + * Note the difference from {@code MIXIN_BASE}: {@code TARGET_METHOD} is not required, because + * {@code @Overwrite} has no {@code method} property - it is derived from the handler instead. + */ + private static final PropertyContainerTemplate TEMPLATE = PropertyContainerTemplate.builder() + .require(ControlKeys.MIXIN_TYPE, ControlKeys.TARGET_CLASS, ControlKeys.PARAMETERS, ControlKeys.RETURN_TYPE) + .keys(MixinKeys.LOCALS, MixinKeys.REQUIRE) + .pluralKeys(MixinKeys.TARGET_METHOD) + .build(); + + @Override + public PropertyContainerTemplate getConfigurationTemplate() { + return TEMPLATE; + } + + @Override + public MethodQualifier deriveTarget(PropertyContainer properties, MethodNode method) { + // @Overwrite has no target information: it replaces the method with its own name. + return new MethodQualifier(method.name, null); + } + + @Override + public TxResult preProcess(MixinContext context, MutableConfiguration clean, Resolvers resolvers, Processors processors) { + // An overwrite takes over the whole method, so the handler mirrors the target's parameters. + clean.setParameters(MethodParameters.create(context.methodNode(), List.of(METHOD_PARAMS))); + + return TxResult.SUCCESS; + } + + @Override + public TxResult postProcess(MixinContext context, Configuration clean, MutableConfiguration dirty, Recipe recipe) { + if (dirty.getTargetMethod() == null || dirty.getTargetMethod().desc() == null) { + // No method of that name exists in the patched target class any more. + return TxResult.FAIL; + } + + TargetPair target = context.methods().findOwnMethodPair(context.dirtyLookup(), dirty.getTargetMethod()); + if (target == null) { + return TxResult.FAIL; + } + + return TxResult.SUCCESS; + } +} diff --git a/core/src/main/java/org/sinytra/adapter/patch/mixin/RedirectMixin.java b/core/src/main/java/org/sinytra/adapter/patch/mixin/RedirectMixin.java index f4784e1..af4cb0d 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/mixin/RedirectMixin.java +++ b/core/src/main/java/org/sinytra/adapter/patch/mixin/RedirectMixin.java @@ -1,5 +1,8 @@ package org.sinytra.adapter.patch.mixin; +import org.objectweb.asm.Opcodes; +import org.objectweb.asm.tree.AbstractInsnNode; +import org.objectweb.asm.tree.MethodInsnNode; import org.objectweb.asm.Type; import org.sinytra.adapter.env.ctx.MethodHelper; import org.sinytra.adapter.env.ctx.MixinContext; @@ -23,6 +26,7 @@ import org.sinytra.adapter.util.MethodQualifier; import java.util.EnumSet; +import java.util.ArrayList; import java.util.List; import java.util.Set; @@ -51,8 +55,20 @@ public TxResult preProcess(MixinContext context, MutableConfiguration clean, Res .addAfter(ParametersProcessor.class, new DivertRedirectProcessor()) .addAfter(ParametersProcessor.class, new ParameterUsageProcessor()); - if (clean.getAtData() == null || !MixinAnnotationConstants.AT_VAL_INVOKE.equals(clean.getAtData().getValue())) + if (clean.getAtData() == null) return TxResult.FAIL; + if (!MixinAnnotationConstants.AT_VAL_INVOKE.equals(clean.getAtData().getValue())) { + // @Redirect is legal for FIELD (and other) targets too - e.g. carpet's + // @Redirect(method = "causeExtraKnockback", at = @At(value = "FIELD", + // target = "Lnet/minecraft/world/entity/Entity;hurtMarked:Z")). Failing unconditionally here + // dropped such mixins outright even though the referenced member still exists and needs no + // rewriting. Model the handler as written and let the pipeline's "does the injection point still + // resolve?" check decide instead. + clean.setParameters(MethodParameters.create(context.methodNode(), + List.of(MethodParameters.ParamGroup.SINGLE_ANY))); + clean.setReturnType(Type.getReturnType(context.methodNode().desc)); + return TxResult.SUCCESS; + } MethodQualifier targetDesc = clean.getAtData().getTarget().flatMap(MethodQualifier::parse).orElse(null); if (targetDesc == null) @@ -86,18 +102,69 @@ public TxResult postProcess(MixinContext context, Configuration clean, MutableCo if (dirty.getTargetMethod() == null) return TxResult.FAIL; + // A non-INVOKE @Redirect (e.g. FIELD) targets a member, not a call: there is no method to + // resolve or re-describe, and the ref the mod wrote stays valid. Nothing to adapt. + if (!MixinAnnotationConstants.AT_VAL_INVOKE.equals(dirty.getAtData().getValue())) + return TxResult.SUCCESS; + MethodQualifier targetDesc = dirty.getAtData().getTarget().flatMap(MethodQualifier::parse).orElse(null); if (targetDesc == null || targetDesc.desc() == null) return TxResult.FAIL; + // Locate the call this @At addresses. The injection point IS a call instruction inside the method + // being injected into, so find it THERE rather than resolving the @At's target through a class + // lookup. That lookup has to see the declaring class, and NeoForge routinely declares such members on + // its extension INTERFACES (BlockState implements IBlockStateExtension, which declares canStickTo) - + // a lookup of the @At's owner then misses it and the mixin is dropped as "failed postProcess". The + // call itself carries the real owner and descriptor regardless. What we find here is also exactly + // what Mixin binds to, so the handler is built to match. + TargetPair injected = context.methods().findOwnMethodPair(context.dirtyLookup(), dirty.getTargetMethod()); + MethodInsnNode callInsn = null; + if (injected != null) { + List calls = new ArrayList<>(); + for (AbstractInsnNode node : context.methods().findInjectionTargetInsns(injected, dirty.getAtData())) { + if (node instanceof MethodInsnNode mi) { + calls.add(mi); + } + } + // Several call sites to the SAME member are normal and expected - Mixin's @At without an ordinal + // addresses the member, not one occurrence (carpet calls canStickTo twice inside addBlockLine). + // What has to be unambiguous is which MEMBER it is, so compare the distinct owner+name+descriptor + // rather than the instruction count. + if (calls.stream().map(c -> c.owner + c.name + c.desc).distinct().limit(2).count() == 1) { + callInsn = calls.getFirst(); + } + } + TargetPair dirtyTarget = context.methods().findOwnMethodPair(context.dirtyLookup(), targetDesc); - if (dirtyTarget == null) + if (dirtyTarget == null) { + // Fall back to the descriptor being ignored (MethodQualifier#ignoreDesc leaves desc null, which + // matches any descriptor) - the same owner+name strategy ChangedMemberDescriptorSubResolver + // applies to call sites. + dirtyTarget = context.methods().findOwnMethodPair(context.dirtyLookup(), targetDesc.ignoreDesc()); + } + // Either the call itself or a resolvable target is enough - see the comment above for why the call is + // the better source when both exist. + if (dirtyTarget == null && callInsn == null) return TxResult.FAIL; List dirtyCaptured = context.methods().resolveCapturedMethodParams(recipe.clean(), recipe.dirty()); - List callTypes = Parameters.getParameterTypes(targetDesc.desc()); - if (!MethodHelper.isStatic(dirtyTarget.methodNode())) { - Type owner = Type.getObjectType(dirtyTarget.classNode().name); + + // Build the handler's parameter model from the call we located when there is one. Deriving it from the + // resolved method instead is what broke carpet's updateNeighborsMaybe: the @At names the CALL + // (Level#updateNeighborsAt(BlockPos, Block)V, still 2-arg) while the method DECLARATION gained an + // Orientation parameter, so the two disagree and Mixin rejected the handler. + // See 12-issue草案.md entry 15. + List callTypes = callInsn != null + ? Parameters.getParameterTypes(callInsn.desc) + : Parameters.getParameterTypes(targetDesc.desc()); + boolean staticCall = callInsn != null + ? callInsn.getOpcode() == Opcodes.INVOKESTATIC + : MethodHelper.isStatic(dirtyTarget.methodNode()); + if (!staticCall) { + Type owner = callInsn != null + ? Type.getObjectType(callInsn.owner) + : Type.getObjectType(dirtyTarget.classNode().name); callTypes.addFirst(owner); } @@ -121,3 +188,11 @@ private boolean isStaticRedirect(MixinContext context, MethodQualifier targetDes return methodParams.size() >= callTypes.size() && methodParams.subList(0, callTypes.size()).equals(callTypes); } } + + + + + + + + From 79e3ef1a836dbf1e5266282803cbd94a02effca9 Mon Sep 17 00:00:00 2001 From: Tong317 Date: Thu, 1 Oct 2026 18:23:32 +0800 Subject: [PATCH 04/16] Sync parameter annotation counts with the descriptor --- .../org/sinytra/adapter/patch/Patcher.java | 181 +++++++++++++++++- 1 file changed, 177 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/sinytra/adapter/patch/Patcher.java b/core/src/main/java/org/sinytra/adapter/patch/Patcher.java index 43963d7..e5d8e9e 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/Patcher.java +++ b/core/src/main/java/org/sinytra/adapter/patch/Patcher.java @@ -3,15 +3,21 @@ import com.google.common.collect.HashMultimap; import com.google.common.collect.Multimap; import com.mojang.logging.LogUtils; +import org.objectweb.asm.Type; +import org.objectweb.asm.tree.AbstractInsnNode; +import org.objectweb.asm.tree.AnnotationNode; import org.objectweb.asm.tree.ClassNode; +import org.objectweb.asm.tree.MethodInsnNode; import org.objectweb.asm.tree.MethodNode; import org.sinytra.adapter.analysis.MixinGroups; import org.sinytra.adapter.analysis.selector.AnnotationHandle; +import org.sinytra.adapter.env.ann.AtData; import org.sinytra.adapter.env.ann.ClassTarget; import org.sinytra.adapter.env.ctx.*; import org.sinytra.adapter.env.util.MixinAnnotationConstants; import org.sinytra.adapter.patch.MixinParser.MixinMethodHandle; import org.sinytra.adapter.patch.config.key.MixinKeys; +import org.sinytra.adapter.patch.config.key.SpecialKeys; import org.sinytra.adapter.patch.config.MutableConfiguration; import org.sinytra.adapter.patch.config.PropertyContainerTemplate; import org.sinytra.adapter.patch.mixin.MixinFlag; @@ -24,6 +30,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.List; +import java.util.Objects; import java.util.Set; import static org.sinytra.adapter.util.AdapterUtil.MIXINPATCH; @@ -60,15 +67,27 @@ public PatchResult process(ClassNode classNode) { if (mixinClass == null) return result; // Mixin-level transformations + List failedMixins = new ArrayList<>(); for (MixinParser.MixinMethodHandle mixin : mixinClass.mixins()) { - PatchResult subResult = processMixin(classNode, classTarget, context, mixin); + PatchResult subResult = processMixin(classNode, classTarget, context, mixin, failedMixins); result = result.or(subResult); } + if (!failedMixins.isEmpty() && this.environment.stripFailingMixins()) { + result = result.or(stripFailedMixins(classNode, failedMixins)); + } + context.run(); MixinGroups groups = MixinGroups.create(mixinClass.mixins().stream().map(MixinMethodHandle::methodNode).toList()); this.environment.auditTrail().processGroups(classNode, groups); + // Must be the very last thing: context.run() applies deferred changes that can still add parameter + // annotations, and ASM sizes their arrays from the descriptor as it stood when the array was first + // created, so they can end up shorter than the descriptor. + if (normalizeParameterAnnotations(classNode)) { + result = result.or(PatchResult.COMPUTE_FRAMES); + } + return result; } @@ -78,16 +97,47 @@ public PatchResult process(ClassNode classNode) { */ private static PatchResult skip(MixinContext context, org.sinytra.adapter.patch.config.Configuration config, String reason) { context.environment().auditTrail().recordResult(context, config, org.sinytra.adapter.env.ctx.AuditTrail.Match.NONE); + // The early exits in #processMixin run OUTSIDE any pushAudit/popAudit frame (only the EARLY/LOADED/ + // VALIDATED transformer loops open one), so recordCtxAudit used to throw here and the reason was + // swallowed - the mixin showed up in the failure list with no explanation at all, which is very + // expensive to diagnose. Open a frame so the reason is always reported. + context.pushAudit(SKIP_REASON_FRAME); try { context.recordCtxAudit("Skipped mixin: %s", reason); } catch (RuntimeException ignored) { - // Not every exit runs inside an audit frame (pushAudit/popAudit). recordResult above is - // what actually makes the skip visible; the reason is best-effort. + // still best-effort: recordResult above is what makes the skip itself visible + } finally { + context.popAudit(); } return PatchResult.PASS; } - private PatchResult processMixin(ClassNode classNode, ClassTarget classTarget, PatchContext patchContext, MixinParser.MixinMethodHandle mixin) { + /** Audit frame marker for give-up exits; its {@code toString} is what the report shows. */ + private static final Object SKIP_REASON_FRAME = new Object() { + @Override + public String toString() { + return "skip"; + } + }; + + private PatchResult processMixin(ClassNode classNode, ClassTarget classTarget, PatchContext patchContext, MixinParser.MixinMethodHandle mixin, List failed) { + MixinType handlerType = mixin.mixinType(); + + // Types that do not target a method (@Accessor exposes a field) have no TARGET_METHOD, so the + // early return below used to drop them silently: no adaptation, no audit entry, nothing. + // Dispatch them to their own validation instead, which records the result itself. + if (!handlerType.targetsMethod()) { + MixinContext nonMethodContext = new MixinContext(handlerType, patchContext, classTarget, classNode, + mixin.methodNode(), mixin.methodAnnotation(), null, handlerType.getFlags()); + this.environment.auditTrail().prepareMethod(nonMethodContext); + + MutableConfiguration nonMethodConfig = MutableConfiguration.create(handlerType.getConfigurationTemplate()); + nonMethodConfig.mergeFrom(mixin.properties()); + + handlerType.validateNonMethodTarget(nonMethodContext, nonMethodConfig); + return PatchResult.PASS; + } + MethodQualifier target = mixin.properties().getProperty(MixinKeys.TARGET_METHOD).orElse(null); if (target == null) return PatchResult.PASS; @@ -119,6 +169,7 @@ private PatchResult processMixin(ClassNode classNode, ClassTarget classTarget, P TxResult preResult = mixinType.preProcess(mixinContext, configuration, mixinContext.getResolvers(), mixinContext.getProcessors()); if (preResult == TxResult.FAIL) { LOGGER.debug(MIXINPATCH, "Skipping mixin {} due to failed preProcess", mixinId); + failed.add(mixin); return skip(mixinContext, configuration, "failed preProcess"); } @@ -133,6 +184,7 @@ private PatchResult processMixin(ClassNode classNode, ClassTarget classTarget, P // Validate clean config if (!configuration.validate()) { LOGGER.debug(MIXINPATCH, "Skipping mixin {} due to invalid CLEAN config", mixinId); + failed.add(mixin); return skip(mixinContext, configuration, "invalid CLEAN config"); } @@ -148,9 +200,129 @@ private PatchResult processMixin(ClassNode classNode, ClassTarget classTarget, P result = result.or(txResult); } + // A "locals = LocalCapture..." handler's capture list is compiled against the injection site as it + // was when the mod was built, and Mixin aborts at runtime when that list no longer lines up with the + // patched method ("Critical injection failure: LVT in ..." - CAPTURE_FAILHARD exists to do exactly + // that). LocalCaptureUpgradeTransformer marks the config once it has confirmed or re-derived the + // list; a handler without that mark cannot be trusted, so treat it as failed. Note this cannot go + // through the audit trail: Match.or() is a maximum, so a later success would overwrite a NONE. + if (configuration.hasProperty(MixinKeys.LOCALS) + && !configuration.hasProperty(SpecialKeys.LOCALS_UPGRADED)) { + LOGGER.debug(MIXINPATCH, "Mixin {} captures locals that could not be verified; it would abort at runtime", mixinId); + failed.add(mixin); + } + + // A resolver failure inside the pipeline records Match.NONE rather than throwing, so the audit + // trail is the only place that knows this handler did not make it. + if (this.environment.auditTrail().getMatch(mixinContext) == AuditTrail.Match.NONE) { + failed.add(mixin); + } + return result; } + /** + * Removes handlers that could not be adapted, so Mixin never registers them and the mod loads with + * those features disabled instead of crashing at runtime. + *

+ * A handler is only removed when nothing else in the class calls it - dropping a referenced method + * would produce a broken class, which is worse than the failure we are working around. + */ + private PatchResult stripFailedMixins(ClassNode classNode, List failed) { + boolean removedAny = false; + for (MixinParser.MixinMethodHandle mixin : failed) { + MethodNode method = mixin.methodNode(); + if (isReferenced(classNode, method)) { + LOGGER.debug(MIXINPATCH, "Not stripping {}#{}, it is referenced by other code in the class", + classNode.name, method.name); + continue; + } + if (classNode.methods.remove(method)) { + removedAny = true; + LOGGER.warn(MIXINPATCH, "Removed unadaptable mixin handler {}#{}{} - the mod loads with this feature disabled", + classNode.name, method.name, method.desc); + } + } + // Removing methods invalidates any previously computed frames. + return removedAny ? PatchResult.COMPUTE_FRAMES : PatchResult.PASS; + } + + /** + * Keeps a method's parameter-annotation bookkeeping consistent with its descriptor. + *

+ * ASM tracks two separate things: the annotation arrays themselves, and + * {@code visible/invisibleAnnotableParameterCount} - the {@code num_parameters} value of the + * RuntimeVisible/InvisibleParameterAnnotations attributes, which {@code MethodWriter} copies straight + * into its own arrays and then walks in + * {@code AnnotationWriter.computeParameterAnnotationsSize(String, AnnotationWriter[], int)}: + * 

+     *   for (int i = 0; i < annotableParameterCount; ++i) {
+     *     AnnotationWriter annotationWriter = annotationWriters[i];   // <- IndexOutOfBounds here
+     *
+ * Transformers here add and remove handler parameters, so a count captured for the old descriptor + * outlives the array that matched it and the write blows up with + * {@code ArrayIndexOutOfBoundsException: Index N out of bounds for length N}. Because the failure + * happens while writing - outside this method - it takes the whole mod's transform down. + *

+ * Resizing the arrays alone is not enough; the counts have to be re-derived too. + * + * @return whether anything changed + */ + private static boolean normalizeParameterAnnotations(ClassNode classNode) { + boolean changed = false; + for (MethodNode method : classNode.methods) { + int paramCount = Type.getArgumentTypes(method.desc).length; + + List[] visible = resizeParameterAnnotations(method.visibleParameterAnnotations, paramCount); + if (visible != method.visibleParameterAnnotations) { + method.visibleParameterAnnotations = visible; + changed = true; + } + List[] invisible = resizeParameterAnnotations(method.invisibleParameterAnnotations, paramCount); + if (invisible != method.invisibleParameterAnnotations) { + method.invisibleParameterAnnotations = invisible; + changed = true; + } + + int visibleCount = visible == null ? 0 : visible.length; + if (method.visibleAnnotableParameterCount != visibleCount) { + method.visibleAnnotableParameterCount = visibleCount; + changed = true; + } + int invisibleCount = invisible == null ? 0 : invisible.length; + if (method.invisibleAnnotableParameterCount != invisibleCount) { + method.invisibleAnnotableParameterCount = invisibleCount; + changed = true; + } + } + return changed; + } + + @SuppressWarnings("unchecked") + private static List[] resizeParameterAnnotations(List[] annotations, int paramCount) { + if (annotations == null || annotations.length == paramCount) { + return annotations; + } + List[] resized = (List[]) new List[paramCount]; + System.arraycopy(annotations, 0, resized, 0, Math.min(annotations.length, paramCount)); + return resized; + } + + private static boolean isReferenced(ClassNode classNode, MethodNode method) { + for (MethodNode other : classNode.methods) { + if (other == method) continue; + for (AbstractInsnNode insn : other.instructions) { + if (insn instanceof MethodInsnNode call + && call.name.equals(method.name) + && call.desc.equals(method.desc) + && call.owner.equals(classNode.name)) { + return true; + } + } + } + return false; + } + private Collection getTransformers(TxPhase phase) { return this.methodTransformers.get(phase); } @@ -193,3 +365,4 @@ public Patcher build() { } } } + From 85bfc5326aec67767d48120323a870270aa790e4 Mon Sep 17 00:00:00 2001 From: Tong317 Date: Thu, 1 Oct 2026 18:23:32 +0800 Subject: [PATCH 05/16] Fix ClassTarget parsing for dotted class names --- .../main/java/org/sinytra/adapter/env/ann/ClassTarget.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/core/src/main/java/org/sinytra/adapter/env/ann/ClassTarget.java b/core/src/main/java/org/sinytra/adapter/env/ann/ClassTarget.java index edf6ba7..c1ec79c 100644 --- a/core/src/main/java/org/sinytra/adapter/env/ann/ClassTarget.java +++ b/core/src/main/java/org/sinytra/adapter/env/ann/ClassTarget.java @@ -44,6 +44,12 @@ public static ClassTarget parse(AnnotationHandle handle, RefMapper mapper) { .or(() -> handle.>getValue(MIXIN_TARGETS).map(v -> { List types = v.get().stream() .map(mapper::remap) + // "targets" holds dotted binary names (net.minecraft.Foo$Bar), while Type.getObjectType + // expects an internal name (net/minecraft/Foo$Bar). Passing the dotted form through + // produced a Type whose internal name kept the dots, so every subsequent class lookup + // missed and the mixin was reported as "target method not found". Note that the '$' of + // an inner class must be preserved - only '.' becomes '/'. + .map(name -> name.replace('.', '/')) .map(Type::getObjectType) .toList(); return new ClassTarget(types, Either.right(v)); From 9dc465364ed0bd517cacdc05bd5498ec2171bc1a Mon Sep 17 00:00:00 2001 From: Tong317 Date: Thu, 1 Oct 2026 18:23:33 +0800 Subject: [PATCH 06/16] Resolve injection points after descriptor changes --- .../ChangedMemberDescriptorSubResolver.java | 80 +++++++++++++++++++ .../injection/InjectionPointResolver.java | 1 + 2 files changed, 81 insertions(+) create mode 100644 core/src/main/java/org/sinytra/adapter/patch/resolver/injection/ChangedMemberDescriptorSubResolver.java diff --git a/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/ChangedMemberDescriptorSubResolver.java b/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/ChangedMemberDescriptorSubResolver.java new file mode 100644 index 0000000..8e7951d --- /dev/null +++ b/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/ChangedMemberDescriptorSubResolver.java @@ -0,0 +1,80 @@ +package org.sinytra.adapter.patch.resolver.injection; + +import org.jetbrains.annotations.Nullable; +import org.objectweb.asm.tree.MethodInsnNode; +import org.sinytra.adapter.analysis.method.MethodCallAnalyzer; +import org.sinytra.adapter.env.ann.AtData; +import org.sinytra.adapter.env.ctx.MixinContext; +import org.sinytra.adapter.env.ctx.TargetPair; +import org.sinytra.adapter.patch.Recipe; +import org.sinytra.adapter.patch.config.Configuration; +import org.sinytra.adapter.patch.config.MutableConfiguration; +import org.sinytra.adapter.patch.resolver.SubResolver; +import org.sinytra.adapter.util.MethodQualifier; + +import java.util.List; + +import static org.sinytra.adapter.env.util.MixinAnnotationConstants.AT_VAL_INVOKE; +import static org.sinytra.adapter.env.util.MixinAnnotationConstants.AT_VAL_INVOKE_ASSIGN; + +/** + * Handles injection points whose target member survived but changed its descriptor. + *

+ * Example: {@code Level.updateNeighborsAt(BlockPos, Block)} became + * {@code Level.updateNeighborsAt(BlockPos, Block, Orientation)}. The owner and name still match, so the + * call site is still there - only the original {@code @At} descriptor no longer matches anything, which + * made the whole injection point unresolvable. + *

+ * {@link OverloadedInjectionPointSubResolver} covers a related but narrower situation (the old + * overload is still present and marked {@code @Deprecated}); when the old member is gone entirely that + * resolver bails out, which is exactly the case handled here. Note that this is deliberately + * conservative: the replacement has to be unambiguous, otherwise changing the target would be a guess. + */ +public class ChangedMemberDescriptorSubResolver implements SubResolver { + @Nullable + @Override + public Configuration resolve(MixinContext context, Recipe recipe) { + AtData at = recipe.clean().getAtData(); + if (at == null) { + return null; + } + if (!AT_VAL_INVOKE.equals(at.getValue()) && !AT_VAL_INVOKE_ASSIGN.equals(at.getValue())) { + return null; + } + + String targetDesc = at.getTarget().orElse(null); + if (targetDesc == null) { + return null; + } + + MethodQualifier original = MethodQualifier.parse(targetDesc).orElse(null); + if (original == null || original.name() == null) { + return null; + } + + TargetPair dirtyPair = recipe.getDirtyTarget(); + if (dirtyPair == null) { + return null; + } + + // Match on owner + name only: the descriptor is precisely what changed. + List calls = MethodCallAnalyzer.getMethodCallMinsns(dirtyPair.methodNode(), original.ignoreDesc()); + if (calls.isEmpty()) { + return null; + } + + // Only adapt when the replacement is unambiguous; two different overloads would be a guess. + if (calls.stream().map(call -> call.name + call.desc).distinct().limit(2).count() > 1) { + return null; + } + + MethodInsnNode call = calls.getFirst(); + if (call.desc.equals(original.desc())) { + // Descriptor unchanged - this resolver has nothing to contribute. + return null; + } + + return MutableConfiguration.create() + .setAtData(at.withTarget(call)); + } +} diff --git a/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/InjectionPointResolver.java b/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/InjectionPointResolver.java index d20851f..5709f61 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/InjectionPointResolver.java +++ b/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/InjectionPointResolver.java @@ -16,6 +16,7 @@ public class InjectionPointResolver extends CompoundResolver { public InjectionPointResolver() { addSubResolver(new OverloadedInjectionPointSubResolver()); + addSubResolver(new ChangedMemberDescriptorSubResolver()); addSubResolver(new InheritedInjectionPointSubResolver()); addSubResolver(InjectionPointSubResolvers.REPLACED_TYPE); addSubResolver(InjectionPointSubResolvers.EXTRACTED_CALL); From 578c9357faddf8b5a034d9f2b66bc3947e46a835 Mon Sep 17 00:00:00 2001 From: Tong317 Date: Thu, 1 Oct 2026 18:23:33 +0800 Subject: [PATCH 07/16] Add the stripFailingMixins toggle --- .../sinytra/adapter/env/ctx/PatchEnvironment.java | 15 +++++++++++++++ .../org/sinytra/adapter/patch/DynamicPatches.java | 15 +++++++++++++-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/sinytra/adapter/env/ctx/PatchEnvironment.java b/core/src/main/java/org/sinytra/adapter/env/ctx/PatchEnvironment.java index e82698d..9ca4cae 100644 --- a/core/src/main/java/org/sinytra/adapter/env/ctx/PatchEnvironment.java +++ b/core/src/main/java/org/sinytra/adapter/env/ctx/PatchEnvironment.java @@ -38,4 +38,19 @@ static PatchEnvironment create(RefmapHolder refmapHolder, ClassLookup cleanClass Collection getKnownNamespaces(); boolean isKnownPackage(String pkg); + + /** + * When enabled, mixin handlers that could not be adapted are removed from their class + * instead of being left in the jar. The mod then loads with those features disabled. + *

+ * This is the third option next to the existing two, both of which are dead ends: leaving the + * handler alone makes Mixin throw at runtime (the game crashes), and refusing the whole mod costs + * the user every working feature in it. + *

+ * Read from the {@code connector.stripFailingMixins} system property so both the standalone + * transformer and the in-game transformer honour it without extra plumbing. + */ + default boolean stripFailingMixins() { + return Boolean.getBoolean("connector.stripFailingMixins"); + } } diff --git a/core/src/main/java/org/sinytra/adapter/patch/DynamicPatches.java b/core/src/main/java/org/sinytra/adapter/patch/DynamicPatches.java index 343b08f..7a2f5c7 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/DynamicPatches.java +++ b/core/src/main/java/org/sinytra/adapter/patch/DynamicPatches.java @@ -23,9 +23,20 @@ public class DynamicPatches { public static Multimap methodTransformers(List patches) { return ImmutableMultimap.of( - TxPhase.EARLY, new LocalCaptureUpgradeTransformer(), TxPhase.LOADED, new FieldAccessorTypeTransformer(), - TxPhase.VALIDATED, new PipelineMethodTransformer(patches, false) + TxPhase.VALIDATED, new PipelineMethodTransformer(patches, false), + // Captured-locals ("locals = LocalCapture...") signatures have to be re-derived from the PATCHED + // method at the ADAPTED injection point, so this has to run after the pipeline: only then has + // preProcess/the pipeline finished rewriting "method" and "at". Running it earlier (it used to be + // in EARLY) meant it read the pre-adaptation target, found no injection point, and quietly left a + // stale capture list behind - which Mixin then aborted on at runtime ("Critical injection failure: + // LVT in ...", exactly what CAPTURE_FAILHARD is for). + // + // This ordering only became safe once Patcher#normalizeParameterAnnotations also re-derived the + // annotableParameterCount values; without that, adding the captured parameters here while the + // pipeline had already changed the descriptor threw ArrayIndexOutOfBoundsException inside ASM and + // failed the whole mod's transform. + TxPhase.VALIDATED, new LocalCaptureUpgradeTransformer() ); } } From 6c0914834bede9a7f28053b6ece0b34d5742cba7 Mon Sep 17 00:00:00 2001 From: Tong317 Date: Thu, 1 Oct 2026 18:23:33 +0800 Subject: [PATCH 08/16] Re-derive captured locals from the adapted injection point --- .../analysis/locals/LocalVarAnalyzer.java | 13 ++- .../adapter/patch/config/key/SpecialKeys.java | 6 ++ .../processor/TargetMethodProcessor.java | 5 + .../LocalCaptureUpgradeTransformer.java | 94 ++++++++++++++++++- 4 files changed, 113 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/sinytra/adapter/analysis/locals/LocalVarAnalyzer.java b/core/src/main/java/org/sinytra/adapter/analysis/locals/LocalVarAnalyzer.java index 63cec50..ed829c3 100644 --- a/core/src/main/java/org/sinytra/adapter/analysis/locals/LocalVarAnalyzer.java +++ b/core/src/main/java/org/sinytra/adapter/analysis/locals/LocalVarAnalyzer.java @@ -9,6 +9,7 @@ import org.objectweb.asm.tree.*; import org.sinytra.adapter.analysis.params.EnhancedParamsDiff; import org.sinytra.adapter.analysis.params.ParamsDiffSnapshot; +import org.sinytra.adapter.env.ann.AtData; import org.sinytra.adapter.env.ctx.LocalVariable; import org.sinytra.adapter.env.ctx.MixinContext; import org.sinytra.adapter.env.ctx.TargetPair; @@ -26,11 +27,21 @@ public record CapturedLocalsInfo(AdapterUtil.CapturedLocals capturedLocals, Para @Nullable public static CapturedLocalsInfo getCapturedLocals(MixinContext context, TargetPair dirtyTarget) { + return getCapturedLocals(context, dirtyTarget, null); + } + + /** + * @param atData the effective {@code @At} data, or {@code null} to fall back to the annotation as written. + * Adaptation may have moved the injection site, in which case the locals available there + * have to be read from the new one. + */ + @Nullable + public static CapturedLocalsInfo getCapturedLocals(MixinContext context, TargetPair dirtyTarget, @Nullable AtData atData) { AdapterUtil.CapturedLocals capturedLocals = AdapterUtil.getCapturedLocals(context, dirtyTarget); if (capturedLocals == null) return null; // Get available local variables at the injection point in the target method - List available = context.methods().getTargetMethodLocals(capturedLocals.target()); + List available = context.methods().getTargetMethodLocals(capturedLocals.target(), atData); if (available == null) return null; List availableTypes = available.stream().map(LocalVariable::type).toList(); diff --git a/core/src/main/java/org/sinytra/adapter/patch/config/key/SpecialKeys.java b/core/src/main/java/org/sinytra/adapter/patch/config/key/SpecialKeys.java index 9c7e1d5..1b1e320 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/config/key/SpecialKeys.java +++ b/core/src/main/java/org/sinytra/adapter/patch/config/key/SpecialKeys.java @@ -15,6 +15,12 @@ public final class SpecialKeys { public static final PropertyKey EXTRACT_ORIGIN_PARAM = PropertyKey.create("_extract_origin_param"); public static final PropertyKey> REDIRECT_ADAPTER = PropertyKey.create("_redirect_adapter"); public static final PropertyKey STATIC = PropertyKey.create("_static"); + /** + * Set by {@code LocalCaptureUpgradeTransformer} once it has re-derived a {@code locals = LocalCapture...} + * handler's capture list. {@code Patcher} uses it to tell an upgraded capture apart from one it could + * not verify, instead of inferring that from the audit trail. + */ + public static final PropertyKey LOCALS_UPGRADED = PropertyKey.create("_locals_upgraded"); private SpecialKeys() { } diff --git a/core/src/main/java/org/sinytra/adapter/patch/processor/TargetMethodProcessor.java b/core/src/main/java/org/sinytra/adapter/patch/processor/TargetMethodProcessor.java index 0391643..5bd50ad 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/processor/TargetMethodProcessor.java +++ b/core/src/main/java/org/sinytra/adapter/patch/processor/TargetMethodProcessor.java @@ -26,6 +26,11 @@ private static void upgradeCapturedLocals(MethodNode methodNode, MixinContext co AdapterUtil.CapturedLocals capturedLocals = AdapterUtil.getCapturedLocals(context, recipe); if (capturedLocals == null) return; + // NOTE: passing recipe.dirty().getAtData() here (to read the locals at the ADAPTED injection point) + // changes this processor from "no-op" to "rewrites the handler's parameter list" for far more + // mixins, which in testing broke unrelated mods (owo-lib failed with ClassNotFoundException while + // unused parameters were being removed mid-pipeline). Enable it together with the parameter + // annotation fix, not before. List availableLocals = context.methods().getTargetMethodLocals(capturedLocals.target()); // For now, only handle cases where all locals are part of the method's params, convenient when switching the target to a lambda if (availableLocals == null || !availableLocals.isEmpty()) return; diff --git a/core/src/main/java/org/sinytra/adapter/transform/preprocess/LocalCaptureUpgradeTransformer.java b/core/src/main/java/org/sinytra/adapter/transform/preprocess/LocalCaptureUpgradeTransformer.java index 0428493..e1d5749 100644 --- a/core/src/main/java/org/sinytra/adapter/transform/preprocess/LocalCaptureUpgradeTransformer.java +++ b/core/src/main/java/org/sinytra/adapter/transform/preprocess/LocalCaptureUpgradeTransformer.java @@ -12,11 +12,14 @@ import org.sinytra.adapter.env.ctx.TargetPair; import org.sinytra.adapter.env.util.MixinAnnotations; import org.sinytra.adapter.patch.config.Configuration; +import org.sinytra.adapter.patch.config.MutableConfiguration; import org.sinytra.adapter.patch.config.key.MixinKeys; +import org.sinytra.adapter.patch.config.key.SpecialKeys; import org.sinytra.adapter.transform.MethodTransformer; import org.sinytra.adapter.analysis.locals.LocalVarAnalyzer; import org.sinytra.adapter.env.ctx.PatchResult; import org.sinytra.adapter.util.AdapterUtil; +import org.sinytra.adapter.env.util.TypeConstants; import java.util.*; @@ -40,8 +43,33 @@ public PatchResult apply(MixinContext context, Configuration config) { List> localAnnotations = AdapterUtil.getAnnotatedParameters(methodNode, paramTypes, MixinAnnotations.LOCAL, Pair::of); if (!localAnnotations.isEmpty()) return PatchResult.PASS; - LocalVarAnalyzer.CapturedLocalsInfo info = LocalVarAnalyzer.getCapturedLocals(context, dirtyTarget); - if (info == null || info.diff().isEmpty()) return PatchResult.PASS; + // NOTE: the capture list is re-derived from the injection point adaptation actually produced - see + // MixinContext#effectiveAtData, published by PipelineMethodTransformer. Reading it from the annotation + // instead made this transformer silently leave stale capture lists behind for every handler the + // pipeline moved. + LocalVarAnalyzer.CapturedLocalsInfo info = LocalVarAnalyzer.getCapturedLocals(context, dirtyTarget, context.effectiveAtData()); + if (info == null) { + // NOTE: in-game (server-only runtime) this is reached with context.effectiveAtData() == null for + // handlers the pipeline handled, i.e. the injection point was never published. PipelineMethodTransformer + // has three exits and only two of them publish; the remaining one is + // "target method not found in the clean (vanilla) class" -> skip(...). That is where to look next. + // + // A handler whose CallbackInfo/CallbackInfoReturnable is its LAST parameter captures nothing, so + // there is no capture list that could have gone stale - AdapterUtil reports that as "unknown" too, + // because it only derives a position when something follows the callback. Treating it as unsafe + // would strip perfectly good handlers. Anything else really is undeterminable, and those are left + // unmarked so Patcher strips them rather than let Mixin abort at runtime. + if (capturesNoLocals(methodNode)) { + markUpgraded(config); + } + return PatchResult.PASS; + } + + if (info.diff().isEmpty()) { + // The compiled capture list still matches the patched method - nothing to re-derive. + markUpgraded(config); + return PatchResult.PASS; + } LocalVarAnalyzer.CapturedLocalsTransform transform = LocalVarAnalyzer.analyzeCapturedLocals(info.capturedLocals(), methodNode); @@ -61,8 +89,7 @@ public PatchResult apply(MixinContext context, Configuration config) { long matching = info.availableTypes().stream() .filter(expected::equals) .count(); - if (matching != 1) - return PatchResult.PASS; + if (matching != 1) return PatchResult.PASS; addOrdinal = false; } @@ -79,6 +106,7 @@ public PatchResult apply(MixinContext context, Configuration config) { } Type[] args = Type.getArgumentTypes(methodNode.desc); + ensureParameterAnnotationCapacity(methodNode, args.length); int start = info.capturedLocals().paramLocalStart(); for (int i = start; i < args.length; i++) { if (!usedOrdinals.contains(i)) continue; @@ -92,8 +120,66 @@ public PatchResult apply(MixinContext context, Configuration config) { PatchResult result = transform.remover().isEmpty() ? PatchResult.APPLY : transform.remover().apply(context); if (result == PatchResult.PASS) return PatchResult.PASS; + // Mark the capture as re-derived so Patcher knows this handler was handled. + markUpgraded(config); + context.recordCtxAudit("Upgrade captured locals"); context.environment().auditTrail().recordResult(context, config, AuditTrail.Match.FULL); return result; } + + /** + * ASM sizes the parameter-annotation arrays from the descriptor at the time it first writes them. This + * transformer runs after the pipeline, which may have added or removed handler parameters, so the arrays + * can be shorter than the current parameter count - writing an annotation then throws + * {@code ArrayIndexOutOfBoundsException} inside {@code AnnotationWriter}. Resize them (keeping whatever + * is already there) so annotations land on the right parameters. + */ + @SuppressWarnings("unchecked") + private static void ensureParameterAnnotationCapacity(MethodNode methodNode, int paramCount) { + methodNode.visibleParameterAnnotations = resizeParameterAnnotations(methodNode.visibleParameterAnnotations, paramCount); + methodNode.invisibleParameterAnnotations = resizeParameterAnnotations(methodNode.invisibleParameterAnnotations, paramCount); + } + + @SuppressWarnings("unchecked") + private static List[] resizeParameterAnnotations(List[] annotations, int paramCount) { + if (annotations == null || annotations.length == paramCount) { + return annotations; + } + List[] resized = (List[]) new List[paramCount]; + System.arraycopy(annotations, 0, resized, 0, Math.min(annotations.length, paramCount)); + return resized; + } + + /** + * Whether this injector declares no captured locals at all, i.e. its CallbackInfo/CallbackInfoReturnable + * is the last parameter. Such a handler needs no capture list, so there is nothing that can go stale. + */ + private static boolean capturesNoLocals(MethodNode methodNode) { + Type[] params = Type.getArgumentTypes(methodNode.desc); + for (int i = 0; i < params.length; i++) { + boolean isCallback = params[i].equals(TypeConstants.CI_TYPE) || params[i].equals(TypeConstants.CIR_TYPE); + if (isCallback) { + return i + 1 >= params.length; + } + } + return false; + } + + /** + * Records that this handler's captured-locals list is known to line up with the patched method - + * either because it already matched, or because it was just re-derived. {@code Patcher} strips the + * handlers that never get this mark, since for those the capture cannot be trusted. + */ + private static void markUpgraded(Configuration config) { + if (config instanceof MutableConfiguration mutable) { + mutable.setProperty(SpecialKeys.LOCALS_UPGRADED, true); + } + } } + + + + + + From ec5ab08f6c71d870e4a392373445fc0f05d5e983 Mon Sep 17 00:00:00 2001 From: Tong317 Date: Thu, 1 Oct 2026 18:23:33 +0800 Subject: [PATCH 09/16] Publish the effective injection point for later stages --- .../sinytra/adapter/env/ctx/MixinContext.java | 31 +++++++ .../transform/PipelineMethodTransformer.java | 92 ++++++++++++++++++- 2 files changed, 122 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/sinytra/adapter/env/ctx/MixinContext.java b/core/src/main/java/org/sinytra/adapter/env/ctx/MixinContext.java index b9c7f35..d85f8e0 100644 --- a/core/src/main/java/org/sinytra/adapter/env/ctx/MixinContext.java +++ b/core/src/main/java/org/sinytra/adapter/env/ctx/MixinContext.java @@ -5,6 +5,7 @@ import org.objectweb.asm.tree.ClassNode; import org.objectweb.asm.tree.MethodNode; import org.sinytra.adapter.analysis.selector.AnnotationHandle; +import org.sinytra.adapter.env.ann.AtData; import org.sinytra.adapter.env.ann.ClassTarget; import org.sinytra.adapter.patch.mixin.MixinFlag; import org.sinytra.adapter.patch.mixin.MixinType; @@ -12,6 +13,7 @@ import org.sinytra.adapter.patch.resolver.Resolvers; import org.sinytra.adapter.types.TypeAdapter; import org.sinytra.adapter.util.AdapterUtil; +import org.sinytra.adapter.util.MethodQualifier; import org.sinytra.adapter.util.provider.ClassLookup; import java.util.ArrayList; @@ -36,6 +38,35 @@ public class MixinContext implements RefMapper, Auditor { private final Resolvers resolvers = new Resolvers(); private final Processors processors = new Processors(); + /** + * Where the injection point ended up after adaptation, published by PipelineMethodTransformer. The + * adapted values only exist in its private dirty config, so later transformers (which need to read the + * locals live at that site) would otherwise still see the annotation as the mod wrote it. + */ + @Nullable + private AtData effectiveAtData; + @Nullable + private MethodQualifier effectiveTargetMethod; + + public void setEffectiveInjectionPoint(@Nullable AtData atData, @Nullable MethodQualifier targetMethod) { + this.effectiveAtData = atData; + this.effectiveTargetMethod = targetMethod; + } + + /** + * The {@code @At} data that actually applies to this handler, or {@code null} when adaptation did not run + * (in which case the annotation as written is still the truth). + */ + @Nullable + public AtData effectiveAtData() { + return this.effectiveAtData; + } + + @Nullable + public MethodQualifier effectiveTargetMethod() { + return this.effectiveTargetMethod; + } + public MixinContext(MixinType mixinType, PatchContext patchContext, ClassTarget classTarget, ClassNode classNode, MethodNode methodNode, AnnotationHandle methodAnnotation, AnnotationHandle injectionPointAnnotation, Set flags) { this.mixinType = mixinType; this.patchContext = patchContext; diff --git a/core/src/main/java/org/sinytra/adapter/transform/PipelineMethodTransformer.java b/core/src/main/java/org/sinytra/adapter/transform/PipelineMethodTransformer.java index 0835f55..f7e1598 100644 --- a/core/src/main/java/org/sinytra/adapter/transform/PipelineMethodTransformer.java +++ b/core/src/main/java/org/sinytra/adapter/transform/PipelineMethodTransformer.java @@ -47,7 +47,65 @@ public PipelineMethodTransformer(List methodPatches, boolean patche @Override public PatchResult apply(MixinContext context, Configuration config) { TargetPair cleanTarget = context.methods().findOwnMethodPair(context.cleanLookup(), config.getTargetMethod()); - if (cleanTarget == null) return skip(context, config, "target method not found in the clean (vanilla) class"); + if (cleanTarget == null) { + // The target class may exist only in the patched view: it can be supplied by the mod itself or by + // one of its libraries (io/github/axolotlclient/..., io/jsonwebtoken/..., de/.../classic4j/...), or + // it can be a platform class that the mod was never compiled against. Corpus-wide, + // "target method not found in the clean (vanilla) class" accounts for 763 of 922 skips and 91-94% + // of the sampled ones are the mod-own-class case. (12-issue草案.md entry 16, class A.) + // + // The two cases must NOT be treated alike: a mod-own class needs no adaptation (NeoForge never + // rewrites it), while a platform class usually DOES need adaptation and simply has no clean + // counterpart to compare against - passing those through unmapped produced a fatal, non-strippable + // InvalidInjectionException at runtime (create-fly's adapter_generated_CommonHooks targeting + // net/neoforged/neoforge/common/CommonHooks). + // + // Deciding by namespace would be wrong in both directions (it killed 18 legitimate mixins whose + // target is a platform class but which already resolve correctly). So decide by the only thing that + // actually matters: whether the injection point already resolves in the PATCHED class. If it does, + // there is genuinely nothing to adapt; if it does not, fall through and let the safety net strip the + // handler instead of shipping a mixin that aborts the game. + String targetClass = config.getProperty(ControlKeys.TARGET_CLASS).orElse(null); + TargetPair dirtyCandidate = context.methods().findOwnMethodPair(context.dirtyLookup(), config.getTargetMethod()); + + // Two very different situations hide behind "not found in the vanilla class": + // + // * the target class is the mod's own (or one of its libraries). NeoForge never rewrites those, so + // there is genuinely nothing to adapt and no clean counterpart to compare with. Pass them - this + // is the 91-94% case measured corpus-wide. + // + // * the target class belongs to the platform (net/minecraft, net/neoforged, ...). Those usually DO + // need adaptation, and passing them through unmapped produced a fatal, non-strippable + // InvalidInjectionException at runtime (create-fly's adapter_generated_CommonHooks targeting + // net/neoforged/neoforge/common/CommonHooks). For these, pass ONLY when the injection point already + // resolves in the patched class - which is exactly the question that matters. Deciding by + // namespace alone instead killed 18 legitimate mixins that target a platform class and already + // resolve correctly. + boolean modSupplied = !isPlatformClass(targetClass); + boolean platformButAlreadyResolvable = !modSupplied + && dirtyCandidate != null + && context.methods().hasInjectionTargetInsns(dirtyCandidate); + + if (targetClass != null + && context.dirtyLookup().getClass(targetClass).isPresent() + && context.cleanLookup().getClass(targetClass).isEmpty() + && (modSupplied || platformButAlreadyResolvable)) { + context.environment().auditTrail().recordResult(context, config, AuditTrail.Match.FULL); + context.setEffectiveInjectionPoint(config.getAtData(), config.getTargetMethod()); + try { + context.recordCtxAudit("No adaptation required: target class %s is not in the vanilla jar (%s)", + targetClass, modSupplied ? "supplied by the mod itself" : "injection point already resolves"); + } catch (RuntimeException ignored) { + // no audit frame at this exit + } + return PatchResult.PASS; + } + + // Publish the injection point here too: leaving it unset meant every later stage saw "unknown", + // which is indistinguishable from "no adaptation ran" and silently disabled capture re-derivation. + context.setEffectiveInjectionPoint(config.getAtData(), config.getTargetMethod()); + return skip(context, config, "target method not found in the clean (vanilla) class"); + } TargetPair dirtyTarget = context.methods().findOwnMethodPair(context.dirtyLookup(), config.getTargetMethod()); if (!this.patchResolver.matches(config) && !failsDirtyInjectionCheck(context, config, dirtyTarget) && hasValidSlice(context, config, dirtyTarget)) { @@ -55,6 +113,9 @@ public PatchResult apply(MixinContext context, Configuration config) { // into the patched target as-is, i.e. no adaptation is required. Record it as a success so it // is visible as "considered and fine" rather than never-considered. context.environment().auditTrail().recordResult(context, config, AuditTrail.Match.FULL); + // Still publish the injection point: it resolves here, and later transformers (captured-locals) + // need to know where it is even when nothing had to be rewritten. + context.setEffectiveInjectionPoint(config.getAtData(), config.getTargetMethod()); try { context.recordCtxAudit("No adaptation required: injection point already resolves in the patched target"); } catch (RuntimeException ignored) { @@ -174,6 +235,12 @@ private PatchResult execute(MixinContext context, Configuration config) { context.methodNode().parameters = null; } + // Publish where the injection point ended up. The adapted values only exist inside this method's + // dirty config, so anything that runs afterwards and needs the effective site (notably + // LocalCaptureUpgradeTransformer, which has to read the locals live at THAT point) has no other way + // to see them. + context.setEffectiveInjectionPoint(dirtyConfig.getAtData(), dirtyConfig.getTargetMethod()); + return PatchResult.APPLY; } @@ -219,4 +286,27 @@ private boolean validateAtNode(MixinContext context, AtData at, TargetPair targe ); return !insns.isEmpty(); } + + /** + * Whether the name belongs to a platform namespace (the game or the mod loader) rather than to a mod. + *

+ * Used to decide whether passing a handler through unmapped is safe. A mod-supplied target needs no + * adaptation at all; a platform target only qualifies when its injection point already resolves in the + * patched class (see the class-A branch in {@link #apply}). + */ + private static boolean isPlatformClass(String internalName) { + return internalName.startsWith("net/minecraft/") + || internalName.startsWith("net/neoforged/") + || internalName.startsWith("com/mojang/") + || internalName.startsWith("net/fabricmc/") + || internalName.startsWith("org/spongepowered/"); + } } + + + + + + + + From 8d16a7596ea0dd2cd13a68eb28732f4121de8963 Mon Sep 17 00:00:00 2001 From: Tong317 Date: Thu, 1 Oct 2026 18:23:33 +0800 Subject: [PATCH 10/16] Handle ModifyConstant targets that moved --- .../sinytra/adapter/env/ann/ConstantData.java | 8 +++++++- .../injection/InjectionPointResolver.java | 15 ++++++++++++++ .../target/SplitTargetMethodSubResolver.java | 20 +++++++++++++++++++ 3 files changed, 42 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/sinytra/adapter/env/ann/ConstantData.java b/core/src/main/java/org/sinytra/adapter/env/ann/ConstantData.java index 753f850..2754ded 100644 --- a/core/src/main/java/org/sinytra/adapter/env/ann/ConstantData.java +++ b/core/src/main/java/org/sinytra/adapter/env/ann/ConstantData.java @@ -13,7 +13,7 @@ public class ConstantData { private static final PropertyContainerTemplate TEMPLATE = PropertyContainerTemplate.builder() - .requireOne(Keys.DOUBLE_VALUE, Keys.CLASS_VALUE) + .requireOne(Keys.DOUBLE_VALUE, Keys.CLASS_VALUE, Keys.INT_VALUE, Keys.FLOAT_VALUE, Keys.LONG_VALUE, Keys.BOOLEAN_VALUE) .build(); private final PropertyContainer properties; @@ -49,5 +49,11 @@ public static Optional parse(AnnotationHandle handle) { public static class Keys { public static final PropertyKey DOUBLE_VALUE = PropertyKey.create("doubleValue", Double.class); public static final PropertyKey CLASS_VALUE = PropertyKey.create("classValue", Type.class); + public static final PropertyKey INT_VALUE = PropertyKey.create("intValue", Integer.class); + public static final PropertyKey FLOAT_VALUE = PropertyKey.create("floatValue", Float.class); + public static final PropertyKey LONG_VALUE = PropertyKey.create("longValue", Long.class); + public static final PropertyKey BOOLEAN_VALUE = PropertyKey.create("booleanValue", Boolean.class); } } + + diff --git a/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/InjectionPointResolver.java b/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/InjectionPointResolver.java index 5709f61..68833a4 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/InjectionPointResolver.java +++ b/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/InjectionPointResolver.java @@ -2,6 +2,7 @@ import org.jetbrains.annotations.Nullable; import org.objectweb.asm.Type; +import org.spongepowered.asm.mixin.injection.points.BeforeConstant; import org.objectweb.asm.tree.AbstractInsnNode; import org.sinytra.adapter.env.ctx.MixinContext; import org.sinytra.adapter.patch.Recipe; @@ -44,6 +45,19 @@ protected Configuration tryReuse(MixinContext context, Recipe recipe) { // Try reusing the original List insns = context.methods().findInjectionTargetInsns(dirtyTarget); + if (insns.isEmpty() && context.methodAnnotation().getNested("constant").isPresent()) { + // @ModifyConstant has no @At: its injection point IS the constant. The @At-based lookup above can + // therefore never match it, and the whole resolver reported "failed resolver InjectionPointResolver" + // even after the target method had been retargeted to where the constant moved. Ask the constant + // point instead. (Reference case: carpet's @ModifyConstant(intValue = 16) on Level#setBlock, whose + // code NeoForge moved into Level#markAndNotifyBlock.) + insns = context.methods().computeInjectionTargetInsns( + dirtyTarget, + () -> context.methodAnnotation().getNested("constant").orElse(null), + (ctx, h) -> new BeforeConstant(ctx, h.unwrap(), Type.getReturnType(context.methodNode().desc).getDescriptor()), + false + ); + } if (!insns.isEmpty()) { return dirty.copyClean() .inheritAtData(); @@ -52,3 +66,4 @@ protected Configuration tryReuse(MixinContext context, Recipe recipe) { return null; } } + diff --git a/core/src/main/java/org/sinytra/adapter/patch/resolver/target/SplitTargetMethodSubResolver.java b/core/src/main/java/org/sinytra/adapter/patch/resolver/target/SplitTargetMethodSubResolver.java index 08aabc1..7660df3 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/resolver/target/SplitTargetMethodSubResolver.java +++ b/core/src/main/java/org/sinytra/adapter/patch/resolver/target/SplitTargetMethodSubResolver.java @@ -1,6 +1,7 @@ package org.sinytra.adapter.patch.resolver.target; import com.google.common.collect.Multimap; +import org.objectweb.asm.Type; import org.objectweb.asm.tree.AbstractInsnNode; import org.objectweb.asm.tree.ClassNode; import org.objectweb.asm.tree.MethodInsnNode; @@ -16,6 +17,7 @@ import org.sinytra.adapter.analysis.method.MethodInsnMatcher; import org.sinytra.adapter.env.ctx.TargetPair; import org.sinytra.adapter.util.MethodQualifier; +import org.spongepowered.asm.mixin.injection.points.BeforeConstant; import java.util.List; import java.util.Map; @@ -129,6 +131,22 @@ private static List findInsnsCalls(List methods, Mi return methods.stream() .map(pair -> { List insns = context.methods().findInjectionTargetInsns(pair); + if (insns.isEmpty() && context.methodAnnotation().getNested("constant").isPresent()) { + // A constant injector (@ModifyConstant) has no @At - its injection point IS the constant - so + // the @At-based lookup above can never match it. Without asking the constant point instead, a + // method that was split away TOGETHER WITH ITS CONSTANT is never even considered a candidate, + // and the mixin fails with "failed resolver InjectionPointResolver" while the constant sits + // right there in the new method. + // + // Example: carpet's @ModifyConstant(intValue = 16, method = "setBlock(...)"). NeoForge moved + // that code (and the constant) into Level#markAndNotifyBlock. + insns = context.methods().computeInjectionTargetInsns( + pair, + () -> context.methodAnnotation().getNested("constant").orElse(null), + (ctx, h) -> new BeforeConstant(ctx, h.unwrap(), Type.getReturnType(context.methodNode().desc).getDescriptor()), + false + ); + } return !insns.isEmpty() ? new CandidateMethod(pair.methodNode(), insns) : null; }) .filter(Objects::nonNull) @@ -138,3 +156,5 @@ private static List findInsnsCalls(List methods, Mi private record CandidateMethod(MethodNode method, List insns) { } } + + From 4ea2ab01e8fd40b9f76aaae7bbb4dfa34e50f5c5 Mon Sep 17 00:00:00 2001 From: Tong317 Date: Thu, 1 Oct 2026 18:23:34 +0800 Subject: [PATCH 11/16] Resolve relocated members through moved methods --- .../sinytra/adapter/env/ctx/MethodFinder.java | 50 +++++- .../injection/InjectionPointResolver.java | 3 + .../injection/RelocatedMemberSubResolver.java | 163 ++++++++++++++++++ 3 files changed, 215 insertions(+), 1 deletion(-) create mode 100644 core/src/main/java/org/sinytra/adapter/patch/resolver/injection/RelocatedMemberSubResolver.java diff --git a/core/src/main/java/org/sinytra/adapter/env/ctx/MethodFinder.java b/core/src/main/java/org/sinytra/adapter/env/ctx/MethodFinder.java index caec2cc..8846d3e 100644 --- a/core/src/main/java/org/sinytra/adapter/env/ctx/MethodFinder.java +++ b/core/src/main/java/org/sinytra/adapter/env/ctx/MethodFinder.java @@ -99,7 +99,53 @@ public Pair> findMethods(ClassLookup lookup, MethodQ } if (candidates.isEmpty() && targetClass.superName != null) { - return findMethods(lookup, new MethodQualifier(Type.getObjectType(targetClass.superName).getDescriptor(), qualifier.name(), qualifier.desc()), flags); + // Returns unconditionally on purpose. + // + // Letting this fall through makes the interface traversal below reachable, and it can then hand + // back INTERFACE declarations as if they were owners. That used to abort server startup with + // "Found unexpected argument type ...LevelAccessor at index 0, expected ...Level". + // + // RedirectMixin no longer derives the @At owner from a lookup - it takes it from the actual call + // instruction inside the injected method (12-issue草案.md entry 8) - so re-enabling this is now + // SAFE, and it was tried on a real server: `Done`, no InvalidInjectionException. But it brought NO + // benefit either: carpet stayed at 301/0 offline and 8 stripped in-game, and + // onBlockBroken/onExplosionDone remained stripped. Reverted for that reason alone - no benefit, + // plus a history of breaking startup. See 12-issue草案.md entry 13. + return findMethods(lookup, + new MethodQualifier(Type.getObjectType(targetClass.superName).getDescriptor(), qualifier.name(), qualifier.desc()), flags); + } + + if (candidates.isEmpty()) { + // The walk above follows superName, which reaches superclasses only. A target can equally be + // declared as an interface DEFAULT method - that is precisely how NeoForge attaches helpers to + // the game's classes: BlockState#canStickTo / isStickyBlock are declared on + // net/neoforged/neoforge/common/extensions/IBlockStateExtension, and BlockState merely implements + // it. + // + // This only runs once the class and its whole superclass chain have come up empty. When more than + // one interface declares a match it declines, because picking one would be a guess. + // + // WARNING: do NOT make this reachable by falling through the superName branch above. It returns + // interface declarations as if they were injection owners; mixin then validates the handler + // against the @At string (which still names the class) and aborts server startup with + // InvalidInjectionException: Found unexpected argument type ...LevelAccessor at index 0, + // expected ...Level + // See 12-issue草案.md entry 13. + Pair> viaInterface = null; + for (String itf : targetClass.interfaces) { + Pair> found = findMethods(lookup, + new MethodQualifier(Type.getObjectType(itf).getDescriptor(), qualifier.name(), qualifier.desc()), flags); + if (found == null || found.getSecond().isEmpty()) { + continue; + } + if (viaInterface != null) { + return Pair.of(targetClass, List.of()); + } + viaInterface = found; + } + if (viaInterface != null) { + return viaInterface; + } } return Pair.of(targetClass, candidates); @@ -109,3 +155,5 @@ private static boolean hasFlag(int flags, int flag) { return (flags & flag) != 0; } } + + diff --git a/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/InjectionPointResolver.java b/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/InjectionPointResolver.java index 68833a4..730a3f4 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/InjectionPointResolver.java +++ b/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/InjectionPointResolver.java @@ -22,6 +22,9 @@ public InjectionPointResolver() { addSubResolver(InjectionPointSubResolvers.REPLACED_TYPE); addSubResolver(InjectionPointSubResolvers.EXTRACTED_CALL); addSubResolver(InjectionPointSubResolvers.EXTRACTED_CODEPATH_CALL); + // Last on purpose: this is the most structural guess of the lot (the member moved to a different + // class entirely), so it only gets a turn once every narrower strategy has declined. + addSubResolver(new RelocatedMemberSubResolver()); } @Override diff --git a/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/RelocatedMemberSubResolver.java b/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/RelocatedMemberSubResolver.java new file mode 100644 index 0000000..08479c6 --- /dev/null +++ b/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/RelocatedMemberSubResolver.java @@ -0,0 +1,163 @@ +package org.sinytra.adapter.patch.resolver.injection; + +import org.jetbrains.annotations.Nullable; +import org.objectweb.asm.Opcodes; +import org.objectweb.asm.Type; +import org.objectweb.asm.tree.AbstractInsnNode; +import org.objectweb.asm.tree.MethodInsnNode; +import org.objectweb.asm.tree.MethodNode; +import org.sinytra.adapter.env.ann.AtData; +import org.sinytra.adapter.env.ctx.MixinContext; +import org.sinytra.adapter.env.ctx.TargetPair; +import org.sinytra.adapter.patch.Recipe; +import org.sinytra.adapter.patch.config.Configuration; +import org.sinytra.adapter.patch.config.MutableConfiguration; +import org.sinytra.adapter.patch.resolver.SubResolver; +import org.sinytra.adapter.util.MethodQualifier; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +import static org.sinytra.adapter.env.util.MixinAnnotationConstants.AT_VAL_INVOKE; +import static org.sinytra.adapter.env.util.MixinAnnotationConstants.AT_VAL_INVOKE_ASSIGN; + +/** + * Handles injection points whose target member MOVED to a different owner. + *

+ * The reference case is NeoForge lifting a private static helper onto the class of its first parameter: + * + * 

+ *   vanilla      PistonStructureResolver.canStickToEachOther(BlockState, BlockState)Z   (private static)
+ *   NeoForge     BlockState.canStickTo(BlockState)Z                                     (instance)
+ *
+ * + * Both the owner and the name changed, so every owner/name based lookup - including + * {@link ChangedMemberDescriptorSubResolver}, which keeps owner + name and only ignores the descriptor - + * finds nothing, and the injection point is reported as unresolvable. + *

+ * The relocation survives in the SHAPE of the call: the receiver is dropped and the remaining parameters + * plus the return type stay identical. That is what this resolver matches on, deliberately without looking + * at the new name. + *

+ * Why it scans the target method instead of the receiving class. The natural approach - look for a + * method with the shifted signature on the first parameter's class - does not work here, and that is not an + * accident of this one case: NeoForge implements such helpers as {@code default} methods on its extension + * interfaces, so the member is NOT declared by the receiving class at all. {@code canStickTo} lives on + * {@code net/neoforged/neoforge/common/extensions/IBlockStateExtension} and {@code BlockState} merely + * implements it. (The same holds for the {@code isStickyBlock} half of this pair.) A scan of the class + * therefore finds nothing, and scanning interfaces instead would widen the search surface rather than + * sharpen it. + *

+ * Scanning the patched target method for the call itself is both simpler and better grounded: the call has to + * actually be there, it carries the real owner and descriptor, and the instruction it yields is exactly what + * the {@code @At} target has to be rewritten to. + *

+ * Accepted only when unambiguous - the old member must be gone and have been static, the first parameter type + * must be an object type, and exactly one call in the patched method must match the shifted shape. Anything + * else bails out and leaves the original failure in place: guessing would silently point an injection at an + * unrelated member, which is worse than an honest failure. + *

+ * Only {@code @At} INVOKE targets are considered. A mixin that injects INTO a method which was itself moved + * (carpet's {@code @Inject(at = @At("HEAD"), method = "isSticky")}) is a different mechanism. + */ +public class RelocatedMemberSubResolver implements SubResolver { + @Nullable + @Override + public Configuration resolve(MixinContext context, Recipe recipe) { + AtData at = recipe.clean().getAtData(); + if (at == null) { + return null; + } + if (!AT_VAL_INVOKE.equals(at.getValue()) && !AT_VAL_INVOKE_ASSIGN.equals(at.getValue())) { + return null; + } + + String targetDesc = at.getTarget().orElse(null); + if (targetDesc == null) { + return null; + } + + MethodQualifier original = MethodQualifier.parse(targetDesc).orElse(null); + if (original == null || original.owner() == null || original.name() == null || original.desc() == null) { + return null; + } + + TargetPair dirtyTarget = recipe.getDirtyTarget(); + if (dirtyTarget == null) { + return null; + } + + // The member has to be gone under its old owner+name. If it is still there, this resolver has nothing + // to contribute - ChangedMemberDescriptorSubResolver covers descriptor-only changes. + if (context.methods().findOwnMethodPair(context.dirtyLookup(), original.ignoreDesc()) != null) { + return null; + } + + // "Lifted onto the first parameter's class" only applies to a static helper. + TargetPair cleanOrigin = context.methods().findOwnMethodPair(context.cleanLookup(), original.ignoreDesc()); + if (cleanOrigin == null || !isStatic(cleanOrigin.methodNode())) { + return null; + } + + Type[] oldArgs = Type.getArgumentTypes(original.desc()); + if (oldArgs.length < 1) { + return null; + } + Type receiver = oldArgs[0]; + if (receiver.getSort() != Type.OBJECT) { + return null; + } + Type newReturn = Type.getReturnType(original.desc()); + Type[] newArgs = Arrays.copyOfRange(oldArgs, 1, oldArgs.length); + + List matches = findShiftedCalls(dirtyTarget.methodNode(), receiver, newArgs, newReturn); + // Several call sites to the SAME member are fine and expected - Mixin's @At without an ordinal + // addresses the member, not one particular occurrence (carpet's addBlockLine calls canStickTo twice). + // What has to be unambiguous is which MEMBER the call was relocated to, so compare the distinct + // owner+name+descriptor, not the instruction count. + if (matches.stream().map(call -> call.owner + call.name + call.desc).distinct().limit(2).count() != 1) { + return null; + } + + MethodInsnNode call = matches.getFirst(); + context.recordCtxAudit("Adjusting injection target for relocated member: %s -> %s.%s%s", + original.asDescriptor(), call.owner, call.name, call.desc); + + return MutableConfiguration.create() + .setAtData(at.withTarget(call)); + } + + /** + * Finds the calls in {@code target} that match the relocated shape: made on {@code receiver}, taking + * {@code newArgs} and returning {@code newReturn} - i.e. the old call with its receiver lifted out of the + * argument list. + */ + private static List findShiftedCalls(MethodNode target, Type receiver, Type[] newArgs, Type newReturn) { + List matches = new ArrayList<>(); + for (AbstractInsnNode insn = target.instructions.getFirst(); insn != null; insn = insn.getNext()) { + if (!(insn instanceof MethodInsnNode call)) { + continue; + } + int opcode = call.getOpcode(); + if (opcode != Opcodes.INVOKEVIRTUAL && opcode != Opcodes.INVOKEINTERFACE && opcode != Opcodes.INVOKESPECIAL) { + continue; + } + if (!call.owner.equals(receiver.getInternalName())) { + continue; + } + if (!Arrays.equals(Type.getArgumentTypes(call.desc), newArgs)) { + continue; + } + if (!Type.getReturnType(call.desc).equals(newReturn)) { + continue; + } + matches.add(call); + } + return matches; + } + + private static boolean isStatic(MethodNode methodNode) { + return (methodNode.access & Opcodes.ACC_STATIC) != 0; + } +} From 843f1ad649a87350eb672393d451404a42daef23 Mon Sep 17 00:00:00 2001 From: Tong317 Date: Thu, 1 Oct 2026 18:23:34 +0800 Subject: [PATCH 12/16] Keep the lambda owner when relocating into a lambda --- .../resolver/target/TargetMethodResolver.java | 5 ++ .../target/TargetMethodSubResolvers.java | 82 ++++++++++++++++++- 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/sinytra/adapter/patch/resolver/target/TargetMethodResolver.java b/core/src/main/java/org/sinytra/adapter/patch/resolver/target/TargetMethodResolver.java index ac2560b..0fd2de2 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/resolver/target/TargetMethodResolver.java +++ b/core/src/main/java/org/sinytra/adapter/patch/resolver/target/TargetMethodResolver.java @@ -42,3 +42,8 @@ protected Configuration useFallback(MixinContext context, Recipe recipe) { return null; } } + + + + + diff --git a/core/src/main/java/org/sinytra/adapter/patch/resolver/target/TargetMethodSubResolvers.java b/core/src/main/java/org/sinytra/adapter/patch/resolver/target/TargetMethodSubResolvers.java index ab331ad..b0160c7 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/resolver/target/TargetMethodSubResolvers.java +++ b/core/src/main/java/org/sinytra/adapter/patch/resolver/target/TargetMethodSubResolvers.java @@ -3,6 +3,8 @@ import com.mojang.datafixers.util.Pair; import org.jetbrains.annotations.Nullable; import org.objectweb.asm.Handle; +import org.objectweb.asm.Type; +import org.objectweb.asm.tree.AnnotationNode; import org.objectweb.asm.tree.AbstractInsnNode; import org.objectweb.asm.tree.ClassNode; import org.objectweb.asm.tree.InvokeDynamicInsnNode; @@ -36,6 +38,7 @@ public class TargetMethodSubResolvers { ClassNode targetClass = context.dirtyLookup().getClass(recipe.clean().getTargetClass()).orElse(null); if (targetClass == null) return null; + List candidateMethods = targetClass.methods.stream() .filter(m -> MethodAnalyzer.isLambda(m) && m.desc.equals(cleanQualifier.desc())) .toList(); @@ -79,6 +82,31 @@ public class TargetMethodSubResolvers { * Handle cases where the target instructions have been moved into a lambda inside the target method */ public static final SubResolver MOVED_INTO_LAMBDA = (MixinContext context, Recipe recipe) -> { + // Decline the relocation for handlers that use mixinextras' @Local sugar. + // + // Why @Local specifically: LocalSugarApplicator#validate resolves the sugar against the local variable + // frame AT THE INJECTION POINT (InjectorUtils#getOrCreateLocalContext -> CompatibilityHelper#makeLvtContext + // takes the target method plus the injection instruction). Relocating the target into a lambda re-points + // the injection point, so the sugar is re-resolved against the lambda's frame and local variable table + // instead of the original method's. Whether that still succeeds depends on the shape - a captured local + // becomes a lambda PARAMETER and can still match, especially with argsOnly = true - but when it does not, + // MixinExtras fails validation at runtime with + // Suppressed: SugarApplicationException: Failed to validate sugar @Local Item + // and SugarApplicationException extends MixinException (NOT InvalidInjectionException), so Mixin's count + // check escalates it to a fatal + // InjectionError: Critical injection failure ... (0/1) succeeded. Scanned 0 target(s) + // By then the adaptation is long over, so the handler can no longer be stripped and the whole server + // refuses to start. Without the relocation it fails cleanly and is stripped instead (feature missing, + // server up). Reference case: create-fly's ItemStackMixin#cacheState. + // + // The relocation itself is a supported feature (see DynamicMixinPatchTest#testChangeTargetToLambdaInPipeline), + // so this stays deliberately narrow - only the sugar whose fate depends on the injection point is gated. + // On the 0.3.1 baseline the sugar set is @Local + @Share, and ShareSugarApplicator#validate only checks + // that the parameter is a LocalRef of a known type; it never looks at the target method or the + // instruction. (@Cancellable arrives in later MixinExtras versions and its validate is empty.) So @Local + // is the only mixinextras sugar that can fail because of where the injection point ended up. + if (hasLocalSugar(context)) return null; + MethodQualifier cleanQualifier = recipe.clean().getTargetMethod(); TargetPair target = context.methods().findOwnMethodPair(context.dirtyLookup(), cleanQualifier); @@ -87,8 +115,16 @@ public class TargetMethodSubResolvers { for (AbstractInsnNode insn : target.methodNode().instructions) { // Find lambda invocations and search for target insns inside the lambda if (insn instanceof InvokeDynamicInsnNode indy && indy.bsmArgs.length > 1 && indy.bsmArgs[1] instanceof Handle handle) { + // Keep the handle's OWNER. Parsing only the name and letting findOwnMethodPair guess the + // declaring class made it resolve a same-named lambda belonging to a DIFFERENT class - the + // mixin's target class, which is where an owner-less lookup starts. The resulting target then + // had nothing to do with the @At target, so Mixin could never apply it and the whole server + // died at startup ("Critical injection failure"). Reference case: create-fly's + // ItemStackMixin#cacheState, whose @At targets Item#useOn but which was re-pointed at + // ItemStack's own lambda$useOn$0. See 12-issue草案.md entry 17. TargetPair lambda = MethodQualifier.parse(handle.getName()) - .map(q -> context.methods().findOwnMethodPair(context.dirtyLookup(), q)) + .map(q -> context.methods().findOwnMethodPair(context.dirtyLookup(), + new MethodQualifier(Type.getObjectType(handle.getOwner()).getDescriptor(), q.name(), q.desc()))) .orElse(null); if (lambda == null) return null; @@ -168,4 +204,48 @@ public static boolean isDirtyDeprecatedMethod(MixinContext context, MethodNode d TargetPair pair = context.methods().findOwnMethodPair(context.cleanLookup(), MethodQualifier.create(dirty)); return (pair == null || !AdapterUtil.isDeprecated(pair.methodNode())) && AdapterUtil.isDeprecated(dirty); } + + /** + * Whether any parameter of the handler is annotated with mixinextras' {@code @Local}. + *

+ * Used to keep {@link #MOVED_INTO_LAMBDA} away from handlers whose sugar has to be resolved against the + * injection point; see the call site for the full reasoning. This is deliberately narrower than "any + * mixinextras parameter annotation" - see the call site for why the other sugars cannot fail that way. + */ + private static boolean hasLocalSugar(MixinContext context) { + MethodNode method = context.methodNode(); + return hasLocalSugar(method.visibleParameterAnnotations) || hasLocalSugar(method.invisibleParameterAnnotations); + } + + private static boolean hasLocalSugar(java.util.List[] parameterAnnotations) { + if (parameterAnnotations == null) { + return false; + } + for (java.util.List annotations : parameterAnnotations) { + if (annotations == null) { + continue; + } + for (AnnotationNode annotation : annotations) { + if (isLocalSugar(annotation.desc)) { + return true; + } + } + } + return false; + } + + /** + * Match {@code com.llamalad7.mixinextras.sugar.Local} by its simple name rather than by its full internal + * name: MixinExtras supports being relocated into another package + * ({@code MixinExtrasService#getAllClassNames}/{@code changePackage}), so the prefix is not a stable part of + * the contract. Over-matching is the safe direction here - the only cost is that the relocation is declined + * and the handler fails cleanly instead of being relocated. + */ + private static boolean isLocalSugar(String desc) { + return desc != null && desc.endsWith("/sugar/Local;"); + } } + + + + From f00c6da1d412e9ba61c8d42f97d4971867e0f9aa Mon Sep 17 00:00:00 2001 From: Tong317 Date: Thu, 1 Oct 2026 18:23:34 +0800 Subject: [PATCH 13/16] Make the inheritance cache thread-safe --- .../adapter/analysis/InheritanceHandler.java | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/sinytra/adapter/analysis/InheritanceHandler.java b/core/src/main/java/org/sinytra/adapter/analysis/InheritanceHandler.java index 3d8c6bd..87b1780 100644 --- a/core/src/main/java/org/sinytra/adapter/analysis/InheritanceHandler.java +++ b/core/src/main/java/org/sinytra/adapter/analysis/InheritanceHandler.java @@ -3,11 +3,18 @@ import org.objectweb.asm.tree.ClassNode; import org.sinytra.adapter.util.provider.ClassLookup; +import java.util.concurrent.ConcurrentHashMap; import java.util.*; public class InheritanceHandler { private final ClassLookup classProvider; - private final Map> parentCache = new HashMap<>(); + // ConcurrentHashMap, not HashMap: ONE handler instance is shared by every mixin that uses the same class + // lookup (PatchEnvironmentImpl caches them with putIfAbsent), while ART drives the transformation from a + // ForkJoinPool. Concurrent put() on a plain HashMap corrupts its internal table, so get() could return + // null or a wrong entry - which made inheritance lookups (and therefore "does this target method exist") + // depend on thread interleaving. That is the non-determinism behind onBlockBroken/onExplosionDone + // sometimes being adapted and sometimes being stripped from the same build. See 12-issue草案.md entry 13. + private final Map> parentCache = new ConcurrentHashMap<>(); public InheritanceHandler(ClassLookup classProvider) { this.classProvider = classProvider; @@ -24,11 +31,14 @@ public boolean isClassInherited(String child, String parent) { public Collection getClassParents(String name) { Collection parents = this.parentCache.get(name); - if (parents == null) { - parents = computeClassParents(name); - this.parentCache.put(name, parents); + if (parents != null) { + return parents; } - return parents; + // get/compute/putIfAbsent rather than computeIfAbsent: computeClassParents recurses into + // getClassParents, which ConcurrentHashMap.computeIfAbsent rejects as a recursive update. + Collection computed = computeClassParents(name); + Collection prev = this.parentCache.putIfAbsent(name, computed); + return prev != null ? prev : computed; } private Collection computeClassParents(String name) { @@ -51,3 +61,4 @@ private Collection computeClassParents(String name) { return parents; } } + From 3b8a2db5e251eb407bfccb7fb8dcae2c8326d5c9 Mon Sep 17 00:00:00 2001 From: Tong317 Date: Thu, 1 Oct 2026 21:37:22 +0800 Subject: [PATCH 14/16] Sync the callback modifier when a member moves onto a static target Mixin refuses a non-static callback that targets a static method, and the error is fatal. The member can end up on a static method after being moved into a generated class, so sync the modifier - but only when the handler does not read the receiver. As an instance method the body reserved local 0 for the receiver, so its parameters sat at 1..n; once the receiver is gone every slot reference shifts down by one. Slot numbers are fixed at compile time and do not depend on branches, exception handlers or variable lifetimes, so the uniform shift is correct - the one shape it is not safe for is a body that reads local 0. A body that reads local 0 cannot become static at all: a @Shadow member access compiles to exactly "aload 0 + getfield/invokevirtual", and Candidates#canMove deliberately lets that shape through. Those handlers are now reported as unadapted before anything is moved, so Patcher#stripFailedMixins drops them instead of emitting a class that no longer verifies. Dropping the receiver also invalidates the method's StackMapTable: an explicit full_frame still describes local 0 and every slot after it, while frame deltas relative to the descriptor happen to survive. The transform now returns COMPUTE_FRAMES so Connector re-derives them. Without the shift Mixin sees a body whose locals no longer line up and the game dies with "VerifyError: Bad local variable type" (locals[3] = top); without the frames the same happens for any handler whose encoding contains a full_frame ("Stack map does not match the one at exception handler N"). --- .../extract/ExtractMixinProcessor.java | 82 ++++++++++++++++++- 1 file changed, 81 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/sinytra/adapter/patch/processor/extract/ExtractMixinProcessor.java b/core/src/main/java/org/sinytra/adapter/patch/processor/extract/ExtractMixinProcessor.java index f8f831f..f8fbf94 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/processor/extract/ExtractMixinProcessor.java +++ b/core/src/main/java/org/sinytra/adapter/patch/processor/extract/ExtractMixinProcessor.java @@ -6,8 +6,10 @@ import org.objectweb.asm.Opcodes; import org.objectweb.asm.Type; import org.objectweb.asm.tree.*; +import org.sinytra.adapter.env.ctx.MethodHelper; import org.sinytra.adapter.env.ctx.MixinContext; import org.sinytra.adapter.env.ctx.TargetPair; +import org.sinytra.adapter.util.MethodQualifier; import org.sinytra.adapter.env.util.MixinAnnotations; import org.sinytra.adapter.patch.Recipe; import org.sinytra.adapter.patch.TxResult; @@ -60,6 +62,30 @@ public static PatchResult apply(String targetClassName, MixinContext context, Re Candidates candidates = findCandidates(classNode, methodNode); if (!candidates.canMove(classNode, isInherited)) return PatchResult.PASS; + // The member may have moved to a STATIC method on the new target - a Fabric method mapped onto a + // NeoForge static hook. Mixin rejects that combination outright and the error is fatal: + // + // InvalidInjectionException: non-static callback method ... targets a static method which is not + // supported + // + // StaticAccessProcessor cannot catch this: it runs before extraction, when the callback still matches + // its original target. So the modifier has to be synced here - but it has to be DECIDED here, before + // anything is moved: a body that reads local 0 cannot be made static at all, and bailing out after the + // move would leave the member in both the original and the generated class. + MethodQualifier movedTarget = recipe.dirty().getTargetMethod(); + TargetPair movedPair = movedTarget == null ? null + : context.methods().findOwnMethodPair(context.dirtyLookup(), movedTarget); + boolean syncModifier = movedPair != null && MethodHelper.isStatic(movedPair.methodNode()) && !MethodHelper.isStatic(methodNode); + if (syncModifier && readsReceiverSlot(methodNode)) { + // A @Shadow field or method access compiles to "aload 0 + getfield/invokevirtual", and exactly that + // shape is allowed through Candidates#canMove (a shadowed member does not count as "accessing the + // class instance"). A static target has no receiver, so there is nothing local 0 could hold. + // Report the mixin as unadapted instead: Patcher#stripFailedMixins then drops the handler and the + // mod loads with the feature disabled, which is strictly better than emitting a class that no + // longer passes verification. + return PatchResult.PASS; + } + ClassNode targetClass = context.environment().dirtyClassLookup().getClass(targetClassName).orElse(null); // Get or generate new mixin class ClassNode generatedTarget = context.environment().classGenerator().getOrGenerateMixinClass(classNode, targetClassName, targetClass != null ? targetClass.superName : null); @@ -76,8 +102,39 @@ public static PatchResult apply(String targetClassName, MixinContext context, Re candidates.handleUpdates().forEach(c -> c.accept(generatedTarget)); + if (syncModifier) { + context.recordCtxAudit("Made extracted callback static to match static target %s", movedTarget.asDescriptor()); + methodNode.access |= Opcodes.ACC_STATIC; + + // The callback keeps its original body, and as an instance method that body reserved local 0 for the + // receiver - its parameters sat at 1..n. Now that the receiver is gone everything shifts down by one, + // exactly as StaticAccessProcessor does in the opposite direction ("Remove static"). Slot numbers are + // fixed at compile time and do not depend on branches, exception handlers or variable lifetimes, so + // the uniform shift is correct; the one shape it is NOT safe for - a body that reads local 0 - was + // rejected before anything was moved. Without the shift Mixin sees a body whose locals no longer line + // up and the game dies with "VerifyError: Bad local variable type" (locals[3] = top). + if (methodNode.localVariables != null) { + for (LocalVariableNode lvn : methodNode.localVariables) { + lvn.index--; + } + methodNode.localVariables.removeIf(lvn -> lvn.index < 0); + } + for (AbstractInsnNode insn : methodNode.instructions) { + if (insn instanceof VarInsnNode varInsn) { + varInsn.var--; + } else if (insn instanceof IincInsnNode iincInsn) { + iincInsn.var--; + } + } + methodNode.maxLocals = Math.max(0, methodNode.maxLocals - 1); + } + // Take care of captured locals - PatchResult result = PatchResult.PASS; + // Dropping the receiver also invalidates the method's StackMapTable: a full_frame still describes local 0 + // and every slot after it. Connector only recomputes frames when the result is COMPUTE_FRAMES, so ask + // for them here - the uniform shift above is exactly the kind of change that silently leaves stale + // frames behind (measured: delta frames survive, an explicit full_frame fails verification). + PatchResult result = syncModifier ? PatchResult.COMPUTE_FRAMES : PatchResult.PASS; boolean capturesLocals = recipe.clean().hasProperty(MixinKeys.LOCALS); if (capturesLocals) { result = result.or(recreateLocalVariables(methodNode, context, recipe, generatedTarget)); @@ -187,6 +244,27 @@ private static int fixAccess(int access) { return access; } + /** + * Whether the body reads local 0, which is the receiver of an instance method. + *

+ * javac never assigns to slot 0 of an instance method, so any reference at all means the body uses + * {@code this}. A {@code @Shadow} field or method access compiles to exactly that shape ({@code aload 0} + * followed by {@code getfield}/{@code invokevirtual}), and a lambda that captured {@code this} is reached + * through the same {@code aload 0}. Checking only the handler (not the moved lambdas) is sufficient for the + * same reason: a lambda that needs the receiver forces the handler to load it. + */ + private static boolean readsReceiverSlot(MethodNode methodNode) { + for (AbstractInsnNode insn : methodNode.instructions) { + if (insn instanceof VarInsnNode varInsn && varInsn.var == 0) { + return true; + } + if (insn instanceof IincInsnNode iincInsn && iincInsn.var == 0) { + return true; + } + } + return false; + } + /** * We can't bring captured locals along when extracting a mixin, so our best bet is to recreate them from scratch. *

@@ -376,3 +454,5 @@ private static PatchResult recreateLocalVariables(MethodNode methodNode, MixinCo return PatchResult.COMPUTE_FRAMES; } } + + From b21b37312353d8bfd167da90cf5cd88ddff6c32b Mon Sep 17 00:00:00 2001 From: Tong317 Date: Thu, 1 Oct 2026 21:53:38 +0800 Subject: [PATCH 15/16] Cover repeated annotation parsing in the 26.1.2 suite --- .../mixin/DynamicMixinPatchTest.java | 16 +++++++++++ .../mixin/RepeatedAnnotationsMixin.java | 27 +++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 test/mc-26.1.2/src/testClasses/java/org/sinytra/adapter/test/mc_26_1_2/mixin/RepeatedAnnotationsMixin.java diff --git a/test/mc-26.1.2/src/test/java/org/sinytra/adapter/patch/test/mc_26_1_2/mixin/DynamicMixinPatchTest.java b/test/mc-26.1.2/src/test/java/org/sinytra/adapter/patch/test/mc_26_1_2/mixin/DynamicMixinPatchTest.java index 421188e..466800e 100644 --- a/test/mc-26.1.2/src/test/java/org/sinytra/adapter/patch/test/mc_26_1_2/mixin/DynamicMixinPatchTest.java +++ b/test/mc-26.1.2/src/test/java/org/sinytra/adapter/patch/test/mc_26_1_2/mixin/DynamicMixinPatchTest.java @@ -15,6 +15,8 @@ import org.slf4j.Logger; import org.spongepowered.asm.mixin.FabricUtil; +import static org.junit.jupiter.api.Assertions.assertNotNull; + import java.util.List; public class DynamicMixinPatchTest extends MinecraftMixinPatchTest { @@ -74,6 +76,19 @@ void testModifyArgChangedTargetDesc() throws Exception { ); } + @Test + void testRepeatedAnnotationsAreParsed() throws Exception { + // ASM stores a repeated annotation as a List. TARGET_CONSTANT used to dereference the null result of + // parseSingle and throw a NullPointerException inside MixinParser, aborting the transform for the whole + // mod instead of skipping the property. Verified by removing that null guard again: this test fails. + LoadResult result = load( + "org/sinytra/adapter/test/mc_26_1_2/mixin/RepeatedAnnotationsMixin", + List.of("repeatedAnnotations") + ); + + assertNotNull(result.patched(), "the mixin should be parsed instead of aborting the transform"); + } + @Override protected LoadResult load(String className, List allowedMethods) throws Exception { ClassNode patched = loadClass(className); @@ -82,3 +97,4 @@ protected LoadResult load(String className, List allowedMethods) throws return new LoadResult(patchEnvironment, patched, loadClass(className)); } } + diff --git a/test/mc-26.1.2/src/testClasses/java/org/sinytra/adapter/test/mc_26_1_2/mixin/RepeatedAnnotationsMixin.java b/test/mc-26.1.2/src/testClasses/java/org/sinytra/adapter/test/mc_26_1_2/mixin/RepeatedAnnotationsMixin.java new file mode 100644 index 0000000..8d8d58c --- /dev/null +++ b/test/mc-26.1.2/src/testClasses/java/org/sinytra/adapter/test/mc_26_1_2/mixin/RepeatedAnnotationsMixin.java @@ -0,0 +1,27 @@ +package org.sinytra.adapter.test.mc_26_1_2.mixin; + +import net.minecraft.world.entity.LivingEntity; +import org.spongepowered.asm.mixin.Mixin; +import org.spongepowered.asm.mixin.injection.At; +import org.spongepowered.asm.mixin.injection.Constant; +import org.spongepowered.asm.mixin.injection.ModifyConstant; +import org.spongepowered.asm.mixin.injection.Slice; + +/** + * ASM turns a repeated annotation into a List, and a parser that assumes a single annotation then either + * dereferences null or casts the list straight to AnnotationNode - either way aborting the transform for the + * whole mod instead of skipping the property. TARGET_CONSTANT had exactly that bug: the repeated {@code constant} + * below used to reach {@code new AnnotationHandle(null)} and throw a NullPointerException inside MixinParser + * (reference case: shuttfup's DefaultLANPort). This fixture is the real-world shape that has to be parsed. + */ +@Mixin(LivingEntity.class) +public class RepeatedAnnotationsMixin { + @ModifyConstant( + method = "getMaxAirSupply", + constant = {@Constant(intValue = 1), @Constant(intValue = 2)}, + slice = {@Slice(from = @At("HEAD")), @Slice(to = @At("RETURN"))} + ) + private static int repeatedAnnotations(int original) { + return original; + } +} From 745f07a5b3641a3d1af2144b265f6b79b9c32425 Mon Sep 17 00:00:00 2001 From: Tong317 Date: Thu, 8 Oct 2026 20:09:36 +0800 Subject: [PATCH 16/16] Shorten comments Most of them restated the code or recorded how the change was found. Keep the reason where it is not visible in the code itself, and drop the rest. No code changes: the diff is comment lines only. --- .../adapter/analysis/InheritanceHandler.java | 11 +--- .../analysis/locals/LocalVarAnalyzer.java | 5 +- .../sinytra/adapter/env/ann/ClassTarget.java | 7 +- .../sinytra/adapter/env/ctx/MethodFinder.java | 32 ++------- .../sinytra/adapter/env/ctx/MethodHelper.java | 15 +---- .../sinytra/adapter/env/ctx/MixinContext.java | 7 +- .../adapter/env/ctx/PatchEnvironment.java | 12 +--- .../adapter/env/ctx/PatchEnvironmentImpl.java | 7 +- .../sinytra/adapter/patch/DynamicPatches.java | 14 +--- .../sinytra/adapter/patch/MixinParser.java | 13 ++-- .../org/sinytra/adapter/patch/Patcher.java | 60 ++++------------- .../adapter/patch/config/key/MixinKeys.java | 14 ++-- .../adapter/patch/config/key/SpecialKeys.java | 5 +- .../adapter/patch/mixin/AccessorMixin.java | 18 +---- .../adapter/patch/mixin/InvokerMixin.java | 20 ++---- .../adapter/patch/mixin/MixinType.java | 23 ++----- .../adapter/patch/mixin/MixinTypes.java | 7 +- .../adapter/patch/mixin/ModifyArgsMixin.java | 12 +--- .../patch/mixin/ModifyConstantMixin.java | 34 ++-------- .../adapter/patch/mixin/OverwriteMixin.java | 21 ++---- .../adapter/patch/mixin/RedirectMixin.java | 39 +++-------- .../processor/TargetMethodProcessor.java | 7 +- .../extract/ExtractMixinProcessor.java | 44 +++---------- .../ChangedMemberDescriptorSubResolver.java | 14 +--- .../injection/InjectionPointResolver.java | 10 +-- .../injection/RelocatedMemberSubResolver.java | 55 +++------------- .../target/SplitTargetMethodSubResolver.java | 12 ++-- .../target/TargetMethodSubResolvers.java | 50 ++------------ .../transform/PipelineMethodTransformer.java | 66 +++---------------- .../LocalCaptureUpgradeTransformer.java | 36 ++-------- .../mixin/DynamicMixinPatchTest.java | 5 +- .../mixin/RepeatedAnnotationsMixin.java | 6 +- 32 files changed, 137 insertions(+), 544 deletions(-) diff --git a/core/src/main/java/org/sinytra/adapter/analysis/InheritanceHandler.java b/core/src/main/java/org/sinytra/adapter/analysis/InheritanceHandler.java index 87b1780..2adf4fe 100644 --- a/core/src/main/java/org/sinytra/adapter/analysis/InheritanceHandler.java +++ b/core/src/main/java/org/sinytra/adapter/analysis/InheritanceHandler.java @@ -8,12 +8,8 @@ public class InheritanceHandler { private final ClassLookup classProvider; - // ConcurrentHashMap, not HashMap: ONE handler instance is shared by every mixin that uses the same class - // lookup (PatchEnvironmentImpl caches them with putIfAbsent), while ART drives the transformation from a - // ForkJoinPool. Concurrent put() on a plain HashMap corrupts its internal table, so get() could return - // null or a wrong entry - which made inheritance lookups (and therefore "does this target method exist") - // depend on thread interleaving. That is the non-determinism behind onBlockBroken/onExplosionDone - // sometimes being adapted and sometimes being stripped from the same build. See 12-issue草案.md entry 13. + // ConcurrentHashMap: one handler instance is shared by every mixin using the same class lookup, and the + // transformation is driven from a ForkJoinPool. private final Map> parentCache = new ConcurrentHashMap<>(); public InheritanceHandler(ClassLookup classProvider) { @@ -34,8 +30,7 @@ public Collection getClassParents(String name) { if (parents != null) { return parents; } - // get/compute/putIfAbsent rather than computeIfAbsent: computeClassParents recurses into - // getClassParents, which ConcurrentHashMap.computeIfAbsent rejects as a recursive update. + // Not computeIfAbsent: computeClassParents recurses into getClassParents, which it rejects. Collection computed = computeClassParents(name); Collection prev = this.parentCache.putIfAbsent(name, computed); return prev != null ? prev : computed; diff --git a/core/src/main/java/org/sinytra/adapter/analysis/locals/LocalVarAnalyzer.java b/core/src/main/java/org/sinytra/adapter/analysis/locals/LocalVarAnalyzer.java index ed829c3..b9b6801 100644 --- a/core/src/main/java/org/sinytra/adapter/analysis/locals/LocalVarAnalyzer.java +++ b/core/src/main/java/org/sinytra/adapter/analysis/locals/LocalVarAnalyzer.java @@ -31,9 +31,8 @@ public static CapturedLocalsInfo getCapturedLocals(MixinContext context, TargetP } /** - * @param atData the effective {@code @At} data, or {@code null} to fall back to the annotation as written. - * Adaptation may have moved the injection site, in which case the locals available there - * have to be read from the new one. + * @param atData the effective {@code @At} data, or {@code null} to fall back to the annotation as written + * (adaptation may have moved the injection site) */ @Nullable public static CapturedLocalsInfo getCapturedLocals(MixinContext context, TargetPair dirtyTarget, @Nullable AtData atData) { diff --git a/core/src/main/java/org/sinytra/adapter/env/ann/ClassTarget.java b/core/src/main/java/org/sinytra/adapter/env/ann/ClassTarget.java index c1ec79c..bdfbffc 100644 --- a/core/src/main/java/org/sinytra/adapter/env/ann/ClassTarget.java +++ b/core/src/main/java/org/sinytra/adapter/env/ann/ClassTarget.java @@ -44,11 +44,8 @@ public static ClassTarget parse(AnnotationHandle handle, RefMapper mapper) { .or(() -> handle.>getValue(MIXIN_TARGETS).map(v -> { List types = v.get().stream() .map(mapper::remap) - // "targets" holds dotted binary names (net.minecraft.Foo$Bar), while Type.getObjectType - // expects an internal name (net/minecraft/Foo$Bar). Passing the dotted form through - // produced a Type whose internal name kept the dots, so every subsequent class lookup - // missed and the mixin was reported as "target method not found". Note that the '$' of - // an inner class must be preserved - only '.' becomes '/'. + // "targets" holds dotted binary names while Type.getObjectType expects an internal name; + // only '.' becomes '/', the '$' of an inner class stays. .map(name -> name.replace('.', '/')) .map(Type::getObjectType) .toList(); diff --git a/core/src/main/java/org/sinytra/adapter/env/ctx/MethodFinder.java b/core/src/main/java/org/sinytra/adapter/env/ctx/MethodFinder.java index 8846d3e..bfc74f7 100644 --- a/core/src/main/java/org/sinytra/adapter/env/ctx/MethodFinder.java +++ b/core/src/main/java/org/sinytra/adapter/env/ctx/MethodFinder.java @@ -99,38 +99,16 @@ public Pair> findMethods(ClassLookup lookup, MethodQ } if (candidates.isEmpty() && targetClass.superName != null) { - // Returns unconditionally on purpose. - // - // Letting this fall through makes the interface traversal below reachable, and it can then hand - // back INTERFACE declarations as if they were owners. That used to abort server startup with - // "Found unexpected argument type ...LevelAccessor at index 0, expected ...Level". - // - // RedirectMixin no longer derives the @At owner from a lookup - it takes it from the actual call - // instruction inside the injected method (12-issue草案.md entry 8) - so re-enabling this is now - // SAFE, and it was tried on a real server: `Done`, no InvalidInjectionException. But it brought NO - // benefit either: carpet stayed at 301/0 offline and 8 stripped in-game, and - // onBlockBroken/onExplosionDone remained stripped. Reverted for that reason alone - no benefit, - // plus a history of breaking startup. See 12-issue草案.md entry 13. + // Returns unconditionally: falling through would make the interface traversal below reachable, + // and interface declarations returned as owners break startup. return findMethods(lookup, new MethodQualifier(Type.getObjectType(targetClass.superName).getDescriptor(), qualifier.name(), qualifier.desc()), flags); } if (candidates.isEmpty()) { - // The walk above follows superName, which reaches superclasses only. A target can equally be - // declared as an interface DEFAULT method - that is precisely how NeoForge attaches helpers to - // the game's classes: BlockState#canStickTo / isStickyBlock are declared on - // net/neoforged/neoforge/common/extensions/IBlockStateExtension, and BlockState merely implements - // it. - // - // This only runs once the class and its whole superclass chain have come up empty. When more than - // one interface declares a match it declines, because picking one would be a guess. - // - // WARNING: do NOT make this reachable by falling through the superName branch above. It returns - // interface declarations as if they were injection owners; mixin then validates the handler - // against the @At string (which still names the class) and aborts server startup with - // InvalidInjectionException: Found unexpected argument type ...LevelAccessor at index 0, - // expected ...Level - // See 12-issue草案.md entry 13. + // superName reaches superclasses only; a target can equally be a default method on an interface, + // which is how NeoForge attaches helpers (BlockState#canStickTo is declared on + // IBlockStateExtension). Declines when more than one interface matches. Pair> viaInterface = null; for (String itf : targetClass.interfaces) { Pair> found = findMethods(lookup, diff --git a/core/src/main/java/org/sinytra/adapter/env/ctx/MethodHelper.java b/core/src/main/java/org/sinytra/adapter/env/ctx/MethodHelper.java index 06ac18f..ec0011a 100644 --- a/core/src/main/java/org/sinytra/adapter/env/ctx/MethodHelper.java +++ b/core/src/main/java/org/sinytra/adapter/env/ctx/MethodHelper.java @@ -247,15 +247,7 @@ public List getTargetMethodLocals(TargetPair target, int startPos return getTargetMethodLocals(target, findInjectionTargetInsns(target), startPos, lvtCompatLevel); } - /** - * Like {@link #findInjectionTargetInsns(TargetPair)}, but locates the injection point from the given - * {@code @At} data rather than from the annotation as originally written. - *

- * Adaptation can rewrite {@code @At}, after which the raw annotation points at an injection site that - * no longer exists. Anything derived from that site - notably the locals captured by a - * {@code locals = LocalCapture...} handler - would then be computed from the wrong place, or not at - * all. - */ + /** Like {@link #findInjectionTargetInsns(TargetPair)}, but from the given {@code @At} data. */ public List findInjectionTargetInsns(@Nullable TargetPair target, @Nullable AtData atData) { if (target == null) return List.of(); if (atData == null) return findInjectionTargetInsns(target); @@ -274,10 +266,7 @@ public List findInjectionTargetInsns(@Nullable TargetPair targ ); } - /** - * Variant of {@link #getTargetMethodLocals(TargetPair)} that uses the effective {@code @At} data. - * See {@link #findInjectionTargetInsns(TargetPair, AtData)}. - */ + /** Variant of {@link #getTargetMethodLocals(TargetPair)} using the effective {@code @At} data. */ @Nullable public List getTargetMethodLocals(TargetPair target, @Nullable AtData atData) { Type[] targetParams = Type.getArgumentTypes(target.methodNode().desc); diff --git a/core/src/main/java/org/sinytra/adapter/env/ctx/MixinContext.java b/core/src/main/java/org/sinytra/adapter/env/ctx/MixinContext.java index d85f8e0..327e56a 100644 --- a/core/src/main/java/org/sinytra/adapter/env/ctx/MixinContext.java +++ b/core/src/main/java/org/sinytra/adapter/env/ctx/MixinContext.java @@ -39,9 +39,7 @@ public class MixinContext implements RefMapper, Auditor { private final Processors processors = new Processors(); /** - * Where the injection point ended up after adaptation, published by PipelineMethodTransformer. The - * adapted values only exist in its private dirty config, so later transformers (which need to read the - * locals live at that site) would otherwise still see the annotation as the mod wrote it. + * Where the injection point ended up after adaptation, published by PipelineMethodTransformer. */ @Nullable private AtData effectiveAtData; @@ -54,8 +52,7 @@ public void setEffectiveInjectionPoint(@Nullable AtData atData, @Nullable Method } /** - * The {@code @At} data that actually applies to this handler, or {@code null} when adaptation did not run - * (in which case the annotation as written is still the truth). + * The {@code @At} data that applies to this handler, or {@code null} when adaptation did not run. */ @Nullable public AtData effectiveAtData() { diff --git a/core/src/main/java/org/sinytra/adapter/env/ctx/PatchEnvironment.java b/core/src/main/java/org/sinytra/adapter/env/ctx/PatchEnvironment.java index 9ca4cae..3623c20 100644 --- a/core/src/main/java/org/sinytra/adapter/env/ctx/PatchEnvironment.java +++ b/core/src/main/java/org/sinytra/adapter/env/ctx/PatchEnvironment.java @@ -40,15 +40,9 @@ static PatchEnvironment create(RefmapHolder refmapHolder, ClassLookup cleanClass boolean isKnownPackage(String pkg); /** - * When enabled, mixin handlers that could not be adapted are removed from their class - * instead of being left in the jar. The mod then loads with those features disabled. - *

- * This is the third option next to the existing two, both of which are dead ends: leaving the - * handler alone makes Mixin throw at runtime (the game crashes), and refusing the whole mod costs - * the user every working feature in it. - *

- * Read from the {@code connector.stripFailingMixins} system property so both the standalone - * transformer and the in-game transformer honour it without extra plumbing. + * When enabled, mixin handlers that could not be adapted are removed from their class rather than left in + * the jar, so the mod loads with those features disabled. Read from the + * {@code connector.stripFailingMixins} system property. */ default boolean stripFailingMixins() { return Boolean.getBoolean("connector.stripFailingMixins"); diff --git a/core/src/main/java/org/sinytra/adapter/env/ctx/PatchEnvironmentImpl.java b/core/src/main/java/org/sinytra/adapter/env/ctx/PatchEnvironmentImpl.java index 4d883fb..d9d6569 100644 --- a/core/src/main/java/org/sinytra/adapter/env/ctx/PatchEnvironmentImpl.java +++ b/core/src/main/java/org/sinytra/adapter/env/ctx/PatchEnvironmentImpl.java @@ -21,9 +21,7 @@ public final class PatchEnvironmentImpl implements PatchEnvironment { private final AuditTrail auditTrail; private final @Nullable Collection pkgNamespaces; - // The transformer processes classes in parallel (ForkJoinPool via AsyncHelper), so this cache is - // touched from multiple threads. A plain HashMap.computeIfAbsent throws ConcurrentModificationException - // under concurrent modification (and also when the mapping function re-enters this method). + // Classes are processed in parallel, so the cache is shared across threads. private final Map inheritanceHandlers = new ConcurrentHashMap<>(); public PatchEnvironmentImpl( @@ -80,8 +78,7 @@ public MixinClassGenerator classGenerator() { @Override public InheritanceHandler inheritanceHandler(ClassLookup lookup) { - // get-then-putIfAbsent instead of computeIfAbsent: the handler constructor can re-enter this - // method, which ConcurrentHashMap.computeIfAbsent would reject as a recursive update. + // Not computeIfAbsent: the handler constructor can re-enter this method. InheritanceHandler existing = this.inheritanceHandlers.get(lookup); if (existing != null) { return existing; diff --git a/core/src/main/java/org/sinytra/adapter/patch/DynamicPatches.java b/core/src/main/java/org/sinytra/adapter/patch/DynamicPatches.java index 7a2f5c7..863f402 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/DynamicPatches.java +++ b/core/src/main/java/org/sinytra/adapter/patch/DynamicPatches.java @@ -25,17 +25,9 @@ public static Multimap methodTransformers(List target.matches(MethodQualifier.create(m))) .toList(); - // Fall back to the superclass chain. An injector may name a method the mixin's target class only - // INHERITS - carve's @Redirect(method = "causeExtraKnockback") on @Mixin(Player.class), where the - // method is declared on LivingEntity, is the reference case. Mixin itself resolves the target - // through the hierarchy, so mirroring that here is what lets the descriptor be completed (and the - // mixin stop being reported as an unexplained failure). + // Fall back to the superclass chain: an injector may name a method its target class only + // inherits (carve's @Redirect on Player, declared on LivingEntity), and Mixin resolves the target + // through the hierarchy too. for (ClassNode cls = dirtyTarget; targets.isEmpty() && cls.superName != null; ) { ClassNode parent = environment.dirtyClassLookup().getClass(cls.superName).orElse(null); if (parent == null) break; diff --git a/core/src/main/java/org/sinytra/adapter/patch/Patcher.java b/core/src/main/java/org/sinytra/adapter/patch/Patcher.java index e5d8e9e..01d4c79 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/Patcher.java +++ b/core/src/main/java/org/sinytra/adapter/patch/Patcher.java @@ -81,9 +81,8 @@ public PatchResult process(ClassNode classNode) { MixinGroups groups = MixinGroups.create(mixinClass.mixins().stream().map(MixinMethodHandle::methodNode).toList()); this.environment.auditTrail().processGroups(classNode, groups); - // Must be the very last thing: context.run() applies deferred changes that can still add parameter - // annotations, and ASM sizes their arrays from the descriptor as it stood when the array was first - // created, so they can end up shorter than the descriptor. + // Must run last: context.run() can still add parameter annotations, and ASM sizes their arrays from + // the descriptor as it stood when the array was created, so they can end up shorter. if (normalizeParameterAnnotations(classNode)) { result = result.or(PatchResult.COMPUTE_FRAMES); } @@ -91,16 +90,10 @@ public PatchResult process(ClassNode classNode) { return result; } - /** - * See PipelineMethodTransformer#skip: every "give up quietly" exit must record the mixin, - * otherwise it disappears from the audit trail and the jar is reported as compatible. - */ + /** See PipelineMethodTransformer#skip: every give-up exit must record the mixin, or the jar is reported as compatible. */ private static PatchResult skip(MixinContext context, org.sinytra.adapter.patch.config.Configuration config, String reason) { context.environment().auditTrail().recordResult(context, config, org.sinytra.adapter.env.ctx.AuditTrail.Match.NONE); - // The early exits in #processMixin run OUTSIDE any pushAudit/popAudit frame (only the EARLY/LOADED/ - // VALIDATED transformer loops open one), so recordCtxAudit used to throw here and the reason was - // swallowed - the mixin showed up in the failure list with no explanation at all, which is very - // expensive to diagnose. Open a frame so the reason is always reported. + // These early exits run outside any pushAudit/popAudit frame, so open one to report the reason. context.pushAudit(SKIP_REASON_FRAME); try { context.recordCtxAudit("Skipped mixin: %s", reason); @@ -123,9 +116,7 @@ public String toString() { private PatchResult processMixin(ClassNode classNode, ClassTarget classTarget, PatchContext patchContext, MixinParser.MixinMethodHandle mixin, List failed) { MixinType handlerType = mixin.mixinType(); - // Types that do not target a method (@Accessor exposes a field) have no TARGET_METHOD, so the - // early return below used to drop them silently: no adaptation, no audit entry, nothing. - // Dispatch them to their own validation instead, which records the result itself. + // Types without a method target (@Accessor) carry no TARGET_METHOD; send them to their own validation. if (!handlerType.targetsMethod()) { MixinContext nonMethodContext = new MixinContext(handlerType, patchContext, classTarget, classNode, mixin.methodNode(), mixin.methodAnnotation(), null, handlerType.getFlags()); @@ -200,20 +191,16 @@ private PatchResult processMixin(ClassNode classNode, ClassTarget classTarget, P result = result.or(txResult); } - // A "locals = LocalCapture..." handler's capture list is compiled against the injection site as it - // was when the mod was built, and Mixin aborts at runtime when that list no longer lines up with the - // patched method ("Critical injection failure: LVT in ..." - CAPTURE_FAILHARD exists to do exactly - // that). LocalCaptureUpgradeTransformer marks the config once it has confirmed or re-derived the - // list; a handler without that mark cannot be trusted, so treat it as failed. Note this cannot go - // through the audit trail: Match.or() is a maximum, so a later success would overwrite a NONE. + // A "locals = LocalCapture..." list is compiled against the injection site as it was when the mod was + // built, and Mixin aborts at runtime when it no longer lines up. A handler without the mark set by + // LocalCaptureUpgradeTransformer is failed via the failure list, as Match.or() would overwrite a NONE. if (configuration.hasProperty(MixinKeys.LOCALS) && !configuration.hasProperty(SpecialKeys.LOCALS_UPGRADED)) { LOGGER.debug(MIXINPATCH, "Mixin {} captures locals that could not be verified; it would abort at runtime", mixinId); failed.add(mixin); } - // A resolver failure inside the pipeline records Match.NONE rather than throwing, so the audit - // trail is the only place that knows this handler did not make it. + // A resolver failure records Match.NONE rather than throwing, so the audit trail is the only witness. if (this.environment.auditTrail().getMatch(mixinContext) == AuditTrail.Match.NONE) { failed.add(mixin); } @@ -221,13 +208,7 @@ private PatchResult processMixin(ClassNode classNode, ClassTarget classTarget, P return result; } - /** - * Removes handlers that could not be adapted, so Mixin never registers them and the mod loads with - * those features disabled instead of crashing at runtime. - *

- * A handler is only removed when nothing else in the class calls it - dropping a referenced method - * would produce a broken class, which is worse than the failure we are working around. - */ + /** Removes handlers that could not be adapted so Mixin never registers them; only when nothing calls them. */ private PatchResult stripFailedMixins(ClassNode classNode, List failed) { boolean removedAny = false; for (MixinParser.MixinMethodHandle mixin : failed) { @@ -248,25 +229,8 @@ private PatchResult stripFailedMixins(ClassNode classNode, List - * ASM tracks two separate things: the annotation arrays themselves, and - * {@code visible/invisibleAnnotableParameterCount} - the {@code num_parameters} value of the - * RuntimeVisible/InvisibleParameterAnnotations attributes, which {@code MethodWriter} copies straight - * into its own arrays and then walks in - * {@code AnnotationWriter.computeParameterAnnotationsSize(String, AnnotationWriter[], int)}: - * 

-     *   for (int i = 0; i < annotableParameterCount; ++i) {
-     *     AnnotationWriter annotationWriter = annotationWriters[i];   // <- IndexOutOfBounds here
-     *
- * Transformers here add and remove handler parameters, so a count captured for the old descriptor - * outlives the array that matched it and the write blows up with - * {@code ArrayIndexOutOfBoundsException: Index N out of bounds for length N}. Because the failure - * happens while writing - outside this method - it takes the whole mod's transform down. - *

- * Resizing the arrays alone is not enough; the counts have to be re-derived too. - * - * @return whether anything changed + * Keeps a method's parameter-annotation arrays and counts in sync with its descriptor; a stale count + * aborts the whole mod's transform inside ASM's {@code MethodWriter}. */ private static boolean normalizeParameterAnnotations(ClassNode classNode) { boolean changed = false; diff --git a/core/src/main/java/org/sinytra/adapter/patch/config/key/MixinKeys.java b/core/src/main/java/org/sinytra/adapter/patch/config/key/MixinKeys.java index 5778fac..496c8db 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/config/key/MixinKeys.java +++ b/core/src/main/java/org/sinytra/adapter/patch/config/key/MixinKeys.java @@ -43,10 +43,8 @@ public class MixinKeys { .build(); public static final PropertyKey TARGET_CONSTANT = PropertyKey.builder("constant") .parser((value, mapper) -> { - // Same null guard as TARGET_AT above. Without it, a @ModifyConstant whose "constant" is not a single - // annotation (a repeated/array form, or a plain int) reached new AnnotationHandle(null) and blew up - // with a NullPointerException inside MixinParser - which aborted the whole transform for that mod - // instead of simply skipping the property. Reference case: shuttfup's DefaultLANPort. + // Same null guard as TARGET_AT above: "constant" is not always a single annotation, and the null + // would reach new AnnotationHandle(null) inside MixinParser and abort the whole transform. AnnotationNode node = ControlKeys.parseSingle(value, AnnotationNode.class); if (node == null) return null; @@ -63,12 +61,8 @@ public class MixinKeys { public static final PropertyKey ORDINAL = PropertyKey.create("ordinal", Integer.class); public static final PropertyKey ARGS_ONLY = PropertyKey.create("argsOnly", Boolean.class); public static final PropertyKey SLICE = PropertyKey.builder("slice") - // ASM stores a repeated annotation as a List, so the bare cast (AnnotationNode) value threw - // ClassCastException: ArrayList cannot be cast to AnnotationNode, aborting the transform for the whole mod. - // parseSingle unwraps the single-element case and yields null otherwise ("no slice"; the multi-element - // form is handled by SLICES above). A/B verified: this does NOT change carpet's outcome - the - // onBlockBroken/onExplosionDone strip count stayed identical with and without this change. - // Reference case: cookeymod (CRASH -> 61/7). + // A repeated annotation arrives as a List, so the bare cast to AnnotationNode throws; parseSingle + // unwraps the single element and yields null otherwise (the multi-element form is handled by SLICES). .parser((value, mapper) -> { AnnotationNode node = ControlKeys.parseSingle(value, AnnotationNode.class); return node == null ? null : SliceData.parse(new AnnotationHandle(node), mapper); diff --git a/core/src/main/java/org/sinytra/adapter/patch/config/key/SpecialKeys.java b/core/src/main/java/org/sinytra/adapter/patch/config/key/SpecialKeys.java index 1b1e320..0a75234 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/config/key/SpecialKeys.java +++ b/core/src/main/java/org/sinytra/adapter/patch/config/key/SpecialKeys.java @@ -16,9 +16,8 @@ public final class SpecialKeys { public static final PropertyKey> REDIRECT_ADAPTER = PropertyKey.create("_redirect_adapter"); public static final PropertyKey STATIC = PropertyKey.create("_static"); /** - * Set by {@code LocalCaptureUpgradeTransformer} once it has re-derived a {@code locals = LocalCapture...} - * handler's capture list. {@code Patcher} uses it to tell an upgraded capture apart from one it could - * not verify, instead of inferring that from the audit trail. + * Set by {@code LocalCaptureUpgradeTransformer} once it has re-derived a capture list, so {@code Patcher} + * can tell an upgraded capture apart from one it could not verify. */ public static final PropertyKey LOCALS_UPGRADED = PropertyKey.create("_locals_upgraded"); diff --git a/core/src/main/java/org/sinytra/adapter/patch/mixin/AccessorMixin.java b/core/src/main/java/org/sinytra/adapter/patch/mixin/AccessorMixin.java index 6d1aadb..caf0f59 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/mixin/AccessorMixin.java +++ b/core/src/main/java/org/sinytra/adapter/patch/mixin/AccessorMixin.java @@ -14,17 +14,8 @@ import org.sinytra.adapter.patch.resolver.Resolvers; /** - * Handles {@code @Accessor} mixins. - *

- * Accessors are the one handler that does not target a method at all: they expose a field of - * the target class. The usual pipeline is method-first - {@code PipelineMethodTransformer.apply} - * resolves a target method before doing anything else and records a failure when it cannot - so - * simply registering this type would have marked every accessor as unadapted - * ({@code @Accessor} appears in 280 classes across 31 mods of the reference corpus) and made - * Connector's mixin safeguard reject all of them. - *

- * That is why this type opts out with {@link #targetsMethod()} and validates itself instead: the - * field is looked up in the patched target class and only a genuinely missing field is reported. + * Handles {@code @Accessor} mixins, the one handler that does not target a method at all. It opts out with + * {@link #targetsMethod()} because the method-first pipeline would mark every accessor as unadapted. */ public class AccessorMixin implements MixinType { private static final PropertyContainerTemplate TEMPLATE = PropertyContainerTemplate.builder() @@ -69,10 +60,7 @@ public void validateNonMethodTarget(MixinContext context, Configuration config) found ? AuditTrail.Match.FULL : AuditTrail.Match.NONE); } - /** - * Mixin infers the field for {@code @Accessor} from the method name when no value is given: - * {@code getFoo}/{@code setFoo}/{@code isFoo} access the field {@code foo}. - */ + /** Mixin infers the field from the method name when no value is given: {@code getFoo} accesses {@code foo}. */ private static String inferFieldName(String methodName) { String base; if (methodName.startsWith("get") || methodName.startsWith("set")) { diff --git a/core/src/main/java/org/sinytra/adapter/patch/mixin/InvokerMixin.java b/core/src/main/java/org/sinytra/adapter/patch/mixin/InvokerMixin.java index da8951d..8232cd0 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/mixin/InvokerMixin.java +++ b/core/src/main/java/org/sinytra/adapter/patch/mixin/InvokerMixin.java @@ -18,18 +18,9 @@ import org.sinytra.adapter.util.MethodQualifier; /** - * Handles {@code @Invoker} mixins. - *

- * Like {@code @Accessor}, an invoker is an interface mixin: Mixin generates a - * method that calls the target, it does not inject into an existing method body. Running the - * method-target injection pipeline on it therefore always finds no injection point and reports a - * failure, which is why this type opts out with {@link #targetsMethod()} and validates itself - * instead - the target method named by {@code value} (or {@code callXxx} -> {@code xxx}) is looked - * up in the patched target class, and only a genuinely missing method is reported. - *

- * Registering the type matters for the same reason as the others: before this, {@code MixinParser} - * skipped invokers entirely, so an invoker whose target had moved stayed in the jar and failed at - * runtime. {@code @Invoker} appears in 23 of the 46 mods of the reference corpus. + * Handles {@code @Invoker} mixins. Like {@code @Accessor} an invoker is an interface mixin - Mixin generates a + * method that calls the target rather than injecting into a body - so the injection pipeline would always + * report a failure. It opts out with {@link #targetsMethod()} and looks the named method up instead. */ public class InvokerMixin implements MixinType { private static final PropertyContainerTemplate TEMPLATE = PropertyContainerTemplate.builder() @@ -72,10 +63,7 @@ public void validateNonMethodTarget(MixinContext context, Configuration config) found ? AuditTrail.Match.FULL : AuditTrail.Match.NONE); } - /** - * The invoker names its target with {@code value}; without one Mixin falls back to the handler's - * own name with a {@code call}/{@code invoke}/{@code invoker} prefix removed. - */ + /** The invoker names its target with {@code value}; without one Mixin strips a call/invoke prefix. */ @Nullable @Override public MethodQualifier deriveTarget(PropertyContainer properties, MethodNode method) { diff --git a/core/src/main/java/org/sinytra/adapter/patch/mixin/MixinType.java b/core/src/main/java/org/sinytra/adapter/patch/mixin/MixinType.java index 66c26db..e2fea30 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/mixin/MixinType.java +++ b/core/src/main/java/org/sinytra/adapter/patch/mixin/MixinType.java @@ -31,14 +31,8 @@ default boolean canInject(MixinContext context, Configuration config) { } /** - * Types whose target is not named by a {@code method} property derive it from the mixin method - * itself: {@code @Overwrite} replaces the method of the same name, while {@code @Invoker} calls - * the method named by its {@code value} (falling back to {@code callXxx} -> {@code xxx}). - *

- * Parsers use this to fill in the target qualifier before the usual resolution runs. - * - * @return the qualifier to use as {@code TARGET_METHOD}, or {@code null} when this type does not - * derive one + * Derives the target qualifier from the mixin method for types whose target is not named by a + * {@code method} property, e.g. {@code @Overwrite} and {@code @Invoker}. */ @Nullable default MethodQualifier deriveTarget(PropertyContainer properties, MethodNode method) { @@ -46,21 +40,14 @@ default MethodQualifier deriveTarget(PropertyContainer properties, MethodNode me } /** - * Whether this handler targets a method (and therefore belongs in the method-target pipeline). - *

- * {@code @Accessor} returns {@code false}: it exposes a field, so the method-first pipeline would - * report every accessor as unadapted. Such types instead implement - * {@link #validateNonMethodTarget(MixinContext, Configuration)} and are dispatched there. + * Whether this handler targets a method and therefore belongs in the method-target pipeline. + * {@code @Accessor} returns false. */ default boolean targetsMethod() { return true; } - /** - * Validation for types that opted out of the method pipeline via {@link #targetsMethod()}. - * Implementations must record an audit result themselves - the pipeline does not run for them. - * The default does nothing, so no result is recorded and the mixin stays invisible. - */ + /** Validation for types that opted out of the method pipeline; they must record their own audit result. */ default void validateNonMethodTarget(MixinContext context, Configuration config) { } diff --git a/core/src/main/java/org/sinytra/adapter/patch/mixin/MixinTypes.java b/core/src/main/java/org/sinytra/adapter/patch/mixin/MixinTypes.java index b6801ef..ec90ff8 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/mixin/MixinTypes.java +++ b/core/src/main/java/org/sinytra/adapter/patch/mixin/MixinTypes.java @@ -14,9 +14,6 @@ public class MixinTypes { private static final Map MIXIN_TYPES = new HashMap<>(); // TODO: - // MixinConstants.WRAP_WITH_CONDITION - // Measured 2026-09: @WrapWithCondition appears in 0 of the 46 mods of the reference corpus - // (it is a MixinExtras annotation and rare in practice), so it is deliberately left out. public static final MixinType INJECT = new InjectMixin(); public static final MixinType MODIFY_VAR = new ModifyVariableMixin(); public static final MixinType MODIFY_ARG = new ModifyArgMixin(); @@ -34,8 +31,8 @@ public class MixinTypes { registerMixinType(Inject.class, INJECT); registerMixinType(ModifyVariable.class, MODIFY_VAR); registerMixinType(ModifyArg.class, MODIFY_ARG); - // NOTE: this map is keyed by annotation *internal names* (MixinParser passes - // Type.getType(desc).getInternalName()), so the "L...;" descriptor form would never match. + // MixinConstants.WRAP_WITH_CONDITION is deliberately left out; it appears in none of the corpus mods. + // The map is keyed by annotation internal names, so the "L...;" descriptor form would never match. registerMixinType(MixinAnnotations.MODIFY_ARGS_INTERNAL_NAME, MODIFY_ARGS); registerMixinType(MixinAnnotations.MODIFY_CONST_INTERNAL_NAME, MODIFY_CONST); registerMixinType(MixinAnnotations.MODIFY_RETURN_VAL_INTERNAL_NAME, MODIFY_RET); diff --git a/core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyArgsMixin.java b/core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyArgsMixin.java index cd1b5f2..8f7c6c7 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyArgsMixin.java +++ b/core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyArgsMixin.java @@ -19,13 +19,8 @@ import static org.sinytra.adapter.env.param.MethodParameters.ParamGroup.SINGLE_ANY; /** - * Handles {@code @ModifyArgs} mixins. - *

- * The handler signature is fixed - {@code void handler(Args args)} - no matter what the injection - * point looks like, so unlike {@link ModifyArgMixin} there is no argument type to re-derive. The - * work here is registering the type at all: previously {@code MixinParser} skipped it, so a - * {@code @ModifyArgs} mixin that could not be injected was neither adapted nor recorded and only - * blew up at runtime. + * Handles {@code @ModifyArgs} mixins. The handler signature is fixed - {@code void handler(Args args)} - + * whatever the injection point looks like, so there is no argument type to re-derive. */ public class ModifyArgsMixin implements MixinType { private static final PropertyContainerTemplate TEMPLATE = ConfigurationTemplates.MIXIN_AT.extend() @@ -51,8 +46,7 @@ public TxResult preProcess(MixinContext context, MutableConfiguration clean, Res public TxResult postProcess(MixinContext context, Configuration clean, MutableConfiguration dirty, Recipe recipe) { dirty.inheritProperyIfAbsent(MixinKeys.ORDINAL); - // The handler always takes the argument list and returns void, so its signature never has to - // follow the (possibly re-targeted) injection point. + // The handler always takes the argument list and returns void. dirty.inheritParameters(); dirty.inheritReturnType(); diff --git a/core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyConstantMixin.java b/core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyConstantMixin.java index 401b771..767c6b8 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyConstantMixin.java +++ b/core/src/main/java/org/sinytra/adapter/patch/mixin/ModifyConstantMixin.java @@ -22,23 +22,9 @@ import static org.sinytra.adapter.env.param.MethodParameters.ParamGroup.SINGLE_ANY; /** - * Handles {@code @ModifyConstant} mixins. - *

- * Unlike the other handlers, {@code @ModifyConstant} does not carry an {@code @At}: the injection - * point is derived from the {@code @Constant} annotation itself. The pipeline already knows how to - * locate such constants ({@link PipelineMethodTransformer#computeConstantTargetInsns}), but until - * this type existed the annotation was not registered in {@link MixinTypes}, so - * {@code MixinParser} skipped the method outright: it was never resolved, never validated and never - * recorded. A handler whose target constant had moved therefore stayed in the transformed jar and - * blew up at runtime with {@code InjectionError: ... failed injection check, (0/1) succeeded}. - *

- * Registering the type fixes two things at once: - *

    - *
  • mixins whose constant still resolves take the pipeline's normal path, and
    • - *
  • mixins whose constant no longer exists fail in {@link #postProcess} - which both drops - * them from the jar and, crucially, records the failure so Connector's mixin safeguard can - * refuse the mod instead of letting the game crash later.
    • - *
+ * Handles {@code @ModifyConstant} mixins, which carry no {@code @At}: the injection point comes from the + * {@code @Constant} annotation. Handlers whose constant no longer resolves fail in {@link #postProcess}, so + * they are dropped and reported rather than left in the jar to abort at runtime. */ public class ModifyConstantMixin implements MixinType { private static final PropertyContainerTemplate TEMPLATE = ConfigurationTemplates.MIXIN_BASE.extend() @@ -53,7 +39,6 @@ public PropertyContainerTemplate getConfigurationTemplate() { @Override public TxResult preProcess(MixinContext context, MutableConfiguration clean, Resolvers resolvers, Processors processors) { - // The handler receives the constant it replaces and returns the replacement value. clean.setParameters(MethodParameters.create(context.methodNode(), List.of(SINGLE_ANY))); return TxResult.SUCCESS; @@ -63,19 +48,15 @@ public TxResult preProcess(MixinContext context, MutableConfiguration clean, Res public TxResult postProcess(MixinContext context, Configuration clean, MutableConfiguration dirty, Recipe recipe) { dirty.inheritProperyIfAbsent(MixinKeys.ORDINAL); - // The template declares TARGET_CONSTANT, but nothing ever put a value in it: PropertyProcessor only - // forwards keys that are ALREADY set, it does not read them off the annotation. A dirty config with - // "constant" missing therefore failed validation ("invalid DIRTY config") and the mixin was dropped -- - // even after its target had been successfully retargeted to the method the constant moved into. + // PropertyProcessor only forwards keys that are already set, so TARGET_CONSTANT has to be read off the + // annotation here; without it the dirty config fails validation and the mixin is dropped. if (!dirty.hasProperty(MixinKeys.TARGET_CONSTANT)) { context.methodAnnotation().getNested("constant") .flatMap(ConstantData::parse) .ifPresent(constant -> dirty.setProperty(MixinKeys.TARGET_CONSTANT, constant)); } - // MIXIN_BASE.require() mandates PARAMETERS and RETURN_TYPE, and every other mixin type populates both - // here. This one did not, so its dirty config never validated ("invalid DIRTY config") and the handler - // was dropped - even once its target had been retargeted and its constant resolved. + // MIXIN_BASE.require() mandates PARAMETERS and RETURN_TYPE, which every mixin type populates here. dirty.setParameters(MethodParameters.create(context.methodNode(), List.of(SINGLE_ANY))); dirty.setReturnType(Type.getReturnType(context.methodNode().desc)); @@ -88,8 +69,7 @@ public TxResult postProcess(MixinContext context, Configuration clean, MutableCo return TxResult.FAIL; } - // Ask the same machinery the pipeline uses, so that "can this constant be found?" is answered - // exactly the way Mixin will answer it at runtime. + // Use the pipeline's own machinery so the answer matches what Mixin does at runtime. List constantInsns = context.methods().computeInjectionTargetInsns( dirtyTarget, () -> context.methodAnnotation().getNested("constant").orElse(null), diff --git a/core/src/main/java/org/sinytra/adapter/patch/mixin/OverwriteMixin.java b/core/src/main/java/org/sinytra/adapter/patch/mixin/OverwriteMixin.java index 4df5100..03c77e9 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/mixin/OverwriteMixin.java +++ b/core/src/main/java/org/sinytra/adapter/patch/mixin/OverwriteMixin.java @@ -21,23 +21,12 @@ import static org.sinytra.adapter.env.param.MethodParameters.ParamGroup.METHOD_PARAMS; /** - * Handles {@code @Overwrite} mixins. - *

- * {@code @Overwrite} is the one handler that carries no target information at all: it replaces the - * target method wholesale, and the target is identified by the handler's own name. That is - * why it needs {@link MixinType#derivesTargetFromMixinMethod()} and a template that does not require - * {@code method} - see {@code MixinParser}, which fills in the qualifier (name only) so the usual - * "resolve by name inside the target class" step can complete it. - *

- * Registering the type is what makes an unadaptable overwrite visible: previously - * {@code MixinParser} skipped it entirely, so a handler whose target had moved stayed in the jar - * and failed only at runtime. + * Handles {@code @Overwrite} mixins, the one handler that carries no target information at all: it replaces + * the target method wholesale and is identified by the handler's own name, which {@link MixinType#deriveTarget} + * supplies (name only) so the usual name resolution can complete it. */ public class OverwriteMixin implements MixinType { - /** - * Note the difference from {@code MIXIN_BASE}: {@code TARGET_METHOD} is not required, because - * {@code @Overwrite} has no {@code method} property - it is derived from the handler instead. - */ + /** TARGET_METHOD is not required here: {@code @Overwrite} derives it from the handler. */ private static final PropertyContainerTemplate TEMPLATE = PropertyContainerTemplate.builder() .require(ControlKeys.MIXIN_TYPE, ControlKeys.TARGET_CLASS, ControlKeys.PARAMETERS, ControlKeys.RETURN_TYPE) .keys(MixinKeys.LOCALS, MixinKeys.REQUIRE) @@ -51,7 +40,6 @@ public PropertyContainerTemplate getConfigurationTemplate() { @Override public MethodQualifier deriveTarget(PropertyContainer properties, MethodNode method) { - // @Overwrite has no target information: it replaces the method with its own name. return new MethodQualifier(method.name, null); } @@ -66,7 +54,6 @@ public TxResult preProcess(MixinContext context, MutableConfiguration clean, Res @Override public TxResult postProcess(MixinContext context, Configuration clean, MutableConfiguration dirty, Recipe recipe) { if (dirty.getTargetMethod() == null || dirty.getTargetMethod().desc() == null) { - // No method of that name exists in the patched target class any more. return TxResult.FAIL; } diff --git a/core/src/main/java/org/sinytra/adapter/patch/mixin/RedirectMixin.java b/core/src/main/java/org/sinytra/adapter/patch/mixin/RedirectMixin.java index af4cb0d..6951bb8 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/mixin/RedirectMixin.java +++ b/core/src/main/java/org/sinytra/adapter/patch/mixin/RedirectMixin.java @@ -58,12 +58,8 @@ public TxResult preProcess(MixinContext context, MutableConfiguration clean, Res if (clean.getAtData() == null) return TxResult.FAIL; if (!MixinAnnotationConstants.AT_VAL_INVOKE.equals(clean.getAtData().getValue())) { - // @Redirect is legal for FIELD (and other) targets too - e.g. carpet's - // @Redirect(method = "causeExtraKnockback", at = @At(value = "FIELD", - // target = "Lnet/minecraft/world/entity/Entity;hurtMarked:Z")). Failing unconditionally here - // dropped such mixins outright even though the referenced member still exists and needs no - // rewriting. Model the handler as written and let the pipeline's "does the injection point still - // resolve?" check decide instead. + // @Redirect is also legal for FIELD targets. Failing unconditionally dropped such mixins even + // when the referenced member still exists, so model the handler and let the pipeline decide. clean.setParameters(MethodParameters.create(context.methodNode(), List.of(MethodParameters.ParamGroup.SINGLE_ANY))); clean.setReturnType(Type.getReturnType(context.methodNode().desc)); @@ -102,8 +98,7 @@ public TxResult postProcess(MixinContext context, Configuration clean, MutableCo if (dirty.getTargetMethod() == null) return TxResult.FAIL; - // A non-INVOKE @Redirect (e.g. FIELD) targets a member, not a call: there is no method to - // resolve or re-describe, and the ref the mod wrote stays valid. Nothing to adapt. + // A non-INVOKE @Redirect targets a member, not a call: nothing to resolve and the ref stays valid. if (!MixinAnnotationConstants.AT_VAL_INVOKE.equals(dirty.getAtData().getValue())) return TxResult.SUCCESS; @@ -111,13 +106,8 @@ public TxResult postProcess(MixinContext context, Configuration clean, MutableCo if (targetDesc == null || targetDesc.desc() == null) return TxResult.FAIL; - // Locate the call this @At addresses. The injection point IS a call instruction inside the method - // being injected into, so find it THERE rather than resolving the @At's target through a class - // lookup. That lookup has to see the declaring class, and NeoForge routinely declares such members on - // its extension INTERFACES (BlockState implements IBlockStateExtension, which declares canStickTo) - - // a lookup of the @At's owner then misses it and the mixin is dropped as "failed postProcess". The - // call itself carries the real owner and descriptor regardless. What we find here is also exactly - // what Mixin binds to, so the handler is built to match. + // Find the call inside the patched method instead of resolving the @At's owner through a class lookup: + // NeoForge declares such members on extension interfaces, which that lookup misses. TargetPair injected = context.methods().findOwnMethodPair(context.dirtyLookup(), dirty.getTargetMethod()); MethodInsnNode callInsn = null; if (injected != null) { @@ -127,10 +117,7 @@ public TxResult postProcess(MixinContext context, Configuration clean, MutableCo calls.add(mi); } } - // Several call sites to the SAME member are normal and expected - Mixin's @At without an ordinal - // addresses the member, not one occurrence (carpet calls canStickTo twice inside addBlockLine). - // What has to be unambiguous is which MEMBER it is, so compare the distinct owner+name+descriptor - // rather than the instruction count. + // Several call sites are normal; what must be unambiguous is the distinct owner+name+descriptor. if (calls.stream().map(c -> c.owner + c.name + c.desc).distinct().limit(2).count() == 1) { callInsn = calls.getFirst(); } @@ -138,23 +125,17 @@ public TxResult postProcess(MixinContext context, Configuration clean, MutableCo TargetPair dirtyTarget = context.methods().findOwnMethodPair(context.dirtyLookup(), targetDesc); if (dirtyTarget == null) { - // Fall back to the descriptor being ignored (MethodQualifier#ignoreDesc leaves desc null, which - // matches any descriptor) - the same owner+name strategy ChangedMemberDescriptorSubResolver - // applies to call sites. + // Fall back to owner+name only, the same strategy ChangedMemberDescriptorSubResolver uses. dirtyTarget = context.methods().findOwnMethodPair(context.dirtyLookup(), targetDesc.ignoreDesc()); } - // Either the call itself or a resolvable target is enough - see the comment above for why the call is - // the better source when both exist. + // Either the call or a resolvable target is enough. if (dirtyTarget == null && callInsn == null) return TxResult.FAIL; List dirtyCaptured = context.methods().resolveCapturedMethodParams(recipe.clean(), recipe.dirty()); - // Build the handler's parameter model from the call we located when there is one. Deriving it from the - // resolved method instead is what broke carpet's updateNeighborsMaybe: the @At names the CALL - // (Level#updateNeighborsAt(BlockPos, Block)V, still 2-arg) while the method DECLARATION gained an - // Orientation parameter, so the two disagree and Mixin rejected the handler. - // See 12-issue草案.md entry 15. + // Prefer the call: the @At may name a call whose descriptor differs from the (since changed) method + // declaration, and Mixin binds the handler to the call. List callTypes = callInsn != null ? Parameters.getParameterTypes(callInsn.desc) : Parameters.getParameterTypes(targetDesc.desc()); diff --git a/core/src/main/java/org/sinytra/adapter/patch/processor/TargetMethodProcessor.java b/core/src/main/java/org/sinytra/adapter/patch/processor/TargetMethodProcessor.java index 5bd50ad..7a20ac4 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/processor/TargetMethodProcessor.java +++ b/core/src/main/java/org/sinytra/adapter/patch/processor/TargetMethodProcessor.java @@ -26,11 +26,8 @@ private static void upgradeCapturedLocals(MethodNode methodNode, MixinContext co AdapterUtil.CapturedLocals capturedLocals = AdapterUtil.getCapturedLocals(context, recipe); if (capturedLocals == null) return; - // NOTE: passing recipe.dirty().getAtData() here (to read the locals at the ADAPTED injection point) - // changes this processor from "no-op" to "rewrites the handler's parameter list" for far more - // mixins, which in testing broke unrelated mods (owo-lib failed with ClassNotFoundException while - // unused parameters were being removed mid-pipeline). Enable it together with the parameter - // annotation fix, not before. + // NOTE: passing the adapted @At data here makes this processor rewrite the parameter list of far + // more mixins and broke unrelated mods; enable it only together with the parameter annotation fix. List availableLocals = context.methods().getTargetMethodLocals(capturedLocals.target()); // For now, only handle cases where all locals are part of the method's params, convenient when switching the target to a lambda if (availableLocals == null || !availableLocals.isEmpty()) return; diff --git a/core/src/main/java/org/sinytra/adapter/patch/processor/extract/ExtractMixinProcessor.java b/core/src/main/java/org/sinytra/adapter/patch/processor/extract/ExtractMixinProcessor.java index f8fbf94..4b52ec1 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/processor/extract/ExtractMixinProcessor.java +++ b/core/src/main/java/org/sinytra/adapter/patch/processor/extract/ExtractMixinProcessor.java @@ -62,27 +62,16 @@ public static PatchResult apply(String targetClassName, MixinContext context, Re Candidates candidates = findCandidates(classNode, methodNode); if (!candidates.canMove(classNode, isInherited)) return PatchResult.PASS; - // The member may have moved to a STATIC method on the new target - a Fabric method mapped onto a - // NeoForge static hook. Mixin rejects that combination outright and the error is fatal: - // - // InvalidInjectionException: non-static callback method ... targets a static method which is not - // supported - // - // StaticAccessProcessor cannot catch this: it runs before extraction, when the callback still matches - // its original target. So the modifier has to be synced here - but it has to be DECIDED here, before - // anything is moved: a body that reads local 0 cannot be made static at all, and bailing out after the - // move would leave the member in both the original and the generated class. + // The member may have moved to a STATIC method on the new target, which Mixin rejects outright. + // StaticAccessProcessor runs before extraction and cannot catch it, so the modifier is synced here - + // decided before anything is moved, since a body reading local 0 cannot be made static. MethodQualifier movedTarget = recipe.dirty().getTargetMethod(); TargetPair movedPair = movedTarget == null ? null : context.methods().findOwnMethodPair(context.dirtyLookup(), movedTarget); boolean syncModifier = movedPair != null && MethodHelper.isStatic(movedPair.methodNode()) && !MethodHelper.isStatic(methodNode); if (syncModifier && readsReceiverSlot(methodNode)) { - // A @Shadow field or method access compiles to "aload 0 + getfield/invokevirtual", and exactly that - // shape is allowed through Candidates#canMove (a shadowed member does not count as "accessing the - // class instance"). A static target has no receiver, so there is nothing local 0 could hold. - // Report the mixin as unadapted instead: Patcher#stripFailedMixins then drops the handler and the - // mod loads with the feature disabled, which is strictly better than emitting a class that no - // longer passes verification. + // A @Shadow access compiles to "aload 0 + getfield/invokevirtual", which Candidates#canMove lets + // through, but a static target has no receiver. Bail out so Patcher#stripFailedMixins drops it. return PatchResult.PASS; } @@ -106,13 +95,7 @@ public static PatchResult apply(String targetClassName, MixinContext context, Re context.recordCtxAudit("Made extracted callback static to match static target %s", movedTarget.asDescriptor()); methodNode.access |= Opcodes.ACC_STATIC; - // The callback keeps its original body, and as an instance method that body reserved local 0 for the - // receiver - its parameters sat at 1..n. Now that the receiver is gone everything shifts down by one, - // exactly as StaticAccessProcessor does in the opposite direction ("Remove static"). Slot numbers are - // fixed at compile time and do not depend on branches, exception handlers or variable lifetimes, so - // the uniform shift is correct; the one shape it is NOT safe for - a body that reads local 0 - was - // rejected before anything was moved. Without the shift Mixin sees a body whose locals no longer line - // up and the game dies with "VerifyError: Bad local variable type" (locals[3] = top). + // The body reserved local 0 for the receiver, so its parameters shift down by one. if (methodNode.localVariables != null) { for (LocalVariableNode lvn : methodNode.localVariables) { lvn.index--; @@ -130,10 +113,7 @@ public static PatchResult apply(String targetClassName, MixinContext context, Re } // Take care of captured locals - // Dropping the receiver also invalidates the method's StackMapTable: a full_frame still describes local 0 - // and every slot after it. Connector only recomputes frames when the result is COMPUTE_FRAMES, so ask - // for them here - the uniform shift above is exactly the kind of change that silently leaves stale - // frames behind (measured: delta frames survive, an explicit full_frame fails verification). + // Dropping the receiver invalidates the StackMapTable, so ask for a full recomputation. PatchResult result = syncModifier ? PatchResult.COMPUTE_FRAMES : PatchResult.PASS; boolean capturesLocals = recipe.clean().hasProperty(MixinKeys.LOCALS); if (capturesLocals) { @@ -244,15 +224,7 @@ private static int fixAccess(int access) { return access; } - /** - * Whether the body reads local 0, which is the receiver of an instance method. - *

- * javac never assigns to slot 0 of an instance method, so any reference at all means the body uses - * {@code this}. A {@code @Shadow} field or method access compiles to exactly that shape ({@code aload 0} - * followed by {@code getfield}/{@code invokevirtual}), and a lambda that captured {@code this} is reached - * through the same {@code aload 0}. Checking only the handler (not the moved lambdas) is sufficient for the - * same reason: a lambda that needs the receiver forces the handler to load it. - */ + /** Whether the body reads local 0, i.e. uses {@code this}. */ private static boolean readsReceiverSlot(MethodNode methodNode) { for (AbstractInsnNode insn : methodNode.instructions) { if (insn instanceof VarInsnNode varInsn && varInsn.var == 0) { diff --git a/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/ChangedMemberDescriptorSubResolver.java b/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/ChangedMemberDescriptorSubResolver.java index 8e7951d..b40efae 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/ChangedMemberDescriptorSubResolver.java +++ b/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/ChangedMemberDescriptorSubResolver.java @@ -18,17 +18,8 @@ import static org.sinytra.adapter.env.util.MixinAnnotationConstants.AT_VAL_INVOKE_ASSIGN; /** - * Handles injection points whose target member survived but changed its descriptor. - *

- * Example: {@code Level.updateNeighborsAt(BlockPos, Block)} became - * {@code Level.updateNeighborsAt(BlockPos, Block, Orientation)}. The owner and name still match, so the - * call site is still there - only the original {@code @At} descriptor no longer matches anything, which - * made the whole injection point unresolvable. - *

- * {@link OverloadedInjectionPointSubResolver} covers a related but narrower situation (the old - * overload is still present and marked {@code @Deprecated}); when the old member is gone entirely that - * resolver bails out, which is exactly the case handled here. Note that this is deliberately - * conservative: the replacement has to be unambiguous, otherwise changing the target would be a guess. + * Handles injection points whose target member survived but changed its descriptor, e.g. a method that gained + * a parameter; only the old {@code @At} descriptor is stale. */ public class ChangedMemberDescriptorSubResolver implements SubResolver { @Nullable @@ -70,7 +61,6 @@ public Configuration resolve(MixinContext context, Recipe recipe) { MethodInsnNode call = calls.getFirst(); if (call.desc.equals(original.desc())) { - // Descriptor unchanged - this resolver has nothing to contribute. return null; } diff --git a/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/InjectionPointResolver.java b/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/InjectionPointResolver.java index 730a3f4..8080bfa 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/InjectionPointResolver.java +++ b/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/InjectionPointResolver.java @@ -22,8 +22,7 @@ public InjectionPointResolver() { addSubResolver(InjectionPointSubResolvers.REPLACED_TYPE); addSubResolver(InjectionPointSubResolvers.EXTRACTED_CALL); addSubResolver(InjectionPointSubResolvers.EXTRACTED_CODEPATH_CALL); - // Last on purpose: this is the most structural guess of the lot (the member moved to a different - // class entirely), so it only gets a turn once every narrower strategy has declined. + // Last on purpose: this is the most structural guess, so every narrower strategy declines first. addSubResolver(new RelocatedMemberSubResolver()); } @@ -49,11 +48,8 @@ protected Configuration tryReuse(MixinContext context, Recipe recipe) { // Try reusing the original List insns = context.methods().findInjectionTargetInsns(dirtyTarget); if (insns.isEmpty() && context.methodAnnotation().getNested("constant").isPresent()) { - // @ModifyConstant has no @At: its injection point IS the constant. The @At-based lookup above can - // therefore never match it, and the whole resolver reported "failed resolver InjectionPointResolver" - // even after the target method had been retargeted to where the constant moved. Ask the constant - // point instead. (Reference case: carpet's @ModifyConstant(intValue = 16) on Level#setBlock, whose - // code NeoForge moved into Level#markAndNotifyBlock.) + // @ModifyConstant has no @At, so the lookup above can never match it even after the target method + // was retargeted; ask the constant point instead. insns = context.methods().computeInjectionTargetInsns( dirtyTarget, () -> context.methodAnnotation().getNested("constant").orElse(null), diff --git a/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/RelocatedMemberSubResolver.java b/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/RelocatedMemberSubResolver.java index 08479c6..9320767 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/RelocatedMemberSubResolver.java +++ b/core/src/main/java/org/sinytra/adapter/patch/resolver/injection/RelocatedMemberSubResolver.java @@ -23,43 +23,12 @@ import static org.sinytra.adapter.env.util.MixinAnnotationConstants.AT_VAL_INVOKE_ASSIGN; /** - * Handles injection points whose target member MOVED to a different owner. + * Handles injection points whose target member MOVED to a different owner: NeoForge lifts a private static + * helper onto the class of its first parameter, so owner and name change while the call keeps its shape. The + * new name is deliberately ignored. *

- * The reference case is NeoForge lifting a private static helper onto the class of its first parameter: - * - * 

- *   vanilla      PistonStructureResolver.canStickToEachOther(BlockState, BlockState)Z   (private static)
- *   NeoForge     BlockState.canStickTo(BlockState)Z                                     (instance)
- *
- * - * Both the owner and the name changed, so every owner/name based lookup - including - * {@link ChangedMemberDescriptorSubResolver}, which keeps owner + name and only ignores the descriptor - - * finds nothing, and the injection point is reported as unresolvable. - *

- * The relocation survives in the SHAPE of the call: the receiver is dropped and the remaining parameters - * plus the return type stay identical. That is what this resolver matches on, deliberately without looking - * at the new name. - *

- * Why it scans the target method instead of the receiving class. The natural approach - look for a - * method with the shifted signature on the first parameter's class - does not work here, and that is not an - * accident of this one case: NeoForge implements such helpers as {@code default} methods on its extension - * interfaces, so the member is NOT declared by the receiving class at all. {@code canStickTo} lives on - * {@code net/neoforged/neoforge/common/extensions/IBlockStateExtension} and {@code BlockState} merely - * implements it. (The same holds for the {@code isStickyBlock} half of this pair.) A scan of the class - * therefore finds nothing, and scanning interfaces instead would widen the search surface rather than - * sharpen it. - *

- * Scanning the patched target method for the call itself is both simpler and better grounded: the call has to - * actually be there, it carries the real owner and descriptor, and the instruction it yields is exactly what - * the {@code @At} target has to be rewritten to. - *

- * Accepted only when unambiguous - the old member must be gone and have been static, the first parameter type - * must be an object type, and exactly one call in the patched method must match the shifted shape. Anything - * else bails out and leaves the original failure in place: guessing would silently point an injection at an - * unrelated member, which is worse than an honest failure. - *

- * Only {@code @At} INVOKE targets are considered. A mixin that injects INTO a method which was itself moved - * (carpet's {@code @Inject(at = @At("HEAD"), method = "isSticky")}) is a different mechanism. + * Matches the calls in the patched target method, not the receiving class: these helpers are usually + * {@code default} methods on extension interfaces. */ public class RelocatedMemberSubResolver implements SubResolver { @Nullable @@ -88,8 +57,7 @@ public Configuration resolve(MixinContext context, Recipe recipe) { return null; } - // The member has to be gone under its old owner+name. If it is still there, this resolver has nothing - // to contribute - ChangedMemberDescriptorSubResolver covers descriptor-only changes. + // ChangedMemberDescriptorSubResolver covers descriptor-only changes; this one needs the member gone. if (context.methods().findOwnMethodPair(context.dirtyLookup(), original.ignoreDesc()) != null) { return null; } @@ -112,10 +80,7 @@ public Configuration resolve(MixinContext context, Recipe recipe) { Type[] newArgs = Arrays.copyOfRange(oldArgs, 1, oldArgs.length); List matches = findShiftedCalls(dirtyTarget.methodNode(), receiver, newArgs, newReturn); - // Several call sites to the SAME member are fine and expected - Mixin's @At without an ordinal - // addresses the member, not one particular occurrence (carpet's addBlockLine calls canStickTo twice). - // What has to be unambiguous is which MEMBER the call was relocated to, so compare the distinct - // owner+name+descriptor, not the instruction count. + // Several call sites are fine; what must be unambiguous is the distinct owner+name+descriptor. if (matches.stream().map(call -> call.owner + call.name + call.desc).distinct().limit(2).count() != 1) { return null; } @@ -128,11 +93,7 @@ public Configuration resolve(MixinContext context, Recipe recipe) { .setAtData(at.withTarget(call)); } - /** - * Finds the calls in {@code target} that match the relocated shape: made on {@code receiver}, taking - * {@code newArgs} and returning {@code newReturn} - i.e. the old call with its receiver lifted out of the - * argument list. - */ + /** Finds the calls matching the shifted shape: the old call with its receiver lifted out of the arguments. */ private static List findShiftedCalls(MethodNode target, Type receiver, Type[] newArgs, Type newReturn) { List matches = new ArrayList<>(); for (AbstractInsnNode insn = target.instructions.getFirst(); insn != null; insn = insn.getNext()) { diff --git a/core/src/main/java/org/sinytra/adapter/patch/resolver/target/SplitTargetMethodSubResolver.java b/core/src/main/java/org/sinytra/adapter/patch/resolver/target/SplitTargetMethodSubResolver.java index 7660df3..3f27ce7 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/resolver/target/SplitTargetMethodSubResolver.java +++ b/core/src/main/java/org/sinytra/adapter/patch/resolver/target/SplitTargetMethodSubResolver.java @@ -132,14 +132,10 @@ private static List findInsnsCalls(List methods, Mi .map(pair -> { List insns = context.methods().findInjectionTargetInsns(pair); if (insns.isEmpty() && context.methodAnnotation().getNested("constant").isPresent()) { - // A constant injector (@ModifyConstant) has no @At - its injection point IS the constant - so - // the @At-based lookup above can never match it. Without asking the constant point instead, a - // method that was split away TOGETHER WITH ITS CONSTANT is never even considered a candidate, - // and the mixin fails with "failed resolver InjectionPointResolver" while the constant sits - // right there in the new method. - // - // Example: carpet's @ModifyConstant(intValue = 16, method = "setBlock(...)"). NeoForge moved - // that code (and the constant) into Level#markAndNotifyBlock. + // A constant injector (@ModifyConstant) has no @At, so the lookup above can never match it; + // without asking the constant point, a method split away together with its constant is never + // considered a candidate (carpet's @ModifyConstant on Level#setBlock, moved by NeoForge into + // Level#markAndNotifyBlock). insns = context.methods().computeInjectionTargetInsns( pair, () -> context.methodAnnotation().getNested("constant").orElse(null), diff --git a/core/src/main/java/org/sinytra/adapter/patch/resolver/target/TargetMethodSubResolvers.java b/core/src/main/java/org/sinytra/adapter/patch/resolver/target/TargetMethodSubResolvers.java index b0160c7..a1bb052 100644 --- a/core/src/main/java/org/sinytra/adapter/patch/resolver/target/TargetMethodSubResolvers.java +++ b/core/src/main/java/org/sinytra/adapter/patch/resolver/target/TargetMethodSubResolvers.java @@ -82,29 +82,9 @@ public class TargetMethodSubResolvers { * Handle cases where the target instructions have been moved into a lambda inside the target method */ public static final SubResolver MOVED_INTO_LAMBDA = (MixinContext context, Recipe recipe) -> { - // Decline the relocation for handlers that use mixinextras' @Local sugar. - // - // Why @Local specifically: LocalSugarApplicator#validate resolves the sugar against the local variable - // frame AT THE INJECTION POINT (InjectorUtils#getOrCreateLocalContext -> CompatibilityHelper#makeLvtContext - // takes the target method plus the injection instruction). Relocating the target into a lambda re-points - // the injection point, so the sugar is re-resolved against the lambda's frame and local variable table - // instead of the original method's. Whether that still succeeds depends on the shape - a captured local - // becomes a lambda PARAMETER and can still match, especially with argsOnly = true - but when it does not, - // MixinExtras fails validation at runtime with - // Suppressed: SugarApplicationException: Failed to validate sugar @Local Item - // and SugarApplicationException extends MixinException (NOT InvalidInjectionException), so Mixin's count - // check escalates it to a fatal - // InjectionError: Critical injection failure ... (0/1) succeeded. Scanned 0 target(s) - // By then the adaptation is long over, so the handler can no longer be stripped and the whole server - // refuses to start. Without the relocation it fails cleanly and is stripped instead (feature missing, - // server up). Reference case: create-fly's ItemStackMixin#cacheState. - // - // The relocation itself is a supported feature (see DynamicMixinPatchTest#testChangeTargetToLambdaInPipeline), - // so this stays deliberately narrow - only the sugar whose fate depends on the injection point is gated. - // On the 0.3.1 baseline the sugar set is @Local + @Share, and ShareSugarApplicator#validate only checks - // that the parameter is a LocalRef of a known type; it never looks at the target method or the - // instruction. (@Cancellable arrives in later MixinExtras versions and its validate is empty.) So @Local - // is the only mixinextras sugar that can fail because of where the injection point ended up. + // Decline relocation for handlers using mixinextras' @Local sugar: MixinExtras resolves the sugar + // against the local frame at the injection point, so re-pointing it at a lambda can fail validation at + // runtime, too late to strip the handler. @Local is the only sugar that depends on the injection point. if (hasLocalSugar(context)) return null; MethodQualifier cleanQualifier = recipe.clean().getTargetMethod(); @@ -115,13 +95,7 @@ public class TargetMethodSubResolvers { for (AbstractInsnNode insn : target.methodNode().instructions) { // Find lambda invocations and search for target insns inside the lambda if (insn instanceof InvokeDynamicInsnNode indy && indy.bsmArgs.length > 1 && indy.bsmArgs[1] instanceof Handle handle) { - // Keep the handle's OWNER. Parsing only the name and letting findOwnMethodPair guess the - // declaring class made it resolve a same-named lambda belonging to a DIFFERENT class - the - // mixin's target class, which is where an owner-less lookup starts. The resulting target then - // had nothing to do with the @At target, so Mixin could never apply it and the whole server - // died at startup ("Critical injection failure"). Reference case: create-fly's - // ItemStackMixin#cacheState, whose @At targets Item#useOn but which was re-pointed at - // ItemStack's own lambda$useOn$0. See 12-issue草案.md entry 17. + // Keep the handle's owner: an owner-less lookup starts at the mixin's target class. TargetPair lambda = MethodQualifier.parse(handle.getName()) .map(q -> context.methods().findOwnMethodPair(context.dirtyLookup(), new MethodQualifier(Type.getObjectType(handle.getOwner()).getDescriptor(), q.name(), q.desc()))) @@ -205,13 +179,7 @@ public static boolean isDirtyDeprecatedMethod(MixinContext context, MethodNode d return (pair == null || !AdapterUtil.isDeprecated(pair.methodNode())) && AdapterUtil.isDeprecated(dirty); } - /** - * Whether any parameter of the handler is annotated with mixinextras' {@code @Local}. - *

- * Used to keep {@link #MOVED_INTO_LAMBDA} away from handlers whose sugar has to be resolved against the - * injection point; see the call site for the full reasoning. This is deliberately narrower than "any - * mixinextras parameter annotation" - see the call site for why the other sugars cannot fail that way. - */ + /** Whether any parameter of the handler is annotated with mixinextras' {@code @Local}. */ private static boolean hasLocalSugar(MixinContext context) { MethodNode method = context.methodNode(); return hasLocalSugar(method.visibleParameterAnnotations) || hasLocalSugar(method.invisibleParameterAnnotations); @@ -234,13 +202,7 @@ private static boolean hasLocalSugar(java.util.List[] parameterA return false; } - /** - * Match {@code com.llamalad7.mixinextras.sugar.Local} by its simple name rather than by its full internal - * name: MixinExtras supports being relocated into another package - * ({@code MixinExtrasService#getAllClassNames}/{@code changePackage}), so the prefix is not a stable part of - * the contract. Over-matching is the safe direction here - the only cost is that the relocation is declined - * and the handler fails cleanly instead of being relocated. - */ + /** Matches by simple name: MixinExtras can be relocated into another package. */ private static boolean isLocalSugar(String desc) { return desc != null && desc.endsWith("/sugar/Local;"); } diff --git a/core/src/main/java/org/sinytra/adapter/transform/PipelineMethodTransformer.java b/core/src/main/java/org/sinytra/adapter/transform/PipelineMethodTransformer.java index f7e1598..827af42 100644 --- a/core/src/main/java/org/sinytra/adapter/transform/PipelineMethodTransformer.java +++ b/core/src/main/java/org/sinytra/adapter/transform/PipelineMethodTransformer.java @@ -48,39 +48,12 @@ public PipelineMethodTransformer(List methodPatches, boolean patche public PatchResult apply(MixinContext context, Configuration config) { TargetPair cleanTarget = context.methods().findOwnMethodPair(context.cleanLookup(), config.getTargetMethod()); if (cleanTarget == null) { - // The target class may exist only in the patched view: it can be supplied by the mod itself or by - // one of its libraries (io/github/axolotlclient/..., io/jsonwebtoken/..., de/.../classic4j/...), or - // it can be a platform class that the mod was never compiled against. Corpus-wide, - // "target method not found in the clean (vanilla) class" accounts for 763 of 922 skips and 91-94% - // of the sampled ones are the mod-own-class case. (12-issue草案.md entry 16, class A.) - // - // The two cases must NOT be treated alike: a mod-own class needs no adaptation (NeoForge never - // rewrites it), while a platform class usually DOES need adaptation and simply has no clean - // counterpart to compare against - passing those through unmapped produced a fatal, non-strippable - // InvalidInjectionException at runtime (create-fly's adapter_generated_CommonHooks targeting - // net/neoforged/neoforge/common/CommonHooks). - // - // Deciding by namespace would be wrong in both directions (it killed 18 legitimate mixins whose - // target is a platform class but which already resolve correctly). So decide by the only thing that - // actually matters: whether the injection point already resolves in the PATCHED class. If it does, - // there is genuinely nothing to adapt; if it does not, fall through and let the safety net strip the - // handler instead of shipping a mixin that aborts the game. + // The target class can exist only in the patched view: supplied by the mod, by one of its + // libraries, or by the platform. Mod-supplied classes never need adaptation; platform classes pass + // only when the injection point already resolves in the patched class. String targetClass = config.getProperty(ControlKeys.TARGET_CLASS).orElse(null); TargetPair dirtyCandidate = context.methods().findOwnMethodPair(context.dirtyLookup(), config.getTargetMethod()); - // Two very different situations hide behind "not found in the vanilla class": - // - // * the target class is the mod's own (or one of its libraries). NeoForge never rewrites those, so - // there is genuinely nothing to adapt and no clean counterpart to compare with. Pass them - this - // is the 91-94% case measured corpus-wide. - // - // * the target class belongs to the platform (net/minecraft, net/neoforged, ...). Those usually DO - // need adaptation, and passing them through unmapped produced a fatal, non-strippable - // InvalidInjectionException at runtime (create-fly's adapter_generated_CommonHooks targeting - // net/neoforged/neoforge/common/CommonHooks). For these, pass ONLY when the injection point already - // resolves in the patched class - which is exactly the question that matters. Deciding by - // namespace alone instead killed 18 legitimate mixins that target a platform class and already - // resolve correctly. boolean modSupplied = !isPlatformClass(targetClass); boolean platformButAlreadyResolvable = !modSupplied && dirtyCandidate != null @@ -101,20 +74,16 @@ public PatchResult apply(MixinContext context, Configuration config) { return PatchResult.PASS; } - // Publish the injection point here too: leaving it unset meant every later stage saw "unknown", - // which is indistinguishable from "no adaptation ran" and silently disabled capture re-derivation. + // Publish the injection point: later stages need it, and leaving it unset disables capture re-derivation. context.setEffectiveInjectionPoint(config.getAtData(), config.getTargetMethod()); return skip(context, config, "target method not found in the clean (vanilla) class"); } TargetPair dirtyTarget = context.methods().findOwnMethodPair(context.dirtyLookup(), config.getTargetMethod()); if (!this.patchResolver.matches(config) && !failsDirtyInjectionCheck(context, config, dirtyTarget) && hasValidSlice(context, config, dirtyTarget)) { - // NOT a failure: failsDirtyInjectionCheck() returning false means the mixin can already inject - // into the patched target as-is, i.e. no adaptation is required. Record it as a success so it - // is visible as "considered and fine" rather than never-considered. + // Not a failure: the mixin already injects into the patched target as-is. context.environment().auditTrail().recordResult(context, config, AuditTrail.Match.FULL); - // Still publish the injection point: it resolves here, and later transformers (captured-locals) - // need to know where it is even when nothing had to be rewritten. + // It resolves here, and captured-local transformers need the site even when nothing was rewritten. context.setEffectiveInjectionPoint(config.getAtData(), config.getTargetMethod()); try { context.recordCtxAudit("No adaptation required: injection point already resolves in the patched target"); @@ -138,19 +107,13 @@ public PatchResult apply(MixinContext context, Configuration config) { return PatchResult.PASS; } - /** - * The pipeline has several "give up quietly" exits. Each of them used to return PASS without - * recording anything, so the mixin vanished from the audit trail and the jar was reported as - * compatible. Record the skip so it shows up in the report. AuditTrailImpl downgrades it to - * IGNORED when the mixin is not required, so non-applicable mixins do not fail the mod. - */ + /** Records a give-up exit so the mixin shows up in the audit trail instead of vanishing from the report. */ private static PatchResult skip(MixinContext context, Configuration config, String reason, Object... args) { context.environment().auditTrail().recordResult(context, config, AuditTrail.Match.NONE); try { context.recordCtxAudit("Skipped mixin: " + reason, args); } catch (RuntimeException ignored) { - // Not every exit runs inside an audit frame (pushAudit/popAudit). recordResult above is - // what actually makes the skip visible; the reason is best-effort. + // Not every exit runs inside an audit frame; the reason is best-effort. } return PatchResult.PASS; } @@ -235,10 +198,7 @@ private PatchResult execute(MixinContext context, Configuration config) { context.methodNode().parameters = null; } - // Publish where the injection point ended up. The adapted values only exist inside this method's - // dirty config, so anything that runs afterwards and needs the effective site (notably - // LocalCaptureUpgradeTransformer, which has to read the locals live at THAT point) has no other way - // to see them. + // Publish where the injection point ended up; later transformers need the effective site. context.setEffectiveInjectionPoint(dirtyConfig.getAtData(), dirtyConfig.getTargetMethod()); return PatchResult.APPLY; @@ -287,13 +247,7 @@ private boolean validateAtNode(MixinContext context, AtData at, TargetPair targe return !insns.isEmpty(); } - /** - * Whether the name belongs to a platform namespace (the game or the mod loader) rather than to a mod. - *

- * Used to decide whether passing a handler through unmapped is safe. A mod-supplied target needs no - * adaptation at all; a platform target only qualifies when its injection point already resolves in the - * patched class (see the class-A branch in {@link #apply}). - */ + /** Whether the name belongs to a platform namespace (game or mod loader) rather than to a mod. */ private static boolean isPlatformClass(String internalName) { return internalName.startsWith("net/minecraft/") || internalName.startsWith("net/neoforged/") diff --git a/core/src/main/java/org/sinytra/adapter/transform/preprocess/LocalCaptureUpgradeTransformer.java b/core/src/main/java/org/sinytra/adapter/transform/preprocess/LocalCaptureUpgradeTransformer.java index e1d5749..4963a97 100644 --- a/core/src/main/java/org/sinytra/adapter/transform/preprocess/LocalCaptureUpgradeTransformer.java +++ b/core/src/main/java/org/sinytra/adapter/transform/preprocess/LocalCaptureUpgradeTransformer.java @@ -43,22 +43,11 @@ public PatchResult apply(MixinContext context, Configuration config) { List> localAnnotations = AdapterUtil.getAnnotatedParameters(methodNode, paramTypes, MixinAnnotations.LOCAL, Pair::of); if (!localAnnotations.isEmpty()) return PatchResult.PASS; - // NOTE: the capture list is re-derived from the injection point adaptation actually produced - see - // MixinContext#effectiveAtData, published by PipelineMethodTransformer. Reading it from the annotation - // instead made this transformer silently leave stale capture lists behind for every handler the - // pipeline moved. + // Re-derive against the injection point the pipeline actually produced (MixinContext#effectiveAtData). LocalVarAnalyzer.CapturedLocalsInfo info = LocalVarAnalyzer.getCapturedLocals(context, dirtyTarget, context.effectiveAtData()); if (info == null) { - // NOTE: in-game (server-only runtime) this is reached with context.effectiveAtData() == null for - // handlers the pipeline handled, i.e. the injection point was never published. PipelineMethodTransformer - // has three exits and only two of them publish; the remaining one is - // "target method not found in the clean (vanilla) class" -> skip(...). That is where to look next. - // - // A handler whose CallbackInfo/CallbackInfoReturnable is its LAST parameter captures nothing, so - // there is no capture list that could have gone stale - AdapterUtil reports that as "unknown" too, - // because it only derives a position when something follows the callback. Treating it as unsafe - // would strip perfectly good handlers. Anything else really is undeterminable, and those are left - // unmarked so Patcher strips them rather than let Mixin abort at runtime. + // effectiveAtData is null when the pipeline never published the injection point. A handler whose + // CallbackInfo is its last parameter captures nothing; anything else is left unmarked for Patcher. if (capturesNoLocals(methodNode)) { markUpgraded(config); } @@ -128,13 +117,7 @@ public PatchResult apply(MixinContext context, Configuration config) { return result; } - /** - * ASM sizes the parameter-annotation arrays from the descriptor at the time it first writes them. This - * transformer runs after the pipeline, which may have added or removed handler parameters, so the arrays - * can be shorter than the current parameter count - writing an annotation then throws - * {@code ArrayIndexOutOfBoundsException} inside {@code AnnotationWriter}. Resize them (keeping whatever - * is already there) so annotations land on the right parameters. - */ + /** Resizes the parameter-annotation arrays so they cover the current parameter count. */ @SuppressWarnings("unchecked") private static void ensureParameterAnnotationCapacity(MethodNode methodNode, int paramCount) { methodNode.visibleParameterAnnotations = resizeParameterAnnotations(methodNode.visibleParameterAnnotations, paramCount); @@ -151,10 +134,7 @@ private static List[] resizeParameterAnnotations(List