Stop composing a dispatch slot which names no method

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<int, int>` 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

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<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);
+ 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<String> usedNames) {
  return result;
  }
 
- private static void addDirectAliases(ImMethod method, Set<String> aliases) {
+ /**
+ * The composed names which more than one method of the same class produces.
+ * <p>
+ * 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<int, int>} 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<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;
+ }
+ 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<String> aliases,
+ Set<String> ambiguousDirectAliases) {
  if (method == null) {
  return;
  }
@@ -155,9 +208,9 @@ private static void addDirectAliases(ImMethod method, Set<String> 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<String> 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 "";
  }

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,17 +1030,71 @@ private Set<String> collectDispatchSlotNames(ImClass receiverClass, List<ImMetho
  }
  }
  if (receiverClass != null && !semanticNames.isEmpty()) {
+ // A semantic name which several of the class's methods share names none of them, so a
+ // slot composed from it would be claimed by whichever is bound first. For a specialised
+ // 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

@@ -474,6 +474,48 @@ public void aFieldNamedLikeAMethodKeepsOneKeyAcrossSpecialisation() throws IOExc
  compiledLua("aFieldNamedLikeAMethodKeepsOneKeyAcrossSpecialisation"));
  }
 
+ /**
+ * Every dispatch slot on a specialised class names one of its methods.
+ * <p>
+ * 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.
+ * <p>
+ * 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<String> 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);
  }