diff --git a/BACKLOG.md b/BACKLOG.md index ba7ec03be..7d6c0ad50 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -45,9 +45,11 @@ because `LOOP.md` refers to items by number. 8. **`MOD_INT`/`DIV_INT` return the left operand's type** rather than `int` (`AttrExprType.java`, the `case MOD_INT` branch), where `caseMathOperation` returns - `WurstTypeInt.instance()` for `+`, `-`, `*`. Only observable if something is a proper - subtype of int, so it may be harmless — establish whether it is reachable, then either fix - it or leave a comment saying why the asymmetry is intended. Small. + `WurstTypeInt.instance()` for `+`, `-`, `*`. It *is* reachable: `WurstTypeIntLiteral` is a + proper subtype of both int and real, and `caseMathOperation` collapses two literals to `int` + precisely so `real r = 1 + 1` stays an error. Returning `leftType` skips that collapse, so + `real r = 7 div 2` and `real r = 7 mod 2` should be accepted where `+` is rejected. Confirm + with a test first — that is the failing repro — then return `WurstTypeInt.instance()`. Small. 9. **Keep `WURST_LANGUAGE.md` and `CHANGELOG.md` current** as items land. The bounds section says nothing about closures, which now work on Jass. Fold this into whichever item changes @@ -64,6 +66,40 @@ because `LOOP.md` refers to items by number. many tests ran before. Not wrong for compiling one map, but it means `.j` cannot be diffed across runs to validate a change — only `.lua` can. Fixing it would make Jass diffable. +13. **A bounded generic class cannot be subclassed.** Found while building a repro for item 3; + both backends break, differently, on the same program: + + interface Show + function show(T x) returns int + implements Show + function show(int x) returns int + return x + class Box + K key + construct(K k) + key = k + function size(int extra) returns int + return K.show(key) + extra + function shift(int extra) returns int + return 1000 + extra + class SubBox extends Box + construct(int k) + super(k) + override function size(int extra) returns int + return super.size(extra) + 100 + init + Box b = new Box(5) + Box s = new SubBox(5) + if b.size(1) == 6 and b.shift(1) == 1001 and s.size(1) == 106 + testSuccess() + + Jass fails to compile: `Typevar dispatch not eliminated.` Lua compiles and runs but never + reaches `testSuccess`: the override makes `size` dispatched, and the emitted call is + `b:Box_size_specialized_integer(1)` while `b` was allocated from the *erased* `Box` table, + which binds only `shift`. The specialised table `Box_specialized_integer` has the slot; the + instance never gets that table. Split this if the two turn out to have separate causes — + the Jass one is loud and probably the smaller of the two. + 12. **Standing item, never finished.** When nothing above is left, find the next thing worth doing and add it here rather than stopping. Good sources, in order: a test that would have caught a bug already found; a place where two mechanisms do the same job and disagree; a @@ -88,6 +124,12 @@ because `LOOP.md` refers to items by number. ## Done +- 14. The Lua execution harness no longer hangs. It read the spawned interpreter's stderr to EOF + before touching stdout, and never bounded the wait, so a program that filled the stdout pipe + deadlocked the whole suite with no output and no timeout — a stray JVM was still sitting on it + twenty minutes later. `checkLuaSyntax`, ten lines further down the same file, already drained + both pipes concurrently and waited with a timeout; the execution path now uses the same + helper. Verified against the item 13 repro: hung indefinitely before, fails in 8s after. - 1 + 2. Lua method names are sanitised where they are assigned, not where they are printed. `LuaDispatchPreparation.normalizeMethodNames` is the pass that gives one name to a whole dispatch group, so it now sanitises before uniquing — two names differing only in characters @@ -104,6 +146,8 @@ because `LOOP.md` refers to items by number. - `%` is real modulo in Wurst; `mod` is integer modulo. `int % 8` types as `real`. - Emitted Lua must be byte-identical for identical input (AGENTS.md §8). It is the only emitted output that can be diffed across runs — see item 11. +- A test that hangs looks exactly like a test that is slow. If the suite stops making progress, + take a thread dump of the forked worker (`jstack `) before killing it — it names the line. - Method names are not what the frontend called them. `LuaDispatchPreparation` renames a whole dispatch group to one name and attaches alias sets, and only then does the backend run. A question about which Lua slot something lands in is a question about that pass, not about diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaRunnerTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaRunnerTests.java new file mode 100644 index 000000000..11655e5ad --- /dev/null +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaRunnerTests.java @@ -0,0 +1,85 @@ +package tests.wurstscript.tests; + +import org.testng.annotations.Test; + +import static org.testng.Assert.assertTrue; +import static org.testng.Assert.fail; + +/** + * Tests for the harness that runs emitted Lua, rather than for anything it compiles. + *

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

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

+ * Whatever the caller is looking for is recognised here rather than read back out of + * {@code out} afterwards, because it may arrive after the limit: a program that prints a + * million lines and then succeeds has still succeeded. + */ + private Thread collectStreamAsync(InputStream stream, StringBuilder out, + String watchedLine, java.util.concurrent.atomic.AtomicBoolean sawWatchedLine) { Thread t = new Thread(() -> { - try (BufferedReader input = new BufferedReader(new InputStreamReader(stream))) { - String line; - while ((line = input.readLine()) != null) { - out.append(line).append("\n"); + // Fixed-size chunks rather than lines: a line is only bounded by what the program + // chose to print, and reading one materialises all of it before any limit here could + // apply. Nothing below holds more than the retained limit plus one chunk. + char[] chunk = new char[8 * 1024]; + StringBuilder pendingLine = new StringBuilder(); + boolean lineIsLongerThanWatched = false; + boolean truncated = false; + try (Reader input = new InputStreamReader(stream)) { + int read; + while ((read = input.read(chunk)) >= 0) { + if (watchedLine != null) { + for (int i = 0; i < read; i++) { + char c = chunk[i]; + if (c == '\n') { + if (!lineIsLongerThanWatched && pendingLine.length() == watchedLine.length() + && watchedLine.contentEquals(pendingLine)) { + sawWatchedLine.set(true); + } + pendingLine.setLength(0); + lineIsLongerThanWatched = false; + } else if (c != '\r' && !lineIsLongerThanWatched) { + // Only ever as long as what is being looked for; past that the + // line cannot be it, so there is no reason to keep any of it. + if (pendingLine.length() == watchedLine.length()) { + lineIsLongerThanWatched = true; + pendingLine.setLength(0); + } else { + pendingLine.append(c); + } + } + } + } + int room = RETAINED_OUTPUT_LIMIT - out.length(); + if (room > 0) { + out.append(chunk, 0, Math.min(read, room)); + } + // Said as soon as anything is dropped, including when that happens part way + // through the last chunk there is: otherwise the output ends at exactly the + // limit and reads as though that were all the program had to say. + if (read > room && !truncated) { + truncated = true; + out.append("\n... further output dropped after ") + .append(RETAINED_OUTPUT_LIMIT).append(" characters\n"); + } } } catch (IOException ignored) { }