Skip to content

Commit 0d2bb0f

Browse files
FrottyFrotty
authored
Fix duplicate requirements across type class bounds (#1228)
1 parent 82a298b commit 0d2bb0f

4 files changed

Lines changed: 236 additions & 29 deletions

File tree

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

Lines changed: 76 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,41 @@ public void transformGenericNewOnly() {
128128
}
129129
eliminateRemainingGenericNewCalls();
130130
assertNoReachableGenericNewMarkers();
131+
settleRemainingDispatches();
132+
}
133+
134+
/**
135+
* Neutralises the dispatches left in functions that were specialized.
136+
* <p>
137+
* Such a function is dead: every reachable call to it was rewritten to its specialization, so a
138+
* dispatch still sitting in the original can never run. This backend keeps generics rather than
139+
* removing them wholesale, so those originals are still translated and the dispatch would reach
140+
* a backend with no way to express it.
141+
* <p>
142+
* Anything else is left alone. A dispatch may legitimately remain in a bounded generic that is
143+
* merely declared and never called, and garbage removal deletes those later; deciding here
144+
* would mean duplicating reachability. One which survives that far and still reaches the
145+
* backend is reported there, where it is known to be both reachable and unresolvable.
146+
*/
147+
private void settleRemainingDispatches() {
148+
List<ImTypeVarDispatch> remaining = new ArrayList<>();
149+
prog.accept(new Element.DefaultVisitor() {
150+
@Override
151+
public void visit(ImTypeVarDispatch dispatch) {
152+
super.visit(dispatch);
153+
remaining.add(dispatch);
154+
}
155+
});
156+
for (ImTypeVarDispatch dispatch : remaining) {
157+
ImFunction owner = dispatch.getNearestFunc();
158+
if (owner != null && specializedFunctions.containsRow(owner)) {
159+
dispatch.replaceBy(defaultValueFor(dispatch.getTypeClassFunc().getReturnType()));
160+
}
161+
}
162+
}
163+
164+
private static ImExpr defaultValueFor(ImType type) {
165+
return JassIm.ImNull(type.copy());
131166
}
132167

133168

