Add field accessors to destructing assignment (#2213)

This Pull Request changes the following:

- Implement parsing/cover-conversion for field accessor member expressions in [DestructuringAssignmentTarget](https://tc39.es/ecma262/#prod-DestructuringAssignmentTarget).
- Modify the `CopyDataProperties` opcode to account for excluded keys that are only known at runtime.

boa_engine/src/bytecompiler/mod.rs

@@ -989,7 +989,7 @@ impl<'b> ByteCompiler<'b> {
                         PropertyDefinition::SpreadObject(expr) => {
                             self.compile_expr(expr, true)?;
                             self.emit_opcode(Opcode::Swap);
-                            self.emit(Opcode::CopyDataProperties, &[0]);
+                            self.emit(Opcode::CopyDataProperties, &[0, 0]);
                             self.emit_opcode(Opcode::Pop);
                         }
                         PropertyDefinition::CoverInitializedName(_, _) => {
@@ -2195,9 +2195,13 @@ impl<'b> ByteCompiler<'b> {
 
                 self.emit_opcode(Opcode::RequireObjectCoercible);
 
+                let mut additional_excluded_keys_count = 0;
+                let rest_exits = pattern.has_rest();
+
                 for binding in pattern.bindings() {
                     use BindingPatternTypeObject::{
-                        BindingPattern, Empty, RestGetConstField, RestProperty, SingleName,
+                        AssignmentGetConstField, AssignmentGetField, AssignmentRestProperty,
+                        BindingPattern, Empty, RestProperty, SingleName,
                     };
 
                     match binding {
@@ -2218,7 +2222,11 @@ impl<'b> ByteCompiler<'b> {
                                 PropertyName::Computed(node) => {
                                     self.compile_expr(node, true)?;
                                     self.emit_opcode(Opcode::Swap);
-                                    self.emit_opcode(Opcode::GetPropertyByValue);
+                                    if rest_exits {
+                                        self.emit_opcode(Opcode::GetPropertyByValuePush);
+                                    } else {
+                                        self.emit_opcode(Opcode::GetPropertyByValue);
+                                    }
                                 }
                             }
 
@@ -2229,13 +2237,17 @@ impl<'b> ByteCompiler<'b> {
                                 self.patch_jump(skip);
                             }
                             self.emit_binding(def, *ident);
+
+                            if rest_exits && property_name.computed().is_some() {
+                                self.emit_opcode(Opcode::Swap);
+                                additional_excluded_keys_count += 1;
+                            }
                         }
                         //  BindingRestProperty : ... BindingIdentifier
                         RestProperty {
                             ident,
                             excluded_keys,
                         } => {
-                            self.emit_opcode(Opcode::Dup);
                             self.emit_opcode(Opcode::PushEmptyObject);
 
                             for key in excluded_keys {
@@ -2244,10 +2256,13 @@ impl<'b> ByteCompiler<'b> {
                                 ));
                             }
 
-                            self.emit(Opcode::CopyDataProperties, &[excluded_keys.len() as u32]);
+                            self.emit(
+                                Opcode::CopyDataProperties,
+                                &[excluded_keys.len() as u32, additional_excluded_keys_count],
+                            );
                             self.emit_binding(def, *ident);
                         }
-                        RestGetConstField {
+                        AssignmentRestProperty {
                             get_const_field,
                             excluded_keys,
                         } => {
@@ -2258,7 +2273,44 @@ impl<'b> ByteCompiler<'b> {
                                     self.interner().resolve_expect(*key).into(),
                                 ));
                             }
-                            self.emit(Opcode::CopyDataProperties, &[excluded_keys.len() as u32]);
+                            self.emit(Opcode::CopyDataProperties, &[excluded_keys.len() as u32, 0]);
+                            self.access_set(
+                                Access::ByName {
+                                    node: get_const_field,
+                                },
+                                None,
+                                false,
+                            )?;
+                        }
+                        AssignmentGetConstField {
+                            property_name,
+                            get_const_field,
+                            default_init,
+                        } => {
+                            self.emit_opcode(Opcode::Dup);
+                            match property_name {
+                                PropertyName::Literal(name) => {
+                                    let index = self.get_or_insert_name(*name);
+                                    self.emit(Opcode::GetPropertyByName, &[index]);
+                                }
+                                PropertyName::Computed(node) => {
+                                    self.compile_expr(node, true)?;
+                                    self.emit_opcode(Opcode::Swap);
+                                    if rest_exits {
+                                        self.emit_opcode(Opcode::GetPropertyByValuePush);
+                                    } else {
+                                        self.emit_opcode(Opcode::GetPropertyByValue);
+                                    }
+                                }
+                            }
+
+                            if let Some(init) = default_init {
+                                let skip =
+                                    self.emit_opcode_with_operand(Opcode::JumpIfNotUndefined);
+                                self.compile_expr(init, true)?;
+                                self.patch_jump(skip);
+                            }
+
                             self.access_set(
                                 Access::ByName {
                                     node: get_const_field,
@@ -2266,6 +2318,47 @@ impl<'b> ByteCompiler<'b> {
                                 None,
                                 false,
                             )?;
+
+                            if rest_exits && property_name.computed().is_some() {
+                                self.emit_opcode(Opcode::Swap);
+                                additional_excluded_keys_count += 1;
+                            }
+                        }
+                        AssignmentGetField {
+                            property_name,
+                            get_field,
+                            default_init,
+                        } => {
+                            self.emit_opcode(Opcode::Dup);
+                            match property_name {
+                                PropertyName::Literal(name) => {
+                                    let index = self.get_or_insert_name(*name);
+                                    self.emit(Opcode::GetPropertyByName, &[index]);
+                                }
+                                PropertyName::Computed(node) => {
+                                    self.compile_expr(node, true)?;
+                                    self.emit_opcode(Opcode::Swap);
+                                    if rest_exits {
+                                        self.emit_opcode(Opcode::GetPropertyByValuePush);
+                                    } else {
+                                        self.emit_opcode(Opcode::GetPropertyByValue);
+                                    }
+                                }
+                            }
+
+                            if let Some(init) = default_init {
+                                let skip =
+                                    self.emit_opcode_with_operand(Opcode::JumpIfNotUndefined);
+                                self.compile_expr(init, true)?;
+                                self.patch_jump(skip);
+                            }
+
+                            self.access_set(Access::ByValue { node: get_field }, None, false)?;
+
+                            if rest_exits && property_name.computed().is_some() {
+                                self.emit_opcode(Opcode::Swap);
+                                additional_excluded_keys_count += 1;
+                            }
                         }
                         BindingPattern {
                             ident,
@@ -2297,7 +2390,9 @@ impl<'b> ByteCompiler<'b> {
                     }
                 }
 
-                self.emit_opcode(Opcode::Pop);
+                if !rest_exits {
+                    self.emit_opcode(Opcode::Pop);
+                }
             }
             DeclarationPattern::Array(pattern) => {
                 let skip_init = self.emit_opcode_with_operand(Opcode::JumpIfNotUndefined);

boa_engine/src/syntax/ast/node/declaration/mod.rs

@@ -333,8 +333,9 @@ impl DeclarationPattern {
                                 }
                             }
                         }
-                        BindingPatternTypeObject::RestGetConstField {
+                        BindingPatternTypeObject::AssignmentRestProperty {
-                            get_const_field, ..
+                            get_const_field,
+                            ..
                         } => {
                             if get_const_field.obj().contains_arguments() {
                                 return true;
@@ -432,8 +433,9 @@ impl DeclarationPattern {
                                 return true;
                             }
                         }
-                        BindingPatternTypeObject::RestGetConstField {
+                        BindingPatternTypeObject::AssignmentRestProperty {
-                            get_const_field, ..
+                            get_const_field,
+                            ..
                         } => {
                             if get_const_field.obj().contains(symbol) {
                                 return true;
@@ -560,6 +562,15 @@ impl DeclarationPatternObject {
         &self.bindings
     }
 
+    // Returns if the object binding pattern has a rest element.
+    #[inline]
+    pub(crate) fn has_rest(&self) -> bool {
+        matches!(
+            self.bindings.last(),
+            Some(BindingPatternTypeObject::RestProperty { .. })
+        )
+    }
+
     /// Gets the list of identifiers declared by the object binding pattern.
     #[inline]
     pub(crate) fn idents(&self) -> Vec<Sym> {
@@ -567,11 +578,11 @@ impl DeclarationPatternObject {
 
         for binding in &self.bindings {
             use BindingPatternTypeObject::{
-                BindingPattern, Empty, RestGetConstField, RestProperty, SingleName,
+                AssignmentRestProperty, BindingPattern, Empty, RestProperty, SingleName,
             };
 
             match binding {
-                Empty | RestGetConstField { .. } => {}
+                Empty | AssignmentRestProperty { .. } => {}
                 SingleName {
                     ident,
                     property_name: _,
@@ -585,6 +596,12 @@ impl DeclarationPatternObject {
                 } => {
                     idents.push(*property_name);
                 }
+                BindingPatternTypeObject::AssignmentGetConstField { property_name, .. }
+                | BindingPatternTypeObject::AssignmentGetField { property_name, .. } => {
+                    if let Some(name) = property_name.literal() {
+                        idents.push(name);
+                    }
+                }
                 BindingPattern {
                     ident: _,
                     pattern,
@@ -737,20 +754,50 @@ pub enum BindingPatternTypeObject {
     /// [spec1]: https://tc39.es/ecma262/#prod-BindingRestProperty
     RestProperty { ident: Sym, excluded_keys: Vec<Sym> },
 
-    /// RestGetConstField represents a rest property (spread operator) with a property accessor.
+    /// AssignmentRestProperty represents a rest property with a DestructuringAssignmentTarget.
     ///
     /// Note: According to the spec this is not part of an ObjectBindingPattern.
-    /// This is only used when a object literal is used as the left-hand-side of an assignment expression.
+    /// This is only used when a object literal is used to cover an AssignmentPattern.
     ///
     /// More information:
     ///  - [ECMAScript reference][spec]
     ///
-    /// [spec]: https://tc39.es/ecma262/#prod-AssignmentExpression
+    /// [spec]: https://tc39.es/ecma262/#prod-AssignmentRestProperty
-    RestGetConstField {
+    AssignmentRestProperty {
         get_const_field: GetConstField,
         excluded_keys: Vec<Sym>,
     },
 
+    /// AssignmentGetConstField represents an AssignmentProperty with a cost field member expression AssignmentElement.
+    ///
+    /// Note: According to the spec this is not part of an ObjectBindingPattern.
+    /// This is only used when a object literal is used to cover an AssignmentPattern.
+    ///
+    /// More information:
+    ///  - [ECMAScript reference][spec]
+    ///
+    /// [spec]: https://tc39.es/ecma262/#prod-AssignmentProperty
+    AssignmentGetConstField {
+        property_name: PropertyName,
+        get_const_field: GetConstField,
+        default_init: Option<Node>,
+    },
+
+    /// AssignmentGetField represents an AssignmentProperty with an expression field member expression AssignmentElement.
+    ///
+    /// Note: According to the spec this is not part of an ObjectBindingPattern.
+    /// This is only used when a object literal is used to cover an AssignmentPattern.
+    ///
+    /// More information:
+    ///  - [ECMAScript reference][spec]
+    ///
+    /// [spec]: https://tc39.es/ecma262/#prod-AssignmentProperty
+    AssignmentGetField {
+        property_name: PropertyName,
+        get_field: GetField,
+        default_init: Option<Node>,
+    },
+
     /// BindingPattern represents a `BindingProperty` with a `BindingPattern` as the `BindingElement`.
     ///
     /// Additionally to the identifier of the new property and the nested binding pattern,
@@ -806,11 +853,63 @@ impl ToInternedString for BindingPatternTypeObject {
             } => {
                 format!(" ... {}", interner.resolve_expect(*property_name))
             }
-            Self::RestGetConstField {
+            Self::AssignmentRestProperty {
                 get_const_field, ..
             } => {
                 format!(" ... {}", get_const_field.to_interned_string(interner))
             }
+            Self::AssignmentGetConstField {
+                property_name,
+                get_const_field,
+                default_init,
+            } => {
+                let mut buf = match property_name {
+                    PropertyName::Literal(name) => {
+                        format!(
+                            " {} : {}",
+                            interner.resolve_expect(*name),
+                            get_const_field.to_interned_string(interner)
+                        )
+                    }
+                    PropertyName::Computed(node) => {
+                        format!(
+                            " [{}] : {}",
+                            node.to_interned_string(interner),
+                            get_const_field.to_interned_string(interner)
+                        )
+                    }
+                };
+                if let Some(init) = &default_init {
+                    buf.push_str(&format!(" = {}", init.to_interned_string(interner)));
+                }
+                buf
+            }
+            Self::AssignmentGetField {
+                property_name,
+                get_field,
+                default_init,
+            } => {
+                let mut buf = match property_name {
+                    PropertyName::Literal(name) => {
+                        format!(
+                            " {} : {}",
+                            interner.resolve_expect(*name),
+                            get_field.to_interned_string(interner)
+                        )
+                    }
+                    PropertyName::Computed(node) => {
+                        format!(
+                            " [{}] : {}",
+                            node.to_interned_string(interner),
+                            get_field.to_interned_string(interner)
+                        )
+                    }
+                };
+                if let Some(init) = &default_init {
+                    buf.push_str(&format!(" = {}", init.to_interned_string(interner)));
+                }
+                buf
+            }
             Self::BindingPattern {
                 ident: property_name,
                 pattern,

boa_engine/src/syntax/ast/node/object/mod.rs

@@ -390,7 +390,7 @@ pub enum PropertyName {
 
 impl PropertyName {
     /// Returns the literal property name if it exists.
-    pub(in crate::syntax) fn literal(&self) -> Option<Sym> {
+    pub(crate) fn literal(&self) -> Option<Sym> {
         if let Self::Literal(sym) = self {
             Some(*sym)
         } else {
@@ -399,7 +399,7 @@ impl PropertyName {
     }
 
     /// Returns the expression node if the property name is computed.
-    pub(in crate::syntax) fn computed(&self) -> Option<&Node> {
+    pub(crate) fn computed(&self) -> Option<&Node> {
         if let Self::Computed(node) = self {
             Some(node)
         } else {

boa_engine/src/syntax/ast/node/operator/assign/mod.rs

@@ -204,38 +204,70 @@ pub(crate) fn object_decl_to_declaration_pattern(
                         default_init: None,
                     });
                 }
-                (PropertyName::Literal(name), Node::Assign(assign)) => match assign.lhs() {
+                (_, Node::Assign(assign)) => match assign.lhs() {
-                    AssignTarget::Identifier(ident) if *name == ident.sym() => {
+                    AssignTarget::Identifier(ident) => {
-                        if strict && *name == Sym::EVAL {
+                        if let Some(name) = name.literal() {
-                            return None;
+                            if name == ident.sym() {
-                        }
+                                if strict && name == Sym::EVAL {
-                        if strict && RESERVED_IDENTIFIERS_STRICT.contains(name) {
+                                    return None;
+                                }
+                                if strict && RESERVED_IDENTIFIERS_STRICT.contains(&name) {
+                                    return None;
+                                }
+                                excluded_keys.push(name);
+                                bindings.push(BindingPatternTypeObject::SingleName {
+                                    ident: name,
+                                    property_name: PropertyName::Literal(name),
+                                    default_init: Some(assign.rhs().clone()),
+                                });
+                            } else {
+                                bindings.push(BindingPatternTypeObject::SingleName {
+                                    ident: ident.sym(),
+                                    property_name: PropertyName::Literal(name),
+                                    default_init: Some(assign.rhs().clone()),
+                                });
+                            }
+                        } else {
                             return None;
                         }
-
+                    }
-                        excluded_keys.push(*name);
+                    AssignTarget::DeclarationPattern(pattern) => {
-                        bindings.push(BindingPatternTypeObject::SingleName {
+                        bindings.push(BindingPatternTypeObject::BindingPattern {
-                            ident: *name,
+                            ident: name.clone(),
-                            property_name: PropertyName::Literal(*name),
+                            pattern: pattern.clone(),
                             default_init: Some(assign.rhs().clone()),
                         });
                     }
-                    AssignTarget::Identifier(ident) => {
+                    AssignTarget::GetConstField(field) => {
-                        bindings.push(BindingPatternTypeObject::SingleName {
+                        bindings.push(BindingPatternTypeObject::AssignmentGetConstField {
-                            ident: ident.sym(),
+                            property_name: name.clone(),
-                            property_name: PropertyName::Literal(*name),
+                            get_const_field: field.clone(),
                             default_init: Some(assign.rhs().clone()),
                         });
                     }
-                    AssignTarget::DeclarationPattern(pattern) => {
+                    AssignTarget::GetField(field) => {
-                        bindings.push(BindingPatternTypeObject::BindingPattern {
+                        bindings.push(BindingPatternTypeObject::AssignmentGetField {
-                            ident: PropertyName::Literal(*name),
+                            property_name: name.clone(),
-                            pattern: pattern.clone(),
+                            get_field: field.clone(),
                             default_init: Some(assign.rhs().clone()),
                         });
                     }
-                    _ => return None,
+                    AssignTarget::GetPrivateField(_) => return None,
                 },
+                (_, Node::GetConstField(field)) => {
+                    bindings.push(BindingPatternTypeObject::AssignmentGetConstField {
+                        property_name: name.clone(),
+                        get_const_field: field.clone(),
+                        default_init: None,
+                    });
+                }
+                (_, Node::GetField(field)) => {
+                    bindings.push(BindingPatternTypeObject::AssignmentGetField {
+                        property_name: name.clone(),
+                        get_field: field.clone(),
+                        default_init: None,
+                    });
+                }
                 (PropertyName::Computed(name), Node::Identifier(ident)) => {
                     bindings.push(BindingPatternTypeObject::SingleName {
                         ident: ident.sym(),
@@ -254,7 +286,7 @@ pub(crate) fn object_decl_to_declaration_pattern(
                         });
                     }
                     Node::GetConstField(get_const_field) => {
-                        bindings.push(BindingPatternTypeObject::RestGetConstField {
+                        bindings.push(BindingPatternTypeObject::AssignmentRestProperty {
                             get_const_field: get_const_field.clone(),
                             excluded_keys: excluded_keys.clone(),
                         });

boa_engine/src/vm/code_block.rs

@@ -209,7 +209,6 @@ impl CodeBlock {
             | Opcode::ForInLoopInitIterator
             | Opcode::ForInLoopNext
             | Opcode::ConcatToString
-            | Opcode::CopyDataProperties
             | Opcode::GeneratorNextDelegate => {
                 let result = self.read::<u32>(*pc).to_string();
                 *pc += size_of::<u32>();
@@ -217,7 +216,8 @@ impl CodeBlock {
             }
             Opcode::TryStart
             | Opcode::PushDeclarativeEnvironment
-            | Opcode::PushFunctionEnvironment => {
+            | Opcode::PushFunctionEnvironment
+            | Opcode::CopyDataProperties => {
                 let operand1 = self.read::<u32>(*pc);
                 *pc += size_of::<u32>();
                 let operand2 = self.read::<u32>(*pc);
@@ -329,6 +329,7 @@ impl CodeBlock {
             | Opcode::Dec
             | Opcode::DecPost
             | Opcode::GetPropertyByValue
+            | Opcode::GetPropertyByValuePush
             | Opcode::SetPropertyByValue
             | Opcode::DefineOwnPropertyByValue
             | Opcode::DefineClassMethodByValue

boa_engine/src/vm/mod.rs

@@ -760,6 +760,21 @@ impl Context {
 
                 self.vm.push(value);
             }
+            Opcode::GetPropertyByValuePush => {
+                let object = self.vm.pop();
+                let key = self.vm.pop();
+                let object = if let Some(object) = object.as_object() {
+                    object.clone()
+                } else {
+                    object.to_object(self)?
+                };
+
+                let property_key = key.to_property_key(self)?;
+                let value = object.get(property_key, self)?;
+
+                self.vm.push(key);
+                self.vm.push(value);
+            }
             Opcode::SetPropertyByName => {
                 let index = self.vm.read::<u32>();
 
@@ -1353,13 +1368,21 @@ impl Context {
             }
             Opcode::CopyDataProperties => {
                 let excluded_key_count = self.vm.read::<u32>();
+                let excluded_key_count_computed = self.vm.read::<u32>();
                 let mut excluded_keys = Vec::with_capacity(excluded_key_count as usize);
                 for _ in 0..excluded_key_count {
-                    excluded_keys.push(self.vm.pop().as_string().expect("not a string").clone());
+                    let key = self.vm.pop();
+                    excluded_keys
+                        .push(key.to_property_key(self).expect("key must be property key"));
                 }
                 let value = self.vm.pop();
                 let object = value.as_object().expect("not an object");
                 let source = self.vm.pop();
+                for _ in 0..excluded_key_count_computed {
+                    let key = self.vm.pop();
+                    excluded_keys
+                        .push(key.to_property_key(self).expect("key must be property key"));
+                }
                 object.copy_data_properties(&source, excluded_keys, self)?;
                 self.vm.push(value);
             }

boa_engine/src/vm/opcode.rs

@@ -520,6 +520,15 @@ pub enum Opcode {
     /// Stack: key, object **=>** value
     GetPropertyByValue,
 
+    /// Get a property by value from an object an push the key and value on the stack.
+    ///
+    /// Like `object[key]`
+    ///
+    /// Operands:
+    ///
+    /// Stack: key, object **=>** key, value
+    GetPropertyByValuePush,
+
     /// Sets a property by name of an object.
     ///
     /// Like `object.name = value`
@@ -747,9 +756,9 @@ pub enum Opcode {
 
     /// Copy all properties of one object to another object.
     ///
-    /// Operands: excluded_key_count: `u32`
+    /// Operands: excluded_key_count: `u32`, excluded_key_count_computed: `u32`
     ///
-    /// Stack: source, value, excluded_key_0 ... excluded_key_n **=>** value
+    /// Stack: excluded_key_computed_0 ... excluded_key_computed_n, source, value, excluded_key_0 ... excluded_key_n **=>** value
     CopyDataProperties,
 
     /// Call ToPropertyKey on the value on the stack.
@@ -1247,6 +1256,7 @@ impl Opcode {
             Self::SetName => "SetName",
             Self::GetPropertyByName => "GetPropertyByName",
             Self::GetPropertyByValue => "GetPropertyByValue",
+            Self::GetPropertyByValuePush => "GetPropertyByValuePush",
             Self::SetPropertyByName => "SetPropertyByName",
             Self::DefineOwnPropertyByName => "DefineOwnPropertyByName",
             Self::DefineClassMethodByName => "DefineClassMethodByName",
@@ -1407,6 +1417,7 @@ impl Opcode {
             Self::SetName => "INST - SetName",
             Self::GetPropertyByName => "INST - GetPropertyByName",
             Self::GetPropertyByValue => "INST - GetPropertyByValue",
+            Self::GetPropertyByValuePush => "INST - GetPropertyByValuePush",
             Self::SetPropertyByName => "INST - SetPropertyByName",
             Self::DefineOwnPropertyByName => "INST - DefineOwnPropertyByName",
             Self::SetPropertyByValue => "INST - SetPropertyByValue",