diff --git a/BACKLOG.md b/BACKLOG.md index 04e4e650c..420406814 100644 --- a/BACKLOG.md +++ b/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 - private static T array none - static function first() returns T - return none[0] - init - if Box.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` 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` 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(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` 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` 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.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 `) before killing it — it names the line. - Method names are not what the frontend called them. `LuaDispatchPreparation` renames a whole diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/WurstOperator.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/WurstOperator.java index d9e27d8b3..b298a8702 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/WurstOperator.java +++ b/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. + *

+ * 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 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: diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/ILconstUnsafeDefault.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/ILconstUnsafeDefault.java index ca1a28f39..52cfbbb53 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/ILconstUnsafeDefault.java +++ b/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; } - } diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/EvaluateExpr.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/EvaluateExpr.java index b4ef93be9..df15e6d64 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/EvaluateExpr.java +++ b/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) { diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/ProgramState.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/ProgramState.java index d89f0b376..9a36bb7b7 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/ProgramState.java +++ b/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. + *

+ * 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; diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ExpressionTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ExpressionTests.java index 6780129a7..e92b3c230 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ExpressionTests.java +++ b/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); 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 021b416d7..89bcb0702 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 @@ -264,6 +264,46 @@ public void nestedClosuresInsideBoundedGenericAreRejectedForLua() { ); } + /** + * 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 + * it produces a stand-in — and a stand-in compares equal only to another stand-in, which made + * this quietly false on the interpreter while both backends had it right. + */ + @Test + public void unwrittenArrayOfATypeParameterReadsAsItsDefault() { + testAssertOkLines(true, + "package test", + "native testSuccess()", + "class Box", + " private static T array none", + " static function first() returns T", + " return none[0]", + "init", + " if Box.first() == 0 and Box.first() == null", + " testSuccess()" + ); + } + + /** + * The same comparison the other way round. Only the left operand is asked whether it is equal, + * so a stand-in on the right would have gone quietly false while one on the left complained. + */ + @Test + public void unwrittenArrayOfATypeParameterReadsAsItsDefaultReversed() { + testAssertOkLines(true, + "package test", + "native testSuccess()", + "class Box", + " private static T array none", + " static function first() returns T", + " return none[0]", + "init", + " if 0 == Box.first() and null == Box.first()", + " testSuccess()" + ); + } + /** Each type argument picks its own instance, so one generic serves several types. */ @Test public void twoInstancesOfOneClass() {