Properly scope IArguments to the current function call

This is a horrible commit: It's a breaking change in a pretty subtle
way, which means it won't be visible while updating. Fortunately I think
the only mod on 1.19.4 is Plethora, but other mods (Mek, Advanced
Peripherals) may be impacted when they update. Sorry!

For some motivation behind the original issue:

The default IArguments implementation (VarargArguments) lazily converts
Lua arguments to Java ones. This is mostly important when passing tables
to Java functions, as we can avoid the conversion entirely if the
function uses IArguments.getTableUnsafe.

However, this lazy conversion breaks down if IArguments is accessed on a
separate thread, as Lua values are not thread-safe. Thus we need to
perform this conversion before the cross-thread sharing occurs.

Now, ideally this would be an implementation detail and entirely
invisible to the user. One approach here would be to only perform this
lazy conversion for methods annotated with @LuaFunction(unsafe=true),
and have it be eager otherwise.

However, the peripheral API gets in the way here, as it means we can no
longer inspect the "actual" method being invoked. And so, alas, this
must leak into the public API.

TLDR: If you're getting weird errors about scope, add an
IArguments.escapes() call before sharing the arguments between threads.

Closes #1384

projects/common/src/main/java/dan200/computercraft/shared/turtle/apis/TurtleAPI.java

