diff --git a/JAVA_MIGRATION.md b/JAVA_MIGRATION.md index 64d7d73..3fc504d 100644 --- a/JAVA_MIGRATION.md +++ b/JAVA_MIGRATION.md @@ -1349,8 +1349,8 @@ generated-trait compatibility work. Scala-runtime removal remains a later projec - Backfilled, on request, the characterization tests the six ports from `b45527e` to `8581d30` had skipped. Six new suites in `src/test/java/codechicken/multipart/asm/` add 32 plain-JVM tests, taking that suite from 276 to 308. `ByteCodeReader` and the signature parser are covered by behavior; `ScalaSigReader` by round trips, annotation - lookup and its replaced-value result. `DebugPrinter`, `ASMMixinFactory` and `MultipartMixinFactory` initialize - through Forge, so those tests pin public surfaces, fields and bytecode constants and calls, leaving generated + lookup and its replaced-value result. Debug initialization and generator execution need Forge, so those tests + pin public surfaces, fields and bytecode constants and calls, leaving generated tiles, pass-through delegation and Java-trait `copyFrom` to the Forge suite. - Ran the backfill against `src/main` restored from `1faf0dd`: it compiles against the untouched Scala and 45 of 46 tests pass. The only failure is `MultipartMixinFactory`'s facade method set, which is the four additive static @@ -1359,3 +1359,22 @@ generated-trait compatibility work. Scala-runtime removal remains a later projec - Two behaviors were pinned that inspection had not recorded. `ScalaSigReader.encode` drops the final 7-bit group, so a decode round trip is exact only when that group is empty; a payload ending in a high-bit byte does not survive it. `ScalaSignature.evalS` throws `MatchError` on an unknown tag while `eval` returns the table entry unchanged. + +### 2026-09-03 + +- Addressed the three backfill review findings without changing production code. The lossy signature round trip now + asserts the exact reference bytes `[0, 62]`, rather than accepting any result different from `[0, 0xfe]`. +- Replaced the two presence-only `DebugPrinter` JVM checks with three Forge behavior tests for both config modes: + directory creation, immediate-child-only startup cleanup, dump gating/naming/content and cumulative 16,000-byte + logging boundaries (including an empty input and a single input crossing two boundaries). An isolated copy changes + only the hard-coded output path to `@TempDir`; config and logger state are restored and live dev dumps are untouched. + A temporary fault-injection run removing deletion and reversing dump/log guards failed all three new printer tests; + those production edits were then restored. +- Replaced the `copyFrom` call-presence check with three Forge tests invoking the actual completer. They assert + byte-for-byte no-ops for empty/transient-only nodes and an existing method, then exact emitted super-call/guard/field + order, transient exclusion and idempotence for mixed fields. Corrected the coverage claims: singleton construction + is headless, but generation reaches Forge through `ObfMapping`; earlier dump comparisons were one-off checks. +- Verification baseline is now 305 JVM / 122 Forge tests: three weak JVM checks moved to six focused Forge checks. + Formatting/checkstyle/build and the Forge suite pass; all 39 generated dump hashes and the dev config are unchanged. + No production ABI, source counts or next target changed. `multipart/asm/StackAnalyser.scala` remains next, using + explicit migration-test authorization and separate characterization and refactor commits. diff --git a/JAVA_MIGRATION_HANDOFF.md b/JAVA_MIGRATION_HANDOFF.md index dcfbd6e..1217255 100644 --- a/JAVA_MIGRATION_HANDOFF.md +++ b/JAVA_MIGRATION_HANDOFF.md @@ -12,8 +12,8 @@ what breaks, and what is left. The other documents hold the reasoning. | `JAVA_MIGRATION_MANUAL_CHECKS.md` | What no automated test can cover, and must be checked by hand in a client | | `JAVA_MIGRATION_PROFILE.md` | The focused baseline, first measured result, findings, and exact rerun command | -Branch: `algent/java`. Base: `master`. 164 commits including the API-cleanup plan, divergence-log cleanup and ports -through the `ScalaSignature` parser extraction. +Branch: `algent/java`. Base: `master`. 172 commits including the API-cleanup plan, divergence-log cleanup, ports +through the `ScalaSignature` parser extraction, and its characterization backfill and review fixes. ## The one rule that matters @@ -26,7 +26,9 @@ Followed for every port so far. It has caught three real ABI breaks that inspect **Tests are added only when requested.** The six ASM ports from `b45527e` to `8581d30` were made under that instruction, verified with ABI, disassembly and dump comparisons instead; their characterization tests were -backfilled afterwards on request. Ask before skipping step 1 again, and record which check replaced it. +backfilled afterwards on request. If a fresh session lacks explicit authorization for migration tests, ask for it +before starting the target; do not silently skip step 1. A request to add characterization tests for each migration +target satisfies the general "do not write tests unless asked" instruction. 1. **Characterize first, commit separately.** Write tests against the untouched Scala and confirm they pass. Commit as `test: characterize X` before touching the implementation. @@ -72,7 +74,7 @@ awk -F'"' '/")) continue; + for (AbstractInsnNode instruction : method.instructions.toArray()) { + if (instruction instanceof LdcInsnNode && "asm/multipart".equals(((LdcInsnNode) instruction).cst)) { + ((LdcInsnNode) instruction).cst = directory.toAbsolutePath().toString(); + paths++; + } + } + } + assertEquals(1, paths, "Never initialize a test copy with the live dev dump path"); + ClassWriter writer = new ClassWriter(0); + node.accept(writer); + Class type = new FixtureClassLoader(DebugPrinter$.class.getClassLoader()).define(writer.toByteArray()); + ConfigTag tag = MultipartProxy.config().getTag("debug_asm"); + String previous = tag.value; + try { + // In-memory override only: do not save or rewrite the development config file. + tag.value = Boolean.toString(enabled); + Object printer = type.getField("MODULE$").get(null); + assertEquals(enabled, type.getMethod("debug").invoke(printer)); + assertEquals(directory.toAbsolutePath().toFile(), type.getMethod("dir").invoke(printer)); + return printer; + } finally { + tag.value = previous; + } + } + + private static final class FixtureClassLoader extends ClassLoader { + + private FixtureClassLoader(ClassLoader parent) { + super(parent); + } + + private Class define(byte[] bytecode) { + return defineClass(null, bytecode, 0, bytecode.length); + } + } + + private static final class RecordingAppender extends AbstractAppender { + + private final List messages = new ArrayList<>(); + + private RecordingAppender() { + super("fmp-debug-printer-test", null, null, false); + } + + @Override + public void append(LogEvent event) { + assertEquals(Level.DEBUG, event.getLevel()); + messages.add(event.getMessage().getFormattedMessage()); + } + } +} diff --git a/src/functionalTest/java/codechicken/multipart/test/MultipartMixinFactoryFunctionalTest.java b/src/functionalTest/java/codechicken/multipart/test/MultipartMixinFactoryFunctionalTest.java new file mode 100644 index 0000000..d4d46e4 --- /dev/null +++ b/src/functionalTest/java/codechicken/multipart/test/MultipartMixinFactoryFunctionalTest.java @@ -0,0 +1,145 @@ +package codechicken.multipart.test; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.objectweb.asm.Opcodes.ACC_PUBLIC; +import static org.objectweb.asm.Opcodes.ACC_TRANSIENT; +import static org.objectweb.asm.Opcodes.ALOAD; +import static org.objectweb.asm.Opcodes.GETFIELD; +import static org.objectweb.asm.Opcodes.IFEQ; +import static org.objectweb.asm.Opcodes.INSTANCEOF; +import static org.objectweb.asm.Opcodes.INVOKESPECIAL; +import static org.objectweb.asm.Opcodes.NOP; +import static org.objectweb.asm.Opcodes.PUTFIELD; +import static org.objectweb.asm.Opcodes.RETURN; +import static org.objectweb.asm.Opcodes.V1_6; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +import org.junit.jupiter.api.Test; +import org.objectweb.asm.tree.AbstractInsnNode; +import org.objectweb.asm.tree.ClassNode; +import org.objectweb.asm.tree.FieldInsnNode; +import org.objectweb.asm.tree.JumpInsnNode; +import org.objectweb.asm.tree.MethodInsnNode; +import org.objectweb.asm.tree.MethodNode; +import org.objectweb.asm.tree.TypeInsnNode; +import org.objectweb.asm.tree.VarInsnNode; + +import codechicken.lib.asm.ASMHelper; +import codechicken.multipart.asm.MultipartMixinFactory$; + +class MultipartMixinFactoryFunctionalTest { + + private static final String TILE = "codechicken/multipart/TileMultipart"; + private static final String COPY_DESC = "(L" + TILE + ";)V"; + + @Test + void leavesEmptyAndTransientOnlyTraitsUntouched() { + for (boolean transientField : new boolean[] { false, true }) { + ClassNode node = trait(); + if (transientField) node.visitField(ACC_PUBLIC | ACC_TRANSIENT, "cache", "I", null, null); + byte[] before = ASMHelper.createBytes(node, 0); + + MultipartMixinFactory$.MODULE$.autoCompleteJavaTrait(node); + + assertEquals(0, node.methods.size()); + assertArrayEquals(before, ASMHelper.createBytes(node, 0)); + } + } + + @Test + void preservesAnExistingCopyFromWithoutEmittingAnything() { + ClassNode node = trait(); + node.visitField(ACC_PUBLIC, "saved", "I", null, null); + MethodNode existing = new MethodNode(ACC_PUBLIC, "copyFrom", COPY_DESC, null, null); + existing.visitInsn(NOP); + existing.visitInsn(RETURN); + existing.visitMaxs(0, 2); + node.methods.add(existing); + byte[] before = ASMHelper.createBytes(node, 0); + + MultipartMixinFactory$.MODULE$.autoCompleteJavaTrait(node); + + assertEquals(1, node.methods.size()); + assertSame(existing, node.methods.get(0)); + assertArrayEquals(before, ASMHelper.createBytes(node, 0)); + } + + @Test + void emitsSuperCallThenTypeGuardAndOnlyNonTransientFieldCopies() { + ClassNode node = trait(); + node.visitField(ACC_PUBLIC, "first", "I", null, null); + node.visitField(ACC_PUBLIC | ACC_TRANSIENT, "cache", "I", null, null); + node.visitField(ACC_PUBLIC, "second", "Ljava/lang/String;", null, null); + + MultipartMixinFactory$.MODULE$.autoCompleteJavaTrait(node); + + assertEquals(1, node.methods.size()); + MethodNode copy = node.methods.get(0); + assertEquals("copyFrom", copy.name); + assertEquals(COPY_DESC, copy.desc); + assertEquals(ACC_PUBLIC, copy.access); + assertEquals(2, copy.maxStack); + assertEquals(2, copy.maxLocals); + List opcodes = new ArrayList<>(); + List locals = new ArrayList<>(); + List fields = new ArrayList<>(); + for (AbstractInsnNode instruction : copy.instructions.toArray()) { + if (instruction.getOpcode() >= 0) opcodes.add(instruction.getOpcode()); + if (instruction instanceof VarInsnNode) locals.add(((VarInsnNode) instruction).var); + if (instruction instanceof MethodInsnNode) { + MethodInsnNode call = (MethodInsnNode) instruction; + assertEquals(TILE, call.owner); + assertEquals("copyFrom", call.name); + assertEquals(COPY_DESC, call.desc); + } + if (instruction instanceof TypeInsnNode) { + assertEquals(node.name, ((TypeInsnNode) instruction).desc); + } + if (instruction instanceof JumpInsnNode) { + assertEquals(RETURN, ((JumpInsnNode) instruction).label.getNext().getOpcode()); + } + if (instruction instanceof FieldInsnNode) { + FieldInsnNode field = (FieldInsnNode) instruction; + assertEquals(node.name, field.owner); + fields.add(field.name + ':' + field.desc); + } + } + assertEquals( + Arrays.asList( + ALOAD, + ALOAD, + INVOKESPECIAL, + ALOAD, + INSTANCEOF, + IFEQ, + ALOAD, + ALOAD, + GETFIELD, + PUTFIELD, + ALOAD, + ALOAD, + GETFIELD, + PUTFIELD, + RETURN), + opcodes); + assertEquals(Arrays.asList(0, 1, 1, 0, 1, 0, 1), locals); + assertEquals( + Arrays.asList("first:I", "first:I", "second:Ljava/lang/String;", "second:Ljava/lang/String;"), + fields); + + byte[] before = ASMHelper.createBytes(node, 0); + MultipartMixinFactory$.MODULE$.autoCompleteJavaTrait(node); + assertArrayEquals(before, ASMHelper.createBytes(node, 0), "A second completion must not duplicate copyFrom"); + } + + private static ClassNode trait() { + ClassNode node = new ClassNode(); + node.visit(V1_6, ACC_PUBLIC, "test/CopyFromFixture", null, TILE, null); + return node; + } +} diff --git a/src/test/java/codechicken/multipart/asm/DebugPrinterCharacterizationTest.java b/src/test/java/codechicken/multipart/asm/DebugPrinterCharacterizationTest.java index 38173ed..1bc1964 100644 --- a/src/test/java/codechicken/multipart/asm/DebugPrinterCharacterizationTest.java +++ b/src/test/java/codechicken/multipart/asm/DebugPrinterCharacterizationTest.java @@ -14,18 +14,11 @@ import java.util.TreeSet; import org.apache.logging.log4j.Logger; import org.junit.jupiter.api.Test; -import org.objectweb.asm.ClassReader; import org.objectweb.asm.Type; -import org.objectweb.asm.tree.AbstractInsnNode; -import org.objectweb.asm.tree.ClassNode; -import org.objectweb.asm.tree.IntInsnNode; -import org.objectweb.asm.tree.LdcInsnNode; -import org.objectweb.asm.tree.MethodInsnNode; -import org.objectweb.asm.tree.MethodNode; /** - * Reflection and bytecode only: the companion's initializer reads the Forge config, so loading it in a plain JVM is not - * possible. Dump content and the byte-count log are covered by the Forge suite. + * Reflection only: initialization reads the Forge config. DebugPrinterFunctionalTest covers directory cleanup, + * enabled/disabled dumping and byte-count logging under Forge. */ class DebugPrinterCharacterizationTest { @@ -65,73 +58,6 @@ class DebugPrinterCharacterizationTest { assertEquals(5, DebugPrinter$.class.getDeclaredFields().length); } - @Test - void gatesOnTheConfigTagAndClearsTheDumpDirectoryOnce() throws Exception { - ClassNode node = companion(); - assertEquals(signatures("debug_asm", "Multipart ASM", "asm/multipart"), constants(node, "")); - assertCall(node, "", "codechicken/multipart/handler/MultipartProxy", "config"); - assertCall(node, "", "java/io/File", "mkdirs"); - // The per-file delete itself is covered by the Forge suite, which compares generated dump names. - assertCall(node, "", "java/io/File", "listFiles"); - assertCall(node, "", "java/io/File", "exists"); - } - - @Test - void dumpsThroughASMHelperAndLogsEverySixteenThousandBytes() throws Exception { - ClassNode node = companion(); - assertEquals(signatures(".txt"), constants(node, "dump")); - assertCall(node, "dump", "codechicken/lib/asm/ASMHelper", "dump"); - - assertEquals(signatures(" bytes of permGen has been used by ASMMixinCompiler"), constants(node, "defined")); - assertTrue(intOperands(node, "defined").contains(16000)); - assertCall(node, "defined", "org/apache/logging/log4j/Logger", "debug"); - } - - private static ClassNode companion() throws Exception { - ClassNode node = new ClassNode(); - new ClassReader(DebugPrinter$.class.getName()).accept(node, 0); - return node; - } - - private static Set constants(ClassNode node, String methodName) { - Set found = new TreeSet<>(); - for (AbstractInsnNode instruction : instructions(node, methodName)) { - if (instruction instanceof LdcInsnNode && ((LdcInsnNode) instruction).cst instanceof String) { - found.add((String) ((LdcInsnNode) instruction).cst); - } - } - return found; - } - - private static Set intOperands(ClassNode node, String methodName) { - Set found = new TreeSet<>(); - for (AbstractInsnNode instruction : instructions(node, methodName)) { - if (instruction instanceof IntInsnNode) { - found.add(((IntInsnNode) instruction).operand); - } else if (instruction instanceof LdcInsnNode && ((LdcInsnNode) instruction).cst instanceof Integer) { - found.add((Integer) ((LdcInsnNode) instruction).cst); - } - } - return found; - } - - private static void assertCall(ClassNode node, String methodName, String owner, String name) { - for (AbstractInsnNode instruction : instructions(node, methodName)) { - if (instruction instanceof MethodInsnNode) { - MethodInsnNode call = (MethodInsnNode) instruction; - if (call.owner.equals(owner) && call.name.equals(name)) return; - } - } - throw new AssertionError("Missing call " + owner + '.' + name + " in " + methodName); - } - - private static AbstractInsnNode[] instructions(ClassNode node, String methodName) { - for (MethodNode method : node.methods) { - if (method.name.equals(methodName)) return method.instructions.toArray(); - } - throw new AssertionError("Missing method " + methodName); - } - private static void assertField(String name, Class type, int modifiers) throws Exception { Field field = DebugPrinter$.class.getDeclaredField(name); assertSame(type, field.getType()); diff --git a/src/test/java/codechicken/multipart/asm/MultipartMixinFactoryCharacterizationTest.java b/src/test/java/codechicken/multipart/asm/MultipartMixinFactoryCharacterizationTest.java index 2323cfd..260646a 100644 --- a/src/test/java/codechicken/multipart/asm/MultipartMixinFactoryCharacterizationTest.java +++ b/src/test/java/codechicken/multipart/asm/MultipartMixinFactoryCharacterizationTest.java @@ -21,9 +21,8 @@ import org.objectweb.asm.tree.MethodInsnNode; import org.objectweb.asm.tree.MethodNode; /** - * The companion's initializer reaches Forge through TileMultipart, so generation is exercised by the Forge suite: - * generated tiles, pass-through delegation and the copied fields of Java traits. Surface and emission order are checked - * here. + * Surface and bytecode smoke checks only. Generation reaches Forge through ObfMapping; the Forge suite checks copyFrom + * guards and emitted instructions, generated tiles and pass-through delegation. */ class MultipartMixinFactoryCharacterizationTest { @@ -83,18 +82,6 @@ class MultipartMixinFactoryCharacterizationTest { assertCall(node, "onCompiled", "codechicken/multipart/MultipartGenerator$", "registerTileClass"); } - @Test - void completesJavaTraitsWithAGuardedCopyFrom() throws Exception { - ClassNode node = companion(); - Set constants = constants(node, "autoCompleteJavaTrait"); - assertTrue(constants.contains("copyFrom")); - assertTrue(constants.contains("(Lcodechicken/multipart/TileMultipart;)V")); - assertTrue(constants.contains("codechicken/multipart/TileMultipart")); - // The existing-method guard runs before anything is emitted. - assertCall(node, "autoCompleteJavaTrait", "codechicken/lib/asm/ASMHelper", "findMethod"); - assertCall(node, "autoCompleteJavaTrait", "org/objectweb/asm/MethodVisitor", "visitMaxs"); - } - @Test void namesAndFillsThePassThroughTrait() throws Exception { ClassNode node = companion(); diff --git a/src/test/java/codechicken/multipart/asm/ScalaSigReaderCharacterizationTest.java b/src/test/java/codechicken/multipart/asm/ScalaSigReaderCharacterizationTest.java index f99447a..8ba3d3c 100644 --- a/src/test/java/codechicken/multipart/asm/ScalaSigReaderCharacterizationTest.java +++ b/src/test/java/codechicken/multipart/asm/ScalaSigReaderCharacterizationTest.java @@ -74,11 +74,11 @@ class ScalaSigReaderCharacterizationTest { assertEquals("", ScalaSigReader.encode(new byte[0])); assertArrayEquals(new byte[0], ScalaSigReader.decode("")); - // The dropped trailing group is empty for a short payload, but carries the high bits of a long one. + // The dropped group is zero for 42, but carries high bits for [0, 0xfe]. byte[] single = { 42 }; assertArrayEquals(single, ScalaSigReader.decode(ScalaSigReader.encode(single))); byte[] highBit = { 0, (byte) 0xfe }; - assertFalse(Arrays.equals(highBit, ScalaSigReader.decode(ScalaSigReader.encode(highBit)))); + assertArrayEquals(new byte[] { 0, 62 }, ScalaSigReader.decode(ScalaSigReader.encode(highBit))); } @Test