Take a closure's instantiation from its construction on Lua (#1233)

Author: Frotty

BACKLOG.md

@@ -43,28 +43,35 @@ because `LOOP.md` refers to items by number.
     mistake, and the alias it *should* produce is the class qualified with the declared name.
     Fixing it changes emitted slot names, so it wants its own commit and its own suite run.
 
-5. **Lua dispatch inside a closure.** Works on Jass since #1229. On Lua the specialised class
-   is built correctly but nothing calls it, because the closure is reached through its
-   interface and `specializeMethod` renames the method out of its dispatch slot.
-   `TypeClassTests.dispatchInsideClosureIsRejectedForLua` pins the current diagnostic and
-   should become a success test. Related to item 1; AGENTS.md flags this machinery.
-
-6. **Lua dispatch inside the constructor** of a bounded generic class. Works on Jass.
+6. **Lua dispatch inside the constructor** of a bounded generic class. Works on Jass; there is now
+   a repro for both targets, `TypeClassTests.dispatchInsideConstructor` and
+   `dispatchInsideConstructorIsRejectedForLua`, the second pinning the current diagnostic.
+
+   Not the same gap as item 5, and the fix from it does not reach: a constructor belongs to the
+   class rather than to a generic function of its own, so the call that runs it carries no type
+   arguments at all. The intermediate language has `b = new_Box(21)` with `b` typed
+   `Box<integer{show}>`, and `new_Box` still generic; the calls *inside* it
+   (`construct_Box<T>`, `Box_init<T>`) do carry the class's type variable, but nothing gives the
+   outermost one a concrete argument. `collectGenericNewUse` requires non-empty type arguments, so
+   it never starts.
+
+   The instantiation is only on the type of what the call is assigned to. Three ways to get at it,
+   roughly in order of how much they would disturb: attach the class's type arguments to
+   constructor calls when the intermediate language is built, which is where the frontend still
+   knows them and would serve both targets uniformly — but it changes the Jass path, which reaches
+   the same answer another way today, so the emitted `.j` needs checking; read them from the
+   assignment target on the Lua path, which is a syntactic shape and would miss
+   `foo(new Box<int>(21))`; or specialise from the `#alloc` inside the constructor, which is the
+   item 5 mechanism but would have to reach back out to the caller. The first looks right; confirm
+   it is what the Jass path already relies on before changing it.
 
 7. **Module bounds.** `module M<T: Show>` is rejected with a clear message today. Needs
    receiver rewriting during expansion, or type parameters on `ModuleInstanciation`.
 
-8. **`MOD_INT`/`DIV_INT` return the left operand's type** rather than `int`
-   (`AttrExprType.java`, the `case MOD_INT` branch), where `caseMathOperation` returns
-   `WurstTypeInt.instance()` for `+`, `-`, `*`. It *is* reachable: `WurstTypeIntLiteral` is a
-   proper subtype of both int and real, and `caseMathOperation` collapses two literals to `int`
-   precisely so `real r = 1 + 1` stays an error. Returning `leftType` skips that collapse, so
-   `real r = 7 div 2` and `real r = 7 mod 2` should be accepted where `+` is rejected. Confirm
-   with a test first — that is the failing repro — then return `WurstTypeInt.instance()`. Small.
-
-9. **Keep `WURST_LANGUAGE.md` and `CHANGELOG.md` current** as items land. The bounds section
-   says nothing about closures, which now work on Jass. Fold this into whichever item changes
-   the behaviour rather than doing it as a separate pass.
+9. **Keep `WURST_LANGUAGE.md` and `CHANGELOG.md` current** as items land — a standing practice
+   rather than a task to finish. Fold it into whichever item changes the behaviour rather than
+   doing it as a separate pass. Both now cover closures on either target, which is what this item
+   originally pointed at.
 
 10. **One `ImTypeVar` per type parameter.** Name-tolerant lookups remain in
    `EliminateGenerics.indexOfTypeVar`, `inheritTypeClassBinding` and
