Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(transformer): async-to-generator plugin. #5590

Merged
merged 26 commits into from
Oct 14, 2024

Conversation

7086cmd
Copy link
Contributor

@7086cmd 7086cmd commented Sep 7, 2024

Tests are still not passed. A lot need to do yet.

Copy link

graphite-app bot commented Sep 7, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@7086cmd 7086cmd force-pushed the feat/transform-async-to-generator branch from 136bfbc to aad667e Compare September 7, 2024 16:45
@github-actions github-actions bot added the A-transformer Area - Transformer / Transpiler label Sep 7, 2024
Copy link

codspeed-hq bot commented Sep 7, 2024

CodSpeed Performance Report

Merging #5590 will not alter performance

Comparing 7086cmd:feat/transform-async-to-generator (58863b5) with main (67ad08a)

Summary

✅ 29 untouched benchmarks

@7086cmd
Copy link
Contributor Author

7086cmd commented Sep 7, 2024

It appears that the iteration of the ast tree incurs performance degradation. However, it necessitates the duplication of statements for replacement. Are there alternative approaches that could be considered?

@7086cmd 7086cmd force-pushed the feat/transform-async-to-generator branch from 45a0ff9 to 3d069a3 Compare September 7, 2024 23:05
@Dunqing Dunqing requested review from Dunqing, Boshen and overlookmotel and removed request for Dunqing, Boshen and overlookmotel September 9, 2024 03:42
@Boshen
Copy link
Member

Boshen commented Sep 9, 2024

@7086cmd Can you hold onto this and let us take a look first? We may end up implementing this differently with architectural changes.

@7086cmd
Copy link
Contributor Author

7086cmd commented Sep 9, 2024

@7086cmd Can you hold onto this and let us take a look first? We may end up implementing this differently with architectural changes.

Ok. It does need some refactor, especially when it comes some polyfills here, we'd better do as what babel does: from js text to ast, which is visually more vibrant and more convenient. Replacing to multiple statements is also a handful problem.

@7086cmd
Copy link
Contributor Author

7086cmd commented Sep 9, 2024

For this PR, I would prefer what esbuild does: more visible and no need for refactor for multiple statements here: https://esbuild.github.io/try/#dAAwLjIzLjEALS10YXJnZXQ9Y2hyb21lNTAAYXN5bmMgZnVuY3Rpb24gYSgpIHsKICAgIGF3YWl0IDE7Cn0KCmFzeW5jIChkKSA9PiB7CiAgICBhd2FpdCAyOwogICAgYXdhaXQgZCgpCn0K.

@7086cmd 7086cmd force-pushed the feat/transform-async-to-generator branch 4 times, most recently from 5d2d419 to 2dd3fd1 Compare September 22, 2024 04:46
@7086cmd 7086cmd marked this pull request as ready for review September 22, 2024 04:46
@overlookmotel
Copy link
Contributor

I've not dug into this in full, but at first glance, it looks like this is using ESBuild's technique, rather than Babel's.

I can see why you've chosen to do that, but I'm afraid we need to follow @babel/plugin-transform-async-to-generator exactly.

There's a couple of reasons for this:

1. Runtime performance

ESBuild's technique is easier to implement, and maybe faster, but the code it generates will be slower at runtime.

Input:

async function foo() {
  await bar();
}

Babel output:

var foo = _asyncToGenerator(function*() {
  yield bar();
});

ESBuild output:

function foo() {
  return __async(this, null, function*() {
    yield bar();
  });
}

Babel's version creates a single function foo. But ESBuild's version has the cost of generating a new function function*() every time you call foo.

Basically, Babel's version is better.

2. Testing

Our testing infra depends on passing Babel's tests. Sometimes it's annoying that we have to produce exactly same output as Babel, even in cosmetic things like variable naming. But the advantage is that we don't have to write loads of tests ourselves. I don't know about you, but I don't particularly enjoy writing tests!