@@ -271,7 +306,7 @@ public void visit(ImAlloc alloc) {
271306
// Constructing a class whose methods dispatch has to be specialised as well:
272307
// otherwise the constructor keeps a generic result type, and a method call on that
273308
// result never becomes concrete enough to resolve.
274-
if (classNeedsSpecialization(alloc.getClazz().getClassDef(), visitedFunctions, visitedMethods)) {
309+
if (classNeedsSpecialization(alloc.getClazz().getClassDef())) {
275310
found[0] = true;
276311
return;
277312
}
@@ -320,44 +355,59 @@ && functionNeedsSpecialization(method.getImplementation(), visitedFunctions, vis
320355
/**
321356
* Whether constructing this class requires the concrete type argument, because one of its own
322357
* or inherited members dispatches on a type class bound.
358+
* <p>
359+
* Deliberately a property of the class alone, not of the path that asked. An earlier version
360+
* threaded the caller's visited set through here and memoised the answer, so a query made while
361+
* one of the class's own functions was already being visited recorded a negative result that
362+
* then stood for every later query.
323363
*/
324-
private boolean classNeedsSpecialization(ImClass classDef, Set<ImFunction> visitedFunctions,
325-
Set<ImMethod> visitedMethods) {
326-
Boolean cached = classNeedsSpecialization.get(classDef);
364+
private boolean classNeedsSpecialization(ImClass classDef) {
365+
Boolean cached = classNeedsSpecializationCache.get(classDef);
327366
if (cached != null) {
328-
// Already answered, or currently being answered: a class reached through its own
329-
// members contributes nothing new to the decision.
330367
return cached;
331368
}
332-
classNeedsSpecialization.put(classDef, false);
333-
boolean result = false;
369+
classNeedsSpecializationCache.put(classDef, false);
370+
boolean result = classDispatchesOnBound(classDef,
371+
Collections.newSetFromMap(new IdentityHashMap<>()));
372+
classNeedsSpecializationCache.put(classDef, result);
373+
return result;
374+
}
375+
376+
private boolean classDispatchesOnBound(ImClass classDef, Set<ImClass> visited) {
377+
if (!visited.add(classDef)) {
378+
return false;
379+
}
334380
for (ImFunction f : classDef.getFunctions()) {
335-
if (functionNeedsSpecialization(f, visitedFunctions, visitedMethods)) {
336-
result = true;
337-
break;
381+
if (containsDispatch(f)) {
382+
return true;
338383
}
339384
}
340-
if (!result) {
341-
for (ImMethod m : classDef.getMethods()) {
342-
if (methodNeedsSpecialization(m, visitedFunctions, visitedMethods)) {
343-
result = true;
344-
break;
345-
}
385+
for (ImMethod m : classDef.getMethods()) {
386+
if (m.getImplementation() != null && containsDispatch(m.getImplementation())) {
387+
return true;
346388
}
347389
}
348-
if (!result) {
349-
for (ImClassType superType : classDef.getSuperClasses()) {
350-
if (classNeedsSpecialization(superType.getClassDef(), visitedFunctions, visitedMethods)) {
351-
result = true;
352-
break;
353-
}
390+
for (ImClassType superType : classDef.getSuperClasses()) {
391+
if (classDispatchesOnBound(superType.getClassDef(), visited)) {
392+
return true;
354393
}
355394
}
356-
classNeedsSpecialization.put(classDef, result);
357-
return result;
395+
return false;
396+
}
397+
398+
/** Whether this function body dispatches on a bound, without following calls out of it. */
399+
private static boolean containsDispatch(ImFunction f) {
400+
boolean[] found = {false};
401+
f.accept(new Element.DefaultVisitor() {
402+
@Override
403+
public void visit(ImTypeVarDispatch dispatch) {
404+
found[0] = true;
405+
}
406+
});
407+
return found[0];
358408
}
359409

360-
private final Map<ImClass, Boolean> classNeedsSpecialization = new IdentityHashMap<>();
410+
private final Map<ImClass, Boolean> classNeedsSpecializationCache = new IdentityHashMap<>();
361411

362412
private void assertNoReachableGenericNewMarkers() {
363413
prog.accept(new Element.DefaultVisitor() {

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package de.peeeq.wurstscript.translation.lua.translation;
22

33
import de.peeeq.wurstscript.WurstOperator;
4+
import de.peeeq.wurstscript.attributes.CompileError;
45
import de.peeeq.wurstscript.jassIm.*;
56
import de.peeeq.wurstscript.luaAst.*;
67
import de.peeeq.wurstscript.translation.imtranslation.ImTranslator;
@@ -461,7 +462,12 @@ public static LuaExpr translate(ImCompiletimeExpr imCompiletimeExpr, LuaTranslat
461462
}
462463

463464
public static LuaExpr translate(ImTypeVarDispatch imTypeVarDispatch, LuaTranslator tr) {
464-
throw new Error("not implemented");
465+
// Reaching the backend means specialization never supplied a concrete type for this
466+
// dispatch and the code is reachable, since unreachable functions have been removed by now.
467+
throw new CompileError(imTypeVarDispatch.attrTrace().attrSource(),
468+
"Type class dispatch of " + imTypeVarDispatch.getTypeClassFunc().getName()
469+
+ " could not be resolved for the Lua target: the concrete type is not available"
470+
+ " where it is used.");
465471
}
466472

467473
public static LuaExpr translate(ImCast imCast, LuaTranslator tr) {

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

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import io.vavr.control.Option;
1414
import org.eclipse.jdt.annotation.Nullable;
1515

16+
import java.util.ArrayList;
1617
import java.util.List;
1718
import java.util.stream.Stream;
1819

@@ -98,13 +99,45 @@ public void addMemberMethods(Element node, String name, List<FuncLink> result) {
9899
if (!staticRef) {
99100
return;
100101
}
102+
// Bounds are ordered and an earlier one wins, but only over the same signature: two bounds
103+
// may require the very same operation, and offering both would make every call ambiguous.
104+
// Differently shaped overloads are not in competition, so later bounds still contribute
105+
// them and overload resolution picks between them as usual.
106+
List<FuncLink> supplied = new ArrayList<>();
101107
for (InterfaceDef bound : TypeClassConstraints.boundInterfaces(def)) {
102108
for (FuncDef method : bound.getMethods()) {
103-
if (method.getName().equals(name)) {
104-
result.add(requirementLink(node, bound, method));
109+
if (!method.getName().equals(name)) {
110+
continue;
105111
}
112+
FuncLink candidate = requirementLink(node, bound, method);
113+
if (!alreadySupplied(supplied, candidate, node)) {
114+
supplied.add(candidate);
115+
}
116+
}
117+
}
118+
result.addAll(supplied);
119+
}
120+
121+
/** True when an earlier bound already supplied a requirement of the same shape. */
122+
private static boolean alreadySupplied(List<FuncLink> supplied, FuncLink candidate, Element node) {
123+
for (FuncLink existing : supplied) {
124+
List<WurstType> a = existing.getParameterTypes();
125+
List<WurstType> b = candidate.getParameterTypes();
126+
if (a.size() != b.size()) {
127+
continue;
128+
}
129+
boolean same = true;
130+
for (int i = 0; i < a.size(); i++) {
131+
if (!a.get(i).equalsType(b.get(i), node)) {
132+
same = false;
133+
break;
134+
}
135+
}
136+
if (same) {
137+
return true;
106138
}
107139
}
140+
return false;
108141
}
109142

110143
@Override

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

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -847,6 +847,124 @@ public void sameSimpleNameThroughRegistryFallback() {
847847
);
848848
}
849849

850+
/**
851+
* Two bounds may require the same operation. Bounds are ordered and the earlier one wins,
852+
* rather than every call to the shared operation becoming ambiguous.
853+
*/
854+
@Test
855+
public void duplicateRequirementAcrossBounds() {
856+
testAssertOkLines(true,
857+
"package test",
858+
"native testSuccess()",
859+
"interface First<T:>",
860+
" function show(T x) returns string",
861+
"interface Second<T:>",
862+
" function other(T x) returns int",
863+
" function show(T x) returns string",
864+
"implements First<int>",
865+
" function show(int x) returns string",
866+
" return \"first\"",
867+
"implements Second<int>",
868+
" function other(int x) returns int",
869+
" return 1",
870+
" function show(int x) returns string",
871+
" return \"second\"",
872+
"function render<Q: First and Second>(Q x) returns string",
873+
" return Q.show(x)",
874+
"init",
875+
" if render(1) == \"first\"",
876+
" testSuccess()"
877+
);
878+
}
879+
880+
/**
881+
* Bounds only shadow each other when they require the same shape. A later bound still supplies
882+
* a differently shaped overload, which overload resolution then chooses between.
883+
*/
884+
@Test
885+
public void overloadFromLaterBoundStaysAvailable() {
886+
testAssertOkLines(true,
887+
"package test",
888+
"native testSuccess()",
889+
"interface First<T:>",
890+
" function show(T x) returns string",
891+
"interface Second<T:>",
892+
" function show(T x) returns string",
893+
" function show(int scale, T x) returns string",
894+
"implements First<int>",
895+
" function show(int x) returns string",
896+
" return \"first\"",
897+
"implements Second<int>",
898+
" function show(int x) returns string",
899+
" return \"second\"",
900+
" function show(int scale, int x) returns string",
901+
" return \"scaled\"",
902+
"function render<Q: First and Second>(Q x) returns string",
903+
" return Q.show(x) + Q.show(2, x)",
904+
"init",
905+
" if render(1) == \"firstscaled\"",
906+
" testSuccess()"
907+
);
908+
}
909+
910+
/**
911+
* A bounded generic which is only declared, never called, stays valid: it is unreachable, so
912+
* nothing has to supply a concrete type for it.
913+
*/
914+
@Test
915+
public void unusedBoundedGenericFunctionLua() {
916+
test().testLua(true).executeProg().lines(
917+
"package test",
918+
"native testSuccess()",
919+
"interface Show<T:>",
920+
" function show(T x) returns string",
921+
"implements Show<int>",
922+
" function show(int x) returns string",
923+
" return \"i\"",
924+
"function unused<Q: Show>(Q x) returns string",
925+
" return Q.show(x)",
926+
"init",
927+
" testSuccess()"
928+
);
929+
}
930+
931+
/** The same for a bounded generic class which is never constructed. */
932+
@Test
933+
public void unusedBoundedGenericClassLua() {
934+
test().testLua(true).executeProg().lines(
935+
"package test",
936+
"native testSuccess()",
937+
"interface Show<T:>",
938+
" function show(T x) returns string",
939+
"implements Show<int>",
940+
" function show(int x) returns string",
941+
" return \"i\"",
942+
"class Unused<Q: Show>",
943+
" function render(Q x) returns string",
944+
" return Q.show(x)",
945+
"init",
946+
" testSuccess()"
947+
);
948+
}
949+
950+
/** Declared and unused on Jass too, which is where it already worked. */
951+
@Test
952+
public void unusedBoundedGenericFunction() {
953+
testAssertOkLines(true,
954+
"package test",
955+
"native testSuccess()",
956+
"interface Show<T:>",
957+
" function show(T x) returns string",
958+
"implements Show<int>",
959+
" function show(int x) returns string",
960+
" return \"i\"",
961+
"function unused<Q: Show>(Q x) returns string",
962+
" return Q.show(x)",
963+
"init",
964+
" testSuccess()"
965+
);
966+
}
967+
850968
/** A type parameter is not a value, so it may only appear as the receiver of a requirement. */
851969
@Test
852970
public void typeParameterIsNotAValue() {

0 commit comments

Comments
 (0)