Skip to content

Conversation

@taearls
Copy link
Contributor

@taearls taearls commented Sep 18, 2025

Part of #11490

had to open this new PR because #12525 was closed by mistake.

Motivation

AstKind::Argument was an architectural inconsistency. The AstKind system is designed for struct types, but Argument is an enum (SpreadElement | Expression). This created unnecessary complexity in the AST traversal system.

Solution

Replace AST-based argument detection with span-based utilities. Instead of traversing the tree to find AstKind::Argument nodes, we now use direct span containment checks.

New Utilities

Added to oxc_linter/src/ast_util.rs:

pub fn is_node_exact_call_argument<'a>(
    node: &AstNode<'a>,
    ctx: &LintContext<'a>
) -> bool

Checks if a node is exactly a direct argument to its parent call expression by checking if the parent is a call and the node's span matches one of the argument spans.

pub fn is_node_within_call_argument<'a>(
    node: &AstNode<'a>,
    call: &CallExpression<'a>,
    target_arg_index: usize,
) -> bool

Checks if a node is nested within a specific argument at the given index of the provided call expression.

Added to oxc_ast/src/ast_kind_impl.rs:

pub fn has_argument_with_span(&self, span: Span) -> bool

Checks if this AstKind is a call expression with an argument matching the given span.

pub fn is_callee_with_span(&self, span: Span) -> bool

Checks if this AstKind is a call expression whose callee matches the given span.

Architecture Change

Before:

// Required parent traversal and state management
match parent.kind() {
    AstKind::Argument { .. } => {
        // Check grandparent for call context
    }
}

After:

// Direct span-based detection
if is_node_exact_call_argument(node, ctx) {
    // Node is a direct call argument
}

if is_node_within_call_argument(node, call, 0) {
    // Node is within first argument
}

Impact

  • AST: Removed AstKind::Argument variant, reduced AST_TYPE_MAX from 186 → 185
  • Linter: Updated 38 rules across React, Promise, ESLint, Unicorn, Jest, and other plugins
  • Formatter: Modified parenthesization behavior for expressions in call arguments. The removal of AstKind::Argument caused the formatter to make different formatting decisions for arrow functions and object expressions used as arguments, generally producing more compact output (fewer parentheses). Snapshot tests show formatting changes while maintaining Prettier conformance pass rates.
  • Performance: Span comparisons are O(1) operations, eliminating the need for parent AST traversal in many cases

Testing

  • ✅ All linter tests pass with updated rules
  • ✅ Prettier conformance suites maintain the same passing percentage
  • ⚠️ Note: Formatter snapshot tests show output changes for some patterns (arrow functions as arguments now formatted more compactly)

Breaking Changes

None - this is an internal refactoring that doesn't affect public APIs.

@github-actions github-actions bot added A-linter Area - Linter A-semantic Area - Semantic A-ast Area - AST A-ast-tools Area - AST tools A-editor Area - Editor and Language Server A-formatter Area - Formatter C-bug Category - Bug labels Sep 18, 2025
@taearls taearls changed the title fix(ast): remove AstKind::Argument with CodSpeed performance optimizations refactor(linter): remove AstKind::Argument Sep 18, 2025
@github-actions github-actions bot added the C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior label Sep 18, 2025
@taearls taearls changed the title refactor(linter): remove AstKind::Argument refactor(linter): Remove AstKind for Argument Sep 18, 2025
@taearls taearls force-pushed the 07-08-refactor_ast_remove_astkind_for_argument_ branch from b9c702a to 1d021ce Compare September 18, 2025 17:40
@taearls taearls marked this pull request as draft September 18, 2025 17:43
@codspeed-hq
Copy link

codspeed-hq bot commented Sep 18, 2025

CodSpeed Performance Report

Merging #13902 will improve performances by 5.8%

Comparing taearls:07-08-refactor_ast_remove_astkind_for_argument_ (be22770) with main (2bbee24)1

Summary

⚡ 2 improvements
✅ 35 untouched

Benchmarks breakdown

Mode Benchmark BASE HEAD Change
Simulation linter[binder.ts] 49.2 ms 46.5 ms +5.8%
Simulation linter[react.development.js] 19.3 ms 18.7 ms +3.47%

Footnotes

  1. No successful run was found on main (d5fa3f8) during the generation of this report, so 2bbee24 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@github-actions github-actions bot added A-cli Area - CLI A-transformer Area - Transformer / Transpiler labels Sep 18, 2025
@taearls taearls marked this pull request as ready for review September 18, 2025 19:29
@taearls
Copy link
Contributor Author

taearls commented Sep 18, 2025

the CI is fully passing now! I still need to take some time to review my own changes, but I think generally this is ready for review now

@Sysix Sysix removed their request for review September 18, 2025 19:34
@taearls taearls force-pushed the 07-08-refactor_ast_remove_astkind_for_argument_ branch 3 times, most recently from 206c534 to 3356209 Compare September 27, 2025 17:32
@taearls taearls requested a review from leaysgur as a code owner September 28, 2025 16:09
@taearls taearls force-pushed the 07-08-refactor_ast_remove_astkind_for_argument_ branch from f5a0c3a to 65e8711 Compare November 7, 2025 15:51
taearls added a commit to taearls/oxc that referenced this pull request Nov 7, 2025
Add `has_argument_with_span()` and `is_callee_with_span()` helper methods to `AstKind` to simplify AST traversal after the removal of `AstKind::Argument`.

