diff --git a/BACKLOG.md b/BACKLOG.md index 1d7a5d24c..04e4e650c 100644 --- a/BACKLOG.md +++ b/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`, and `new_Box` still generic; the calls *inside* it + (`construct_Box`, `Box_init`) 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(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` 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` diff --git a/CHANGELOG.md b/CHANGELOG.md index 413b51211..ededc734f 100644 --- a/CHANGELOG.md +++ b/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: 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 6baf40263..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 @@ -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. + *

+ * 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. + *

+ * 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. + *

+ * 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 originalMethods = original.getMethods(); + List 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 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) { diff --git a/de.peeeq.wurstscript/src/main/resources/agent-docs/WURST_LANGUAGE.md b/de.peeeq.wurstscript/src/main/resources/agent-docs/WURST_LANGUAGE.md index 3b8b2f538..c094ba98f 100644 --- a/de.peeeq.wurstscript/src/main/resources/agent-docs/WURST_LANGUAGE.md +++ b/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 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 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 cd8429a4e..021b416d7 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; @@ -94,15 +96,14 @@ public void dispatchInsideClosure() { } /** - * The same closure is still rejected for Lua, and this pins that it is rejected clearly rather - * than mistranslated. Lua keeps generics erased and specialises only what it can reach through a - * concrete type; a closure is reached through its interface, so the specialised class exists but - * nothing calls it. Making that work is a change to Lua's erasure, not to substitution. Should - * it be made, this test fails and becomes the success case above. + * The same closure on Lua, which keeps generics erased and specialises only what it can reach + * from a concrete type. A closure is reached through its interface, so no call names the + * instantiation — the construction is the only thing that knows it, and that is what the + * specialisation is now driven from. */ @Test - public void dispatchInsideClosureIsRejectedForLua() { - test().testLua(true).executeProg().expectError("could not be resolved for the Lua target").lines( + public void dispatchInsideClosureLua() throws IOException { + test().testLua(true).executeProg().lines( "package test", "native testSuccess()", "interface ToIndex", @@ -119,6 +120,97 @@ public void dispatchInsideClosureIsRejectedForLua() { " 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); + } + + /** + * A constructor runs before the object exists, so the bound has to be resolved from the type + * argument the construction names rather than from anything reachable on the receiver. + */ + private static final String[] DISPATCH_IN_CONSTRUCTOR = { + "package test", + "native testSuccess()", + "interface Show", + " function show(T x) returns int", + "implements Show", + " function show(int x) returns int", + " return x * 2", + "class Box", + " int cached", + " construct(T x)", + " cached = T.show(x)", + "init", + " let b = new Box(21)", + " if b.cached == 42", + " testSuccess()", + }; + + @Test + public void dispatchInsideConstructor() { + testAssertOkLines(true, DISPATCH_IN_CONSTRUCTOR); + } + + /** + * Still rejected for Lua, and this pins that it is rejected clearly rather than mistranslated. + * A constructor belongs to the class, not to a generic function of its own, so the call that + * runs it carries no type arguments — {@code new_Box(21)} in the intermediate language, with + * the instantiation only on the type of what it is assigned to. Nothing on the Lua path reads + * it from there, so the dispatch inside the constructor is never given a concrete type. + * Should that be made to work, this test fails and becomes the success case above. + */ + @Test + public void dispatchInsideConstructorIsRejectedForLua() { + test().testLua(true).executeProg().expectError("could not be resolved for the Lua target") + .lines(DISPATCH_IN_CONSTRUCTOR); + } + + /** + * The closure has no dispatch of its own — it calls something that does. The gate deciding + * whether a construction is the only place an instantiation is stated has to follow calls to + * see that, or this reaches the backend with the bound unresolved. + */ + @Test + public void dispatchInsideClosureThroughHelperLua() { + test().testLua(true).executeProg().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 helper(Q x) returns int", + " return Q.toIndex(x)", + "function foo(Q x) returns int", + " Producer p = () -> helper(x)", + " return p.produce()", + "init", + " if foo(21) == 42", + " testSuccess()" + ); } /** @@ -141,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()" + ); + } + /** Each type argument picks its own instance, so one generic serves several types. */ @Test public void twoInstancesOfOneClass() {