diff --git a/BACKLOG.md b/BACKLOG.md index 240db7375..aa81417ac 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -140,23 +140,27 @@ itself, and one gap in what the suite can see. `simplifyClasses` nor `addMemberTypeArguments`, so the type variables are never lifted there and a super call has nothing to carry. Closing it is item 23. -15. **One junk dispatch slot per specialised class.** `addDirectAliases` and - `LuaTranslator.collectDispatchSlotNames` both compose `owner.getName() + "_" + - semanticNameFromMethodName(name)`, and for a specialised method that trailing segment is the - type argument — so every method of `FastHashMap` claims one shared - `FastHashMap_specialized_integer__integer` slot and the alphabetically first wins it. - Nothing calls it, so it is dead weight rather than a wrong result. - - Tried using the declared name instead and reverted it: overloads share a declared name, so - `setup(int)` and `setup(string)` collapse into one slot, which is what - `LuaTranslationTests.overloadedMethodsDoNotAliasInLuaDispatchTables` and - `moduleProvidedOverloadedOverrideDoesNotCollapseLuaSlots` exist to prevent. Both sources of a - semantic name are wrong, in opposite directions: the mangled trailing segment collides across - the siblings of one specialisation, the declared name collides across overloads. A fix needs a - name separating both — the declared name together with the dispatch signature key would, since - that is already what distinguishes overloads elsewhere in the same file. Worth doing only if - this stops being dead weight, because the cost of getting it wrong is a real mis-binding while - the cost of leaving it is one unused table key per specialised class. +15. **A dead dispatch slot survives for overloads inside a specialised class.** What is left of the + junk slot, which is otherwise gone. + + A slot's name is the owner's plus the segment after the last underscore of the method's, and for a + specialised method that segment is the type argument, so every method of one specialisation + composes the same name. Both composers now leave such a name uncomposed, deciding by how many + distinct declared names produce it: a method and its overrides declare one name and must share a + slot, while siblings declare different ones. + + That leaves overloads. Two overloads of one source method share a declared name, so a specialised + class holding only overloads still composes one shared name and binds it to whichever is reached + first. Dead weight as before - nothing calls it - but no longer true of the general case. + + The sharper identity is the dispatch group key, which separates overloads by signature. It cannot + be used here: the signature embeds each class's type variable, so a generic override chain reads as + `void|T192,real` against `void|T636,real` and the overrides look unrelated, which drops the slot + they must share. `LuaTranslationTests.genericOverrideChainBindsRootSlotToMostSpecificImplInLua` + fails exactly that way, and is how this was found rather than shipped. + + A fix needs an identity which treats a chain's differing type variables as the same signature while + still separating real parameter differences. Worth doing only if this stops being dead weight. 24. **`luaOutputIsDeterministicForGenericOverrideSlots` failed once and has not since.** It failed on Windows CI and passed on a re-run of the same commit, having blocked an unrelated pull request diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaDispatchPreparation.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaDispatchPreparation.java index 86b95b25c..4891947a2 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaDispatchPreparation.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaDispatchPreparation.java @@ -18,6 +18,8 @@ import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; +import org.eclipse.jdt.annotation.Nullable; + import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -114,9 +116,11 @@ private static void assignDispatchAliases(ImProg prog, List allMethods Map> closureFamilyAnchorsCache = new HashMap<>(); Map> closureFamilyClassesByAnchor = new HashMap<>(); + Set ambiguousDirectAliases = ambiguousDirectAliases(allMethods); + for (ImMethod method : allMethods) { TreeSet aliases = new TreeSet<>(); - addDirectAliases(method, aliases); + addDirectAliases(method, aliases, ambiguousDirectAliases); addHierarchyAliases(method, aliases, sortedMethodsByClass); addClosureFamilyAliases(prog, method, aliases, sortedMethodsByClass, closureFamilyAnchorsCache, closureFamilyClassesByAnchor); method.setLuaMethodDispatchAliases(new ArrayList<>(aliases)); @@ -146,7 +150,56 @@ private static String uniqueName(String name, Set usedNames) { return result; } - private static void addDirectAliases(ImMethod method, Set aliases) { + /** + * The composed names which more than one method of the same class produces. + *

