From 0192629f91e01b887e3b51fe3bf1304f7d705185 Mon Sep 17 00:00:00 2001 From: Frotty Date: Sun, 16 Aug 2026 18:06:57 +0200 Subject: [PATCH 1/3] Find a type variable by what it was copied from, not by its name Three places copy a function's or a class's type variables and then empty the copy's list, which leaves every reference inside the copy pointing at a node with no owner at all. Comparing names was not compensating for the occasional duplicate the note here described: it was the only thing holding those references together, and removing it broke most of the generics tests. Recording each copy against what it was made from, before the list is emptied, replaces it. Name matching is gone from indexOfTypeVar and from inheritTypeClassBinding through it, so two parameters which merely share a name can no longer be taken for one where dispatch is resolved. The interpreter's lookup still matches names. The record lives on the ImTranslator and the interpreter is handed a program rather than the translation which produced it; the comment there says so rather than leaving it looking like an oversight. --- BACKLOG.md | 29 ++++++++++----- .../interpreter/ProgramState.java | 8 ++++ .../imtranslation/EliminateGenerics.java | 36 +++++++++++++++--- .../imtranslation/ImTranslator.java | 30 +++++++++++++++ .../wurstscript/tests/TypeClassTests.java | 37 +++++++++++++++++++ 5 files changed, 126 insertions(+), 14 deletions(-) diff --git a/BACKLOG.md b/BACKLOG.md index 86165e337..ae3fa1bdf 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -87,15 +87,26 @@ itself, and one gap in what the suite can see. the repository root, which is easy to miss when looking for it. Fold an update into whichever item changes the behaviour rather than doing it as a separate pass. -10. **One `ImTypeVar` per type parameter.** Name-tolerant lookups remain in - `EliminateGenerics.indexOfTypeVar`, `inheritTypeClassBinding` and - `ProgramState.getCurrentTypeArgument`, compensating for several nodes standing for one - source parameter. Making the node canonical lets all three compare by identity and removes - a class of silent wrong dispatch. Mechanical, well covered by the suite. - - Note before starting: `moveFunctionsOutOfClass` copies a class's type variables onto each - function it moves out, deliberately, and #1237 depends on that copy. Identity cannot hold across - that boundary, so what is being made canonical is per scope rather than per program. +10. **The interpreter still finds a type argument by name.** + `ProgramState.getCurrentTypeArgument` compares `ImTypeVar` nodes by name, so two parameters which + merely share one look like the same parameter. + + The two lookups in `EliminateGenerics` no longer do. What they compare is what each node was + copied from: three places copy a function's or a class's type variables and then empty the copy's + list, which leaves every reference inside pointing at a node belonging to nothing — the name + match was not compensating for the odd duplicate, it was the only thing holding those references + together. Recording the copy before the list is emptied replaces it entirely, and name matching + is gone from `indexOfTypeVar` and from `inheritTypeClassBinding` through it. + + The record lives on the `ImTranslator`. The interpreter is handed a program rather than the + translation which produced it, so reaching the record from `ProgramState` means threading the + translator through the interpreter — four construction sites, three of them tests, but an API + change all the same. That is what is left of this item. + + Note for whoever takes it: a test which merely gives a class and a method a same-named parameter + passes either way. The lookups are not reached with both in one list by that shape, and a program + which does reach them that way was not found, so the change rests on removing the name comparison + rather than on a failing case. 13. **A bounded generic class cannot be subclassed on Lua.** The Jass half landed in #1237: a call which names its target outright now carries the class's type arguments, taken from the class its 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 9a36bb7b7..7906cee26 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 @@ -509,6 +509,14 @@ public void popTypeArguments() { for (Map.Entry e : frame.entrySet()) { // A class and its constructor hold separate nodes for the same source type // parameter, so identity alone is not enough to find the binding. + // + // EliminateGenerics no longer needs this: it records what each copy was made from and + // compares that. The record lives on the ImTranslator, which the interpreter is not + // given - it is handed a program, not the translation that produced it - so matching + // on the name is what is left here. It is wrong in the same way it was wrong there: + // two parameters which merely share a name look like one. Reaching the record from + // here means threading the translator through the interpreter, which is its own + // change; backlog item 10 carries it. boolean sameVar = e.getKey() == typeVar || e.getKey().getName().equals(typeVar.getName()); if (sameVar && !e.getValue().getTypeClassBinding().isEmpty()) { return e.getValue(); 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 6698de6e8..5c62725d4 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 @@ -911,6 +911,9 @@ private void moveFunctionsOutOfClass(ImClass c) { List newTypeVars = new ArrayList<>(); for (ImTypeVar imTypeVar : c.getTypeVariables()) { ImTypeVar copy = imTypeVar.copy(); + // One source parameter becomes several nodes here. Recorded so the two can be + // recognised as the same parameter without falling back to comparing names. + translator.recordCopiedTypeVar(copy, imTypeVar); newTypeVars.add(copy); } f.getTypeVariables().addAll(0, newTypeVars); @@ -1133,6 +1136,7 @@ private ImFunction specializeFunction(ImFunction f, GenericTypes generics) { prog.getFunctions().add(newF); // concrete clone => no type vars + recordCopiedTypeVars(f.getTypeVariables(), newF.getTypeVariables()); newF.getTypeVariables().removeAll(); newF.setName(genericNewOnly @@ -1219,6 +1223,7 @@ private ImFunction specializeMethodImplementation(ImMethod method, GenericTypes specializedFunctions.put(implementation, generics, newImplementation); specializedFunctionGenerics.put(newImplementation, generics); prog.getFunctions().add(newImplementation); + recordCopiedTypeVars(implementation.getTypeVariables(), newImplementation.getTypeVariables()); newImplementation.getTypeVariables().removeAll(); newImplementation.setName(implementation.getName() + "_specialized"); rewriteGenerics(newImplementation, generics, typeVariables); @@ -1367,8 +1372,8 @@ public void visit(ImTypeVarDispatch e) { * outer parameter also supplies the instance it was specialised with, which is what makes a * chain of bounded generics resolve without any runtime dictionary. */ - private static void inheritTypeClassBinding(ImTypeArgument ta, ImType original, - GenericTypes generics, List typeVars) { + private void inheritTypeClassBinding(ImTypeArgument ta, ImType original, + GenericTypes generics, List typeVars) { if (!ta.getTypeClassBinding().isEmpty() || !(original instanceof ImTypeVarRef ref)) { return; } @@ -1395,10 +1400,30 @@ private static String enclosingFunctionName(Element e) { return cur == null ? "?" : ((ImFunction) cur).getName(); } - private static int indexOfTypeVar(List typeVars, ImTypeVar target) { + /** + * Where {@code target} sits in {@code typeVars}, comparing what each was copied from rather than + * the nodes themselves: moving a function out of its class copies the class's type variables onto + * it, so one source parameter is several nodes. Comparing names instead would make two parameters + * which merely share a name look like one. + */ + /** + * Pairs a copy's type variables with the ones they were copied from, index by index. + *

