From fa5783688bd739ae6d91af041ed17595a65469eb Mon Sep 17 00:00:00 2001 From: raskad <32105367+raskad@users.noreply.github.com> Date: Thu, 22 Jul 2021 00:00:17 +0200 Subject: [PATCH 1/2] Fix DataDescriptor Value to possibly be empty --- boa/src/builtins/date/tests.rs | 3 +- boa/src/builtins/json/mod.rs | 2 +- .../environment/object_environment_record.rs | 4 +- boa/src/object/gcobject.rs | 6 +- boa/src/object/internal_methods.rs | 64 +++++++++++++++---- boa/src/property/mod.rs | 15 ++++- .../ast/node/iteration/for_in_loop/mod.rs | 7 +- boa/src/value/display.rs | 13 ++-- boa/src/value/tests.rs | 1 + 9 files changed, 91 insertions(+), 24 deletions(-) diff --git a/boa/src/builtins/date/tests.rs b/boa/src/builtins/date/tests.rs index fa47954a705..67b14aaf51c 100644 --- a/boa/src/builtins/date/tests.rs +++ b/boa/src/builtins/date/tests.rs @@ -65,7 +65,8 @@ fn date_this_time_value() { .expect("Expected 'message' property") .as_data_descriptor() .unwrap() - .value(); + .value() + .unwrap(); assert_eq!(Value::string("\'this\' is not a Date"), *message_property); } diff --git a/boa/src/builtins/json/mod.rs b/boa/src/builtins/json/mod.rs index d5d828e0c31..c97e6fda2b2 100644 --- a/boa/src/builtins/json/mod.rs +++ b/boa/src/builtins/json/mod.rs @@ -230,7 +230,7 @@ impl Json { .get_property(key) .as_ref() .and_then(|p| p.as_data_descriptor()) - .map(|d| d.value()) + .map(|d| d.value().unwrap_or_else(Value::undefined)) .unwrap_or_else(Value::undefined), ) } diff --git a/boa/src/environment/object_environment_record.rs b/boa/src/environment/object_environment_record.rs index f2e4d818803..84e7874031e 100644 --- a/boa/src/environment/object_environment_record.rs +++ b/boa/src/environment/object_environment_record.rs @@ -112,7 +112,9 @@ impl EnvironmentRecordTrait for ObjectEnvironmentRecord { fn get_binding_value(&self, name: &str, strict: bool, context: &mut Context) -> Result { if self.bindings.has_field(name) { match self.bindings.get_property(name) { - Some(PropertyDescriptor::Data(ref d)) => Ok(d.value()), + Some(PropertyDescriptor::Data(ref d)) => { + Ok(d.value().unwrap_or_else(Value::undefined)) + } _ => Ok(Value::undefined()), } } else if strict { diff --git a/boa/src/object/gcobject.rs b/boa/src/object/gcobject.rs index a138d7e098d..6df2fa994d4 100644 --- a/boa/src/object/gcobject.rs +++ b/boa/src/object/gcobject.rs @@ -534,12 +534,14 @@ impl GcObject { } Ok(AccessorDescriptor::new(get, set, attribute).into()) + } else if let Some(v) = value { + Ok(DataDescriptor::new(v, attribute).into()) } else { - Ok(DataDescriptor::new(value.unwrap_or_else(Value::undefined), attribute).into()) + Ok(DataDescriptor::new_without_value(attribute).into()) } } - /// Reeturn `true` if it is a native object and the native type is `T`. + /// Return `true` if it is a native object and the native type is `T`. /// /// # Panics /// diff --git a/boa/src/object/internal_methods.rs b/boa/src/object/internal_methods.rs index 3a6a1891b67..adc5d23bed9 100644 --- a/boa/src/object/internal_methods.rs +++ b/boa/src/object/internal_methods.rs @@ -82,7 +82,7 @@ impl GcObject { } } Some(ref desc) => match desc { - PropertyDescriptor::Data(desc) => Ok(desc.value()), + PropertyDescriptor::Data(desc) => Ok(desc.value().unwrap_or_else(Value::undefined)), PropertyDescriptor::Accessor(AccessorDescriptor { get: Some(get), .. }) => { get.call(&receiver, &[], context) } @@ -195,7 +195,19 @@ impl GcObject { return false; } - self.insert(key, desc); + if let PropertyDescriptor::Data(data) = &desc { + if data.value.is_some() { + self.insert(key, desc); + } else { + self.insert( + key, + DataDescriptor::new(Value::undefined(), data.attributes()), + ); + } + } else { + self.insert(key, desc); + } + return true; }; @@ -234,8 +246,14 @@ impl GcObject { return false; } - let current = DataDescriptor::new(value, current.attributes()); - self.insert(key, current); + self.insert( + key, + DataDescriptor::new( + value.clone().unwrap_or_else(Value::undefined), + current.attributes(), + ), + ); + return true; } (PropertyDescriptor::Data(current), PropertyDescriptor::Data(desc)) => { @@ -245,7 +263,10 @@ impl GcObject { return false; } - if !Value::same_value(&desc.value(), ¤t.value()) { + if !Value::same_value( + &desc.value().unwrap_or_else(Value::undefined), + ¤t.value().unwrap_or_else(Value::undefined), + ) { return false; } } @@ -268,7 +289,22 @@ impl GcObject { } } - self.insert(key, desc); + if let PropertyDescriptor::Data(data) = &desc { + let value = if let PropertyDescriptor::Data(data) = ¤t { + data.value.clone().unwrap_or_else(Value::undefined) + } else { + Value::undefined() + }; + + if data.value.is_some() { + self.insert(key, desc); + } else { + self.insert(key, DataDescriptor::new(value, data.attributes())); + } + } else { + self.insert(key, desc); + } + true } @@ -295,11 +331,14 @@ impl GcObject { return Ok(self.ordinary_define_own_property("length", desc)) } PropertyDescriptor::Data(ref d) => { - if d.value().is_undefined() { + if d.value().unwrap_or_else(Value::undefined).is_undefined() { return Ok(self.ordinary_define_own_property("length", desc)); } - let new_len = d.value().to_u32(context)?; - let number_len = d.value().to_number(context)?; + let new_len = d.value().unwrap_or_else(Value::undefined).to_u32(context)?; + let number_len = d + .value() + .unwrap_or_else(Value::undefined) + .to_number(context)?; #[allow(clippy::float_cmp)] if new_len as f64 != number_len { return Err(context.construct_range_error("bad length for array")); @@ -308,7 +347,7 @@ impl GcObject { PropertyDescriptor::Data(DataDescriptor::new(new_len, d.attributes())); let old_len_desc = self.get_own_property(&"length".into()).unwrap(); let old_len_desc = old_len_desc.as_data_descriptor().unwrap(); - let old_len = old_len_desc.value(); + let old_len = old_len_desc.value().unwrap_or_else(Value::undefined); if new_len >= old_len.to_u32(context)? { return Ok(self.ordinary_define_own_property("length", new_len_desc)); } @@ -369,7 +408,10 @@ impl GcObject { PropertyKey::Index(index) => { let old_len_desc = self.get_own_property(&"length".into()).unwrap(); let old_len_data_desc = old_len_desc.as_data_descriptor().unwrap(); - let old_len = old_len_data_desc.value().to_u32(context)?; + let old_len = old_len_data_desc + .value() + .unwrap_or_else(Value::undefined) + .to_u32(context)?; if index >= old_len && !old_len_data_desc.writable() { return Ok(false); } diff --git a/boa/src/property/mod.rs b/boa/src/property/mod.rs index b5411c6645d..4be1468c816 100644 --- a/boa/src/property/mod.rs +++ b/boa/src/property/mod.rs @@ -34,7 +34,7 @@ pub use attribute::Attribute; /// [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty #[derive(Debug, Clone, Trace, Finalize)] pub struct DataDescriptor { - pub(crate) value: Value, + pub(crate) value: Option, attributes: Attribute, } @@ -46,14 +46,23 @@ impl DataDescriptor { V: Into, { Self { - value: value.into(), + value: Some(value.into()), + attributes, + } + } + + /// Create a new `DataDescriptor` without a value. + #[inline] + pub fn new_without_value(attributes: Attribute) -> Self { + Self { + value: None, attributes, } } /// Return the `value` of the data descriptor. #[inline] - pub fn value(&self) -> Value { + pub fn value(&self) -> Option { self.value.clone() } diff --git a/boa/src/syntax/ast/node/iteration/for_in_loop/mod.rs b/boa/src/syntax/ast/node/iteration/for_in_loop/mod.rs index 8e17489b0cf..9482798cfe7 100644 --- a/boa/src/syntax/ast/node/iteration/for_in_loop/mod.rs +++ b/boa/src/syntax/ast/node/iteration/for_in_loop/mod.rs @@ -92,7 +92,12 @@ impl Executable for ForInLoop { let for_in_iterator = ForInIterator::create_for_in_iterator(context, Value::from(object)); let next_function = for_in_iterator .get_property("next") - .map(|p| p.as_data_descriptor().unwrap().value()) + .map(|p| { + p.as_data_descriptor() + .unwrap() + .value() + .unwrap_or_else(Value::undefined) + }) .ok_or_else(|| context.construct_type_error("Could not find property `next`"))?; let iterator = IteratorRecord::new(for_in_iterator, next_function); diff --git a/boa/src/value/display.rs b/boa/src/value/display.rs index cb7a0d9e93d..59842677d2c 100644 --- a/boa/src/value/display.rs +++ b/boa/src/value/display.rs @@ -52,7 +52,8 @@ macro_rules! print_obj_value { let v = &val .as_data_descriptor() .unwrap() - .value(); + .value() + .unwrap_or(Value::undefined()); format!( "{:>width$}: {}", key, @@ -109,6 +110,7 @@ pub(crate) fn log_string_from(x: &Value, print_internals: bool, print_children: .as_data_descriptor() .unwrap() .value() + .unwrap_or_else(Value::undefined) .as_number() .map(|n| n as i32) .unwrap_or_default(); @@ -125,7 +127,10 @@ pub(crate) fn log_string_from(x: &Value, print_internals: bool, print_children: log_string_from( &v.get_own_property(&i.into()) // FIXME: handle accessor descriptors - .and_then(|p| p.as_data_descriptor().map(|d| d.value())) + .and_then(|p| { + p.as_data_descriptor() + .map(|d| d.value().unwrap_or_else(Value::undefined)) + }) .unwrap_or_default(), print_internals, false, @@ -205,13 +210,13 @@ pub(crate) fn display_obj(v: &Value, print_internals: bool) -> String { .get_property("name") .as_ref() .and_then(|p| p.as_data_descriptor()) - .map(|d| d.value()) + .map(|d| d.value().unwrap_or_else(Value::undefined)) .unwrap_or_else(Value::undefined); let message = v .get_property("message") .as_ref() .and_then(|p| p.as_data_descriptor()) - .map(|d| d.value()) + .map(|d| d.value().unwrap_or_else(Value::undefined)) .unwrap_or_else(Value::undefined); return format!("{}: {}", name.display(), message.display()); } diff --git a/boa/src/value/tests.rs b/boa/src/value/tests.rs index d5cf20e3de6..0f6dfdcb714 100644 --- a/boa/src/value/tests.rs +++ b/boa/src/value/tests.rs @@ -286,6 +286,7 @@ fn string_length_is_in_utf16_codeunits() { .as_data_descriptor() .unwrap() .value() + .unwrap() .to_integer_or_infinity(&mut context) .unwrap(), IntegerOrInfinity::Integer(2) From adac728b63bc0333d53149fd01d28e6fdbe61c99 Mon Sep 17 00:00:00 2001 From: raskad <32105367+raskad@users.noreply.github.com> Date: Thu, 22 Jul 2021 19:48:50 +0200 Subject: [PATCH 2/2] Refactor to use flag instead of option --- boa/src/builtins/date/tests.rs | 3 +- boa/src/builtins/json/mod.rs | 2 +- .../environment/object_environment_record.rs | 4 +- boa/src/object/internal_methods.rs | 68 ++++++------------- boa/src/property/mod.rs | 17 +++-- .../ast/node/iteration/for_in_loop/mod.rs | 7 +- boa/src/value/display.rs | 13 ++-- boa/src/value/tests.rs | 1 - 8 files changed, 42 insertions(+), 73 deletions(-) diff --git a/boa/src/builtins/date/tests.rs b/boa/src/builtins/date/tests.rs index 67b14aaf51c..fa47954a705 100644 --- a/boa/src/builtins/date/tests.rs +++ b/boa/src/builtins/date/tests.rs @@ -65,8 +65,7 @@ fn date_this_time_value() { .expect("Expected 'message' property") .as_data_descriptor() .unwrap() - .value() - .unwrap(); + .value(); assert_eq!(Value::string("\'this\' is not a Date"), *message_property); } diff --git a/boa/src/builtins/json/mod.rs b/boa/src/builtins/json/mod.rs index c97e6fda2b2..d5d828e0c31 100644 --- a/boa/src/builtins/json/mod.rs +++ b/boa/src/builtins/json/mod.rs @@ -230,7 +230,7 @@ impl Json { .get_property(key) .as_ref() .and_then(|p| p.as_data_descriptor()) - .map(|d| d.value().unwrap_or_else(Value::undefined)) + .map(|d| d.value()) .unwrap_or_else(Value::undefined), ) } diff --git a/boa/src/environment/object_environment_record.rs b/boa/src/environment/object_environment_record.rs index 84e7874031e..f2e4d818803 100644 --- a/boa/src/environment/object_environment_record.rs +++ b/boa/src/environment/object_environment_record.rs @@ -112,9 +112,7 @@ impl EnvironmentRecordTrait for ObjectEnvironmentRecord { fn get_binding_value(&self, name: &str, strict: bool, context: &mut Context) -> Result { if self.bindings.has_field(name) { match self.bindings.get_property(name) { - Some(PropertyDescriptor::Data(ref d)) => { - Ok(d.value().unwrap_or_else(Value::undefined)) - } + Some(PropertyDescriptor::Data(ref d)) => Ok(d.value()), _ => Ok(Value::undefined()), } } else if strict { diff --git a/boa/src/object/internal_methods.rs b/boa/src/object/internal_methods.rs index adc5d23bed9..b71249bc09d 100644 --- a/boa/src/object/internal_methods.rs +++ b/boa/src/object/internal_methods.rs @@ -82,7 +82,7 @@ impl GcObject { } } Some(ref desc) => match desc { - PropertyDescriptor::Data(desc) => Ok(desc.value().unwrap_or_else(Value::undefined)), + PropertyDescriptor::Data(desc) => Ok(desc.value()), PropertyDescriptor::Accessor(AccessorDescriptor { get: Some(get), .. }) => { get.call(&receiver, &[], context) } @@ -195,18 +195,7 @@ impl GcObject { return false; } - if let PropertyDescriptor::Data(data) = &desc { - if data.value.is_some() { - self.insert(key, desc); - } else { - self.insert( - key, - DataDescriptor::new(Value::undefined(), data.attributes()), - ); - } - } else { - self.insert(key, desc); - } + self.insert(key, desc); return true; }; @@ -246,13 +235,7 @@ impl GcObject { return false; } - self.insert( - key, - DataDescriptor::new( - value.clone().unwrap_or_else(Value::undefined), - current.attributes(), - ), - ); + self.insert(key, DataDescriptor::new(value, current.attributes())); return true; } @@ -263,10 +246,7 @@ impl GcObject { return false; } - if !Value::same_value( - &desc.value().unwrap_or_else(Value::undefined), - ¤t.value().unwrap_or_else(Value::undefined), - ) { + if !Value::same_value(&desc.value(), ¤t.value()) { return false; } } @@ -289,20 +269,20 @@ impl GcObject { } } - if let PropertyDescriptor::Data(data) = &desc { - let value = if let PropertyDescriptor::Data(data) = ¤t { - data.value.clone().unwrap_or_else(Value::undefined) - } else { - Value::undefined() - }; - - if data.value.is_some() { + match (¤t, &desc) { + (PropertyDescriptor::Data(current_data), PropertyDescriptor::Data(desc_data)) => { + if desc_data.has_value() { + self.insert(key, desc); + } else { + self.insert( + key, + DataDescriptor::new(current_data.value.clone(), desc_data.attributes()), + ); + } + } + _ => { self.insert(key, desc); - } else { - self.insert(key, DataDescriptor::new(value, data.attributes())); } - } else { - self.insert(key, desc); } true @@ -331,14 +311,11 @@ impl GcObject { return Ok(self.ordinary_define_own_property("length", desc)) } PropertyDescriptor::Data(ref d) => { - if d.value().unwrap_or_else(Value::undefined).is_undefined() { + if d.value().is_undefined() { return Ok(self.ordinary_define_own_property("length", desc)); } - let new_len = d.value().unwrap_or_else(Value::undefined).to_u32(context)?; - let number_len = d - .value() - .unwrap_or_else(Value::undefined) - .to_number(context)?; + let new_len = d.value().to_u32(context)?; + let number_len = d.value().to_number(context)?; #[allow(clippy::float_cmp)] if new_len as f64 != number_len { return Err(context.construct_range_error("bad length for array")); @@ -347,7 +324,7 @@ impl GcObject { PropertyDescriptor::Data(DataDescriptor::new(new_len, d.attributes())); let old_len_desc = self.get_own_property(&"length".into()).unwrap(); let old_len_desc = old_len_desc.as_data_descriptor().unwrap(); - let old_len = old_len_desc.value().unwrap_or_else(Value::undefined); + let old_len = old_len_desc.value(); if new_len >= old_len.to_u32(context)? { return Ok(self.ordinary_define_own_property("length", new_len_desc)); } @@ -408,10 +385,7 @@ impl GcObject { PropertyKey::Index(index) => { let old_len_desc = self.get_own_property(&"length".into()).unwrap(); let old_len_data_desc = old_len_desc.as_data_descriptor().unwrap(); - let old_len = old_len_data_desc - .value() - .unwrap_or_else(Value::undefined) - .to_u32(context)?; + let old_len = old_len_data_desc.value().to_u32(context)?; if index >= old_len && !old_len_data_desc.writable() { return Ok(false); } diff --git a/boa/src/property/mod.rs b/boa/src/property/mod.rs index 4be1468c816..d7dc93ec42f 100644 --- a/boa/src/property/mod.rs +++ b/boa/src/property/mod.rs @@ -34,8 +34,9 @@ pub use attribute::Attribute; /// [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty #[derive(Debug, Clone, Trace, Finalize)] pub struct DataDescriptor { - pub(crate) value: Option, + pub(crate) value: Value, attributes: Attribute, + has_value: bool, } impl DataDescriptor { @@ -46,8 +47,9 @@ impl DataDescriptor { V: Into, { Self { - value: Some(value.into()), + value: value.into(), attributes, + has_value: true, } } @@ -55,17 +57,24 @@ impl DataDescriptor { #[inline] pub fn new_without_value(attributes: Attribute) -> Self { Self { - value: None, + value: Value::undefined(), attributes, + has_value: false, } } /// Return the `value` of the data descriptor. #[inline] - pub fn value(&self) -> Option { + pub fn value(&self) -> Value { self.value.clone() } + /// Check whether the data descriptor has a value. + #[inline] + pub fn has_value(&self) -> bool { + self.has_value + } + /// Return the attributes of the descriptor. #[inline] pub fn attributes(&self) -> Attribute { diff --git a/boa/src/syntax/ast/node/iteration/for_in_loop/mod.rs b/boa/src/syntax/ast/node/iteration/for_in_loop/mod.rs index 9482798cfe7..8e17489b0cf 100644 --- a/boa/src/syntax/ast/node/iteration/for_in_loop/mod.rs +++ b/boa/src/syntax/ast/node/iteration/for_in_loop/mod.rs @@ -92,12 +92,7 @@ impl Executable for ForInLoop { let for_in_iterator = ForInIterator::create_for_in_iterator(context, Value::from(object)); let next_function = for_in_iterator .get_property("next") - .map(|p| { - p.as_data_descriptor() - .unwrap() - .value() - .unwrap_or_else(Value::undefined) - }) + .map(|p| p.as_data_descriptor().unwrap().value()) .ok_or_else(|| context.construct_type_error("Could not find property `next`"))?; let iterator = IteratorRecord::new(for_in_iterator, next_function); diff --git a/boa/src/value/display.rs b/boa/src/value/display.rs index 59842677d2c..cb7a0d9e93d 100644 --- a/boa/src/value/display.rs +++ b/boa/src/value/display.rs @@ -52,8 +52,7 @@ macro_rules! print_obj_value { let v = &val .as_data_descriptor() .unwrap() - .value() - .unwrap_or(Value::undefined()); + .value(); format!( "{:>width$}: {}", key, @@ -110,7 +109,6 @@ pub(crate) fn log_string_from(x: &Value, print_internals: bool, print_children: .as_data_descriptor() .unwrap() .value() - .unwrap_or_else(Value::undefined) .as_number() .map(|n| n as i32) .unwrap_or_default(); @@ -127,10 +125,7 @@ pub(crate) fn log_string_from(x: &Value, print_internals: bool, print_children: log_string_from( &v.get_own_property(&i.into()) // FIXME: handle accessor descriptors - .and_then(|p| { - p.as_data_descriptor() - .map(|d| d.value().unwrap_or_else(Value::undefined)) - }) + .and_then(|p| p.as_data_descriptor().map(|d| d.value())) .unwrap_or_default(), print_internals, false, @@ -210,13 +205,13 @@ pub(crate) fn display_obj(v: &Value, print_internals: bool) -> String { .get_property("name") .as_ref() .and_then(|p| p.as_data_descriptor()) - .map(|d| d.value().unwrap_or_else(Value::undefined)) + .map(|d| d.value()) .unwrap_or_else(Value::undefined); let message = v .get_property("message") .as_ref() .and_then(|p| p.as_data_descriptor()) - .map(|d| d.value().unwrap_or_else(Value::undefined)) + .map(|d| d.value()) .unwrap_or_else(Value::undefined); return format!("{}: {}", name.display(), message.display()); } diff --git a/boa/src/value/tests.rs b/boa/src/value/tests.rs index 0f6dfdcb714..d5cf20e3de6 100644 --- a/boa/src/value/tests.rs +++ b/boa/src/value/tests.rs @@ -286,7 +286,6 @@ fn string_length_is_in_utf16_codeunits() { .as_data_descriptor() .unwrap() .value() - .unwrap() .to_integer_or_infinity(&mut context) .unwrap(), IntegerOrInfinity::Integer(2)