diff --git a/BACKLOG.md b/BACKLOG.md index aa81417ac..561a46df4 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -49,54 +49,43 @@ itself, and one gap in what the suite can see. Collecting from types on the Lua path runs straight into item 23, so settle that first. -23. **The Lua erasure model, which items 6 and 13 both end at.** On Lua a generic class is erased, - and specialised copies are made only where a construction names the instantiation. Every - remaining gap on that target is one question: an object allocated from a specialised class while - its methods are bound to the erased one breaks, and an object allocated from the erased class - cannot reach a specialised method. - - Two ways out, and it is a decision rather than a patch. Either specialise only the paths which - need a concrete type and leave the object erased throughout, or stop erasing constructed generic - classes on Lua and pay the code size. - - #1239 is a reason to take it seriously rather than leave it. Both class shapes existing at once - is what let an instance be allocated with no fields at all, and then with its fields under a key - nothing read. Both are fixed; the shape which produced them is still there. - - Do not start this autonomously. - -7. **Module bounds.** `module M` is rejected with a clear message today, and - `TypeClassTests.boundOnModuleTypeParameterIsRejected` pins that. Tried and reverted; what follows - is why, because the earlier note here suggested a fix which cannot work. - - Expansion copies the module body into the user and replaces the module's type parameters - **in type positions**. A requirement is called on the parameter itself — `T.show(x)` — and that - receiver is a name, resolved by `lookupBoundedTypeParam` through `lookupType`, so the replacement - never touches it. - - Renaming the receiver to the using class's parameter, which is what "receiver rewriting during - expansion" meant, does not work. `NameResolution.nextScope` sends a `ModuleInstanciation` to - `attrModuleOrigin()` rather than to the class using it: - - if (currentScope instanceof ModuleInstanciation) { - return nextScope(moduleInstanciation.attrModuleOrigin()); - } - - That is deliberate — a module body resolves in the module's own scope so it cannot capture the - names of whoever uses it — so the renamed receiver names something that scope cannot see. The - rename itself works: with it, the error moves from the rejection to `Could not find variable K` - at the dispatch, which is this scope rule and not a mistake in the rename. - - That leaves the other half of the original note: **type parameters on `ModuleInstanciation`**. The - instantiation declares the parameter itself, bound to the argument, so the copied body keeps - saying `T` and `T` resolves without any rename. It needs `ModuleInstanciation` to carry type - parameters in the grammar, so it is a change to `wurstscript.parseq` and everything reading that - node, not a patch to the expander. - - Worth knowing before starting: an argument which is a concrete type (`use Shower`) is a - second case even then. A requirement is dispatched on a type parameter, so `int.show(x)` is not a - dispatch at all — that one has to resolve to the instance during expansion rather than resolve by - name. +23. **The Lua erasure model.** Decided: **specialise only the paths which need a concrete type and + leave the object erased throughout.** Generated scripts stay small, which is the reason for the + choice; the cost is that it is more compiler work than the alternative of not erasing at all. + + What that means in practice. An object keeps coming from the erased class, so it must never need a + specialised method - the concrete type is threaded to the places which use it rather than to the + object. Items 6 and 13 both end here: a constructor's dispatch needs the type at the construction + site, and a subclass's `super` call needs it on the call rather than on the receiver's class. + + Take it seriously rather than working around it. Two class shapes existing at once is what let an + instance be allocated with no fields at all, and then with its fields under a key nothing read + (#1239). Both are fixed; the shape which produced them is what this decision removes. + + Unblocks items 6 and 13's Lua half. Not blocking the container, which works on both targets today. + +7. **Module bounds.** Decided: the instantiation declares the module's type parameters **only so a + dispatch receiver has a name to resolve**, and they are excluded from type inference, which keeps + resolving a generic module's parameters by matching the receiver type as it does today. + + Why that way. A requirement of a bound is called on the parameter itself - `T.show(x)` - and that + receiver is a name, which the type replacement during expansion never touches. Renaming it to the + using class's parameter cannot work: `NameResolution.nextScope` sends a `ModuleInstanciation` to + `attrModuleOrigin()` rather than to the class using it, deliberately, so a module body cannot see + the names of whoever uses it. The parameter therefore has to be declared where the body can see it. + + Why only for the receiver. Declaring it and letting inference see it collides with the existing + mechanism: `GenericsModuleTests.genericModuleInGenericClassGet` fails with "Cannot infer type for + type parameter T". Two mechanisms answering one question is the cost of the alternative; this is + the smaller change, at the price of the parameter meaning something narrower than it looks. + + Started on `feat/module-instanciation-type-params`, unpushed. The grammar carries `typeParameters` + and `typeArgs` (resolved, since an argument names something only the user's scope can see), + resolution binds a declared parameter to its argument through `WurstTypeBoundTypeParam`, and + `isTypeClassDispatch` accepts a receiver which denotes a parameter through a binding. The error + chain reached "Could not find function show", which is the requirement lookup not following a + binding to the underlying parameter's bounds - the same widening, wherever a bound's functions are + surfaced. Verify before continuing that inference can be told to ignore the declared parameters. 9. **Keep `WURST_LANGUAGE.md` and `CHANGELOG.md` current** as items land — a standing practice rather than a task to finish. `WURST_LANGUAGE.md` is tracked, at @@ -200,6 +189,34 @@ itself, and one gap in what the suite can see. path, and `transformGenericNewOnly` does not lift them, so the method's own parameter is counted against a list which does not include the class's. +26. **Nothing in the compiler should recover structure from a name, and one place doing it is a bug + today.** A dispatch slot's name is composed from the segment after the last underscore of a + method's mangled name, which is the declared name only when the declared name has no underscore in + it: `get_it` contributes `it`, which is nobody's method. + + That is not only untidy. `LuaTranslationTests.underscoreNamedOverrideInAGenericHierarchyIsStillBrokenOnLua` + pins the consequence - an override named `get_it` in a generic hierarchy does not dispatch on Lua, + while the same shape without the underscore does. Found by auditing the junk-slot rule for what its + name comparison does to unrelated methods, not by anyone hitting it. + + Asking the declaration instead is not the whole fix, and trying it is how the rest of this was + found. `LuaDispatchPreparation.declaredName` already reads the name off the trace, but two + overloads share a declared name: they mangle to `Foo_bar` and `Foo_bar_1`, and the segment after + the last underscore is also what currently keeps their slots apart. Pointing both composers at the + declared name fixes the underscore case and collapses overloaded slots instead - + `overloadedMethodsDoNotAliasInLuaDispatchTables` and + `moduleProvidedOverloadedOverrideDoesNotCollapseLuaSlots` both catch it. + + So the replacement is keyed on the declared signature, not the declared name: what a method and its + overrides share, and what distinguishes two overloads, are two different questions and the mangled + name is currently answering both at once by accident. Same shape as #1248 and #1249 - record it + where it is known - but it is a signature, not a rename. + + The rest of the family, for the same treatment: `ProgramState.identifyGenericStaticGlobals` takes + the longest prefix of a global's name ending at an underscore which matches a class name, which a + class whose name contains an underscore answers wrongly and silently. #1249 made the recorded owner + preferred where one exists, so this is now only the fallback. + 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 caught a bug already found; a place where two mechanisms do the same job and disagree; a @@ -208,23 +225,11 @@ itself, and one gap in what the suite can see. ## 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. +- **8. Settled: `div` and `mod` keep returning the left operand's type**, so `real r = 7 div 2` + compiles and is meant to. The branch in `AttrExprType` now says so, rather than looking like an + oversight next to `caseMathOperation`, which collapses two literals to int precisely so + `real r = 1 + 1` is an error. `ExpressionTests.integerDivisionOfLiteralsIsStillAssignableToReal` + pins it and `OptimizerTests.realFormatting_consistent_fromIntOps` depends on it. Nothing to do. - **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, @@ -234,6 +239,10 @@ itself, and one gap in what the suite can see. question: what the syntax is, where such an instance may be declared under the orphan rule, and whether a specific instance always beats a family one. Do not start this autonomously. + Deferred deliberately, not forgotten. It decides whether type class bounds stay a tool for new + containers or become how the existing ones work, which is worth deciding when there is appetite for + the language design rather than alongside compiler work. + ## Out of scope - The stdlib itself. `de.peeeq.wurstscript/temp/WurstStdlib2` is a fetched artefact for tests; diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/AttrExprType.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/AttrExprType.java index 1a385b49b..fd15c06af 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/AttrExprType.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/AttrExprType.java @@ -333,6 +333,14 @@ public static WurstType calculate(final ExprBinary term) { case JASS_MOD_INT: case DIV_INT: if (leftType.isSubtypeOf(WurstTypeInt.instance(), term) && rightType.isSubtypeOf(WurstTypeInt.instance(), term)) { + // The left operand's type, deliberately, which is not what caseMathOperation does: + // that collapses two literals to int so `real r = 1 + 1` is an error. Returning it + // here means `real r = 7 div 2` compiles, and it is meant to - the division is + // integer either way and the result is then widened. + // + // Asked and settled rather than left as an accident. + // ExpressionTests.integerDivisionOfLiteralsIsStillAssignableToReal pins it, and + // OptimizerTests.realFormatting_consistent_fromIntOps opens with that assignment. return leftType; } term.addError("Operator " + term.getOp() + " is not defined for " + diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaTranslationTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaTranslationTests.java index c64ac4144..93cbc66cd 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaTranslationTests.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaTranslationTests.java @@ -1408,6 +1408,44 @@ public void genericOverrideChainBindsRootSlotToMostSpecificImplInLua() throws IO } } + /** + * An override whose declared name contains an underscore does not dispatch inside a generic + * hierarchy on Lua, and this pins that rather than leaving it to be met by surprise. + *

