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

ast: ArrayExpressionElement vs ExpressionArrayElement is confusing #6392

Closed
DonIsaac opened this issue Oct 9, 2024 · 2 comments · Fixed by #6740
Closed

ast: ArrayExpressionElement vs ExpressionArrayElement is confusing #6392

DonIsaac opened this issue Oct 9, 2024 · 2 comments · Fixed by #6740
Assignees
Labels
A-ast Area - AST

Comments

@DonIsaac
Copy link
Contributor

DonIsaac commented Oct 9, 2024

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.

@DonIsaac DonIsaac added C-bug Category - Bug A-ast Area - AST labels Oct 9, 2024
@overlookmotel
Copy link
Contributor

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 Boshen self-assigned this Oct 10, 2024
@Boshen
Copy link
Member

Boshen commented Oct 10, 2024

We need a consensus on visiting these partial expressions. See context #4060

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

I also had confusion when visiting assignment targets.

@Boshen Boshen added P-high Priority - High needs-discussion Requires a discussion from the core team and removed C-bug Category - Bug P-high Priority - High labels Oct 10, 2024
@Boshen Boshen assigned overlookmotel and unassigned Boshen Oct 11, 2024
@Boshen Boshen removed the needs-discussion Requires a discussion from the core team label Oct 11, 2024
@Boshen Boshen assigned Boshen and unassigned overlookmotel Oct 19, 2024
Boshen added a commit that referenced this issue Oct 21, 2024
Boshen added a commit that referenced this issue Oct 21, 2024
@Boshen Boshen closed this as completed Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ast Area - AST
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants