Fix duplicate requirements across type class bounds

de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/EliminateGenerics.java

@@ -128,6 +128,41 @@ public void transformGenericNewOnly() {
         }
         eliminateRemainingGenericNewCalls();
         assertNoReachableGenericNewMarkers();
+        settleRemainingDispatches();
+    }
+
+    /**
+     * Neutralises the dispatches left in functions that were specialized.
+     * <p>
+     * Such a function is dead: every reachable call to it was rewritten to its specialization, so a
+     * dispatch still sitting in the original can never run. This backend keeps generics rather than
+     * removing them wholesale, so those originals are still translated and the dispatch would reach
+     * a backend with no way to express it.
+     * <p>
+     * Anything else is left alone. A dispatch may legitimately remain in a bounded generic that is
+     * merely declared and never called, and garbage removal deletes those later; deciding here
+     * would mean duplicating reachability. One which survives that far and still reaches the
+     * backend is reported there, where it is known to be both reachable and unresolvable.
+     */
+    private void settleRemainingDispatches() {
+        List<ImTypeVarDispatch> remaining = new ArrayList<>();
+        prog.accept(new Element.DefaultVisitor() {
+            @Override
+            public void visit(ImTypeVarDispatch dispatch) {
+                super.visit(dispatch);
+                remaining.add(dispatch);
+            }
+        });
+        for (ImTypeVarDispatch dispatch : remaining) {
+            ImFunction owner = dispatch.getNearestFunc();
+            if (owner != null && specializedFunctions.containsRow(owner)) {
+                dispatch.replaceBy(defaultValueFor(dispatch.getTypeClassFunc().getReturnType()));
+            }
+        }
+    }
+
+    private static ImExpr defaultValueFor(ImType type) {
+        return JassIm.ImNull(type.copy());
     }
 
 
