refactor(ast): add `AstKind` for `ComputedMemberExpression` #11766

camchenry · 2025-06-17T03:57:55Z

part of Make AstKind consistent #11490

camchenry · 2025-06-17T03:58:14Z

refactor(ast): Remove AstKind for MemberExpression and replace with StaticMemberExpression and PrivateFieldExpression #11767 : 2 dependent PRs (#11888 , #11889 )
refactor(ast): add AstKind for ComputedMemberExpression #11766 👈 (View in Graphite)
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 · 2025-06-17T04:02:48Z

CodSpeed Instrumentation Performance Report

Merging #11766 will improve performances by 14.65%

_{Comparing 06-16-refactor_ast_add_astkind_for_computedmemberexpression_ (190e390) with main (19cee8c)}

Summary

⚡ 1 improvements
✅ 37 untouched benchmarks

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
⚡	`mangler[cal.com.tsx]`	3.5 ms	3.1 ms	+14.65%

crates/oxc_linter/src/rules/oxc/bad_array_method_on_arguments.rs

crates/oxc_formatter/src/parentheses/expression.rs

crates/oxc_ast/src/ast_impl/js.rs

overlookmotel · 2025-06-24T14:17:52Z

I've started working through this. Immediately got sidetracked by static_property_info and TemplateElements (#11879), but now onwards...

Have taken liberty of rebasing this and #11767 on latest main to remove merge conflicts.

overlookmotel

I've made some comments below. Many relate to refactoring to deduplicate code which appears in both the branches for MemberExpression and ComputedMemberExpression.

But 3 of the comments relate to where I think the changes in this PR may alter behavior, and might have introduced bugs. Our test coverage in linter tends to be a little sparse, compared to the wide variety of wacky patterns which are legal JavaScript! So it's hard to rely on the tests.

#11767 is stacked on top of this and presumably touches much of the same code, so making changes in this PR would be a pain. And some of my comments may be rendered obsolete by #11767 anyway.

So I'm going to merge this now. But @camchenry could you possibly try to address any of my comments which are valid after these 2 PRs are merged?

crates/oxc_linter/src/rules/eslint/no_extend_native.rs

overlookmotel · 2025-06-24T14:58:34Z

crates/oxc_linter/src/rules/eslint/no_extend_native.rs

