oxc-project / backlog

backlog for collborators only
1 stars 0 forks source link

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

Open overlookmotel opened 2 weeks ago

overlookmotel commented 2 weeks ago

https://github.com/oxc-project/oxc/issues/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 commented 2 weeks ago

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 commented 2 weeks ago

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.