refactor(ast): Remove AstKind for MemberExpression and replace with StaticMemberExpression and PrivateFieldExpression

[camchenry]

• related to Make AstKind consistent #11490

Replaces the current AstKind::MemberExpression with StaticMemberExpresion and PrivateFieldExpression.

To make this change a little bit more ergonomic I've a new MemberExpressionKind which represents this subset of kinds.

[camchenry]

• refactor(ast): Remove AstKind for MemberExpression and replace with StaticMemberExpression and PrivateFieldExpression #11767 : 2 dependent PRs (#11888 , #11889 ) 👈 (View in Graphite)
• refactor(ast): add AstKind for ComputedMemberExpression #11766
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[codspeed-hq]

CodSpeed Instrumentation Performance Report

Merging #11767 will degrade performances by 13.21%

_{Comparing 06-17-refactor_ast_add_astkind_for_staticmemberexpression_ (87b8496) with main (e840680)}

Summary

❌ 1 regressions
✅ 37 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
❌	mangler[cal.com.tsx]	3.1 ms	3.5 ms	-13.21%

[camc314]

should we introduce a MemberExpression enum so that we can go from AstKind to MemberExpression? it's such a common pattern in the ;linter

[overlookmotel]

should we introduce a MemberExpression enum so that we can go from AstKind to MemberExpression? it's such a common pattern in the ;linter

You mean something like?

enum MemberExpressionKind<'a> {
    ComputedMemberExpression(&'a ComputedMemberExpression<'a>),
    StaticMemberExpression(&'a StaticMemberExpression<'a>),
    PrivateFieldExpression(&'a PrivateFieldExpression<'a>),
}

This kind of "enum narrowing" pattern could also be useful in many places. There's lots of code all over Oxc which checks somewhere that an Expression is e.g. a literal, and then later on somewhere else there'll be a match on the same Expression with an _ => unreachable!() branch for all the variants which aren't literals.

That's both unperformant (unnecessary checks) and error-prone - we learn about mistakes when there's a panic, rather than getting a compile-time error.

It'd be ideal if you could do something like:

let expr: &Expression = get_expression_somehow();
if let Some(expr_literal) = expr.as_literal() {
    // `expr_literal` is an `&ExpressionLiteral` enum which only has variants
    // for `BooleanLiteral`, `StringLiteral` etc.
    // Pass `expr_literal` to the next function, instead of passing `expr`.
}

Like with the "enum inheritance" schema in AST, we could codegen these conversions and make them very cheap (just a check and transmute).

[overlookmotel]

I've written up the broader "enum narrowing" feature in oxc-project/backlog#166. But it'd be pretty simple to hand-code MemberExpressionKind in meantime if that's the type where this pattern is most common.

[camchenry]

@camc314 @overlookmotel Thanks for the suggestion. I agree having the separate enum is a good idea, it makes the code a lot simpler and probably more maintainable in the future if ECMAScript should ever add more member expression types.

I've gone ahead and manually implemented an API for this, but an autogenerated one in the future would be nice.

[camc314]

Yeah enum narrowing would definitly be nice. But for this specific case, I think it's definitly worth having a member expr enum (even if temporary) for this case due to how heavily member expresssion/static member expression is used

[overlookmotel]

I rebased this on main, and it's now broken. Looks like clash with #11790 which introduced a usage of AstKind::MemberExpression in the no-console rule.

[overlookmotel]

I've pushed a commit to hopefully fix no-console, but I'm out of time, and now am away for the next week. @camc314 can I please hand over to you to review?

[overlookmotel]

Conformance fail relates to formatter. Not sure how serious it is.

[camc314]

Merge activity

• Jun 25, 11:14 AM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Jun 25, 11:21 AM UTC: camc314 added this pull request to the Graphite merge queue.
• Jun 25, 11:28 AM UTC: Merged by the Graphite merge queue.