+ * A dispatch slot's name is composed from the segment after the last underscore of the method's + * mangled name, so {@code get_it} contributes {@code it} - which is nobody's method - and the slot + * the call goes through is not the one the override was bound to. The same shape without the + * underscore works, and so does this one outside a generic hierarchy. + *

+ * Found by auditing the junk-slot rule for what its name comparison does to unrelated methods, + * rather than by anyone hitting it. Asking the declaration for the name instead is not the whole + * fix: two overloads share a declared name and mangle to {@code Foo_bar} and {@code Foo_bar_1}, so + * the segment after the last underscore is also what currently keeps their slots apart. Backlog + * item 26 carries what the replacement has to be keyed on. + */ + @Test(expectedExceptions = Error.class, expectedExceptionsMessageRegExp = ".*Succeed function not called.*") + public void underscoreNamedOverrideInAGenericHierarchyIsStillBrokenOnLua() { + test().testLua(true).executeProg().lines( + "package test", + "native testSuccess()", + "class Holder", + " T value", + " construct(T value)", + " this.value = value", + " function get_it() returns T", + " return value", + "class Doubler extends Holder", + " construct(int value)", + " super(value)", + " override function get_it() returns int", + " return value * 2", + "init", + " Holder h = new Doubler(21)", + " if h.get_it() == 42", + " testSuccess()" + ); + } + @Test public void luaOutputIsDeterministicForGenericOverrideSlots() throws IOException { test().testLua(true).compilationUnits(genericOverrideReproUnits());