@@ -135,6 +142,26 @@ because `LOOP.md` refers to items by number.
 
 ## Done
 
+- 8. `div` and `mod` return int rather than the left operand's type, matching `caseMathOperation`.
+  Reachable, not harmless: an integer literal is a proper subtype of both int and real, and
+  addition collapses two of them to int precisely so `real r = 1 + 1` stays an error — returning
+  `leftType` skipped that, so `real r = 7 div 2` was accepted. Three tests in `ExpressionTests`:
+  both operators rejected against a real, and both still int.
+- 17. A failing Lua test says so. `translateAndTestLua` now sets the environment label instead of
+  reporting under whatever Jass configuration ran last.
+- 5 (+ the part of 9 that follows it). A type class bound now dispatches from inside a closure on
+  Lua, and `TypeClassTests.dispatchInsideClosureLua` is a success test. The note in this file was
+  wrong about the cause: no specialised class was being built at all. Lua specialisation is driven
+  by calls that carry type arguments, and a closure has none — it is reached through the interface
+  it implements, which is not generic, so only the construction knows the instantiation. Three
+  pieces were missing, all present already for Jass: collect the instantiation from `ImAlloc`,
+  collect the member access so the capture write lands on the specialised field, and bind the
+  specialised methods to the roots the originals were submethods of (registering the original
+  implementation as specialised so `settleRemainingDispatches` neutralises what it leaves behind).
+  All three are gated on the class being closure-generated. Widening them to any constructed class
+  made the two mechanisms disagree — the object came from the specialised class while its methods
+  were bound to the erased one — and broke every FastHashMap Lua test, which is the shape of
+  regression AGENTS.md §9 warns about. `WURST_LANGUAGE.md` and `CHANGELOG.md` say so now.
 - 4. The FastHashMap proof is complete. `remove` leaves a tombstone, which `slotFor` passes over
   when searching and reuses when putting; the probe is bounded by capacity rather than running
   until it finds a gap, so a table full of tombstones cannot spin. `emittedCodeCostsNothingExtra`

CHANGELOG.md

@@ -37,8 +37,9 @@
             return () -> T.toIndex(x)
 
     Substituting a type variable now carries the instance chosen for it along with the type, rather than the
-    type alone, so lifting a body into a class of its own no longer loses it. Jass only for now: Lua reaches
-    such a class through its interface and still reports the bound as unresolvable there.
+    type alone, so lifting a body into a class of its own no longer loses it. This works on both targets.
+    Lua reaches such a class through the interface it implements, so no call names the instantiation and the
+    construction is what the specialisation is taken from.
 
 - Added new pseudo-natives for debugging memory leaks:
 

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

@@ -5,6 +5,7 @@
 import de.peeeq.wurstscript.WLogger;
 import de.peeeq.wurstscript.ast.ClassDef;
 import de.peeeq.wurstscript.ast.ConstructorDef;
+import de.peeeq.wurstscript.ast.ExprClosure;
 import de.peeeq.wurstscript.ast.InterfaceDef;
 import de.peeeq.wurstscript.ast.PackageOrGlobal;
 import de.peeeq.wurstscript.ast.WPackage;
@@ -188,6 +189,18 @@ public void visit(ImMethodCall call) {
                 super.visit(call);
                 collectGenericNewUse(call);
             }
+
+            @Override
+            public void visit(ImAlloc alloc) {
+                super.visit(alloc);
+                collectGenericNewUse(alloc);
+            }
+
+            @Override
+            public void visit(ImMemberAccess memberAccess) {
+                super.visit(memberAccess);
+                collectGenericNewUse(memberAccess);
+            }
         });
     }
 
@@ -204,6 +217,18 @@ public void visit(ImMethodCall call) {
                 super.visit(call);
                 collectGenericNewUse(call);
             }
+
+            @Override
+            public void visit(ImAlloc alloc) {
+                super.visit(alloc);
+                collectGenericNewUse(alloc);
+            }
+
+            @Override
+            public void visit(ImMemberAccess memberAccess) {
+                super.visit(memberAccess);
+                collectGenericNewUse(memberAccess);
+            }
         });
     }
 