+ * Copying a function or a class copies its type variables with it, and the references inside the + * copy point at the new nodes. The copy's list is then emptied, which leaves those nodes with no + * owner at all - so a reference reached later has nothing to compare against but a name. Recorded + * before that happens, the copy still leads back to the parameter it stands for. + */ + private void recordCopiedTypeVars(List originals, List copies) { + for (int i = 0; i < originals.size() && i < copies.size(); i++) { + translator.recordCopiedTypeVar(copies.get(i), originals.get(i)); + } + } + + private int indexOfTypeVar(List typeVars, ImTypeVar target) { + ImTypeVar wanted = translator.canonicalTypeVar(target); for (int i = 0; i < typeVars.size(); i++) { - ImTypeVar tv = typeVars.get(i); - if (tv == target || tv.getName().equals(target.getName())) { + if (translator.canonicalTypeVar(typeVars.get(i)) == wanted) { return i; } } @@ -1479,6 +1504,7 @@ private ImClass specializeClass(ImClass c, GenericTypes generics) { } specializedClasses.put(c, generics, newC); prog.getClasses().add(newC); + recordCopiedTypeVars(c.getTypeVariables(), newC.getTypeVariables()); newC.getTypeVariables().removeAll(); newC.setName(genericNewOnly diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/ImTranslator.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/ImTranslator.java index 93f480826..2cfed11f6 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/ImTranslator.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/ImTranslator.java @@ -52,6 +52,36 @@ public class ImTranslator { */ private final java.util.Map specializedFieldOrigins = new java.util.IdentityHashMap<>(); + /** + * The type variable each copy was made from. + *

