Fix overflow when converting recursive objects to Lua

In cdcd826, we rewrote the Lua
conversion function to update the "Java -> Lua" mapping after
conversion, rather than part way through.

This made the code a little cleaner (as we only updated the mapping in
one place), but is entirely incorrect — we need to store the object
first, in order to correctly handle recursive objects — otherwise we'll
just recurse infinitely (or until we overflow).

This partially reverts the above commit, while preserving the new
behaviour for singleton collections.

Fixes #1955.

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

@@ -95,11 +95,8 @@ public CobaltLuaMachine(MachineEnvironment environment, InputStream bios) throws
 
     private void addAPI(LuaState state, LuaTable globals, ILuaAPI api) throws LuaError {
         // Add the methods of an API to the global table
-        var table = wrapLuaObject(api);
-        if (table == null) {
-            LOG.warn("API {} does not provide any methods", api);
-            table = new LuaTable();
-        }
+        var table = new LuaTable();
+        if (!makeLuaObject(api, table)) LOG.warn("API {} does not provide any methods", api);
 
         var names = api.getNames();
         for (var name : names) globals.rawset(name, table);
@@ -163,13 +160,16 @@ public void close() {
         timeout.removeListener(timeoutListener);
     }
 
-    @Nullable
-    private LuaTable wrapLuaObject(Object object) {
-        var table = new LuaTable();
-        var found = luaMethods.forEachMethod(object, (target, name, method, info) ->
+    /**
+     * Populate a table with methods from an object.
+     *
+     * @param object The object to draw methods from.
+     * @param table  The table to fill.
+     * @return Whether any methods were found.
+     */
+    private boolean makeLuaObject(Object object, LuaTable table) {
+        return luaMethods.forEachMethod(object, (target, name, method, info) ->
             table.rawset(name, new ResultInterpreterFunction(this, method, target, context, name)));
-
-        return found ? table : null;
     }
 
     private LuaValue toValue(@Nullable Object object, @Nullable IdentityHashMap<Object, LuaValue> values) throws LuaError {
@@ -184,47 +184,35 @@ private LuaValue toValue(@Nullable Object object, @Nullable IdentityHashMap<Obje
             return ValueFactory.valueOf(bytes);
         }
 
-        // Don't share singleton values, and instead convert them to a new table.
-        if (LuaUtil.isSingletonCollection(object)) return new LuaTable();
-
+        // We have a more complex object, which is possibly recursive. First look up our object in the lookup map,
+        // and reuse it if present.
         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());
+            var function = new ResultInterpreterFunction(this, FUNCTION_METHOD, object, context, object.toString());
+            values.put(object, function);
+            return function;
         }
 
         if (object instanceof IDynamicLuaObject) {
-            LuaValue wrapped = wrapLuaObject(object);
-            if (wrapped == null) wrapped = new LuaTable();
-            return wrapped;
+            var table = new LuaTable();
+            makeLuaObject(object, table);
+            values.put(object, table);
+            return table;
         }
 
+        // The following objects may be recursive. In these instances, we need to be careful to store the value *before*
+        // recursing, to avoid stack overflows.
+
         if (object instanceof Map<?, ?> map) {
+            // Don't share singleton values, and instead convert them to a new table.
+            if (LuaUtil.isSingletonMap(map)) return new LuaTable();
+
             var table = new LuaTable();
+            values.put(object, table);
+
             for (var pair : map.entrySet()) {
                 var key = toValue(pair.getKey(), values);
                 var value = toValue(pair.getValue(), values);
@@ -234,19 +222,33 @@ private LuaValue toValue(@Nullable Object object, @Nullable IdentityHashMap<Obje
         }
 
         if (object instanceof Collection<?> objects) {
+            // Don't share singleton values, and instead convert them to a new table.
+            if (LuaUtil.isSingletonCollection(objects)) return new LuaTable();
+
             var table = new LuaTable(objects.size(), 0);
+            values.put(object, table);
+
             var i = 0;
             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;
         }
 
-        return wrapLuaObject(object);
+        var table = new LuaTable();
+        if (makeLuaObject(object, table)) {
+            values.put(object, table);
+            return table;
+        }
+
+        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

@@ -36,7 +36,20 @@ public static Object[] consArray(Object value, Collection<?> rest) {
      * @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;
+    public static boolean isSingletonCollection(Collection<?> value) {
+        return value == EMPTY_LIST || value == EMPTY_SET;
+    }
+
+    /**
+     * Determine whether a value is a singleton map, such as one created with {@link Map#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 map.
+     */
+    public static boolean isSingletonMap(Map<?, ?> value) {
+        return value == EMPTY_MAP;
     }
 }

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

@@ -202,6 +202,14 @@ describe("The os library", function()
             expect(xs[1]):eq(xs[2])
         end)
 
+        it("handles recursive tables", function()
+            local tbl = {}
+            tbl[1] = tbl
+
+            local xs = roundtrip(tbl)
+            expect(xs):eq(xs[1])
+        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 = {}