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 4891947a2..1be77ae3f 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 @@ -20,6 +20,8 @@ import java.util.HashSet; import org.eclipse.jdt.annotation.Nullable; +import java.util.Collections; +import java.util.IdentityHashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -116,11 +118,9 @@ 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, ambiguousDirectAliases); + addDirectAliases(method, aliases); addHierarchyAliases(method, aliases, sortedMethodsByClass); addClosureFamilyAliases(prog, method, aliases, sortedMethodsByClass, closureFamilyAnchorsCache, closureFamilyClassesByAnchor); method.setLuaMethodDispatchAliases(new ArrayList<>(aliases)); @@ -160,32 +160,6 @@ private static String uniqueName(String name, Set usedNames) { * 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; @@ -198,8 +172,7 @@ private static Set ambiguousDirectAliases(List allMethods) { return owner.getName() + "_" + semanticName; } - private static void addDirectAliases(ImMethod method, Set aliases, - Set ambiguousDirectAliases) { + private static void addDirectAliases(ImMethod method, Set aliases) { if (method == null) { return; } @@ -208,8 +181,13 @@ private static void addDirectAliases(ImMethod method, Set aliases, aliases.add(methodName); } ImClass owner = method.attrClass(); + // A method composes the class-qualified name only when the segment it would be built from is + // this method's own declared name. For a specialised method that segment is the type argument + // instead, which names no method - and one method happening to be declared with the same word + // as the type argument does not entitle the others to the slot it owns. String composed = directAliasFor(method); - if (composed != null && !ambiguousDirectAliases.contains(composed)) { + if (composed != null + && namesItsDeclaredMethod(method)) { aliases.add(composed); } String sourceSemanticName = sourceSemanticName(method); @@ -327,6 +305,37 @@ private static boolean sharesSemanticName(ImMethod method, Set semanticN } /** The name the method was written with, or empty when there is no declaration to ask. */ + /** + * Whether the name recovered from this method's mangled name is the name it was declared with. + *

+ * The recovered name is the segment after the last underscore, and a method the translation + * numbered to keep it apart from its overloads carries that number in the segment: the second + * {@code route} of a class is mangled to {@code Base_route1}, so the segment is {@code route1} + * where the declaration says {@code route}. Requiring the two to be equal refuses the alias for + * every overload past the first, which loses the class-qualified slot an override of it needs to + * replace - a call through the base then stays bound to the base implementation. + *

+ * The number is the translation's own and means the same method, so it is allowed. Anything else + * between the two names is not: that is the case this check exists for, where the segment is a type + * argument rather than a method and the slot composed from it names nothing. + */ + private static boolean namesItsDeclaredMethod(ImMethod method) { + String recovered = semanticNameFromMethodName(method.getName()); + String declared = declaredName(method); + if (recovered.equals(declared)) { + return true; + } + if (declared.isEmpty() || !recovered.startsWith(declared)) { + return false; + } + return isOverloadNumber(recovered.substring(declared.length())); + } + + /** The suffix the translation appends to tell overloads of one name apart. */ + public static boolean isOverloadNumber(String suffix) { + return !suffix.isEmpty() && suffix.chars().allMatch(Character::isDigit); + } + /** 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) { diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/OverrideUtils.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/OverrideUtils.java index 07aa1a4f4..4f43e38e0 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/OverrideUtils.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/OverrideUtils.java @@ -122,6 +122,11 @@ public static void addOverride( ImMethod wrapperMethod = JassIm.ImMethod(e, subMethod.getMethodClass(), subMethod.getName() + "_wrapper", implementation, JassIm.ImMethods(), new ArrayList<>(), "", false); subClass.getMethods().add(wrapperMethod); superMethodIm.getSubMethods().add(wrapperMethod); + // Deliberately not linking wrapperMethod to subMethod as a submethod, though the wrapper does + // call it. Doing so makes the wrapper's dispatch reach the override directly, without the + // conversion the wrapper exists to perform, and fails the implicit conversion and generic + // overload tests. The relation is real but this is not the way to state it; the family below + // is derived without needing it. } 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 05915f337..081eca069 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 @@ -1020,8 +1020,18 @@ private Set collectDispatchSlotNames(ImClass receiverClass, List 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)); } } @@ -1050,51 +1056,6 @@ 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 074d47b85..a61e603de 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 @@ -516,6 +516,55 @@ public void everySlotOnASpecialisedClassNamesAMethod() throws IOException { } } + /** + * Overloads of one method inside a specialised class, which nothing else covers. + *

+ * It passes under the previous identity as well, so it does not pin the dispatch family: the two + * overloads compose different names rather than colliding, which means the residual backlog item + * 15 described is narrower than it claimed, or not reachable this way. Kept for the shape. + */ + @Test + public void overloadsInASpecialisedClassLeaveNoDeadSlot() throws IOException { + String[] withOverload = program(fastHashMap( + " function get(K key, V fallback) returns V", + " let s = slotFor(key)", + " if s < base or not used[s]", + " return fallback", + " return values[s]" + ), INT_INSTANCE, USE_WITH_COLLISION); + + test().testLua(true).executeProg().lines(withOverload); + assertSpecialisedClassesAllocateTheirFields( + compiledLua("overloadsInASpecialisedClassLeaveNoDeadSlot")); + + assertEverySlotNamesAMethod(compiledLua("overloadsInASpecialisedClassLeaveNoDeadSlot")); + } + + /** Every slot assigned on a specialised class table carries one of the container's method names. */ + private static void assertEverySlotNamesAMethod(String lua) { + 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; + } + 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); } diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaTranslationTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaTranslationTests.java index c64ac4144..325f22dab 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaTranslationTests.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaTranslationTests.java @@ -1408,6 +1408,70 @@ public void genericOverrideChainBindsRootSlotToMostSpecificImplInLua() throws IO } } + @Test + public void overloadedOverrideOnAGenericBaseIsReachedThroughTheBase() { + test().testLua(true).executeProg().lines( + "package test", + "native testSuccess()", + "class Base", + " function route(T t) returns int", + " return 1", + " function route(T t, int extra) returns int", + " return 2", + "class Child extends Base", + " override function route(int t) returns int", + " return 10", + " override function route(int t, int extra) returns int", + " return 20", + "init", + " Base b = new Child()", + " if b.route(1) == 10 and b.route(1, 2) == 20", + " testSuccess()" + ); + } + + /** + * A type argument whose name is a method's name followed by a number, which is what the rule + * allowing an overload number could in principle be fooled by. + *

+ * It holds, because the type argument is part of the owning class's name rather than the tail of the + * method's: the segment a slot name is composed from is {@code route} for {@code route} and + * {@code route1} for {@code route1}, and neither needs the number tolerated. The case is kept + * because it was raised against that rule and reasoning about which segment carries the type is + * exactly the kind of thing to check rather than argue about. + */ + /** + * A type argument whose name is a method's name followed by a number, which is what the rule + * allowing an overload number could in principle be fooled by. + *

+ * It holds, because the type argument is part of the owning class's name rather than the tail of the + * method's: the segment a slot name is composed from is {@code route} for {@code route} and + * {@code route1} for {@code route1}, and neither needs the number tolerated. The case is kept + * because it was raised against that rule, and which segment carries the type argument is exactly + * the kind of thing to check rather than argue about. + */ + @Test + public void aTypeNamedLikeAnOverloadNumberDoesNotStealTheSlot() { + test().testLua(true).executeProg().lines( + "package test", + "native testSuccess()", + "class route1", + " int v = 3", + "class Holder", + " T item", + " construct(T item)", + " this.item = item", + " function route() returns int", + " return 1", + " function route1() returns int", + " return 2", + "init", + " let h = new Holder(new route1())", + " if h.route() == 1 and h.route1() == 2", + " testSuccess()" + ); + } + @Test public void luaOutputIsDeterministicForGenericOverrideSlots() throws IOException { test().testLua(true).compilationUnits(genericOverrideReproUnits());