Don't share singleton collections

CC tries to preserve sharing of objects when crossing the Lua/Java
boundary. For instance, if you queue (or send over a modem)
`{ tbl, tbl }`, then the returned table will have `x[1] == x[2]`.

However, this sharing causes issues with Java singletons. If some code
uses a singleton collection (such as List.of()) in multiple places, then
the same Lua table will be used in all those locations. It's incredibly
easy to accidentally, especially when using using Stream.toList.

For now, we special case these collections and don't de-duplicate them.
I'm not wild about this (it's a bit of a hack!), but I think it's
probably the easiest solution for now.

Fixes #1940

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

@@ -8,9 +8,7 @@ import dan200.computercraft.api.peripheral.IComputerAccess;
 
 import javax.annotation.Nullable;
 import java.nio.ByteBuffer;
-import java.util.Collection;
+import java.util.*;
-import java.util.Map;
-import java.util.Objects;
 
 /**
  * The result of invoking a Lua method.
@@ -55,6 +53,12 @@ public final class MethodResult {
      * <p>
      * In order to provide a custom object with methods, one may return a {@link IDynamicLuaObject}, or an arbitrary
      * class with {@link LuaFunction} annotations. Anything else will be converted to {@code nil}.
+     * <p>
+     * Shared objects in a {@link MethodResult} will preserve their sharing when converted to Lua values. For instance,
+     * {@code Map<?, ?> m = new HashMap(); return MethodResult.of(m, m); } will return two values {@code a}, {@code b}
+     * where {@code a == b}. The one exception to this is Java's singleton collections ({@link List#of()},
+     * {@link Set#of()} and {@link Map#of()}), which are always converted to new table. This is not true for other
+     * singleton collections, such as those provided by {@link Collections} or Guava.
      *
      * @param value The value to return to the calling Lua function.
      * @return A method result which returns immediately with the given value.

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

@@ -13,6 +13,7 @@ import dan200.computercraft.core.Logging;
 import dan200.computercraft.core.computer.TimeoutState;
 import dan200.computercraft.core.methods.LuaMethod;
 import dan200.computercraft.core.methods.MethodSupplier;
+import dan200.computercraft.core.util.LuaUtil;
 import dan200.computercraft.core.util.Nullability;
 import dan200.computercraft.core.util.SanitisedError;
 import org.slf4j.Logger;
@@ -183,10 +184,35 @@ public class CobaltLuaMachine implements ILuaMachine {
             return ValueFactory.valueOf(bytes);
         }
 
+        // Don't share singleton values, and instead convert them to a new table.
+        if (LuaUtil.isSingletonCollection(object)) return new LuaTable();
+
         if (values == null) values = new IdentityHashMap<>(1);
         var result = values.get(object);
         if (result != null) return result;
 
+        var wrapped = toValueWorker(object, values);
+        if (wrapped == null) {
+            LOG.warn(Logging.JAVA_ERROR, "Received unknown type '{}', returning nil.", object.getClass().getName());
+            return Constants.NIL;
+        }
+
+        values.put(object, wrapped);
+        return wrapped;
+    }
+
+    /**
+     * Convert a complex Java object (such as a collection or Lua object) to a Lua value.
+     * <p>
+     * This is a worker function for {@link #toValue(Object, IdentityHashMap)}, which handles the actual construction
+     * of values, without reading/writing from the value map.
+     *
+     * @param object The object to convert.
+     * @param values The map of Java to Lua values.
+     * @return The converted value, or {@code null} if it could not be converted.
+     * @throws LuaError If the value could not be converted.
+     */
+    private @Nullable LuaValue toValueWorker(Object object, IdentityHashMap<Object, LuaValue> values) throws LuaError {
         if (object instanceof ILuaFunction) {
             return new ResultInterpreterFunction(this, FUNCTION_METHOD, object, context, object.toString());
         }
@@ -194,15 +220,12 @@ public class CobaltLuaMachine implements ILuaMachine {
         if (object instanceof IDynamicLuaObject) {
             LuaValue wrapped = wrapLuaObject(object);
             if (wrapped == null) wrapped = new LuaTable();
-            values.put(object, wrapped);
             return wrapped;
         }
 
         if (object instanceof Map<?, ?> map) {
             var table = new LuaTable();
-            values.put(object, table);
+            for (var pair : map.entrySet()) {
-
-            for (Map.Entry<?, ?> pair : map.entrySet()) {
                 var key = toValue(pair.getKey(), values);
                 var value = toValue(pair.getValue(), values);
                 if (!key.isNil() && !value.isNil()) table.rawset(key, value);
@@ -212,27 +235,18 @@ public class CobaltLuaMachine implements ILuaMachine {
 
         if (object instanceof Collection<?> objects) {
             var table = new LuaTable(objects.size(), 0);
-            values.put(object, table);
             var i = 0;
-            for (Object child : objects) table.rawset(++i, toValue(child, values));
+            for (var child : objects) table.rawset(++i, toValue(child, values));
             return table;
         }
 
         if (object instanceof Object[] objects) {
             var table = new LuaTable(objects.length, 0);
-            values.put(object, table);
             for (var i = 0; i < objects.length; i++) table.rawset(i + 1, toValue(objects[i], values));
             return table;
         }
 
-        var wrapped = wrapLuaObject(object);
+        return wrapLuaObject(object);
-        if (wrapped != null) {
-            values.put(object, wrapped);
-            return wrapped;
-        }
-
-        LOG.warn(Logging.JAVA_ERROR, "Received unknown type '{}', returning nil.", object.getClass().getName());
-        return Constants.NIL;
     }
 
     Varargs toValues(@Nullable Object[] objects) throws LuaError {

projects/core/src/main/java/dan200/computercraft/core/util/LuaUtil.java

@@ -4,9 +4,18 @@
 
 package dan200.computercraft.core.util;
 
+import dan200.computercraft.core.lua.ILuaMachine;
+
 import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
 
 public class LuaUtil {
+    private static final List<?> EMPTY_LIST = List.of();
+    private static final Set<?> EMPTY_SET = Set.of();
+    private static final Map<?, ?> EMPTY_MAP = Map.of();
+
     public static Object[] consArray(Object value, Collection<?> rest) {
         if (rest.isEmpty()) return new Object[]{ value };
 
@@ -14,7 +23,20 @@ public class LuaUtil {
         var out = new Object[rest.size() + 1];
         out[0] = value;
         var i = 1;
-        for (Object additionalType : rest) out[i++] = additionalType;
+        for (var additionalType : rest) out[i++] = additionalType;
         return out;
     }
+
+    /**
+     * Determine whether a value is a singleton collection, such as one created with {@link List#of()}.
+     * <p>
+     * These collections are treated specially by {@link ILuaMachine} implementations: we skip sharing for them, and
+     * create a new table each time.
+     *
+     * @param value The value to test.
+     * @return Whether this is a singleton collection.
+     */
+    public static boolean isSingletonCollection(Object value) {
+        return value == EMPTY_LIST || value == EMPTY_SET || value == EMPTY_MAP;
+    }
 }

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

@@ -5,11 +5,14 @@
 package dan200.computercraft.core.computer;
 
 import com.google.common.io.CharStreams;
+import dan200.computercraft.api.lua.ILuaAPI;
+import dan200.computercraft.api.lua.LuaFunction;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 
 import java.io.InputStreamReader;
 import java.nio.charset.StandardCharsets;
+import java.util.List;
 import java.util.Objects;
 
 import static java.time.Duration.ofSeconds;
@@ -30,6 +33,26 @@ public class ComputerTest {
         });
     }
 
+    @Test
+    public void testDuplicateObjects() {
+        class CustomApi implements ILuaAPI {
+            @Override
+            public String[] getNames() {
+                return new String[]{ "custom" };
+            }
+
+            @LuaFunction
+            public final Object[] getObjects() {
+                return new Object[]{ List.of(), List.of() };
+            }
+        }
+
+        ComputerBootstrap.run("""
+            local x, y = custom.getObjects()
+            assert(x ~= y)
+            """, i -> i.addApi(new CustomApi()), 50);
+    }
+
     public static void main(String[] args) throws Exception {
         var stream = ComputerTest.class.getClassLoader().getResourceAsStream("benchmark.lua");
         try (var reader = new InputStreamReader(Objects.requireNonNull(stream), StandardCharsets.UTF_8)) {

projects/core/src/test/resources/test-rom/spec/apis/os_spec.lua

@@ -188,4 +188,31 @@ describe("The os library", function()
             expect.error(os.loadAPI, nil):eq("bad argument #1 (string expected, got nil)")
         end)
     end)
+
+    describe("os.queueEvent", function()
+        local function roundtrip(...)
+            local event_name = ("event_%08x"):format(math.random(1, 0x7FFFFFFF))
+            os.queueEvent(event_name, ...)
+            return select(2, os.pullEvent(event_name))
+        end
+
+        it("preserves references in tables", function()
+            local tbl = {}
+            local xs = roundtrip({ tbl, tbl })
+            expect(xs[1]):eq(xs[2])
+        end)
+
+        it("does not preserve references in separate args", function()
+            -- I'm not sure I like this behaviour, but it is what CC has always done.
+            local tbl = {}
+            local xs, ys = roundtrip(tbl, tbl)
+            expect(xs):ne(ys)
+        end)
+
+        it("clones objects", function()
+            local tbl = {}
+            local xs = roundtrip(tbl)
+            expect(xs):ne(tbl)
+        end)
+    end)
 end)

projects/core/src/test/resources/test-rom/spec/modules/cc/internal/syntax/lexer_spec.md

@@ -19,7 +19,7 @@ We can lex some basic comments:
 ```
 
 ```txt
-1:1-1:37 COMMENT -- A basic singleline comment
+1:1-1:29 COMMENT -- A basic singleline comment
 2:1-2:27 COMMENT --[ Not a multiline comment
 3:1-3:34 COMMENT --[= Also not a multiline comment!
 ```