@@ -225,6 +250,106 @@ && functionNeedsSpecialization(call.getFunc(), Collections.newSetFromMap(new Ide
         }
     }
 
+    /**
+     * A construction states an instantiation that no call site has to mention. A closure is the case
+     * that needs it: its class is built from the enclosing type variables and reached through its
+     * interface, so the call carries no type arguments at all and only the allocation knows what the
+     * body dispatches on. Restricted to classes that actually dispatch on a bound, so this stays a
+     * targeted specialisation rather than general monomorphisation on Lua.
+     */
+    private void collectGenericNewUse(ImAlloc alloc) {
+        ImClassType clazz = alloc.getClazz();
+        if (clazz.getTypeArguments().isEmpty()
+            || typeArgumentsContainTypeVariable(clazz.getTypeArguments())
+            || !isConstructionOnlyInstantiation(clazz.getClassDef())) {
+            return;
+        }
+        genericsUses.add(new GenericClazzUse(alloc));
+    }
+
+    /**
+     * Whether the construction is the only place a class's instantiation is stated.
+     * <p>
+     * A class the user writes is used through calls that carry its type arguments, and those already
+     * specialise what they need onto the erased class. A closure has no such call: it is reached
+     * through the interface it implements, which is not generic, so the allocation is the only thing
+     * that knows what the body dispatches on. Widening this beyond that case makes the two
+     * mechanisms disagree — the object comes from the specialised class while its methods were bound
+     * to the erased one.
+     */
+    private boolean isConstructionOnlyInstantiation(ImClass classDef) {
+        return classDef.attrTrace() instanceof ExprClosure closure
+            && !isInsideAnotherClosure(closure)
+            && classReachesDispatch(classDef);
+    }
+
+    /**
+     * A closure written inside another one is left alone.
+     * <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;
+    }
+
+    /**
+     * Whether anything the class does ends in a dispatch on a bound, including through the
+     * functions it calls. `classNeedsSpecialization` asks only whether a dispatch sits in the class
+     * itself, which is the wrong question here: a closure whose body is `() -> helper(x)` has no
+     * dispatch of its own, and the instantiation it needs is still only known at its construction.
+     * That question is kept as it is, because widening it would change what gets specialised on
+     * paths that have nothing to do with closures.
+     */
+    private boolean classReachesDispatch(ImClass classDef) {
+        for (ImFunction f : classDef.getFunctions()) {
+            if (functionNeedsSpecialization(f, Collections.newSetFromMap(new IdentityHashMap<>()),
+                Collections.newSetFromMap(new IdentityHashMap<>()))) {
+                return true;
+            }
+        }
+        for (ImMethod m : classDef.getMethods()) {
+            if (m.getImplementation() != null
+                && functionNeedsSpecialization(m.getImplementation(),
+                    Collections.newSetFromMap(new IdentityHashMap<>()),
+                    Collections.newSetFromMap(new IdentityHashMap<>()))) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    /**
+     * A field of a class specialised from a construction has to be reached on the copy. The write
+     * that captures a closure's environment is the case that needs it: it names the field of the
+     * generic class, which nothing allocates any more once the construction was redirected.
+     */
+    private void collectGenericNewUse(ImMemberAccess memberAccess) {
+        ImVar field = memberAccess.getVar();
+        if (field.getParent() == null || !(field.getParent().getParent() instanceof ImClass owningClass)) {
+            return;
+        }
+        // A class that has already been specialised has nothing left to select, and asking the
+        // receiver to adapt to it fails outright: the receiver is still typed by the generic class
+        // the specialised one was copied from, which is not a superclass of it.
+        if (owningClass.getTypeVariables().isEmpty() || !isConstructionOnlyInstantiation(owningClass)) {
+            return;
+        }
+        if (memberAccess.getTypeArguments().isEmpty()) {
+            // The access names a field, not an instantiation; the receiver is what knows which one.
+            addMemberTypeArguments(memberAccess, owningClass);
+        }
+        if (memberAccess.getTypeArguments().isEmpty()
+            || typeArgumentsContainTypeVariable(memberAccess.getTypeArguments())) {
+            return;
+        }
+        genericsUses.add(new GenericMemberAccess(memberAccess));
+    }
+
     private void collectGenericNewUse(ImMethodCall call) {
         if (specializedCallSites.contains(call)) {
             return;
@@ -1316,12 +1441,56 @@ private ImClass specializeClass(ImClass c, GenericTypes generics) {
         // NEW: Create specialized global variables for this class instantiation
         createSpecializedGlobals(c, generics, typeVars);
 
+        if (genericNewOnly && isConstructionOnlyInstantiation(c)) {
+            attachSpecializedClassMethods(c, newC, generics);
+        }
 
         onSpecializedClassTriggers.get(c).forEach(consumer ->
             consumer.accept(generics, newC));
         return newC;
     }
 
+    /**
+     * Makes the methods of a class specialised from a construction reachable.
+     * <p>
+     * A class specialised because a call named its instantiation is reached through that call.
+     * One specialised because it was constructed is not: the receiver is held as its interface, so
+     * dispatch goes through the root method, whose submethods still list only the generic original.
+     * Each copy is bound to the same roots, and the original's implementation is recorded as having
+     * a specialisation so the dispatch left behind in it settles instead of reaching the backend.
+     */
+    private void attachSpecializedClassMethods(ImClass original, ImClass specialized, GenericTypes generics) {
+        List<ImMethod> originalMethods = original.getMethods();
+        List<ImMethod> specializedMethods = specialized.getMethods();
+        if (originalMethods.size() != specializedMethods.size()) {
+            // The copy is structural, so this cannot happen; bail rather than pair the wrong ones.
+            return;
+        }
+        Map<ImMethod, ImMethod> specializationOf = new IdentityHashMap<>();
+        for (int i = 0; i < originalMethods.size(); i++) {
+            ImMethod copy = specializedMethods.get(i);
+            copy.setMethodClass(JassIm.ImClassType(specialized, JassIm.ImTypeArguments()));
+            specializationOf.put(originalMethods.get(i), copy);
+
+            ImFunction implementation = originalMethods.get(i).getImplementation();
+            ImFunction copyImplementation = copy.getImplementation();
+            if (implementation != null && copyImplementation != null && implementation != copyImplementation
+                && specializedFunctions.get(implementation, generics) == null) {
+                specializedFunctions.put(implementation, generics, copyImplementation);
+            }
+        }
+        for (ImClass c : new ArrayList<>(prog.getClasses())) {
+            for (ImMethod root : c.getMethods()) {
+                for (ImMethod sub : new ArrayList<>(root.getSubMethods())) {
+                    ImMethod copy = specializationOf.get(sub);
+                    if (copy != null && !root.getSubMethods().contains(copy)) {
+                        root.getSubMethods().add(copy);
+                    }
+                }
+            }
+        }
+    }
+
     private ImExpr rewriteGenericGlobalsInExpr(ImExpr e, ImClass owningClass, GenericTypes generics) {
         e.accept(new Element.DefaultVisitor() {
             @Override public void visit(ImVarAccess va) {

de.peeeq.wurstscript/src/main/resources/agent-docs/WURST_LANGUAGE.md

@@ -186,6 +186,14 @@ Instances are unique and must be declared next to what they relate. An instance
 
 An instance must implement each requirement with the signature it has after the interface's type parameter is replaced by the instance type; a matching name is not enough. An interface used as a bound must not extend another interface, because the requirements of a bound are the interface's own functions.
 
+A requirement can also be dispatched from inside a closure written in a bounded generic. The closure captures the type parameter along with the values it uses, so the instance is still chosen by the caller's type argument:
+
+```wurst
+function foo<Q: ToIndex>(Q x) returns int
+	Producer p = () -> Q.toIndex(x)
+	return p.produce()
+```
+
 A generic which passes its own type parameter to another bounded generic must declare that bound itself:
 
 ```wurst