Make generated Jass comparable across runs, and cut the suite from 13 to 7 minutes

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<T:>
             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.

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',
     )
 }
 

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();

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.
+     * <p>
+     * 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;
     }
 
     /**

de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/Flatten.java

@@ -52,6 +52,22 @@ public class Flatten {
     private static final ThreadLocal<Integer> andLeftVarCounter = ThreadLocal.withInitial(() -> 0);
     private static final ThreadLocal<Integer> tupleTempVarCounter = ThreadLocal.withInitial(() -> 0);
 
+    /**
+     * Starts temporary names from zero again for a new compilation.
+     * <p>
+     * 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);

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",

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<String> 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<String> 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");
     }
 
     // -------------------------------------------------------------------------