Refactor some class features (#2513)

This Pull Request fixes various bugs related to classes.

The biggest changes are:

- Changed private names to be unique across multiple classes.
- Changed private name resolution to work via a visitor after a class is parsed. The way class early errors are defined makes it impossible to perform private name resolution while parsing.
- Added function names to class methods.
- Added class name binding to method function environments.
- Separated opcodes for `static` and non-`static` class method definitions to make the above operations possible.

There are still some bugs and further issues with classes but this is already a lot.

boa_ast/src/expression/access.rs

@@ -15,6 +15,7 @@
 //! [access]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Property_Accessors
 
 use crate::expression::Expression;
+use crate::function::PrivateName;
 use crate::try_break;
 use crate::visitor::{VisitWith, Visitor, VisitorMut};
 use boa_interner::{Interner, Sym, ToInternedString};
@@ -215,14 +216,14 @@ impl VisitWith for SimplePropertyAccess {
 #[derive(Clone, Debug, PartialEq)]
 pub struct PrivatePropertyAccess {
     target: Box<Expression>,
-    field: Sym,
+    field: PrivateName,
 }
 
 impl PrivatePropertyAccess {
     /// Creates a `GetPrivateField` AST Expression.
     #[inline]
     #[must_use]
-    pub fn new(value: Expression, field: Sym) -> Self {
+    pub fn new(value: Expression, field: PrivateName) -> Self {
         Self {
             target: value.into(),
             field,
@@ -239,7 +240,7 @@ impl PrivatePropertyAccess {
     /// Gets the name of the field to retrieve.
     #[inline]
     #[must_use]
-    pub const fn field(&self) -> Sym {
+    pub const fn field(&self) -> PrivateName {
         self.field
     }
 }
@@ -250,7 +251,7 @@ impl ToInternedString for PrivatePropertyAccess {
         format!(
             "{}.#{}",
             self.target.to_interned_string(interner),
-            interner.resolve_expect(self.field)
+            interner.resolve_expect(self.field.description())
         )
     }
 }
@@ -268,15 +269,15 @@ impl VisitWith for PrivatePropertyAccess {
         V: Visitor<'a>,
     {
         try_break!(visitor.visit_expression(&self.target));
-        visitor.visit_sym(&self.field)
+        visitor.visit_private_name(&self.field)
     }
 
     fn visit_with_mut<'a, V>(&'a mut self, visitor: &mut V) -> ControlFlow<V::BreakTy>
     where
         V: VisitorMut<'a>,
     {
         try_break!(visitor.visit_expression_mut(&mut self.target));
-        visitor.visit_sym_mut(&mut self.field)
+        visitor.visit_private_name_mut(&mut self.field)
     }
 }
 

boa_ast/src/expression/mod.rs

@@ -216,6 +216,27 @@ impl Expression {
                 | Self::Class(_)
         )
     }
+
+    /// Returns if the expression is a function definition without a name.
+    ///
+    /// More information:
+    ///  - [ECMAScript reference][spec]
+    ///
+    /// [spec]: https://tc39.es/ecma262/#sec-isanonymousfunctiondefinition
+    #[must_use]
+    #[inline]
+    pub const fn is_anonymous_function_definition(&self) -> bool {
+        match self {
+            Self::ArrowFunction(f) => f.name().is_none(),
+            Self::AsyncArrowFunction(f) => f.name().is_none(),
+            Self::Function(f) => f.name().is_none(),
+            Self::Generator(f) => f.name().is_none(),
+            Self::AsyncGenerator(f) => f.name().is_none(),
+            Self::AsyncFunction(f) => f.name().is_none(),
+            Self::Class(f) => f.name().is_none(),
+            _ => false,
+        }
+    }
 }
 
 impl From<Expression> for Statement {

boa_ast/src/expression/optional.rs

@@ -1,11 +1,11 @@
-use boa_interner::{Interner, Sym, ToInternedString};
-use core::ops::ControlFlow;
-
-use crate::join_nodes;
-use crate::try_break;
-use crate::visitor::{VisitWith, Visitor, VisitorMut};
-
 use super::{access::PropertyAccessField, Expression};
+use crate::{
+    function::PrivateName,
+    join_nodes, try_break,
+    visitor::{VisitWith, Visitor, VisitorMut},
+};
+use boa_interner::{Interner, ToInternedString};
+use core::ops::ControlFlow;
 
 /// List of valid operations in an [`Optional`] chain.
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
@@ -20,7 +20,7 @@ pub enum OptionalOperationKind {
     /// A private property access (`a?.#prop`).
     PrivatePropertyAccess {
         /// The private property accessed.
-        field: Sym,
+        field: PrivateName,
     },
     /// A function call (`a?.(arg)`).
     Call {
@@ -36,7 +36,7 @@ impl VisitWith for OptionalOperationKind {
     {
         match self {
             Self::SimplePropertyAccess { field } => visitor.visit_property_access_field(field),
-            Self::PrivatePropertyAccess { field } => visitor.visit_sym(field),
+            Self::PrivatePropertyAccess { field } => visitor.visit_private_name(field),
             Self::Call { args } => {
                 for arg in args.iter() {
                     try_break!(visitor.visit_expression(arg));
@@ -52,7 +52,7 @@ impl VisitWith for OptionalOperationKind {
     {
         match self {
             Self::SimplePropertyAccess { field } => visitor.visit_property_access_field_mut(field),
-            Self::PrivatePropertyAccess { field } => visitor.visit_sym_mut(field),
+            Self::PrivatePropertyAccess { field } => visitor.visit_private_name_mut(field),
             Self::Call { args } => {
                 for arg in args.iter_mut() {
                     try_break!(visitor.visit_expression_mut(arg));
@@ -113,7 +113,7 @@ impl ToInternedString for OptionalOperation {
             }
 
             if let OptionalOperationKind::PrivatePropertyAccess { field } = &self.kind {
-                return format!(".#{}", interner.resolve_expect(*field));
+                return format!(".#{}", interner.resolve_expect(field.description()));
             }
 
             String::new()
@@ -126,7 +126,7 @@ impl ToInternedString for OptionalOperation {
                 }
             },
             OptionalOperationKind::PrivatePropertyAccess { field } => {
-                format!("#{}", interner.resolve_expect(*field))
+                format!("#{}", interner.resolve_expect(field.description()))
             }
             OptionalOperationKind::Call { args } => format!("({})", join_nodes(interner, args)),
         });

boa_ast/src/function/class.rs

@@ -1,6 +1,4 @@
-use core::ops::ControlFlow;
-use std::borrow::Cow;
-
+use super::Function;
 use crate::{
     block_to_string,
     expression::{Expression, Identifier},
@@ -11,8 +9,9 @@ use crate::{
     Declaration, StatementList, ToStringEscaped,
 };
 use boa_interner::{Interner, Sym, ToIndentedString, ToInternedString};
-
-use super::Function;
+use core::ops::ControlFlow;
+use std::borrow::Cow;
+use std::hash::Hash;
 
 /// A class declaration, as defined by the [spec].
 ///
@@ -27,8 +26,9 @@ use super::Function;
 pub struct Class {
     name: Option<Identifier>,
     super_ref: Option<Expression>,
-    constructor: Option<Function>,
-    elements: Box<[ClassElement]>,
+    pub(crate) constructor: Option<Function>,
+    pub(crate) elements: Box<[ClassElement]>,
+    has_binding_identifier: bool,
 }
 
 impl Class {
@@ -40,12 +40,14 @@ impl Class {
         super_ref: Option<Expression>,
         constructor: Option<Function>,
         elements: Box<[ClassElement]>,
+        has_binding_identifier: bool,
     ) -> Self {
         Self {
             name,
             super_ref,
             constructor,
             elements,
+            has_binding_identifier,
         }
     }
 
@@ -76,6 +78,13 @@ impl Class {
     pub const fn elements(&self) -> &[ClassElement] {
         &self.elements
     }
+
+    /// Returns whether the class has a binding identifier.
+    #[inline]
+    #[must_use]
+    pub const fn has_binding_identifier(&self) -> bool {
+        self.has_binding_identifier
+    }
 }
 
 impl ToIndentedString for Class {
@@ -238,7 +247,7 @@ impl ToIndentedString for Class {
                             MethodDefinition::Set(_) => "set ",
                             _ => "",
                         },
-                        interner.resolve_expect(*name),
+                        interner.resolve_expect(name.description()),
                         match &method {
                             MethodDefinition::Get(expr)
                             | MethodDefinition::Set(expr)
@@ -281,7 +290,7 @@ impl ToIndentedString for Class {
                             MethodDefinition::Set(_) => "set ",
                             _ => "",
                         },
-                        interner.resolve_expect(*name),
+                        interner.resolve_expect(name.description()),
                         match &method {
                             MethodDefinition::Get(expr)
                             | MethodDefinition::Set(expr)
@@ -320,24 +329,30 @@ impl ToIndentedString for Class {
                     Some(expr) => {
                         format!(
                             "{indentation}#{} = {};\n",
-                            interner.resolve_expect(*name),
+                            interner.resolve_expect(name.description()),
                             expr.to_no_indent_string(interner, indent_n + 1)
                         )
                     }
                     None => {
-                        format!("{indentation}#{};\n", interner.resolve_expect(*name),)
+                        format!(
+                            "{indentation}#{};\n",
+                            interner.resolve_expect(name.description()),
+                        )
                     }
                 },
                 ClassElement::PrivateStaticFieldDefinition(name, field) => match field {
                     Some(expr) => {
                         format!(
                             "{indentation}static #{} = {};\n",
-                            interner.resolve_expect(*name),
+                            interner.resolve_expect(name.description()),
                             expr.to_no_indent_string(interner, indent_n + 1)
                         )
                     }
                     None => {
-                        format!("{indentation}static #{};\n", interner.resolve_expect(*name),)
+                        format!(
+                            "{indentation}static #{};\n",
+                            interner.resolve_expect(name.description()),
+                        )
                     }
                 },
                 ClassElement::StaticBlock(statement_list) => {
@@ -414,22 +429,30 @@ impl VisitWith for Class {
 pub enum ClassElement {
     /// A method definition, including `get` and `set` accessors.
     MethodDefinition(PropertyName, MethodDefinition),
+
     /// A static method definition, accessible from the class constructor object.
     StaticMethodDefinition(PropertyName, MethodDefinition),
+
     /// A field definition.
     FieldDefinition(PropertyName, Option<Expression>),
+
     /// A static field definition, accessible from the class constructor object
     StaticFieldDefinition(PropertyName, Option<Expression>),
+
     /// A private method definition, only accessible inside the class declaration.
-    PrivateMethodDefinition(Sym, MethodDefinition),
+    PrivateMethodDefinition(PrivateName, MethodDefinition),
+
     /// A private static method definition, only accessible from static methods and fields inside
     /// the class declaration.
-    PrivateStaticMethodDefinition(Sym, MethodDefinition),
+    PrivateStaticMethodDefinition(PrivateName, MethodDefinition),
+
     /// A private field definition, only accessible inside the class declaration.
-    PrivateFieldDefinition(Sym, Option<Expression>),
+    PrivateFieldDefinition(PrivateName, Option<Expression>),
+
     /// A private static field definition, only accessible from static methods and fields inside the
     /// class declaration.
-    PrivateStaticFieldDefinition(Sym, Option<Expression>),
+    PrivateStaticFieldDefinition(PrivateName, Option<Expression>),
+
     /// A static block, where a class can have initialization logic for its static fields.
     StaticBlock(StatementList),
 }
@@ -452,14 +475,14 @@ impl VisitWith for ClassElement {
                     ControlFlow::Continue(())
                 }
             }
-            Self::PrivateMethodDefinition(sym, md)
-            | Self::PrivateStaticMethodDefinition(sym, md) => {
-                try_break!(visitor.visit_sym(sym));
+            Self::PrivateMethodDefinition(name, md)
+            | Self::PrivateStaticMethodDefinition(name, md) => {
+                try_break!(visitor.visit_private_name(name));
                 visitor.visit_method_definition(md)
             }
-            Self::PrivateFieldDefinition(sym, maybe_expr)
-            | Self::PrivateStaticFieldDefinition(sym, maybe_expr) => {
-                try_break!(visitor.visit_sym(sym));
+            Self::PrivateFieldDefinition(name, maybe_expr)
+            | Self::PrivateStaticFieldDefinition(name, maybe_expr) => {
+                try_break!(visitor.visit_private_name(name));
                 if let Some(expr) = maybe_expr {
                     visitor.visit_expression(expr)
                 } else {
@@ -487,14 +510,14 @@ impl VisitWith for ClassElement {
                     ControlFlow::Continue(())
                 }
             }
-            Self::PrivateMethodDefinition(sym, md)
-            | Self::PrivateStaticMethodDefinition(sym, md) => {
-                try_break!(visitor.visit_sym_mut(sym));
+            Self::PrivateMethodDefinition(name, md)
+            | Self::PrivateStaticMethodDefinition(name, md) => {
+                try_break!(visitor.visit_private_name_mut(name));
                 visitor.visit_method_definition_mut(md)
             }
-            Self::PrivateFieldDefinition(sym, maybe_expr)
-            | Self::PrivateStaticFieldDefinition(sym, maybe_expr) => {
-                try_break!(visitor.visit_sym_mut(sym));
+            Self::PrivateFieldDefinition(name, maybe_expr)
+            | Self::PrivateStaticFieldDefinition(name, maybe_expr) => {
+                try_break!(visitor.visit_private_name_mut(name));
                 if let Some(expr) = maybe_expr {
                     visitor.visit_expression_mut(expr)
                 } else {
@@ -505,3 +528,52 @@ impl VisitWith for ClassElement {
         }
     }
 }
+
+/// A private name as defined by the [spec].
+///
+/// [spec]: https://tc39.es/ecma262/#sec-private-names
+#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
+#[cfg_attr(feature = "fuzz", derive(arbitrary::Arbitrary))]
+#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
+pub struct PrivateName {
+    /// The `[[Description]]` internal slot of the private name.
+    description: Sym,
+
+    /// The indices of the private name are included to ensure that private names are unique.
+    pub(crate) indices: (usize, usize),
+}
+
+impl PrivateName {
+    /// Create a new private name.
+    #[inline]
+    #[must_use]
+    pub const fn new(description: Sym) -> Self {
+        Self {
+            description,
+            indices: (0, 0),
+        }
+    }
+
+    /// Get the description of the private name.
+    #[inline]
+    #[must_use]
+    pub const fn description(&self) -> Sym {
+        self.description
+    }
+}
+
+impl VisitWith for PrivateName {
+    fn visit_with<'a, V>(&'a self, visitor: &mut V) -> ControlFlow<V::BreakTy>
+    where
+        V: Visitor<'a>,
+    {
+        visitor.visit_sym(&self.description)
+    }
+
+    fn visit_with_mut<'a, V>(&'a mut self, visitor: &mut V) -> ControlFlow<V::BreakTy>
+    where
+        V: VisitorMut<'a>,
+    {
+        visitor.visit_sym_mut(&mut self.description)
+    }
+}