Skip to content

Commit 757d0d7

Browse files
committed
Fix eval hoisting and property descriptor handling
- Implement CreateGlobalVarBinding configurable flag for indirect eval in `hoist_name`. - Implement CreateGlobalFunctionBinding semantics and add hoist traces (`hoist_declarations`). - Harden `check_global_declarations` to follow DefinePropertyOrThrow rules (accessor/non-writable checks). - Update `define_property_internal` to accept borrowed key, enforce configurable/writable/enumerable flags and update property attributes correctly. - Remove creation of own `__proto__` property in `set_internal_prototype_from_constructor`. - Add recursion depth guard to JSON serialization to avoid stack overflows. - Add `PropertyKey` conversions from `Value` and helpers to `JSObjectData` for writable/configurable/enumerable. - Add `Object.prototype.propertyIsEnumerable` builtin. - Handle environment binding descriptors in `evaluate_var`.
1 parent f514cd0 commit 757d0d7

7 files changed

Lines changed: 256 additions & 60 deletions

File tree

src/core/eval.rs

Lines changed: 112 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -753,7 +753,25 @@ fn hoist_name<'gc>(mc: &MutationContext<'gc>, env: &JSObjectDataPtr<'gc>, name:
753753
}
754754
}
755755
if env_get_own(&target_env, name).is_none() {
756-
env_set(mc, &target_env, name, Value::Undefined)?;
756+
// If target is global object, use CreateGlobalVarBinding semantics
757+
// For indirect eval, CreateGlobalVarBinding(..., true) should be used which creates
758+
// a configurable=true property. Detect indirect eval marker on the global env.
759+
if target_env.borrow().prototype.is_none() {
760+
let deletable = if let Some(flag) = crate::core::object_get_key_value(&target_env, "__is_indirect_eval") {
761+
matches!(*flag.borrow(), Value::Boolean(true))
762+
} else {
763+
false
764+
};
765+
let key = PropertyKey::String(name.to_string());
766+
let desc_obj = crate::core::new_js_object_data(mc);
767+
object_set_key_value(mc, &desc_obj, "value", Value::Undefined)?;
768+
object_set_key_value(mc, &desc_obj, "writable", Value::Boolean(true))?;
769+
object_set_key_value(mc, &desc_obj, "enumerable", Value::Boolean(true))?;
770+
object_set_key_value(mc, &desc_obj, "configurable", Value::Boolean(deletable))?;
771+
crate::js_object::define_property_internal(mc, &target_env, &key, &desc_obj)?;
772+
} else {
773+
env_set(mc, &target_env, name, Value::Undefined)?;
774+
}
757775
}
758776
Ok(())
759777
}
@@ -889,6 +907,11 @@ fn hoist_declarations<'gc>(mc: &MutationContext<'gc>, env: &JSObjectDataPtr<'gc>
889907
for stmt in statements {
890908
if let StatementKind::FunctionDeclaration(name, params, body, is_generator, is_async) = &*stmt.kind {
891909
let mut body_clone = body.clone();
910+
log::trace!(
911+
"hoist_declarations: found function declaration '{}'; exec env proto is_none={}",
912+
name,
913+
env.borrow().prototype.is_none()
914+
);
892915
if *is_generator {
893916
// Create a generator function object (hoisted)
894917
let func_obj = crate::core::new_js_object_data(mc);
@@ -959,8 +982,38 @@ fn hoist_declarations<'gc>(mc: &MutationContext<'gc>, env: &JSObjectDataPtr<'gc>
959982
let func = evaluate_function_expression(mc, env, None, params, &mut body_clone)?;
960983
if let Value::Object(func_obj) = &func {
961984
object_set_key_value(mc, func_obj, "name", Value::String(utf8_to_utf16(name)))?;
985+
// CreateGlobalFunctionBinding semantics when executing in the global environment
986+
let key = crate::core::PropertyKey::String(name.clone());
987+
if env.borrow().prototype.is_none() {
988+
let existing = crate::core::get_own_property(env, &key);
989+
log::trace!(
990+
"hoist_declarations: creating global function binding for '{}' existing={:?} is_configurable={}",
991+
name,
992+
existing,
993+
env.borrow().is_configurable(&key)
994+
);
995+
if existing.is_none() || env.borrow().is_configurable(&key) {
996+
let desc_obj = crate::core::new_js_object_data(mc);
997+
object_set_key_value(mc, &desc_obj, "value", Value::Object(*func_obj))?;
998+
object_set_key_value(mc, &desc_obj, "writable", Value::Boolean(true))?;
999+
object_set_key_value(mc, &desc_obj, "enumerable", Value::Boolean(true))?;
1000+
object_set_key_value(mc, &desc_obj, "configurable", Value::Boolean(true))?;
1001+
crate::js_object::define_property_internal(mc, env, &key, &desc_obj)?;
1002+
} else {
1003+
let desc_obj = crate::core::new_js_object_data(mc);
1004+
object_set_key_value(mc, &desc_obj, "value", Value::Object(*func_obj))?;
1005+
crate::js_object::define_property_internal(mc, env, &key, &desc_obj)?;
1006+
}
1007+
log::trace!(
1008+
"hoist_declarations: after create binding is_configurable={}",
1009+
env.borrow().is_configurable(&key)
1010+
);
1011+
} else {
1012+
env_set(mc, env, name, Value::Object(*func_obj))?;
1013+
}
1014+
} else {
1015+
env_set(mc, env, name, func)?;
9621016
}
963-
env_set(mc, env, name, func)?;
9641017
}
9651018
}
9661019
}
@@ -5232,11 +5285,31 @@ fn check_global_declarations<'gc>(env: &JSObjectDataPtr<'gc>, statements: &[Stat
52325285
// Check in reverse order as per spec semantics
52335286
for name in fn_names.iter().rev() {
52345287
let key = crate::core::PropertyKey::String(name.clone());
5235-
if crate::core::get_own_property(env, &key).is_some() && !env.borrow().is_configurable(&key) {
5236-
return Err(EvalError::Js(crate::raise_type_error!(format!(
5237-
"Cannot declare global function '{}'",
5238-
name
5239-
))));
5288+
if let Some(existing_rc) = crate::core::get_own_property(env, &key) {
5289+
// If it's configurable we can replace it freely
5290+
if env.borrow().is_configurable(&key) {
5291+
continue;
5292+
}
5293+
5294+
// If the existing property is an accessor, defining a data value would be incompatible
5295+
let existing_is_accessor = match &*existing_rc.borrow() {
5296+
crate::core::Value::Property { getter, setter, .. } => getter.is_some() || setter.is_some(),
5297+
crate::core::Value::Getter(..) | crate::core::Value::Setter(..) => true,
5298+
_ => false,
5299+
};
5300+
5301+
if existing_is_accessor {
5302+
return Err(EvalError::Js(crate::raise_type_error!(format!(
5303+
"Cannot declare global function '{name}'"
5304+
))));
5305+
}
5306+
5307+
// If it's a non-writable data property and non-configurable, we cannot change its value
5308+
if !env.borrow().is_writable(&key) {
5309+
return Err(EvalError::Js(crate::raise_type_error!(format!(
5310+
"Cannot declare global function '{name}'"
5311+
))));
5312+
}
52405313
}
52415314
}
52425315
Ok(())
@@ -7521,13 +7594,43 @@ pub fn evaluate_expr<'gc>(mc: &MutationContext<'gc>, env: &JSObjectDataPtr<'gc>,
75217594
}
75227595
}
75237596