+        AstKind::ComputedMemberExpression(computed_prop_access_expr) => {
+            let prop_name = computed_prop_access_expr.static_property_name()?;
+            if prop_name != "prototype" {
+                return None;
+            }

-    if let AstKind::ChainExpression(_) = grandparent_node.kind() {
-        prototype_node = Some(grandparent_node);
-        if let Some(grandparent_parent) = ctx.nodes().parent_node(grandparent_node.id()) {
-            prototype_node = Some(grandparent_parent);
-        }
-    }
+            let grandparent_node = ctx.nodes().parent_node(parent.id())?;

-    if is_computed_member_expression_matching(grandparent_node, prop_access_expr) {
-        prototype_node = Some(grandparent_node);
-    }
+            if let AstKind::ChainExpression(_) = grandparent_node.kind() {
+                prototype_node = Some(grandparent_node);
+                if let Some(grandparent_parent) = ctx.nodes().parent_node(grandparent_node.id()) {
+                    prototype_node = Some(grandparent_parent);
+                }
+            }


Isn't the parent of a ComputedMemberExpression always a MemberExpression?

So if let AstKind::ChainExpression(_) = grandparent_node.kind() will never match.

#11767 will remove AstKind::MemberExpression, which will fix it again. But it's worrying that this didn't cause any tests to fail. I suggest adding a test which fails due to this problem, fix it in this PR, and then we can make sure that test still passes after #11767.

Also, I think the code related to ChainExpression which is in both match arms could be de-duplicated by moving it to after the match (or maybe not worthwhile doing that if #11767 simplifies it again).

I changed my mind. Going to merge this now, but we should check after both PRs are merged that this didn't break anything.

crates/oxc_linter/src/rules/eslint/prefer_promise_reject_errors.rs

crates/oxc_linter/src/rules/jest/no_alias_methods.rs

crates/oxc_linter/src/rules/oxc/bad_array_method_on_arguments.rs

crates/oxc_linter/src/rules/oxc/uninvoked_array_callback.rs

overlookmotel · 2025-06-24T19:01:26Z

Merge activity

Jun 24, 7:01 PM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Jun 24, 7:03 PM UTC: overlookmotel added this pull request to the Graphite merge queue.
Jun 24, 7:11 PM UTC: Merged by the Graphite merge queue.

- part of #11490

overlookmotel · 2025-06-24T19:05:13Z

By the way, I hope you don't take my litany of comments as criticism. These AstKind changes are surprisingly subtle and brain-bending (well I find them so anyway). I think it's nigh on impossible to get through the big ones like MemberExpression bug-free without at least 2 pairs of eyes on it. Maybe even 2 pairs is not enough...

overlookmotel · 2025-06-24T19:08:15Z

Side note: ctx.nodes().parent_node(node_id) returning an Option is a massive pain. Every node except Program has a parent, and generally you know the node you're getting the parent of is not Program, so all the let Some(parent) = ... is just boilerplate noise in the code, and introduces unnecessary branches all over the place.

Should we make Program have itself as its parent, so parent_node can return AstNode not Option<AstNode>?

) Follow up to comment in #11766 (comment)

Follow-up to some comments in #11766. This addresses an issue where we were not properly checking which side of assignment the prototype access occurred on.

github-actions bot added A-linter Area - Linter A-ast Area - AST A-ast-tools Area - AST tools A-formatter Area - Formatter C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Jun 17, 2025

camchenry force-pushed the 06-16-refactor_ast_add_astkind_for_computedmemberexpression_ branch from 73d5ce7 to f4ef54b Compare June 17, 2025 04:07

camchenry marked this pull request as ready for review June 17, 2025 04:07

camchenry requested review from Dunqing, camc314 and overlookmotel as code owners June 17, 2025 04:07

github-actions bot added the A-semantic Area - Semantic label Jun 17, 2025

This was referenced Jun 17, 2025

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

Merged

Make AstKind consistent #11490

Open

camchenry force-pushed the 06-16-refactor_ast_add_astkind_for_computedmemberexpression_ branch from f4ef54b to c424ef3 Compare June 17, 2025 19:02

graphite-app bot reviewed Jun 17, 2025

View reviewed changes

crates/oxc_linter/src/rules/oxc/bad_array_method_on_arguments.rs Show resolved Hide resolved

camc314 reviewed Jun 20, 2025

View reviewed changes

crates/oxc_formatter/src/parentheses/expression.rs Outdated Show resolved Hide resolved

overlookmotel reviewed Jun 20, 2025

View reviewed changes

crates/oxc_ast/src/ast_impl/js.rs Outdated Show resolved Hide resolved

camchenry force-pushed the 06-16-refactor_ast_add_astkind_for_computedmemberexpression_ branch 2 times, most recently from 6e099bb to 3616da5 Compare June 21, 2025 21:45

overlookmotel force-pushed the 06-16-refactor_ast_add_astkind_for_computedmemberexpression_ branch from 3616da5 to 48d83ab Compare June 24, 2025 14:11

overlookmotel approved these changes Jun 24, 2025

View reviewed changes

overlookmotel added the 0-merge Merge with Graphite Merge Queue label Jun 24, 2025

refactor(ast): add AstKind for ComputedMemberExpression (#11766)

190e390

- part of #11490

graphite-app bot force-pushed the 06-16-refactor_ast_add_astkind_for_computedmemberexpression_ branch from 48d83ab to 190e390 Compare June 24, 2025 19:04

graphite-app bot merged commit 190e390 into main Jun 24, 2025
24 checks passed

graphite-app bot deleted the 06-16-refactor_ast_add_astkind_for_computedmemberexpression_ branch June 24, 2025 19:11

graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Jun 24, 2025

This was referenced Jun 25, 2025

fix(linter): remove false positives for no-extend-native #11888

Merged

fix(linter): revert prefer-promise-reject-errors to old behavior #11889

Merged

graphite-app bot pushed a commit that referenced this pull request Jun 25, 2025

fix(linter): revert prefer-promise-reject-errors to old behavior (#11889

6f67b52

) Follow up to comment in #11766 (comment)

This was referenced Jun 25, 2025

release(crates): v0.75.0 #11897

Merged

release(oxlint): v1.4.0 #11979

Merged

Uh oh!

refactor(ast): add AstKind for ComputedMemberExpression #11766

refactor(ast): add AstKind for ComputedMemberExpression #11766

Uh oh!

Conversation

camchenry commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camchenry commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

codspeed-hq bot commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Instrumentation Performance Report

Merging #11766 will improve performances by 14.65%

Summary

Benchmarks breakdown

Uh oh!

Uh oh!

Uh oh!

Uh oh!

overlookmotel commented Jun 24, 2025

Uh oh!

overlookmotel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

overlookmotel Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

overlookmotel Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

overlookmotel commented Jun 24, 2025 • edited by graphite-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

overlookmotel commented Jun 24, 2025

Uh oh!

overlookmotel commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

refactor(ast): add `AstKind` for `ComputedMemberExpression` #11766

refactor(ast): add `AstKind` for `ComputedMemberExpression` #11766

camchenry commented Jun 17, 2025 •

edited

Loading

camchenry commented Jun 17, 2025 •

edited

Loading

codspeed-hq bot commented Jun 17, 2025 •

edited

Loading

overlookmotel left a comment •

edited

Loading

overlookmotel commented Jun 24, 2025 •

edited by graphite-app bot

Loading

overlookmotel commented Jun 24, 2025 •

edited

Loading