These methods help downstream tools (like rolldown) avoid verbose span-matching patterns when determining:
- Whether a node is an argument to a CallExpression/NewExpression
- Whether a node is the callee of a CallExpression/NewExpression

Example usage:
```rust
// Before: verbose span matching
if let Some(arg) = call_expr.arguments.iter()
    .find(|arg| arg.span() == node.span()) { ... }

// After: clean API
if parent.has_argument_with_span(node.span()) { ... }
```

Addresses review feedback from PR oxc-project#13902.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@overlookmotel
Copy link
Member

@taearls Sorry this has taken so long to get to, and thanks for constantly keeping the PR updated. We're aiming to (finally) get this merged next week. @camc314 and @Dunqing are reviewing the parts related to linter and formatter respectively, and then I'm going to do a 2nd pass through the whole thing. I'll co-ordinate with them both on Monday.

@taearls
Copy link
Contributor Author

taearls commented Nov 7, 2025

@taearls Sorry this has taken so long to get to, and thanks for constantly keeping the PR updated. We're aiming to (finally) get this merged next week. @camc314 and @Dunqing are reviewing the parts related to linter and formatter respectively, and then I'm going to do a 2nd pass through the whole thing. I'll co-ordinate with them both on Monday.

hey! I appreciate that a lot. It's all good! I know this is a big change so I can understand that it's harder to prioritize this with everything else that's in flight right now, especially with the formatter.

I just rebased onto main earlier today and got the conformance tests to the same baseline as was on main (with some minor improvements!) so it's all ready to go.

@overlookmotel
Copy link
Member

Amazing. Thank you.

Can I please ask one thing? Could you please rebase onto main (rebase not merge) and squash everything down to a single commit? The list of commits on this page is so long that it's quite hard to find the review comments!

@taearls taearls force-pushed the 07-08-refactor_ast_remove_astkind_for_argument_ branch from 47d87eb to ee46d2c Compare November 7, 2025 22:16
@taearls
Copy link
Contributor Author

taearls commented Nov 7, 2025

Amazing. Thank you.

Can I please ask one thing? Could you please rebase onto main (rebase not merge) and squash everything down to a single commit? The list of commits on this page is so long that it's quite hard to find the review comments!

done!

@camc314 camc314 force-pushed the 07-08-refactor_ast_remove_astkind_for_argument_ branch 2 times, most recently from e4b038f to f777c5c Compare November 10, 2025 14:41
@camc314 camc314 force-pushed the 07-08-refactor_ast_remove_astkind_for_argument_ branch from f777c5c to 9ef5df9 Compare November 11, 2025 15:36
@camc314 camc314 force-pushed the 07-08-refactor_ast_remove_astkind_for_argument_ branch from 9ef5df9 to be22770 Compare November 11, 2025 15:48
@camc314 camc314 merged commit b2af6b5 into oxc-project:main Nov 11, 2025
35 of 36 checks passed
@camc314
Copy link
Contributor

camc314 commented Nov 11, 2025

@taearls Thank you for your work on this!

@overlookmotel
Copy link
Member

overlookmotel commented Nov 11, 2025

Yes, thank you thank you thank you @taearls. I've actually not reviewed yet, but we decided best to just get it merged now before it accrues more merge conflicts, and I'll go through it later this week, and I can make any follow-ups then.

@overlookmotel overlookmotel removed the request for review from leaysgur November 11, 2025 16:00
@taearls
Copy link
Contributor Author

taearls commented Nov 11, 2025

Yes, thank you thank you thank you @taearls. I've actually not reviewed yet, but we decided best to just get it merged now before it accrues more merge conflicts, and I'll go through it later this week, and I can make any follow-ups then.

That makes sense. I'm happy to help! This effort taught me a lot about working with this project, and I feel much more comfortable in it as a whole because of it.

Let me know if I can be helpful with any follow-up items.

graphite-app bot pushed a commit that referenced this pull request Nov 17, 2025
…ent` (#15675)

Revert all changes in the formatter that were introduced in #13902, as none of them are the ideal solution for the change in the formatter.
graphite-app bot pushed a commit that referenced this pull request Nov 17, 2025
…was removed (#15676)

Re-fix all cases that are relevant to the change in #13902. I've manually checked that the Prettier coverage compatibility has gone back exactly to the state before #13902.
@leaysgur leaysgur changed the title refactor(linter): Remove AstKind for Argument refactor(ast)!: Remove AstKind for Argument Nov 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ast Area - AST A-ast-tools Area - AST tools A-cfg Area - Control Flow Graph A-cli Area - CLI A-codegen Area - Code Generation A-editor Area - Editor and Language Server A-formatter Area - Formatter A-isolated-declarations Isolated Declarations A-linter Area - Linter A-minifier Area - Minifier A-parser Area - Parser A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler C-bug Category - Bug C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants