CodSpeed Instrumentation Performance Report

Merging #12127 will improve performances by 8.02%

_{Comparing 07-08-refactor_ast_remove_astkind_for_argument_ (2b6ace7) with main (977d3ba)}

Summary

⚡ 3 improvements
✅ 31 untouched benchmarks

Benchmarks breakdown

• refactor(ast): remove AstKind for Argument #12127 👈 (View in Graphite)
• refactor(ast): remove AstKind for PropertyKey #12108
• refactor(ast): remove AstKind for AssignmentTargetPattern #12105
• 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.

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

Prompted by this PR, me, @camc314, and @Dunqing had a lengthy conversation about whether we should add a new node type to AST CallExpressionArguments.

struct CallExpressionArguments<'a> {
    span: Span,
    arguments: Vec<'a, Argument<'a>>,
}

callee( x, y, z );
//    ^^^^^^^^^^^ CallExpressionArguments

The arguments in favour were:

• Having AstKind::CallExpressionArguments would be a simple way to answer the question "is this node inside the arguments of a call expression?" (a role which AstKind::Argument was playing up until now). That would simplify this PR a lot.
• It would store the Span of the arguments, including the ( and ). That would be helpful for attaching comments in codegen and formatter.

The main argument against was:

• Another AstKind = more AstNodes = perf cost.

There wasn't an obvious answer to the question - we debated back and forth quite a bit. But in the end we concluded:

1. Probably best to not add CallExpressionArguments AST node for now. We'll only do it later on and take the perf hit if we need to, if it turns out there's a case which cannot be solved without it.
2. Consider later on adding data to AstNodes which allows not just answering the question "what is the parent of this node?" but also "what property of the parent is this node?" (similar to Ancestor in Traverse). This would allow finding out whether a node is inside the arguments of a call expression without using Spans, and would simplify a lot of other code in the linter. It might also allow us to remove other AstKinds e.g. FormalParameters.

So, in short, conclusion was that the approach this PR is taking is good. We should remove AstKind::Argument and not replace it with anything else. I just wanted to record that we did consider other options, and the reasons why we dismissed them.

thanks Cam for your work on this.

@ulrichstark or @taearls do either of you fancy picking up and continuing these changes?

thanks Cam for your work on this.

@ulrichstark or @taearls do either of you fancy picking up and continuing these changes?

I'd be happy to take a look once I'm done with #12401!

I'm hoping to have that one ready to review later this week, so I could pick this up after that if it hasn't been taken by then.

I just opened #12401 for review, so I can start working on this now. Since I can only contribute behind a fork of the oxc repository, I will need to create a separate PR that targets this branch. Is that cool?

I'm not sure if there's a better way to do this. @camc314 @overlookmotel

yes pleas feel free to take over this branch, and just make a new PR from it. I won't push any new commits

thanks!

Superceded by #12525.