From 5af333cea805e5754c496c5bdae898f84adeb3f3 Mon Sep 17 00:00:00 2001 From: Algent Date: Thu, 3 Sep 2026 14:02:47 +0200 Subject: [PATCH] fix: restore multipart equality and removal hooks --- JAVA_MIGRATION.md | 20 ++ JAVA_MIGRATION_HANDOFF.md | 22 +- .../codechicken/multipart/TileMultipart.java | 24 +- .../multipart/scalatraits/TRedstoneTile.java | 5 +- .../TileMultipartEqualityRegressionTest.java | 236 ++++++++++++++++++ 5 files changed, 295 insertions(+), 12 deletions(-) create mode 100644 src/test/java/codechicken/multipart/TileMultipartEqualityRegressionTest.java diff --git a/JAVA_MIGRATION.md b/JAVA_MIGRATION.md index d6037a9..4a967f9 100644 --- a/JAVA_MIGRATION.md +++ b/JAVA_MIGRATION.md @@ -1479,3 +1479,23 @@ generated-trait compatibility work. Scala-runtime removal remains a later projec matches, the local dev config is unchanged, and all five `@Mod` versions match both packaged jar versions with the forced Scala-compilation guard retained. External Scala-trait tests remain green; existing manual client checks and Java-source model-bridge limitations remain outstanding. +- Reviewed the reported `TileMultipart` compatibility findings against `cacc9a3^`. Confirmed all three equality + changes and the dropped virtual light query. Restored null-safe Scala equality in change notifications and + replacement/removal filtering, removed all equal entries, and retained Scala `contains`/`indexOf` behavior. + Removal still reports the first matching index and invokes hooks/detaches only the requested part, as Scala did. +- Restored `getLightValue()` before `preRemove()` and corrected the related early/stale snapshot reads: filtering + now sees list updates from those callbacks, while empty/ticking decisions see updates from removal callbacks. + Seven JVM regression tests cover these behaviors, including light-query exceptions and missing-part rejection. + Six failed against the unfixed Java port; all seven pass against the complete original Scala class and fixed Java. + The reference class needed only explicit Java getter/setter, singleton and boxed-function call syntax for migrated + dependencies. Sources, compilation/test logs and reports are saved in ignored `run/tile-compatibility-review/`. +- Added brief comments explaining the profiled direct-list traversal and arbitrary-`Seq` fallback. No traversal or + compiler algorithm changed. The existing `NEWARRAY` limitation, forced Scala-compilation guard and manual client + checklist require no change in this review. All 450 class APIs match by names/descriptors/modifiers/generic signatures + and private fields. All 40 generated dumps retain the same instructions: 39 hashes are exact, with only debug line + numbers changed in the redstone helper by its added comment. These fixes restore the reference behavior and need + no divergence entry. The next migration target remains `ASMMixinCompiler.listSideOnly`. +- Clean formatting/checkstyle/build and the full Forge suite pass after stopping Gradle: 333 JVM / 141 Forge tests, + zero failures/errors/skips. The clean jar repeats the API/generated-output checks above; all five `@Mod` annotations + match both packaged jar versions. The forced Scala-compilation guard is unchanged. Review corrections are isolated + on `codex/tile-compatibility-fixes`, based on `fa008b8`; no additional migration unit was started. diff --git a/JAVA_MIGRATION_HANDOFF.md b/JAVA_MIGRATION_HANDOFF.md index eaea7ad..431b51e 100644 --- a/JAVA_MIGRATION_HANDOFF.md +++ b/JAVA_MIGRATION_HANDOFF.md @@ -15,6 +15,8 @@ what breaks, and what is left. The other documents hold the reasoning. Branch: `algent/java`. Base: `master`. 180 commits including the API-cleanup plan, divergence-log cleanup, ports through ASM super-call lookup, and the ASM characterization backfill and review fixes. +Review corrections are on `codex/tile-compatibility-fixes`, based on `fa008b8`, in the separate review worktree. + ## The one rule that matters **Check both consumer audits before changing a compatibility surface.** The ABI inventory is the authority on what @@ -74,7 +76,7 @@ awk -F'"' '/ f) { Seq current = partList; @@ -184,7 +185,7 @@ public class TileMultipart extends TileEntity implements IChunkLoadTile { /** Notifies parts sharing this host of a change. */ public void internalPartChange(TMultiPart part) { operate(action(p -> { - if (part != p) { + if (part == null ? p != null : !part.equals(p)) { p.onPartChanged(part); } })); @@ -328,15 +329,16 @@ public class TileMultipart extends TileEntity implements IChunkLoadTile { Iterator iterator = partList.iterator(); while (iterator.hasNext()) { TMultiPart part = iterator.next(); - if (part != opart) { + if (part == null ? opart != null : !part.equals(opart)) { olist.add(part); } } - if (olist.contains(npart)) { + Seq others = toSeq(olist); + if (others.contains(npart)) { return false; } - return occlusionTest(toSeq(olist), npart); + return occlusionTest(others, npart); } /** Returns true if parts do not occlude npart. */ @@ -427,14 +429,15 @@ public class TileMultipart extends TileEntity implements IChunkLoadTile { } private int remPart_do(TMultiPart part, boolean sendPacket) { - List current = mutablePartsSnapshot(); - int r = current.indexOf(part); + int r = partList.indexOf(part); if (r < 0) { throw new IllegalArgumentException("Tried to remove a non-existant part"); } + part.getLightValue(); part.preRemove(); - current.remove(r); + List current = mutablePartsSnapshot(); + current.removeIf(p -> p == null ? part == null : p.equals(part)); partList = toSeq(current); if (sendPacket) { @@ -445,12 +448,13 @@ public class TileMultipart extends TileEntity implements IChunkLoadTile { part.onRemoved(); part.tile_$eq(null); - if (current.isEmpty()) { + if (partList.isEmpty()) { worldObj.setBlockToAir(xCoord, yCoord, zCoord); } else if (part.doesTick() && doesTick) { boolean ntick = false; - for (TMultiPart p : current) { - ntick |= p.doesTick(); + Iterator iterator = partList.iterator(); + while (iterator.hasNext()) { + ntick |= iterator.next().doesTick(); } if (!ntick) { setTicking(false); diff --git a/src/main/scala/codechicken/multipart/scalatraits/TRedstoneTile.java b/src/main/scala/codechicken/multipart/scalatraits/TRedstoneTile.java index 280b731..0947266 100644 --- a/src/main/scala/codechicken/multipart/scalatraits/TRedstoneTile.java +++ b/src/main/scala/codechicken/multipart/scalatraits/TRedstoneTile.java @@ -12,7 +12,10 @@ import codechicken.multipart.TileMultipart; import scala.collection.Iterator; import scala.collection.Seq; -/** Mixin implementation for multipart redstone queries. */ +/** + * Mixin implementation for multipart redstone queries. Direct list walks avoid measured iterator/wrapper allocations; + * the public part-list setter also accepts other Seq types, which need the iterator fallback. + */ public class TRedstoneTile extends TileMultipart implements IRedstoneTile { @Override diff --git a/src/test/java/codechicken/multipart/TileMultipartEqualityRegressionTest.java b/src/test/java/codechicken/multipart/TileMultipartEqualityRegressionTest.java new file mode 100644 index 0000000..f61a55e --- /dev/null +++ b/src/test/java/codechicken/multipart/TileMultipartEqualityRegressionTest.java @@ -0,0 +1,236 @@ +package codechicken.multipart; + +import static org.junit.jupiter.api.Assertions.*; + +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +import org.junit.jupiter.api.Test; + +import scala.collection.JavaConversions; +import scala.collection.Seq; + +/** Equality and callback semantics of the pre-port TileMultipart implementation. */ +class TileMultipartEqualityRegressionTest { + + @Test + void internalPartChangeExcludesDistinctEqualPartsAndStillBroadcastsNull() { + List events = new ArrayList<>(); + TileMultipart tile = new TileMultipart(); + EqualPart first = new EqualPart("equal", "first", events); + EqualPart second = new EqualPart("equal", "second", events); + EqualPart other = new EqualPart("other", "other", events); + add(tile, first, second, other); + + tile.internalPartChange(new EqualPart("equal", "external", events)); + assertEquals(Arrays.asList("other.changed:external"), events); + events.clear(); + tile.internalPartChange(null); + assertEquals(Arrays.asList("first.changed:null", "second.changed:null", "other.changed:null"), events); + } + + @Test + void replacementFiltersEveryEqualPartBeforeOcclusionAndDuplicateChecks() { + List events = new ArrayList<>(); + TileMultipart tile = new TileMultipart(); + EqualPart first = new EqualPart("equal", "first", events); + EqualPart second = new EqualPart("equal", "second", events); + EqualPart survivor = new EqualPart("other", "survivor", events); + first.rejectOcclusion = true; + second.rejectOcclusion = true; + add(tile, first, second, survivor); + + assertTrue( + tile.canReplacePart( + new EqualPart("equal", "external", events), + new EqualPart("equal", "incoming", events))); + assertEquals(Arrays.asList("survivor.occlusion:incoming", "incoming.occlusion:survivor"), events); + assertFalse(tile.canReplacePart(first, new EqualPart("other", "duplicate", events))); + assertSame(first, tile.partList().apply(0)); + assertSame(second, tile.partList().apply(1)); + assertSame(survivor, tile.partList().apply(2)); + } + + @Test + void removalFiltersAllEqualEntriesButKeepsTheOriginalIndexAndCallbackTarget() throws Throwable { + List events = new ArrayList<>(); + RecordingTile tile = new RecordingTile(events); + EqualPart survivor = new EqualPart("other", "survivor", events); + EqualPart first = new EqualPart("equal", "first", events); + EqualPart second = new EqualPart("equal", "second", events); + add(tile, survivor, first, second); + Seq published = tile.partList(); + + assertEquals(1, remove(tile, second)); + assertEquals(Arrays.asList("second.light", "second.pre", "tile.removed:second:1", "second.removed"), events); + assertEquals(1, tile.partList().size()); + assertSame(survivor, tile.partList().head()); + assertEquals(3, published.size()); + assertNull(second.tile()); + assertSame(tile, first.tile()); // The reference only detaches the requested instance. + } + + @Test + void removalReadsThePublishedListAfterLightAndPreRemoveCallbacks() throws Throwable { + List events = new ArrayList<>(); + RecordingTile tile = new RecordingTile(events); + EqualPart victim = new EqualPart("victim", "victim", events); + EqualPart stale = new EqualPart("stale", "stale", events); + EqualPart addedByLight = new EqualPart("light", "light", events); + EqualPart addedByPre = new EqualPart("pre", "pre", events); + add(tile, stale, victim); + victim.lightAction = () -> tile.partList_$eq(seq(victim, addedByLight)); + victim.preAction = () -> { + assertSame(addedByLight, tile.partList().apply(1)); + tile.partList_$eq(seq(addedByPre, victim, addedByLight)); + }; + + assertEquals(1, remove(tile, victim)); + assertEquals(2, tile.partList().size()); + assertSame(addedByPre, tile.partList().apply(0)); + assertSame(addedByLight, tile.partList().apply(1)); + assertEquals(Arrays.asList("victim.light", "victim.pre", "tile.removed:victim:1", "victim.removed"), events); + } + + @Test + void removalUsesTheListPublishedByOnRemovedWhenCheckingForAnEmptyTile() throws Throwable { + List events = new ArrayList<>(); + RecordingTile tile = new RecordingTile(events); + EqualPart victim = new EqualPart("victim", "victim", events); + EqualPart replacement = new EqualPart("replacement", "replacement", events); + add(tile, victim); + victim.removedAction = () -> tile.partList_$eq(seq(replacement)); + + assertEquals(0, remove(tile, victim)); + assertSame(replacement, tile.partList().head()); + assertNull(victim.tile()); + } + + @Test + void lightFailureStopsRemovalBeforePreRemoveOrListMutation() { + List events = new ArrayList<>(); + RecordingTile tile = new RecordingTile(events); + EqualPart victim = new EqualPart("victim", "victim", events); + EqualPart survivor = new EqualPart("other", "survivor", events); + add(tile, victim, survivor); + Seq published = tile.partList(); + IllegalStateException failure = new IllegalStateException("light failed"); + victim.lightAction = () -> { throw failure; }; + + assertSame(failure, assertThrows(IllegalStateException.class, () -> remove(tile, victim))); + assertSame(published, tile.partList()); + assertSame(tile, victim.tile()); + assertEquals(Arrays.asList("victim.light"), events); + } + + @Test + void missingPartFailsBeforeInvokingItsLightOrRemovalHooks() { + List events = new ArrayList<>(); + RecordingTile tile = new RecordingTile(events); + add(tile, new EqualPart("present", "present", events)); + assertThrows(IllegalArgumentException.class, () -> remove(tile, new EqualPart("absent", "absent", events))); + assertTrue(events.isEmpty()); + } + + private static int remove(TileMultipart tile, TMultiPart part) throws Throwable { + Method method = TileMultipart.class.getDeclaredMethod("remPart_do", TMultiPart.class, boolean.class); + method.setAccessible(true); + try { + return (Integer) method.invoke(tile, part, false); + } catch (InvocationTargetException exception) { + throw exception.getCause(); + } + } + + private static void add(TileMultipart tile, TMultiPart... parts) { + for (TMultiPart part : parts) tile.addPart_do(part); + } + + private static Seq seq(TMultiPart... parts) { + return JavaConversions.asScalaBuffer(Arrays.asList(parts)).toList(); + } + + private static final class RecordingTile extends TileMultipart { + + private final List events; + + RecordingTile(List events) { + this.events = events; + } + + @Override + public void partRemoved(TMultiPart part, int index) { + events.add("tile.removed:" + ((EqualPart) part).name + ":" + index); + } + } + + private static class EqualPart extends TMultiPart { + + private final String key; + private final String name; + private final List events; + private boolean rejectOcclusion; + private Runnable lightAction = () -> {}; + private Runnable preAction = () -> {}; + private Runnable removedAction = () -> {}; + + EqualPart(String key, String name, List events) { + this.key = key; + this.name = name; + this.events = events; + } + + @Override + public String getType() { + return name; + } + + @Override + public boolean doesTick() { + return false; + } + + @Override + public boolean equals(Object other) { + return other instanceof EqualPart && key.equals(((EqualPart) other).key); + } + + @Override + public int hashCode() { + return key.hashCode(); + } + + @Override + public void onPartChanged(TMultiPart part) { + events.add(name + ".changed:" + (part == null ? "null" : ((EqualPart) part).name)); + } + + @Override + public boolean occlusionTest(TMultiPart other) { + events.add(name + ".occlusion:" + ((EqualPart) other).name); + return !rejectOcclusion; + } + + @Override + public int getLightValue() { + events.add(name + ".light"); + lightAction.run(); + return 7; + } + + @Override + public void preRemove() { + events.add(name + ".pre"); + preAction.run(); + } + + @Override + public void onRemoved() { + events.add(name + ".removed"); + removedAction.run(); + } + } +}