From 4e690c0e336a4e360ba96f4edf9e8da2c6d2b343 Mon Sep 17 00:00:00 2001 From: Frotty Date: Sat, 15 Aug 2026 03:39:46 +0200 Subject: [PATCH 01/12] WIP: FastHashMap as a compiler-side proof MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Exercises type class bounds through the container they were added for. Six of the seven cases pass on both backends with no compiler change: int keys, tuple keys on Jass, a user class key, two specialisations coexisting, and two instances of one specialisation. tupleKeyLua fails and is a real, pre-existing backend bug. Method names become Lua table keys, but luaMethod.initFor passes the name through raw while every sibling (luaVar, luaFunc, luaClassVar) sanitises via uniqueName. Names are valid identifiers in ordinary code, so nothing hit it until a class method was specialised for Lua with more than one type argument: specializeMethod builds name + "_specialized_" + generics.makeName(), and makeName joins arguments with ", ". Two simple arguments give "get_specialized_integer, integer", which emits "Class.get_specialized_integer, integer = impl" -- valid Lua that assigns to two targets and quietly writes a junk global. A tuple argument gives "⦅integer, integer⦆" and fails the syntax check outright. --- .../wurstscript/tests/FastHashMapTests.java | 207 ++++++++++++++++++ 1 file changed, 207 insertions(+) create mode 100644 de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/FastHashMapTests.java 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()" + })); + } +} From 3ccbba245f652eb548ebfc047b6d0759ff79f7f3 Mon Sep 17 00:00:00 2001 From: Frotty Date: Sat, 15 Aug 2026 03:44:47 +0200 Subject: [PATCH 02/12] Add a working backlog for the type class bound follow-ups --- BACKLOG.md | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 BACKLOG.md diff --git a/BACKLOG.md b/BACKLOG.md new file mode 100644 index 000000000..753288587 --- /dev/null +++ b/BACKLOG.md @@ -0,0 +1,70 @@ +# 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 + +1. **Lua method names reach the backend unsanitised.** `LuaTranslator.luaMethod.initFor` + passes `a.getName()` raw; `luaVar`, `luaFunc` and `luaClassVar` all sanitise via + `uniqueName`. Method names become Lua table keys, so they must be valid identifiers. + `EliminateGenerics.specializeMethod` builds `name + "_specialized_" + generics.makeName()` + on the Lua path, and `makeName` joins type arguments with `", "` — so a class method + specialised with two type arguments emits `Class.get_specialized_integer, integer = impl`, + which is valid Lua assigning to two targets, and a tuple argument emits `⦅⦆` and fails + the syntax check. Repro: `FastHashMapTests.tupleKeyLua`. Commas are already visible in + `test-output/lua/FastHashMapTests_fastHashMapRuntimeLua.lua`. + Overriding methods must keep landing in the same slot, so normalise per distinct original + name, not per method node. + +2. **`slotFor` looks bound to `get`'s implementation** in the same emitted Lua. May be a real + mis-binding in `specializeMethod`/`adaptSubmethods`, may be an artefact of item 1 mangling + the output. Diagnose only after item 1, from freshly emitted Lua. + +3. **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`. + +4. **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. + +5. **Lua dispatch inside the constructor** of a bounded generic class. Works on Jass. + +6. **Module bounds.** `module M` is rejected with a clear message today. Needs + receiver rewriting during expansion, or type parameters on `ModuleInstanciation`. + +7. **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. + +8. **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. + +## Done + +- 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 8. +- 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. From 6fac1f027073e4225f76280351a3e7dbb65a996d Mon Sep 17 00:00:00 2001 From: Frotty Date: Sat, 15 Aug 2026 03:48:38 +0200 Subject: [PATCH 03/12] Expand the backlog with a recurrence guard, design blockers and scope --- BACKLOG.md | 59 +++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 49 insertions(+), 10 deletions(-) diff --git a/BACKLOG.md b/BACKLOG.md index 753288587..286550bec 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -22,36 +22,75 @@ Notes rather than leaving it in a commit message. Overriding methods must keep landing in the same slot, so normalise per distinct original name, not per method node. -2. **`slotFor` looks bound to `get`'s implementation** in the same emitted Lua. May be a real +2. **Nothing checks that emitted Lua identifiers are valid.** The luac syntax check catches a + hard break, but not the silent case: `Class.get_specialized_integer, integer = impl` parses + fine and quietly assigns to two targets. That is how item 1 shipped unnoticed. Assert in the + Lua test harness that every emitted name — variable, function, class, method slot, field — + matches `[A-Za-z_][A-Za-z0-9_]*`, so this whole class of bug fails loudly at the point it is + introduced. Do this alongside item 1; it is what stops item 1 recurring. + +3. **`slotFor` looks bound to `get`'s implementation** in the same emitted Lua. May be a real mis-binding in `specializeMethod`/`adaptSubmethods`, may be an artefact of item 1 mangling the output. Diagnose only after item 1, from freshly emitted Lua. -3. **Finish the FastHashMap proof.** `FastHashMapTests` is the first real use of bounds. +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`. -4. **Lua dispatch inside a closure.** Works on Jass since #1229. On Lua the specialised class +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. -5. **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. -6. **Module bounds.** `module M` is rejected with a clear message today. Needs +7. **Module bounds.** `module M` is rejected with a clear message today. Needs receiver rewriting during expansion, or type parameters on `ModuleInstanciation`. -7. **One `ImTypeVar` per type parameter.** Name-tolerant lookups remain in +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. -8. **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. +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 From e4da96d7d5d032d07da28a51fc9be608f6b12bed Mon Sep 17 00:00:00 2001 From: Frotty Date: Sat, 15 Aug 2026 03:52:22 +0200 Subject: [PATCH 04/12] Add the overnight loop brief --- .claude/ralph-loop.local.md | 10 ++++++++ LOOP.md | 48 +++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) create mode 100644 .claude/ralph-loop.local.md create mode 100644 LOOP.md diff --git a/.claude/ralph-loop.local.md b/.claude/ralph-loop.local.md new file mode 100644 index 000000000..305f01377 --- /dev/null +++ b/.claude/ralph-loop.local.md @@ -0,0 +1,10 @@ +--- +active: true +iteration: 1 +session_id: 0413fc86-e9a2-4954-bc47-1620ed81d95e +max_iterations: 0 +completion_promise: null +started_at: "2026-08-15T01:51:05Z" +--- + +Work on the WurstScript compiler at c:Usersrun-eDocumentsGitHubWurstScript, on the branch feat/fasthashmap-proof. Read AGENTS.md and BACKLOG.md before doing anything. diff --git a/LOOP.md b/LOOP.md new file mode 100644 index 000000000..ed3234d69 --- /dev/null +++ b/LOOP.md @@ -0,0 +1,48 @@ +# Overnight loop brief + +Work on the WurstScript compiler in this repository, on the branch +`feat/fasthashmap-proof`. Read `AGENTS.md` and `BACKLOG.md` before doing anything. + +## Each iteration + +1. Read `BACKLOG.md`. Take the top item that isn't blocked. +2. Do it. Root-cause it — adjust the underlying system rather than patching a symptom. + If it turns out to be bigger than one iteration, split it in the backlog and do the + first part. +3. Verify. Targeted tests while iterating; the full suite before any commit that touches + main source: + + cd de.peeeq.wurstscript && ./gradlew test + + It must be green. Emitted Lua must stay byte-identical unless the change is meant to + alter it — compare two runs to check. Do not diff `.j` across runs; it is not stable, + and backlog item 11 explains why. +4. Commit and push. Small commits, one concern each. Never end an iteration with + uncommitted work. +5. Update `BACKLOG.md`: move finished items to Done with one line on what actually + happened, add anything learned to Notes, reorder if something more urgent turned up. + +## Rules + +- Never force-push, rewrite history, merge, push to master, or open a PR. The branch gets + reviewed in the morning. +- Commits are authored as the repository owner. No AI, assistant, or co-author references + anywhere in commit messages or code comments. +- Never stop to ask. If something needs a decision from the owner, write the question into + `BACKLOG.md` under that item, mark it blocked, and move to the next item. +- If the full suite goes red and it can't be fixed within the iteration, revert the change, + note why in `BACKLOG.md`, and move on. Leave the branch green. +- Comments explain why, not what. Match the surrounding style. +- Do not touch `de.peeeq.wurstscript/temp/WurstStdlib2` — a fetched test artefact, editing + it changes nothing real. + +## Keeping going + +- Finishing an item is not the end of the run. Go back to step 1 and take the next one. Do + not stop to summarise, and do not treat a green suite as a finish line. +- At most one item may be marked blocked per iteration, and only if it genuinely needs a + decision rather than more work. If everything left looks blocked, that is wrong about at + least one of them — re-read and start the one that can be moved furthest. +- If an item goes three iterations without landing a commit, split it in the backlog and + move on. Don't spend the whole run on one thing. +- Backlog item 12 is a standing item that never completes. There is always a next item. From cc994da12d2235f0f04fcdd5f7944cc0fdfd3222 Mon Sep 17 00:00:00 2001 From: Frotty Date: Sat, 15 Aug 2026 04:15:00 +0200 Subject: [PATCH 05/12] Sanitise Lua method names where they are assigned Method names become Lua table keys, so they must be identifiers. A method specialised with two type arguments was named after them, commas included, and emitted `Class.get_specialized_integer, integer = impl` - valid Lua that quietly assigns to two targets; a tuple argument produced characters luac rejects outright. normalizeMethodNames is the pass that gives one name to a whole dispatch group, so it sanitises before uniquing: two names that differed only in characters Lua has no place for still get a slot each. The backend maps every slot key and every LuaMethod name through the same function, so call sites and class tables keep agreeing. Lua's identifier rule now has one home. The luac check never caught this, because the broken output parses. Assert instead on the names themselves: every emitted function, method, variable, field and call-by-name must be an identifier, checked for every testLua compile. --- BACKLOG.md | 52 ++++++++------ .../imtranslation/LuaDispatchPreparation.java | 5 +- .../lua/translation/LuaAssertions.java | 70 +++++++++++++++++++ .../lua/translation/LuaIdentifiers.java | 54 ++++++++++++++ .../lua/translation/LuaTranslator.java | 45 +++++------- .../wurstscript/tests/WurstScriptTest.java | 2 + 6 files changed, 177 insertions(+), 51 deletions(-) create mode 100644 de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaIdentifiers.java diff --git a/BACKLOG.md b/BACKLOG.md index 286550bec..ba7ec03be 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -10,28 +10,22 @@ Notes rather than leaving it in a commit message. ## Todo -1. **Lua method names reach the backend unsanitised.** `LuaTranslator.luaMethod.initFor` - passes `a.getName()` raw; `luaVar`, `luaFunc` and `luaClassVar` all sanitise via - `uniqueName`. Method names become Lua table keys, so they must be valid identifiers. - `EliminateGenerics.specializeMethod` builds `name + "_specialized_" + generics.makeName()` - on the Lua path, and `makeName` joins type arguments with `", "` — so a class method - specialised with two type arguments emits `Class.get_specialized_integer, integer = impl`, - which is valid Lua assigning to two targets, and a tuple argument emits `⦅⦆` and fails - the syntax check. Repro: `FastHashMapTests.tupleKeyLua`. Commas are already visible in - `test-output/lua/FastHashMapTests_fastHashMapRuntimeLua.lua`. - Overriding methods must keep landing in the same slot, so normalise per distinct original - name, not per method node. - -2. **Nothing checks that emitted Lua identifiers are valid.** The luac syntax check catches a - hard break, but not the silent case: `Class.get_specialized_integer, integer = impl` parses - fine and quietly assigns to two targets. That is how item 1 shipped unnoticed. Assert in the - Lua test harness that every emitted name — variable, function, class, method slot, field — - matches `[A-Za-z_][A-Za-z0-9_]*`, so this whole class of bug fails loudly at the point it is - introduced. Do this alongside item 1; it is what stops item 1 recurring. - -3. **`slotFor` looks bound to `get`'s implementation** in the same emitted Lua. May be a real - mis-binding in `specializeMethod`/`adaptSubmethods`, may be an artefact of item 1 mangling - the output. Diagnose only after item 1, from freshly emitted Lua. +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 @@ -94,6 +88,14 @@ Notes rather than leaving it in a commit message. ## 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. @@ -101,7 +103,11 @@ Notes rather than leaving it in a commit message. - `%` 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 8. + 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. 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..95cde0a8b --- /dev/null +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaIdentifiers.java @@ -0,0 +1,54 @@ +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. */ + 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 true; + } + + /** 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, '_'); + } + 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/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 596b6fae6ce66d2964cd8e0b41582cb395f3bfe0 Mon Sep 17 00:00:00 2001 From: Frotty Date: Sat, 15 Aug 2026 04:45:31 +0200 Subject: [PATCH 06/12] Bound the wait on the Lua interpreter in tests The execution path read the spawned process's stderr to EOF before touching stdout, and never bounded the wait. A program that fills the stdout pipe blocks writing while the harness blocks reading stderr, and the suite stops with no output, no timeout and no failing test - a stray worker JVM was still sitting on the build directory twenty minutes later. checkLuaSyntax, in the same file, already drained both pipes on their own threads and waited with a timeout. The execution path now uses the same helper, so a program that does not terminate fails its test instead of the run. Also records what building a repro turned up: a bounded generic class cannot be subclassed on either backend, and the div/mod result type asymmetry is reachable through integer literals rather than harmless. --- BACKLOG.md | 50 +++++++++++++++++-- .../wurstscript/tests/WurstScriptTest.java | 32 ++++++------ 2 files changed, 65 insertions(+), 17 deletions(-) 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/WurstScriptTest.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/WurstScriptTest.java index 545f28c4d..da1c2a9e7 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,9 @@ 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; private static volatile String resolvedLuaExecutable; private static volatile String resolvedLuacExecutable; private static volatile String extractedLuaWin; @@ -527,7 +530,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,26 +552,28 @@ 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. + Thread outCollector = collectStreamAsync(p.getInputStream(), output); + Thread errCollector = collectStreamAsync(p.getErrorStream(), errors); + if (!p.waitFor(LUA_EXECUTION_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { + p.destroyForcibly(); + throw new Error(currentTestEnv + ": Lua program did not terminate within " + + LUA_EXECUTION_TIMEOUT_SECONDS + "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"); + for (String outputLine : output.toString().split("\n", -1)) { + if (outputLine.equals("testSuccess")) { + success = true; } } if (!success) { From a12ee4ce4aee376af8472c9664d0ef63814de40f Mon Sep 17 00:00:00 2001 From: Frotty Date: Sat, 15 Aug 2026 05:00:19 +0200 Subject: [PATCH 07/12] Let declared names decide which dispatch slot a method claims A method claims the slots of every method it might be an override of, and sharesSemanticName accepted a match on either the declared name or the segment after the last underscore of the mangled one. For a specialised method that segment is a fragment of the type argument: get and slotFor of FastHashMap both read as 'integer', and since specialisation gave them the same signature too, get claimed slotFor's slot and the class table bound FastHashMap_slotFor_specialized_integer__integer to FastHashMap_get_specialized. Declared names now settle it whenever both methods have one. The segment stays as the fallback for closures and bridges, which have no declaration to ask - that is the case it was there for. The emitted Lua is the only record of which implementation a slot should hold, so the test asserts it there: every slot named after a method must bind an implementation named after the same method. --- BACKLOG.md | 31 +++++++------ .../imtranslation/LuaDispatchPreparation.java | 43 +++++++++++++++--- .../wurstscript/tests/FastHashMapTests.java | 44 ++++++++++++++++++- 3 files changed, 99 insertions(+), 19 deletions(-) diff --git a/BACKLOG.md b/BACKLOG.md index 7d6c0ad50..620023adb 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -13,19 +13,14 @@ 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. +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. 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 @@ -124,6 +119,16 @@ because `LOOP.md` refers to items by number. ## Done +- 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..2c8ea988d 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. @@ -101,8 +108,43 @@ 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"}; + + 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 + "_"); } /** From 5c234916bc16ca44771c22a81794782f7c9f5c6e Mon Sep 17 00:00:00 2001 From: Frotty Date: Sat, 15 Aug 2026 05:21:33 +0200 Subject: [PATCH 08/12] Finish the FastHashMap proof with remove and a cost assertion remove leaves a tombstone rather than an empty slot, because a probe that stopped at one would miss keys put down beyond it. slotFor passes tombstones over when searching and returns the first one when putting, so removing and re-putting a key reuses its slot instead of lengthening the run. The probe is bounded by capacity, so a table full of tombstones cannot spin. The cost claim is asserted on the least optimised configuration, because it has to hold by construction rather than because the inliner removed it: storage is four plain Jass arrays, no hashtable native is reached for, slotFor takes nothing beyond the receiver and the key, and both requirements are direct calls rather than ExecuteFunc or a dispatch wrapper. With optimisation on, hash(key) becomes key and equals(a, key) becomes a != key. The dispatch_ functions in the output are the nullpointer check every class method gets, not type class dispatch, so the test looks at the real function underneath rather than asserting they are absent. Building the fixture also turned up a silent wrong result in the interpreter, recorded as item 16: a never-written array of a type parameter compares unequal to the type argument's default. --- BACKLOG.md | 36 ++++- .../wurstscript/tests/FastHashMapTests.java | 133 +++++++++++++++++- 2 files changed, 159 insertions(+), 10 deletions(-) diff --git a/BACKLOG.md b/BACKLOG.md index 620023adb..1d7a5d24c 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -13,6 +13,27 @@ 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() + "_" + semanticNameFromMethodName(name)`, and for a specialised method that trailing segment is the @@ -22,11 +43,6 @@ 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. -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. @@ -119,6 +135,16 @@ 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 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 2c8ea988d..4b4938434 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 @@ -35,30 +35,65 @@ 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)", " 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", }; @@ -113,7 +148,7 @@ public void fastHashMapRuntimeLua() throws IOException { assertEachSlotBindsItsOwnMethod(compiledLua("fastHashMapRuntimeLua")); } - private static final String[] METHOD_NAMES = {"slotFor", "put", "get", "has", "size"}; + 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); @@ -147,6 +182,35 @@ private static boolean namesMethod(String name, String 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()", + }; + + @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)); + } + /** * 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. @@ -246,4 +310,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 08:37:19 +0200 Subject: [PATCH 09/12] Reject Lua keywords when validating a generated name Wurst and Lua reserve different words, so a method can be declared repeat or goto and reach the backend under that name. Method names survive it, because the pass that assigns them uniques against the reserved set. A closure does not: it adds the name it implements as a dispatch alias directly, so the alias arrives as a bare keyword and is emitted as a table key. luac rejects that, so it was loud rather than wrong, but the check added alongside it accepted the name - and catching this before the syntax check is the whole point of having it. isValid now rejects keywords and toIdentifier maps them out of the way. Underscores rather than a counter, so a keyword maps to the same name wherever it is derived: call sites and class tables have to agree without consulting each other. --- .../lua/translation/LuaIdentifiers.java | 17 ++++++++++++++-- .../wurstscript/tests/TypeClassTests.java | 20 +++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) 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 index 95cde0a8b..b0da4381f 100644 --- 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 @@ -13,7 +13,14 @@ 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. */ + /** + * 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; @@ -23,7 +30,7 @@ public static boolean isValid(String name) { return false; } } - return true; + return !LuaReservedNames.LUA_KEYWORDS.contains(name); } /** Maps any name onto a Lua identifier, leaving names that already are one untouched. */ @@ -39,6 +46,12 @@ public static String toIdentifier(String name) { 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(); } 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() { From c1ac493a7bb17634e6c54617007dfbf6afc07532 Mon Sep 17 00:00:00 2001 From: Frotty Date: Sun, 16 Aug 2026 08:45:51 +0200 Subject: [PATCH 10/12] Cover the bounded Lua runner, and cap what it keeps Two tests for the runner itself, both Lua only: run through the Jass configurations the non-terminating one would hang the interpreter instead, which is the same problem somewhere this fix does not reach. - a program that loops forever fails its own test rather than the run - a program that prints twenty thousand lines still finishes The second is the deadlock this change was for: with the streams read one after the other, the program blocks writing stdout while the runner blocks reading stderr. The timeout is overridable so these take seconds rather than a minute. Output kept for the failure message is capped. Draining still never stops, since stopping is what blocks the process, but a program that loops while printing would otherwise exhaust the worker before the timeout fires. Capping meant success could no longer be read back out of the retained text - testSuccess prints last, well past the limit - so the line is recognised while draining. Which is better regardless: whether a program succeeded no longer depends on how much of its output was kept. --- .../wurstscript/tests/LuaRunnerTests.java | 63 +++++++++++++++++++ .../wurstscript/tests/WurstScriptTest.java | 54 ++++++++++++---- 2 files changed, 106 insertions(+), 11 deletions(-) create mode 100644 de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaRunnerTests.java 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..a719745b5 --- /dev/null +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaRunnerTests.java @@ -0,0 +1,63 @@ +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()" + ); + } +} 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 da1c2a9e7..320a9a375 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 @@ -51,6 +51,18 @@ public class WurstScriptTest { /** 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; @@ -555,12 +567,14 @@ private void translateAndTestLua(String name, boolean executeProg, WurstGui gui, // 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. - Thread outCollector = collectStreamAsync(p.getInputStream(), output); + 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(LUA_EXECUTION_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { + if (!p.waitFor(luaExecutionTimeoutSeconds(), TimeUnit.SECONDS)) { p.destroyForcibly(); throw new Error(currentTestEnv + ": Lua program did not terminate within " - + LUA_EXECUTION_TIMEOUT_SECONDS + "s: " + luaFile.getName()); + + luaExecutionTimeoutSeconds() + "s: " + luaFile.getName()); } outCollector.join(); errCollector.join(); @@ -570,13 +584,7 @@ private void translateAndTestLua(String name, boolean executeProg, WurstGui gui, throw new TestFailException(errors.toString()); } - boolean success = false; - for (String outputLine : output.toString().split("\n", -1)) { - if (outputLine.equals("testSuccess")) { - success = true; - } - } - if (!success) { + if (!sawTestSuccess.get()) { throw new Error(currentTestEnv + ": Succeed function not called"); } } @@ -625,11 +633,35 @@ 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; + boolean truncated = false; while ((line = input.readLine()) != null) { - out.append(line).append("\n"); + if (watchedLine != null && watchedLine.equals(line)) { + sawWatchedLine.set(true); + } + if (out.length() < RETAINED_OUTPUT_LIMIT) { + out.append(line).append("\n"); + } else if (!truncated) { + truncated = true; + out.append("... further output dropped after ") + .append(RETAINED_OUTPUT_LIMIT).append(" characters\n"); + } } } catch (IOException ignored) { } From 9f16d43c8d364cef01d32cf0fa018bb1cfc7dfc4 Mon Sep 17 00:00:00 2001 From: Frotty Date: Sun, 16 Aug 2026 08:47:58 +0200 Subject: [PATCH 11/12] Check the full-table sentinel in put as well slotFor says a key has nowhere to go by returning a slot below the instance's section, and get, has and remove all check for it. put did not, so a ninth distinct key wrote through the sentinel: an entry nothing can reach, a count of nine, and a negative array index on targets less forgiving than Lua. The test fills the map exactly - keys one to eight hash to the eight slots - and puts a ninth. It does not read the absent key back, because reading one of a type parameter returns a stand-in the interpreter cannot compare; that is a separate bug, fixed later in this stack. --- .../wurstscript/tests/FastHashMapTests.java | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) 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 4b4938434..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 @@ -72,6 +72,10 @@ private static String[] fastHashMap(String... extra) { " 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", @@ -201,6 +205,28 @@ private static boolean namesMethod(String name, String method) { " 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)); From a587e86c960dafc8054eb7631ced79911dd353be Mon Sep 17 00:00:00 2001 From: Frotty Date: Sun, 16 Aug 2026 09:05:33 +0200 Subject: [PATCH 12/12] Keep local session files out of the repository Neither of these belongs in a branch that gets merged. The loop configuration is machine-local state - an active session id, an iteration count, a path and a branch name from one checkout - and another checkout picking it up would inherit an automation session that has nothing to do with it. The brief beside it is one run's instructions, not documentation of anything. Both came in with the branch this work was based on rather than with the work. Ignored as well, so they do not come back the next time either is written. --- .claude/ralph-loop.local.md | 10 -------- .gitignore | 2 ++ LOOP.md | 48 ------------------------------------- 3 files changed, 2 insertions(+), 58 deletions(-) delete mode 100644 .claude/ralph-loop.local.md delete mode 100644 LOOP.md diff --git a/.claude/ralph-loop.local.md b/.claude/ralph-loop.local.md deleted file mode 100644 index 305f01377..000000000 --- a/.claude/ralph-loop.local.md +++ /dev/null @@ -1,10 +0,0 @@ ---- -active: true -iteration: 1 -session_id: 0413fc86-e9a2-4954-bc47-1620ed81d95e -max_iterations: 0 -completion_promise: null -started_at: "2026-08-15T01:51:05Z" ---- - -Work on the WurstScript compiler at c:Usersrun-eDocumentsGitHubWurstScript, on the branch feat/fasthashmap-proof. Read AGENTS.md and BACKLOG.md before doing anything. 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/LOOP.md b/LOOP.md deleted file mode 100644 index ed3234d69..000000000 --- a/LOOP.md +++ /dev/null @@ -1,48 +0,0 @@ -# Overnight loop brief - -Work on the WurstScript compiler in this repository, on the branch -`feat/fasthashmap-proof`. Read `AGENTS.md` and `BACKLOG.md` before doing anything. - -## Each iteration - -1. Read `BACKLOG.md`. Take the top item that isn't blocked. -2. Do it. Root-cause it — adjust the underlying system rather than patching a symptom. - If it turns out to be bigger than one iteration, split it in the backlog and do the - first part. -3. Verify. Targeted tests while iterating; the full suite before any commit that touches - main source: - - cd de.peeeq.wurstscript && ./gradlew test - - It must be green. Emitted Lua must stay byte-identical unless the change is meant to - alter it — compare two runs to check. Do not diff `.j` across runs; it is not stable, - and backlog item 11 explains why. -4. Commit and push. Small commits, one concern each. Never end an iteration with - uncommitted work. -5. Update `BACKLOG.md`: move finished items to Done with one line on what actually - happened, add anything learned to Notes, reorder if something more urgent turned up. - -## Rules - -- Never force-push, rewrite history, merge, push to master, or open a PR. The branch gets - reviewed in the morning. -- Commits are authored as the repository owner. No AI, assistant, or co-author references - anywhere in commit messages or code comments. -- Never stop to ask. If something needs a decision from the owner, write the question into - `BACKLOG.md` under that item, mark it blocked, and move to the next item. -- If the full suite goes red and it can't be fixed within the iteration, revert the change, - note why in `BACKLOG.md`, and move on. Leave the branch green. -- Comments explain why, not what. Match the surrounding style. -- Do not touch `de.peeeq.wurstscript/temp/WurstStdlib2` — a fetched test artefact, editing - it changes nothing real. - -## Keeping going - -- Finishing an item is not the end of the run. Go back to step 1 and take the next one. Do - not stop to summarise, and do not treat a green suite as a finish line. -- At most one item may be marked blocked per iteration, and only if it genuinely needs a - decision rather than more work. If everything left looks blocked, that is wrong about at - least one of them — re-read and start the one that can be moved furthest. -- If an item goes three iterations without landing a commit, split it in the backlog and - move on. Don't spend the whole run on one thing. -- Backlog item 12 is a standing item that never completes. There is always a next item.