@@ -271,7 +306,7 @@ public void visit(ImAlloc alloc) {
                 // Constructing a class whose methods dispatch has to be specialised as well:
                 // otherwise the constructor keeps a generic result type, and a method call on that
                 // result never becomes concrete enough to resolve.
-                if (classNeedsSpecialization(alloc.getClazz().getClassDef(), visitedFunctions, visitedMethods)) {
+                if (classNeedsSpecialization(alloc.getClazz().getClassDef())) {
                     found[0] = true;
                     return;
                 }
@@ -320,44 +355,59 @@ && functionNeedsSpecialization(method.getImplementation(), visitedFunctions, vis
     /**
      * Whether constructing this class requires the concrete type argument, because one of its own
      * or inherited members dispatches on a type class bound.
+     * <p>
+     * Deliberately a property of the class alone, not of the path that asked. An earlier version
+     * threaded the caller's visited set through here and memoised the answer, so a query made while
+     * one of the class's own functions was already being visited recorded a negative result that
+     * then stood for every later query.
      */
-    private boolean classNeedsSpecialization(ImClass classDef, Set<ImFunction> visitedFunctions,
-                                             Set<ImMethod> visitedMethods) {
-        Boolean cached = classNeedsSpecialization.get(classDef);
+    private boolean classNeedsSpecialization(ImClass classDef) {
+        Boolean cached = classNeedsSpecializationCache.get(classDef);
         if (cached != null) {
-            // Already answered, or currently being answered: a class reached through its own
-            // members contributes nothing new to the decision.
             return cached;
         }
-        classNeedsSpecialization.put(classDef, false);
-        boolean result = false;
+        classNeedsSpecializationCache.put(classDef, false);
+        boolean result = classDispatchesOnBound(classDef,
+            Collections.newSetFromMap(new IdentityHashMap<>()));
+        classNeedsSpecializationCache.put(classDef, result);
+        return result;
+    }
+
+    private boolean classDispatchesOnBound(ImClass classDef, Set<ImClass> visited) {
+        if (!visited.add(classDef)) {
+            return false;
+        }
         for (ImFunction f : classDef.getFunctions()) {
-            if (functionNeedsSpecialization(f, visitedFunctions, visitedMethods)) {
-                result = true;
-                break;
+            if (containsDispatch(f)) {
+                return true;
             }
         }
-        if (!result) {
-            for (ImMethod m : classDef.getMethods()) {
-                if (methodNeedsSpecialization(m, visitedFunctions, visitedMethods)) {
-                    result = true;
-                    break;
-                }
+        for (ImMethod m : classDef.getMethods()) {
+            if (m.getImplementation() != null && containsDispatch(m.getImplementation())) {
+                return true;
             }
         }
-        if (!result) {
-            for (ImClassType superType : classDef.getSuperClasses()) {
-                if (classNeedsSpecialization(superType.getClassDef(), visitedFunctions, visitedMethods)) {
-                    result = true;
-                    break;
-                }
+        for (ImClassType superType : classDef.getSuperClasses()) {
+            if (classDispatchesOnBound(superType.getClassDef(), visited)) {
+                return true;
             }
         }
-        classNeedsSpecialization.put(classDef, result);
-        return result;
+        return false;
+    }
+
+    /** Whether this function body dispatches on a bound, without following calls out of it. */
+    private static boolean containsDispatch(ImFunction f) {
+        boolean[] found = {false};
+        f.accept(new Element.DefaultVisitor() {
+            @Override
+            public void visit(ImTypeVarDispatch dispatch) {
+                found[0] = true;
+            }
+        });
+        return found[0];
     }
 
-    private final Map<ImClass, Boolean> classNeedsSpecialization = new IdentityHashMap<>();
+    private final Map<ImClass, Boolean> classNeedsSpecializationCache = new IdentityHashMap<>();
 
     private void assertNoReachableGenericNewMarkers() {
         prog.accept(new Element.DefaultVisitor() {

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

@@ -1,6 +1,7 @@
 package de.peeeq.wurstscript.translation.lua.translation;
 
 import de.peeeq.wurstscript.WurstOperator;
+import de.peeeq.wurstscript.attributes.CompileError;
 import de.peeeq.wurstscript.jassIm.*;
 import de.peeeq.wurstscript.luaAst.*;
 import de.peeeq.wurstscript.translation.imtranslation.ImTranslator;
@@ -461,7 +462,12 @@ public static LuaExpr translate(ImCompiletimeExpr imCompiletimeExpr, LuaTranslat
     }
 
     public static LuaExpr translate(ImTypeVarDispatch imTypeVarDispatch, LuaTranslator tr) {
-        throw new Error("not implemented");
+        // Reaching the backend means specialization never supplied a concrete type for this
+        // dispatch and the code is reachable, since unreachable functions have been removed by now.
+        throw new CompileError(imTypeVarDispatch.attrTrace().attrSource(),
+            "Type class dispatch of " + imTypeVarDispatch.getTypeClassFunc().getName()
+                + " could not be resolved for the Lua target: the concrete type is not available"
+                + " where it is used.");
     }
 
     public static LuaExpr translate(ImCast imCast, LuaTranslator tr) {

de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/types/WurstTypeTypeParam.java

@@ -13,6 +13,7 @@
 import io.vavr.control.Option;
 import org.eclipse.jdt.annotation.Nullable;
 
+import java.util.ArrayList;
 import java.util.List;
 import java.util.stream.Stream;
 
@@ -98,13 +99,45 @@ public void addMemberMethods(Element node, String name, List<FuncLink> result) {
         if (!staticRef) {
             return;
         }
+        // Bounds are ordered and an earlier one wins, but only over the same signature: two bounds
+        // may require the very same operation, and offering both would make every call ambiguous.
+        // Differently shaped overloads are not in competition, so later bounds still contribute
+        // them and overload resolution picks between them as usual.
+        List<FuncLink> supplied = new ArrayList<>();
         for (InterfaceDef bound : TypeClassConstraints.boundInterfaces(def)) {
             for (FuncDef method : bound.getMethods()) {
-                if (method.getName().equals(name)) {
-                    result.add(requirementLink(node, bound, method));
+                if (!method.getName().equals(name)) {
+                    continue;
                 }
+                FuncLink candidate = requirementLink(node, bound, method);
+                if (!alreadySupplied(supplied, candidate, node)) {
+                    supplied.add(candidate);
+                }
+            }
+        }
+        result.addAll(supplied);
+    }
+
+    /** True when an earlier bound already supplied a requirement of the same shape. */
+    private static boolean alreadySupplied(List<FuncLink> supplied, FuncLink candidate, Element node) {
+        for (FuncLink existing : supplied) {
+            List<WurstType> a = existing.getParameterTypes();
+            List<WurstType> b = candidate.getParameterTypes();
+            if (a.size() != b.size()) {
+                continue;
+            }
+            boolean same = true;
+            for (int i = 0; i < a.size(); i++) {
+                if (!a.get(i).equalsType(b.get(i), node)) {
+                    same = false;
+                    break;
+                }
+            }
+            if (same) {
+                return true;
             }
         }
+        return false;
     }
 
     @Override

de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java

@@ -847,6 +847,124 @@ public void sameSimpleNameThroughRegistryFallback() {
         );
     }
 
+    /**
+     * Two bounds may require the same operation. Bounds are ordered and the earlier one wins,
+     * rather than every call to the shared operation becoming ambiguous.
+     */
+    @Test
+    public void duplicateRequirementAcrossBounds() {
+        testAssertOkLines(true,
+            "package test",
+            "native testSuccess()",
+            "interface First<T:>",
+            "    function show(T x) returns string",
+            "interface Second<T:>",
+            "    function other(T x) returns int",
+            "    function show(T x) returns string",
+            "implements First<int>",
+            "    function show(int x) returns string",
+            "        return \"first\"",
+            "implements Second<int>",
+            "    function other(int x) returns int",
+            "        return 1",
+            "    function show(int x) returns string",
+            "        return \"second\"",
+            "function render<Q: First and Second>(Q x) returns string",
+            "    return Q.show(x)",
+            "init",
+            "    if render(1) == \"first\"",
+            "        testSuccess()"
+        );
+    }
+
+    /**
+     * Bounds only shadow each other when they require the same shape. A later bound still supplies
+     * a differently shaped overload, which overload resolution then chooses between.
+     */
+    @Test
+    public void overloadFromLaterBoundStaysAvailable() {
+        testAssertOkLines(true,
+            "package test",
+            "native testSuccess()",
+            "interface First<T:>",
+            "    function show(T x) returns string",
+            "interface Second<T:>",
+            "    function show(T x) returns string",
+            "    function show(int scale, T x) returns string",
+            "implements First<int>",
+            "    function show(int x) returns string",
+            "        return \"first\"",
+            "implements Second<int>",
+            "    function show(int x) returns string",
+            "        return \"second\"",
+            "    function show(int scale, int x) returns string",
+            "        return \"scaled\"",
+            "function render<Q: First and Second>(Q x) returns string",
+            "    return Q.show(x) + Q.show(2, x)",
+            "init",
+            "    if render(1) == \"firstscaled\"",
+            "        testSuccess()"
+        );
+    }
+
+    /**
+     * A bounded generic which is only declared, never called, stays valid: it is unreachable, so
+     * nothing has to supply a concrete type for it.
+     */
+    @Test
+    public void unusedBoundedGenericFunctionLua() {
+        test().testLua(true).executeProg().lines(
+            "package test",
+            "native testSuccess()",
+            "interface Show<T:>",
+            "    function show(T x) returns string",
+            "implements Show<int>",
+            "    function show(int x) returns string",
+            "        return \"i\"",
+            "function unused<Q: Show>(Q x) returns string",
+            "    return Q.show(x)",
+            "init",
+            "    testSuccess()"
+        );
+    }
+
+    /** The same for a bounded generic class which is never constructed. */
+    @Test
+    public void unusedBoundedGenericClassLua() {
+        test().testLua(true).executeProg().lines(
+            "package test",
+            "native testSuccess()",
+            "interface Show<T:>",
+            "    function show(T x) returns string",
+            "implements Show<int>",
+            "    function show(int x) returns string",
+            "        return \"i\"",
+            "class Unused<Q: Show>",
+            "    function render(Q x) returns string",
+            "        return Q.show(x)",
+            "init",
+            "    testSuccess()"
+        );
+    }
+
+    /** Declared and unused on Jass too, which is where it already worked. */
+    @Test
+    public void unusedBoundedGenericFunction() {
+        testAssertOkLines(true,
+            "package test",
+            "native testSuccess()",
+            "interface Show<T:>",
+            "    function show(T x) returns string",
+            "implements Show<int>",
+            "    function show(int x) returns string",
+            "        return \"i\"",
+            "function unused<Q: Show>(Q x) returns string",
+            "    return Q.show(x)",
+            "init",
+            "    testSuccess()"
+        );
+    }
+
     /** A type parameter is not a value, so it may only appear as the receiver of a requirement. */
     @Test
     public void typeParameterIsNotAValue() {