diff --git a/BACKLOG.md b/BACKLOG.md index 420406814..6ece56210 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -72,13 +72,8 @@ because `LOOP.md` refers to items by number. source parameter. Making the node canonical lets all three compare by identity and removes a class of silent wrong dispatch. Mechanical, well covered by the suite. -11. **Jass temp counter is not reset between compilations.** Two runs of the same commit emit - different `.j` (`temp151` vs `temp8`) because the counter is JVM-wide and depends on how - many tests ran before. Not wrong for compiling one map, but it means `.j` cannot be diffed - across runs to validate a change — only `.lua` can. Fixing it would make Jass diffable. - -13. **A bounded generic class cannot be subclassed.** Found while building a repro for item 3; - both backends break, differently, on the same program: +13. **A bounded generic class cannot be subclassed.** Diagnosed on the Jass side and pinned by + `TypeClassTests.subclassOfBoundedGenericIsRejected`; the Lua half is still open. The program: interface Show function show(T x) returns int @@ -104,12 +99,27 @@ because `LOOP.md` refers to items by number. if b.size(1) == 6 and b.shift(1) == 1001 and s.size(1) == 106 testSuccess() - Jass fails to compile: `Typevar dispatch not eliminated.` Lua compiles and runs but never - reaches `testSuccess`: the override makes `size` dispatched, and the emitted call is - `b:Box_size_specialized_integer(1)` while `b` was allocated from the *erased* `Box` table, - which binds only `shift`. The specialised table `Box_specialized_integer` has the slot; the - instance never gets that table. Split this if the two turn out to have separate causes — - the Jass one is loud and probably the smaller of the two. + On Jass the override's `super.size(extra)` becomes a direct call to the superclass + implementation, and once `Box` is specialised that call points at a function which has been + replaced by `Box_size⟪integer⟫`. The `.jim` for the test shows both side by side: the + specialised copy with its dispatch resolved, and `SubBox_size` still calling the original. + + Tried extending `addMemberTypeArguments` to attach the receiver's type arguments to such calls, + the way it already does for method calls and member accesses, and it changes nothing. The + callee has no type variables of its own — they belong to the class — so there is nothing for + type arguments on the call to select, and specialisation of a class function happens by + `specializeClass` copying the whole class instead. The call has to be **redirected** to that + copy, not annotated. The natural place is wherever a class is specialised: every call from a + subclass into a superclass function needs to follow. + + Item 6 is the same family but not the same fix: there the constructor call also carries no type + arguments, and there the instantiation is not on any argument either, only on the type of what + the call is assigned to. + + Lua compiles and runs but never reaches `testSuccess`: the override makes `size` dispatched, + and the emitted call is `b:Box_size_specialized_integer(1)` while `b` was allocated from the + *erased* `Box` table, which binds only `shift`. The specialised table has the slot; the instance + never gets that table. That half is the erasure question again, as in item 5. 12. **Standing item, never finished.** When nothing above is left, find the next thing worth doing and add it here rather than stopping. Good sources, in order: a test that would have @@ -153,6 +163,23 @@ because `LOOP.md` refers to items by number. ## Done +- 19. Tried giving Jass the dangling-reference check Lua has, and reverted it. The two backends do + not agree on which functions exist: `LuaTranslator` requires every reference to be rooted in the + program, while `ImToJassTranslator` is handed `getCalledFunctions()` and emits whatever is + called, rooted or not. Three passing tests rely on that — a closure's `construct_Lazy` is + detached from the program and still called — so the invariant is Lua's rather than universal. + Worth knowing when reading a Jass error: a function a pass detached is still translated, so the + error names what was inside it rather than the reference that kept it alive, which is exactly how + item 13 shows up. (Requiring natives to be rooted also fails: `$debugPrint` is built with + `IS_NATIVE, IS_BJ` and deliberately never added.) +- 11. Generated Jass can be compared across runs. The counters naming temporaries were per thread + and never reset, so a name depended on how much had been compiled before it: the same source gave + `temp0` alone and `temp70` after other tests, measured directly rather than assumed. They now + start from zero at the top of each compilation — not at each flattening, which happens again + after every optimisation and would name two locals of one function alike. + `DeterministicChecks.temporaryNamesDoNotDependOnEarlierCompilations` compiles the same source, + then other programs, then the same source again; the compilation in between is the part that + matters, and it is the inlining configuration that emits a temporary for that source. - 18. Comparing an unresolved type parameter default is now an error rather than a quiet "not equal". Item 16 closed the path that reached a program, but the stand-in is produced by a static attribute and could surface anywhere, so the silence was the part worth removing. The whole suite @@ -219,9 +246,27 @@ because `LOOP.md` refers to items by number. ## Notes +- Fork count is worth measuring rather than reasoning about, because two effects pull against each + other: more forks means more parallelism but also more contention, and every test gets slower. + Measured on eight cores, whole suite, wall clock against total reported test time: + serial 13m11s / 786s; four forks 8m39s / 1230s; eight forks 7m03s / 2048s. Eight wins even + though each test runs 2.6 times slower there than alone. Sixteen was not tried; the limit by + then is the slowest single class, not the scheduling. +- Where the suite's 13 minutes went, measured from `build/test-results/test/TEST-*.xml`: 786s of + test time across 79 classes and 1663 tests, so effectively all of it is the tests themselves + rather than the build. The top ten classes are 61% of it, led by `ExportToWurstTest` at 108s, + then `OptimizerTests` 67s, `BugTests` 66s, `RealWorldExamples` 54s, + `GenericsWithTypeclassesTests` 45s. Gradle hands whole classes to forks, so wall time cannot go + below the slowest class — 108s is the floor until `ExportToWurstTest` is split. + Below that floor the remaining costs, in the order worth attacking: 401 tests compile their + program five times (each Jass configuration) and spawn `pjass.exe` per configuration, which + re-parses `common.j` and `blizzard.j` every time; and 102 `withStdLib` sites re-parse 169 stdlib + files because `GlobalCaches.clearAll()` runs before and after every method. Both change what the + tests actually verify, so neither is a free win the way the scheduling was. + - `%` is real modulo in Wurst; `mod` is integer modulo. `int % 8` types as `real`. -- Emitted Lua must be byte-identical for identical input (AGENTS.md §8). It is the only - emitted output that can be diffed across runs — see item 11. +- Emitted Lua must be byte-identical for identical input (AGENTS.md §8). Emitted Jass can be + diffed across runs too now, since item 11 — `LOOP.md` still says otherwise. - Two of this run's reverts were the same mistake: a name that looks redundant is usually carrying a distinction. The mangled method name separates overloads; `leftType` on `div` keeps a literal assignable to a real. Check what a name distinguishes before replacing it with a tidier one. diff --git a/de.peeeq.wurstscript/build.gradle b/de.peeeq.wurstscript/build.gradle index f4fa668bb..9fd42ad1e 100644 --- a/de.peeeq.wurstscript/build.gradle +++ b/de.peeeq.wurstscript/build.gradle @@ -235,15 +235,85 @@ tasks.named('compileJava') { it.dependsOn('gen') } /** -------- Tests -------- */ +/** + * How many test workers to run at once. + * + * Cores decide how much parallelism is useful; memory decides how much is survivable. Each worker + * is a JVM with the heap set below, and the Gradle daemon holds its own on top, so a machine with + * many cores and little memory has to be counted the other way. Measured on eight cores: serial + * 13m11s, four forks 8m39s, eight forks 7m03s — more forks kept winning even though each test runs + * slower under the contention, so this errs towards cores where memory allows. + */ +int testForkCount() { + def override = project.findProperty('testForks') + if (override) { + return Math.max(1, override.toString().toInteger()) + } + int byCores = Math.max(1, (int) (Runtime.runtime.availableProcessors() / 2)) + int workerHeapGb = 2 // keep in step with -Xmx below + int reservedGb = 4 // the daemon's own heap, plus room for the OS and the lua/pjass runs + try { + def os = java.lang.management.ManagementFactory.operatingSystemMXBean + long totalBytes = os."getTotalMemorySize"() + int byMemory = (int) ((totalBytes / (1024L * 1024L * 1024L) - reservedGb) / workerHeapGb) + return Math.max(1, Math.min(byCores, byMemory)) + } catch (Throwable ignored) { + // No reliable reading of physical memory; cores alone, conservatively. + return Math.max(1, Math.min(byCores, 4)) + } +} + + +/** + * Fetches the standard library the tests compile against, once, before they fork. + * + * StdLib guards the fetch with a lock held in one process, which is no guard at all across + * workers: on a clean checkout each of them would clone into the same directory at the same time. + * The pinned repository and commit stay defined in that one place rather than being repeated here. + */ +tasks.register('ensureStdLib', JavaExec) { + description "Fetches the pinned standard library used by the tests" + classpath = sourceSets.test.runtimeClasspath + mainClass.set('tests.wurstscript.tests.StdLib') + workingDir = projectDir + // An escape hatch for working without the network: a focused run of tests that never touch + // the library should not be stopped by not being able to reach GitHub. Read now rather than + // when the task runs, which the configuration cache does not allow. + def skipFetch = project.hasProperty('skipStdLibFetch') + onlyIf { !skipFetch } + // Deliberately not declaring the checkout as an output: that would skip this whenever the + // directory merely exists, which is precisely when it may be at the wrong commit or half + // cloned. The fetch checks the pinned commit and repairs it, and costs nothing when correct. + outputs.upToDateWhen { false } +} + test { + dependsOn 'ensureStdLib' useTestNG() + // The suite is a few thousand independent compilations and was running one at a time, so it + // took as long as the sum of them. Forks rather than threads: the harness keeps state in + // statics (the current test environment, the global caches, the extracted lua binaries), and a + // fork gets its own copy of all of it. Gradle hands out whole classes, so a class that writes + // fixed file names stays inside one fork. + // Wall time cannot fall below the slowest single class, which is why this does not need to be + // every core to get most of the win. + // + // Bounded by memory as well as cores, because the two are not proportional everywhere: a CI + // runner with sixteen cores and eight gigabytes would otherwise start eight two-gigabyte + // workers next to the daemon's own three and be killed for it. Override with -PtestForks=N. + maxParallelForks = testForkCount() + jvmArgs( '-Xmx2g', // local: give it room to finish and dump '-XX:MaxMetaspaceSize=256m', '-XX:+HeapDumpOnOutOfMemoryError', '-XX:+UnlockExperimentalVMOptions', // needed for UseCompactObjectHeaders until it graduates '-XX:+UseCompactObjectHeaders', // Java 24+: 8-byte headers (vs 16) — big win for AST-heavy workloads + // Each fork otherwise sizes its garbage collector and compiler threads for the whole + // machine, so running several at once oversubscribes it badly: eight forks made every + // test about three times slower and gave back only a third of the parallelism. + '-XX:ActiveProcessorCount=2', ) } diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/WurstCompilerJassImpl.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/WurstCompilerJassImpl.java index f15fc9d12..9d4708c1b 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/WurstCompilerJassImpl.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/WurstCompilerJassImpl.java @@ -634,6 +634,9 @@ public ImTranslator getImTranslator() { public @Nullable ImProg translateProgToIm(WurstModel root) { beginPhase(1, "to intermediate lang"); + // Names of generated temporaries are counted from here, so that the same source compiles + // to the same script whatever was compiled before it in this process. + Flatten.resetTempVarCounters(); // translate wurst to intermediate lang: imTranslator = new ImTranslator(root, errorHandler.isUnitTestMode(), runArgs); imProg = getImTranslator().translateProg(); diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/EliminateGenerics.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/EliminateGenerics.java index 43d4dca0b..9d423291b 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/EliminateGenerics.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/EliminateGenerics.java @@ -278,7 +278,23 @@ private void collectGenericNewUse(ImAlloc alloc) { * to the erased one. */ private boolean isConstructionOnlyInstantiation(ImClass classDef) { - return classDef.attrTrace() instanceof ExprClosure && classReachesDispatch(classDef); + return classDef.attrTrace() instanceof ExprClosure closure + && !isInsideAnotherClosure(closure) + && classReachesDispatch(classDef); + } + + /** + * A closure written inside another one is left alone. + *

