From 97513fe3252d3d2f4826b71adfb5dd9b931a3c5e Mon Sep 17 00:00:00 2001 From: Frotty Date: Mon, 17 Aug 2026 13:49:31 +0200 Subject: [PATCH 1/2] Let the interpreter ask what a node was copied from, and who owns it Two name-based answers in ProgramState, both replaced by recorded ones. getCurrentTypeArgument matched type variables by name, which takes two parameters that merely share one for the same parameter - the mistake EliminateGenerics made, where it dispatched a value through the wrong instance and killed the interpreter. It was the same bug waiting in the compiletime path. identifyGenericStaticGlobals found a static field's owning class by taking the longest prefix of the global's name ending at an underscore which names a generic class. A class whose name contains an underscore answers that wrongly, and silently. The owner is now recorded where the specialised global is created. Both reach the interpreter through SpecialisationLookup: one narrow interface, handed over instead of the translator, since the interpreter is given a program rather than the translation which produced it. Without one, every node is its own original and the name search stands in - which is what a hand-built program in a test means, and it still answers those correctly. The owners are worked out in the constructor, before the lookup is supplied, so supplying it asks again rather than leaving the name-derived answers in place. Behaviour is unchanged where the two agreed, which is everywhere the relation exists; what changes is that they can no longer agree by accident. --- .../wurstio/CompiletimeFunctionRunner.java | 3 ++ .../interpreter/ProgramState.java | 45 +++++++++++++------ .../imtranslation/EliminateGenerics.java | 5 +++ .../imtranslation/ImTranslator.java | 22 ++++++++- .../imtranslation/SpecialisationLookup.java | 45 +++++++++++++++++++ 5 files changed, 106 insertions(+), 14 deletions(-) create mode 100644 de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/SpecialisationLookup.java diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/CompiletimeFunctionRunner.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/CompiletimeFunctionRunner.java index 6ef67034b..b27e6189b 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/CompiletimeFunctionRunner.java +++ b/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); 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 7906cee26..f5a0bf4f2 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 @@ -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 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() { @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 frame : typeArgumentFrames) { 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()); + // 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(); } 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 b4219a14b..1e0bbdc0e 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 @@ -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); 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 4a5576d5e..455aedeb5 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 @@ -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. + *

+ * 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 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 canonical(T copy) { Element current = copy; diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/SpecialisationLookup.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/SpecialisationLookup.java new file mode 100644 index 000000000..2148bfa5c --- /dev/null +++ b/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. + *

+ * 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 canonical(T node); + + /** + * The generic class a static field belongs to, or null when it is not one. + *

+ * 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 canonical(T node) { + return node; + } + + @Override + public @Nullable ImClass genericStaticOwnerOf(ImVar global) { + return null; + } + }; +} From a5cb32fd0c712ad9fc722c87092bffafd4fd4561 Mon Sep 17 00:00:00 2001 From: Frotty Date: Mon, 17 Aug 2026 13:55:36 +0200 Subject: [PATCH 2/2] Pin the interpreter lookup with a case the name comparison gets wrong The change altered which binding is selected and no test moved, so nothing demonstrated it. aBindingIsFoundByOriginRatherThanByName does: a frame holding two parameters both called T with different bindings, and a copy which came from only one of them. With the name comparison restored it returns Right(unrelated_show) where Right(original_show) is correct - the wrong instance, which is the shape that killed the interpreter in the pass. With the relation it returns the binding of the parameter the copy was made from. --- .../tests/SpecialisationOriginTest.java | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/SpecialisationOriginTest.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/SpecialisationOriginTest.java index 6887aa2a3..8bd6590ff 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/SpecialisationOriginTest.java +++ b/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> 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. + *

+ * 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 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"); + } }