get rid of leaked abstraction

aggyomfg · aggyomfg · commit 96c9156bf371 · 2025-10-05T22:50:10.000+04:00
diff --git a/assets/tests/iter/hashmap.lua b/assets/tests/iter/hashmap.lua
@@ -3,13 +3,17 @@ local res = world.get_resource(res_type)
 
 local map = res.string_map
 
+local iterator = map:iter()
 local count = 0
 local found_keys = {}
 
---- Iterate over PartialReflect refs using pairs
-for key, value in pairs(map) do
+local result = iterator()
+while result ~= nil do
+    local key = result[1]
+    local value = result[2]
     count = count + 1
     found_keys[key] = value
+    result = iterator()
 end
 
 assert(count == 2, "Expected 2 entries, got " .. count)
diff --git a/assets/tests/iter/hashmap_ipairs.lua b/assets/tests/iter/hashmap_ipairs.lua
@@ -6,8 +6,8 @@ local map = res.string_map
 local count = 0
 local found_keys = {}
 
--- ipairs_clone on a map returns (index, [key, value]) where value is a list
-for i, entry in map:ipairs_clone() do
+-- ipairs on a map returns (index, [key, value]) where value is a list
+for i, entry in map:ipairs() do
     assert(i == count + 1, "Index should be sequential: expected " .. (count + 1) .. ", got " .. i)
 
     -- entry should be a list with [key, value]
diff --git a/assets/tests/iter/hashmap_pairs.lua b/assets/tests/iter/hashmap_pairs.lua
@@ -6,8 +6,7 @@ local map = res.string_map
 local count = 0
 local found_keys = {}
 
--- Use pairs_clone to loop over Reflect values
-for key, value in map:pairs_clone() do
+for key, value in map:pairs() do
     count = count + 1
     found_keys[key] = value
 end
diff --git a/assets/tests/iter/vec.lua b/assets/tests/iter/vec.lua
@@ -1,10 +1,14 @@
 local res_type = world.get_type_by_name("TestResourceWithVariousFields")
 local res = world.get_resource(res_type)
 
