diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/CyclicFunctionRemover.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/CyclicFunctionRemover.java index a205bc694..7bf304330 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/CyclicFunctionRemover.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/CyclicFunctionRemover.java @@ -5,8 +5,10 @@ import de.peeeq.datastructures.GraphInterpreter; import de.peeeq.wurstio.TimeTaker; import de.peeeq.wurstscript.WurstOperator; +import de.peeeq.wurstscript.attributes.CompileError; import de.peeeq.wurstscript.jassIm.*; import de.peeeq.wurstscript.types.WurstTypeInt; +import org.eclipse.jdt.annotation.Nullable; import java.util.*; @@ -64,6 +66,7 @@ private void removeCycle(List funcs, Set funcSet) { prog.getFunctions().add(newFunc); newFunc.getParameters().add(choiceVar); newFunc.getParameters().addAll(newParameters); + checkMergedSignatureFits(funcs, newFunc); ImStmts stmts = newFunc.getBody(); @@ -206,18 +209,19 @@ private void replaceImFunctionCall(Set funcSet, Map oldArgs = fc.getArguments().removeAll(); - int pos = 0; + Map argumentBySlot = new IdentityHashMap<>(); + for (int i = 0; i < oldArgs.size() && i < oldFunc.getParameters().size(); i++) { + argumentBySlot.put(oldToNewVar.get(oldFunc.getParameters().get(i)), oldArgs.get(i)); + } for (int i = 1; i < newFunc.getParameters().size(); i++) { ImVar p = newFunc.getParameters().get(i); - if (pos < oldArgs.size() && oldToNewVar.get(oldFunc.getParameters().get(pos)) == p) { - arguments.add(oldArgs.get(pos)); - pos++; - } else { - // use default value - arguments.add(tr.getDefaultValueForJassType(p.getType())); - } + ImExpr argument = argumentBySlot.get(p); + arguments.add(argument != null ? argument : tr.getDefaultValueForJassType(p.getType())); } @@ -273,27 +277,73 @@ private String makeName(List funcs) { return "cyc_" + funcs.get(0).getName(); } + /** + * Builds the parameters the merged function takes: one slot per parameter which needs to exist at + * the same time as another, shared by every function in the cycle which can use it. + *

+ * Two parameters of the same function are live at once and so can never share a slot, which is the + * only constraint here. This used to be enforced by searching for a slot from a position which only + * moved forwards, which enforces rather more than that: a parameter matching a slot late in the + * union puts every parameter after it past everything already allocated, so it allocates again. The + * cost is per function and accumulates along the cycle, and three functions taking the same + * parameters in different orders already came out at fifteen slots where nine were needed. + *

+ * That is how a merged function ends up over the Jass parameter limit without any function in the + * source being anywhere near it. Remembering which slots this function has already claimed says the + * same thing about liveness without the ordering, and needs one slot per type per position at which + * some function in the cycle uses that type, which is the fewest this scheme can use. + */ private void calculateNewParameters(List funcs, List newParameters, Map oldToNewVar) { for (ImFunction f : funcs) { - int pos = 0; - withNextParameter: + Set claimedByThisFunction = Collections.newSetFromMap(new IdentityHashMap<>()); for (ImVar v : f.getParameters()) { - // first check if we can reuse a parameter from the newParameters - for (int i = pos; i < newParameters.size(); i++) { - if (newParameters.get(i).getType().translateType().equals(v.getType().translateType())) { - // found a var we can reuse - oldToNewVar.put(v, newParameters.get(i)); - pos = i + 1; - continue withNextParameter; - } + ImVar slot = firstFreeSlotOfType(newParameters, claimedByThisFunction, v); + if (slot == null) { + slot = JassIm.ImVar(v.getTrace(), v.getType().copy(), v.getName(), false); + newParameters.add(slot); } - // otherwise, we have to create a new var: - ImVar newVar = JassIm.ImVar(v.getTrace(), v.getType().copy(), v.getName(), false); - oldToNewVar.put(v, newVar); - newParameters.add(newVar); - pos = newParameters.size() + 1; + claimedByThisFunction.add(slot); + oldToNewVar.put(v, slot); + } + } + } + + /** The first slot this function has not claimed which holds the same Jass type, if there is one. */ + private @Nullable ImVar firstFreeSlotOfType(List newParameters, Set claimed, ImVar v) { + for (ImVar candidate : newParameters) { + if (!claimed.contains(candidate) + && candidate.getType().translateType().equals(v.getType().translateType())) { + return candidate; + } + } + return null; + } + + /** + * Refuses a merged function the game could not load, rather than emitting one. + *

+ * With slots shared as tightly as they can be this needs a cycle whose functions genuinely need + * more than 31 Jass parameters live at once, which no reachable source has produced - but the + * failure it replaces is silent, and a script the game rejects at load is the worst way to find out. + * Passing the excess through an array indexed by recursion depth would lift the limit, and is worth + * doing only once something hits this. + */ + private void checkMergedSignatureFits(List funcs, ImFunction newFunc) { + int jassParameterCount = newFunc.getParameters().stream() + .mapToInt(p -> ImHelper.flattenedJassArity(p.getType())) + .sum(); + if (jassParameterCount > ImHelper.JASS_MAX_PARAMETERS) { + StringBuilder names = new StringBuilder(); + for (ImFunction f : funcs) { + names.append(names.length() == 0 ? "" : ", ").append(f.getName()); } + throw new CompileError(newFunc.getTrace(), "These functions call each other in a cycle: " + + names + ". Jass cannot declare a function before it is defined, so they are compiled" + + " into one function taking the parameters of all of them, which comes to " + + jassParameterCount + " Jass parameters and the maximum is " + + ImHelper.JASS_MAX_PARAMETERS + " (a tuple counts as one per component). Break the" + + " cycle, or pass fewer values through it."); } } diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/ImHelper.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/ImHelper.java index bc5af484e..015bd7da7 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/ImHelper.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/ImHelper.java @@ -10,6 +10,25 @@ public class ImHelper { + /** A Jass function takes at most this many parameters. */ + public static final int JASS_MAX_PARAMETERS = 31; + + /** + * How many Jass parameters a value of this type occupies. + *

+ * A tuple is passed as one parameter per component, so an IM signature which looks well inside the + * limit can emit a Jass one which is not. Any pass which builds a signature has to count this way + * rather than counting parameters. + */ + public static int flattenedJassArity(ImType type) { + if (type instanceof ImTupleType) { + return ((ImTupleType) type).getTypes().stream() + .mapToInt(ImHelper::flattenedJassArity) + .sum(); + } + return 1; + } + public static Set calculateFunctionsOfProg(ImProg prog) { Set allFunctions = new HashSet<>(prog.getFunctions()); for(ImClass c : prog.getClasses()) { diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/VarargEliminator.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/VarargEliminator.java index 54287da50..6a131105c 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/VarargEliminator.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/VarargEliminator.java @@ -19,7 +19,7 @@ */ public class VarargEliminator { - private static final int JASS_MAX_PARAMETERS = 31; + private static final int JASS_MAX_PARAMETERS = ImHelper.JASS_MAX_PARAMETERS; private final ImProg prog; // original + number of args --> new function private final Table varargFuncs = HashBasedTable.create(); @@ -72,7 +72,7 @@ private void generateVarargFunc(ImFunctionCall sourceCall) { ImFunction func = sourceCall.getFunc(); int numberOfParams = sourceCall.getArguments().size(); int jassParameterCount = sourceCall.getArguments().stream() - .mapToInt(argument -> flattenedJassArity(argument.attrTyp())) + .mapToInt(argument -> ImHelper.flattenedJassArity(argument.attrTyp())) .sum(); if (jassParameterCount > JASS_MAX_PARAMETERS) { throw new CompileError(sourceCall, "Vararg call would generate " + jassParameterCount @@ -147,15 +147,6 @@ public void visit(ImVarargLoop imLoop) { varargFuncs.put(func, numberOfParams, newFunc); } - private int flattenedJassArity(ImType type) { - if (type instanceof ImTupleType) { - return ((ImTupleType) type).getTypes().stream() - .mapToInt(this::flattenedJassArity) - .sum(); - } - return 1; - } - @NotNull private List collectUsesOfVar(ImFunction newFunc, ImVar varargParam) { List varargParamUses = new ArrayList<>(); diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/CyclicFunctionTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/CyclicFunctionTests.java new file mode 100644 index 000000000..388171b96 --- /dev/null +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/CyclicFunctionTests.java @@ -0,0 +1,185 @@ +package tests.wurstscript.tests; + +import com.google.common.base.Charsets; +import com.google.common.io.Files; +import org.testng.annotations.Test; + +import java.io.File; +import java.io.IOException; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * Jass has no forward declaration, so mutually recursive functions cannot be emitted as themselves: + * a cycle is merged into one function which takes a choice of which body to run plus the parameters + * of all of them. That merged signature has to fit in a Jass function, and nothing used to check that + * it did. + */ +public class CyclicFunctionTests extends WurstScriptTest { + + /** A Jass function takes at most 31 parameters, and a tuple counts as one per component. */ + private static final int JASS_MAX_PARAMETERS = 31; + + /** + * Two functions in a cycle taking the same parameters in a different order. + *

+ * Sharing a slot used to be looked for from a position which only moved forwards, so a parameter + * matching a slot late in the union pushed every parameter after it past everything already there: + * {@code pong} matched its {@code int} against the last slot and then had five tuples left with + * nowhere behind it to go. Eleven slots for six parameters, 32 Jass parameters for a signature + * which needs 17, and the emitted function was over the limit while the IM function looked fine. + *

+ * Reordering arguments between two functions which call each other is ordinary, so this is not a + * corner: it is two functions and one swap. + */ + @Test + public void mutuallyRecursiveFunctionsSharingParametersFitTheJassLimit() throws IOException { + test().executeProg().lines( + "package test", + "native testSuccess()", + "tuple vec3(real x, real y, real z)", + "function ping(vec3 a, vec3 b, vec3 c, vec3 d, vec3 e, int n) returns int", + " if n <= 0", + " return 0", + " return pong(n - 1, a, b, c, d, e)", + "function pong(int n, vec3 a, vec3 b, vec3 c, vec3 d, vec3 e) returns int", + " if n <= 0", + " return 1", + " return ping(a, b, c, d, e, n - 1)", + "init", + " vec3 v = vec3(1., 2., 3.)", + " if ping(v, v, v, v, v, 3) == 1", + " testSuccess()" + ); + String jass = compiledJass("mutuallyRecursiveFunctionsSharingParametersFitTheJassLimit"); + assertNoFunctionExceedsTheJassLimit(jass); + // choice, one int, and five tuples of three reals: what the wider of the two needs. + assertMergedCycleTakes(jass, 17); + } + + /** + * The same shape across more of the cycle, since the union is built one function at a time and a + * slot the next function cannot reach is a slot every function after it cannot reach either. + */ + @Test + public void aLongerCycleReusesSlotsRatherThanAccumulatingThem() throws IOException { + test().executeProg().lines( + "package test", + "native testSuccess()", + "tuple pair(real x, real y)", + "function first(pair a, pair b, pair c, int n, string s) returns int", + " if n <= 0", + " return 7", + " return second(s, a, n - 1, b, c)", + "function second(string s, pair a, int n, pair b, pair c) returns int", + " if n <= 0", + " return 7", + " return third(a, n - 1, b, s, c)", + "function third(pair a, int n, pair b, string s, pair c) returns int", + " if n <= 0", + " return 7", + " return first(a, b, c, n - 1, s)", + "init", + " pair p = pair(1., 2.)", + " if first(p, p, p, 6, \"x\") == 7", + " testSuccess()" + ); + String jass = compiledJass("aLongerCycleReusesSlotsRatherThanAccumulatingThem"); + assertNoFunctionExceedsTheJassLimit(jass); + // choice, three tuples of two reals, one int and one string. It took fifteen before. + assertMergedCycleTakes(jass, 9); + } + + /** + * Audit probe for the sharing this got more aggressive about. Slots are shared by Jass type, so a + * class reference and an {@code int} are interchangeable and so are two tuples with the same + * components - the slot keeps the IM type of whichever parameter claimed it first, and the other + * function's body then reads a variable typed as something else. Searching backwards reaches those + * slots far more often than searching forwards did, so what used to be rare is now the common case. + *

+ * Every parameter here is a mismatch of that kind: {@code down} takes an int where the slot is a + * class, a point where the slot is a pair, and a class where the slot is an int. + */ + @Test + public void slotsSharedBetweenUnlikeTypesWithTheSameJassTypeStillCarryTheirValues() throws IOException { + test().executeProg().lines( + "package test", + "native testSuccess()", + "tuple pair(real x, real y)", + "tuple point(real a, real b)", + "class Foo", + " int v", + " construct(int v)", + " this.v = v", + "function up(Foo f, pair p, int n) returns int", + " if n <= 0", + " return f.v", + " return down(n - 1, point(p.x, p.y), f)", + "function down(int n, point q, Foo f) returns int", + " if n <= 0", + " return f.v", + " return up(f, pair(q.a, q.b), n - 1)", + "init", + " Foo f = new Foo(5)", + " if up(f, pair(1., 2.), 3) == 5", + " testSuccess()" + ); + String jass = compiledJass("slotsSharedBetweenUnlikeTypesWithTheSameJassTypeStillCarryTheirValues"); + assertNoFunctionExceedsTheJassLimit(jass); + // Three slots for six parameters, every one of them shared across a type mismatch: an integer + // for the class or the count, a tuple, and an integer for the other. Five Jass parameters with + // the choice, since the tuple is two of them. + assertMergedCycleTakes(jass, 5); + } + + private String compiledJass(String testName) throws IOException { + return Files.toString( + new File(TEST_OUTPUT_PATH + "CyclicFunctionTests_" + testName + "_no_opts.j"), + Charsets.UTF_8); + } + + private static final Pattern FUNCTION_HEADER = + Pattern.compile("(?m)^function\\s+(\\w+)\\s+takes\\s+(.*?)\\s+returns\\s"); + + /** + * Counts the parameters the emitted Jass actually declares, which is the number the game applies + * its limit to - the IM function's parameter count is smaller wherever a tuple is passed. + */ + private static void assertNoFunctionExceedsTheJassLimit(String jass) { + Matcher header = FUNCTION_HEADER.matcher(jass); + while (header.find()) { + String parameters = header.group(2); + if (parameters.equals("nothing")) { + continue; + } + int count = parameters.split(",").length; + if (count > JASS_MAX_PARAMETERS) { + throw new AssertionError("function '" + header.group(1) + "' takes " + count + + " parameters, and Jass allows " + JASS_MAX_PARAMETERS + + "\n" + header.group()); + } + } + } + + private static final Pattern MERGED_CYCLE = + Pattern.compile("(?m)^function\\s+(cyc_\\w+)\\s+takes\\s+(.*?)\\s+returns\\s"); + + /** + * The merged function takes exactly the parameters the cycle needs live at once. + *

+ * Staying under the limit is the requirement, but a signature which is merely under it is how this + * got here: the slack was being spent one duplicate slot at a time, per function, and only a long + * enough cycle made it visible. The count is asserted so that spending starts failing again. + */ + private static void assertMergedCycleTakes(String jass, int expectedParameters) { + Matcher merged = MERGED_CYCLE.matcher(jass); + if (!merged.find()) { + throw new AssertionError("no merged cycle function in the emitted Jass"); + } + int count = merged.group(2).equals("nothing") ? 0 : merged.group(2).split(",").length; + if (count != expectedParameters) { + throw new AssertionError("merged cycle '" + merged.group(1) + "' takes " + count + + " parameters, expected " + expectedParameters + "\n" + merged.group()); + } + } +}