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.

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

Add separate variants to ObjectPropertyKind for methods, getters and setters #142

Open
overlookmotel opened this issue Nov 6, 2024 · 2 comments

Comments

@overlookmotel
Copy link

oxc-project/oxc#7175 demonstrates that it's easy to confuse a function which is the property value of an object expression with an object method. i.e. to conflate these 2:

obj = {
    foo() {}
};

obj = {
    foo: function() {}
};

We could avoid this confusing by changing ObjectPropertyKind to have separate variants for methods, getters and setters.

Current:

pub enum ObjectPropertyKind<'a> {
    // This variant can be either a property or a method
    ObjectProperty(Box<'a, ObjectProperty<'a>>),
    SpreadProperty(Box<'a, SpreadElement<'a>>),
}

Proposed:

pub enum ObjectPropertyKind<'a> {
    ObjectProperty(Box<'a, ObjectProperty<'a>>),
    SpreadProperty(Box<'a, SpreadElement<'a>>),
    Method(Box<'a, ObjectMethod<'a>>),
    Getter(Box<'a, ObjectMethod<'a>>),
    Setter(Box<'a, ObjectMethod<'a>>),
}

pub struct ObjectProperty<'a> {
    pub span: Span,
    // pub kind: PropertyKind, <-- removed
    pub key: PropertyKey<'a>,
    pub value: Expression<'a>,
    pub init: Option<Expression<'a>>, // for `CoverInitializedName`
    // pub method: bool, <-- removed
    pub shorthand: bool,
    pub computed: bool,
}

pub struct ObjectMethod<'a> {
    pub span: Span,
    pub key: PropertyKey<'a>,
    pub func: Function<'a>,
    pub computed: bool,
}
@Boshen
Copy link
Member

Boshen commented Nov 6, 2024

This aligns with jsparagus https://gist.github.com/Boshen/0b481a058cd715576aaf1624d2c6d469#file-types_generated-rs-L511-L516

#[derive(Debug, PartialEq)]
pub enum ObjectProperty<'alloc> {
    NamedObjectProperty(NamedObjectProperty<'alloc>),
    ShorthandProperty(ShorthandProperty),
    SpreadProperty(arena::Box<'alloc, Expression<'alloc>>),
}

#[derive(Debug, PartialEq)]
pub enum NamedObjectProperty<'alloc> {
    MethodDefinition(MethodDefinition<'alloc>),
    DataProperty(DataProperty<'alloc>),
}

#[derive(Debug, PartialEq)]
pub enum MethodDefinition<'alloc> {
    Method(Method<'alloc>),
    Getter(Getter<'alloc>),
    Setter(Setter<'alloc>),
}

but breaks estree. I went for estree back in time.

@overlookmotel
Copy link
Author

Let's see how @ottomated's work progresses, but hopefully it'll make it easier for us to shape our Rust AST however we choose, while still having an ESTree-compatible AST on JS side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants