Decide slot ambiguity by the dispatch family, not by a declared name

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<ImMethod> allMethods
         Map<ImClass, Set<ImClass>> closureFamilyAnchorsCache = new HashMap<>();
         Map<ImClass, List<ImClass>> closureFamilyClassesByAnchor = new HashMap<>();
 
-        Set<String> ambiguousDirectAliases = ambiguousDirectAliases(allMethods);
-
         for (ImMethod method : allMethods) {
             TreeSet<String> 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<String> usedNames) {
      * arbitrarily" is worse than a name meaning nothing. {@code LuaTranslator} skips composing the
      * matching slot for the same reason.
      */
-    private static Set<String> ambiguousDirectAliases(List<ImMethod> allMethods) {
-        Map<String, String> claimedBy = new LinkedHashMap<>();
-        Set<String> 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<String> ambiguousDirectAliases(List<ImMethod> allMethods) {
         return owner.getName() + "_" + semanticName;
     }
 
-    private static void addDirectAliases(ImMethod method, Set<String> aliases,
-                                         Set<String> ambiguousDirectAliases) {
+    private static void addDirectAliases(ImMethod method, Set<String> aliases) {
         if (method == null) {
             return;
         }
@@ -208,8 +181,13 @@ private static void addDirectAliases(ImMethod method, Set<String> 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<String> 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.
+     * <p>
+     * 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.
+     * <p>
+     * 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) {

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.
     }
 
 

de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaTranslator.java

@@ -1020,8 +1020,18 @@ private Set<String> collectDispatchSlotNames(ImClass receiverClass, List<ImMetho
                     slotNames.add(dispatchSlotName(alias));
                 }
             }
+            // Only the method's own declared name. A specialised method's trailing segment is the
+            // type argument, which names no method, so composing a slot from it hands one method's
+            // implementation a name that belongs to nobody - and to the wrong method if some other
+            // method happens to be declared with that word.
             String semanticName = semanticNameFromMethodName(m.getName());
-            if (!semanticName.isEmpty()) {
+            // The same rule as the one composing the aliases: the declared name, or that name with the
+            // number the translation uses to tell overloads apart. An override of a numbered overload
+            // has to be able to replace the slot its ancestor composed.
+            String declared = LuaDispatchPreparation.declaredName(m);
+            if (!semanticName.isEmpty() && (semanticName.equals(declared)
+                    || (!declared.isEmpty() && semanticName.startsWith(declared)
+                        && LuaDispatchPreparation.isOverloadNumber(semanticName.substring(declared.length()))))) {
                 semanticNames.add(semanticName);
             }
             String sourceSemanticName = sourceSemanticName(m);
@@ -1035,66 +1045,17 @@ private Set<String> collectDispatchSlotNames(ImClass receiverClass, List<ImMetho
             // class every method's trailing segment is the type argument, which is exactly that
             // case, and the resulting slot is never called. Left uncomposed rather than bound
             // arbitrarily; LuaDispatchPreparation drops the matching alias for the same reason.
-            Set<String> ambiguous = ambiguousSemanticNames(receiverClass);
             Set<String> 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));
                 }
             }
         }
         return slotNames;
     }
 
-    /**
-     * The semantic names which name no method in particular, cached per class.
-     * <p>
-     * 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.
-     * <p>
-     * 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.
-     * <p>
-     * 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<ImClass, Set<String>> ambiguousSemanticNamesByClass = new LinkedHashMap<>();
-
-    private Set<String> ambiguousSemanticNames(ImClass c) {
-        return ambiguousSemanticNamesByClass.computeIfAbsent(c, owner -> {
-            Map<String, Set<String>> 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<String> ambiguous = new TreeSet<>();
-            claimants.forEach((name, keys) -> {
-                if (keys.size() > 1) {
-                    ambiguous.add(name);
-                }
-            });
-            return ambiguous;
-        });
-    }
-
     private void collectClassNamesInHierarchy(ImClass c, Set<String> out, Set<ImClass> visited) {
         if (c == null || !visited.add(c)) {
             return;

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.
+     * <p>
+     * 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<String> 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);
     }

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<T>",
+            "    function route(T t) returns int",
+            "        return 1",
+            "    function route(T t, int extra) returns int",
+            "        return 2",
+            "class Child extends Base<int>",
+            "    override function route(int t) returns int",
+            "        return 10",
+            "    override function route(int t, int extra) returns int",
+            "        return 20",
+            "init",
+            "    Base<int> 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.
+     * <p>
+     * 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.
+     * <p>
+     * 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>",
+            "    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<route1>(new route1())",
+            "    if h.route() == 1 and h.route1() == 2",
+            "        testSuccess()"
+        );
+    }
+
     @Test
     public void luaOutputIsDeterministicForGenericOverrideSlots() throws IOException {
         test().testLua(true).compilationUnits(genericOverrideReproUnits());