fix(ast): add `RestElement`s in serialized AST to elements array by overlookmotel · Pull Request #2567 · oxc-project/oxc

overlookmotel · 2024-03-02T02:53:10Z

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 · 2024-03-02T02:53:23Z

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 · 2024-03-02T03:00:19Z

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 · 2024-03-02T03:08:42Z

@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 · 2024-03-02T14:55:27Z

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 · 2024-03-02T17:10:12Z

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 · 2024-03-03T14:17:00Z

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 · 2024-03-03T15:16:02Z

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

ArnaudBarre · 2024-03-03T23:46:09Z

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 · 2024-03-04T00:47:19Z

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

ArnaudBarre · 2024-03-04T01:55:07Z

For ... b only

Boshen · 2024-03-04T03:43:08Z

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 · 2024-03-04T03:52:38Z

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

Boshen · 2024-03-04T04:29:25Z

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 · 2024-03-04T06:30:56Z

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 · 2024-03-04T11:57:49Z

Amazing! Thank you.

overlookmotel · 2024-03-04T14:26:30Z

@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 · 2024-03-05T01:04:22Z

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 · 2024-03-05T01:30:42Z

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 · 2024-03-06T09:58:39Z

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 · 2024-03-08T01:39:08Z

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 · 2024-03-08T02:30:15Z

Merge conflict 😅 Feel free to resolve and merge.

overlookmotel · 2024-03-08T02:47:37Z

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.

Fix a mistake I made in #2567. Length that `serialize_seq` is called with should only be `+1` if there is a rest element being added on the end. Makes no difference for serializing to JSON, as JSON serializer doesn't use the `len` value, but still better to get it right.

github-actions bot added the A-ast Area - AST label Mar 2, 2024

overlookmotel mentioned this pull request Mar 2, 2024

fix(ast): rename serialized fields to camel case #2566

Merged

Base automatically changed from 03-02-fix_ast_rename_serialized_fields_to_camel_case to main March 2, 2024 14:47

Boshen mentioned this pull request Mar 4, 2024

feat(ast): add AssignmentRestElement #2598

Closed

overlookmotel force-pushed the 03-02-fix_ast_add_RestElement_s_in_serialized_AST_to_elements_array branch 2 times, most recently from 50cbb11 to 07c5190 Compare March 4, 2024 14:20

overlookmotel marked this pull request as ready for review March 4, 2024 14:21

overlookmotel marked this pull request as draft March 4, 2024 14:27

overlookmotel force-pushed the 03-02-fix_ast_add_RestElement_s_in_serialized_AST_to_elements_array branch 3 times, most recently from 72c3c15 to eba945f Compare March 7, 2024 23:14

overlookmotel force-pushed the 03-02-fix_ast_add_RestElement_s_in_serialized_AST_to_elements_array branch 2 times, most recently from 4cd286f to 6981385 Compare March 8, 2024 01:31

overlookmotel marked this pull request as ready for review March 8, 2024 01:36

Boshen approved these changes Mar 8, 2024

View reviewed changes

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

f2cd4ed

overlookmotel force-pushed the 03-02-fix_ast_add_RestElement_s_in_serialized_AST_to_elements_array branch from 6981385 to f2cd4ed Compare March 8, 2024 02:46

Boshen enabled auto-merge (squash) March 8, 2024 02:49

Boshen merged commit 88f94bb into main Mar 8, 2024

Boshen deleted the 03-02-fix_ast_add_RestElement_s_in_serialized_AST_to_elements_array branch March 8, 2024 02:56

This was referenced Mar 9, 2024

fix(ast): fix serializing rest elements #2652

Merged

TS types for RestElement incorrect #2658

Closed

Uh oh!

Conversation

overlookmotel commented Mar 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

overlookmotel commented Mar 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codspeed-hq bot commented Mar 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #2567 will not alter performance

Summary

Uh oh!

overlookmotel commented Mar 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Boshen commented Mar 2, 2024

Uh oh!

overlookmotel commented Mar 2, 2024

Uh oh!

Boshen commented Mar 3, 2024

Uh oh!

overlookmotel commented Mar 3, 2024

Uh oh!

ArnaudBarre commented Mar 3, 2024

Uh oh!

overlookmotel commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArnaudBarre commented Mar 4, 2024

Uh oh!

Boshen commented Mar 4, 2024

Uh oh!

overlookmotel commented Mar 4, 2024

Uh oh!

Boshen commented Mar 4, 2024

Uh oh!

Boshen commented Mar 4, 2024

Uh oh!

overlookmotel commented Mar 4, 2024

Uh oh!

overlookmotel commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArnaudBarre commented Mar 5, 2024

Uh oh!

overlookmotel commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArnaudBarre commented Mar 6, 2024

Uh oh!

overlookmotel commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Boshen left a comment

Choose a reason for hiding this comment

Uh oh!

Boshen commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

overlookmotel commented Mar 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

overlookmotel commented Mar 2, 2024 •

edited

Loading

overlookmotel commented Mar 2, 2024 •

edited

Loading

codspeed-hq bot commented Mar 2, 2024 •

edited

Loading

overlookmotel commented Mar 2, 2024 •

edited

Loading

overlookmotel commented Mar 4, 2024 •

edited

Loading

overlookmotel commented Mar 4, 2024 •

edited

Loading

overlookmotel commented Mar 5, 2024 •

edited

Loading

overlookmotel commented Mar 8, 2024 •

edited

Loading

Boshen commented Mar 8, 2024 •

edited

Loading