Let the interpreter ask what a node was copied from, and who owns it

de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/CompiletimeFunctionRunner.java

@@ -90,6 +90,9 @@ public CompiletimeFunctionRunner(
         this.translator = tr;
         this.imProg = imProg;
         globalState = new ProgramStateIO(mapFile, mpqEditor, gui, imProg, true);
+        // The interpreter is handed a program; this hands over the one thing it cannot work out from
+        // the program alone, which is what a specialised node was copied from.
+        globalState.setSpecialisations(tr);
         initializeBackendConstants();
         this.interpreter = new ILInterpreter(imProg, gui, mapFile, globalState);
 

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

@@ -11,6 +11,7 @@
 import de.peeeq.wurstscript.jassIm.*;
 import de.peeeq.wurstscript.parser.WPos;
 import de.peeeq.wurstscript.translation.imtojass.ImAttrType;
+import de.peeeq.wurstscript.translation.imtranslation.SpecialisationLookup;
 import de.peeeq.wurstscript.utils.LineOffsets;
 import de.peeeq.wurstscript.utils.Utils;
 import it.unimi.dsi.fastutil.ints.Int2ObjectOpenHashMap;
@@ -50,6 +51,20 @@ public class ProgramState extends State implements AutoCloseable {
     private final Object2ObjectOpenHashMap<String, ILconst> genericStaticScalarVals = new Object2ObjectOpenHashMap<>();
     private int untrackedWriteDepth;
 
+    /**
+     * What each specialised node was copied from, when the caller knows. A program handed over without
+     * it is treated as having no specialisation in it, which is what a hand-built program means.
+     */
+    private SpecialisationLookup specialisations = SpecialisationLookup.NONE;
+
+    public void setSpecialisations(SpecialisationLookup specialisations) {
+        this.specialisations = specialisations;
+        // The owners were worked out in the constructor, before this arrived, and the recorded answer
+        // is better than the one read out of a name - so ask again now that it can be asked.
+        genericStaticOwner.clear();
+        identifyGenericStaticGlobals();
+    }
+
     private static boolean containsTypeVariable(ImType type) {
         return type.match(new ImType.Matcher<Boolean>() {
             @Override public Boolean case_ImTypeVarRef(ImTypeVarRef t) { return true; }
@@ -122,9 +137,17 @@ private void identifyGenericStaticGlobals() {
         }
 
         for (ImVar global : prog.getGlobals()) {
-            String n = global.getName();
+            // Recorded where the global was created, when the caller supplied the relation.
+            ImClass recorded = specialisations.genericStaticOwnerOf(global);
+            if (recorded != null) {
+                genericStaticOwner.put(global, recorded);
+                continue;
+            }
 
-            // longest prefix ending at an underscore that matches a class name
+            // Otherwise the name is all there is: the longest prefix ending at an underscore which
+            // names a generic class. Wrong for a class whose name contains an underscore, and wrong
+            // silently, which is why the recorded answer is preferred.
+            String n = global.getName();
             int pos = n.lastIndexOf('_');
             while (pos > 0) {
                 String className = n.substring(0, pos);
@@ -507,17 +530,13 @@ public void popTypeArguments() {
     public @Nullable ImTypeArgument getCurrentTypeArgument(ImTypeVar typeVar) {
         for (Map<ImTypeVar, ImTypeArgument> frame : typeArgumentFrames) {
             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());
+                // A class and its constructor hold separate nodes for the same source type parameter,
+                // so identity alone does not find the binding. This used to fall back to comparing
+                // names, which takes two parameters that merely share one for the same parameter - the
+                // same mistake EliminateGenerics made, where it dispatched a value through the wrong
+                // instance. Both now ask what the node was copied from.
+                boolean sameVar = e.getKey() == typeVar
+                    || specialisations.canonical(e.getKey()) == specialisations.canonical(typeVar);
                 if (sameVar && !e.getValue().getTypeClassBinding().isEmpty()) {
                     return e.getValue();
                 }

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

@@ -1629,6 +1629,11 @@ private void createSpecializedGlobals(ImClass originalClass, GenericTypes generi
 
             // Create + register global
             translator.addGlobal(specializedGlobal);
+            // Both halves of what the interpreter used to read out of the name: what this was copied
+            // from, and which class it belongs to.
+            translator.recordSpecialisation(specializedGlobal, originalGlobal, generics.getTypeArguments());
+            translator.recordGenericStaticOwner(specializedGlobal, originalClass);
+            translator.recordGenericStaticOwner(originalGlobal, originalClass);
             specializedGlobals.put(originalGlobal, key, specializedGlobal);
             dbg("Created specialized global: " + specializedName + " type=" + specializedType);
 

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

@@ -37,7 +37,7 @@
 import static de.peeeq.wurstscript.translation.imtranslation.FunctionFlagEnum.*;
 import static de.peeeq.wurstscript.utils.Utils.elementNameWithPath;
 
-public class ImTranslator {
+public class ImTranslator implements SpecialisationLookup {
 
 
     public static final String $DEBUG_PRINT = "$debugPrint";
@@ -75,6 +75,25 @@ public void recordSpecialisation(Element copy, Element original) {
         recordSpecialisation(copy, original, List.of());
     }
 
+    /**
+     * The generic class a static field belongs to.
+     * <p>
+     * A static field of a generic class becomes a global named after the class, and the interpreter
+     * used to recover the owner by taking the longest prefix of that name ending at an underscore
+     * which matches a class name. A class whose name contains an underscore, or a field whose name
+     * begins like a class, answers that wrongly and silently. Recorded here instead, where it is
+     * known.
+     */
+    private final Map<ImVar, ImClass> genericStaticOwners = new IdentityHashMap<>();
+
+    public void recordGenericStaticOwner(ImVar global, ImClass owner) {
+        genericStaticOwners.put(global, owner);
+    }
+
+    public @Nullable ImClass genericStaticOwnerOf(ImVar global) {
+        return genericStaticOwners.get(global);
+    }
+
     /** What {@code copy} was made from and for, or null when it is not a copy. */
     public @Nullable Specialisation specialisationOf(Element copy) {
         return specialisations.get(copy);
@@ -87,6 +106,7 @@ public void recordSpecialisation(Element copy, Element original) {
      * construction because a copy is always newer than what it was made from; the bound is there so a
      * mistake elsewhere fails loudly rather than hanging.
      */
+    @Override
     @SuppressWarnings("unchecked")
     public <T extends Element> T canonical(T copy) {
         Element current = copy;

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

@@ -0,0 +1,45 @@
+package de.peeeq.wurstscript.translation.imtranslation;
+
+import de.peeeq.wurstscript.jassIm.Element;
+import de.peeeq.wurstscript.jassIm.ImClass;
+import de.peeeq.wurstscript.jassIm.ImVar;
+import org.eclipse.jdt.annotation.Nullable;
+
+/**
+ * Answers what a specialised node was copied from.
+ * <p>
+ * The interpreter is handed a program rather than the translation which produced it, which is why it
+ * had no way to tell two type variables apart except by name - and two parameters which merely share
+ * a name are not the same parameter. This is the one question it needs answered, narrow enough to hand
+ * over without handing over the translator.
+ */
+public interface SpecialisationLookup {
+
+    /** The node {@code node} was ultimately copied from, or {@code node} itself. */
+    <T extends Element> T canonical(T node);
+
+    /**
+     * The generic class a static field belongs to, or null when it is not one.
+     * <p>
+     * The alternative was reading the owner out of the global's name, which a class name containing
+     * an underscore answers wrongly and without saying so.
+     */
+    @Nullable ImClass genericStaticOwnerOf(ImVar global);
+
+    /**
+     * For a program which did not come from a translation that recorded anything - a hand-built
+     * program in a test, say. Every node is its own original, which is what a program with no
+     * specialisation in it means.
+     */
+    SpecialisationLookup NONE = new SpecialisationLookup() {
+        @Override
+        public <T extends Element> T canonical(T node) {
+            return node;
+        }
+
+        @Override
+        public @Nullable ImClass genericStaticOwnerOf(ImVar global) {
+            return null;
+        }
+    };
+}

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

@@ -1,13 +1,19 @@
 package tests.wurstscript.tests;
 
+import de.peeeq.wurstscript.intermediatelang.interpreter.ProgramState;
+import de.peeeq.wurstscript.jassIm.ImFunction;
 import de.peeeq.wurstscript.jassIm.ImTypeArgument;
+import de.peeeq.wurstscript.jassIm.ImTypeClassFunc;
+import de.peeeq.wurstscript.jassIm.ImTypeVar;
 import de.peeeq.wurstscript.jassIm.ImVar;
 import de.peeeq.wurstscript.jassIm.JassIm;
 import de.peeeq.wurstscript.translation.imtranslation.ImTranslator;
 import org.testng.annotations.Test;
 
 import java.util.Collections;
+import java.util.LinkedHashMap;
 import java.util.List;
+import java.util.Map;
 
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertNull;
@@ -111,4 +117,59 @@ public void acycleIsReportedRatherThanFollowedForever() {
 
         assertThrows(IllegalStateException.class, () -> translator.canonical(a));
     }
+
+    private static ImTypeVar typeVar(String name) {
+        return JassIm.ImTypeVar(name);
+    }
+
+    /**
+     * A type argument carrying a binding, since the lookup only returns arguments which have one -
+     * an argument with an empty binding is one nothing was dispatched through.
+     */
+    private static ImTypeArgument boundArgument(String instanceName) {
+        ImFunction instance = JassIm.ImFunction(de.peeeq.wurstscript.ast.Ast.NoExpr(), instanceName,
+            JassIm.ImTypeVars(), JassIm.ImVars(), JassIm.ImVoid(), JassIm.ImVars(), JassIm.ImStmts(),
+            new java.util.ArrayList<>());
+        ImTypeClassFunc requirement = JassIm.ImTypeClassFunc(de.peeeq.wurstscript.ast.Ast.NoExpr(),
+            "show", JassIm.ImTypeVars(), JassIm.ImVars(), JassIm.ImVoid());
+        Map<ImTypeClassFunc, io.vavr.control.Either<de.peeeq.wurstscript.jassIm.ImMethod, ImFunction>> binding =
+            new LinkedHashMap<>();
+        binding.put(requirement, io.vavr.control.Either.right(instance));
+        return JassIm.ImTypeArgument(JassIm.ImSimpleType("integer"), binding);
+    }
+
+    /**
+     * The interpreter picks a binding for a type variable, and two unrelated parameters may share a
+     * name. Comparing names returns whichever the frame happens to hold, which is how a value gets
+     * dispatched through the wrong instance; asking what each was copied from does not.
+     * <p>
+     * Fails without the change: the frame below holds two parameters called T, and the copy being
+     * looked up belongs to only one of them.
+     */
+    @Test
+    public void aBindingIsFoundByOriginRatherThanByName() {
+        ImTranslator translator = translator();
+        ImTypeVar unrelated = typeVar("T");
+        ImTypeVar original = typeVar("T");
+        ImTypeVar copy = typeVar("T");
+        translator.recordSpecialisation(copy, original);
+
+        ImTypeArgument wrong = boundArgument("unrelated_show");
+        ImTypeArgument right = boundArgument("original_show");
+
+        ProgramState state = new ProgramState(new de.peeeq.wurstscript.gui.WurstGuiLogger(),
+            JassIm.ImProg(de.peeeq.wurstscript.ast.Ast.NoExpr(), JassIm.ImVars(), JassIm.ImFunctions(),
+                JassIm.ImMethods(), JassIm.ImClasses(), JassIm.ImTypeClassFuncs(), new LinkedHashMap<>()),
+            true);
+        state.setSpecialisations(translator);
+
+        // The unrelated parameter is first, so a name comparison reaches it before the right one.
+        Map<ImTypeVar, ImTypeArgument> frame = new LinkedHashMap<>();
+        frame.put(unrelated, wrong);
+        frame.put(original, right);
+        state.pushTypeArguments(frame);
+
+        assertSame(state.getCurrentTypeArgument(copy), right,
+            "the binding of the parameter this copy came from, not of one which shares its name");
+    }
 }