oxc-project / oxc

⚓ A collection of JavaScript tools written in Rust.
https://oxc.rs
MIT License
11.9k stars 427 forks source link

ast: `ArrayExpressionElement` vs `ExpressionArrayElement` is confusing #6392

Open DonIsaac opened 1 week ago

DonIsaac commented 1 week ago

AstKind has two variants with almost identical names:

  1. AstKind::ArrayExpressionElement(_) which contains an ArrayExpressionElement
  2. AstKind::ExpressionArrayElement(_) which contains an Expression.

From digging around the code, it looks like ExpressionArrayElement is visited when an ArrayExpressionElement contains an Expression

    // crates/oxc_ast/src/generated/visit.rs, line 1662
    #[inline]
    pub fn walk_array_expression_element<'a, V: Visit<'a>>(
        visitor: &mut V,
        it: &ArrayExpressionElement<'a>,
    ) {
        let kind = AstKind::ArrayExpressionElement(visitor.alloc(it));
        visitor.enter_node(kind);
        match it {
            ArrayExpressionElement::SpreadElement(it) => visitor.visit_spread_element(it),
            ArrayExpressionElement::Elision(it) => visitor.visit_elision(it),
            match_expression!(ArrayExpressionElement) => {
                visitor.visit_expression_array_element(it.to_expression())
            }
        }
        visitor.leave_node(kind);
    }

Is this intentional? If it is, we should document the difference between the two since it's quite confusing. If not, we should remove one of these in favor of the other.

overlookmotel commented 1 week ago

I would like to remove these "special cases" like ExpressionArrayElement entirely. As you say, it's confusing, and it complicates our code. If we get rid of it, then you can still figure out if the Expression you're visiting is an element of an ArrayExpression or not - just look up the parent.

Boshen commented 1 week ago

We need a consensus on visiting these partial expressions. See context https://github.com/oxc-project/oxc/issues/4060

This was also brought up in the confusing partial visit of chain expression.

I also had confusion when visiting assignment targets.