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