+ * Moving a function out of its class copies the class's type variables onto it, deliberately, so + * one source parameter is several nodes and identity alone cannot recognise them. Matching on the + * name instead makes two parameters which merely share a name look like one, which is a wrong + * dispatch rather than a missed one; following the copy back to what it was made from tells them + * apart. + */ + private final java.util.Map typeVarOrigins = new java.util.IdentityHashMap<>(); + + public void recordCopiedTypeVar(ImTypeVar copy, ImTypeVar original) { + typeVarOrigins.put(copy, original); + } + + /** The type variable {@code tv} was ultimately copied from, or {@code tv} itself. */ + public ImTypeVar canonicalTypeVar(ImTypeVar tv) { + ImTypeVar current = tv; + // A copy of a copy is possible, so follow to the root; the map is acyclic by construction + // because a copy is always newer than what it was made from. + for (int steps = 0; steps < 100; steps++) { + ImTypeVar origin = typeVarOrigins.get(current); + if (origin == null) { + return current; + } + current = origin; + } + return current; + } + public void recordSpecializedField(ImVar copy, ImVar original) { specializedFieldOrigins.put(copy, original); } 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 031583379..35dc08b9b 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 @@ -393,6 +393,43 @@ public void superCallToAGenericMethodOfABoundedGenericClass() { testAssertOkLines(true, SUPER_CALL_TO_A_GENERIC_METHOD); } + /** + * A class and a method inside it may each declare a type parameter spelled the same. They are + * different parameters bound to different types at the same moment, and this covers that the + * shape compiles and runs. + *