+ * Its captured environment is reached through a receiver belonging to the enclosing closure, + * which by then has been specialised itself, and specialising the owner again with what is + * left over fails inside the rewrite. Supporting that is a further step; until it is taken, + * saying the bound could not be resolved - which is what happens without any of this - is + * better than an error about generics of the wrong size. + */ + private static boolean isInsideAnotherClosure(ExprClosure closure) { + de.peeeq.wurstscript.ast.Element parent = closure.getParent(); + return parent != null && parent.attrNearestExprClosure() != null; } /** diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/Flatten.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/Flatten.java index f2becc2ef..34c69dac8 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/Flatten.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/Flatten.java @@ -52,6 +52,22 @@ public class Flatten { private static final ThreadLocal andLeftVarCounter = ThreadLocal.withInitial(() -> 0); private static final ThreadLocal tupleTempVarCounter = ThreadLocal.withInitial(() -> 0); + /** + * Starts temporary names from zero again for a new compilation. + *

+ * The counters are per thread and were never reset, so the name a temporary got depended on how + * many programs had been compiled before it on that thread — the same source emitted {@code + * temp0} alone and {@code temp70} after other work, which is why generated Jass could not be + * compared across runs. They still run on through one compilation, because flattening happens + * again after each optimisation and restarting mid-compilation would name two locals of one + * function alike. + */ + public static void resetTempVarCounters() { + tempVarCounter.set(0); + andLeftVarCounter.set(0); + tupleTempVarCounter.set(0); + } + private static String getTempVarName() { int count = tempVarCounter.get(); tempVarCounter.set(count + 1); diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/DeterministicChecks.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/DeterministicChecks.java index 786dbb818..93df7ee9f 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/DeterministicChecks.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/DeterministicChecks.java @@ -40,6 +40,48 @@ private void run(Runnable example, String name) throws IOException { assertEquals(script1, script2); } + /** + * The same source has to emit the same script whatever was compiled before it. Names of + * generated temporaries used to be counted per thread and never reset, so a program compiled + * alone got {@code temp0} and the same program compiled after other work got {@code temp70} — + * which is what made generated Jass impossible to compare across runs. Compiling something + * else in between is the part that matters here; two runs on their own would agree either way. + * The inlining configuration is the one that emits a temporary for this source. + */ + @Test + public void temporaryNamesDoNotDependOnEarlierCompilations() throws IOException { + ErrorHandler.outputTestSource = true; + try { + usesTemporaries(); + String first = Files.toString( + new File("test-output/DeterministicChecks_usesTemporaries_inl.j"), Charsets.UTF_8); + + exampleCode(); + cycleExample(); + + usesTemporaries(); + String afterOtherWork = Files.toString( + new File("test-output/DeterministicChecks_usesTemporaries_inl.j"), Charsets.UTF_8); + + assertEquals(first, afterOtherWork); + } finally { + ErrorHandler.outputTestSource = false; + } + } + + /** Nested calls in one expression are what makes the flattener allocate temporaries. */ + private void usesTemporaries() { + testAssertOkLines(false, + "package test", + "native testSuccess()", + "function f(int x) returns int", + " return x + 1", + "init", + " if f(f(1)) + f(f(2)) == 8", + " testSuccess()" + ); + } + private void exampleCode() { testAssertOkLines(false, "package test", diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ExportToWurstTest.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ExportToWurstTest.java index 5eebb70b4..ea290db97 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ExportToWurstTest.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ExportToWurstTest.java @@ -12,6 +12,7 @@ import org.testng.annotations.Test; import java.io.IOException; +import java.util.ArrayList; import java.util.List; import static org.testng.Assert.*; @@ -132,6 +133,7 @@ public void abilityAliasBaseIdsUseAliasSpecificWrapperClasses() throws IOExcepti "Eah1", ObjMod.ValType.UNREAL, 1, 1, 0.1, "..setDamageDealttoAttackers(1, 0.1)"} }; + List exports = new ArrayList<>(); for (Object[] c : cases) { W3A w3a = new W3A(); String newId = (String) c[0]; @@ -147,8 +149,12 @@ public void abilityAliasBaseIdsUseAliasSpecificWrapperClasses() throws IOExcepti assertTrue(out.contains((String) c[9]), baseId + " export:\n" + out); assertFalse(out.contains("new " + implementationCodeClass + "("), baseId + " must not export via implementation-code class:\n" + out); assertFalse(out.contains("createObjectDefinition"), baseId + " must not fall back to raw export:\n" + out); - assertExportCompiles(out, "import AbilityObjEditing"); + exports.add(out); } + // All five at once: each is a compile against the whole standard library, and the cases + // declare different objects, so compiling them together checks the same code for a fifth + // of the time. + assertExportCompiles(String.join("\n", exports), "import AbilityObjEditing"); } @Test @@ -679,6 +685,7 @@ public void abilityIntegerLevelFieldsWithWrapperMethodsCompile() throws IOExcept {"AHca", "Hca4", 4, "AbilityDefinitionRangerColdArrows", "setStackFlags", "Zh04"}, }; + List exports = new ArrayList<>(); for (Object[] c : cases) { W3A w3a = new W3A(); W3A.Obj obj = w3a.addObj(ObjId.valueOf((String) c[5]), ObjId.valueOf((String) c[0])); @@ -688,8 +695,10 @@ public void abilityIntegerLevelFieldsWithWrapperMethodsCompile() throws IOExcept assertTrue(out.contains("new " + c[3] + "('" + c[5] + "')"), out); assertTrue(out.contains(".." + c[4] + "(1, 0)"), out); - assertExportCompiles(out, "import AbilityObjEditing"); + exports.add(out); } + // One compile for all five, as above. + assertExportCompiles(String.join("\n", exports), "import AbilityObjEditing"); } // ------------------------------------------------------------------------- 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 index a719745b5..11655e5ad 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaRunnerTests.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaRunnerTests.java @@ -60,4 +60,26 @@ public void aProgramThatFloodsStdoutStillFinishes() { " 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/StdLib.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java index 6a4dc9b1d..c418fe53a 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java @@ -2,6 +2,7 @@ import de.peeeq.wurstscript.WLogger; import org.eclipse.jgit.api.Git; +import org.eclipse.jgit.api.ResetCommand; import org.eclipse.jgit.api.errors.GitAPIException; import org.eclipse.jgit.lib.Constants; import org.testng.annotations.Test; @@ -38,12 +39,66 @@ public void download() { assert(downloadStandardlib()); } + /** + * Entry point for the build, which fetches the library once before the tests fork. + *

+ * The guard below is a lock and a flag in one process, and every fork is a process of its own, + * so several of them starting on a clean checkout would clone into the same directory at the + * same time. Doing it here, from the build's own JVM, means they all find it already there. + */ + public static void main(String[] args) { + // Deep clean here and only here: this runs alone, before the workers, so it is the one + // moment when rewriting the directory disturbs nobody. + if (!ensureCheckout(true)) { + throw new RuntimeException("Could not fetch the standard library the tests compile against."); + } + } + + private static boolean isUsableCheckout() { + try (Git git = Git.open(stdLibFolder)) { + return git.getRepository().resolve(Constants.HEAD) != null; + } catch (IOException | RuntimeException e) { + return false; + } + } + + private static void deleteRecursively(File file) { + File[] children = file.listFiles(); + if (children != null) { + for (File child : children) { + deleteRecursively(child); + } + } + if (!file.delete()) { + file.deleteOnExit(); + } + } + public synchronized static boolean downloadStandardlib() { + return ensureCheckout(false); + } + + /** + * @param deepClean also discard ignored build artefacts. Only the build's prefetch asks for + * this. A worker must not: the artefacts are what a previous run left inside the library + * folder, and removing them while other workers are reading it is the contention this + * avoids. Leaving them is not harmless either - the compiler scans the library folder + * recursively and a stale `_build` can hold `.wurst` files that take a pinned package's + * name - which is why the prefetch does it once for everyone. + */ + private synchronized static boolean ensureCheckout(boolean deepClean) { if (isInitialized) { return true; } try { + // A checkout that cannot be opened or has no HEAD is worse than none: every test now + // waits on this, so a half-written directory left by an interrupted run would stop the + // whole suite rather than the few cases that need the library. + if (stdLibFolder.exists() && !isUsableCheckout()) { + System.out.println("Discarding an unusable standard library checkout at " + stdLibFolder); + deleteRecursively(stdLibFolder); + } if (!stdLibFolder.exists()) { tempFolder.mkdirs(); try (Git git = Git @@ -55,24 +110,46 @@ public synchronized static boolean downloadStandardlib() { } } + boolean repaired = false; try (Git git = Git.open(stdLibFolder)) { String head = git.getRepository().resolve(Constants.HEAD).getName(); if (!head.equals(version)) { - System.out.println("Wrong version '" + head + "', executing git pull to get '" + version + "'"); - - git.checkout().setName(Constants.MASTER).call(); - git.pull().call(); + repaired = true; + System.out.println("Wrong version '" + head + "', fetching to get '" + version + "'"); + + // Straight to the pinned commit rather than by way of master. A checkout left + // detached - which is what this leaves behind, so it is the normal state - has + // no local master to check out, and asking for one fails with "Ref master + // cannot be resolved" before the fetch that would have created it. + git.fetch() + .setRemote(Constants.DEFAULT_REMOTE_NAME) + .setRefSpecs("+refs/heads/*:refs/remotes/origin/*") + .call(); git.checkout().setName(version).setForceRefUpdate(true).call(); } } - // reset all possible changes - Git.open(stdLibFolder).clean().setForce(true).setCleanDirectories(true).setIgnore(false).call(); - Git.open(stdLibFolder).checkout().setName(version).call(); + // Undo whatever a previous run left behind, but only when there is something to undo. + // Every worker is its own process and so runs this once; unconditionally cleaning and + // checking out means several of them rewriting one directory at the same time, each + // taking the index lock, while other workers are reading the same files. Reading the + // status does not write anything, and the ordinary case has nothing to repair. + try (Git git = Git.open(stdLibFolder)) { + if (repaired || deepClean || !git.status().call().isClean()) { + // Reset rather than checkout: checking out the commit HEAD already points at + // does nothing, so a modified file survived what claimed to undo it. + git.reset().setMode(ResetCommand.ResetType.HARD).setRef(version).call(); + git.clean().setForce(true).setCleanDirectories(true).setIgnore(false).call(); + } + } isInitialized = true; } catch (IOException | GitAPIException e) { - WLogger.severe(e.getStackTrace().toString()); + // The array's identity is no use to anyone; there is an overload that prints the trace. + WLogger.severe(e); + // And on the console too: the build waits on this now, so whoever is looking at a + // failed run needs to see why here rather than in a log file. + e.printStackTrace(System.err); return false; } 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 85a941e4f..2aa4f6f0e 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java @@ -6,6 +6,8 @@ import java.io.File; import java.io.IOException; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertTrue; @@ -100,7 +102,7 @@ public void dispatchInsideClosure() { * specialisation is now driven from. */ @Test - public void dispatchInsideClosureLua() { + public void dispatchInsideClosureLua() throws IOException { test().testLua(true).executeProg().lines( "package test", "native testSuccess()", @@ -118,6 +120,28 @@ public void dispatchInsideClosureLua() { " if foo(21) == 42", " testSuccess()" ); + + // Running is not enough on its own: the same answer comes out whether the closure was + // specialised or the erased class happened to carry a working implementation. These say + // which of the two happened. + String compiled = Files.toString( + new File("test-output/lua/TypeClassTests_dispatchInsideClosureLua.lua"), Charsets.UTF_8); + + Matcher allocation = Pattern.compile("(\\w+_specialized\\w*):create\\d*\\(").matcher(compiled); + assertTrue(allocation.find(), + "the closure should be allocated from its specialised class:\n" + compiled); + String specialised = allocation.group(1); + + Matcher call = Pattern.compile("\\w+:(\\w*produce\\w*)\\(").matcher(compiled); + assertTrue(call.find(), "expected a dispatched produce slot:\n" + compiled); + String slot = call.group(1); + + assertTrue(Pattern.compile(Pattern.quote(specialised) + "\\." + Pattern.quote(slot) + + "\\s*=\\s*" + Pattern.quote(specialised) + "\\w*").matcher(compiled).find(), + "the specialised class should bind " + slot + " to its own implementation:\n" + compiled); + assertFalse(Pattern.compile(Pattern.quote(specialised) + "\\." + Pattern.quote(slot) + + "\\s*=\\s*Producer_test_produce\\b").matcher(compiled).find(), + "the specialised class must not bind " + slot + " to the generic original:\n" + compiled); } /** @@ -209,6 +233,37 @@ public void closureImplementingALuaKeywordName() { ); } + /** + * A closure written inside another one is still rejected, and this pins that it is rejected in + * the same words as before rather than falling over inside the rewrite. The inner closure + * reaches its captured environment through a receiver belonging to the outer one, which has + * been specialised by then, so specialising the owner again with what is left over does not + * work. Should that be made to work, this test fails and becomes a success case. + */ + @Test + public void nestedClosuresInsideBoundedGenericAreRejectedForLua() { + test().testLua(true).executeProg().expectError("could not be resolved for the Lua target").lines( + "package test", + "native testSuccess()", + "interface ToIndex", + " function toIndex(T x) returns int", + "implements ToIndex", + " function toIndex(int x) returns int", + " return x * 2", + "interface Producer", + " function produce() returns int", + "function foo(Q x) returns int", + " Producer outer = () -> begin", + " Producer inner = () -> Q.toIndex(x)", + " return inner.produce()", + " end", + " return outer.produce()", + "init", + " if foo(21) == 42", + " testSuccess()" + ); + } + /** * A slot of a {@code T array} that was never written reads as the default of whatever T stands * for. The default is computed by a static attribute, which cannot see what T is bound to, so @@ -249,6 +304,49 @@ public void unwrittenArrayOfATypeParameterReadsAsItsDefaultReversed() { ); } + private static final String[] SUBCLASS_OF_BOUNDED_GENERIC = { + "package test", + "native testSuccess()", + "interface Show", + " function show(T x) returns int", + "implements Show", + " function show(int x) returns int", + " return x", + "class Box", + " K key", + " construct(K k)", + " key = k", + " function size(int extra) returns int", + " return K.show(key) + extra", + "class SubBox extends Box", + " construct(int k)", + " super(k)", + " override function size(int extra) returns int", + " return super.size(extra) + 100", + "init", + " Box b = new Box(5)", + " Box s = new SubBox(5)", + " if b.size(1) == 6 and s.size(1) == 106", + " testSuccess()", + }; + + /** + * Subclassing a bounded generic class does not work yet, and this pins where it stops. The + * override's {@code super.size(extra)} becomes a direct call to the superclass implementation, + * and that call carries no type arguments — the type variables belong to the class, not to the + * method — so nothing specialises it. Specialising the class replaces the original with + * {@code Box_size⟪integer⟫}, and the super call is left pointing at what was removed. + *

+ * The same shape as the constructor case above: class type arguments reach method calls and + * member accesses, but not constructor calls or super calls. Should either be fixed, look at + * both. The message names what was inside the function rather than the reference that kept it + * alive, because Jass emits whatever is called rather than only what the program still holds. + */ + @Test + public void subclassOfBoundedGenericIsRejected() { + testAssertErrorsLines(false, "Typevar dispatch not eliminated", SUBCLASS_OF_BOUNDED_GENERIC); + } + /** Each type argument picks its own instance, so one generic serves several types. */ @Test public void twoInstancesOfOneClass() { 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 63aec1f19..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 @@ -508,8 +508,6 @@ private void testWithoutInliningAndOptimization(String name, boolean executeProg } private void translateAndTestLua(String name, boolean executeProg, WurstGui gui, WurstModel model, WurstCompilerJassImpl compiler) { - // Otherwise a Lua failure is reported under whatever Jass configuration ran last. - setCurrentTestEnv("Lua"); try { name = name.replaceAll("[^a-zA-Z0-9_]", "_"); @@ -650,18 +648,48 @@ private Thread collectStreamAsync(InputStream stream, StringBuilder out) { 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) { - if (watchedLine != null && watchedLine.equals(line)) { - sawWatchedLine.set(true); + // 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)); } - if (out.length() < RETAINED_OUTPUT_LIMIT) { - out.append(line).append("\n"); - } else if (!truncated) { + // 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("... further output dropped after ") + out.append("\n... further output dropped after ") .append(RETAINED_OUTPUT_LIMIT).append(" characters\n"); } }