From b25a33ba389515e6f2e4ffbf4d4b795c21523085 Mon Sep 17 00:00:00 2001 From: Frotty Date: Sat, 15 Aug 2026 10:26:10 +0200 Subject: [PATCH 01/13] Count temporary names from the start of each compilation The counters naming generated temporaries were per thread and never reset, so the name a temporary got depended on how much had been compiled before it in the process: the same source emitted temp0 on its own and temp70 after other tests had run. That is what made generated Jass impossible to compare across runs, which is the cheapest way there is to check that a change did not alter the output. They restart at the top of each compilation rather than at each flattening, because flattening happens again after every optimisation and restarting there would give one function two locals of the same name. --- BACKLOG.md | 17 ++++---- .../peeeq/wurstio/WurstCompilerJassImpl.java | 3 ++ .../translation/imtranslation/Flatten.java | 16 +++++++ .../tests/DeterministicChecks.java | 42 +++++++++++++++++++ 4 files changed, 71 insertions(+), 7 deletions(-) diff --git a/BACKLOG.md b/BACKLOG.md index 420406814..818dee7fa 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -72,11 +72,6 @@ because `LOOP.md` refers to items by number. source parameter. Making the node canonical lets all three compare by identity and removes a class of silent wrong dispatch. Mechanical, well covered by the suite. -11. **Jass temp counter is not reset between compilations.** Two runs of the same commit emit - different `.j` (`temp151` vs `temp8`) because the counter is JVM-wide and depends on how - many tests ran before. Not wrong for compiling one map, but it means `.j` cannot be diffed - across runs to validate a change — only `.lua` can. Fixing it would make Jass diffable. - 13. **A bounded generic class cannot be subclassed.** Found while building a repro for item 3; both backends break, differently, on the same program: @@ -153,6 +148,14 @@ because `LOOP.md` refers to items by number. ## Done +- 11. Generated Jass can be compared across runs. The counters naming temporaries were per thread + and never reset, so a name depended on how much had been compiled before it: the same source gave + `temp0` alone and `temp70` after other tests, measured directly rather than assumed. They now + start from zero at the top of each compilation — not at each flattening, which happens again + after every optimisation and would name two locals of one function alike. + `DeterministicChecks.temporaryNamesDoNotDependOnEarlierCompilations` compiles the same source, + then other programs, then the same source again; the compilation in between is the part that + matters, and it is the inlining configuration that emits a temporary for that source. - 18. Comparing an unresolved type parameter default is now an error rather than a quiet "not equal". Item 16 closed the path that reached a program, but the stand-in is produced by a static attribute and could surface anywhere, so the silence was the part worth removing. The whole suite @@ -220,8 +223,8 @@ because `LOOP.md` refers to items by number. ## Notes - `%` is real modulo in Wurst; `mod` is integer modulo. `int % 8` types as `real`. -- Emitted Lua must be byte-identical for identical input (AGENTS.md §8). It is the only - emitted output that can be diffed across runs — see item 11. +- Emitted Lua must be byte-identical for identical input (AGENTS.md §8). Emitted Jass can be + diffed across runs too now, since item 11 — `LOOP.md` still says otherwise. - Two of this run's reverts were the same mistake: a name that looks redundant is usually carrying a distinction. The mangled method name separates overloads; `leftType` on `div` keeps a literal assignable to a real. Check what a name distinguishes before replacing it with a tidier one. diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/WurstCompilerJassImpl.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/WurstCompilerJassImpl.java index f15fc9d12..9d4708c1b 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/WurstCompilerJassImpl.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/WurstCompilerJassImpl.java @@ -634,6 +634,9 @@ public ImTranslator getImTranslator() { public @Nullable ImProg translateProgToIm(WurstModel root) { beginPhase(1, "to intermediate lang"); + // Names of generated temporaries are counted from here, so that the same source compiles + // to the same script whatever was compiled before it in this process. + Flatten.resetTempVarCounters(); // translate wurst to intermediate lang: imTranslator = new ImTranslator(root, errorHandler.isUnitTestMode(), runArgs); imProg = getImTranslator().translateProg(); diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/Flatten.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/Flatten.java index f2becc2ef..34c69dac8 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/Flatten.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/Flatten.java @@ -52,6 +52,22 @@ public class Flatten { private static final ThreadLocal andLeftVarCounter = ThreadLocal.withInitial(() -> 0); private static final ThreadLocal tupleTempVarCounter = ThreadLocal.withInitial(() -> 0); + /** + * Starts temporary names from zero again for a new compilation. + *

