Find a type variable by what it was copied from, not by its name

BACKLOG.md

@@ -87,15 +87,30 @@ 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.
+
+ 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
@@ -142,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<T: Show>` with `function convert<Q: Show>(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

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

@@ -509,6 +509,14 @@ public void popTypeArguments() {
  for (Map.Entry<ImTypeVar, ImTypeArgument> 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();

de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/EliminateGenerics.java

@@ -911,6 +911,9 @@ private void moveFunctionsOutOfClass(ImClass c) {
  List<ImTypeVar> 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<ImTypeVar> typeVars) {
+ private void inheritTypeClassBinding(ImTypeArgument ta, ImType original,
+ GenericTypes generics, List<ImTypeVar> 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<ImTypeVar> 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.
+ * <p>
+ * 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<ImTypeVar> originals, List<ImTypeVar> copies) {
+ for (int i = 0; i < originals.size() && i < copies.size(); i++) {
+ translator.recordCopiedTypeVar(copies.get(i), originals.get(i));
+ }
+ }
+
+ private int indexOfTypeVar(List<ImTypeVar> 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

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<ImVar, ImVar> specializedFieldOrigins = new java.util.IdentityHashMap<>();
 
+ /**
+ * The type variable each copy was made from.
+ * <p>
+ * 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<ImTypeVar, ImTypeVar> 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);
  }

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

@@ -393,6 +393,64 @@ public void superCallToAGenericMethodOfABoundedGenericClass() {
  testAssertOkLines(true, SUPER_CALL_TO_A_GENERIC_METHOD);
  }
 
+ /**
+ * 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.
+ * <p>
+ * 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<T:>",
+ " function show(T x) returns int",
+ "implements Show<int>",
+ " function show(int x) returns int",
+ " return x * 2",
+ "implements Show<string>",
+ " function show(string x) returns int",
+ " return 7",
+ "class Holder<T: Show>",
+ " 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: Show>(T other) returns int",
+ " return T.show(other)",
+ "init",
+ " let h = new Holder<int>(21)",
+ " if h.shown() == 42 and h.convert<string>(\"x\") == 7",
+ " testSuccess()",
+ };
+
+ @Test
+ public void aBoundedMethodParameterMayShareTheClassParameterName() {
+ 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.
+ * <p>
+ * 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. */
  @Test
  public void twoInstancesOfOneClass() {