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

[Merged by Bors] - Refactor some class features #2513

Closed
wants to merge 3 commits into from
Closed

[Merged by Bors] - Refactor some class features #2513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
35ca5f4
Refactor some class features
raskad Jan 3, 2023
d08a7d1
Refactor private object methods
raskad Jan 13, 2023
85abc6e
Apply suggestions
raskad Jan 13, 2023
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
13 changes: 7 additions & 6 deletions boa_ast/src/expression/access.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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,
Expand All @@ -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
}
}
Expand All @@ -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())
)
}
}
Expand All @@ -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)
}
}

Expand Down
21 changes: 21 additions & 0 deletions boa_ast/src/expression/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
24 changes: 12 additions & 12 deletions boa_ast/src/expression/optional.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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))]
Expand All @@ -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 {
Expand All @@ -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));
Expand All @@ -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));
Expand Down Expand Up @@ -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()
Expand All @@ -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)),
});
Expand Down
130 changes: 101 additions & 29 deletions boa_ast/src/function/class.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
use core::ops::ControlFlow;
use std::borrow::Cow;

use super::Function;
use crate::{
block_to_string,
expression::{Expression, Identifier},
Expand All @@ -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].
///
Expand All @@ -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 {
Expand All @@ -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,
}
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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),
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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),
Copy link
Member

@jedel1043 jedel1043 Jan 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was reading the spec about private names and it specifies that PrivateNames should be globally unique, but indices are only unique in the context of a single Context, right? I was thinking that the global uniqueness property is probably to restrict the access of private names from objects that share the same indices but don't belong to the same Context, but to me it seems like this doesn't prevent that? I ask this because I don't know if you've already considered that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still some uniqueness missing with this implementation. We indeed have to add some runtime unique identifier to separate PrivateName of multiple objects that are created from the same class. I think this will probably be some kind of Context wide index. As far as different Contexts go, I think that would need some further work as our Context is currently also a Realm. If we have figured out how we can separate those, the Context should be the outermost api to the engine and imo should the the uniqueness-border.

I left the runtime uniqueness out of the PR to limit the scope somewhat.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's okay! Can you add a TODO describing this then? It would be good to track this for when we implement proper Realms.

}

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)
}
}
Loading