+ * The name is the owner's plus the segment after the last underscore of the method's. For a + * specialised class that segment is the type argument, so every method of + * {@code FastHashMap} composes the same one, which then names no method in particular. + * Whichever is bound first would claim it, so it is left unbound: a name meaning "one of these, + * arbitrarily" is worse than a name meaning nothing. {@code LuaTranslator} skips composing the + * matching slot for the same reason. + */ + private static Set ambiguousDirectAliases(List allMethods) { + Map claimedBy = new LinkedHashMap<>(); + Set ambiguous = new HashSet<>(); + for (ImMethod method : allMethods) { + String composed = directAliasFor(method); + if (composed == null) { + continue; + } + // A method and its overrides are one dispatchable thing and must share a slot - that is + // what dispatch is - so they are not a collision, and they all declare the same name in + // the source. The siblings of one specialisation declare different ones and merely end up + // composing the same segment, because for them that segment is the type argument. + // + // The dispatch group key would separate overloads too, but it embeds the signature, and a + // generic override chain's signatures differ by each class's type variable - so overrides + // would read as unrelated and lose the slot they must share. Backlog item 15 records what + // that leaves: overloads inside a specialised class keep one dead key. + String identity = declaredName(method); + String previous = claimedBy.put(composed, identity); + if (previous != null && !previous.equals(identity)) { + ambiguous.add(composed); + } + } + return ambiguous; + } + + private static @Nullable String directAliasFor(ImMethod method) { + if (method == null) { + return null; + } + ImClass owner = method.attrClass(); + String semanticName = semanticNameFromMethodName(method.getName()); + if (owner == null || semanticName.isEmpty()) { + return null; + } + return owner.getName() + "_" + semanticName; + } + + private static void addDirectAliases(ImMethod method, Set aliases, + Set ambiguousDirectAliases) { if (method == null) { return; } @@ -155,9 +208,9 @@ private static void addDirectAliases(ImMethod method, Set aliases) { aliases.add(methodName); } ImClass owner = method.attrClass(); - String semanticName = semanticNameFromMethodName(methodName); - if (owner != null && !semanticName.isEmpty()) { - aliases.add(owner.getName() + "_" + semanticName); + String composed = directAliasFor(method); + if (composed != null && !ambiguousDirectAliases.contains(composed)) { + aliases.add(composed); } String sourceSemanticName = sourceSemanticName(method); if (owner != null && isClosureGeneratedClass(owner) && !sourceSemanticName.isEmpty()) { @@ -274,7 +327,8 @@ private static boolean sharesSemanticName(ImMethod method, Set semanticN } /** The name the method was written with, or empty when there is no declaration to ask. */ - private static String declaredName(ImMethod method) { + /** The name a method carries in the source, which a method and its overrides all share. */ + public static String declaredName(ImMethod method) { if (method == null) { return ""; } diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaTranslator.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaTranslator.java index ea86cbfc1..d9f6ba2dd 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaTranslator.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaTranslator.java @@ -8,6 +8,7 @@ import de.peeeq.wurstscript.translation.imtranslation.GetAForB; import de.peeeq.wurstscript.translation.imtranslation.ImHelper; import de.peeeq.wurstscript.translation.imtranslation.ImTranslator; +import de.peeeq.wurstscript.translation.imtranslation.LuaDispatchPreparation; import de.peeeq.wurstscript.translation.imtranslation.LuaNativeLowering; import de.peeeq.wurstscript.types.TypesHelper; import de.peeeq.wurstscript.utils.Lazy; @@ -1029,10 +1030,19 @@ private Set collectDispatchSlotNames(ImClass receiverClass, List ambiguous = ambiguousSemanticNames(receiverClass); Set classNames = new TreeSet<>(); collectClassNamesInHierarchy(receiverClass, classNames, new HashSet<>()); for (String className : classNames) { for (String semanticName : semanticNames) { + if (ambiguous.contains(semanticName)) { + continue; + } slotNames.add(dispatchSlotName(className + "_" + semanticName)); } } @@ -1040,6 +1050,51 @@ private Set collectDispatchSlotNames(ImClass receiverClass, List + * A method and its overrides share a semantic name and must share a slot: that is dispatch, and + * they all declare the same name in the source. The siblings of one specialisation declare + * different names and still compose the same segment, because for a specialised method that + * segment is the type argument - and the slot composed from it is claimed by whichever is bound + * first, then never called. + *

+ * The dispatch group key would be a sharper identity but cannot be used: it embeds the signature, + * and a generic override chain's signatures differ by the type variable of each class in it + * ({@code void|T192,real} against {@code void|T636,real}), so overrides would read as unrelated + * and their shared slot would be dropped. What that leaves uncovered is recorded in backlog + * item 15: overloads of one source method inside a specialised class share a declared name, so + * their composed name is not seen as ambiguous and one dead key survives there. + *

+ * Cached because {@code createMethods} asks twice per dispatch group and each ask would otherwise + * rebuild and sort the whole inherited method list. + */ + private final Map> ambiguousSemanticNamesByClass = new LinkedHashMap<>(); + + private Set ambiguousSemanticNames(ImClass c) { + return ambiguousSemanticNamesByClass.computeIfAbsent(c, owner -> { + Map> claimants = new TreeMap<>(); + for (ImMethod m : collectMethodsInHierarchy(owner)) { + if (m == null) { + continue; + } + String semanticName = semanticNameFromMethodName(m.getName()); + if (semanticName.isEmpty()) { + continue; + } + claimants.computeIfAbsent(semanticName, name -> new TreeSet<>()) + .add(LuaDispatchPreparation.declaredName(m)); + } + Set ambiguous = new TreeSet<>(); + claimants.forEach((name, keys) -> { + if (keys.size() > 1) { + ambiguous.add(name); + } + }); + return ambiguous; + }); + } + private void collectClassNamesInHierarchy(ImClass c, Set out, Set visited) { if (c == null || !visited.add(c)) { return; diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/FastHashMapTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/FastHashMapTests.java index 446610d5b..074d47b85 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/FastHashMapTests.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/FastHashMapTests.java @@ -474,6 +474,48 @@ public void aFieldNamedLikeAMethodKeepsOneKeyAcrossSpecialisation() throws IOExc compiledLua("aFieldNamedLikeAMethodKeepsOneKeyAcrossSpecialisation")); } + /** + * Every dispatch slot on a specialised class names one of its methods. + *

+ * A slot's name is composed from the owner's and the segment after the last underscore of the + * method's, and for a specialised method that segment is the type argument - so every method of + * one specialisation used to compose the same name and the first bound claimed it. Nothing called + * it, which is why it went unnoticed; it is not emitted now, and this says so rather than leaving + * the next reader to wonder what it was. + *

+ * A method and its overrides do share a slot, which is dispatch rather than a collision. They + * share a declared name, which is what tells the two cases apart. + */ + @Test + public void everySlotOnASpecialisedClassNamesAMethod() throws IOException { + test().testLua(true).executeProg().lines(program(fastHashMap(), INT_INSTANCE, USE_WITH_COLLISION)); + String lua = compiledLua("everySlotOnASpecialisedClassNamesAMethod"); + + Matcher table = Pattern.compile("(FastHashMap_specialized\\w*)\\.(\\w+)\\s*=").matcher(lua); + java.util.List unnamed = new java.util.ArrayList<>(); + while (table.find()) { + String slot = table.group(2); + if (slot.startsWith("__")) { + continue; + } + // A real slot carries a method name; the junk one was the class name and the type + // argument with no method name anywhere in it. + boolean namesAMethod = false; + for (String method : METHOD_NAMES) { + if (slot.contains(method)) { + namesAMethod = true; + break; + } + } + if (!namesAMethod) { + unnamed.add(slot); + } + } + if (!unnamed.isEmpty()) { + throw new AssertionError("these slots name no method: " + unnamed + "\n" + lua); + } + } + private String compiledJass(String testName) throws IOException { return Files.toString(new File(TEST_OUTPUT_PATH, "FastHashMapTests_" + testName + ".j"), Charsets.UTF_8); }