+ * It passes without the canonical type variable change as well as with it, so it does not pin + * that change — the lookups which used to compare names are not reached with both parameters in + * one list here, and a program which does reach them that way was not found. It is kept for the + * language case rather than for the fix. + */ + @Test + public void aMethodTypeParameterMayShareTheClassParameterName() { + testAssertOkLines(true, + "package test", + "native testSuccess()", + "interface Show", + " function show(T x) returns int", + "implements Show", + " function show(int x) returns int", + " return x * 2", + "class Holder", + " T held", + " construct(T held)", + " this.held = held", + " function shown() returns int", + " return T.show(held)", + // The method's T is a different parameter which happens to share the name, and it is + // unbounded: nothing may be dispatched on it. + " function convert(T other, int extra) returns int", + " return extra + 1", + "init", + " let h = new Holder(21)", + " if h.shown() == 42 and h.convert(\"x\", 1) == 2", + " testSuccess()" + ); + } + /** Each type argument picks its own instance, so one generic serves several types. */ @Test public void twoInstancesOfOneClass() { From 0dfd6b91a225767329814c71b187d6f00c50a6f0 Mon Sep 17 00:00:00 2001 From: Frotty Date: Mon, 17 Aug 2026 09:48:05 +0200 Subject: [PATCH 2/3] Pin the type variable fix with a repro that fails without it The earlier test gave a class and a method a same-named parameter and left the method's unbounded, so nothing ever dispatched on it and the lookup was never asked to tell the two apart. It passed either way, which I said in the pull request rather than fixing. Bounding the method's parameter and giving it a different instance is what makes both reach the same lookup. The class is built over int, whose instance doubles; the method is called with string, whose instance answers 7. Without the change the string is dispatched through the int instance and the interpreter dies casting it to a number, so the bug was a wrong dispatch rather than the tidiness the backlog described. --- BACKLOG.md | 12 ++++++--- .../wurstscript/tests/TypeClassTests.java | 27 ++++++++++--------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/BACKLOG.md b/BACKLOG.md index ae3fa1bdf..28d4d378d 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -103,10 +103,14 @@ itself, and one gap in what the suite can see. translator through the interpreter — four construction sites, three of them tests, but an API change all the same. That is what is left of this item. - Note for whoever takes it: a test which merely gives a class and a method a same-named parameter - passes either way. The lookups are not reached with both in one list by that shape, and a program - which does reach them that way was not found, so the change rests on removing the name comparison - rather than on a failing case. + Pinned by `TypeClassTests.aBoundedMethodParameterMayShareTheClassParameterName`: a class over int + whose instance doubles, and a method parameter of the same name bounded and called with string, + whose instance answers 7. Without the change the string is dispatched through the int instance and + the interpreter dies casting it to a number, which is what telling the two apart prevents. + + A weaker version of that test - same names, method parameter unbounded - passes either way, so it + proved nothing. The bound and the second instance are what make the two parameters reach the same + lookup. 13. **A bounded generic class cannot be subclassed on Lua.** The Jass half landed in #1237: a call which names its target outright now carries the class's type arguments, taken from the class its 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 35dc08b9b..694798546 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 @@ -394,17 +394,16 @@ public void superCallToAGenericMethodOfABoundedGenericClass() { } /** - * A class and a method inside it may each declare a type parameter spelled the same. They are - * different parameters bound to different types at the same moment, and this covers that the - * shape compiles and runs. + * A class and a method inside it may each declare a bounded type parameter spelled the same, and + * they are different parameters bound to different types at the same moment. Which instance a + * requirement dispatches to therefore depends on telling the two apart. *

- * It passes without the canonical type variable change as well as with it, so it does not pin - * that change — the lookups which used to compare names are not reached with both parameters in - * one list here, and a program which does reach them that way was not found. It is kept for the - * language case rather than for the fix. + * {@code Holder} is built over int, whose instance doubles; {@code convert} is + * called with string, whose instance answers 7. Reading the method's parameter as the class's + * would dispatch through the int instance and answer 42 instead. */ @Test - public void aMethodTypeParameterMayShareTheClassParameterName() { + public void aBoundedMethodParameterMayShareTheClassParameterName() { testAssertOkLines(true, "package test", "native testSuccess()", @@ -413,19 +412,21 @@ public void aMethodTypeParameterMayShareTheClassParameterName() { "implements Show", " function show(int x) returns int", " return x * 2", + "implements Show", + " function show(string x) returns int", + " return 7", "class Holder", " T held", " construct(T held)", " this.held = held", " function shown() returns int", " return T.show(held)", - // The method's T is a different parameter which happens to share the name, and it is - // unbounded: nothing may be dispatched on it. - " function convert(T other, int extra) returns int", - " return extra + 1", + // A different parameter which happens to share the name, bound to a different instance. + " function convert(T other) returns int", + " return T.show(other)", "init", " let h = new Holder(21)", - " if h.shown() == 42 and h.convert(\"x\", 1) == 2", + " if h.shown() == 42 and h.convert(\"x\") == 7", " testSuccess()" ); } From 1f028680920258936d3062137d496801fd8685b1 Mon Sep 17 00:00:00 2001 From: Frotty Date: Mon, 17 Aug 2026 10:13:49 +0200 Subject: [PATCH 3/3] Pin the Lua behaviour of the same-named parameter shape Asked for Lua coverage of the corrected lookup, and it cannot be given: the only shape which brings two same-named parameters into one lookup is a class parameter beside a method parameter, and Lua rejects that with "Generics should match class method type variables" - on master as well, so it is a gap rather than a regression. A version Lua does compile, with the second parameter on a free function, was written and then deleted: it passes with the change and without it, because the two parameters never reach one lookup that way, so it would have claimed coverage it does not provide. The rejection is pinned instead, and the gap behind it is recorded as item 25 with where to start - the arity check counts a method's own parameter against a list that transformGenericNewOnly never lifted the class's variables onto. --- BACKLOG.md | 16 ++++ .../wurstscript/tests/TypeClassTests.java | 76 ++++++++++++------- 2 files changed, 64 insertions(+), 28 deletions(-) diff --git a/BACKLOG.md b/BACKLOG.md index 28d4d378d..6166a2b32 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -157,6 +157,22 @@ itself, and one gap in what the suite can see. between equal starting states, which would invalidate the conclusion rather than explain the failure. +25. **A bounded type parameter on a method of a generic class is rejected on Lua.** + `class Holder` with `function convert(Q other)` fails there with "Generics should + match class method type variables", while the same program compiles and runs on the other target. + On master as well, so it is not a regression — found while trying to give item 10's fix Lua + coverage, which is what it blocks: the only shape reaching that lookup with two parameters at once + is a class parameter beside a method parameter, and Lua will not compile it. + + Pinned by `TypeClassTests.aBoundedMethodParameterInAGenericClassIsRejectedForLua`. A version with + the second parameter on a free function does compile on Lua and passes with or without item 10's + change, so it covers nothing; that is why the rejection is pinned instead. + + Where to start: the message comes from the arity check between a call's generics and the callee's + type variables. A method of a generic class has the class's variables lifted onto it on the Jass + 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. + 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 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 694798546..4f6def0f2 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 @@ -398,37 +398,57 @@ public void superCallToAGenericMethodOfABoundedGenericClass() { * they are different parameters bound to different types at the same moment. Which instance a * requirement dispatches to therefore depends on telling the two apart. *

- * {@code Holder} is built over int, whose instance doubles; {@code convert} is - * called with string, whose instance answers 7. Reading the method's parameter as the class's - * would dispatch through the int instance and answer 42 instead. + * The class is built over int, whose instance doubles; the method is called with string, whose + * instance answers 7. Reading the method's parameter as the class's dispatches the string through + * the int instance, which multiplies it. */ + private static final String[] SAME_NAMED_BOUNDED_PARAMETERS = { + "package test", + "native testSuccess()", + "interface Show", + " function show(T x) returns int", + "implements Show", + " function show(int x) returns int", + " return x * 2", + "implements Show", + " function show(string x) returns int", + " return 7", + "class Holder", + " T held", + " construct(T held)", + " this.held = held", + " function shown() returns int", + " return T.show(held)", + // A different parameter which happens to share the name, bound to a different instance. + " function convert(T other) returns int", + " return T.show(other)", + "init", + " let h = new Holder(21)", + " if h.shown() == 42 and h.convert(\"x\") == 7", + " testSuccess()", + }; + @Test public void aBoundedMethodParameterMayShareTheClassParameterName() { - testAssertOkLines(true, - "package test", - "native testSuccess()", - "interface Show", - " function show(T x) returns int", - "implements Show", - " function show(int x) returns int", - " return x * 2", - "implements Show", - " function show(string x) returns int", - " return 7", - "class Holder", - " T held", - " construct(T held)", - " this.held = held", - " function shown() returns int", - " return T.show(held)", - // A different parameter which happens to share the name, bound to a different instance. - " function convert(T other) returns int", - " return T.show(other)", - "init", - " let h = new Holder(21)", - " if h.shown() == 42 and h.convert(\"x\") == 7", - " testSuccess()" - ); + testAssertOkLines(true, SAME_NAMED_BOUNDED_PARAMETERS); + } + + /** + * The same program on Lua, where it does not compile at all — and not because of the collision. + * A bounded type parameter on a method of a generic class is rejected on that target with + * "Generics should match class method type variables", on master as well as here, so the shape + * which exercises this lookup cannot be run there to check the dispatch. + *

+ * A version Lua does compile, with the second parameter on a free function rather than a method, + * passes without the change as well as with it: the two parameters never reach one lookup that + * way, so it would be coverage in name only. This pins the rejection instead, and the gap behind + * it is backlog item 25. Should that be fixed, this test fails and gains a dispatch assertion. + */ + @Test + public void aBoundedMethodParameterInAGenericClassIsRejectedForLua() { + test().testLua(true).executeProg() + .expectError("Generics should match class method type variables") + .lines(SAME_NAMED_BOUNDED_PARAMETERS); } /** Each type argument picks its own instance, so one generic serves several types. */