fix(ast): add RestElements in serialized AST to elements array

[overlookmotel]

A step towards #2463.

This PR adds rest onto end of elements / properties array in JSON AST for ObjectPattern, ArrayPattern, ObjectAssignmentTarget, ArrayAssignmentTarget and FormalParameters.

[overlookmotel]

• fix(ast): add RestElements in serialized AST to elements array #2567 👈
• fix(ast): rename serialized fields to camel case #2566
• main

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

Join @overlookmotel and the rest of your teammates on Graphite

[codspeed-hq]

CodSpeed Performance Report

Merging #2567 will not alter performance

_{Comparing 03-02-fix_ast_add_RestElement_s_in_serialized_AST_to_elements_array (f2cd4ed) with main (d76ee6b)}

Summary

✅ 29 untouched benchmarks

[overlookmotel]

@Boshen This is incomplete at present. But I came across one query in the process:

ObjectPattern and ArrayPattern contain field rest: Option<Box<'a, BindingRestElement<'a>>>. BindingRestElement has a span field which covers the whole rest element (including the ...).

oxc/crates/oxc_ast/src/ast/js.rs

Lines 1695 to 1699 in 78f8c2c

	pub struct BindingRestElement<'a> {
	#[cfg_attr(feature = "serde", serde(flatten))]
	pub span: Span,
	pub argument: BindingPattern<'a>,
	}

However, ObjectAssignmentTarget and ArrayAssignmentTarget just have rest: Option<AssignmentTarget<'a>>, so no span for the entire rest element:

oxc/crates/oxc_ast/src/ast/js.rs

Lines 993 to 998 in 78f8c2c

	pub struct ObjectAssignmentTarget<'a> {
	#[cfg_attr(feature = "serde", serde(flatten))]
	pub span: Span,
	pub properties: Vec<'a, AssignmentTargetProperty<'a>>,
	pub rest: Option<AssignmentTarget<'a>>,
	}

If it's useful to have a span including ..., should we add a type AssignmentRestElement which wraps AssignmentTarget here? Or if this span is not useful, should we remove BindingRestElement?

Either way, it'd be more consistent.

[Boshen]

On one hand I want the version without the ... span, to align with jsparagus https://gist.github.com/Boshen/0b481a058cd715576aaf1624d2c6d469#file-types_generated-rs-L643-L648

On another hand I want to align with the spec, which has the named node BindingRestElement

🤔 Let me experiment with which one is better next week.

[overlookmotel]

OK cool, let's leave this PR as draft for now then, as it'll need to change one way or another.

I'm guessing the span including ... might be good for source maps.

[Boshen]

After some experimentation, I'm in favor of adding AssignmentRestElement.

The span for the ellipsis ... is useful in many ways (diagnostics and finding the rest element).

[overlookmotel]

OK great. You want to do that, or shall I?

[ArnaudBarre]

Oh yeah didn't yet discover that mismatch on my testing but this is needed to be able to produce an ESTree and have correct range for this code ({a, ... b} = {})

[overlookmotel]

Does ESTree have a span for the ... alone, or for the whole rest element ...x? Or both?

[ArnaudBarre]

For ... b only

[Boshen]

OK great. You want to do that, or shall I?

Shall we get this PR merged, and I'll follow up with an update for AssignmentRestElement?

[overlookmotel]

Could we possibly do it the other way around? A lot of this PR will need to be reverted after that change.

[Boshen]

Could we possibly do it the other way around? A lot of this PR will need to be reverted after that change.

Sure, I'll dive in.

[Boshen]

Could we possibly do it the other way around? A lot of this PR will need to be reverted after that change.

Sure, I'll dive in.

Done in #2601

[overlookmotel]

Amazing! Thank you.

[overlookmotel]

@ArnaudBarre I think I've got this right. But could you please check that you can now remove the modifications for RestElements from your JS-side transform?

I'll need to fix the types created by tsify, but would be good to know the shape of the objects are right first.

[ArnaudBarre]

Didn't have time to test today. My current fork is main only so checkout of your branch was not trivial (I didn't find a settings to revert main only fork). @Boshen is there a rule for being able to work directly on the main repo? If this is too early for me I will temporary update the remote of my repo tomorrow to test it

[overlookmotel]

Oh I'm sorry, I assumed that'd be easy, or I wouldn't have asked. Don't spend your time on it. I'll fix the types, we can get this merged, and then if it's incorrect in some way, can iterate again.

[ArnaudBarre]

I started testing, it fixed a weird case I still need to understand if this is an bug from my wrapper or OXC. And I got another issue but maybe related to the binding pattern change, I will continue tonight

[overlookmotel]

OK, I think this is ready now.

Obviously there are more changes still required, but this at least merges rest into elements / properties / items - which is how ESTree represents it.

@ArnaudBarre Once it's merged please let me know if you find any mistakes. FormalParameters in particular is a bit of a pain.

[Boshen]

Amazing! I'll never figure out how serde and all these different lifetimes work ...

[Boshen]

Merge conflict 😅 Feel free to resolve and merge.

[overlookmotel]

Oh bollocks! It conflicted with my other PR. Should have stacked them.

Conflict fixed now, but please let tests pass before merging. Had to make a lot of changes to fix the conflicts.