From ec380802a8b30626d4624258327492875aff25e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=88=B1=E7=89=B9=E6=AF=8F=E7=BE=8A=E9=98=9F?= Date: Sat, 27 Jun 2026 17:15:32 +0800 Subject: [PATCH] =?UTF-8?q?fix:=20for-in=20=E6=95=B4=E6=95=B0=E7=B4=A2?= =?UTF-8?q?=E5=BC=95=E5=8D=87=E5=BA=8F+=E5=AD=97=E7=AC=A6=E4=B8=B2?= =?UTF-8?q?=E6=8F=92=E5=85=A5=E5=BA=8F=E6=9E=9A=E4=B8=BE=EF=BC=8Cnull/unde?= =?UTF-8?q?fined=20=E7=A9=BA=E9=81=8D=E5=8E=86?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- crates/oxide_vm/src/session_gc.rs | 2 +- crates/oxide_vm/src/vm.rs | 8 +- crates/oxide_vm/src/vm_dispatch_misc.rs | 34 +++++++- .../tests/for_in_null_undefined_tests.rs | 58 +++++++++++++ .../oxide_vm/tests/for_in_ordering_tests.rs | 82 +++++++++++++++++++ 5 files changed, 176 insertions(+), 8 deletions(-) create mode 100644 crates/oxide_vm/tests/for_in_null_undefined_tests.rs create mode 100644 crates/oxide_vm/tests/for_in_ordering_tests.rs diff --git a/crates/oxide_vm/src/session_gc.rs b/crates/oxide_vm/src/session_gc.rs index e86b910..39a8b73 100644 --- a/crates/oxide_vm/src/session_gc.rs +++ b/crates/oxide_vm/src/session_gc.rs @@ -498,7 +498,7 @@ fn rewrite_vm_roots(vm: &mut Vm, forwarding: &HashMap<*mut JsObject, *mut JsObje } // SAFETY: for_in_iters stores live iterator pointers owned by the current VM epoch. unsafe { - for value in (*(*iter)).keys.iter_mut() { + for (value, _si) in (*(*iter)).keys.iter_mut() { *value = rewrite_forwarded_value(*value, forwarding); } } diff --git a/crates/oxide_vm/src/vm.rs b/crates/oxide_vm/src/vm.rs index 9e69089..9a2b7fb 100644 --- a/crates/oxide_vm/src/vm.rs +++ b/crates/oxide_vm/src/vm.rs @@ -103,7 +103,9 @@ pub struct CallFrame { } pub struct ForInIter<'bump> { - pub keys: bumpalo::collections::Vec<'bump, JsValue>, + /// Each key paired with its string-intern id, so for-in can sort + /// integer-index keys ahead of string keys without re-interning. + pub keys: bumpalo::collections::Vec<'bump, (JsValue, u32)>, pub index: usize, } @@ -358,8 +360,8 @@ impl Vm { continue; } unsafe { - for &v in (*(*iter)).keys.iter() { - f(v); + for (v, _si) in (*(*iter)).keys.iter() { + f(*v); } } } diff --git a/crates/oxide_vm/src/vm_dispatch_misc.rs b/crates/oxide_vm/src/vm_dispatch_misc.rs index ee5ec83..3816277 100644 --- a/crates/oxide_vm/src/vm_dispatch_misc.rs +++ b/crates/oxide_vm/src/vm_dispatch_misc.rs @@ -264,11 +264,21 @@ impl Vm { pub(crate) fn dispatch_for_in_init(&mut self, a: usize) -> Result<(), String> { vm_trace!("FOR_IN_INIT r{}={:?}", a, self.regs[a]); let obj_val = self.regs[a]; + if obj_val.is_null() || obj_val.is_undefined() { + // null/undefined enumerate nothing — an empty for-in, not a TypeError. + let keys_vec: bumpalo::collections::Vec<(JsValue, u32)> = + bumpalo::collections::Vec::new_in(self.epoch.bump()); + let iter = self.epoch.alloc(ForInIter { keys: keys_vec, index: 0 }); + self.iters.push_for_in(iter.cast::>()); + return Ok(()); + } if !obj_val.is_object() { + // ToObject coercion for other primitives is not implemented yet — TypeError is correct until it lands. return self.raise_type_error("for-in right-hand side is not an object"); } - let mut keys_vec = bumpalo::collections::Vec::new_in(self.epoch.bump()); + let mut keys_vec: bumpalo::collections::Vec<(JsValue, u32)> = + bumpalo::collections::Vec::new_in(self.epoch.bump()); let mut seen = std::collections::HashSet::new(); let mut current = obj_val; let mut depth = 0usize; @@ -282,6 +292,7 @@ impl Vm { } depth += 1; let cur = unsafe { &*current.as_js_object_ptr() }; + let obj_start = keys_vec.len(); let mut cursor = Some(cur.shape_id()); while let Some(id) = cursor { if id == oxide_kernel::shape_forge::EMPTY_SHAPE_ID { @@ -300,8 +311,9 @@ impl Vm { .map(|meta| meta.attributes.enumerable()) .unwrap_or(PropAttributes::DEFAULT_DATA.enumerable()); if enumerable { - keys_vec.push(JsValue::perm_string( - self.kernel_core.perm_interner().string_ptr(shape.property_name), + keys_vec.push(( + JsValue::perm_string(self.kernel_core.perm_interner().string_ptr(shape.property_name)), + shape.property_name, )); } } @@ -310,9 +322,23 @@ impl Vm { break; } } + // The shape chain is walked leaf->root (reverse of insertion order); + // flip this object's slice back to insertion order before its proto. + keys_vec[obj_start..].reverse(); current = cur.proto(); } + // ES enumeration order: integer-index keys ascending, then the rest in + // insertion order. Stable sort preserves insertion order among non-index keys. + keys_vec.sort_by(|(_, a_si), (_, b_si)| { + match (self.array_index_from_property_key(*a_si), self.array_index_from_property_key(*b_si)) { + (Some(ai), Some(bi)) => ai.cmp(&bi), + (Some(_), None) => std::cmp::Ordering::Less, + (None, Some(_)) => std::cmp::Ordering::Greater, + (None, None) => std::cmp::Ordering::Equal, + } + }); + let iter = self.epoch.alloc(ForInIter { keys: keys_vec, index: 0 }); self.iters.push_for_in(iter.cast::>()); Ok(()) @@ -326,7 +352,7 @@ impl Vm { } let iter = unsafe { &mut *iter_ptr }; if iter.index < iter.keys.len() { - self.regs[rd] = iter.keys[iter.index]; + self.regs[rd] = iter.keys[iter.index].0; iter.index += 1; } else { self.regs[rd] = JsValue::undefined(); diff --git a/crates/oxide_vm/tests/for_in_null_undefined_tests.rs b/crates/oxide_vm/tests/for_in_null_undefined_tests.rs new file mode 100644 index 0000000..a205bde --- /dev/null +++ b/crates/oxide_vm/tests/for_in_null_undefined_tests.rs @@ -0,0 +1,58 @@ +use oxide_compiler::compiler::Compiler; +use oxide_parser::Allocator; +use oxide_vm::vm::Vm; + +fn eval(source: &str) -> String { + let allocator = Allocator::default(); + let program = match oxide_parser::parse(&allocator, source) { + Ok(p) => p, + Err(e) => return format!("parse error: {}", e[0].message), + }; + let module = match Compiler::new().compile(&program) { + Ok(m) => m, + Err(e) => return format!("compile error: {e}"), + }; + let mut vm = Vm::new(); + match vm.run(&module) { + Ok(result) => format!("{result}"), + Err(e) => format!("vm error: {e}"), + } +} + +#[test] +fn for_in_null_empty_loop() { + assert_eq!(eval("var r=0;for(var k in null){r=r+1;}r"), "0", "for-in null iterates zero times"); +} + +#[test] +fn for_in_undefined_empty_loop() { + assert_eq!( + eval("var r=0;for(var k in undefined){r=r+1;}r"), + "0", + "for-in undefined iterates zero times" + ); +} + +#[test] +fn for_in_null_does_not_execute_body() { + assert_eq!( + eval("var hit=false;for(var k in null){hit=true;}hit"), + "false", + "for-in null skips the body" + ); +} + +#[test] +fn for_in_null_does_not_throw() { + // Must NOT produce a "vm error: ..." — a quiet empty loop, then a sentinel value. + assert_eq!(eval("for(var k in null){} 7"), "7", "for-in null must not throw"); +} + +#[test] +fn for_in_number_primitive_currently_throws() { + // Interim behavior: ToObject coercion for non-null/undefined primitives is not + // implemented yet, so a number right-hand side still throws TypeError. When + // ToObject lands this becomes an empty loop and this assertion should change. + let out = eval("for(var k in 42){}"); + assert!(out.contains("TypeError"), "expected TypeError for number primitive, got: {out}"); +} diff --git a/crates/oxide_vm/tests/for_in_ordering_tests.rs b/crates/oxide_vm/tests/for_in_ordering_tests.rs new file mode 100644 index 0000000..4e8c1f0 --- /dev/null +++ b/crates/oxide_vm/tests/for_in_ordering_tests.rs @@ -0,0 +1,82 @@ +use oxide_compiler::compiler::Compiler; +use oxide_parser::Allocator; +use oxide_vm::vm::Vm; + +fn eval(source: &str) -> String { + let allocator = Allocator::default(); + let program = match oxide_parser::parse(&allocator, source) { + Ok(p) => p, + Err(e) => return format!("parse error: {}", e[0].message), + }; + let module = match Compiler::new().compile(&program) { + Ok(m) => m, + Err(e) => return format!("compile error: {e}"), + }; + let mut vm = Vm::new(); + match vm.run(&module) { + Ok(result) => format!("{result}"), + Err(e) => format!("vm error: {e}"), + } +} + +// Order is asserted via a position map built inside JS, so the comparison +// returns a boolean (JsValue Display only reveals number/bool, not string +// contents). This is also tolerant of any inherited enumerable keys the +// engine appends after the object's own keys. + +#[test] +fn for_in_ordering_integer_indices_first() { + assert_eq!( + eval( + "var pos={};var i=0;for(var x in {b:2,a:1,'2':3,'1':4}){pos[x]=i;i=i+1;}\ + pos['1']=2"), + "true", + "for-in still enumerates the object's own enumerable keys" + ); +}