Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DataDescriptor Value to possibly be empty #1419

Merged
merged 2 commits into from
Jul 22, 2021
Merged

Fix DataDescriptor Value to possibly be empty #1419

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fa57836
Fix DataDescriptor Value to possibly be empty
raskad Jul 21, 2021
adac728
Refactor to use flag instead of option
raskad Jul 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion boa/src/builtins/date/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
2 changes: 1 addition & 1 deletion boa/src/builtins/json/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
)
}
Expand Down
4 changes: 3 additions & 1 deletion boa/src/environment/object_environment_record.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,9 @@ impl EnvironmentRecordTrait for ObjectEnvironmentRecord {
fn get_binding_value(&self, name: &str, strict: bool, context: &mut Context) -> Result<Value> {
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 {
Expand Down
6 changes: 4 additions & 2 deletions boa/src/object/gcobject.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
///
Expand Down
64 changes: 53 additions & 11 deletions boa/src/object/internal_methods.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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;
};

Expand Down Expand Up @@ -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)) => {
Expand All @@ -245,7 +263,10 @@ impl GcObject {
return false;
}

if !Value::same_value(&desc.value(), &current.value()) {
if !Value::same_value(
&desc.value().unwrap_or_else(Value::undefined),
&current.value().unwrap_or_else(Value::undefined),
) {
return false;
}
}
Expand All @@ -268,7 +289,22 @@ impl GcObject {
}
}

self.insert(key, desc);
if let PropertyDescriptor::Data(data) = &desc {
let value = if let PropertyDescriptor::Data(data) = &current {
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
}

Expand All @@ -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"));
Expand All @@ -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));
}
Expand Down Expand Up @@ -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);
}
Expand Down
15 changes: 12 additions & 3 deletions boa/src/property/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Value>,
attributes: Attribute,
}
HalidOdat marked this conversation as resolved.
Show resolved Hide resolved

Expand All @@ -46,14 +46,23 @@ impl DataDescriptor {
V: Into<Value>,
{
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<Value> {
self.value.clone()
}

Expand Down
7 changes: 6 additions & 1 deletion boa/src/syntax/ast/node/iteration/for_in_loop/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
13 changes: 9 additions & 4 deletions boa/src/value/display.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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();
Expand All @@ -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,
Expand Down Expand Up @@ -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());
}
Expand Down
1 change: 1 addition & 0 deletions boa/src/value/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down