@@ -219,7 +219,7 @@ public class TurtleAPI implements ILuaAPI {
      * @cc.since 1.4
      */
     @LuaFunction
-    public final MethodResult place(IArguments args) {
+    public final MethodResult place(IArguments args) throws LuaException {
         return trackCommand(new TurtlePlaceCommand(InteractDirection.FORWARD, args.getAll()));
     }
 
@@ -235,7 +235,7 @@ public class TurtleAPI implements ILuaAPI {
      * @see #place For more information about placing items.
      */
     @LuaFunction
-    public final MethodResult placeUp(IArguments args) {
+    public final MethodResult placeUp(IArguments args) throws LuaException {
         return trackCommand(new TurtlePlaceCommand(InteractDirection.UP, args.getAll()));
     }
 
@@ -251,7 +251,7 @@ public class TurtleAPI implements ILuaAPI {
      * @see #place For more information about placing items.
      */
     @LuaFunction
-    public final MethodResult placeDown(IArguments args) {
+    public final MethodResult placeDown(IArguments args) throws LuaException {
         return trackCommand(new TurtlePlaceCommand(InteractDirection.DOWN, args.getAll()));
     }
 

projects/core-api/src/main/java/dan200/computercraft/api/lua/IArguments.java

@@ -35,9 +35,15 @@ public interface IArguments {
      *
      * @param index The argument number.
      * @return The argument's value, or {@code null} if not present.
+     * @throws LuaException          If the argument cannot be converted to Java. This should be thrown in extraneous
+     *                               circumstances (if the conversion would allocate too much memory) and should
+     *                               <em>not</em> be thrown if the original argument is not present or is an unsupported
+     *                               data type (such as a function or userdata).
+     * @throws IllegalStateException If accessing these arguments outside the scope of the original function. See
+     *                               {@link #escapes()}.
      */
     @Nullable
-    Object get(int index);
+    Object get(int index) throws LuaException;
 
     /**
      * Get the type name of the argument at the specific index.
@@ -49,9 +55,7 @@ public interface IArguments {
      * @return The name of this type.
      * @see LuaValues#getType(Object)
      */
-    default String getType(int index) {
-        return LuaValues.getType(get(index));
-    }
+    String getType(int index);
 
     /**
      * Drop a number of arguments. The returned arguments instance will access arguments at position {@code i + count},
@@ -62,7 +66,14 @@ public interface IArguments {
      */
     IArguments drop(int count);
 
-    default Object[] getAll() {
+    /**
+     * Get an array containing all as {@link Object}s.
+     *
+     * @return All arguments.
+     * @throws LuaException If an error occurred while fetching an argument.
+     * @see #get(int) To get a single argument.
+     */
+    default Object[] getAll() throws LuaException {
         var result = new Object[count()];
         for (var i = 0; i < result.length; i++) result[i] = get(i);
         return result;
@@ -421,4 +432,26 @@ public interface IArguments {
     default Map<?, ?> optTable(int index, @Nullable Map<Object, Object> def) throws LuaException {
         return optTable(index).orElse(def);
     }
+
+    /**
+     * Create a version of these arguments which escapes the scope of the current function call.
+     * <p>
+     * Some {@link IArguments} implementations provide a view over the underlying Lua data structures, allowing for
+     * zero-copy implementations of some methods (such as {@link #getTableUnsafe(int)} or {@link #getBytes(int)}).
+     * However, this means the arguments can only be accessed inside the current function call.
+     * <p>
+     * If the arguments escape the scope of the current call (for instance, are later accessed on the main server
+     * thread), then these arguments must be marked as "escaping", which may choose to perform a copy of the underlying
+     * arguments.
+     * <p>
+     * If you are using {@link LuaFunction#mainThread()}, this will be done automatically. However, if you call
+     * {@link ILuaContext#issueMainThreadTask(LuaTask)} (or similar), then you will need to mark arguments as escaping
+     * yourself.
+     *
+     * @return An {@link IArguments} instance which can escape the current scope. May be {@code this}.
+     * @throws LuaException For the same reasons as {@link #get(int)}.
+     */
+    default IArguments escapes() throws LuaException {
+        return this;
+    }
 }

projects/core-api/src/main/java/dan200/computercraft/api/lua/ObjectArguments.java

@@ -51,6 +51,11 @@ public final class ObjectArguments implements IArguments {
         return index >= args.size() ? null : args.get(index);
     }
 
+    @Override
+    public String getType(int index) {
+        return LuaValues.getType(get(index));
+    }
+
     @Override
     public Object[] getAll() {
         return args.toArray();

projects/core/src/main/java/dan200/computercraft/core/apis/OSAPI.java

@@ -136,7 +136,7 @@ public class OSAPI implements ILuaAPI {
      * @cc.see os.pullEvent To pull the event queued
      */
     @LuaFunction
-    public final void queueEvent(String name, IArguments args) {
+    public final void queueEvent(String name, IArguments args) throws LuaException {
         apiEnvironment.queueEvent(name, args.drop(1).getAll());
     }
 
@@ -165,8 +165,8 @@ public class OSAPI implements ILuaAPI {
      * timer from firing.
      *
      * @param token The ID of the timer to cancel.
-     * @see #startTimer To start a timer.
      * @cc.since 1.6
+     * @see #startTimer To start a timer.
      */
     @LuaFunction
     public final void cancelTimer(int token) {
@@ -351,7 +351,7 @@ public class OSAPI implements ILuaAPI {
      * January 1970 in the UTC timezone.
      * * If called with {@code local}, returns the number of milliseconds since 1
      * January 1970 in the server's local timezone.
-     *
+     * <p>
      * :::info
      * The {@code ingame} time zone assumes that one Minecraft day consists of 86,400,000
      * milliseconds. Since one in-game day is much faster than a real day (20 minutes), this

projects/core/src/main/java/dan200/computercraft/core/asm/LuaMethod.java

@@ -10,7 +10,7 @@ import java.util.Collections;
 
 public interface LuaMethod {
     Generator<LuaMethod> GENERATOR = new Generator<>(LuaMethod.class, Collections.singletonList(ILuaContext.class),
-        m -> (target, context, args) -> context.executeMainThreadTask(() -> ResultHelpers.checkNormalResult(m.apply(target, context, args)))
+        m -> (target, context, args) -> context.executeMainThreadTask(() -> ResultHelpers.checkNormalResult(m.apply(target, context, args.escapes())))
     );
 
     IntCache<LuaMethod> DYNAMIC = new IntCache<>(

projects/core/src/main/java/dan200/computercraft/core/asm/PeripheralMethod.java

@@ -15,7 +15,7 @@ import java.util.Arrays;
 
 public interface PeripheralMethod {
     Generator<PeripheralMethod> GENERATOR = new Generator<>(PeripheralMethod.class, Arrays.asList(ILuaContext.class, IComputerAccess.class),
-        m -> (target, context, computer, args) -> context.executeMainThreadTask(() -> ResultHelpers.checkNormalResult(m.apply(target, context, computer, args)))
+        m -> (target, context, computer, args) -> context.executeMainThreadTask(() -> ResultHelpers.checkNormalResult(m.apply(target, context, computer, args.escapes())))
     );
 
     IntCache<PeripheralMethod> DYNAMIC = new IntCache<>(

projects/core/src/main/java/dan200/computercraft/core/lua/TableImpl.java

@@ -103,7 +103,7 @@ class TableImpl implements dan200.computercraft.api.lua.LuaTable<Object, Object>
     }
 
     private void checkValid() {
-        if (arguments.closed) {
+        if (arguments.isClosed()) {
             throw new IllegalStateException("Cannot use LuaTable after IArguments has been released");
         }
     }

projects/core/src/main/java/dan200/computercraft/core/lua/VarargArguments.java

@@ -7,6 +7,8 @@ package dan200.computercraft.core.lua;
 import dan200.computercraft.api.lua.IArguments;
 import dan200.computercraft.api.lua.LuaException;
 import dan200.computercraft.api.lua.LuaValues;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import org.squiddev.cobalt.*;
 
 import javax.annotation.Nullable;
@@ -16,20 +18,64 @@ import java.util.Optional;
 import static org.squiddev.cobalt.Constants.NAME;
 
 final class VarargArguments implements IArguments {
-    private static final VarargArguments EMPTY = new VarargArguments(Constants.NONE);
+    private static final Logger LOG = LoggerFactory.getLogger(VarargArguments.class);
+
+    private static final VarargArguments EMPTY = new VarargArguments(Constants.NONE);
+    private static boolean reportedIllegalGet;
+
+    static {
+        EMPTY.escapes = EMPTY.closed = true;
+    }
 
-    boolean closed;
     private final Varargs varargs;
-    private @Nullable Object[] cache;
+
+    private volatile boolean closed;
+    private final VarargArguments root;
+
+    private @Nullable ArraySlice<Object> cache;
+    private @Nullable ArraySlice<String> typeNames;
+
+    private boolean escapes;
 
     private VarargArguments(Varargs varargs) {
         this.varargs = varargs;
+        root = this;
+    }
+
+    private VarargArguments(Varargs varargs, VarargArguments root, int offset) {
+        this.varargs = varargs;
+        this.root = root;
+        escapes = root.escapes;
+        cache = root.cache == null ? null : root.cache.drop(offset);
+        typeNames = root.typeNames == null ? null : root.typeNames.drop(offset);
     }
 
     static VarargArguments of(Varargs values) {
         return values == Constants.NONE ? EMPTY : new VarargArguments(values);
     }
 
+    boolean isClosed() {
+        return root.closed;
+    }
+
+    private void checkAccessible() {
+        if (isClosed() && !escapes) throwInaccessible();
+    }
+
+    private void throwInaccessible() {
+        var error = new IllegalStateException("Function arguments have escaped their original scope.");
+        if (!reportedIllegalGet) {
+            reportedIllegalGet = true;
+            LOG.error(
+                "A function attempted to access arguments outside the scope of the original function. This is probably " +
+                    "caused by the function scheduling work on the main thread. You may need to call IArguments.escapes().",
+                error
+            );
+        }
+
+        throw error;
+    }
+
     @Override
     public int count() {
         return varargs.count();
@@ -38,26 +84,37 @@ final class VarargArguments implements IArguments {
     @Nullable
     @Override
     public Object get(int index) {
+        checkAccessible();
         if (index < 0 || index >= varargs.count()) return null;
 
         var cache = this.cache;
         if (cache == null) {
-            cache = this.cache = new Object[varargs.count()];
+            cache = this.cache = new ArraySlice<>(new Object[varargs.count()], 0);
         } else {
-            var existing = cache[index];
+            var existing = cache.get(index);
             if (existing != null) return existing;
         }
 
-        return cache[index] = CobaltLuaMachine.toObject(varargs.arg(index + 1), null);
+        var arg = varargs.arg(index + 1);
+
+        // This holds as either a) the arguments are not closed or b) the arguments were escaped, in which case
+        // tables should have been converted already.
+        assert !isClosed() || !(arg instanceof LuaTable) : "Converting a LuaTable after arguments were closed.";
+
+        var converted = CobaltLuaMachine.toObject(arg, null);
+        cache.set(index, converted);
+        return converted;
     }
 
     @Override
     public String getType(int index) {
+        checkAccessible();
+
         var value = varargs.arg(index + 1);
-        if (value instanceof LuaTable || value instanceof LuaUserdata) {
-            var metatable = value.getMetatable(null);
-            if (metatable != null && metatable.rawget(NAME) instanceof LuaString s) return s.toString();
-        }
+
+        // If we've escaped, read it from the precomputed list, otherwise get the custom name.
+        var name = escapes ? (typeNames == null ? null : typeNames.get(index)) : getCustomType(value);
+        if (name != null) return name;
 
         return value.typeName();
     }
@@ -66,11 +123,15 @@ final class VarargArguments implements IArguments {
     public IArguments drop(int count) {
         if (count < 0) throw new IllegalStateException("count cannot be negative");
         if (count == 0) return this;
-        return new VarargArguments(varargs.subargs(count + 1));
+
+        var newArgs = varargs.subargs(count + 1);
+        if (newArgs == Constants.NONE) return EMPTY;
+        return new VarargArguments(newArgs, this, count);
     }
 
     @Override
     public double getDouble(int index) throws LuaException {
+        checkAccessible();
         var value = varargs.arg(index + 1);
         if (!(value instanceof LuaNumber)) throw LuaValues.badArgument(index, "number", value.typeName());
         return value.toDouble();
@@ -78,6 +139,7 @@ final class VarargArguments implements IArguments {
 
     @Override
     public long getLong(int index) throws LuaException {
+        checkAccessible();
         var value = varargs.arg(index + 1);
         if (!(value instanceof LuaNumber)) throw LuaValues.badArgument(index, "number", value.typeName());
         return value instanceof LuaInteger ? value.toInteger() : (long) LuaValues.checkFinite(index, value.toDouble());
@@ -85,6 +147,7 @@ final class VarargArguments implements IArguments {
 
     @Override
     public ByteBuffer getBytes(int index) throws LuaException {
+        checkAccessible();
         var value = varargs.arg(index + 1);
         if (!(value instanceof LuaString str)) throw LuaValues.badArgument(index, "string", value.typeName());
         return str.toBuffer();
@@ -92,6 +155,7 @@ final class VarargArguments implements IArguments {
 
     @Override
     public Optional<ByteBuffer> optBytes(int index) throws LuaException {
+        checkAccessible();
         var value = varargs.arg(index + 1);
         if (value.isNil()) return Optional.empty();
         if (!(value instanceof LuaString str)) throw LuaValues.badArgument(index, "string", value.typeName());
@@ -100,9 +164,7 @@ final class VarargArguments implements IArguments {
 
     @Override
     public dan200.computercraft.api.lua.LuaTable<?, ?> getTableUnsafe(int index) throws LuaException {
-        if (closed) {
-            throw new IllegalStateException("Cannot use getTableUnsafe after IArguments has been closed.");
-        }
+        if (isClosed()) throw new IllegalStateException("Cannot use getTableUnsafe after IArguments has been closed.");
 
         var value = varargs.arg(index + 1);
         if (!(value instanceof LuaTable)) throw LuaValues.badArgument(index, "table", value.typeName());
@@ -111,9 +173,7 @@ final class VarargArguments implements IArguments {
 
     @Override
     public Optional<dan200.computercraft.api.lua.LuaTable<?, ?>> optTableUnsafe(int index) throws LuaException {
-        if (closed) {
-            throw new IllegalStateException("Cannot use optTableUnsafe after IArguments has been closed.");
-        }
+        if (isClosed()) throw new IllegalStateException("Cannot use optTableUnsafe after IArguments has been closed.");
 
         var value = varargs.arg(index + 1);
         if (value.isNil()) return Optional.empty();
@@ -121,7 +181,61 @@ final class VarargArguments implements IArguments {
         return Optional.of(new TableImpl(this, (LuaTable) value));
     }
 
-    public void close() {
+    @Override
+    public IArguments escapes() {
+        if (escapes) return this;
+
+        var cache = this.cache;
+        var typeNames = this.typeNames;
+
+        for (int i = 0, count = varargs.count(); i < count; i++) {
+            var arg = varargs.arg(i + 1);
+
+            // Convert tables.
+            if (arg instanceof LuaTable) {
+                if (cache == null) cache = new ArraySlice<>(new Object[count], 0);
+                cache.set(i, CobaltLuaMachine.toObject(arg, null));
+            }
+
+            // Fetch custom type names.
+            var typeName = getCustomType(arg);
+            if (typeName != null) {
+                if (typeNames == null) typeNames = new ArraySlice<>(new String[count], 0);
+                typeNames.set(i, typeName);
+            }
+        }
+
+        escapes = true;
+        this.cache = cache;
+        this.typeNames = typeNames;
+        return this;
+    }
+
+    void close() {
         closed = true;
     }
+
+    private static @Nullable String getCustomType(LuaValue arg) {
+        if (!(arg instanceof LuaTable) && !(arg instanceof LuaUserdata)) return null;
+
+        var metatable = arg.getMetatable(null);
+        return metatable != null && metatable.rawget(NAME) instanceof LuaString s ? s.toString() : null;
+    }
+
+    private record ArraySlice<T>(T[] array, int offset) {
+        // FIXME: We should be able to remove the @Nullables if we update NullAway.
+
+        @Nullable
+        T get(int index) {
+            return array[offset + index];
+        }
+
+        void set(int index, @Nullable T value) {
+            array[offset + index] = value;
+        }
+
+        ArraySlice<T> drop(int count) {
+            return new ArraySlice<>(array, offset + count);
+        }
+    }
 }

projects/core/src/test/java/dan200/computercraft/core/computer/ComputerBootstrap.java

@@ -118,7 +118,7 @@ public class ComputerBootstrap {
         }
 
         @LuaFunction
-        public final void log(IArguments arguments) {
+        public final void log(IArguments arguments) throws LuaException {
             LOG.info("[Computer] {}", Arrays.toString(arguments.getAll()));
         }
 

projects/core/src/test/java/dan200/computercraft/core/lua/VarargArgumentsTest.java

@@ -0,0 +1,63 @@
+// SPDX-FileCopyrightText: 2023 The CC: Tweaked Developers
+//
+// SPDX-License-Identifier: MPL-2.0
+
+package dan200.computercraft.core.lua;
+
+import dan200.computercraft.api.lua.LuaException;
+import org.junit.jupiter.api.Test;
+import org.squiddev.cobalt.Constants;
+import org.squiddev.cobalt.LuaTable;
+import org.squiddev.cobalt.ValueFactory;
+
+import java.util.Map;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+class VarargArgumentsTest {
+    private static LuaTable tableWithCustomType() {
+        var metatable = new LuaTable();
+        metatable.rawset(Constants.NAME, ValueFactory.valueOf("some type"));
+
+        var table = new LuaTable();
+        table.setMetatable(null, metatable);
+        return table;
+    }
+
+    @Test
+    public void testGet() {
+        var args = VarargArguments.of(tableWithCustomType());
+        assertEquals(Map.of(), args.get(0));
+        assertEquals("some type", args.getType(0));
+    }
+
+    @Test
+    public void testGetAfterEscape() {
+        var args = VarargArguments.of(tableWithCustomType());
+        args.escapes();
+        args.close();
+
+        assertEquals(Map.of(), args.get(0));
+        assertEquals("some type", args.getType(0));
+    }
+
+    @Test
+    public void testGetAfterEscapeDrop() throws LuaException {
+        var args = VarargArguments.of(ValueFactory.varargsOf(Constants.NIL, tableWithCustomType()));
+        args.escapes();
+        args.close();
+
+        assertEquals(Map.of(), args.drop(1).get(0));
+        assertEquals("some type", args.drop(1).getType(0));
+    }
+
+    @Test
+    public void testGetAfterClose() {
+        var args = VarargArguments.of(tableWithCustomType());
+        args.close();
+
+        assertThrows(IllegalStateException.class, () -> args.get(0));
+        assertThrows(IllegalStateException.class, () -> args.getType(0));
+    }
+}