+ * The counters are per thread and were never reset, so the name a temporary got depended on how + * many programs had been compiled before it on that thread — the same source emitted {@code + * temp0} alone and {@code temp70} after other work, which is why generated Jass could not be + * compared across runs. They still run on through one compilation, because flattening happens + * again after each optimisation and restarting mid-compilation would name two locals of one + * function alike. + */ + public static void resetTempVarCounters() { + tempVarCounter.set(0); + andLeftVarCounter.set(0); + tupleTempVarCounter.set(0); + } + private static String getTempVarName() { int count = tempVarCounter.get(); tempVarCounter.set(count + 1); diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/DeterministicChecks.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/DeterministicChecks.java index 786dbb818..93df7ee9f 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/DeterministicChecks.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/DeterministicChecks.java @@ -40,6 +40,48 @@ private void run(Runnable example, String name) throws IOException { assertEquals(script1, script2); } + /** + * The same source has to emit the same script whatever was compiled before it. Names of + * generated temporaries used to be counted per thread and never reset, so a program compiled + * alone got {@code temp0} and the same program compiled after other work got {@code temp70} — + * which is what made generated Jass impossible to compare across runs. Compiling something + * else in between is the part that matters here; two runs on their own would agree either way. + * The inlining configuration is the one that emits a temporary for this source. + */ + @Test + public void temporaryNamesDoNotDependOnEarlierCompilations() throws IOException { + ErrorHandler.outputTestSource = true; + try { + usesTemporaries(); + String first = Files.toString( + new File("test-output/DeterministicChecks_usesTemporaries_inl.j"), Charsets.UTF_8); + + exampleCode(); + cycleExample(); + + usesTemporaries(); + String afterOtherWork = Files.toString( + new File("test-output/DeterministicChecks_usesTemporaries_inl.j"), Charsets.UTF_8); + + assertEquals(first, afterOtherWork); + } finally { + ErrorHandler.outputTestSource = false; + } + } + + /** Nested calls in one expression are what makes the flattener allocate temporaries. */ + private void usesTemporaries() { + testAssertOkLines(false, + "package test", + "native testSuccess()", + "function f(int x) returns int", + " return x + 1", + "init", + " if f(f(1)) + f(f(2)) == 8", + " testSuccess()" + ); + } + private void exampleCode() { testAssertOkLines(false, "package test", From 4ef22da2453409a5063ca5b1e43a23194e0f1a0d Mon Sep 17 00:00:00 2001 From: Frotty Date: Sat, 15 Aug 2026 10:46:06 +0200 Subject: [PATCH 02/13] Run the test suite in parallel and stop paying for the stdlib five times The suite ran one test at a time on an eight core machine: 786s of test time, 13m11s of wall clock. It now forks, which needs two things to actually help. Forks rather than threads, because the harness keeps state in statics - the current test environment, the global caches, the extracted lua binaries - and a fork gets its own copy. Gradle hands out whole classes, so DeterministicChecks, which writes fixed file names, stays inside one fork. And a processor count per fork, because each one otherwise sizes its garbage collector and compiler threads for the whole machine. Eight forks doing that made every test about three times slower and gave back only a third of the parallelism: 8m34s. Capping them takes it to 7m03s. ExportToWurstTest was the slowest class at 108s, and none of it was the object data work - thirty of its thirty-two tests take no measurable time. Two tests looped over five cases and compiled each one separately against the whole standard library. The cases declare different objects and only the compile needs the library, so they are compiled together now: 108s to 67s. That class is worth the attention because wall clock cannot fall below the slowest single class. --- BACKLOG.md | 12 ++++++++++++ de.peeeq.wurstscript/build.gradle | 13 +++++++++++++ .../tests/wurstscript/tests/ExportToWurstTest.java | 13 +++++++++++-- 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/BACKLOG.md b/BACKLOG.md index 818dee7fa..ddf13597c 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -222,6 +222,18 @@ because `LOOP.md` refers to items by number. ## Notes +- Where the suite's 13 minutes went, measured from `build/test-results/test/TEST-*.xml`: 786s of + test time across 79 classes and 1663 tests, so effectively all of it is the tests themselves + rather than the build. The top ten classes are 61% of it, led by `ExportToWurstTest` at 108s, + then `OptimizerTests` 67s, `BugTests` 66s, `RealWorldExamples` 54s, + `GenericsWithTypeclassesTests` 45s. Gradle hands whole classes to forks, so wall time cannot go + below the slowest class — 108s is the floor until `ExportToWurstTest` is split. + Below that floor the remaining costs, in the order worth attacking: 401 tests compile their + program five times (each Jass configuration) and spawn `pjass.exe` per configuration, which + re-parses `common.j` and `blizzard.j` every time; and 102 `withStdLib` sites re-parse 169 stdlib + files because `GlobalCaches.clearAll()` runs before and after every method. Both change what the + tests actually verify, so neither is a free win the way the scheduling was. + - `%` is real modulo in Wurst; `mod` is integer modulo. `int % 8` types as `real`. - Emitted Lua must be byte-identical for identical input (AGENTS.md §8). Emitted Jass can be diffed across runs too now, since item 11 — `LOOP.md` still says otherwise. diff --git a/de.peeeq.wurstscript/build.gradle b/de.peeeq.wurstscript/build.gradle index f4fa668bb..8898113ca 100644 --- a/de.peeeq.wurstscript/build.gradle +++ b/de.peeeq.wurstscript/build.gradle @@ -238,12 +238,25 @@ tasks.named('compileJava') { it.dependsOn('gen') } test { useTestNG() + // The suite is a few thousand independent compilations and was running one at a time, so it + // took as long as the sum of them. Forks rather than threads: the harness keeps state in + // statics (the current test environment, the global caches, the extracted lua binaries), and a + // fork gets its own copy of all of it. Gradle hands out whole classes, so a class that writes + // fixed file names stays inside one fork. + // Wall time cannot fall below the slowest single class, which is why this does not need to be + // every core to get most of the win. + maxParallelForks = Math.max(1, (int) (Runtime.runtime.availableProcessors() / 2)) + jvmArgs( '-Xmx2g', // local: give it room to finish and dump '-XX:MaxMetaspaceSize=256m', '-XX:+HeapDumpOnOutOfMemoryError', '-XX:+UnlockExperimentalVMOptions', // needed for UseCompactObjectHeaders until it graduates '-XX:+UseCompactObjectHeaders', // Java 24+: 8-byte headers (vs 16) — big win for AST-heavy workloads + // Each fork otherwise sizes its garbage collector and compiler threads for the whole + // machine, so running several at once oversubscribes it badly: eight forks made every + // test about three times slower and gave back only a third of the parallelism. + '-XX:ActiveProcessorCount=2', ) } diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ExportToWurstTest.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ExportToWurstTest.java index 5eebb70b4..ea290db97 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ExportToWurstTest.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ExportToWurstTest.java @@ -12,6 +12,7 @@ import org.testng.annotations.Test; import java.io.IOException; +import java.util.ArrayList; import java.util.List; import static org.testng.Assert.*; @@ -132,6 +133,7 @@ public void abilityAliasBaseIdsUseAliasSpecificWrapperClasses() throws IOExcepti "Eah1", ObjMod.ValType.UNREAL, 1, 1, 0.1, "..setDamageDealttoAttackers(1, 0.1)"} }; + List exports = new ArrayList<>(); for (Object[] c : cases) { W3A w3a = new W3A(); String newId = (String) c[0]; @@ -147,8 +149,12 @@ public void abilityAliasBaseIdsUseAliasSpecificWrapperClasses() throws IOExcepti assertTrue(out.contains((String) c[9]), baseId + " export:\n" + out); assertFalse(out.contains("new " + implementationCodeClass + "("), baseId + " must not export via implementation-code class:\n" + out); assertFalse(out.contains("createObjectDefinition"), baseId + " must not fall back to raw export:\n" + out); - assertExportCompiles(out, "import AbilityObjEditing"); + exports.add(out); } + // All five at once: each is a compile against the whole standard library, and the cases + // declare different objects, so compiling them together checks the same code for a fifth + // of the time. + assertExportCompiles(String.join("\n", exports), "import AbilityObjEditing"); } @Test @@ -679,6 +685,7 @@ public void abilityIntegerLevelFieldsWithWrapperMethodsCompile() throws IOExcept {"AHca", "Hca4", 4, "AbilityDefinitionRangerColdArrows", "setStackFlags", "Zh04"}, }; + List exports = new ArrayList<>(); for (Object[] c : cases) { W3A w3a = new W3A(); W3A.Obj obj = w3a.addObj(ObjId.valueOf((String) c[5]), ObjId.valueOf((String) c[0])); @@ -688,8 +695,10 @@ public void abilityIntegerLevelFieldsWithWrapperMethodsCompile() throws IOExcept assertTrue(out.contains("new " + c[3] + "('" + c[5] + "')"), out); assertTrue(out.contains(".." + c[4] + "(1, 0)"), out); - assertExportCompiles(out, "import AbilityObjEditing"); + exports.add(out); } + // One compile for all five, as above. + assertExportCompiles(String.join("\n", exports), "import AbilityObjEditing"); } // ------------------------------------------------------------------------- From 5773d1c60f5de66e1ae8f43154a78b05b46fe37f Mon Sep 17 00:00:00 2001 From: Frotty Date: Sat, 15 Aug 2026 11:18:01 +0200 Subject: [PATCH 03/13] Pin where subclassing a bounded generic stops, and why MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The override's super.size(extra) becomes a direct call to the superclass implementation, and that call carries no type arguments, because the type variables belong to the class rather than to the method. Nothing specialises it, so once the class is specialised and Box_size is replaced by Box_size⟪integer⟫, the super call points at what was removed. That is the same gap as the constructor case: addMemberTypeArguments visits method calls and member accesses, and nothing else. Constructor calls and super calls both go without. Whoever closes one should close the other. Also tried and reverted giving Jass the dangling-reference check the Lua backend has, which would have named the removed function instead of what happened to be inside it. The backends disagree on which functions exist: Jass is handed getCalledFunctions() and emits whatever is called, and three passing tests rely on a closure's construct_Lazy being detached and still called. The invariant is Lua's, not a shared one, so it stays where it was. --- BACKLOG.md | 38 ++++++++++++---- .../wurstscript/tests/TypeClassTests.java | 43 +++++++++++++++++++ 2 files changed, 73 insertions(+), 8 deletions(-) diff --git a/BACKLOG.md b/BACKLOG.md index ddf13597c..2018919e0 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -72,8 +72,8 @@ because `LOOP.md` refers to items by number. source parameter. Making the node canonical lets all three compare by identity and removes a class of silent wrong dispatch. Mechanical, well covered by the suite. -13. **A bounded generic class cannot be subclassed.** Found while building a repro for item 3; - both backends break, differently, on the same program: +13. **A bounded generic class cannot be subclassed.** Diagnosed on the Jass side and pinned by + `TypeClassTests.subclassOfBoundedGenericIsRejected`; the Lua half is still open. The program: interface Show function show(T x) returns int @@ -99,12 +99,19 @@ because `LOOP.md` refers to items by number. if b.size(1) == 6 and b.shift(1) == 1001 and s.size(1) == 106 testSuccess() - Jass fails to compile: `Typevar dispatch not eliminated.` Lua compiles and runs but never - reaches `testSuccess`: the override makes `size` dispatched, and the emitted call is - `b:Box_size_specialized_integer(1)` while `b` was allocated from the *erased* `Box` table, - which binds only `shift`. The specialised table `Box_specialized_integer` has the slot; the - instance never gets that table. Split this if the two turn out to have separate causes — - the Jass one is loud and probably the smaller of the two. + On Jass the override's `super.size(extra)` becomes a direct call to the superclass + implementation, and that call carries no type arguments, because the type variables belong to + the class rather than to the method. Nothing specialises it, so when the class is specialised + and `Box_size` is replaced by `Box_size⟪integer⟫`, the super call is left pointing at what was + removed. That is the same gap as item 6: class type arguments reach method calls and member + accesses (`addMemberTypeArguments` visits exactly those two), but not constructor calls and not + super calls. Whoever fixes one should look at the other — a single collection point that also + covers direct calls to a generic class's own functions would close both. + + Lua compiles and runs but never reaches `testSuccess`: the override makes `size` dispatched, + and the emitted call is `b:Box_size_specialized_integer(1)` while `b` was allocated from the + *erased* `Box` table, which binds only `shift`. The specialised table has the slot; the instance + never gets that table. That half is the erasure question again, as in item 5. 12. **Standing item, never finished.** When nothing above is left, find the next thing worth doing and add it here rather than stopping. Good sources, in order: a test that would have @@ -148,6 +155,15 @@ because `LOOP.md` refers to items by number. ## Done +- 19. Tried giving Jass the dangling-reference check Lua has, and reverted it. The two backends do + not agree on which functions exist: `LuaTranslator` requires every reference to be rooted in the + program, while `ImToJassTranslator` is handed `getCalledFunctions()` and emits whatever is + called, rooted or not. Three passing tests rely on that — a closure's `construct_Lazy` is + detached from the program and still called — so the invariant is Lua's rather than universal. + Worth knowing when reading a Jass error: a function a pass detached is still translated, so the + error names what was inside it rather than the reference that kept it alive, which is exactly how + item 13 shows up. (Requiring natives to be rooted also fails: `$debugPrint` is built with + `IS_NATIVE, IS_BJ` and deliberately never added.) - 11. Generated Jass can be compared across runs. The counters naming temporaries were per thread and never reset, so a name depended on how much had been compiled before it: the same source gave `temp0` alone and `temp70` after other tests, measured directly rather than assumed. They now @@ -222,6 +238,12 @@ because `LOOP.md` refers to items by number. ## Notes +- Fork count is worth measuring rather than reasoning about, because two effects pull against each + other: more forks means more parallelism but also more contention, and every test gets slower. + Measured on eight cores, whole suite, wall clock against total reported test time: + serial 13m11s / 786s; four forks 8m39s / 1230s; eight forks 7m03s / 2048s. Eight wins even + though each test runs 2.6 times slower there than alone. Sixteen was not tried; the limit by + then is the slowest single class, not the scheduling. - Where the suite's 13 minutes went, measured from `build/test-results/test/TEST-*.xml`: 786s of test time across 79 classes and 1663 tests, so effectively all of it is the tests themselves rather than the build. The top ten classes are 61% of it, led by `ExportToWurstTest` at 108s, diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java index 656aabfa7..fe4aa8c22 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java @@ -182,6 +182,49 @@ public void unwrittenArrayOfATypeParameterReadsAsItsDefault() { ); } + private static final String[] SUBCLASS_OF_BOUNDED_GENERIC = { + "package test", + "native testSuccess()", + "interface Show", + " function show(T x) returns int", + "implements Show", + " function show(int x) returns int", + " return x", + "class Box", + " K key", + " construct(K k)", + " key = k", + " function size(int extra) returns int", + " return K.show(key) + extra", + "class SubBox extends Box", + " construct(int k)", + " super(k)", + " override function size(int extra) returns int", + " return super.size(extra) + 100", + "init", + " Box b = new Box(5)", + " Box s = new SubBox(5)", + " if b.size(1) == 6 and s.size(1) == 106", + " testSuccess()", + }; + + /** + * Subclassing a bounded generic class does not work yet, and this pins where it stops. The + * override's {@code super.size(extra)} becomes a direct call to the superclass implementation, + * and that call carries no type arguments — the type variables belong to the class, not to the + * method — so nothing specialises it. Specialising the class replaces the original with + * {@code Box_size⟪integer⟫}, and the super call is left pointing at what was removed. + *

+ * The same shape as the constructor case above: class type arguments reach method calls and + * member accesses, but not constructor calls or super calls. Should either be fixed, look at + * both. The message names what was inside the function rather than the reference that kept it + * alive, because Jass emits whatever is called rather than only what the program still holds. + */ + @Test + public void subclassOfBoundedGenericIsRejected() { + testAssertErrorsLines(false, "Typevar dispatch not eliminated", SUBCLASS_OF_BOUNDED_GENERIC); + } + /** Each type argument picks its own instance, so one generic serves several types. */ @Test public void twoInstancesOfOneClass() { From bd64d0842399d283a09b768976e14d7bb51acc0d Mon Sep 17 00:00:00 2001 From: Frotty Date: Sat, 15 Aug 2026 11:21:38 +0200 Subject: [PATCH 04/13] Record what the subclass fix actually needs Tried the obvious reading of the previous commit's diagnosis: extend addMemberTypeArguments so a super call gets the receiver's type arguments the way a method call does. It changes nothing, and the reason is worth writing down. The callee has no type variables of its own - they belong to the class - so type arguments on the call have nothing to select. A class function is specialised by copying the whole class, so the call has to be redirected to that copy rather than annotated. Which also separates this from the constructor case it was grouped with: there the instantiation is not on any argument either, only on the type of what the call is assigned to. Same symptom, two different fixes. --- BACKLOG.md | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/BACKLOG.md b/BACKLOG.md index 2018919e0..6ece56210 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -100,13 +100,21 @@ because `LOOP.md` refers to items by number. testSuccess() On Jass the override's `super.size(extra)` becomes a direct call to the superclass - implementation, and that call carries no type arguments, because the type variables belong to - the class rather than to the method. Nothing specialises it, so when the class is specialised - and `Box_size` is replaced by `Box_size⟪integer⟫`, the super call is left pointing at what was - removed. That is the same gap as item 6: class type arguments reach method calls and member - accesses (`addMemberTypeArguments` visits exactly those two), but not constructor calls and not - super calls. Whoever fixes one should look at the other — a single collection point that also - covers direct calls to a generic class's own functions would close both. + implementation, and once `Box` is specialised that call points at a function which has been + replaced by `Box_size⟪integer⟫`. The `.jim` for the test shows both side by side: the + specialised copy with its dispatch resolved, and `SubBox_size` still calling the original. + + Tried extending `addMemberTypeArguments` to attach the receiver's type arguments to such calls, + the way it already does for method calls and member accesses, and it changes nothing. The + callee has no type variables of its own — they belong to the class — so there is nothing for + type arguments on the call to select, and specialisation of a class function happens by + `specializeClass` copying the whole class instead. The call has to be **redirected** to that + copy, not annotated. The natural place is wherever a class is specialised: every call from a + subclass into a superclass function needs to follow. + + Item 6 is the same family but not the same fix: there the constructor call also carries no type + arguments, and there the instantiation is not on any argument either, only on the type of what + the call is assigned to. Lua compiles and runs but never reaches `testSuccess`: the override makes `size` dispatched, and the emitted call is `b:Box_size_specialized_integer(1)` while `b` was allocated from the From b9fda9d610883f9132ea8c05bae89d938d07f4b5 Mon Sep 17 00:00:00 2001 From: Frotty Date: Sun, 16 Aug 2026 09:21:48 +0200 Subject: [PATCH 05/13] Sanitise Lua method names where they are assigned (#1230) --- .gitignore | 2 + BACKLOG.md | 115 ++++++++++ .../imtranslation/LuaDispatchPreparation.java | 5 +- .../lua/translation/LuaAssertions.java | 70 ++++++ .../lua/translation/LuaIdentifiers.java | 67 ++++++ .../lua/translation/LuaTranslator.java | 45 ++-- .../wurstscript/tests/FastHashMapTests.java | 207 ++++++++++++++++++ .../wurstscript/tests/TypeClassTests.java | 20 ++ .../wurstscript/tests/WurstScriptTest.java | 2 + 9 files changed, 505 insertions(+), 28 deletions(-) create mode 100644 BACKLOG.md create mode 100644 de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaIdentifiers.java create mode 100644 de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/FastHashMapTests.java diff --git a/.gitignore b/.gitignore index 7b7bf5ad3..fad3ad29c 100644 --- a/.gitignore +++ b/.gitignore @@ -60,3 +60,5 @@ de.peeeq.wurstscript/output.txt /HelperScripts/.gradle /.gradle-user-home /gradle-home-temp +/.claude/ +/LOOP.md diff --git a/BACKLOG.md b/BACKLOG.md new file mode 100644 index 000000000..ba7ec03be --- /dev/null +++ b/BACKLOG.md @@ -0,0 +1,115 @@ +# Language feature backlog + +Working notes for ongoing work on type class bounds for `T:` generics +(shipped in #1226, #1228, #1229). Ordered: take the top unblocked item. + +Keep this file current. It is the only memory that survives between sessions. +When an item is finished, move it to Done with one line on what actually +happened. When something is learned that would have saved time, write it under +Notes rather than leaving it in a commit message. + +## Todo + +Numbering is stable: finished items leave a gap rather than shifting the ones below, +because `LOOP.md` refers to items by number. + +3. **`slotFor` is bound to `get`'s implementation** in the emitted Lua — confirmed real, and + not an artefact of item 1: it survives sanitisation unchanged. Diagnosed, not yet fixed. + The alias sets in `LuaDispatchPreparation` decide which slots a method claims, and + `sharesSemanticName` accepts a match on *either* of two names: the source name (`get`, + `slotFor`) or `semanticNameFromMethodName`, which is the substring after the last + underscore. For a specialised method that substring is a type-argument fragment — + `FastHashMap_get_specialized__integer__integer___integer` and the `slotFor` one both yield + `integer` — so two unrelated methods count as sharing a name. They also share a dispatch + signature here (`(pos) returns int` both), which is the other half of the guard, so `get` + claims `slotFor`'s slot. Fix: when both methods have a real source name, that should decide; + the substring heuristic is a fallback for when there is no trace to ask, not an alternative. + Watch the closure and bridge cases in `TypeClassTests`/`LuaBackendAuditTests` — they are what + the loose match was presumably widened for. + +4. **Finish the FastHashMap proof.** `FastHashMapTests` is the first real use of bounds. + Add `remove` with tombstones, and an assertion that the emitted code stays cheap: no + dispatch node, no instance dictionary, and no WC3 hashtable natives — array access only, + which is the whole point versus `HashMap extends Table`. + +5. **Lua dispatch inside a closure.** Works on Jass since #1229. On Lua the specialised class + is built correctly but nothing calls it, because the closure is reached through its + interface and `specializeMethod` renames the method out of its dispatch slot. + `TypeClassTests.dispatchInsideClosureIsRejectedForLua` pins the current diagnostic and + should become a success test. Related to item 1; AGENTS.md flags this machinery. + +6. **Lua dispatch inside the constructor** of a bounded generic class. Works on Jass. + +7. **Module bounds.** `module M` is rejected with a clear message today. Needs + receiver rewriting during expansion, or type parameters on `ModuleInstanciation`. + +8. **`MOD_INT`/`DIV_INT` return the left operand's type** rather than `int` + (`AttrExprType.java`, the `case MOD_INT` branch), where `caseMathOperation` returns + `WurstTypeInt.instance()` for `+`, `-`, `*`. Only observable if something is a proper + subtype of int, so it may be harmless — establish whether it is reachable, then either fix + it or leave a comment saying why the asymmetry is intended. Small. + +9. **Keep `WURST_LANGUAGE.md` and `CHANGELOG.md` current** as items land. The bounds section + says nothing about closures, which now work on Jass. Fold this into whichever item changes + the behaviour rather than doing it as a separate pass. + +10. **One `ImTypeVar` per type parameter.** Name-tolerant lookups remain in + `EliminateGenerics.indexOfTypeVar`, `inheritTypeClassBinding` and + `ProgramState.getCurrentTypeArgument`, compensating for several nodes standing for one + source parameter. Making the node canonical lets all three compare by identity and removes + a class of silent wrong dispatch. Mechanical, well covered by the suite. + +11. **Jass temp counter is not reset between compilations.** Two runs of the same commit emit + different `.j` (`temp151` vs `temp8`) because the counter is JVM-wide and depends on how + many tests ran before. Not wrong for compiling one map, but it means `.j` cannot be diffed + across runs to validate a change — only `.lua` can. Fixing it would make Jass diffable. + +12. **Standing item, never finished.** When nothing above is left, find the next thing worth + doing and add it here rather than stopping. Good sources, in order: a test that would have + caught a bug already found; a place where two mechanisms do the same job and disagree; a + comment claiming something the code no longer does; a path where a wrong result is silent + rather than loud. Add what is found as a numbered item and start on it. + +## Blocked on a decision + +- **Eliminating the remaining `castTo int`.** The motivating case is timer data attachment + (`ClosureTimers.wurst`), and the containers behind it: `Table` has 81 casts, `HashList` 13, + `HashSet` 6, `HashMap` 4. None can adopt bounds as things stand, because an instance is + declared one type at a time and these accept any type. It needs a way to give an instance + for a whole family — every class type, or every handle type — which is a language design + question: what the syntax is, where such an instance may be declared under the orphan rule, + and whether a specific instance always beats a family one. Do not start this autonomously. + +## Out of scope + +- The stdlib itself. `de.peeeq.wurstscript/temp/WurstStdlib2` is a fetched artefact for tests; + editing it changes nothing real. `FastHashMap` ships from the WurstStdlib2 repo once the + compiler-side proof is complete, and that is a separate decision. + +## Done + +- 1 + 2. Lua method names are sanitised where they are assigned, not where they are printed. + `LuaDispatchPreparation.normalizeMethodNames` is the pass that gives one name to a whole + dispatch group, so it now sanitises before uniquing — two names differing only in characters + Lua has no place for still get a slot each. `LuaTranslator` maps every slot key and every + `LuaMethod` name through the same function, so call sites and class tables agree. Lua's + identifier rule now lives in one place, `LuaIdentifiers`. `LuaAssertions.assertNamesAreValidIdentifiers` + walks the emitted Lua and fails on any name that is not an identifier; it runs for every + `testLua` compile, so the silent two-target-assignment case cannot come back. +- Substitution now carries the type class binding with the type (#1229). Also fixed the + type-variable reference on `ImTypeVarDispatch`, which a walk over types alone missed. + +## Notes + +- `%` is real modulo in Wurst; `mod` is integer modulo. `int % 8` types as `real`. +- Emitted Lua must be byte-identical for identical input (AGENTS.md §8). It is the only + emitted output that can be diffed across runs — see item 11. +- Method names are not what the frontend called them. `LuaDispatchPreparation` renames a whole + dispatch group to one name and attaches alias sets, and only then does the backend run. A + question about which Lua slot something lands in is a question about that pass, not about + `LuaTranslator`. +- Tests run five Jass configurations plus the interpreter, then the Lua target separately. + `testAssertOkLines(true, ...)` covers both the pre-transform interpreter and full + monomorphisation, so it is a stronger check than it looks. +- The stdlib copy under `de.peeeq.wurstscript/temp/WurstStdlib2` is a fetched artefact for + tests. Real stdlib changes belong in the WurstStdlib2 repo, not here. diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaDispatchPreparation.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaDispatchPreparation.java index 662c812b9..cbed8db1b 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaDispatchPreparation.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaDispatchPreparation.java @@ -11,6 +11,7 @@ import de.peeeq.wurstscript.jassIm.ImProg; import de.peeeq.wurstscript.jassIm.ImType; import de.peeeq.wurstscript.jassIm.ImVars; +import de.peeeq.wurstscript.translation.lua.translation.LuaIdentifiers; import java.util.ArrayList; import java.util.Collection; @@ -99,7 +100,9 @@ private static void normalizeMethodNames(ImProg prog, List allMethods) continue; } group.sort(Comparator.comparing(LuaDispatchPreparation::methodSortKey)); - String name = uniqueName(group.get(0).getName(), usedNames); + // The name is about to become a Lua table key. Sanitising before uniquing means two + // names that only differed in characters Lua has no place for still get one slot each. + String name = uniqueName(LuaIdentifiers.toIdentifier(group.get(0).getName()), usedNames); for (ImMethod method : group) { method.setName(name); } diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaAssertions.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaAssertions.java index 82d003d60..4c1ed5298 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaAssertions.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaAssertions.java @@ -1,9 +1,19 @@ package de.peeeq.wurstscript.translation.lua.translation; +import de.peeeq.wurstscript.luaAst.Element; +import de.peeeq.wurstscript.luaAst.LuaCompilationUnit; +import de.peeeq.wurstscript.luaAst.LuaExprFieldAccess; +import de.peeeq.wurstscript.luaAst.LuaExprFunctionCallByName; +import de.peeeq.wurstscript.luaAst.LuaFunction; +import de.peeeq.wurstscript.luaAst.LuaMethod; +import de.peeeq.wurstscript.luaAst.LuaTableNamedField; +import de.peeeq.wurstscript.luaAst.LuaVariable; + import java.util.ArrayList; import java.util.HashSet; import java.util.List; import java.util.Set; +import java.util.TreeSet; /** * Static assertion helpers for the Lua backend. @@ -15,6 +25,66 @@ public class LuaAssertions { private LuaAssertions() {} + /** + * Asserts that every name the backend emits is a Lua identifier. + * + *

A name that is not one usually breaks the syntax check, but not always: a table key + * containing a comma parses as an assignment to two targets and quietly stores the value in + * the wrong place. Checking the names themselves catches that case at the point it is + * introduced, rather than as a wrong result at runtime. + */ + public static void assertNamesAreValidIdentifiers(LuaCompilationUnit luaCode) { + Set invalid = new TreeSet<>(); + luaCode.accept(new Element.DefaultVisitor() { + private void check(String kind, String name) { + // A vararg parameter is the one name that is legal without being an identifier. + if (!LuaIdentifiers.isValid(name) && !LuaIdentifiers.VARARG.equals(name)) { + invalid.add(kind + " '" + name + "'"); + } + } + + @Override + public void visit(LuaFunction f) { + super.visit(f); + check("function", f.getName()); + } + + @Override + public void visit(LuaMethod m) { + super.visit(m); + check("method", m.getName()); + } + + @Override + public void visit(LuaVariable v) { + super.visit(v); + check("variable", v.getName()); + } + + @Override + public void visit(LuaExprFieldAccess fa) { + super.visit(fa); + check("field", fa.getFieldName()); + } + + @Override + public void visit(LuaTableNamedField f) { + super.visit(f); + check("field", f.getFieldName()); + } + + @Override + public void visit(LuaExprFunctionCallByName call) { + super.visit(call); + check("call to", call.getFuncName()); + } + }); + if (!invalid.isEmpty()) { + throw new RuntimeException("Wurst Lua backend assertion failed: emitted names are not Lua identifiers: " + + String.join(", ", invalid)); + } + } + /** * Asserts that every emitted call to {@code __wurst_GetHandleId} has a helper definition. * diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaIdentifiers.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaIdentifiers.java new file mode 100644 index 000000000..b0da4381f --- /dev/null +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaIdentifiers.java @@ -0,0 +1,67 @@ +package de.peeeq.wurstscript.translation.lua.translation; + +/** + * Lua's rule for what a generated name may look like, kept in one place. + * + *

Names from the intermediate language are not constrained to Lua's identifier syntax; + * specialised generics, for example, are named after their type arguments. Sanitising in the + * backend keeps that rule where it belongs rather than requiring every earlier pass to know + * about Lua. Any collisions the mapping introduces are resolved by the usual uniquing. + */ +public final class LuaIdentifiers { + + /** Lua's vararg parameter, which is a legal parameter name but not an identifier. */ + public static final String VARARG = "..."; + + /** + * Whether {@code name} can be used as-is as a Lua identifier or table key. + * + *

A keyword is spelled like an identifier and is not one. Wurst reserves a different set, so + * a method can be declared {@code repeat} or {@code goto} and reach the backend under that + * name; emitted as a table key it is a syntax error rather than a wrong result, but this is the + * check that is supposed to catch it first. + */ + public static boolean isValid(String name) { + if (name == null || name.isEmpty() || isDigit(name.charAt(0))) { + return false; + } + for (int i = 0; i < name.length(); i++) { + if (!isIdentifierPart(name.charAt(i))) { + return false; + } + } + return !LuaReservedNames.LUA_KEYWORDS.contains(name); + } + + /** Maps any name onto a Lua identifier, leaving names that already are one untouched. */ + public static String toIdentifier(String name) { + if (isValid(name)) { + return name; + } + StringBuilder sb = new StringBuilder(name.length()); + for (int i = 0; i < name.length(); i++) { + char c = name.charAt(i); + sb.append(isIdentifierPart(c) ? c : '_'); + } + if (sb.length() == 0 || isDigit(sb.charAt(0))) { + sb.insert(0, '_'); + } + // A trailing underscore rather than a counter, so the name a keyword maps to is the same + // wherever it is derived - call sites and class tables have to agree without consulting + // each other. + while (LuaReservedNames.LUA_KEYWORDS.contains(sb.toString())) { + sb.append('_'); + } + return sb.toString(); + } + + private static boolean isIdentifierPart(char c) { + return c == '_' || (c < 128 && Character.isLetterOrDigit(c)); + } + + private static boolean isDigit(char c) { + return c >= '0' && c <= '9'; + } + + private LuaIdentifiers() {} +} diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaTranslator.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaTranslator.java index 4e729a863..f00242efa 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaTranslator.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaTranslator.java @@ -135,7 +135,9 @@ public LuaFunction initFor(ImFunction a) { @Override public LuaMethod initFor(ImMethod a) { LuaExpr receiver = LuaAst.LuaExprVarAccess(luaClassVar.getFor(a.attrClass())); - return LuaAst.LuaMethod(receiver, a.getName(), LuaAst.LuaParams(), LuaAst.LuaStatements()); + // A method name is a table key, so it must be an identifier - but unlike a variable + // it must not be uniqued: every override has to keep landing in the same slot. + return LuaAst.LuaMethod(receiver, dispatchSlotName(a.getName()), LuaAst.LuaParams(), LuaAst.LuaStatements()); } }; @@ -196,28 +198,8 @@ public LuaTranslator(ImProg prog, ImTranslator imTr) { luaModel = LuaAst.LuaCompilationUnit(); } - /** - * Makes an intermediate-language name usable as a Lua identifier. - *

- * Names from the IM are not constrained to Lua's identifier syntax; specialised generics, for - * example, are named after their type arguments. Sanitising here keeps that rule where it - * belongs, in the backend, rather than requiring every earlier pass to know about Lua. Any - * collisions the mapping introduces are resolved by the usual uniquing. - */ - private static String toLuaIdentifier(String name) { - StringBuilder sb = new StringBuilder(name.length()); - for (int i = 0; i < name.length(); i++) { - char c = name.charAt(i); - sb.append(c == '_' || Character.isLetterOrDigit(c) && c < 128 ? c : '_'); - } - if (sb.length() == 0 || Character.isDigit(sb.charAt(0))) { - sb.insert(0, '_'); - } - return sb.toString(); - } - protected String uniqueName(String rawName) { - String name = toLuaIdentifier(rawName); + String name = LuaIdentifiers.toIdentifier(rawName); Integer nextIndex = uniqueNameCounters.get(name); if (nextIndex == null) { uniqueNameCounters.put(name, 1); @@ -462,7 +444,7 @@ private void collectMethodNames(ImClass c, Set methodNames, Set } visited.add(c); for (ImMethod method : c.getMethods()) { - methodNames.add(method.getName()); + methodNames.add(dispatchSlotName(method.getName())); } for (ImClassType sc : c.getSuperClasses()) { collectMethodNames(sc.getClassDef(), methodNames, visited); @@ -914,7 +896,7 @@ private void createMethods(ImClass c, LuaVariable classVar) { ImMethod chosen = chosenByGroup.get(groupMethods); Set memberNames = new HashSet<>(); for (ImMethod m : groupMethods) { - memberNames.add(m.getName()); + memberNames.add(dispatchSlotName(m.getName())); } Set slotNames = collectDispatchSlotNames(c, groupMethods); for (String slotName : slotNames) { @@ -934,7 +916,7 @@ private void createMethods(ImClass c, LuaVariable classVar) { ImMethod chosen = chosenByGroup.get(groupMethods); Set memberNames = new HashSet<>(); for (ImMethod m : groupMethods) { - memberNames.add(m.getName()); + memberNames.add(dispatchSlotName(m.getName())); } for (String slotName : collectDispatchSlotNames(c, groupMethods)) { if (memberNames.contains(slotName)) { @@ -973,6 +955,15 @@ && implArity(chosen) != implArity(current)) { } } + /** + * The Lua table key a dispatch slot is emitted under. Aliases and class-qualified names are + * built from IM names, which may contain characters Lua has no place for; the mapping has to + * be the same one call sites go through, so that a slot is still found under its new name. + */ + private String dispatchSlotName(String rawName) { + return LuaIdentifiers.toIdentifier(rawName); + } + private Set collectDispatchSlotNames(ImClass receiverClass, List groupMethods) { Set slotNames = new TreeSet<>(); Set semanticNames = new TreeSet<>(); @@ -982,7 +973,7 @@ private Set collectDispatchSlotNames(ImClass receiverClass, List collectDispatchSlotNames(ImClass receiverClass, List()); for (String className : classNames) { for (String semanticName : semanticNames) { - slotNames.add(className + "_" + semanticName); + slotNames.add(dispatchSlotName(className + "_" + semanticName)); } } } diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/FastHashMapTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/FastHashMapTests.java new file mode 100644 index 000000000..50becb1f3 --- /dev/null +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/FastHashMapTests.java @@ -0,0 +1,207 @@ +package tests.wurstscript.tests; + +import org.testng.annotations.Test; + +/** + * Exercises type class bounds through the container they were added for: a hash map whose key type + * is bounded by {@code Hashable}, so it can hash and compare keys without erasing them to int. + *

+ * This is deliberately a whole working container rather than a minimal repro. It combines things + * nothing else tests together: two type parameters where only the first is bounded, static arrays of + * a bounded type parameter, and a bound used from a private method reached through a public one. + */ +public class FastHashMapTests extends WurstScriptTest { + + /** + * Storage follows ArrayList: one array per specialisation, carved into a section per instance. + * Collision handling is linear probing, so a lookup calls both requirements of the bound. + */ + private static String[] fastHashMap(String... extra) { + String[] head = { + "package test", + "native testSuccess()", + "constant int CAPACITY = 8", + "interface Hashable", + " function hash(T x) returns int", + " function equals(T a, T b) returns boolean", + "class FastHashMap", + " private static K array keys", + " private static V array values", + " private static boolean array used", + " private static int nextFree = 0", + " private int base", + " private int count = 0", + " construct()", + " base = nextFree", + " nextFree += CAPACITY", + " private function slotFor(K key) returns int", + " var i = K.hash(key) mod CAPACITY", + " if i < 0", + " i += CAPACITY", + " while used[base + i] and not K.equals(keys[base + i], key)", + " i = (i + 1) mod CAPACITY", + " return base + i", + " function put(K key, V value)", + " let s = slotFor(key)", + " if not used[s]", + " used[s] = true", + " keys[s] = key", + " count++", + " values[s] = value", + " function get(K key) returns V", + " return values[slotFor(key)]", + " function has(K key) returns boolean", + " return used[slotFor(key)]", + " function size() returns int", + " return count", + }; + String[] all = new String[head.length + extra.length]; + System.arraycopy(head, 0, all, 0, head.length); + System.arraycopy(extra, 0, all, head.length, extra.length); + return all; + } + + private static final String[] INT_INSTANCE = { + "implements Hashable", + " function hash(int x) returns int", + " return x", + " function equals(int a, int b) returns boolean", + " return a == b", + }; + + private static String[] program(String[]... parts) { + int size = 0; + for (String[] part : parts) { + size += part.length; + } + String[] all = new String[size]; + int at = 0; + for (String[] part : parts) { + System.arraycopy(part, 0, all, at, part.length); + at += part.length; + } + return all; + } + + /** Keys 1 and 9 land in the same slot at capacity 8, so the probe path is taken. */ + private static final String[] USE_WITH_COLLISION = { + "init", + " let m = new FastHashMap()", + " m.put(1, 10)", + " m.put(9, 90)", + " m.put(2, 20)", + " if m.get(1) == 10 and m.get(9) == 90 and m.get(2) == 20", + " if m.size() == 3 and not m.has(3)", + " testSuccess()", + }; + + @Test + public void fastHashMapRuntime() { + testAssertOkLines(true, program(fastHashMap(), INT_INSTANCE, USE_WITH_COLLISION)); + } + + @Test + public void fastHashMapRuntimeLua() { + test().testLua(true).executeProg().lines(program(fastHashMap(), INT_INSTANCE, USE_WITH_COLLISION)); + } + + /** + * A tuple key, which is the case old generics cannot serve at all: a tuple has no int + * representation to cast to, so the only way to key a map by one is to say how it hashes. + */ + private static final String[] TUPLE_INSTANCE = { + "tuple pos(int x, int y)", + "implements Hashable", + " function hash(pos p) returns int", + " return p.x * 31 + p.y", + " function equals(pos a, pos b) returns boolean", + " return a.x == b.x and a.y == b.y", + }; + + @Test + public void tupleKey() { + testAssertOkLines(true, program(fastHashMap(), TUPLE_INSTANCE, new String[]{ + "init", + " let m = new FastHashMap()", + " m.put(pos(1, 2), 12)", + " m.put(pos(2, 1), 21)", + " if m.get(pos(1, 2)) == 12 and m.get(pos(2, 1)) == 21", + " if m.has(pos(1, 2)) and not m.has(pos(9, 9))", + " testSuccess()" + })); + } + + @Test + public void tupleKeyLua() { + test().testLua(true).executeProg().lines(program(fastHashMap(), TUPLE_INSTANCE, new String[]{ + "init", + " let m = new FastHashMap()", + " m.put(pos(1, 2), 12)", + " m.put(pos(2, 1), 21)", + " if m.get(pos(1, 2)) == 12 and m.get(pos(2, 1)) == 21", + " testSuccess()" + })); + } + + /** A key type of the user's own, with the instance declared in the same package as the type. */ + @Test + public void classKeyWithUserInstance() { + testAssertOkLines(true, program(fastHashMap(), new String[]{ + "class Item", + " int id", + " construct(int id)", + " this.id = id", + "implements Hashable", + " function hash(Item i) returns int", + " return i.id", + " function equals(Item a, Item b) returns boolean", + " return a.id == b.id", + "init", + " let m = new FastHashMap()", + " let a = new Item(1)", + " let b = new Item(2)", + " m.put(a, \"a\")", + " m.put(b, \"b\")", + " if m.get(a) == \"a\" and m.get(b) == \"b\" and m.size() == 2", + " testSuccess()" + })); + } + + /** + * Two specialisations live at once. Storage is static per specialisation, so this is what would + * break if the arrays of one instantiation were shared with another. + */ + @Test + public void twoSpecialisationsCoexist() { + testAssertOkLines(true, program(fastHashMap(), INT_INSTANCE, new String[]{ + // Every string hashes alike, so this also exercises the probe path on every lookup. + "implements Hashable", + " function hash(string s) returns int", + " return 0", + " function equals(string a, string b) returns boolean", + " return a == b", + "init", + " let ints = new FastHashMap()", + " let strs = new FastHashMap()", + " ints.put(1, 100)", + " strs.put(\"a\", 200)", + " if ints.get(1) == 100 and strs.get(\"a\") == 200", + " if ints.size() == 1 and strs.size() == 1", + " testSuccess()" + })); + } + + /** Two maps of the same instantiation must not share storage either. */ + @Test + public void twoInstancesOfOneSpecialisation() { + testAssertOkLines(true, program(fastHashMap(), INT_INSTANCE, new String[]{ + "init", + " let a = new FastHashMap()", + " let b = new FastHashMap()", + " a.put(1, 10)", + " b.put(1, 20)", + " if a.get(1) == 10 and b.get(1) == 20 and a.size() == 1 and b.size() == 1", + " testSuccess()" + })); + } +} diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java index 6e0ba186c..cd8429a4e 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java @@ -121,6 +121,26 @@ public void dispatchInsideClosureIsRejectedForLua() { ); } + /** + * Wurst and Lua reserve different words, so a method can be declared {@code repeat} and reach + * the backend under that name. A closure adds the name it implements as a dispatch alias + * directly, without the uniquing that protects method names, so the alias arrives as a bare + * keyword and is emitted as a table key — {@code expected near 'repeat'} from luac. + */ + @Test + public void closureImplementingALuaKeywordName() { + test().testLua(true).executeProg().lines( + "package test", + "native testSuccess()", + "interface Producer", + " function repeat() returns int", + "init", + " Producer p = () -> 42", + " if p.repeat() == 42", + " testSuccess()" + ); + } + /** Each type argument picks its own instance, so one generic serves several types. */ @Test public void twoInstancesOfOneClass() { diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/WurstScriptTest.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/WurstScriptTest.java index 3498cec3f..545f28c4d 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/WurstScriptTest.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/WurstScriptTest.java @@ -25,6 +25,7 @@ import de.peeeq.wurstscript.jassprinter.JassPrinter; import de.peeeq.wurstscript.luaAst.LuaCompilationUnit; import de.peeeq.wurstscript.luaAst.*; +import de.peeeq.wurstscript.translation.lua.translation.LuaAssertions; import de.peeeq.wurstscript.translation.imtranslation.ImTranslator; import de.peeeq.wurstscript.translation.imtranslation.RecycleCodeGeneratorQueue; import de.peeeq.wurstscript.utils.Utils; @@ -500,6 +501,7 @@ private void translateAndTestLua(String name, boolean executeProg, WurstGui gui, compiler.runCompiletime(WurstProjectConfigData.empty(), false, false); LuaCompilationUnit luaCode = compiler.transformProgToLua(); + LuaAssertions.assertNamesAreValidIdentifiers(luaCode); checkLuaRootPurity(luaCode); StringBuilder sb = new StringBuilder(); luaCode.print(sb, 0); From ce3a1ea48760004df0380eb15a8716d07d984a1e Mon Sep 17 00:00:00 2001 From: Frotty Date: Sun, 16 Aug 2026 09:44:07 +0200 Subject: [PATCH 06/13] Bound the wait on the Lua interpreter in tests (#1231) --- BACKLOG.md | 50 +++++++- .../wurstscript/tests/LuaRunnerTests.java | 85 ++++++++++++++ .../wurstscript/tests/WurstScriptTest.java | 110 ++++++++++++++---- 3 files changed, 220 insertions(+), 25 deletions(-) create mode 100644 de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaRunnerTests.java diff --git a/BACKLOG.md b/BACKLOG.md index ba7ec03be..7d6c0ad50 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -45,9 +45,11 @@ because `LOOP.md` refers to items by number. 8. **`MOD_INT`/`DIV_INT` return the left operand's type** rather than `int` (`AttrExprType.java`, the `case MOD_INT` branch), where `caseMathOperation` returns - `WurstTypeInt.instance()` for `+`, `-`, `*`. Only observable if something is a proper - subtype of int, so it may be harmless — establish whether it is reachable, then either fix - it or leave a comment saying why the asymmetry is intended. Small. + `WurstTypeInt.instance()` for `+`, `-`, `*`. It *is* reachable: `WurstTypeIntLiteral` is a + proper subtype of both int and real, and `caseMathOperation` collapses two literals to `int` + precisely so `real r = 1 + 1` stays an error. Returning `leftType` skips that collapse, so + `real r = 7 div 2` and `real r = 7 mod 2` should be accepted where `+` is rejected. Confirm + with a test first — that is the failing repro — then return `WurstTypeInt.instance()`. Small. 9. **Keep `WURST_LANGUAGE.md` and `CHANGELOG.md` current** as items land. The bounds section says nothing about closures, which now work on Jass. Fold this into whichever item changes @@ -64,6 +66,40 @@ because `LOOP.md` refers to items by number. many tests ran before. Not wrong for compiling one map, but it means `.j` cannot be diffed across runs to validate a change — only `.lua` can. Fixing it would make Jass diffable. +13. **A bounded generic class cannot be subclassed.** Found while building a repro for item 3; + both backends break, differently, on the same program: + + interface Show + function show(T x) returns int + implements Show + function show(int x) returns int + return x + class Box + K key + construct(K k) + key = k + function size(int extra) returns int + return K.show(key) + extra + function shift(int extra) returns int + return 1000 + extra + class SubBox extends Box + construct(int k) + super(k) + override function size(int extra) returns int + return super.size(extra) + 100 + init + Box b = new Box(5) + Box s = new SubBox(5) + if b.size(1) == 6 and b.shift(1) == 1001 and s.size(1) == 106 + testSuccess() + + Jass fails to compile: `Typevar dispatch not eliminated.` Lua compiles and runs but never + reaches `testSuccess`: the override makes `size` dispatched, and the emitted call is + `b:Box_size_specialized_integer(1)` while `b` was allocated from the *erased* `Box` table, + which binds only `shift`. The specialised table `Box_specialized_integer` has the slot; the + instance never gets that table. Split this if the two turn out to have separate causes — + the Jass one is loud and probably the smaller of the two. + 12. **Standing item, never finished.** When nothing above is left, find the next thing worth doing and add it here rather than stopping. Good sources, in order: a test that would have caught a bug already found; a place where two mechanisms do the same job and disagree; a @@ -88,6 +124,12 @@ because `LOOP.md` refers to items by number. ## Done +- 14. The Lua execution harness no longer hangs. It read the spawned interpreter's stderr to EOF + before touching stdout, and never bounded the wait, so a program that filled the stdout pipe + deadlocked the whole suite with no output and no timeout — a stray JVM was still sitting on it + twenty minutes later. `checkLuaSyntax`, ten lines further down the same file, already drained + both pipes concurrently and waited with a timeout; the execution path now uses the same + helper. Verified against the item 13 repro: hung indefinitely before, fails in 8s after. - 1 + 2. Lua method names are sanitised where they are assigned, not where they are printed. `LuaDispatchPreparation.normalizeMethodNames` is the pass that gives one name to a whole dispatch group, so it now sanitises before uniquing — two names differing only in characters @@ -104,6 +146,8 @@ because `LOOP.md` refers to items by number. - `%` is real modulo in Wurst; `mod` is integer modulo. `int % 8` types as `real`. - Emitted Lua must be byte-identical for identical input (AGENTS.md §8). It is the only emitted output that can be diffed across runs — see item 11. +- A test that hangs looks exactly like a test that is slow. If the suite stops making progress, + take a thread dump of the forked worker (`jstack `) before killing it — it names the line. - Method names are not what the frontend called them. `LuaDispatchPreparation` renames a whole dispatch group to one name and attaches alias sets, and only then does the backend run. A question about which Lua slot something lands in is a question about that pass, not about diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaRunnerTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaRunnerTests.java new file mode 100644 index 000000000..11655e5ad --- /dev/null +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaRunnerTests.java @@ -0,0 +1,85 @@ +package tests.wurstscript.tests; + +import org.testng.annotations.Test; + +import static org.testng.Assert.assertTrue; +import static org.testng.Assert.fail; + +/** + * Tests for the harness that runs emitted Lua, rather than for anything it compiles. + *

+ * Both cases here used to stop the whole suite instead of failing a test: the runner drained the + * spawned interpreter's stderr to the end before reading stdout, and waited for the process without + * a bound. A program that fills the stdout pipe blocks writing while the runner blocks reading the + * other pipe, and neither side ever moves. There is nothing to see when that happens - no output, + * no failing test, no timeout - so it is worth holding onto tests that would notice it coming back. + *

+ * Both are Lua only. Run through the Jass configurations as well, the non-terminating one would + * hang the interpreter instead, which is the same problem in a place this fix does not reach. + */ +public class LuaRunnerTests extends WurstScriptTest { + + /** Long enough that a program which does terminate still does, short enough to wait for. */ + @Override + protected int luaExecutionTimeoutSeconds() { + return 5; + } + + @Test + public void aProgramThatDoesNotTerminateFailsItsOwnTest() { + try { + test().testLua(true).luaOnly(true).executeProg().lines( + "package test", + "native testSuccess()", + "init", + " var i = 0", + " while true", + " i += 1", + " testSuccess()" + ); + fail("expected the runner to give up on a program that does not terminate"); + } catch (Error e) { + assertTrue(e.getMessage() != null && e.getMessage().contains("did not terminate"), + "expected a timeout, got: " + e.getMessage()); + } + } + + /** + * Enough lines to fill the pipe buffer several times over. With the streams read one after the + * other this deadlocks: the program blocks writing stdout, the runner blocks reading stderr. + */ + @Test + public void aProgramThatFloodsStdoutStillFinishes() { + test().testLua(true).luaOnly(true).executeProg().lines( + "package test", + "native testSuccess()", + "native println(string s)", + "init", + " for i = 1 to 20000", + " println(\"filling the pipe buffer with a reasonably long line of output\")", + " testSuccess()" + ); + } + + /** + * The same drain with the output arriving as one line rather than many. This checks that it + * finishes, which is what the two cases above check too — it does not observe how much of the + * line was retained, and would pass against a collector that kept all of it. Keeping a bounded + * amount is worth doing regardless, but a program printing a megabyte without a newline is not + * something this suite expects, so it is not worth test-only machinery to assert. + */ + @Test + public void aProgramPrintingWithoutNewlinesStillFinishes() { + test().testLua(true).luaOnly(true).executeProg().lines( + "package test", + "native testSuccess()", + "native println(string s)", + "init", + " var line = \"\"", + " for i = 1 to 4000", + " line += \"this line keeps growing and is never broken by a newline \"", + " println(line)", + " testSuccess()" + ); + } +} diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/WurstScriptTest.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/WurstScriptTest.java index 545f28c4d..a81e34ac4 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/WurstScriptTest.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/WurstScriptTest.java @@ -48,6 +48,21 @@ public class WurstScriptTest { public static final String TEST_OUTPUT_PATH = "./test-output/"; + + /** Generous enough for the slowest test program, short enough that a loop fails the run. */ + private static final int LUA_EXECUTION_TIMEOUT_SECONDS = 60; + + /** + * How much of a program's output is kept for the failure message. Draining never stops - that + * is what deadlocks the run - but retaining everything from a program that loops while printing + * would exhaust the worker before the timeout fires. + */ + private static final int RETAINED_OUTPUT_LIMIT = 64 * 1024; + + /** Overridden by the tests covering the runner itself, so they need seconds rather than a minute. */ + protected int luaExecutionTimeoutSeconds() { + return LUA_EXECUTION_TIMEOUT_SECONDS; + } private static volatile String resolvedLuaExecutable; private static volatile String resolvedLuacExecutable; private static volatile String extractedLuaWin; @@ -527,7 +542,6 @@ private void translateAndTestLua(String name, boolean executeProg, WurstGui gui, throw new org.testng.SkipException( "Skipped Lua execution (translation and luac syntax check still ran): " + e.getMessage()); } - String line; // Preload the WC3 Lua runtime (Reforged blizzard.j dump + native shim) // when available, so tests execute against real BJ implementations. // The generated script only installs fallbacks for natives that are @@ -550,29 +564,27 @@ private void translateAndTestLua(String name, boolean executeProg, WurstGui gui, Process p = Runtime.getRuntime().exec(args); StringBuilder errors = new StringBuilder(); StringBuilder output = new StringBuilder(); - try (BufferedReader input = new BufferedReader(new InputStreamReader(p.getErrorStream()))) { - while ((line = input.readLine()) != null) { - System.err.println(line); - errors.append(line); - errors.append("\n"); - } + // Both pipes must be drained while the program runs, and the wait must end: a + // generated program that loops forever would otherwise hang the whole suite, + // and one that fills the stdout pipe would deadlock against a stderr-first read. + java.util.concurrent.atomic.AtomicBoolean sawTestSuccess = + new java.util.concurrent.atomic.AtomicBoolean(false); + Thread outCollector = collectStreamAsync(p.getInputStream(), output, "testSuccess", sawTestSuccess); + Thread errCollector = collectStreamAsync(p.getErrorStream(), errors); + if (!p.waitFor(luaExecutionTimeoutSeconds(), TimeUnit.SECONDS)) { + p.destroyForcibly(); + throw new Error(currentTestEnv + ": Lua program did not terminate within " + + luaExecutionTimeoutSeconds() + "s: " + luaFile.getName()); } + outCollector.join(); + errCollector.join(); if (errors.length() > 0) { + System.err.print(errors); throw new TestFailException(errors.toString()); } - boolean success = false; - try (BufferedReader input = new BufferedReader(new InputStreamReader(p.getInputStream()))) { - while ((line = input.readLine()) != null) { - if (line.equals("testSuccess")) { - success = true; - } - output.append(line); - output.append("\n"); - } - } - if (!success) { + if (!sawTestSuccess.get()) { throw new Error(currentTestEnv + ": Succeed function not called"); } } @@ -621,11 +633,65 @@ private void checkLuaSyntax(String luacExecutable, File luaFile) throws IOExcept } private Thread collectStreamAsync(InputStream stream, StringBuilder out) { + return collectStreamAsync(stream, out, null, null); + } + + /** + * Drains {@code stream} into {@code out}, keeping only the first {@link #RETAINED_OUTPUT_LIMIT} + * characters. Reading never stops — that is what blocks the process and deadlocks the run — but + * a program that loops while printing would fill the heap before the timeout could fire. + *

+ * Whatever the caller is looking for is recognised here rather than read back out of + * {@code out} afterwards, because it may arrive after the limit: a program that prints a + * million lines and then succeeds has still succeeded. + */ + private Thread collectStreamAsync(InputStream stream, StringBuilder out, + String watchedLine, java.util.concurrent.atomic.AtomicBoolean sawWatchedLine) { Thread t = new Thread(() -> { - try (BufferedReader input = new BufferedReader(new InputStreamReader(stream))) { - String line; - while ((line = input.readLine()) != null) { - out.append(line).append("\n"); + // Fixed-size chunks rather than lines: a line is only bounded by what the program + // chose to print, and reading one materialises all of it before any limit here could + // apply. Nothing below holds more than the retained limit plus one chunk. + char[] chunk = new char[8 * 1024]; + StringBuilder pendingLine = new StringBuilder(); + boolean lineIsLongerThanWatched = false; + boolean truncated = false; + try (Reader input = new InputStreamReader(stream)) { + int read; + while ((read = input.read(chunk)) >= 0) { + if (watchedLine != null) { + for (int i = 0; i < read; i++) { + char c = chunk[i]; + if (c == '\n') { + if (!lineIsLongerThanWatched && pendingLine.length() == watchedLine.length() + && watchedLine.contentEquals(pendingLine)) { + sawWatchedLine.set(true); + } + pendingLine.setLength(0); + lineIsLongerThanWatched = false; + } else if (c != '\r' && !lineIsLongerThanWatched) { + // Only ever as long as what is being looked for; past that the + // line cannot be it, so there is no reason to keep any of it. + if (pendingLine.length() == watchedLine.length()) { + lineIsLongerThanWatched = true; + pendingLine.setLength(0); + } else { + pendingLine.append(c); + } + } + } + } + int room = RETAINED_OUTPUT_LIMIT - out.length(); + if (room > 0) { + out.append(chunk, 0, Math.min(read, room)); + } + // Said as soon as anything is dropped, including when that happens part way + // through the last chunk there is: otherwise the output ends at exactly the + // limit and reads as though that were all the program had to say. + if (read > room && !truncated) { + truncated = true; + out.append("\n... further output dropped after ") + .append(RETAINED_OUTPUT_LIMIT).append(" characters\n"); + } } } catch (IOException ignored) { } From 7fa5c1511aac072cea8f971f7e02400cdbba6ebb Mon Sep 17 00:00:00 2001 From: Frotty Date: Sun, 16 Aug 2026 09:51:51 +0200 Subject: [PATCH 07/13] Let declared names decide dispatch slots, and finish the FastHashMap proof (#1232) --- BACKLOG.md | 67 ++++-- .../imtranslation/LuaDispatchPreparation.java | 43 +++- .../wurstscript/tests/FastHashMapTests.java | 201 +++++++++++++++++- 3 files changed, 283 insertions(+), 28 deletions(-) diff --git a/BACKLOG.md b/BACKLOG.md index 7d6c0ad50..1d7a5d24c 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -13,24 +13,35 @@ Notes rather than leaving it in a commit message. Numbering is stable: finished items leave a gap rather than shifting the ones below, because `LOOP.md` refers to items by number. -3. **`slotFor` is bound to `get`'s implementation** in the emitted Lua — confirmed real, and - not an artefact of item 1: it survives sanitisation unchanged. Diagnosed, not yet fixed. - The alias sets in `LuaDispatchPreparation` decide which slots a method claims, and - `sharesSemanticName` accepts a match on *either* of two names: the source name (`get`, - `slotFor`) or `semanticNameFromMethodName`, which is the substring after the last - underscore. For a specialised method that substring is a type-argument fragment — - `FastHashMap_get_specialized__integer__integer___integer` and the `slotFor` one both yield - `integer` — so two unrelated methods count as sharing a name. They also share a dispatch - signature here (`(pos) returns int` both), which is the other half of the guard, so `get` - claims `slotFor`'s slot. Fix: when both methods have a real source name, that should decide; - the substring heuristic is a fallback for when there is no trace to ask, not an alternative. - Watch the closure and bridge cases in `TypeClassTests`/`LuaBackendAuditTests` — they are what - the loose match was presumably widened for. - -4. **Finish the FastHashMap proof.** `FastHashMapTests` is the first real use of bounds. - Add `remove` with tombstones, and an assertion that the emitted code stays cheap: no - dispatch node, no instance dictionary, and no WC3 hashtable natives — array access only, - which is the whole point versus `HashMap extends Table`. +16. **A never-written array of a type parameter reads as nothing, silently.** In the interpreter + only — Jass and Lua both give the type argument's default. `DefaultValue.get(ImTypeVarRef)` + returns `ILconstUnsafeDefault`, whose `isEqualTo` matches only another `ILconstUnsafeDefault`, + so a comparison against the real default is quietly false rather than an error. Repro: + + class Box + private static T array none + static function first() returns T + return none[0] + init + if Box.first() == 0 + testSuccess() + + Passes on every Jass configuration and fails on the pre-transform interpreter run. The plain + `int array` version passes, so this is specific to the type parameter. The interpreter knows + the current type argument (`ProgramState.resolveType`), but `DefaultValue` is a static + attribute with no access to it, and the array's default supplier is bound when the array is + allocated rather than when it is read. Either resolve at read time where the state is in hand, + or make the placeholder throw when used — what it must not do is compare unequal in silence. + Found by `FastHashMapTests`: the tombstone fixture needs a "no value" for `V`. + +15. **One junk dispatch slot per specialised class.** Left over from item 3, same heuristic in + the other place it is used. `addDirectAliases` composes `owner.getName() + "_" + + semanticNameFromMethodName(name)`, and for a specialised method that trailing segment is the + type argument, so every method of `FastHashMap` claims the same + `FastHashMap_specialized_integer__integer_integer` slot and the alphabetically first wins. + Nothing calls it, so it is dead weight rather than a wrong result — but it is the same + mistake, and the alias it *should* produce is the class qualified with the declared name. + Fixing it changes emitted slot names, so it wants its own commit and its own suite run. 5. **Lua dispatch inside a closure.** Works on Jass since #1229. On Lua the specialised class is built correctly but nothing calls it, because the closure is reached through its @@ -124,6 +135,26 @@ because `LOOP.md` refers to items by number. ## Done +- 4. The FastHashMap proof is complete. `remove` leaves a tombstone, which `slotFor` passes over + when searching and reuses when putting; the probe is bounded by capacity rather than running + until it finds a gap, so a table full of tombstones cannot spin. `emittedCodeCostsNothingExtra` + asserts the cost claim on the *least* optimised configuration, because it has to hold by + construction rather than by inlining: storage is four plain Jass arrays, no WC3 hashtable native + is reached for, `slotFor` takes nothing beyond the receiver and the key — no instance is threaded + through at runtime — and both requirements are direct calls, not `ExecuteFunc`, not a dispatch + wrapper. In the optimised output `hash(key)` becomes `key` and `equals(a, key)` becomes `a != key`. + The `dispatch_` functions that remain are the nullpointer check every Wurst class method gets, + not type class dispatch; the test is careful to look at the real function under that wrapper. +- 3. `slotFor`'s slot no longer holds `get`'s implementation. The alias sets in + `LuaDispatchPreparation` decide which slots a method claims, and `sharesSemanticName` accepted + a match on either the declared name or `semanticNameFromMethodName` — the substring after the + last underscore, which for a specialised method is a fragment of the type argument. Both + `FastHashMap_get_specialized_integer__integer` and the `slotFor` one end in `integer`, and the + two share a dispatch signature, so `get` claimed `slotFor`'s slot. Declared names now settle it + whenever both methods have one; the substring is a fallback for closures and bridges, which + have no declaration to ask. `FastHashMapTests.fastHashMapRuntimeLua` asserts every slot named + after a method binds that method's implementation — it fails on the old code with exactly the + binding above. - 14. The Lua execution harness no longer hangs. It read the spawned interpreter's stderr to EOF before touching stdout, and never bounded the wait, so a program that filled the stdout pipe deadlocked the whole suite with no output and no timeout — a stray JVM was still sitting on it diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaDispatchPreparation.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaDispatchPreparation.java index cbed8db1b..86b95b25c 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaDispatchPreparation.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaDispatchPreparation.java @@ -176,10 +176,10 @@ private static void addHierarchyAliases(ImMethod method, Set aliases, Ma return; } String dispatchKey = dispatchSignatureKey(method); - collectHierarchyAliases(owner, dispatchKey, semanticNames, aliases, sortedMethodsByClass, new HashSet<>()); + collectHierarchyAliases(owner, method, dispatchKey, semanticNames, aliases, sortedMethodsByClass, new HashSet<>()); } - private static void collectHierarchyAliases(ImClass c, String dispatchKey, Set semanticNames, Set aliases, + private static void collectHierarchyAliases(ImClass c, ImMethod method, String dispatchKey, Set semanticNames, Set aliases, Map> sortedMethodsByClass, Set visited) { if (c == null || !visited.add(c)) { return; @@ -188,7 +188,7 @@ private static void collectHierarchyAliases(ImClass c, String dispatchKey, Set semanticNames(ImMethod method) { return names; } + /** + * Whether {@code candidate} is the same method as {@code method} under a different name, which + * is what makes it worth claiming its slot. + * + *

When both were declared in source, their declared names settle it. The name-derived + * fallback below reads the segment after the last underscore, which for a specialised method + * is a fragment of the type argument: two unrelated methods of one specialisation both end in + * {@code integer} and would otherwise be taken for one another. + */ + private static boolean sharesSemanticName(ImMethod method, ImMethod candidate, Set semanticNames) { + String declared = declaredName(method); + String candidateDeclared = declaredName(candidate); + if (!declared.isEmpty() && !candidateDeclared.isEmpty()) { + return declared.equals(candidateDeclared); + } + return sharesSemanticName(candidate, semanticNames); + } + private static boolean sharesSemanticName(ImMethod method, Set semanticNames) { if (semanticNames.isEmpty()) { return false; @@ -255,6 +273,21 @@ private static boolean sharesSemanticName(ImMethod method, Set semanticN || semanticNames.contains(sourceSemanticName(method)); } + /** The name the method was written with, or empty when there is no declaration to ask. */ + private static String declaredName(ImMethod method) { + if (method == null) { + return ""; + } + de.peeeq.wurstscript.ast.Element trace = method.attrTrace(); + if (trace instanceof FuncDef funcDef) { + return funcDef.getName(); + } + if (trace instanceof AstElementWithFuncName withFuncName) { + return withFuncName.getFuncNameId().getName(); + } + return ""; + } + private static List sortedMethodsForClass(ImClass c, Map> sortedMethodsByClass) { return sortedMethodsByClass.computeIfAbsent(c, key -> { List methods = new ArrayList<>(key.getMethods()); diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/FastHashMapTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/FastHashMapTests.java index 50becb1f3..23817b5a2 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/FastHashMapTests.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/FastHashMapTests.java @@ -1,7 +1,14 @@ package tests.wurstscript.tests; +import com.google.common.base.Charsets; +import com.google.common.io.Files; import org.testng.annotations.Test; +import java.io.File; +import java.io.IOException; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + /** * Exercises type class bounds through the container they were added for: a hash map whose key type * is bounded by {@code Hashable}, so it can hash and compare keys without erasing them to int. @@ -28,30 +35,69 @@ private static String[] fastHashMap(String... extra) { " private static K array keys", " private static V array values", " private static boolean array used", + // A removed slot cannot go back to empty: a probe that stopped there would miss keys + // put down beyond it. It becomes a tombstone instead - passed over when searching, + // reused when putting. + " private static boolean array dead", + // Never written, so a read yields V's default. That is the only way to say "no value" + // for a type parameter, and it costs an array read rather than a branch. + " private static V array none", " private static int nextFree = 0", " private int base", " private int count = 0", " construct()", " base = nextFree", " nextFree += CAPACITY", + // The slot holding key, or the one it belongs in: the first tombstone passed over, + // else the empty slot the probe stopped at. Capacity is fixed, so a full table + // returns -1 rather than probing forever. " private function slotFor(K key) returns int", " var i = K.hash(key) mod CAPACITY", " if i < 0", " i += CAPACITY", - " while used[base + i] and not K.equals(keys[base + i], key)", + " var firstDead = -1", + " var probes = 0", + " while probes < CAPACITY", + " let s = base + i", + " if used[s] and K.equals(keys[s], key)", + " return s", + " if not used[s] and not dead[s]", + " if firstDead >= 0", + " return firstDead", + " return s", + " if dead[s] and firstDead < 0", + " firstDead = s", " i = (i + 1) mod CAPACITY", - " return base + i", + " probes++", + " return firstDead", " function put(K key, V value)", " let s = slotFor(key)", + // Capacity is fixed, so a new key can arrive with nowhere to go. The other three + // guard the same way; without it this writes through the sentinel and counts it. + " if s < base", + " return", " if not used[s]", " used[s] = true", + " dead[s] = false", " keys[s] = key", " count++", " values[s] = value", " function get(K key) returns V", - " return values[slotFor(key)]", + " let s = slotFor(key)", + " if s < base or not used[s]", + " return none[0]", + " return values[s]", " function has(K key) returns boolean", - " return used[slotFor(key)]", + " let s = slotFor(key)", + " return s >= base and used[s]", + " function remove(K key) returns boolean", + " let s = slotFor(key)", + " if s < base or not used[s]", + " return false", + " used[s] = false", + " dead[s] = true", + " count--", + " return true", " function size() returns int", " return count", }; @@ -101,8 +147,94 @@ public void fastHashMapRuntime() { } @Test - public void fastHashMapRuntimeLua() { + public void fastHashMapRuntimeLua() throws IOException { test().testLua(true).executeProg().lines(program(fastHashMap(), INT_INSTANCE, USE_WITH_COLLISION)); + assertEachSlotBindsItsOwnMethod(compiledLua("fastHashMapRuntimeLua")); + } + + private static final String[] METHOD_NAMES = {"slotFor", "put", "get", "has", "size", "remove"}; + + private String compiledLua(String testName) throws IOException { + return Files.toString(new File("test-output/lua/FastHashMapTests_" + testName + ".lua"), Charsets.UTF_8); + } + + /** + * A dispatch slot is named after the method it belongs to, and nothing else in the emitted Lua + * records which implementation belongs there — so this is the only place the two can be checked + * against each other. Specialisation names every method of one instantiation after the same type + * argument, which is exactly when they are easiest to confuse. + */ + private void assertEachSlotBindsItsOwnMethod(String lua) { + Matcher assignment = Pattern.compile("(?m)^\\s*\\w+\\.(\\w+) = (\\w+)\\s*$").matcher(lua); + while (assignment.find()) { + String slot = assignment.group(1); + String implementation = assignment.group(2); + for (String method : METHOD_NAMES) { + if (namesMethod(slot, method) && !namesMethod(implementation, method)) { + throw new AssertionError("slot '" + slot + "' is named after " + method + + " but binds '" + implementation + "'"); + } + } + } + } + + /** Whether {@code name} carries {@code method} as one of its underscore-separated segments. */ + private static boolean namesMethod(String name, String method) { + return name.equals(method) + || name.startsWith(method + "_") + || name.endsWith("_" + method) + || name.contains("_" + method + "_"); + } + + /** + * Keys 1, 9 and 2 form one probe run at capacity 8. Removing the middle of it is the case + * tombstones exist for: with the slot marked empty instead, the probe for 2 would stop at it + * and report the key missing. + */ + private static final String[] USE_WITH_REMOVE = { + "init", + " let m = new FastHashMap()", + " m.put(1, 10)", + " m.put(9, 90)", + " m.put(2, 20)", + " if m.remove(9) and not m.remove(9)", + " if m.get(2) == 20 and m.has(2) and not m.has(9) and m.size() == 2", + // The tombstone is reused rather than leaked, so the run stays the same length. + " m.put(9, 91)", + " if m.get(9) == 91 and m.size() == 3 and m.get(1) == 10 and m.get(2) == 20", + " testSuccess()", + }; + + /** + * Capacity is fixed, so the ninth distinct key has nowhere to go: keys 1 to 8 hash to the eight + * slots exactly. `slotFor` says so by returning a slot below this instance's section, and every + * entry point has to check that before indexing — otherwise the write goes through the sentinel, + * the entry is unreachable, and the count says nine. + */ + @Test + public void puttingIntoAFullMapChangesNothing() { + testAssertOkLines(true, program(fastHashMap(), INT_INSTANCE, new String[]{ + "init", + " let m = new FastHashMap()", + " for i = 1 to 8", + " m.put(i, i * 10)", + " m.put(9, 90)", + " if m.size() == 8 and not m.has(9)", + // Not m.get(9) as well: reading an absent key of a type parameter returns a stand-in + // the interpreter cannot compare, which is a separate bug fixed later in this stack. + " if m.get(1) == 10 and m.get(8) == 80", + " testSuccess()" + })); + } + + @Test + public void removeLeavesATombstone() { + testAssertOkLines(true, program(fastHashMap(), INT_INSTANCE, USE_WITH_REMOVE)); + } + + @Test + public void removeLeavesATombstoneLua() { + test().testLua(true).executeProg().lines(program(fastHashMap(), INT_INSTANCE, USE_WITH_REMOVE)); } /** @@ -204,4 +336,63 @@ public void twoInstancesOfOneSpecialisation() { " testSuccess()" })); } + + /** + * The point of bounds over {@code HashMap extends Table}: the requirements are resolved when the + * map is specialised, not carried to runtime. This asserts on the least optimised configuration + * on purpose — the cost has to be absent by construction, not removed afterwards by the inliner. + */ + @Test + public void emittedCodeCostsNothingExtra() throws IOException { + testAssertOkLines(true, program(fastHashMap(), INT_INSTANCE, USE_WITH_COLLISION)); + String jass = compiledJass("emittedCodeCostsNothingExtra_no_opts"); + + // Storage is plain Jass arrays, so a lookup is an array read, not a native call. + for (String[] storage : new String[][]{{"integer", "keys"}, {"integer", "values"}, + {"boolean", "used"}, {"boolean", "dead"}}) { + assertMatches(jass, "(?m)^" + storage[0] + " array FastHashMap_" + storage[1] + "\\w*$", + "storage array " + storage[1]); + } + for (String hashtableNative : new String[]{"InitHashtable", "SaveInteger", "LoadInteger", + "SaveStr", "LoadStr", "FlushChildHashtable", "GetHandleId"}) { + if (jass.contains(hashtableNative)) { + throw new AssertionError("the map reached for a WC3 hashtable native: " + hashtableNative); + } + } + + // The bound resolves to a direct call to the instance's own function. An instance passed at + // runtime would show up as an extra parameter here, and a dispatched one as an indirection. + Matcher slotFor = Pattern + // Not the dispatch_ wrapper of the same name - that one is the nullpointer check every + // class method gets, and it is the real function underneath that has to be free of cost. + .compile("(?m)^function (?!dispatch_)\\w*slotFor\\w* takes ([^\\n]*?) returns [^\\n]*\\n(.*?)\\nendfunction", + Pattern.DOTALL) + .matcher(jass); + if (!slotFor.find()) { + throw new AssertionError("no slotFor in the emitted Jass:\n" + jass); + } + if (!slotFor.group(1).equals("integer this, integer key")) { + throw new AssertionError("slotFor carries something beyond the receiver and the key: " + + slotFor.group(1)); + } + String body = slotFor.group(2); + assertMatches(body, "(? Date: Sun, 16 Aug 2026 10:06:53 +0200 Subject: [PATCH 08/13] Take a closure's instantiation from its construction on Lua (#1233) --- BACKLOG.md | 63 +++++-- CHANGELOG.md | 5 +- .../imtranslation/EliminateGenerics.java | 169 ++++++++++++++++++ .../resources/agent-docs/WURST_LANGUAGE.md | 8 + .../wurstscript/tests/TypeClassTests.java | 137 +++++++++++++- 5 files changed, 355 insertions(+), 27 deletions(-) diff --git a/BACKLOG.md b/BACKLOG.md index 1d7a5d24c..04e4e650c 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -43,28 +43,35 @@ because `LOOP.md` refers to items by number. mistake, and the alias it *should* produce is the class qualified with the declared name. Fixing it changes emitted slot names, so it wants its own commit and its own suite run. -5. **Lua dispatch inside a closure.** Works on Jass since #1229. On Lua the specialised class - is built correctly but nothing calls it, because the closure is reached through its - interface and `specializeMethod` renames the method out of its dispatch slot. - `TypeClassTests.dispatchInsideClosureIsRejectedForLua` pins the current diagnostic and - should become a success test. Related to item 1; AGENTS.md flags this machinery. - -6. **Lua dispatch inside the constructor** of a bounded generic class. Works on Jass. +6. **Lua dispatch inside the constructor** of a bounded generic class. Works on Jass; there is now + a repro for both targets, `TypeClassTests.dispatchInsideConstructor` and + `dispatchInsideConstructorIsRejectedForLua`, the second pinning the current diagnostic. + + Not the same gap as item 5, and the fix from it does not reach: a constructor belongs to the + class rather than to a generic function of its own, so the call that runs it carries no type + arguments at all. The intermediate language has `b = new_Box(21)` with `b` typed + `Box`, and `new_Box` still generic; the calls *inside* it + (`construct_Box`, `Box_init`) do carry the class's type variable, but nothing gives the + outermost one a concrete argument. `collectGenericNewUse` requires non-empty type arguments, so + it never starts. + + The instantiation is only on the type of what the call is assigned to. Three ways to get at it, + roughly in order of how much they would disturb: attach the class's type arguments to + constructor calls when the intermediate language is built, which is where the frontend still + knows them and would serve both targets uniformly — but it changes the Jass path, which reaches + the same answer another way today, so the emitted `.j` needs checking; read them from the + assignment target on the Lua path, which is a syntactic shape and would miss + `foo(new Box(21))`; or specialise from the `#alloc` inside the constructor, which is the + item 5 mechanism but would have to reach back out to the caller. The first looks right; confirm + it is what the Jass path already relies on before changing it. 7. **Module bounds.** `module M` is rejected with a clear message today. Needs receiver rewriting during expansion, or type parameters on `ModuleInstanciation`. -8. **`MOD_INT`/`DIV_INT` return the left operand's type** rather than `int` - (`AttrExprType.java`, the `case MOD_INT` branch), where `caseMathOperation` returns - `WurstTypeInt.instance()` for `+`, `-`, `*`. It *is* reachable: `WurstTypeIntLiteral` is a - proper subtype of both int and real, and `caseMathOperation` collapses two literals to `int` - precisely so `real r = 1 + 1` stays an error. Returning `leftType` skips that collapse, so - `real r = 7 div 2` and `real r = 7 mod 2` should be accepted where `+` is rejected. Confirm - with a test first — that is the failing repro — then return `WurstTypeInt.instance()`. Small. - -9. **Keep `WURST_LANGUAGE.md` and `CHANGELOG.md` current** as items land. The bounds section - says nothing about closures, which now work on Jass. Fold this into whichever item changes - the behaviour rather than doing it as a separate pass. +9. **Keep `WURST_LANGUAGE.md` and `CHANGELOG.md` current** as items land — a standing practice + rather than a task to finish. Fold it into whichever item changes the behaviour rather than + doing it as a separate pass. Both now cover closures on either target, which is what this item + originally pointed at. 10. **One `ImTypeVar` per type parameter.** Name-tolerant lookups remain in `EliminateGenerics.indexOfTypeVar`, `inheritTypeClassBinding` and @@ -135,6 +142,26 @@ because `LOOP.md` refers to items by number. ## Done +- 8. `div` and `mod` return int rather than the left operand's type, matching `caseMathOperation`. + Reachable, not harmless: an integer literal is a proper subtype of both int and real, and + addition collapses two of them to int precisely so `real r = 1 + 1` stays an error — returning + `leftType` skipped that, so `real r = 7 div 2` was accepted. Three tests in `ExpressionTests`: + both operators rejected against a real, and both still int. +- 17. A failing Lua test says so. `translateAndTestLua` now sets the environment label instead of + reporting under whatever Jass configuration ran last. +- 5 (+ the part of 9 that follows it). A type class bound now dispatches from inside a closure on + Lua, and `TypeClassTests.dispatchInsideClosureLua` is a success test. The note in this file was + wrong about the cause: no specialised class was being built at all. Lua specialisation is driven + by calls that carry type arguments, and a closure has none — it is reached through the interface + it implements, which is not generic, so only the construction knows the instantiation. Three + pieces were missing, all present already for Jass: collect the instantiation from `ImAlloc`, + collect the member access so the capture write lands on the specialised field, and bind the + specialised methods to the roots the originals were submethods of (registering the original + implementation as specialised so `settleRemainingDispatches` neutralises what it leaves behind). + All three are gated on the class being closure-generated. Widening them to any constructed class + made the two mechanisms disagree — the object came from the specialised class while its methods + were bound to the erased one — and broke every FastHashMap Lua test, which is the shape of + regression AGENTS.md §9 warns about. `WURST_LANGUAGE.md` and `CHANGELOG.md` say so now. - 4. The FastHashMap proof is complete. `remove` leaves a tombstone, which `slotFor` passes over when searching and reuses when putting; the probe is bounded by capacity rather than running until it finds a gap, so a table full of tombstones cannot spin. `emittedCodeCostsNothingExtra` diff --git a/CHANGELOG.md b/CHANGELOG.md index 413b51211..ededc734f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,8 +37,9 @@ return () -> T.toIndex(x) Substituting a type variable now carries the instance chosen for it along with the type, rather than the - type alone, so lifting a body into a class of its own no longer loses it. Jass only for now: Lua reaches - such a class through its interface and still reports the bound as unresolvable there. + type alone, so lifting a body into a class of its own no longer loses it. This works on both targets. + Lua reaches such a class through the interface it implements, so no call names the instantiation and the + construction is what the specialisation is taken from. - Added new pseudo-natives for debugging memory leaks: diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/EliminateGenerics.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/EliminateGenerics.java index 6baf40263..9d423291b 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/EliminateGenerics.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/EliminateGenerics.java @@ -5,6 +5,7 @@ import de.peeeq.wurstscript.WLogger; import de.peeeq.wurstscript.ast.ClassDef; import de.peeeq.wurstscript.ast.ConstructorDef; +import de.peeeq.wurstscript.ast.ExprClosure; import de.peeeq.wurstscript.ast.InterfaceDef; import de.peeeq.wurstscript.ast.PackageOrGlobal; import de.peeeq.wurstscript.ast.WPackage; @@ -188,6 +189,18 @@ public void visit(ImMethodCall call) { super.visit(call); collectGenericNewUse(call); } + + @Override + public void visit(ImAlloc alloc) { + super.visit(alloc); + collectGenericNewUse(alloc); + } + + @Override + public void visit(ImMemberAccess memberAccess) { + super.visit(memberAccess); + collectGenericNewUse(memberAccess); + } }); } @@ -204,6 +217,18 @@ public void visit(ImMethodCall call) { super.visit(call); collectGenericNewUse(call); } + + @Override + public void visit(ImAlloc alloc) { + super.visit(alloc); + collectGenericNewUse(alloc); + } + + @Override + public void visit(ImMemberAccess memberAccess) { + super.visit(memberAccess); + collectGenericNewUse(memberAccess); + } }); } @@ -225,6 +250,106 @@ && functionNeedsSpecialization(call.getFunc(), Collections.newSetFromMap(new Ide } } + /** + * A construction states an instantiation that no call site has to mention. A closure is the case + * that needs it: its class is built from the enclosing type variables and reached through its + * interface, so the call carries no type arguments at all and only the allocation knows what the + * body dispatches on. Restricted to classes that actually dispatch on a bound, so this stays a + * targeted specialisation rather than general monomorphisation on Lua. + */ + private void collectGenericNewUse(ImAlloc alloc) { + ImClassType clazz = alloc.getClazz(); + if (clazz.getTypeArguments().isEmpty() + || typeArgumentsContainTypeVariable(clazz.getTypeArguments()) + || !isConstructionOnlyInstantiation(clazz.getClassDef())) { + return; + } + genericsUses.add(new GenericClazzUse(alloc)); + } + + /** + * Whether the construction is the only place a class's instantiation is stated. + *

+ * A class the user writes is used through calls that carry its type arguments, and those already + * specialise what they need onto the erased class. A closure has no such call: it is reached + * through the interface it implements, which is not generic, so the allocation is the only thing + * that knows what the body dispatches on. Widening this beyond that case makes the two + * mechanisms disagree — the object comes from the specialised class while its methods were bound + * to the erased one. + */ + private boolean isConstructionOnlyInstantiation(ImClass classDef) { + return classDef.attrTrace() instanceof ExprClosure closure + && !isInsideAnotherClosure(closure) + && classReachesDispatch(classDef); + } + + /** + * A closure written inside another one is left alone. + *

+ * Its captured environment is reached through a receiver belonging to the enclosing closure, + * which by then has been specialised itself, and specialising the owner again with what is + * left over fails inside the rewrite. Supporting that is a further step; until it is taken, + * saying the bound could not be resolved - which is what happens without any of this - is + * better than an error about generics of the wrong size. + */ + private static boolean isInsideAnotherClosure(ExprClosure closure) { + de.peeeq.wurstscript.ast.Element parent = closure.getParent(); + return parent != null && parent.attrNearestExprClosure() != null; + } + + /** + * Whether anything the class does ends in a dispatch on a bound, including through the + * functions it calls. `classNeedsSpecialization` asks only whether a dispatch sits in the class + * itself, which is the wrong question here: a closure whose body is `() -> helper(x)` has no + * dispatch of its own, and the instantiation it needs is still only known at its construction. + * That question is kept as it is, because widening it would change what gets specialised on + * paths that have nothing to do with closures. + */ + private boolean classReachesDispatch(ImClass classDef) { + for (ImFunction f : classDef.getFunctions()) { + if (functionNeedsSpecialization(f, Collections.newSetFromMap(new IdentityHashMap<>()), + Collections.newSetFromMap(new IdentityHashMap<>()))) { + return true; + } + } + for (ImMethod m : classDef.getMethods()) { + if (m.getImplementation() != null + && functionNeedsSpecialization(m.getImplementation(), + Collections.newSetFromMap(new IdentityHashMap<>()), + Collections.newSetFromMap(new IdentityHashMap<>()))) { + return true; + } + } + return false; + } + + /** + * A field of a class specialised from a construction has to be reached on the copy. The write + * that captures a closure's environment is the case that needs it: it names the field of the + * generic class, which nothing allocates any more once the construction was redirected. + */ + private void collectGenericNewUse(ImMemberAccess memberAccess) { + ImVar field = memberAccess.getVar(); + if (field.getParent() == null || !(field.getParent().getParent() instanceof ImClass owningClass)) { + return; + } + // A class that has already been specialised has nothing left to select, and asking the + // receiver to adapt to it fails outright: the receiver is still typed by the generic class + // the specialised one was copied from, which is not a superclass of it. + if (owningClass.getTypeVariables().isEmpty() || !isConstructionOnlyInstantiation(owningClass)) { + return; + } + if (memberAccess.getTypeArguments().isEmpty()) { + // The access names a field, not an instantiation; the receiver is what knows which one. + addMemberTypeArguments(memberAccess, owningClass); + } + if (memberAccess.getTypeArguments().isEmpty() + || typeArgumentsContainTypeVariable(memberAccess.getTypeArguments())) { + return; + } + genericsUses.add(new GenericMemberAccess(memberAccess)); + } + private void collectGenericNewUse(ImMethodCall call) { if (specializedCallSites.contains(call)) { return; @@ -1316,12 +1441,56 @@ private ImClass specializeClass(ImClass c, GenericTypes generics) { // NEW: Create specialized global variables for this class instantiation createSpecializedGlobals(c, generics, typeVars); + if (genericNewOnly && isConstructionOnlyInstantiation(c)) { + attachSpecializedClassMethods(c, newC, generics); + } onSpecializedClassTriggers.get(c).forEach(consumer -> consumer.accept(generics, newC)); return newC; } + /** + * Makes the methods of a class specialised from a construction reachable. + *

+ * A class specialised because a call named its instantiation is reached through that call. + * One specialised because it was constructed is not: the receiver is held as its interface, so + * dispatch goes through the root method, whose submethods still list only the generic original. + * Each copy is bound to the same roots, and the original's implementation is recorded as having + * a specialisation so the dispatch left behind in it settles instead of reaching the backend. + */ + private void attachSpecializedClassMethods(ImClass original, ImClass specialized, GenericTypes generics) { + List originalMethods = original.getMethods(); + List specializedMethods = specialized.getMethods(); + if (originalMethods.size() != specializedMethods.size()) { + // The copy is structural, so this cannot happen; bail rather than pair the wrong ones. + return; + } + Map specializationOf = new IdentityHashMap<>(); + for (int i = 0; i < originalMethods.size(); i++) { + ImMethod copy = specializedMethods.get(i); + copy.setMethodClass(JassIm.ImClassType(specialized, JassIm.ImTypeArguments())); + specializationOf.put(originalMethods.get(i), copy); + + ImFunction implementation = originalMethods.get(i).getImplementation(); + ImFunction copyImplementation = copy.getImplementation(); + if (implementation != null && copyImplementation != null && implementation != copyImplementation + && specializedFunctions.get(implementation, generics) == null) { + specializedFunctions.put(implementation, generics, copyImplementation); + } + } + for (ImClass c : new ArrayList<>(prog.getClasses())) { + for (ImMethod root : c.getMethods()) { + for (ImMethod sub : new ArrayList<>(root.getSubMethods())) { + ImMethod copy = specializationOf.get(sub); + if (copy != null && !root.getSubMethods().contains(copy)) { + root.getSubMethods().add(copy); + } + } + } + } + } + private ImExpr rewriteGenericGlobalsInExpr(ImExpr e, ImClass owningClass, GenericTypes generics) { e.accept(new Element.DefaultVisitor() { @Override public void visit(ImVarAccess va) { diff --git a/de.peeeq.wurstscript/src/main/resources/agent-docs/WURST_LANGUAGE.md b/de.peeeq.wurstscript/src/main/resources/agent-docs/WURST_LANGUAGE.md index 3b8b2f538..c094ba98f 100644 --- a/de.peeeq.wurstscript/src/main/resources/agent-docs/WURST_LANGUAGE.md +++ b/de.peeeq.wurstscript/src/main/resources/agent-docs/WURST_LANGUAGE.md @@ -186,6 +186,14 @@ Instances are unique and must be declared next to what they relate. An instance An instance must implement each requirement with the signature it has after the interface's type parameter is replaced by the instance type; a matching name is not enough. An interface used as a bound must not extend another interface, because the requirements of a bound are the interface's own functions. +A requirement can also be dispatched from inside a closure written in a bounded generic. The closure captures the type parameter along with the values it uses, so the instance is still chosen by the caller's type argument: + +```wurst +function foo(Q x) returns int + Producer p = () -> Q.toIndex(x) + return p.produce() +``` + A generic which passes its own type parameter to another bounded generic must declare that bound itself: ```wurst diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java index cd8429a4e..021b416d7 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java @@ -6,6 +6,8 @@ import java.io.File; import java.io.IOException; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertTrue; @@ -94,15 +96,14 @@ public void dispatchInsideClosure() { } /** - * The same closure is still rejected for Lua, and this pins that it is rejected clearly rather - * than mistranslated. Lua keeps generics erased and specialises only what it can reach through a - * concrete type; a closure is reached through its interface, so the specialised class exists but - * nothing calls it. Making that work is a change to Lua's erasure, not to substitution. Should - * it be made, this test fails and becomes the success case above. + * The same closure on Lua, which keeps generics erased and specialises only what it can reach + * from a concrete type. A closure is reached through its interface, so no call names the + * instantiation — the construction is the only thing that knows it, and that is what the + * specialisation is now driven from. */ @Test - public void dispatchInsideClosureIsRejectedForLua() { - test().testLua(true).executeProg().expectError("could not be resolved for the Lua target").lines( + public void dispatchInsideClosureLua() throws IOException { + test().testLua(true).executeProg().lines( "package test", "native testSuccess()", "interface ToIndex", @@ -119,6 +120,97 @@ public void dispatchInsideClosureIsRejectedForLua() { " if foo(21) == 42", " testSuccess()" ); + + // Running is not enough on its own: the same answer comes out whether the closure was + // specialised or the erased class happened to carry a working implementation. These say + // which of the two happened. + String compiled = Files.toString( + new File("test-output/lua/TypeClassTests_dispatchInsideClosureLua.lua"), Charsets.UTF_8); + + Matcher allocation = Pattern.compile("(\\w+_specialized\\w*):create\\d*\\(").matcher(compiled); + assertTrue(allocation.find(), + "the closure should be allocated from its specialised class:\n" + compiled); + String specialised = allocation.group(1); + + Matcher call = Pattern.compile("\\w+:(\\w*produce\\w*)\\(").matcher(compiled); + assertTrue(call.find(), "expected a dispatched produce slot:\n" + compiled); + String slot = call.group(1); + + assertTrue(Pattern.compile(Pattern.quote(specialised) + "\\." + Pattern.quote(slot) + + "\\s*=\\s*" + Pattern.quote(specialised) + "\\w*").matcher(compiled).find(), + "the specialised class should bind " + slot + " to its own implementation:\n" + compiled); + assertFalse(Pattern.compile(Pattern.quote(specialised) + "\\." + Pattern.quote(slot) + + "\\s*=\\s*Producer_test_produce\\b").matcher(compiled).find(), + "the specialised class must not bind " + slot + " to the generic original:\n" + compiled); + } + + /** + * A constructor runs before the object exists, so the bound has to be resolved from the type + * argument the construction names rather than from anything reachable on the receiver. + */ + private static final String[] DISPATCH_IN_CONSTRUCTOR = { + "package test", + "native testSuccess()", + "interface Show", + " function show(T x) returns int", + "implements Show", + " function show(int x) returns int", + " return x * 2", + "class Box", + " int cached", + " construct(T x)", + " cached = T.show(x)", + "init", + " let b = new Box(21)", + " if b.cached == 42", + " testSuccess()", + }; + + @Test + public void dispatchInsideConstructor() { + testAssertOkLines(true, DISPATCH_IN_CONSTRUCTOR); + } + + /** + * Still rejected for Lua, and this pins that it is rejected clearly rather than mistranslated. + * A constructor belongs to the class, not to a generic function of its own, so the call that + * runs it carries no type arguments — {@code new_Box(21)} in the intermediate language, with + * the instantiation only on the type of what it is assigned to. Nothing on the Lua path reads + * it from there, so the dispatch inside the constructor is never given a concrete type. + * Should that be made to work, this test fails and becomes the success case above. + */ + @Test + public void dispatchInsideConstructorIsRejectedForLua() { + test().testLua(true).executeProg().expectError("could not be resolved for the Lua target") + .lines(DISPATCH_IN_CONSTRUCTOR); + } + + /** + * The closure has no dispatch of its own — it calls something that does. The gate deciding + * whether a construction is the only place an instantiation is stated has to follow calls to + * see that, or this reaches the backend with the bound unresolved. + */ + @Test + public void dispatchInsideClosureThroughHelperLua() { + test().testLua(true).executeProg().lines( + "package test", + "native testSuccess()", + "interface ToIndex", + " function toIndex(T x) returns int", + "implements ToIndex", + " function toIndex(int x) returns int", + " return x * 2", + "interface Producer", + " function produce() returns int", + "function helper(Q x) returns int", + " return Q.toIndex(x)", + "function foo(Q x) returns int", + " Producer p = () -> helper(x)", + " return p.produce()", + "init", + " if foo(21) == 42", + " testSuccess()" + ); } /** @@ -141,6 +233,37 @@ public void closureImplementingALuaKeywordName() { ); } + /** + * A closure written inside another one is still rejected, and this pins that it is rejected in + * the same words as before rather than falling over inside the rewrite. The inner closure + * reaches its captured environment through a receiver belonging to the outer one, which has + * been specialised by then, so specialising the owner again with what is left over does not + * work. Should that be made to work, this test fails and becomes a success case. + */ + @Test + public void nestedClosuresInsideBoundedGenericAreRejectedForLua() { + test().testLua(true).executeProg().expectError("could not be resolved for the Lua target").lines( + "package test", + "native testSuccess()", + "interface ToIndex", + " function toIndex(T x) returns int", + "implements ToIndex", + " function toIndex(int x) returns int", + " return x * 2", + "interface Producer", + " function produce() returns int", + "function foo(Q x) returns int", + " Producer outer = () -> begin", + " Producer inner = () -> Q.toIndex(x)", + " return inner.produce()", + " end", + " return outer.produce()", + "init", + " if foo(21) == 42", + " testSuccess()" + ); + } + /** Each type argument picks its own instance, so one generic serves several types. */ @Test public void twoInstancesOfOneClass() { From 5ccc791cbfe4c8bc3d28920c09c8684afa7fc5f1 Mon Sep 17 00:00:00 2001 From: Frotty Date: Sun, 16 Aug 2026 10:21:27 +0200 Subject: [PATCH 09/13] Resolve a type parameter's default where the binding is known (#1234) --- BACKLOG.md | 108 +++++++++++------- .../de/peeeq/wurstscript/WurstOperator.java | 37 +++++- .../ILconstUnsafeDefault.java | 7 +- .../interpreter/EvaluateExpr.java | 7 +- .../interpreter/ProgramState.java | 17 +++ .../wurstscript/tests/ExpressionTests.java | 39 +++++++ .../wurstscript/tests/TypeClassTests.java | 40 +++++++ 7 files changed, 205 insertions(+), 50 deletions(-) diff --git a/BACKLOG.md b/BACKLOG.md index 04e4e650c..420406814 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -13,35 +13,23 @@ Notes rather than leaving it in a commit message. Numbering is stable: finished items leave a gap rather than shifting the ones below, because `LOOP.md` refers to items by number. -16. **A never-written array of a type parameter reads as nothing, silently.** In the interpreter - only — Jass and Lua both give the type argument's default. `DefaultValue.get(ImTypeVarRef)` - returns `ILconstUnsafeDefault`, whose `isEqualTo` matches only another `ILconstUnsafeDefault`, - so a comparison against the real default is quietly false rather than an error. Repro: - - class Box - private static T array none - static function first() returns T - return none[0] - init - if Box.first() == 0 - testSuccess() - - Passes on every Jass configuration and fails on the pre-transform interpreter run. The plain - `int array` version passes, so this is specific to the type parameter. The interpreter knows - the current type argument (`ProgramState.resolveType`), but `DefaultValue` is a static - attribute with no access to it, and the array's default supplier is bound when the array is - allocated rather than when it is read. Either resolve at read time where the state is in hand, - or make the placeholder throw when used — what it must not do is compare unequal in silence. - Found by `FastHashMapTests`: the tombstone fixture needs a "no value" for `V`. - -15. **One junk dispatch slot per specialised class.** Left over from item 3, same heuristic in - the other place it is used. `addDirectAliases` composes `owner.getName() + "_" + +15. **One junk dispatch slot per specialised class.** `addDirectAliases` and + `LuaTranslator.collectDispatchSlotNames` both compose `owner.getName() + "_" + semanticNameFromMethodName(name)`, and for a specialised method that trailing segment is the - type argument, so every method of `FastHashMap` claims the same - `FastHashMap_specialized_integer__integer_integer` slot and the alphabetically first wins. - Nothing calls it, so it is dead weight rather than a wrong result — but it is the same - mistake, and the alias it *should* produce is the class qualified with the declared name. - Fixing it changes emitted slot names, so it wants its own commit and its own suite run. + type argument — so every method of `FastHashMap` claims one shared + `FastHashMap_specialized_integer__integer_integer` slot and the alphabetically first wins it. + Nothing calls it, so it is dead weight rather than a wrong result. + + Tried using the declared name instead and reverted it: overloads share a declared name, so + `setup(int)` and `setup(string)` collapse into one slot, which is what + `LuaTranslationTests.overloadedMethodsDoNotAliasInLuaDispatchTables` and + `moduleProvidedOverloadedOverrideDoesNotCollapseLuaSlots` exist to prevent. Both sources of a + semantic name are wrong, in opposite directions: the mangled trailing segment collides across + the siblings of one specialisation, the declared name collides across overloads. A fix needs a + name that separates both — the declared name together with the dispatch signature key would, + since that is already what distinguishes overloads elsewhere in the same file. Worth doing only + if this stops being dead weight, because the cost of getting it wrong is a real mis-binding + while the cost of leaving it is one unused table key per specialised class. 6. **Lua dispatch inside the constructor** of a bounded generic class. Works on Jass; there is now a repro for both targets, `TypeClassTests.dispatchInsideConstructor` and @@ -55,15 +43,20 @@ because `LOOP.md` refers to items by number. outermost one a concrete argument. `collectGenericNewUse` requires non-empty type arguments, so it never starts. - The instantiation is only on the type of what the call is assigned to. Three ways to get at it, - roughly in order of how much they would disturb: attach the class's type arguments to - constructor calls when the intermediate language is built, which is where the frontend still - knows them and would serve both targets uniformly — but it changes the Jass path, which reaches - the same answer another way today, so the emitted `.j` needs checking; read them from the - assignment target on the Lua path, which is a syntactic shape and would miss - `foo(new Box(21))`; or specialise from the `#alloc` inside the constructor, which is the - item 5 mechanism but would have to reach back out to the caller. The first looks right; confirm - it is what the Jass path already relies on before changing it. + What Jass does, from `TypeClassTests_dispatchInsideConstructor_no_opts.jim`: it specialises the + constructor function itself, `b_8 = new_Box⟪integer⟫(21)`. It gets there from *types*, not from + the call — `collectGenericUsages` collects a `GenericVar` for the local declared + `Box` and a `GenericReturnTypeFunc` for `new_Box`, whose return type is generic. + The Lua collector has neither; it only ever looks at calls. So attaching type arguments to + constructor calls, which an earlier note here proposed, is not what the Jass path relies on and + would be a second mechanism rather than the same one. + + The honest next step is to collect from types on the Lua path too, restricted the way item 5's + collection is. That runs straight into the same design question, though: `GenericVar` and + `GenericReturnTypeFunc` specialise the *class*, and item 5 showed that an object coming from a + specialised class while its methods are bound to the erased one breaks everything. Either the + collection has to specialise only the constructor path and leave the object erased, or Lua stops + erasing constructed generic classes — which is a decision about the erasure model, not a patch. 7. **Module bounds.** `module M` is rejected with a clear message today. Needs receiver rewriting during expansion, or type parameters on `ModuleInstanciation`. @@ -126,6 +119,24 @@ because `LOOP.md` refers to items by number. ## Blocked on a decision +- **8. Should `div` and `mod` keep returning the left operand's type?** Tried returning + `WurstTypeInt.instance()` to match `caseMathOperation` and reverted it: it is a user-visible + breaking change, and the suite already defines the current behaviour as correct. + + The asymmetry is real and reachable. `WurstTypeIntLiteral` is a proper subtype of both int and + real, and `caseMathOperation` collapses two literals to int precisely so `real r = 1 + 1` is an + error. `div`/`mod` return `leftType`, so `real r = 7 div 2` compiles. Changing that made exactly + one test fail — `OptimizerTests.realFormatting_consistent_fromIntOps`, which opens with + `real a = 1 div 2` — and AGENTS.md says the existing suite is the authoritative definition of + behaviour. Real maps will contain the same shape. + + So the question is the owner's: is `real r = 7 div 2` meant to compile? If yes, the branch in + `AttrExprType` wants a comment saying so, and this item closes. If no, it is a deliberate + breaking change that needs the changelog, and `realFormatting_consistent_fromIntOps` needs + rewriting to say what it actually tests, which is real formatting rather than that assignment. + `ExpressionTests.integerDivisionOfLiteralsIsStillAssignableToReal` pins the behaviour meanwhile, + so whichever way it goes is deliberate rather than accidental. + - **Eliminating the remaining `castTo int`.** The motivating case is timer data attachment (`ClosureTimers.wurst`), and the containers behind it: `Table` has 81 casts, `HashList` 13, `HashSet` 6, `HashMap` 4. None can adopt bounds as things stand, because an instance is @@ -142,11 +153,18 @@ because `LOOP.md` refers to items by number. ## Done -- 8. `div` and `mod` return int rather than the left operand's type, matching `caseMathOperation`. - Reachable, not harmless: an integer literal is a proper subtype of both int and real, and - addition collapses two of them to int precisely so `real r = 1 + 1` stays an error — returning - `leftType` skipped that, so `real r = 7 div 2` was accepted. Three tests in `ExpressionTests`: - both operators rejected against a real, and both still int. +- 18. Comparing an unresolved type parameter default is now an error rather than a quiet "not + equal". Item 16 closed the path that reached a program, but the stand-in is produced by a static + attribute and could surface anywhere, so the silence was the part worth removing. The whole suite + is green with it throwing, which says nothing reachable produces one any more — and if something + starts to, it says so instead of returning a wrong answer. +- 16. A never-written slot of a `T array` reads as the default of what T stands for. The default + is computed by a static attribute, which cannot see the frames that know the type argument, so + it produced a stand-in that compares equal only to another stand-in — `Box.first() == 0` + was quietly false on the interpreter while both backends had it right. `ProgramState` does know + the substitution, so the stand-in is now resolved where the value is produced, at the array read + and the member read, rather than at the comparison where the symptom shows. Item 18 covers the + paths that could still leak one. - 17. A failing Lua test says so. `translateAndTestLua` now sets the environment label instead of reporting under whatever Jass configuration ran last. - 5 (+ the part of 9 that follows it). A type class bound now dispatches from inside a closure on @@ -204,6 +222,12 @@ because `LOOP.md` refers to items by number. - `%` is real modulo in Wurst; `mod` is integer modulo. `int % 8` types as `real`. - Emitted Lua must be byte-identical for identical input (AGENTS.md §8). It is the only emitted output that can be diffed across runs — see item 11. +- Two of this run's reverts were the same mistake: a name that looks redundant is usually carrying + a distinction. The mangled method name separates overloads; `leftType` on `div` keeps a literal + assignable to a real. Check what a name distinguishes before replacing it with a tidier one. +- The suite is the specification. Before changing what the type checker accepts, grep the tests for + the shape being rejected — item 8 looked like an oversight until one optimizer test turned out to + depend on it. - A test that hangs looks exactly like a test that is slow. If the suite stops making progress, take a thread dump of the forked worker (`jstack `) before killing it — it names the line. - Method names are not what the frontend called them. `LuaDispatchPreparation` renames a whole diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/WurstOperator.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/WurstOperator.java index d9e27d8b3..b298a8702 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/WurstOperator.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/WurstOperator.java @@ -1,6 +1,7 @@ package de.peeeq.wurstscript; import de.peeeq.wurstscript.attributes.AttrFuncDef; +import de.peeeq.wurstio.jassinterpreter.InterpreterException; import de.peeeq.wurstscript.intermediatelang.*; import de.peeeq.wurstscript.jassAst.JassAst; import de.peeeq.wurstscript.jassAst.JassOpBinary; @@ -129,6 +130,28 @@ public LuaOpBinary luaTranslateBinary() { throw new Error("cannot translate " + this); } + /** + * Refuses to compare the stand-in for a type parameter's default, whichever side it is on. + *

+ * The stand-in exists because the default of a value is computed by a static attribute, which + * cannot see what the parameter is bound to. It answers "equal" only for another stand-in, so + * comparing one against a real value is a wrong answer rather than an error. Doing this here + * rather than in the value itself keeps it independent of operand order: only the left operand + * gets asked, so `0 == unresolved` would otherwise go quietly false while `unresolved == 0` + * complained. + */ + private static void rejectUnresolvedDefault(ILconst left, ILconst right) { + ILconstUnsafeDefault unresolved = left instanceof ILconstUnsafeDefault leftDefault ? leftDefault + : right instanceof ILconstUnsafeDefault rightDefault ? rightDefault : null; + if (unresolved == null || (left instanceof ILconstUnsafeDefault && right instanceof ILconstUnsafeDefault)) { + return; + } + throw new InterpreterException("The default value of type parameter " + + unresolved.getTypeVariable().getName() + + " is not known here, so it cannot be compared to " + + (unresolved == left ? right : left).print() + "."); + } + public ILconst evaluateBinaryOperator(ILconst left, Supplier right) { switch (this) { @@ -140,8 +163,11 @@ public ILconst evaluateBinaryOperator(ILconst left, return new ILconstInt(((ILconstInt) left).getVal() / ((ILconstInt) right.get()).getVal()); case DIV_REAL: return new ILconstReal(getReal(left) / getReal(right.get())); - case EQ: - return ILconstBool.instance(left.equals(right.get())); + case EQ: { + ILconst rightVal = right.get(); + rejectUnresolvedDefault(left, rightVal); + return ILconstBool.instance(left.equals(rightVal)); + } case GREATER: return ((ILconstNum) left).greater((ILconstNum) right.get()); case GREATER_EQ: @@ -160,8 +186,11 @@ public ILconst evaluateBinaryOperator(ILconst left, return new ILconstReal(moduloReal(getReal(left), getReal(right.get()))); case MULT: return ((ILconstNum) left).mul((ILconstNum) right.get()); - case NOTEQ: - return ILconstBool.instance(!left.equals(right.get())); + case NOTEQ: { + ILconst rightVal = right.get(); + rejectUnresolvedDefault(left, rightVal); + return ILconstBool.instance(!left.equals(rightVal)); + } case PLUS: return ((ILconstAddable) left).add((ILconstAddable) right.get()); case NOT: diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/ILconstUnsafeDefault.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/ILconstUnsafeDefault.java index ca1a28f39..52cfbbb53 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/ILconstUnsafeDefault.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/ILconstUnsafeDefault.java @@ -18,13 +18,18 @@ public String print() { return "unsafe-default<" + typeVariable.getName() + ">"; } + public ImTypeVar getTypeVariable() { + return typeVariable; + } + public WurstType getType() { return WurstTypeInfer.instance(); } @Override public boolean isEqualTo(ILconst other) { + // Comparing this against a real value is refused by WurstOperator, which can see both + // operands; doing it here would depend on which side the stand-in happened to land on. return other instanceof ILconstUnsafeDefault; } - } diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/EvaluateExpr.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/EvaluateExpr.java index b4ef93be9..df15e6d64 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/EvaluateExpr.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/EvaluateExpr.java @@ -223,9 +223,9 @@ public static ILconst eval(ImVarArrayAccess e, ProgramState globalState, LocalSt } if (e.getVar().isGlobal()) { - return notNull(globalState.getArrayVal(e.getVar(), indexes), e.getVar().getType(), "Variable " + e.getVar().getName() + " is null.", false); + return globalState.resolveDefault(notNull(globalState.getArrayVal(e.getVar(), indexes), e.getVar().getType(), "Variable " + e.getVar().getName() + " is null.", false)); } else { - return notNull(localState.getArrayVal(e.getVar(), indexes), e.getVar().getType(), "Variable " + e.getVar().getName() + " is null.", false); + return globalState.resolveDefault(notNull(localState.getArrayVal(e.getVar(), indexes), e.getVar().getType(), "Variable " + e.getVar().getName() + " is null.", false)); } } @@ -292,7 +292,8 @@ public static ILconst eval(ImMemberAccess ma, ProgramState globalState, LocalSta Integer val = ((ILconstInt) i.evaluate(globalState, localState)).getVal(); indexes.add(val); } - return receiver.get(ma.getVar(), indexes).orElseGet(() -> ma.attrTyp().defaultValue()); + return globalState.resolveDefault( + receiver.get(ma.getVar(), indexes).orElseGet(() -> ma.attrTyp().defaultValue())); } public static ILconst eval(ImAlloc e, ProgramState globalState, LocalState localState) { diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/ProgramState.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/ProgramState.java index d89f0b376..9a36bb7b7 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/ProgramState.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/ProgramState.java @@ -395,6 +395,23 @@ public ImType resolveType(ImType t) { return resolveTypeDeep(t, 32); // small budget to avoid cycles } + /** + * Replaces the stand-in default of a type parameter with the default of the type bound to it. + *

+ * The default of a value is computed by a static attribute, which cannot see the frames that + * know what the parameter stands for, so it produces a stand-in. Reading a slot of a + * {@code T array} that was never written is how one reaches a program: the stand-in compares + * equal only to another stand-in, so a comparison against the real default is quietly false. + * The frames are known here, so resolve it where the value is produced. + */ + public ILconst resolveDefault(ILconst value) { + if (!(value instanceof ILconstUnsafeDefault unsafeDefault)) { + return value; + } + ImType resolved = resolveType(JassIm.ImTypeVarRef(unsafeDefault.getTypeVariable())); + return resolved instanceof ImTypeVarRef ? value : resolved.defaultValue(); + } + private ImType resolveTypeDeep(ImType t, int budget) { if (budget <= 0 || t == null) return t; diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ExpressionTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ExpressionTests.java index 6780129a7..e92b3c230 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ExpressionTests.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ExpressionTests.java @@ -486,6 +486,45 @@ private String makeProg(String booleanExpr) { return prog; } + /** + * An integer literal is a proper subtype of both int and real. Addition collapses two of them + * to int, so {@code real r = 1} is allowed while {@code real r = 1 + 1} is not; {@code div} and + * {@code mod} return the left operand's type instead, so a literal stays assignable to a real + * through them. This pins the asymmetry rather than endorsing it — see backlog item 8. + */ + @Test + public void integerDivisionOfLiteralsIsStillAssignableToReal() { + testAssertOkLines(false, + "package test", + "init", + " real quotient = 7 div 2", + " real remainder = 7 mod 2" + ); + } + + @Test + public void additionOfLiteralsIsNotAssignableToReal() { + testAssertErrorsLines(false, "Cannot assign int to real", + "package test", + "init", + " real sum = 7 + 2" + ); + } + + /** Whatever the declared type, both are integer operations at runtime. */ + @Test + public void integerDivisionAndModuloStayInt() { + testAssertOkLines(true, + "package test", + "native testSuccess()", + "init", + " int d = 7 div 2", + " int m = 7 mod 2", + " if d == 3 and m == 1", + " testSuccess()" + ); + } + public void assertOk(String booleanExpr) { String prog = makeProg(booleanExpr); testAssertOk(UtilsIO.getMethodName(1), true, prog); diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java index 021b416d7..89bcb0702 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java @@ -264,6 +264,46 @@ public void nestedClosuresInsideBoundedGenericAreRejectedForLua() { ); } + /** + * A slot of a {@code T array} that was never written reads as the default of whatever T stands + * for. The default is computed by a static attribute, which cannot see what T is bound to, so + * it produces a stand-in — and a stand-in compares equal only to another stand-in, which made + * this quietly false on the interpreter while both backends had it right. + */ + @Test + public void unwrittenArrayOfATypeParameterReadsAsItsDefault() { + testAssertOkLines(true, + "package test", + "native testSuccess()", + "class Box", + " private static T array none", + " static function first() returns T", + " return none[0]", + "init", + " if Box.first() == 0 and Box.first() == null", + " testSuccess()" + ); + } + + /** + * The same comparison the other way round. Only the left operand is asked whether it is equal, + * so a stand-in on the right would have gone quietly false while one on the left complained. + */ + @Test + public void unwrittenArrayOfATypeParameterReadsAsItsDefaultReversed() { + testAssertOkLines(true, + "package test", + "native testSuccess()", + "class Box", + " private static T array none", + " static function first() returns T", + " return none[0]", + "init", + " if 0 == Box.first() and null == Box.first()", + " testSuccess()" + ); + } + /** Each type argument picks its own instance, so one generic serves several types. */ @Test public void twoInstancesOfOneClass() { From 96f8e2ac68a4c680187c4b3686d05e0c7d3194df Mon Sep 17 00:00:00 2001 From: Frotty Date: Sun, 16 Aug 2026 10:29:31 +0200 Subject: [PATCH 10/13] Fetch the standard library before the tests fork StdLib guards its fetch with a lock and a flag held in one process, which stops guarding anything once the tests run in several. On a clean checkout each worker would clone into the same directory at the same time, so a fresh CI run could fail or leave a half-written checkout behind - a hazard this branch introduced by forking at all. The build now fetches it once, from its own JVM, before the workers start. The repository and pinned commit stay defined in StdLib rather than being repeated in the build file. The task deliberately declares no output: treating the directory as one would skip the fetch whenever it merely exists, which is exactly when it may be at the wrong commit or half cloned - the fetch checks and repairs that, and costs nothing when there is nothing to do. Verified from no checkout at all: clones, notices the wrong commit, and lands on the pinned one. Also logs the exception rather than the identity of its stack trace array, which is what that path printed while this was being diagnosed. --- de.peeeq.wurstscript/build.gradle | 19 +++++++++++++++++++ .../java/tests/wurstscript/tests/StdLib.java | 16 +++++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/de.peeeq.wurstscript/build.gradle b/de.peeeq.wurstscript/build.gradle index aa74bad29..2c26c8904 100644 --- a/de.peeeq.wurstscript/build.gradle +++ b/de.peeeq.wurstscript/build.gradle @@ -264,7 +264,26 @@ int testForkCount() { } +/** + * Fetches the standard library the tests compile against, once, before they fork. + * + * StdLib guards the fetch with a lock held in one process, which is no guard at all across + * workers: on a clean checkout each of them would clone into the same directory at the same time. + * The pinned repository and commit stay defined in that one place rather than being repeated here. + */ +tasks.register('ensureStdLib', JavaExec) { + description "Fetches the pinned standard library used by the tests" + classpath = sourceSets.test.runtimeClasspath + mainClass.set('tests.wurstscript.tests.StdLib') + workingDir = projectDir + // Deliberately not declaring the checkout as an output: that would skip this whenever the + // directory merely exists, which is precisely when it may be at the wrong commit or half + // cloned. The fetch checks the pinned commit and repairs it, and costs nothing when correct. + outputs.upToDateWhen { false } +} + test { + dependsOn 'ensureStdLib' useTestNG() // The suite is a few thousand independent compilations and was running one at a time, so it diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java index 6a4dc9b1d..c8c62412b 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java @@ -38,6 +38,19 @@ public void download() { assert(downloadStandardlib()); } + /** + * Entry point for the build, which fetches the library once before the tests fork. + *

+ * The guard below is a lock and a flag in one process, and every fork is a process of its own, + * so several of them starting on a clean checkout would clone into the same directory at the + * same time. Doing it here, from the build's own JVM, means they all find it already there. + */ + public static void main(String[] args) { + if (!downloadStandardlib()) { + throw new RuntimeException("Could not fetch the standard library the tests compile against."); + } + } + public synchronized static boolean downloadStandardlib() { if (isInitialized) { return true; @@ -72,7 +85,8 @@ public synchronized static boolean downloadStandardlib() { isInitialized = true; } catch (IOException | GitAPIException e) { - WLogger.severe(e.getStackTrace().toString()); + // The array's identity is no use to anyone; there is an overload that prints the trace. + WLogger.severe(e); return false; } From 704691ebebcdd877793b10c5194830afb75e7d0f Mon Sep 17 00:00:00 2001 From: Frotty Date: Sun, 16 Aug 2026 10:40:29 +0200 Subject: [PATCH 11/13] Repair a stale standard library checkout instead of stopping on it The fetch went by way of master, and the checkout it leaves behind is detached with no local master to go back to - so on any machine that already had one, the repair path asked for a ref that was not there and failed with "Ref master cannot be resolved". That mattered little while only the tests using the library were affected. It matters now that every test waits on this, which is a hazard this branch introduced: a stale cache would stop the whole suite. It fetches from the remote with an explicit refspec and checks out the pinned commit directly, with no branch in between. A directory that cannot be opened as a repository, or has no HEAD - what an interrupted clone leaves - is discarded and cloned again rather than treated as a cache. Failures print to the console as well. The build waits on this, so whoever reads a failed run needs to see the reason there rather than in a log file; diagnosing this one started with an exception that reported only that something went wrong. Checked against all four states: no checkout, a good one, one detached with no local master, and one with its .git removed. --- .../java/tests/wurstscript/tests/StdLib.java | 44 +++++++++++++++++-- 1 file changed, 40 insertions(+), 4 deletions(-) diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java index c8c62412b..a3715c3bf 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java @@ -51,12 +51,39 @@ public static void main(String[] args) { } } + private static boolean isUsableCheckout() { + try (Git git = Git.open(stdLibFolder)) { + return git.getRepository().resolve(Constants.HEAD) != null; + } catch (IOException | RuntimeException e) { + return false; + } + } + + private static void deleteRecursively(File file) { + File[] children = file.listFiles(); + if (children != null) { + for (File child : children) { + deleteRecursively(child); + } + } + if (!file.delete()) { + file.deleteOnExit(); + } + } + public synchronized static boolean downloadStandardlib() { if (isInitialized) { return true; } try { + // A checkout that cannot be opened or has no HEAD is worse than none: every test now + // waits on this, so a half-written directory left by an interrupted run would stop the + // whole suite rather than the few cases that need the library. + if (stdLibFolder.exists() && !isUsableCheckout()) { + System.out.println("Discarding an unusable standard library checkout at " + stdLibFolder); + deleteRecursively(stdLibFolder); + } if (!stdLibFolder.exists()) { tempFolder.mkdirs(); try (Git git = Git @@ -71,10 +98,16 @@ public synchronized static boolean downloadStandardlib() { try (Git git = Git.open(stdLibFolder)) { String head = git.getRepository().resolve(Constants.HEAD).getName(); if (!head.equals(version)) { - System.out.println("Wrong version '" + head + "', executing git pull to get '" + version + "'"); - - git.checkout().setName(Constants.MASTER).call(); - git.pull().call(); + System.out.println("Wrong version '" + head + "', fetching to get '" + version + "'"); + + // Straight to the pinned commit rather than by way of master. A checkout left + // detached - which is what this leaves behind, so it is the normal state - has + // no local master to check out, and asking for one fails with "Ref master + // cannot be resolved" before the fetch that would have created it. + git.fetch() + .setRemote(Constants.DEFAULT_REMOTE_NAME) + .setRefSpecs("+refs/heads/*:refs/remotes/origin/*") + .call(); git.checkout().setName(version).setForceRefUpdate(true).call(); } } @@ -87,6 +120,9 @@ public synchronized static boolean downloadStandardlib() { } catch (IOException | GitAPIException e) { // The array's identity is no use to anyone; there is an overload that prints the trace. WLogger.severe(e); + // And on the console too: the build waits on this now, so whoever is looking at a + // failed run needs to see why here rather than in a log file. + e.printStackTrace(System.err); return false; } From a87a8cc781e6a5253113bd7484b9e4518fa07398 Mon Sep 17 00:00:00 2001 From: Frotty Date: Sun, 16 Aug 2026 10:51:00 +0200 Subject: [PATCH 12/13] Only touch the standard library checkout when it needs repairing The clean and the checkout ran on every first call in a process, and every worker is a process, so all of them rewrote the one shared directory - each taking the index lock, while the others were reading the same files. It costs nothing to ask first: reading the status writes nothing, and a checkout that is already at the pinned commit and clean has nothing to undo. Only a worker that finds something wrong touches it. Asking first showed that what was there did not do what it said. Checking out the commit HEAD already points at does nothing, so "reset all possible changes" removed untracked files but left a modified one modified. It resets hard to the pinned commit now, which is what the comment always claimed. --- .../java/tests/wurstscript/tests/StdLib.java | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java index a3715c3bf..6792a174e 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java @@ -2,6 +2,7 @@ import de.peeeq.wurstscript.WLogger; import org.eclipse.jgit.api.Git; +import org.eclipse.jgit.api.ResetCommand; import org.eclipse.jgit.api.errors.GitAPIException; import org.eclipse.jgit.lib.Constants; import org.testng.annotations.Test; @@ -95,9 +96,11 @@ public synchronized static boolean downloadStandardlib() { } } + boolean repaired = false; try (Git git = Git.open(stdLibFolder)) { String head = git.getRepository().resolve(Constants.HEAD).getName(); if (!head.equals(version)) { + repaired = true; System.out.println("Wrong version '" + head + "', fetching to get '" + version + "'"); // Straight to the pinned commit rather than by way of master. A checkout left @@ -112,9 +115,19 @@ public synchronized static boolean downloadStandardlib() { } } - // reset all possible changes - Git.open(stdLibFolder).clean().setForce(true).setCleanDirectories(true).setIgnore(false).call(); - Git.open(stdLibFolder).checkout().setName(version).call(); + // Undo whatever a previous run left behind, but only when there is something to undo. + // Every worker is its own process and so runs this once; unconditionally cleaning and + // checking out means several of them rewriting one directory at the same time, each + // taking the index lock, while other workers are reading the same files. Reading the + // status does not write anything, and the ordinary case has nothing to repair. + try (Git git = Git.open(stdLibFolder)) { + if (repaired || !git.status().call().isClean()) { + // Reset rather than checkout: checking out the commit HEAD already points at + // does nothing, so a modified file survived what claimed to undo it. + git.reset().setMode(ResetCommand.ResetType.HARD).setRef(version).call(); + git.clean().setForce(true).setCleanDirectories(true).setIgnore(false).call(); + } + } isInitialized = true; } catch (IOException | GitAPIException e) { From 645b9432996d6ead113dbf1bd2547d5ac5c1e75d Mon Sep 17 00:00:00 2001 From: Frotty Date: Sun, 16 Aug 2026 11:54:57 +0200 Subject: [PATCH 13/13] Clean the library once for everyone, and let a focused run skip the fetch Asking whether the checkout is clean does not notice ignored files, so guarding the clean on that alone stopped build leftovers being removed. That is not cosmetic: the compiler scans the library folder recursively and puts every .wurst file it finds under its package name, so a stale _build can quietly take a pinned package's place. Removing them per worker is what the guard was avoiding, though. The prefetch does it instead - it runs alone, before the workers, which is the one moment rewriting that directory disturbs nobody. Workers still repair a checkout at the wrong commit or with modified files, which is what a run outside the build sees. The fetch is also skippable with -PskipStdLibFetch, so a focused run of tests that never touch the library is not stopped by being unable to reach GitHub. --- de.peeeq.wurstscript/build.gradle | 5 +++++ .../java/tests/wurstscript/tests/StdLib.java | 18 ++++++++++++++++-- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/de.peeeq.wurstscript/build.gradle b/de.peeeq.wurstscript/build.gradle index 2c26c8904..9fd42ad1e 100644 --- a/de.peeeq.wurstscript/build.gradle +++ b/de.peeeq.wurstscript/build.gradle @@ -276,6 +276,11 @@ tasks.register('ensureStdLib', JavaExec) { classpath = sourceSets.test.runtimeClasspath mainClass.set('tests.wurstscript.tests.StdLib') workingDir = projectDir + // An escape hatch for working without the network: a focused run of tests that never touch + // the library should not be stopped by not being able to reach GitHub. Read now rather than + // when the task runs, which the configuration cache does not allow. + def skipFetch = project.hasProperty('skipStdLibFetch') + onlyIf { !skipFetch } // Deliberately not declaring the checkout as an output: that would skip this whenever the // directory merely exists, which is precisely when it may be at the wrong commit or half // cloned. The fetch checks the pinned commit and repairs it, and costs nothing when correct. diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java index 6792a174e..c418fe53a 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java @@ -47,7 +47,9 @@ public void download() { * same time. Doing it here, from the build's own JVM, means they all find it already there. */ public static void main(String[] args) { - if (!downloadStandardlib()) { + // Deep clean here and only here: this runs alone, before the workers, so it is the one + // moment when rewriting the directory disturbs nobody. + if (!ensureCheckout(true)) { throw new RuntimeException("Could not fetch the standard library the tests compile against."); } } @@ -73,6 +75,18 @@ private static void deleteRecursively(File file) { } public synchronized static boolean downloadStandardlib() { + return ensureCheckout(false); + } + + /** + * @param deepClean also discard ignored build artefacts. Only the build's prefetch asks for + * this. A worker must not: the artefacts are what a previous run left inside the library + * folder, and removing them while other workers are reading it is the contention this + * avoids. Leaving them is not harmless either - the compiler scans the library folder + * recursively and a stale `_build` can hold `.wurst` files that take a pinned package's + * name - which is why the prefetch does it once for everyone. + */ + private synchronized static boolean ensureCheckout(boolean deepClean) { if (isInitialized) { return true; } @@ -121,7 +135,7 @@ public synchronized static boolean downloadStandardlib() { // taking the index lock, while other workers are reading the same files. Reading the // status does not write anything, and the ordinary case has nothing to repair. try (Git git = Git.open(stdLibFolder)) { - if (repaired || !git.status().call().isClean()) { + if (repaired || deepClean || !git.status().call().isClean()) { // Reset rather than checkout: checking out the commit HEAD already points at // does nothing, so a modified file survived what claimed to undo it. git.reset().setMode(ResetCommand.ResetType.HARD).setRef(version).call();