Once we have the transformer passing all Babel's tests, our next step will be run the transformer on all source files for the top 3000 npm packages, like monitor-oxc does. We'll compare Oxc's output to Babel's output transforming the same files. Bombarding our transformer with so many test cases will no doubt reveal many bugs, and once we fix them we can have extremely high confidence that the transformer is behaving correctly. But for this to work, we need Oxc's transformer to emulate Babel exactly.

Only once we've got all that passing, then we'll bootstrap ourselves, and we can start making Oxc produce better output than Babel. But we haven't got to that stage yet.

@7086cmd 7086cmd force-pushed the feat/transform-async-to-generator branch 2 times, most recently from dac2126 to 170072a Compare September 24, 2024 10:23
Copy link
Contributor Author

@7086cmd 7086cmd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some descs.

@7086cmd 7086cmd force-pushed the feat/transform-async-to-generator branch from 2d8ece1 to 28b3074 Compare September 30, 2024 11:28
@Dunqing
Copy link
Member

Dunqing commented Oct 14, 2024

Thanks for your work! I am going to merge this PR and then complete this plugin later

@7086cmd
Copy link
Contributor Author

7086cmd commented Oct 14, 2024

Thanks for your work! I am going to merge this PR and then complete this plugin later

Thanks! Just a heads-up: this PR is from my personal forked repository, so we can't merge it using Graphite. If that's inconvenient, I'm willing to reopen it from the origin repository.

@Dunqing
Copy link
Member

Dunqing commented Oct 14, 2024

Thanks! Just a heads-up: this PR is from my personal forked repository, so we can't merge it using Graphite. If that's inconvenient, I'm willing to reopen it from the origin repository.

It doesn't matter, I have been rebasing the main branch

Copy link
Member

@Dunqing Dunqing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Dunqing Dunqing merged commit a9260cf into oxc-project:main Oct 14, 2024
28 checks passed
@7086cmd 7086cmd deleted the feat/transform-async-to-generator branch October 14, 2024 11:57
Boshen added a commit that referenced this pull request Oct 19, 2024
## [0.32.0] - 2024-10-19

