Resolve a type parameter's default where the binding is known (#1234)

Author: Frotty

BACKLOG.md

@@ -13,35 +13,23 @@ Notes rather than leaving it in a commit message.
 Numbering is stable: finished items leave a gap rather than shifting the ones below,
 because `LOOP.md` refers to items by number.
 
-16. **A never-written array of a type parameter reads as nothing, silently.** In the interpreter
-    only — Jass and Lua both give the type argument's default. `DefaultValue.get(ImTypeVarRef)`
-    returns `ILconstUnsafeDefault`, whose `isEqualTo` matches only another `ILconstUnsafeDefault`,
-    so a comparison against the real default is quietly false rather than an error. Repro:
-
-        class Box<T:>
-            private static T array none
-            static function first() returns T
-                return none[0]
-        init
-            if Box<int>.first() == 0
-                testSuccess()
-
-    Passes on every Jass configuration and fails on the pre-transform interpreter run. The plain
-    `int array` version passes, so this is specific to the type parameter. The interpreter knows
-    the current type argument (`ProgramState.resolveType`), but `DefaultValue` is a static
-    attribute with no access to it, and the array's default supplier is bound when the array is
-    allocated rather than when it is read. Either resolve at read time where the state is in hand,
-    or make the placeholder throw when used — what it must not do is compare unequal in silence.
-    Found by `FastHashMapTests`: the tombstone fixture needs a "no value" for `V`.
-
-15. **One junk dispatch slot per specialised class.** Left over from item 3, same heuristic in
-    the other place it is used. `addDirectAliases` composes `owner.getName() + "_" +
+15. **One junk dispatch slot per specialised class.** `addDirectAliases` and
+    `LuaTranslator.collectDispatchSlotNames` both compose `owner.getName() + "_" +
     semanticNameFromMethodName(name)`, and for a specialised method that trailing segment is the
-    type argument, so every method of `FastHashMap<int, int>` claims the same
-    `FastHashMap_specialized_integer__integer_integer` slot and the alphabetically first wins.
-    Nothing calls it, so it is dead weight rather than a wrong result — but it is the same
-    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.
+    type argument — so every method of `FastHashMap<int, int>` claims one shared
+    `FastHashMap_specialized_integer__integer_integer` slot and the alphabetically first wins it.
+    Nothing calls it, so it is dead weight rather than a wrong result.
+
+    Tried using the declared name instead and reverted it: overloads share a declared name, so
+    `setup(int)` and `setup(string)` collapse into one slot, which is what
+    `LuaTranslationTests.overloadedMethodsDoNotAliasInLuaDispatchTables` and
+    `moduleProvidedOverloadedOverrideDoesNotCollapseLuaSlots` exist to prevent. Both sources of a
+    semantic name are wrong, in opposite directions: the mangled trailing segment collides across
+    the siblings of one specialisation, the declared name collides across overloads. A fix needs a
+    name that separates both — the declared name together with the dispatch signature key would,
+    since that is already what distinguishes overloads elsewhere in the same file. Worth doing only
+    if this stops being dead weight, because the cost of getting it wrong is a real mis-binding
+    while the cost of leaving it is one unused table key per specialised class.
 
 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
@@ -55,15 +43,20 @@ because `LOOP.md` refers to items by number.
    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.
+   What Jass does, from `TypeClassTests_dispatchInsideConstructor_no_opts.jim`: it specialises the
+   constructor function itself, `b_8 = new_Box⟪integer⟫(21)`. It gets there from *types*, not from
+   the call — `collectGenericUsages` collects a `GenericVar` for the local declared
+   `Box<integer{show}>` and a `GenericReturnTypeFunc` for `new_Box`, whose return type is generic.
+   The Lua collector has neither; it only ever looks at calls. So attaching type arguments to
+   constructor calls, which an earlier note here proposed, is not what the Jass path relies on and
+   would be a second mechanism rather than the same one.
+
+   The honest next step is to collect from types on the Lua path too, restricted the way item 5's
+   collection is. That runs straight into the same design question, though: `GenericVar` and
+   `GenericReturnTypeFunc` specialise the *class*, and item 5 showed that an object coming from a
+   specialised class while its methods are bound to the erased one breaks everything. Either the
+   collection has to specialise only the constructor path and leave the object erased, or Lua stops
+   erasing constructed generic classes — which is a decision about the erasure model, not a patch.
 
 7. **Module bounds.** `module M<T: Show>` is rejected with a clear message today. Needs
    receiver rewriting during expansion, or type parameters on `ModuleInstanciation`.
@@ -126,6 +119,24 @@ because `LOOP.md` refers to items by number.
 
 ## Blocked on a decision
 
+- **8. Should `div` and `mod` keep returning the left operand's type?** Tried returning
+  `WurstTypeInt.instance()` to match `caseMathOperation` and reverted it: it is a user-visible
+  breaking change, and the suite already defines the current behaviour as correct.
+
+  The asymmetry is real and reachable. `WurstTypeIntLiteral` is a proper subtype of both int and
+  real, and `caseMathOperation` collapses two literals to int precisely so `real r = 1 + 1` is an
+  error. `div`/`mod` return `leftType`, so `real r = 7 div 2` compiles. Changing that made exactly
+  one test fail — `OptimizerTests.realFormatting_consistent_fromIntOps`, which opens with
+  `real a = 1 div 2` — and AGENTS.md says the existing suite is the authoritative definition of
+  behaviour. Real maps will contain the same shape.
+
+  So the question is the owner's: is `real r = 7 div 2` meant to compile? If yes, the branch in
+  `AttrExprType` wants a comment saying so, and this item closes. If no, it is a deliberate
+  breaking change that needs the changelog, and `realFormatting_consistent_fromIntOps` needs
+  rewriting to say what it actually tests, which is real formatting rather than that assignment.
+  `ExpressionTests.integerDivisionOfLiteralsIsStillAssignableToReal` pins the behaviour meanwhile,
+  so whichever way it goes is deliberate rather than accidental.
+
 - **Eliminating the remaining `castTo int`.** The motivating case is timer data attachment
   (`ClosureTimers.wurst`), and the containers behind it: `Table` has 81 casts, `HashList` 13,
   `HashSet` 6, `HashMap` 4. None can adopt bounds as things stand, because an instance is
@@ -142,11 +153,18 @@ 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.
+- 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
+  is green with it throwing, which says nothing reachable produces one any more — and if something
+  starts to, it says so instead of returning a wrong answer.
+- 16. A never-written slot of a `T array` reads as the default of what T stands for. The default
+  is computed by a static attribute, which cannot see the frames that know the type argument, so
+  it produced a stand-in that compares equal only to another stand-in — `Box<int>.first() == 0`
+  was quietly false on the interpreter while both backends had it right. `ProgramState` does know
+  the substitution, so the stand-in is now resolved where the value is produced, at the array read
+  and the member read, rather than at the comparison where the symptom shows. Item 18 covers the
+  paths that could still leak one.
 - 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
@@ -204,6 +222,12 @@ because `LOOP.md` refers to items by number.
 - `%` is real modulo in Wurst; `mod` is integer modulo. `int % 8` types as `real`.
 - Emitted Lua must be byte-identical for identical input (AGENTS.md §8). It is the only
   emitted output that can be diffed across runs — see item 11.
+- 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.
+- The suite is the specification. Before changing what the type checker accepts, grep the tests for
+  the shape being rejected — item 8 looked like an oversight until one optimizer test turned out to
+  depend on it.
 - A test that hangs looks exactly like a test that is slow. If the suite stops making progress,
   take a thread dump of the forked worker (`jstack <pid>`) before killing it — it names the line.
 - Method names are not what the frontend called them. `LuaDispatchPreparation` renames a whole

de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/WurstOperator.java

@@ -1,6 +1,7 @@
 package de.peeeq.wurstscript;
 
 import de.peeeq.wurstscript.attributes.AttrFuncDef;
+import de.peeeq.wurstio.jassinterpreter.InterpreterException;
 import de.peeeq.wurstscript.intermediatelang.*;
 import de.peeeq.wurstscript.jassAst.JassAst;
 import de.peeeq.wurstscript.jassAst.JassOpBinary;
@@ -129,6 +130,28 @@ public LuaOpBinary luaTranslateBinary() {
         throw new Error("cannot translate " + this);
     }
 
+    /**
+     * Refuses to compare the stand-in for a type parameter's default, whichever side it is on.
+     * <p>
+     * The stand-in exists because the default of a value is computed by a static attribute, which
+     * cannot see what the parameter is bound to. It answers "equal" only for another stand-in, so
+     * comparing one against a real value is a wrong answer rather than an error. Doing this here
+     * rather than in the value itself keeps it independent of operand order: only the left operand
+     * gets asked, so `0 == unresolved` would otherwise go quietly false while `unresolved == 0`
+     * complained.
+     */
+    private static void rejectUnresolvedDefault(ILconst left, ILconst right) {
+        ILconstUnsafeDefault unresolved = left instanceof ILconstUnsafeDefault leftDefault ? leftDefault
+            : right instanceof ILconstUnsafeDefault rightDefault ? rightDefault : null;
+        if (unresolved == null || (left instanceof ILconstUnsafeDefault && right instanceof ILconstUnsafeDefault)) {
+            return;
+        }
+        throw new InterpreterException("The default value of type parameter "
+            + unresolved.getTypeVariable().getName()
+            + " is not known here, so it cannot be compared to "
+            + (unresolved == left ? right : left).print() + ".");
+    }
+
     public ILconst evaluateBinaryOperator(ILconst left,
                                           Supplier<ILconst> right) {
         switch (this) {
@@ -140,8 +163,11 @@ public ILconst evaluateBinaryOperator(ILconst left,
                 return new ILconstInt(((ILconstInt) left).getVal() / ((ILconstInt) right.get()).getVal());
             case DIV_REAL:
                 return new ILconstReal(getReal(left) / getReal(right.get()));
-            case EQ:
-                return ILconstBool.instance(left.equals(right.get()));
+            case EQ: {
+                ILconst rightVal = right.get();
+                rejectUnresolvedDefault(left, rightVal);
+                return ILconstBool.instance(left.equals(rightVal));
+            }
             case GREATER:
                 return ((ILconstNum) left).greater((ILconstNum) right.get());
             case GREATER_EQ:
@@ -160,8 +186,11 @@ public ILconst evaluateBinaryOperator(ILconst left,
                 return new ILconstReal(moduloReal(getReal(left), getReal(right.get())));
             case MULT:
                 return ((ILconstNum) left).mul((ILconstNum) right.get());
-            case NOTEQ:
-                return ILconstBool.instance(!left.equals(right.get()));
+            case NOTEQ: {
+                ILconst rightVal = right.get();
+                rejectUnresolvedDefault(left, rightVal);
+                return ILconstBool.instance(!left.equals(rightVal));
+            }
             case PLUS:
                 return ((ILconstAddable) left).add((ILconstAddable) right.get());
             case NOT:

de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/ILconstUnsafeDefault.java

@@ -18,13 +18,18 @@ public String print() {
         return "unsafe-default<" + typeVariable.getName() + ">";
     }
 
+    public ImTypeVar getTypeVariable() {
+        return typeVariable;
+    }
+
     public WurstType getType() {
         return WurstTypeInfer.instance();
     }
 
     @Override
     public boolean isEqualTo(ILconst other) {
+        // Comparing this against a real value is refused by WurstOperator, which can see both
+        // operands; doing it here would depend on which side the stand-in happened to land on.
         return other instanceof ILconstUnsafeDefault;
     }
-
 }

de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/EvaluateExpr.java

@@ -223,9 +223,9 @@ public static ILconst eval(ImVarArrayAccess e, ProgramState globalState, LocalSt
         }
 
         if (e.getVar().isGlobal()) {
-            return notNull(globalState.getArrayVal(e.getVar(), indexes), e.getVar().getType(), "Variable " + e.getVar().getName() + " is null.", false);
+            return globalState.resolveDefault(notNull(globalState.getArrayVal(e.getVar(), indexes), e.getVar().getType(), "Variable " + e.getVar().getName() + " is null.", false));
         } else {
-            return notNull(localState.getArrayVal(e.getVar(), indexes), e.getVar().getType(), "Variable " + e.getVar().getName() + " is null.", false);
+            return globalState.resolveDefault(notNull(localState.getArrayVal(e.getVar(), indexes), e.getVar().getType(), "Variable " + e.getVar().getName() + " is null.", false));
         }
     }
 
@@ -292,7 +292,8 @@ public static ILconst eval(ImMemberAccess ma, ProgramState globalState, LocalSta
             Integer val = ((ILconstInt) i.evaluate(globalState, localState)).getVal();
             indexes.add(val);
         }
-        return receiver.get(ma.getVar(), indexes).orElseGet(() -> ma.attrTyp().defaultValue());
+        return globalState.resolveDefault(
+            receiver.get(ma.getVar(), indexes).orElseGet(() -> ma.attrTyp().defaultValue()));
     }
 
     public static ILconst eval(ImAlloc e, ProgramState globalState, LocalState localState) {

de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/ProgramState.java

@@ -395,6 +395,23 @@ public ImType resolveType(ImType t) {
         return resolveTypeDeep(t, 32); // small budget to avoid cycles
     }
 
+    /**
+     * Replaces the stand-in default of a type parameter with the default of the type bound to it.
+     * <p>
+     * The default of a value is computed by a static attribute, which cannot see the frames that
+     * know what the parameter stands for, so it produces a stand-in. Reading a slot of a
+     * {@code T array} that was never written is how one reaches a program: the stand-in compares
+     * equal only to another stand-in, so a comparison against the real default is quietly false.
+     * The frames are known here, so resolve it where the value is produced.
+     */
+    public ILconst resolveDefault(ILconst value) {
+        if (!(value instanceof ILconstUnsafeDefault unsafeDefault)) {
+            return value;
+        }
+        ImType resolved = resolveType(JassIm.ImTypeVarRef(unsafeDefault.getTypeVariable()));
+        return resolved instanceof ImTypeVarRef ? value : resolved.defaultValue();
+    }
+
     private ImType resolveTypeDeep(ImType t, int budget) {
         if (budget <= 0 || t == null) return t;
 

de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ExpressionTests.java

@@ -486,6 +486,45 @@ private String makeProg(String booleanExpr) {
         return prog;
     }
 
+    /**
+     * An integer literal is a proper subtype of both int and real. Addition collapses two of them
+     * to int, so {@code real r = 1} is allowed while {@code real r = 1 + 1} is not; {@code div} and
+     * {@code mod} return the left operand's type instead, so a literal stays assignable to a real
+     * through them. This pins the asymmetry rather than endorsing it — see backlog item 8.
+     */
+    @Test
+    public void integerDivisionOfLiteralsIsStillAssignableToReal() {
+        testAssertOkLines(false,
+                "package test",
+                "init",
+                "    real quotient = 7 div 2",
+                "    real remainder = 7 mod 2"
+        );
+    }
+
+    @Test
+    public void additionOfLiteralsIsNotAssignableToReal() {
+        testAssertErrorsLines(false, "Cannot assign int to real",
+                "package test",
+                "init",
+                "    real sum = 7 + 2"
+        );
+    }
+
+    /** Whatever the declared type, both are integer operations at runtime. */
+    @Test
+    public void integerDivisionAndModuloStayInt() {
+        testAssertOkLines(true,
+                "package test",
+                "native testSuccess()",
+                "init",
+                "    int d = 7 div 2",
+                "    int m = 7 mod 2",
+                "    if d == 3 and m == 1",
+                "        testSuccess()"
+        );
+    }
+
     public void assertOk(String booleanExpr) {
         String prog = makeProg(booleanExpr);
         testAssertOk(UtilsIO.getMethodName(1), true, prog);