Skip to content

Commit f4d9c0a

Browse files
FrottyFrotty
authored
Fix new-generic override bug, optimize and test dispatch (#1144)
1 parent 93cad33 commit f4d9c0a

5 files changed

Lines changed: 208 additions & 3 deletions

File tree

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

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -628,9 +628,25 @@ private void replaceMethodCall(ImMethodCall mc) {
628628
ImExprs arguments = JassIm.ImExprs(receiver);
629629
arguments.addAll(mc.getArguments().removeAll());
630630

631-
ImFunction dispatch = dispatchFuncs.get(mc.getMethod());
631+
ImMethod method = mc.getMethod();
632+
// Fast path: with unchecked dispatch, a monomorphic method call can be lowered
633+
// directly to its implementation function.
634+
if (!checkedDispatch
635+
&& !method.getIsAbstract()
636+
&& method.getSubMethods().isEmpty()) {
637+
mc.replaceBy(JassIm.ImFunctionCall(
638+
mc.getTrace(),
639+
method.getImplementation(),
640+
JassIm.ImTypeArguments(),
641+
arguments,
642+
false,
643+
CallType.NORMAL));
644+
return;
645+
}
646+
647+
ImFunction dispatch = dispatchFuncs.get(method);
632648
if (dispatch == null) {
633-
throw new CompileError(mc.attrTrace().attrSource(), "Could not find dispatch for " + mc.getMethod().getName());
649+
throw new CompileError(mc.attrTrace().attrSource(), "Could not find dispatch for " + method.getName());
634650
}
635651
mc.replaceBy(JassIm.ImFunctionCall(mc.getTrace(), dispatch, JassIm.ImTypeArguments(), arguments, false, CallType.NORMAL));
636652

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

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -955,6 +955,8 @@ private boolean isGlobalInitStmt(ImSet s, ImVar v) {
955955
}
956956

957957
private void collectGenericUsages(Element element) {
958+
// Cache expensive recursive submethod checks within this traversal.
959+
Map<ImMethod, Boolean> hasGenericSubmethodCache = new IdentityHashMap<>();
958960
element.accept(new Element.DefaultVisitor() {
959961
@Override
960962
public void visit(ImFunctionCall f) {
@@ -967,7 +969,22 @@ public void visit(ImFunctionCall f) {
967969
@Override
968970
public void visit(ImMethodCall mc) {
969971
super.visit(mc);
970-
if (!mc.getTypeArguments().isEmpty()) {
972+
ImMethod method = mc.getMethod();
973+
boolean hasTypeArgs = !mc.getTypeArguments().isEmpty();
974+
boolean needsDispatchSpecialization = false;
975+
// If type args are present, specialization is unconditional, so avoid extra checks.
976+
if (!hasTypeArgs) {
977+
// Interface/base dispatch methods can be non-generic but still require specialization
978+
// when they dispatch to generic implementors.
979+
needsDispatchSpecialization = methodImplementationIsGeneric(method);
980+
if (!needsDispatchSpecialization) {
981+
needsDispatchSpecialization = hasGenericSubmethodCache.computeIfAbsent(
982+
method,
983+
EliminateGenerics.this::hasGenericSubmethodImplementation
984+
);
985+
}
986+
}
987+
if (hasTypeArgs || needsDispatchSpecialization) {
971988
dbg("COLLECT GenericMethodCall: method=" + mc.getMethod().getName() + " " + id(mc.getMethod())
972989
+ " impl=" + (mc.getMethod().getImplementation() == null ? "null" : (mc.getMethod().getImplementation().getName() + " " + id(mc.getMethod().getImplementation())))
973990
+ " owningClass=" + (mc.getMethod().attrClass() == null ? "null" : (mc.getMethod().attrClass().getName() + " " + id(mc.getMethod().attrClass())))
@@ -1109,6 +1126,30 @@ public void visit(ImTypeIdOfClass f) {
11091126
});
11101127
}
11111128

1129+
private boolean methodImplementationIsGeneric(ImMethod method) {
1130+
ImFunction implementation = method.getImplementation();
1131+
return implementation != null && !implementation.getTypeVariables().isEmpty();
1132+
}
1133+
1134+
private boolean hasGenericSubmethodImplementation(ImMethod method) {
1135+
return hasGenericSubmethodImplementation(method, Collections.newSetFromMap(new IdentityHashMap<>()));
1136+
}
1137+
1138+
private boolean hasGenericSubmethodImplementation(ImMethod method, Set<ImMethod> visited) {
1139+
if (!visited.add(method)) {
1140+
return false;
1141+
}
1142+
for (ImMethod subMethod : method.getSubMethods()) {
1143+
if (methodImplementationIsGeneric(subMethod)) {
1144+
return true;
1145+
}
1146+
if (hasGenericSubmethodImplementation(subMethod, visited)) {
1147+
return true;
1148+
}
1149+
}
1150+
return false;
1151+
}
1152+
11121153
static boolean isGenericType(ImType type) {
11131154
return type.match(new ImType.Matcher<Boolean>() {
11141155
@Override

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

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2091,6 +2091,49 @@ public void fullArrayListTest() throws IOException {
20912091
assertEquals(count, 2);
20922092

20932093
assertFalse(compiled.contains("ArrayList_nextFreeIndex_"));
2094+
// In checked mode, virtual method calls should go through dispatch.
2095+
assertTrue(compiled.contains("dispatch_ArrayList"));
2096+
}
2097+
2098+
@Test
2099+
public void fullArrayListTestUncheckedDispatch() throws IOException {
2100+
test().withStdLib().uncheckedDispatch().executeProg().executeTests().file(new File(TEST_DIR + "arrayList.wurst"));
2101+
2102+
String compiled = Files.toString(
2103+
new File(TEST_OUTPUT_PATH + "GenericsWithTypeclassesTests_fullArrayListTestUncheckedDispatch_no_opts.jim"),
2104+
Charsets.UTF_8);
2105+
2106+
// In unchecked dispatch mode, monomorphic ArrayList<T>.get should be lowered directly.
2107+
assertTrue(compiled.contains("ArrayList_get"));
2108+
assertFalse(compiled.contains("dispatch_ArrayList"));
2109+
}
2110+
2111+
@Test
2112+
public void fullReactiveGenericDispatchTest() throws IOException {
2113+
test().withStdLib().executeProg().executeTests().file(new File(TEST_DIR + "reactiveGenericsDispatch.wurst"));
2114+
2115+
String compiled = Files.toString(
2116+
new File(TEST_OUTPUT_PATH + "GenericsWithTypeclassesTests_fullReactiveGenericDispatchTest_no_opts.jim"),
2117+
Charsets.UTF_8);
2118+
2119+
// In checked mode, polymorphic interface calls are dispatched and guarded.
2120+
assertTrue(compiled.contains("dispatch_ReactiveSource_ReactiveSource_subscribe"));
2121+
assertTrue(compiled.contains("Nullpointer exception when calling ReactiveSource.subscribe"));
2122+
assertTrue(compiled.contains("Called ReactiveSource.subscribe on invalid object."));
2123+
}
2124+
2125+
@Test
2126+
public void fullReactiveGenericDispatchTestUncheckedDispatch() throws IOException {
2127+
test().withStdLib().uncheckedDispatch().executeProg().executeTests().file(new File(TEST_DIR + "reactiveGenericsDispatch.wurst"));
2128+
2129+
String compiled = Files.toString(
2130+
new File(TEST_OUTPUT_PATH + "GenericsWithTypeclassesTests_fullReactiveGenericDispatchTestUncheckedDispatch_no_opts.jim"),
2131+
Charsets.UTF_8);
2132+
2133+
// ReactiveSource is polymorphic, so dispatch remains, but safety checks should be removed.
2134+
assertTrue(compiled.contains("dispatch_ReactiveSource_ReactiveSource_subscribe"));
2135+
assertFalse(compiled.contains("Nullpointer exception when calling ReactiveSource.subscribe"));
2136+
assertFalse(compiled.contains("Called ReactiveSource.subscribe on invalid object."));
20942137
}
20952138

20962139
@Test

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ class TestConfig {
7070
private boolean stopOnFirstError = true;
7171
private boolean runCompiletimeFunctions;
7272
private boolean testLua = false;
73+
private boolean uncheckedDispatch = false;
7374

7475
TestConfig(String name) {
7576
this.name = name;
@@ -123,6 +124,15 @@ public TestConfig executeProgOnlyAfterTransforms(boolean b) {
123124
return this;
124125
}
125126

127+
public TestConfig uncheckedDispatch() {
128+
return uncheckedDispatch(true);
129+
}
130+
131+
public TestConfig uncheckedDispatch(boolean b) {
132+
this.uncheckedDispatch = b;
133+
return this;
134+
}
135+
126136
TestConfig expectError(String expectedError) {
127137
this.expectedError = expectedError;
128138
return this;
@@ -186,6 +196,9 @@ private CompilationResult testScript() {
186196
if (withStdLib) {
187197
runArgs = runArgs.with("-lib", StdLib.getLib());
188198
}
199+
if (uncheckedDispatch) {
200+
runArgs = runArgs.with("-uncheckedDispatch");
201+
}
189202
if (runCompiletimeFunctions) {
190203
runArgs = runArgs.with("-runcompiletimefunctions");
191204
}

‎de.peeeq.wurstscript/testscripts/concept/reactiveGenericsDispatch.wurst‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
package ReactiveGenericDispatchRegression
2+
import NoWurst
3+
import Integer
4+
import Wurstunit
5+
6+
interface ReactiveSource
7+
function subscribe(Observer observer)
8+
function unsubscribe(Observer observer)
9+
10+
class Observer
11+
int dependencyChanges = 0
12+
13+
function trackSource(ReactiveSource source)
14+
source.subscribe(this)
15+
16+
function untrackSource(ReactiveSource source)
17+
source.unsubscribe(this)
18+
19+
function onDependencyChanged()
20+
dependencyChanges += 1
21+
22+
23+
class Signal<T:> implements ReactiveSource
24+
private T value
25+
private Observer subscriber = null
26+
27+
construct(T initial)
28+
value = initial
29+
30+
function set(T newValue)
31+
value = newValue
32+
if subscriber != null
33+
subscriber.onDependencyChanged()
34+
35+
function subscriberCount() returns int
36+
return subscriber == null ? 0 : 1
37+
38+
override function subscribe(Observer observer)
39+
subscriber = observer
40+
41+
override function unsubscribe(Observer observer)
42+
if subscriber == observer
43+
subscriber = null
44+
45+
46+
class PlainSource implements ReactiveSource
47+
private Observer subscriber = null
48+
49+
function fire()
50+
if subscriber != null
51+
subscriber.onDependencyChanged()
52+
53+
function subscriberCount() returns int
54+
return subscriber == null ? 0 : 1
55+
56+
override function subscribe(Observer observer)
57+
subscriber = observer
58+
59+
override function unsubscribe(Observer observer)
60+
if subscriber == observer
61+
subscriber = null
62+
63+
init
64+
let genericObserver = new Observer()
65+
let genericSignal = new Signal<int>(0)
66+
genericObserver.trackSource(genericSignal)
67+
if genericSignal.subscriberCount() != 1
68+
testFail("generic subscribe dispatch failed")
69+
genericSignal.set(1)
70+
if genericObserver.dependencyChanges != 1
71+
testFail("generic notify dispatch failed")
72+
genericObserver.untrackSource(genericSignal)
73+
if genericSignal.subscriberCount() != 0
74+
testFail("generic unsubscribe dispatch failed")
75+
76+
let plainObserver = new Observer()
77+
let plainSource = new PlainSource()
78+
plainObserver.trackSource(plainSource)
79+
if plainSource.subscriberCount() != 1
80+
testFail("plain subscribe dispatch failed")
81+
plainSource.fire()
82+
if plainObserver.dependencyChanges != 1
83+
testFail("plain notify dispatch failed")
84+
plainObserver.untrackSource(plainSource)
85+
if plainSource.subscriberCount() != 0
86+
testFail("plain unsubscribe dispatch failed")
87+
88+
destroy genericSignal
89+
destroy genericObserver
90+
destroy plainSource
91+
destroy plainObserver
92+
testSuccess()

0 commit comments

Comments
 (0)