- c0e9d7e codegen: [**BREAKING**] `Codegen::into_source_text` consume
`Codegen` (#6539) (overlookmotel)

- 782f0a7 codegen: [**BREAKING**] Rename `print_char` method to
`print_ascii_byte` (#6512) (overlookmotel)

- 91c87dd codegen: [**BREAKING**] Remove `Codegen::enableSourceMap` API
(#6452) (Boshen)

- 7645e5c codegen: [**BREAKING**] Remove CommentOptions API (#6451)
(Boshen)

- 5200960 oxc: [**BREAKING**] Remove passing `Trivias` around (#6446)
(Boshen)

- 2808973 ast: [**BREAKING**] Add `Program::comments` (#6445) (Boshen)

### Features

- 5ee1ef3 allocator: Add `Vec::into_boxed_slice` (#6195) (DonIsaac)
- d9718ad ast_tools: Support `#[scope(exit_before)]` (#6350) (DonIsaac)
- e5ed6a5 codegen: Print negative numbers (#6624) (Boshen)
- 15c04e5 ecmascript: Add feature flag for constant evaluation (Boshen)
- d11770d ecmascript: Add `StringToNumber` (#6576) (Boshen)
- e561880 ecmascript: Add constant_evaluation and side_effects code
(#6550) (Boshen)
- 3556062 ecmascript: Add `ConstantEvaluation` (#6549) (Boshen)
- 39c2e66 ecmascript: Add `ToBigInt` and `StringToBigInt` (#6508)
(Boshen)
- 6f22538 ecmascript: Add `ToBoolean`, `ToNumber`, `ToString` (#6502)
(Boshen)
- 15dfc1d isolated-declarations: Impl `Default` for options (#6372)
(DonIsaac)
- 071e564 minifier: Finish implementing folding object expressions
(#6586) (camc314)
- 590925a minifier: Finish implementing folding array expressions
(#6575) (camc314)
- ef237cf minifier: Complete implementation of statement fusion (#6566)
(camc314)
- 97c8a36 minifier: Implement `collapse-variable-declarations` (#6464)
(dalaoshu)
- 096e590 minifier: Implement folding `charAt` string fns (#6436)
(camc314)
- e5a6f5d minifier: Implement converting template literals to strings
(#6486) (camc314)
- 14d0590 minifier: Implement folding of simple function calls
(`Boolean`) (#6484) (camc314)
- 7fbc7b6 minifier: Implement folding of simple function calls
(`String`) (#6483) (camc314)
- a4f57a4 minifier: Implement folding `indexOf` and `lastIndexOf` string
fns (#6435) (camc314)
- 3677ef8 minifier: Dce ExpressionStatements with no side effect (#6457)
(7086cmd)
- 06ea121 minifier: Fold for statement (#6450) (7086cmd)
- a9544ae minifier: Partially implement minification for some known
string methods (#6424) (camc314)
- 9dc4ee9 minifier: Implement block stmt support for `StatementFusion`
(#6422) (camc314)
- ebbf77d minifier: Implement calculations for NumberValue (#6419)
(7086cmd)
- 97ac179 minifier: Arithmetic operations for infinity. (#6332)
(7086cmd)
- 13b0b0b minifier: Fold literal object constructors on window (#6379)
(dalaoshu)
- e310e52 parser: Generate `Serialize` impls in ast_tools (#6404)
(ottomated)
- 58467a5 parser: Better handling of invalid modifiers (#6482)
(DonIsaac)
- 8ea6b72 parser: Better errors for reserved words used as identifier
names (#6478) (DonIsaac)
- b5b0af9 regular_expression: Support RegExp Modifiers (#6410)
(leaysgur)
- a01a5df transformer: Pass TransformerCtx to async-to-generator plugin
(#6633) (Dunqing)
- a9260cf transformer: `async-to-generator` plugin. (#5590) (Ethan Goh)
- 8fe1b0a transformer: Support helper loader (#6162) (Dunqing)
- ab51c2a transformer: Support `DefaultImport` in `ModuleImports`
(#6434) (Dunqing)
- a3dea9c transformer/async-to-generator: Handle arrow-function
correctly (#6640) (Dunqing)
- 41c8675 transformer/object-rest-spread: Using helper loader (#6449)
(Dunqing)

### Bug Fixes

- ba385fc codegen: Panic occurred when printing the comment of the right
parenthesis (#6593) (Dunqing)
- 02bfbfe codegen: Preserve parenthesis for `ChainExpression` (#6430)
(Dunqing)
- 2ade16e codegen: Invalid codegen when `in` inside bin expr in or loop
(#6431) (camc314)
- 6896efc codegen: Fix `in` in sequence expr in for loops (#6428)
(camc314)
- 7cc05f1 data_structures: Fix compilation failure on older Rust
versions (#6526) (overlookmotel)
- 2ce3e5f identifier: Add `ZWSP` to `is_irregular_whitespace` (#6662)
(Boshen)
- 2673397 isolated_declarations: Fix potential memory leak (#6622)
(overlookmotel)
- 389d261 minifier: `~~` operator should only work on numbers (#6598)
(Boshen)
- 16bea12 minifier: Use `to_js_string()` instead of `fs64::to_string`
(#6597) (Boshen)
- a71e8a0 minifier: Preserve init variable declarations when removing
`for` statements during DCE (#6551) (magic-akari)
- 721cf0f parser: Should be treated comments where after `(` as leading
comments of next token (#6588) (Dunqing)
- b1bf12c parser: Do not parse `as` and `satisfies` expression in
javascript (#6442) (Boshen)
- 9f9057b regular_expression: Fixed control Y regular expression (#6524)
(Tapan Prakash)
- c822b48 regular_expression: Fix CharacterClass negative codegen
(#6415) (leaysgur)
- 384d5be regular_expression: Flatten Spans on regex AST nodes (#6396)
(ottomated)
- 834ee2a semantic: `TSConditionalType` scope enter/exit locations
(#6351) (DonIsaac)
- 1d3d256 transformer: Correctly trim JSX (#6639) (magic-akari)
- c6f2b5f transformer: `HelperLoader` common transform: do not assume
`babelHelpers` is global (#6569) (overlookmotel)
- 85d93ed transformer: Arrow function transform: correctly resolve
`this` in class accessor properties (#6386) (overlookmotel)

### Performance

- 77f3a1a codegen: Check last char with byte methods (#6509)
(overlookmotel)
- 18b68ff codegen: Optimize `CodeBuffer::print_ascii_byte` (#6516)
(overlookmotel)
- 4d8bc8c parser: Precompute `is_typescript` (#6443) (Boshen)
- 7c20056 regex: Reduce string allocations in `Display` impls (#6528)
(DonIsaac)
- f70a413 transformer: Object spread transform: do not lookup `Object`
binding if not needed (#6570) (overlookmotel)
- ac77c87 traverse: Optimize `TraverseScoping::generate_uid_name`
(#6663) (overlookmotel)

### Documentation

- 9f555d7 allocator: Clarify docs for `Box` (#6625) (overlookmotel)
- 06e75b0 allocator: Enable lint warnings on missing docs, and add
missing doc comments (#6613) (DonIsaac)
- 7e909a7 codegen: Fix example for `CodeBuffer::print_ascii_bytes`
(#6535) (overlookmotel)
- 235d357 codegen: Improve doc comments for `CodeBuffer` (#6511)
(overlookmotel)
- c8fa2eb codegen: Correct and reformat doc comments for `CodeBuffer`
(#6504) (overlookmotel)
- 40d1ee4 codegen: Fix and reformat `CodeBuffer` examples (#6499)
(overlookmotel)
- de22b81 data-structures: Enable lint warnings on missing docs, and add
missing doc comments (#6612) (DonIsaac)
- 9e9fa9e span: Enable lint warnings on missing docs (#6617)
(overlookmotel)
- 6a194f9 span: Document validity of `ModuleKind::Unambiguous` (#6423)
(Boshen)
- 335b7f2 syntax: Enable lint warnings on missing docs, and add a lot of
documentation (#6611) (DonIsaac)
- f3451d7 transformer/async-to-generator: Remove empty lines from doc
comment (#6642) (overlookmotel)
- 448388a transformer/module_imports: Update outdated comments (#6574)
(Dunqing)

### Refactor

- 073b02a ast: Type params field before params in TS function
declaration types (#6391) (overlookmotel)
- 458f8f3 ast_tools: Consistent comments on `AstBuilder` methods (#6664)
(overlookmotel)
- 51fc63d codegen: Rename `CodeBuffer::print_bytes_unchecked` method
(#6517) (overlookmotel)
- 05a2ebd codegen: Reorder dependencies in `Cargo.toml` (#6514)
(overlookmotel)
- e7f3e28 codegen: Rename var in `CodeBuffer` (#6510) (overlookmotel)
- 1bbd383 codegen: Rename `CodeBuffer::print_ascii_bytes` method (#6507)
(overlookmotel)
- cd9fe9e codegen: Rename vars in `CodeBuffer` methods (#6506)
(overlookmotel)
- fc536a5 codegen: Inline `CodeBuffer` methods (#6501) (overlookmotel)
- 7420620 codegen: Add `CodeBuffer::as_bytes` method (#6498)
(overlookmotel)
- 8ae174b codegen: Rename `CodeBuffer::print_byte_unchecked` method
(#6496) (overlookmotel)
- 5843e01 codegen: Shorten `CodeBuffer::take_source_text` (#6495)
(overlookmotel)
- 951def6 codegen: Clarify safety comments in `CodeBuffer` (#6494)
(overlookmotel)
- 84a51ee codegen: Rename vars in `CodeBuffer` (#6493) (overlookmotel)
- 05bd616 codegen: Add line break (#6492) (overlookmotel)
- 204bf55 codegen: Add `CodeBuffer` to fix soundness hole (#6148)
(DonIsaac)
- 702b574 codegen: Only print necessary parentheses in TSAsExpression
(#6429) (Dunqing)
- aa6ba24 ecmascript: Improve string to number conversion (#6577)
(magic-akari)
- 6d041fb ecmascript: Remove `NumberValue` (#6519) (Boshen)
- 856cab5 ecmascript: Move ToInt32 from `oxc_syntax` to `oxc_ecmascript`
(#6471) (Boshen)
- 1ba2a24 ecmascript: Remove `HasProto` which is not part of the spec
(#6470) (Boshen)
- a504f96 isolated-declarations: Mark return struct as non exhaustive
(#6374) (DonIsaac)
- f4cdc56 minifier: Use constant folding unary expression from
`oxc_ecmascript` (#6647) (Boshen)
- 67ad08a minifier: Unify `ValueType` (#6545) (Boshen)
- bbca743 minifier: Move string methods to `oxc_ecmascript` (#6472)
(Boshen)
- 702c049 minifier: Move compress block to dce (#6468) (7086cmd)
- 46a38c6 minifier: Remove allow `clippy::unused_self` (#6441) (Boshen)
- 994b60b minifier: Use builtin get_number_value. (#6335) (7086cmd)
- 435a89c oxc: Remove useless `allocator.alloc(program)` calls (#6571)
(Boshen)
- c45723b parser: Fix typo in var name (#6500) (overlookmotel)
- 1a90ec4 rust: Backport v1.82.0 changes to main branch first (#6690)
(Boshen)
- 3faee66 span: Remove unused `ContentHash::content_hash_slice` (#6609)
(DonIsaac)
- 9281234 transformer: Shorten imports (#6643) (overlookmotel)
- 3af0840 transformer: `HelperLoader`: add import immediately (#6601)
(overlookmotel)
- f81aa7f transformer: `HelperLoader` common transform: comments (#6599)
(overlookmotel)
- 679cc68 transformer: `HelperLoader` common transform: construct string
directly in arena (#6596) (overlookmotel)
- c346ebb transformer: `HelperLoader` common transform: `Helper` enum
(#6595) (overlookmotel)
- 7a028b3 transformer: Remove unnecessary `#![warn]` attr (#6585)
(overlookmotel)
- 8c6afe0 transformer: Reorder imports (#6582) (overlookmotel)
- 779ff46 transformer: `HelperLoader` common transform: `Helper` struct
(#6568) (overlookmotel)
- bc24a24 transformer: `HelperLoader` common transform: use hashmap
`Entry` API (#6567) (overlookmotel)
- 9f02fc7 transformer: `HelperLoader` common transform: re-order fields
(#6565) (overlookmotel)
- 50ecade transformer: `HelperLoader` common transform: remove `Rc`s
(#6564) (overlookmotel)
- 1c1e9fc transformer: `HelperLoader` common transform: reorder methods
(#6563) (overlookmotel)
- c9054c8 transformer: Rename `ImportKind` to `Import` (#6561)
(overlookmotel)
- 9542c4e transformer: Add more specific methods to `ModuleImportsStore`
(#6560) (overlookmotel)
- 7e57a1d transformer: `ImportKind` use `BoundIdentifier` (#6559)
(overlookmotel)
- 602df9d transformer: Re-order fields of `Common` and `TransformCtx`
(#6562) (overlookmotel)
- 390abca transformer/async-to-generator: Use `helper_call_expr` (#6634)
(Dunqing)
- 2ff917f transformer/async-to-generator: Move internal methods below
entry points (#6632) (Dunqing)

### Styling

- fb916b2 regular_expression: Re-order dependencies in `Cargo.toml`
(#6672) (overlookmotel)
- 9d43a11 transformer: Re-order dependencies (#6659) (overlookmotel)

### Testing

- e7c89a5 codegen: Uncomment passed tests that are related to trailing
comments (#6589) (Dunqing)
- d816b0b codegen: Add test for `CodeBuffer::print_byte_unchecked`
(#6497) (overlookmotel)
- c5deb32 minifier: Port the rest of tests (#6420) (7086cmd)
- e59da61 minifier: Add all test cases for
`collapse_variable_declarations` (#6421) (dalaoshu)
- 73d6a4a minifier: Port all replace_known_methods tests. (#6418)
(7086cmd)

---------

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transformer Area - Transformer / Transpiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants