From cdcd82679c8e286738afd6f19d1d9ed845dcd2aa Mon Sep 17 00:00:00 2001 From: Jonathan Coates Date: Sun, 18 Aug 2024 10:20:54 +0100 Subject: [PATCH] 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 --- .../computercraft/api/lua/MethodResult.java | 10 +++-- .../core/lua/CobaltLuaMachine.java | 44 ++++++++++++------- .../computercraft/core/util/LuaUtil.java | 24 +++++++++- .../core/computer/ComputerTest.java | 23 ++++++++++ .../resources/test-rom/spec/apis/os_spec.lua | 27 ++++++++++++ .../modules/cc/internal/syntax/lexer_spec.md | 2 +- 6 files changed, 110 insertions(+), 20 deletions(-) diff --git a/projects/core-api/src/main/java/dan200/computercraft/api/lua/MethodResult.java b/projects/core-api/src/main/java/dan200/computercraft/api/lua/MethodResult.java index aec0f8615..8b834f54c 100644 --- a/projects/core-api/src/main/java/dan200/computercraft/api/lua/MethodResult.java +++ b/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.Map; -import java.util.Objects; +import java.util.*; /** * The result of invoking a Lua method. @@ -55,6 +53,12 @@ public final class MethodResult { *

* 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}. + *

+ * 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. diff --git a/projects/core/src/main/java/dan200/computercraft/core/lua/CobaltLuaMachine.java b/projects/core/src/main/java/dan200/computercraft/core/lua/CobaltLuaMachine.java index 2b97f00fb..5d51e55e0 100644 --- a/projects/core/src/main/java/dan200/computercraft/core/lua/CobaltLuaMachine.java +++ b/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. + *

+ * 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 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 (Map.Entry pair : map.entrySet()) { + for (var 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); - 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; + return wrapLuaObject(object); } Varargs toValues(@Nullable Object[] objects) throws LuaError { diff --git a/projects/core/src/main/java/dan200/computercraft/core/util/LuaUtil.java b/projects/core/src/main/java/dan200/computercraft/core/util/LuaUtil.java index 86205d319..0ba8dd1a0 100644 --- a/projects/core/src/main/java/dan200/computercraft/core/util/LuaUtil.java +++ b/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()}. + *

+ * 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; + } } diff --git a/projects/core/src/test/java/dan200/computercraft/core/computer/ComputerTest.java b/projects/core/src/test/java/dan200/computercraft/core/computer/ComputerTest.java index 98124fc89..674816828 100644 --- a/projects/core/src/test/java/dan200/computercraft/core/computer/ComputerTest.java +++ b/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)) { diff --git a/projects/core/src/test/resources/test-rom/spec/apis/os_spec.lua b/projects/core/src/test/resources/test-rom/spec/apis/os_spec.lua index 562dcc7ff..95a50b369 100644 --- a/projects/core/src/test/resources/test-rom/spec/apis/os_spec.lua +++ b/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) diff --git a/projects/core/src/test/resources/test-rom/spec/modules/cc/internal/syntax/lexer_spec.md b/projects/core/src/test/resources/test-rom/spec/modules/cc/internal/syntax/lexer_spec.md index 8cb2e646f..9d5af0531 100644 --- a/projects/core/src/test/resources/test-rom/spec/modules/cc/internal/syntax/lexer_spec.md +++ b/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! ```