-iterated_vals = {}
-for v in pairs(res.vec_usize) do
-    iterated_vals[#iterated_vals + 1] = v
+local iterated_vals = {}
+local iterator = res.vec_usize:iter()
+local result = iterator()
+while result ~= nil do
+    iterated_vals[#iterated_vals + 1] = result
+    result = iterator()
 end
+
 assert(#iterated_vals == 5, "Length is not 5")
 assert(iterated_vals[1] == 1, "First value is not 1")
 assert(iterated_vals[2] == 2, "Second value is not 2")
diff --git a/assets/tests/iter/vec.rhai b/assets/tests/iter/vec.rhai
@@ -2,9 +2,16 @@ let res_type = world.get_type_by_name.call("TestResourceWithVariousFields");
 let res = world.get_resource.call(res_type);
 
 let iterated_vals = [];
+let iterator = res.vec_usize.iter();
 
-for v in res.vec_usize {
-    iterated_vals.push(v);
+loop {
+    let result = iterator.next();
+    
+    if result == () {
+        break;
+    }
+    
+    iterated_vals.push(result);
 }
 
 assert(iterated_vals.len == 5, "Length is not 5");
diff --git a/assets/tests/iter/vec_ipairs.lua b/assets/tests/iter/vec_ipairs.lua
@@ -2,7 +2,7 @@ local res_type = world.get_type_by_name("TestResourceWithVariousFields")
 local res = world.get_resource(res_type)
 
 local iterated_vals = {}
-for i, v in res.vec_usize:ipairs_clone() do
+for i, v in res.vec_usize:ipairs() do
     assert(i == #iterated_vals + 1, "Index mismatch: expected " .. (#iterated_vals + 1) .. ", got " .. i)
     iterated_vals[#iterated_vals + 1] = v
 end
diff --git a/assets/tests/iter/vec_pairs.lua b/assets/tests/iter/vec_pairs.lua
@@ -2,7 +2,7 @@ local res_type = world.get_type_by_name("TestResourceWithVariousFields")
 local res = world.get_resource(res_type)
 
 local iterated_vals = {}
-for v in res.vec_usize:pairs_clone() do
+for v in res.vec_usize:pairs() do
     iterated_vals[#iterated_vals + 1] = v
 end
 
diff --git a/assets/tests/iter_clone/hashmap.lua b/assets/tests/iter_clone/hashmap.lua
diff --git a/assets/tests/iter_clone/hashmap.rhai b/assets/tests/iter_clone/hashmap.rhai
diff --git a/assets/tests/iter_clone/vec.lua b/assets/tests/iter_clone/vec.lua
diff --git a/assets/tests/iter_clone/vec.rhai b/assets/tests/iter_clone/vec.rhai
diff --git a/crates/bevy_mod_scripting_functions/src/core.rs b/crates/bevy_mod_scripting_functions/src/core.rs
@@ -606,14 +606,15 @@ impl ReflectReference {
     }
 
     /// Gets and clones the value under the specified key if the underlying type is a map type.
+    /// Uses `from_reflect` to create a fully reflected clone that can be used with all operations.
     ///
     /// Arguments:
     /// * `ctxt`: The function call context.
     /// * `reference`: The reference to index into.
     /// * `key`: The key to index with.
     /// Returns:
     /// * `value`: The value at the key, if the reference is a map.
-    fn map_get_clone(
+    fn map_get(
         ctxt: FunctionCallContext,
         reference: ReflectReference,
         key: ScriptValue,
@@ -645,53 +646,6 @@ impl ReflectReference {
         })?
     }
 
-    /// Gets the value reference under the specified key using dynamic reflection if the underlying type is a map type.
-    /// This method uses `to_dynamic()` which creates a dynamic representation that only implements `PartialReflect`.
-    /// Note: Values returned by this method cannot be used with asset operations that require full `Reflect` trait.
-    ///
-    /// Arguments:
-    /// * `ctxt`: The function call context.
-    /// * `reference`: The reference to index into.
-    /// * `key`: The key to index with.
-    /// Returns:
-    /// * `value`: The dynamic value at the key, if the reference is a map.
-    fn map_get(
-        ctxt: FunctionCallContext,
-        reference: ReflectReference,
-        key: ScriptValue,
-    ) -> Result<Option<ScriptValue>, InteropError> {
-        profiling::function_scope!("map_get_dynamic");
-        let world = ctxt.world()?;
-        let key = <Box<dyn PartialReflect>>::from_script_ref(
-            reference.key_type_id(world.clone())?.ok_or_else(|| {
-                InteropError::unsupported_operation(
-                    reference.tail_type_id(world.clone()).unwrap_or_default(),
-                    Some(Box::new(key.clone())),
-                    "Could not get key type id. Are you trying to index into a type that's not a map?".to_owned(),
-                )
-            })?,
-            key,
-            world.clone(),
-        )?;
-        reference.with_reflect(world.clone(), |s| {
-            s.try_map_get(key.as_ref()).and_then(|maybe_value| {
-                maybe_value
-                    .map(|value| {
-                        let reference = {
-                            let allocator = world.allocator();
-                            let mut allocator_guard = allocator.write();
-                            ReflectReference::new_allocated_boxed_parial_reflect(
-                                value.to_dynamic(),
-                                &mut *allocator_guard,
-                            )?
-                        };
-                        ReflectReference::into_script_ref(reference, world)
-                    })
-                    .transpose()
-            })
-        })?
-    }
-
     /// Pushes the value into the reference, if the reference is an appropriate container type.
     ///
     /// Arguments:
@@ -859,67 +813,8 @@ impl ReflectReference {
     }
 
     /// Iterates over the reference, if the reference is an appropriate container type.
+    /// Uses `from_reflect` to create fully reflected clones that can be used with all operations.
     ///
-    /// Returns an "next" iterator function.
-    ///
-    /// The iterator function should be called until it returns `nil` to signal the end of the iteration.
-    ///
-    /// Arguments:
-    /// * `ctxt`: The function call context.
-    /// * `reference`: The reference to iterate over.
-    /// Returns:
-    /// * `iter`: The iterator function.
-    fn iter(
-        ctxt: FunctionCallContext,
-        reference: ReflectReference,
-    ) -> Result<DynamicScriptFunctionMut, InteropError> {
-        profiling::function_scope!("iter");
-        let world = ctxt.world()?;
-        let mut len = reference.len(world.clone())?.unwrap_or_default();
-        // Check if this is a Map type
-        let is_map = reference.with_reflect(world.clone(), |r| {
-            matches!(r.reflect_ref(), ReflectRef::Map(_))
-        })?;
-
-        if is_map {
-            // Use special map iterator that doesn't rely on path resolution
-            let mut map_iter = reference.into_map_iter();
-            let iter_function = move || {
-                if len == 0 {
-                    return Ok(ScriptValue::Unit);
-                }
-                len -= 1;
-                let world = ThreadWorldContainer.try_get_world()?;
-                match map_iter.next_ref(world.clone())? {
-                    Some((key_ref, value_ref)) => {
-                        // Return both key and value as a List for Lua's pairs() to unpack
-                        let key_value = ReflectReference::into_script_ref(key_ref, world.clone())?;
-                        let value_value = ReflectReference::into_script_ref(value_ref, world)?;
-                        Ok(ScriptValue::List(vec![key_value, value_value]))
-                    }
-                    None => Ok(ScriptValue::Unit),
-                }
-            };
-            Ok(iter_function.into_dynamic_script_function_mut())
-        } else {
-            let mut infinite_iter = reference.into_iter_infinite();
-            let iter_function = move || {
-                if len == 0 {
-                    return Ok(ScriptValue::Unit);
-                }
-                len -= 1;
-                let world = ThreadWorldContainer.try_get_world()?;
-                let (next_ref, _) = infinite_iter.next_ref();
-                // we stop once the reflection path is invalid
-                ReflectReference::into_script_ref(next_ref, world)
-            };
-            Ok(iter_function.into_dynamic_script_function_mut())
-        }
-    }
-
-    /// Iterates over the reference with cloned values, if the reference is an appropriate container type.
-    ///
-    /// This method clones the actual values using `from_reflect` instead of creating dynamic references.
     /// Returns an "next" iterator function that returns fully reflected values (Box<dyn Reflect>).
     ///
     /// The iterator function should be called until it returns `nil` to signal the end of the iteration.
@@ -929,11 +824,11 @@ impl ReflectReference {
     /// * `reference`: The reference to iterate over.
     /// Returns:
     /// * `iter`: The iterator function that returns cloned values.
-    fn iter_clone(
+    fn iter(
         ctxt: FunctionCallContext,
         reference: ReflectReference,
     ) -> Result<DynamicScriptFunctionMut, InteropError> {
-        profiling::function_scope!("iter_clone");
+        profiling::function_scope!("iter");
         let world = ctxt.world()?;
         let mut len = reference.len(world.clone())?.unwrap_or_default();
         // Check if this is a Map type
diff --git a/crates/languages/bevy_mod_scripting_lua/src/bindings/reference.rs b/crates/languages/bevy_mod_scripting_lua/src/bindings/reference.rs
@@ -348,14 +348,14 @@ impl UserData for LuaReflectReference {
             }
         });
 
-        m.add_method("pairs_clone", |lua, s: &LuaReflectReference, _args: ()| {
-            profiling::function_scope!("pairs_clone");
+        m.add_method("pairs", |lua, s: &LuaReflectReference, _args: ()| {
+            profiling::function_scope!("pairs");
             let world = ThreadWorldContainer
                 .try_get_world()
                 .map_err(IntoMluaError::to_lua_error)?;
 
             let iter_func = world
-                .lookup_function([TypeId::of::<ReflectReference>()], "iter_clone")
+                .lookup_function([TypeId::of::<ReflectReference>()], "iter")
                 .map_err(|f| {
                     InteropError::missing_function(f, TypeId::of::<ReflectReference>().into())
                 })
@@ -388,18 +388,18 @@ impl UserData for LuaReflectReference {
                         }
                     })
                 }
-                _ => Err(mlua::Error::RuntimeError("iter_clone function did not return a FunctionMut".to_string()))
+                _ => Err(mlua::Error::RuntimeError("iter function did not return a FunctionMut".to_string()))
             }
         });
 
-        m.add_method("ipairs_clone", |lua, s: &LuaReflectReference, _args: ()| {
-            profiling::function_scope!("ipairs_clone");
+        m.add_method("ipairs", |lua, s: &LuaReflectReference, _args: ()| {
+            profiling::function_scope!("ipairs");
             let world = ThreadWorldContainer
                 .try_get_world()
                 .map_err(IntoMluaError::to_lua_error)?;
 
             let iter_func = world
-                .lookup_function([TypeId::of::<ReflectReference>()], "iter_clone")
+                .lookup_function([TypeId::of::<ReflectReference>()], "iter")
                 .map_err(|f| {
                     InteropError::missing_function(f, TypeId::of::<ReflectReference>().into())
                 })
@@ -432,7 +432,7 @@ impl UserData for LuaReflectReference {
                         }
                     })
                 }
-                _ => Err(mlua::Error::RuntimeError("iter_clone function did not return a FunctionMut".to_string()))
+                _ => Err(mlua::Error::RuntimeError("iter function did not return a FunctionMut".to_string()))
             }
         });
 