7524-
fn evaluate_var<'gc>(_mc: &MutationContext<'gc>, env: &JSObjectDataPtr<'gc>, name: &str) -> Result<Value<'gc>, JSError> {
7597+
fn evaluate_var<'gc>(mc: &MutationContext<'gc>, env: &JSObjectDataPtr<'gc>, name: &str) -> Result<Value<'gc>, JSError> {
7598+
// env_get returns the raw stored slot. If a property descriptor (Value::Property) was installed
7599+
// via DefinePropertyOrThrow, unwrap the stored value. For accessor descriptors, call accessor.
75257600
if let Some(val_ptr) = env_get(env, name) {
75267601
let val = val_ptr.borrow().clone();
75277602
if let Value::Uninitialized = val {
75287603
return Err(raise_reference_error!(format!("Cannot access '{}' before initialization", name)));
75297604
}
7530-
return Ok(val);
7605+
match val {
7606+
Value::Property {
7607+
value: Some(v),
7608+
getter: _,
7609+
setter: _,
7610+
} => return Ok(v.borrow().clone()),
7611+
Value::Property {
7612+
value: None,
7613+
getter: _,
7614+
setter: _,
7615+
} => return Ok(Value::Undefined),
7616+
Value::Getter(..) | Value::Setter(..) => {
7617+
// Unlikely for environment bindings, but support by delegating to accessor call
7618+
// Use helper to invoke accessor and return its result
7619+
if let Some(val_rc) = env_get(env, name)
7620+
&& let Value::Getter(..) = &*val_rc.borrow()
7621+
{
7622+
// Call the getter accessor
7623+
let obj_key = PropertyKey::String(name.to_string());
7624+
let res = get_property_with_accessors(mc, env, env, &obj_key).map_err(|e| match e {
7625+
EvalError::Js(js) => js,
7626+
_ => raise_eval_error!("Accessor invocation failed"),
7627+
})?;
7628+
return Ok(res);
7629+
}
7630+
return Err(raise_eval_error!("Accessor not supported on environment binding"));
7631+
}
7632+
other => return Ok(other),
7633+
}
75317634
}
75327635
Err(raise_reference_error!(format!("{} is not defined", name)))
75337636
}

src/core/mod.rs

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -534,15 +534,8 @@ pub fn set_internal_prototype_from_constructor<'gc>(
534534
// set internal prototype pointer (store Weak to avoid cycles)
535535
log::trace!("setting prototype for ctor='{}' proto_obj={:p}", ctor_name, Gc::as_ptr(proto_obj));
536536
obj.borrow_mut(mc).prototype = Some(proto_obj);
537-
// Also set the `__proto__` own property so `obj.__proto__` accesses match expectations
538-
match object_set_key_value(mc, obj, "__proto__", Value::Object(proto_obj)) {
539-
Ok(_) => {
540-
// __proto__ should be non-enumerable
541-
obj.borrow_mut(mc).set_non_enumerable(PropertyKey::from("__proto__"));
542-
log::trace!("set_internal_prototype_from_constructor: set __proto__ own property");
543-
}
544-
Err(e) => log::trace!("set_internal_prototype_from_constructor: failed to set __proto__: {:?}", e),
545-
}
537+
// Do not create an own `__proto__` property for this helper; only set the internal prototype pointer.
538+
log::trace!("set_internal_prototype_from_constructor: set internal prototype pointer");
546539
}
547540
Ok(())
548541
}

