Add more expression test cases, fix bugs

Also added a few more comments + reorganized exceptions that are
invoke-internal.

(cherry picked from commit cbd686548fd62248fabbaab551a6875a14170957)

worldedit-core/src/main/java/com/sk89q/worldedit/internal/expression/invoke/BreakException.java

@@ -17,13 +17,13 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  */
 
-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);

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<MethodHandle> {
         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<MethodHandle> {
                 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<MethodHandle> {
     }
 
     // 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<MethodHandle> {
         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<MethodHandle> {
                 checkHandle(childResult, (ParserRuleContext) c);
             }
 
-            boolean returning = c instanceof ExpressionParser.ReturnStatementContext;
+            result = aggregateHandleResult(result, childResult);
-            result = aggregateResult(result, childResult, returning);
-            if (returning) {
-                return result;
-            }
         }
 
         return result;
@@ -660,25 +674,26 @@ class CompilingVisitor extends ExpressionBaseVisitor<MethodHandle> {
         throw new UnsupportedOperationException();
     }
 
-    private MethodHandle aggregateResult(MethodHandle oldResult, MethodHandle result,
+    private MethodHandle aggregateHandleResult(MethodHandle oldResult, MethodHandle result) {
-                                         boolean keepDefault) {
+        // 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);
     }
 }

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(
+        // catch ReturnExpression and substitute its result
-            HANDLE_TO_CE_CONVERTER, h -> h.invoke(invokable)
+        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)
         );
     }
 }

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);
         }

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 <http://www.sk89q.com>
+ * 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 <http://www.gnu.org/licenses/>.
+ */
+
+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;
+    }
+}

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<DynamicNode> testEvaluate() throws ExpressionException {
+        List<ExpressionTestCase> 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 {
+    void testVariables() {
-        // 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
         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