From eae2c00008acc554bdc570204beb844c455c9af4 Mon Sep 17 00:00:00 2001 From: Octavia Togami Date: Mon, 24 Feb 2020 03:33:49 -0500 Subject: [PATCH] Add more expression test cases, fix bugs Also added a few more comments + reorganized exceptions that are invoke-internal. (cherry picked from commit cbd686548fd62248fabbaab551a6875a14170957) --- .../{ => invoke}/BreakException.java | 4 +- .../expression/invoke/CompilingVisitor.java | 47 +++++++++----- .../expression/invoke/ExpressionCompiler.java | 19 ++++-- .../expression/invoke/ExpressionHandles.java | 9 ++- .../expression/invoke/ReturnException.java | 39 +++++++++++ .../internal/expression/ExpressionTest.java | 64 +++++++++++++------ 6 files changed, 136 insertions(+), 46 deletions(-) rename worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/{ => invoke}/BreakException.java (92%) create mode 100644 worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/ReturnException.java diff --git a/worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/BreakException.java b/worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/BreakException.java similarity index 92% rename from worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/BreakException.java rename to worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/BreakException.java index 5b2ae6b24..268f047eb 100644 --- a/worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/BreakException.java +++ b/worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/BreakException.java @@ -17,13 +17,13 @@ * along with this program. If not, see . */ -package com.sk89q.worldedit.internal.expression; +package com.sk89q.worldedit.internal.expression.invoke; /** * Thrown when a break or continue is encountered. * Loop constructs catch this exception. */ -public class BreakException extends RuntimeException { +class BreakException extends RuntimeException { public static final BreakException BREAK = new BreakException(false); public static final BreakException CONTINUE = new BreakException(true); diff --git a/worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/CompilingVisitor.java b/worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/CompilingVisitor.java index 2bf2f5b33..ae2b153ba 100644 --- a/worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/CompilingVisitor.java +++ b/worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/CompilingVisitor.java @@ -21,7 +21,6 @@ package com.sk89q.worldedit.internal.expression.invoke; import com.sk89q.worldedit.antlr.ExpressionBaseVisitor; import com.sk89q.worldedit.antlr.ExpressionParser; -import com.sk89q.worldedit.internal.expression.BreakException; import com.sk89q.worldedit.internal.expression.EvaluationException; import com.sk89q.worldedit.internal.expression.ExecutionData; import com.sk89q.worldedit.internal.expression.ExpressionHelper; @@ -217,12 +216,31 @@ class CompilingVisitor extends ExpressionBaseVisitor { return CONTINUE_STATEMENT; } + // MH = (Double)Double + // takes the double to return, conveniently has Double return type + private static final MethodHandle RETURN_STATEMENT_BASE = MethodHandles.filterReturnValue( + // take the (Double)ReturnException constructor + ExpressionHandles.NEW_RETURN_EXCEPTION, + // and map the return type to Double by throwing it + MethodHandles.throwException(Double.class, ReturnException.class) + ); + @Override public MethodHandle visitReturnStatement(ExpressionParser.ReturnStatementContext ctx) { + // MH:returnValue = (ExecutionData)Double + MethodHandle returnValue; if (ctx.value != null) { - return evaluate(ctx.value).handle; + returnValue = evaluate(ctx.value).handle; + } else { + returnValue = defaultResult(); } - return defaultResult(); + return MethodHandles.filterArguments( + // take the (Double)Double return statement + RETURN_STATEMENT_BASE, + 0, + // map the Double back to ExecutionData via the returnValue + returnValue + ); } @Override @@ -450,7 +468,7 @@ class CompilingVisitor extends ExpressionBaseVisitor { case NOT_EQUAL: return (l, r) -> ExpressionHandles.boolToDouble(l != r); case NEAR: - return (l, r) -> ExpressionHandles.boolToDouble(almostEqual2sComplement(l, r, 450359963L)); + return (l, r) -> ExpressionHandles.boolToDouble(almostEqual2sComplement(l, r)); case GREATER_THAN_OR_EQUAL: return (l, r) -> ExpressionHandles.boolToDouble(l >= r); } @@ -459,7 +477,7 @@ class CompilingVisitor extends ExpressionBaseVisitor { } // Usable AlmostEqual function, based on http://www.cygnus-software.com/papers/comparingfloats/comparingfloats.htm - private static boolean almostEqual2sComplement(double a, double b, long maxUlps) { + private static boolean almostEqual2sComplement(double a, double b) { // Make sure maxUlps is non-negative and small enough that the // default NAN won't compare as equal to anything. //assert(maxUlps > 0 && maxUlps < 4 * 1024 * 1024); // this is for floats, not doubles @@ -473,7 +491,7 @@ class CompilingVisitor extends ExpressionBaseVisitor { if (bLong < 0) bLong = 0x8000000000000000L - bLong; final long longDiff = Math.abs(aLong - bLong); - return longDiff <= maxUlps; + return longDiff <= 450359963L; } @Override @@ -645,11 +663,7 @@ class CompilingVisitor extends ExpressionBaseVisitor { checkHandle(childResult, (ParserRuleContext) c); } - boolean returning = c instanceof ExpressionParser.ReturnStatementContext; - result = aggregateResult(result, childResult, returning); - if (returning) { - return result; - } + result = aggregateHandleResult(result, childResult); } return result; @@ -660,25 +674,26 @@ class CompilingVisitor extends ExpressionBaseVisitor { throw new UnsupportedOperationException(); } - private MethodHandle aggregateResult(MethodHandle oldResult, MethodHandle result, - boolean keepDefault) { + private MethodHandle aggregateHandleResult(MethodHandle oldResult, MethodHandle result) { + // MH:oldResult,result = (ExecutionData)Double + // Execute `oldResult` but ignore its return value, then execute result and return that. // If `oldResult` (the old value) is `defaultResult`, it's bogus, so just skip it if (oldResult == DEFAULT_RESULT) { return result; } - if (result == DEFAULT_RESULT && !keepDefault) { - return oldResult; - } // Add a dummy Double parameter to the end + // MH:dummyDouble = (ExecutionData, Double)Double MethodHandle dummyDouble = MethodHandles.dropArguments( result, 1, Double.class ); // Have oldResult turn it from data->Double + // MH:doubledData = (ExecutionData, ExecutionData)Double MethodHandle doubledData = MethodHandles.collectArguments( dummyDouble, 1, oldResult ); // Deduplicate the `data` parameter + // MH:@return = (ExecutionData)Double return ExpressionHandles.dedupData(doubledData); } } diff --git a/worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/ExpressionCompiler.java b/worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/ExpressionCompiler.java index 7d2fb6799..3f3ffd8fe 100644 --- a/worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/ExpressionCompiler.java +++ b/worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/ExpressionCompiler.java @@ -29,8 +29,6 @@ import java.lang.invoke.MethodHandle; import java.lang.invoke.MethodHandles; import java.lang.invoke.MethodType; -import static com.sk89q.worldedit.internal.expression.invoke.ExpressionHandles.COMPILED_EXPRESSION_SIG; -import static com.sk89q.worldedit.internal.expression.invoke.ExpressionHandles.safeInvoke; import static java.lang.invoke.MethodType.methodType; /** @@ -45,7 +43,7 @@ public class ExpressionCompiler { private static final MethodHandle HANDLE_TO_CE_CONVERTER; static { - MethodHandle handleInvoker = MethodHandles.invoker(COMPILED_EXPRESSION_SIG); + MethodHandle handleInvoker = MethodHandles.invoker(ExpressionHandles.COMPILED_EXPRESSION_SIG); try { HANDLE_TO_CE_CONVERTER = LambdaMetafactory.metafactory( MethodHandles.lookup(), @@ -54,11 +52,11 @@ public class ExpressionCompiler { // Take a handle, to be converted to CompiledExpression HANDLE_TO_CE, // Raw signature for SAM type - COMPILED_EXPRESSION_SIG, + ExpressionHandles.COMPILED_EXPRESSION_SIG, // Handle to call the captured handle. handleInvoker, // Actual signature at invoke time - COMPILED_EXPRESSION_SIG + ExpressionHandles.COMPILED_EXPRESSION_SIG ).dynamicInvoker().asType(HANDLE_TO_CE); } catch (LambdaConversionException e) { throw new IllegalStateException("Failed to load ExpressionCompiler MetaFactory", e); @@ -68,8 +66,15 @@ public class ExpressionCompiler { public CompiledExpression compileExpression(ExpressionParser.AllStatementsContext root, Functions functions) { MethodHandle invokable = root.accept(new CompilingVisitor(functions)); - return (CompiledExpression) safeInvoke( - HANDLE_TO_CE_CONVERTER, h -> h.invoke(invokable) + // catch ReturnExpression and substitute its result + invokable = MethodHandles.catchException( + invokable, + ReturnException.class, + ExpressionHandles.RETURN_EXCEPTION_GET_RESULT + ); + MethodHandle finalInvokable = invokable; + return (CompiledExpression) ExpressionHandles.safeInvoke( + HANDLE_TO_CE_CONVERTER, h -> h.invoke(finalInvokable) ); } } diff --git a/worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/ExpressionHandles.java b/worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/ExpressionHandles.java index 0d118d6be..372ef1645 100644 --- a/worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/ExpressionHandles.java +++ b/worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/ExpressionHandles.java @@ -20,7 +20,6 @@ package com.sk89q.worldedit.internal.expression.invoke; import com.google.common.base.Throwables; -import com.sk89q.worldedit.internal.expression.BreakException; import com.sk89q.worldedit.internal.expression.CompiledExpression; import com.sk89q.worldedit.internal.expression.EvaluationException; import com.sk89q.worldedit.internal.expression.ExecutionData; @@ -70,6 +69,10 @@ class ExpressionHandles { // (double, double)Double; static final MethodHandle CALL_BINARY_OP; static final MethodHandle NEW_LS_CONSTANT; + // (Double)ReturnException; + static final MethodHandle NEW_RETURN_EXCEPTION; + // (ReturnException)Double; + static final MethodHandle RETURN_EXCEPTION_GET_RESULT; static final MethodHandle NULL_DOUBLE = dropData(constant(Double.class, null)); @@ -105,6 +108,10 @@ class ExpressionHandles { .asType(methodType(Double.class, DoubleBinaryOperator.class, double.class, double.class)); NEW_LS_CONSTANT = lookup.findConstructor(LocalSlot.Constant.class, methodType(void.class, double.class)); + NEW_RETURN_EXCEPTION = lookup.findConstructor(ReturnException.class, + methodType(void.class, Double.class)); + RETURN_EXCEPTION_GET_RESULT = lookup.findVirtual(ReturnException.class, + "getResult", methodType(Double.class)); } catch (NoSuchMethodException | IllegalAccessException e) { throw new IllegalStateException(e); } diff --git a/worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/ReturnException.java b/worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/ReturnException.java new file mode 100644 index 000000000..bd4a757fb --- /dev/null +++ b/worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/ReturnException.java @@ -0,0 +1,39 @@ +/* + * WorldEdit, a Minecraft world manipulation toolkit + * Copyright (C) sk89q + * Copyright (C) WorldEdit team and contributors + * + * This program is free software: you can redistribute it and/or modify it + * under the terms of the GNU Lesser General Public License as published by the + * Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License + * for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program. If not, see . + */ + +package com.sk89q.worldedit.internal.expression.invoke; + +/** + * Thrown when a return is encountered, to pop the stack frames and return the value easily. + * + * Should be caught by the executor. + */ +class ReturnException extends RuntimeException { + + private final Double result; + + public ReturnException(Double result) { + super("return " + result, null, true, false); + this.result = result; + } + + public Double getResult() { + return result; + } +} diff --git a/worldedit-core/src/test/java/com/sk89q/worldedit/internal/expression/ExpressionTest.java b/worldedit-core/src/test/java/com/sk89q/worldedit/internal/expression/ExpressionTest.java index 2cea279ce..3d870a9fc 100644 --- a/worldedit-core/src/test/java/com/sk89q/worldedit/internal/expression/ExpressionTest.java +++ b/worldedit-core/src/test/java/com/sk89q/worldedit/internal/expression/ExpressionTest.java @@ -19,10 +19,17 @@ package com.sk89q.worldedit.internal.expression; +import com.google.common.collect.ImmutableList; +import org.junit.jupiter.api.DynamicNode; +import org.junit.jupiter.api.DynamicTest; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestFactory; import java.time.Duration; +import java.util.List; +import java.util.stream.Stream; +import static com.sk89q.worldedit.internal.expression.ExpressionTestCase.testCase; import static java.lang.Math.atan2; import static java.lang.Math.sin; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -31,28 +38,45 @@ import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; import static org.junit.jupiter.api.Assertions.assertTrue; class ExpressionTest extends BaseExpressionTest { + + @TestFactory + public Stream testEvaluate() throws ExpressionException { + List testCases = ImmutableList.of( + // basic arithmetic + testCase("1 - 2 + 3", 2), + // unary ops + testCase("2 + +4", 6), + testCase("2 - -4", 6), + testCase("2 * -4", -8), + // check functions + testCase("sin(5)", sin(5)), + testCase("atan2(3, 4)", atan2(3, 4)), + // check conditionals + testCase("0 || 5", 5), + testCase("2 || 5", 2), + testCase("2 && 5", 5), + testCase("5 && 0", 0), + // check ternaries + testCase("false ? 1 : 2", 2), + testCase("true ? 1 : 2", 1), + // - ternary binds inside + testCase("true ? true ? 1 : 2 : 3", 1), + testCase("true ? false ? 1 : 2 : 3", 2), + testCase("false ? true ? 1 : 2 : 3", 3), + testCase("false ? false ? 1 : 2 : 3", 3), + // check return + testCase("return 1; 0", 1) + ); + return testCases.stream() + .map(testCase -> DynamicTest.dynamicTest( + testCase.getExpression(), + () -> assertEquals(testCase.getResult(), simpleEval(testCase.getExpression()), 0) + )); + } + @Test - public void testEvaluate() throws ExpressionException { - // check - assertEquals(1 - 2 + 3, simpleEval("1 - 2 + 3"), 0); - - // check unary ops - assertEquals(2 + +4, simpleEval("2 + +4"), 0); - assertEquals(2 - -4, simpleEval("2 - -4"), 0); - assertEquals(2 * -4, simpleEval("2 * -4"), 0); - - // check functions - assertEquals(sin(5), simpleEval("sin(5)"), 0); - assertEquals(atan2(3, 4), simpleEval("atan2(3, 4)"), 0); - - // check variables + void testVariables() { assertEquals(8, compile("foo+bar", "foo", "bar").evaluate(5D, 3D), 0); - - // check conditionals - assertEquals(5, simpleEval("0 || 5"), 0); - assertEquals(2, simpleEval("2 || 5"), 0); - assertEquals(5, simpleEval("2 && 5"), 0); - assertEquals(0, simpleEval("5 && 0"), 0); } @Test