src/core/property_key.rs

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
use crate::core::SymbolData;
21
use crate::core::{Collect, Gc};
2+
use crate::core::{SymbolData, Value};
33

44
#[derive(Clone, Debug, Collect)]
55
#[collect(no_drop)]
@@ -53,6 +53,21 @@ impl<'gc> From<&String> for PropertyKey<'gc> {
5353
}
5454
}
5555

56+
impl<'gc> From<&Value<'gc>> for PropertyKey<'gc> {
57+
fn from(v: &Value<'gc>) -> Self {
58+
match v {
59+
Value::Symbol(sd) => PropertyKey::Symbol(*sd),
60+
other => PropertyKey::String(crate::core::value_to_string(other)),
61+
}
62+
}
63+
}
64+
65+
impl<'gc> From<Value<'gc>> for PropertyKey<'gc> {
66+
fn from(v: Value<'gc>) -> Self {
67+
PropertyKey::from(&v)
68+
}
69+
}
70+
5671
impl<'gc> PartialEq for PropertyKey<'gc> {
5772
fn eq(&self, other: &Self) -> bool {
5873
match (self, other) {

src/core/value.rs

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,9 +176,19 @@ impl<'gc> JSObjectData<'gc> {
176176
pub fn set_non_configurable(&mut self, key: PropertyKey<'gc>) {
177177
self.non_configurable.insert(key);
178178
}
179+
180+
pub fn set_configurable(&mut self, key: PropertyKey<'gc>) {
181+
self.non_configurable.remove(&key);
182+
}
183+
179184
pub fn set_non_writable(&mut self, key: PropertyKey<'gc>) {
180185
self.non_writable.insert(key);
181186
}
187+
188+
pub fn set_writable(&mut self, key: PropertyKey<'gc>) {
189+
self.non_writable.remove(&key);
190+
}
191+
182192
pub fn is_const(&self, key: &str) -> bool {
183193
self.constants.contains(key)
184194
}
@@ -257,6 +267,10 @@ impl<'gc> JSObjectData<'gc> {
257267
self.non_enumerable.insert(key);
258268
}
259269

270+
pub fn set_enumerable(&mut self, key: PropertyKey<'gc>) {
271+
self.non_enumerable.remove(&key);
272+
}
273+
260274
pub fn is_configurable(&self, key: &PropertyKey<'gc>) -> bool {
261275
!self.non_configurable.contains(key)
262276
}
@@ -672,7 +686,14 @@ pub fn value_to_string<'gc>(val: &Value<'gc>) -> String {
672686
Value::DataView(_) => "[object DataView]".to_string(),
673687
Value::TypedArray(_) => "[object TypedArray]".to_string(),
674688
Value::Property { .. } => "[Property]".to_string(),
675-
_ => "[unknown]".to_string(),
689+
Value::Symbol(sym) => {
690+
if let Some(desc) = &sym.description {
691+
format!("Symbol({desc})")
692+
} else {
693+
"Symbol()".to_string()
694+
}
695+
}
696+
Value::Uninitialized => "[uninitialized]".to_string(),
676697
}
677698
}
678699

src/js_function.rs

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use crate::core::{
22
ClosureData, EvalError, Expr, Gc, JSObjectDataPtr, MutationContext, Statement, StatementKind, Value, evaluate_expr,
33
has_own_property_value, new_js_object_data, prepare_function_call_env,
44
};
5-
use crate::core::{object_get_key_value, object_set_key_value};
5+
use crate::core::{PropertyKey, object_get_key_value, object_set_key_value};
66
use crate::error::{JSError, JSErrorKind};
77
use crate::js_array::handle_array_constructor;
88
use crate::js_class::prepare_call_env_with_this;
@@ -97,6 +97,7 @@ pub fn handle_global_function<'gc>(
9797
)));
9898
}
9999
"Object.prototype.hasOwnProperty" => return handle_object_has_own_property(args, env),
100+
"Object.prototype.propertyIsEnumerable" => return handle_object_property_is_enumerable(args, env),
100101
"RegExp.prototype.exec" => {
101102
if let Some(this_rc) = crate::core::env_get(env, "this") {
102103
let this_val = this_rc.borrow().clone();
@@ -1642,6 +1643,30 @@ fn handle_object_has_own_property<'gc>(args: &[Value<'gc>], env: &JSObjectDataPt
16421643
}
16431644
}
16441645

1646+
fn handle_object_property_is_enumerable<'gc>(args: &[Value<'gc>], env: &JSObjectDataPtr<'gc>) -> Result<Value<'gc>, EvalError<'gc>> {
1647+
if args.len() != 1 {
1648+
return Err(EvalError::Js(raise_eval_error!("propertyIsEnumerable requires one argument")));
1649+
}
1650+
let key_val = args[0].clone();
1651+
let Some(this_rc) = crate::core::env_get(env, "this") else {
1652+
return Err(EvalError::Js(raise_eval_error!("propertyIsEnumerable called without this")));
1653+
};
1654+
let this_val = this_rc.borrow().clone();
1655+
match this_val {
1656+
Value::Object(obj) => {
1657+
// Convert key value to a PropertyKey
1658+
let key: PropertyKey<'gc> = key_val.into();
1659+
1660+
// Check own property and enumerability
1661+
if crate::core::get_own_property(&obj, &key).is_some() {
1662+
return Ok(Value::Boolean(obj.borrow().is_enumerable(&key)));
1663+
}
1664+
Ok(Value::Boolean(false))
1665+
}
1666+
_ => Ok(Value::Boolean(false)),
1667+
}
1668+
}
1669+
16451670
pub fn initialize_function<'gc>(mc: &MutationContext<'gc>, env: &JSObjectDataPtr<'gc>) -> Result<(), JSError> {
16461671
let func_ctor = new_js_object_data(mc);
16471672
object_set_key_value(mc, &func_ctor, "name", Value::String(utf8_to_utf16("Function")))?;

src/js_json.rs

Lines changed: 24 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -157,22 +157,30 @@ fn js_value_to_json_value<'gc>(mc: &MutationContext<'gc>, js_value: &Value<'gc>)
157157

158158
// Minimal JSON stringifier that respects JS property ordering defined by ordinary_own_property_keys.
159159
fn js_value_to_json_string<'gc>(mc: &MutationContext<'gc>, v: &Value<'gc>) -> Option<String> {
160-
fn escape_json_str(s: &str) -> String {
161-
let mut out = String::with_capacity(s.len() + 2);
162-
for ch in s.chars() {
163-
match ch {
164-
'"' => out.push_str("\\\""),
165-
'\\' => out.push_str("\\\\"),
166-
'\n' => out.push_str("\\n"),
167-
'\r' => out.push_str("\\r"),
168-
'\t' => out.push_str("\\t"),
169-
c if c.is_control() => out.push_str(&format!("\\u{:04x}", c as u32)),
170-
c => out.push(c),
171-
}
160+
inner(mc, v, 0)
161+
}
162+
163+
fn escape_json_str(s: &str) -> String {
164+
let mut out = String::with_capacity(s.len() + 2);
165+
for ch in s.chars() {
166+
match ch {
167+
'"' => out.push_str("\\\""),
168+
'\\' => out.push_str("\\\\"),
169+
'\n' => out.push_str("\\n"),
170+
'\r' => out.push_str("\\r"),
171+
'\t' => out.push_str("\\t"),
172+
c if c.is_control() => out.push_str(&format!("\\u{:04x}", c as u32)),
173+
c => out.push(c),
172174
}
173-
out
174175
}
176+
out
177+
}
175178

179+
// Inner helper with depth to avoid infinite recursion on cycles
180+
fn inner<'gc>(mc: &MutationContext<'gc>, v: &Value<'gc>, depth: usize) -> Option<String> {
181+
if depth > 32 {
182+
return None;
183+
}
176184
match v {
177185
Value::Undefined => None,
178186
Value::Boolean(b) => Some(if *b { "true".to_string() } else { "false".to_string() }),
@@ -195,7 +203,7 @@ fn js_value_to_json_string<'gc>(mc: &MutationContext<'gc>, v: &Value<'gc>) -> Op
195203
for i in 0..len {
196204
let key = i.to_string();
197205
if let Some(val_rc) = get_own_property(obj, &key.into()) {
198-
if let Some(item_str) = js_value_to_json_string(mc, &val_rc.borrow()) {
206+
if let Some(item_str) = inner(mc, &val_rc.borrow(), depth + 1) {
199207
parts.push(item_str);
200208
} else {
201209
parts.push("null".to_string());
@@ -214,10 +222,10 @@ fn js_value_to_json_string<'gc>(mc: &MutationContext<'gc>, v: &Value<'gc>) -> Op
214222
continue;
215223
}
216224
if let Some(val_rc) = object_get_key_value(obj, &key) {
217-
if let Some(val_str) = js_value_to_json_string(mc, &val_rc.borrow()) {
225+
if let Some(val_str) = inner(mc, &val_rc.borrow(), depth + 1) {
218226
parts.push(format!("\"{}\":{}", escape_json_str(s), val_str));
219227
} else {
220-
// skip undefined/functions
228+
// skip undefined/functions or deep/cyclic values
221229
}
222230
}
223231
}

0 commit comments

Comments
 (0)