Skip to content

fix(transformer/async-generator-functions): incorrect transformation for for await if it's not placed in a block#7148

Merged
graphite-app[bot] merged 1 commit intomainfrom
11-06-fix_transformer_async-generator-functions_incorrect_transformation_for_if_it_s_not_placed_in_a_block
Nov 6, 2024
Merged

fix(transformer/async-generator-functions): incorrect transformation for for await if it's not placed in a block#7148
graphite-app[bot] merged 1 commit intomainfrom
11-06-fix_transformer_async-generator-functions_incorrect_transformation_for_if_it_s_not_placed_in_a_block

Conversation

@Dunqing
Copy link
Member

@Dunqing Dunqing commented Nov 5, 2024

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 5, 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.

Copy link
Member Author

Dunqing commented Nov 5, 2024

@codspeed-hq
Copy link

codspeed-hq bot commented Nov 5, 2024

CodSpeed Performance Report

Merging #7148 will not alter performance

Comparing 11-06-fix_transformer_async-generator-functions_incorrect_transformation_for_if_it_s_not_placed_in_a_block (b2a888d) with main (d1d1874)

Summary

✅ 30 untouched benchmarks

@Dunqing Dunqing force-pushed the 11-05-fix_transformer_async-generator-functions_transform_incorrectly_for_if_it_s_in_labeledstatement branch from 9bc8ec1 to 80220ca Compare November 5, 2024 17:24
@Dunqing Dunqing force-pushed the 11-06-fix_transformer_async-generator-functions_incorrect_transformation_for_if_it_s_not_placed_in_a_block branch from 503e2e5 to 77c683c Compare November 5, 2024 17:24
@Dunqing Dunqing force-pushed the 11-05-fix_transformer_async-generator-functions_transform_incorrectly_for_if_it_s_in_labeledstatement branch from 80220ca to be47109 Compare November 6, 2024 02:57
@Dunqing Dunqing force-pushed the 11-06-fix_transformer_async-generator-functions_incorrect_transformation_for_if_it_s_not_placed_in_a_block branch from 77c683c to 7d17618 Compare November 6, 2024 02:57
@Dunqing Dunqing force-pushed the 11-05-fix_transformer_async-generator-functions_transform_incorrectly_for_if_it_s_in_labeledstatement branch from be47109 to c46555a Compare November 6, 2024 03:02
@Dunqing Dunqing force-pushed the 11-06-fix_transformer_async-generator-functions_incorrect_transformation_for_if_it_s_not_placed_in_a_block branch from 7d17618 to 122f94c Compare November 6, 2024 03:02
@Dunqing Dunqing force-pushed the 11-05-fix_transformer_async-generator-functions_transform_incorrectly_for_if_it_s_in_labeledstatement branch from c46555a to 9c06d06 Compare November 6, 2024 03:04
@Dunqing Dunqing force-pushed the 11-06-fix_transformer_async-generator-functions_incorrect_transformation_for_if_it_s_not_placed_in_a_block branch from 122f94c to 62f3f9c Compare November 6, 2024 03:04
@Dunqing Dunqing changed the base branch from 11-05-fix_transformer_async-generator-functions_transform_incorrectly_for_if_it_s_in_labeledstatement to graphite-base/7148 November 6, 2024 03:30
@Dunqing Dunqing force-pushed the 11-06-fix_transformer_async-generator-functions_incorrect_transformation_for_if_it_s_not_placed_in_a_block branch from 62f3f9c to c72d238 Compare November 6, 2024 04:00
@Dunqing Dunqing force-pushed the graphite-base/7148 branch from 9c06d06 to 19892ed Compare November 6, 2024 04:00
@Dunqing Dunqing changed the base branch from graphite-base/7148 to main November 6, 2024 04:01
@Dunqing Dunqing force-pushed the 11-06-fix_transformer_async-generator-functions_incorrect_transformation_for_if_it_s_not_placed_in_a_block branch from c72d238 to 56fbe82 Compare November 6, 2024 04:01
@Dunqing Dunqing added the 0-merge Merge with Graphite Merge Queue label Nov 6, 2024
Copy link
Member Author

Dunqing commented Nov 6, 2024

Merge activity

  • Nov 6, 12:50 AM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Nov 6, 12:50 AM EST: A user added this pull request to the Graphite merge queue.
  • Nov 6, 12:55 AM EST: A user merged this pull request with the Graphite merge queue.

@Dunqing Dunqing force-pushed the 11-06-fix_transformer_async-generator-functions_incorrect_transformation_for_if_it_s_not_placed_in_a_block branch from 56fbe82 to b2a888d Compare November 6, 2024 05:51
@graphite-app graphite-app bot merged commit b2a888d into main Nov 6, 2024
@graphite-app graphite-app bot deleted the 11-06-fix_transformer_async-generator-functions_incorrect_transformation_for_if_it_s_not_placed_in_a_block branch November 6, 2024 05:55
Dunqing pushed a commit that referenced this pull request Nov 6, 2024
…tion param (#7165)

Follow-on after stack up to #7148. Remove unused `&mut self` function params. This makes calling these functions slightly cheaper, as unused data doesn't get passed to the function.
Dunqing pushed a commit that referenced this pull request Nov 6, 2024
…ion param (#7166)

Follow-on after stack up to #7148. Remove unused `&self` function param. This makes calling this function slightly cheaper, as unused data doesn't get passed to the function.
Dunqing pushed a commit that referenced this pull request Nov 6, 2024
…low(clippy::unused_self)]` attrs (#7167)

Follow-on after stack up to #7148. Remove 2 x `#[allow(clippy::unused_self)]` attrs where that lint isn't triggered because the functions do use their `&self` param.
Dunqing pushed a commit that referenced this pull request Nov 6, 2024
…#7168)

Follow-on after stack up to #7148. Style nit. End statements with semi-colons. Usually clippy demands this, not sure why it didn't here.
Dunqing pushed a commit that referenced this pull request Nov 6, 2024
…ram (#7169)

Follow-on after stack up to #7148.

Small optimization. `ArrowFunctionExpression` is 56 bytes, whereas `ArenaBox<ArrowFunctionExpression>` is 8 bytes. So it's preferable to pass the `ArenaBox<ArrowFunctionExpression>` to `transform_arrow_function_expression` and call `unbox` on it there instead of unboxing *before* passing to the function.
Dunqing pushed a commit that referenced this pull request Nov 6, 2024
…ne_in` on `LabelIdentifier` (#7172)

Follow-on after stack up to #7148.

Small optimization. `LabelIdentifier` is `Clone` so we can use `Clone::clone` to clone it, instead of `CloneIn::clone_in`.

The difference is that `clone_in` makes a 2nd copy of the `Atom`'s string in the arena, whereas `clone` creates a new `Atom` referencing the same string. This is an unfortunate shortcoming of `CloneIn` (oxc-project/backlog#96), but the more we can avoid `CloneIn`, the better.
Dunqing pushed a commit that referenced this pull request Nov 6, 2024
…Vec` as `ArenaVec` (#7173)

Follow-on after stack up to #7148. Style nit. Following our convention (#6996), import `oxc_allocator::Vec` as `ArenaVec`, to prevent ambiguity.
Dunqing pushed a commit that referenced this pull request Nov 6, 2024
… whether within an async generator function (#7170)

Follow-on after stack up to #7148.

Split `is_inside_async_generator_function` into 2 functions `yield_is_inside_async_generator_function` and `async_is_inside_async_generator_function`.

There are a couple of differences between `yield` and `await`, which we can leverage to make both these functions simpler:

* `yield` cannot appear at top level (there's no such thing as "top level yield").
* `yield` cannot appear in an arrow function (there's no such thing as a "generator arrow function").

In addition:

* Because each function now handles a specific case, no need to check if function containing `async` is an async function. It must be. Ditto for `yield`.
* Remove the `if ctx.current_scope_flags().is_top()` check. Probably this check costs more than it saves because top level `await` is far less common than `await` within async functions. So this check was optimizing for an uncommon case, at a cost to the common case.
* Add more comments to explain the logic.

This is all quite small optimizations, but I was having difficulty understanding the logic, and found that splitting it up made it clearer (at least for me).
@oxc-bot oxc-bot mentioned this pull request Nov 9, 2024
Boshen added a commit that referenced this pull request Nov 9, 2024
## [0.36.0] - 2024-11-09

- b11ed2c ast: [**BREAKING**] Remove useless `ObjectProperty::init`
field (#7220) (Boshen)

- 0e4adc1 ast: [**BREAKING**] Remove invalid expressions from
`TSEnumMemberName` (#7219) (Boshen)

- 846711c transformer: [**BREAKING**] Change API to take a
`&TransformOptions` instead of `TransformOptions` (#7213) (Boshen)

- 092de67 types: [**BREAKING**] Append `rest` field into `elements` for
objects and arrays to align with estree (#7212) (ottomated)

- d1d1874 ast: [**BREAKING**] Change `comment.span` to real position
that contain `//` and `/*` (#7154) (Boshen)

- 843bce4 ast: [**BREAKING**] `IdentifierReference::reference_id` return
`ReferenceId` (#7126) (overlookmotel)

### Features

- cc8a191 ast: Methods on AST nodes to get `scope_id` etc (#7127)
(overlookmotel)
- dc0215c ast_tools: Add #[estree(append_to)], remove some custom
serialization code (#7149) (ottomated)
- 9d6cc9d estree: ESTree compatibility for all literals (#7152)
(ottomated)
- b74686c isolated-declarations: Support transform TSExportAssignment
declaration (#7204) (Dunqing)
- ad3a2f5 tasks/compat_data: Generate our own compat table (#7176)
(Boshen)
- b4258ee transformer: Add defaulted `Module::Preserve` option (#7225)
(Boshen)
- 324c3fe transformer: Add `TransformOptions::module` option (#7188)
(Boshen)
- a166a4a transformer: Add esbuild comma separated target API
`--target=es2020,chrome58` (#7210) (Boshen)
- 3a20b90 transformer: Add es target to `engineTargets` (#7193) (Boshen)
- 22898c8 transformer: Warn BigInt when targeting < ES2020 (#7184)
(Boshen)
- a579011 transformer: Add features `ES2018NamedCapturingGroupsRegex`
and `ES2018LookbehindRegex` (#7182) (Boshen)
- 8573f79 transformer: Turn on async_to_generator and
async_generator_functions plugins in enable_all (#7135) (Dunqing)
- df77241 transformer: Enable `ArrowFunctionConverter` in
`async-to-generator` and `async-generator-functions` plugins (#7113)
(Dunqing)
- b6a5750 transformer/arrow-function-converter: Move scope to changed
scope for `this_var` if scope have changed (#7125) (Dunqing)
- 1910227 transformer/async-to-generator: Support inferring the function
name from the ObjectPropertyValue's key (#7201) (Dunqing)
- ffa8604 transformer/async-to-generator: Do not transform await
expression if is not inside async function (#7138) (Dunqing)
- e536d47 transformer/babel: Add support for trying to get the `Module`
from `BabelPlugins` (#7218) (Dunqing)
- 5cfdc05 transformer/typescript: Support transform `export =` and
`import = require(...)` when module is commonjs (#7206) (Dunqing)

### Bug Fixes

- c82b273 transformer/async-generator-functions: Only transform object
method in exit_function (#7200) (Dunqing)
- b2a888d transformer/async-generator-functions: Incorrect
transformation for `for await` if it's not placed in a block (#7148)
(Dunqing)
- 19892ed transformer/async-generator-functions: Transform incorrectly
for `for await` if it's in LabeledStatement (#7147) (Dunqing)
- ede10dc transformer/async-to-generator: Incorrect transform when super
expression is inside async method (#7171) (Dunqing)
- 293d072 transformer/async-to-generator: Only transform object method
in exit_function (#7199) (Dunqing)
- ae692d7 transformer/async_to_generator: Fix checking if function is
class method (#7117) (overlookmotel)
- eea4ab8 transformer/helper-loader: Incorrect `SymbolFlags` for default
import when `SourceType` is script (#7226) (Dunqing)

### Refactor

- d27e14f ast: `AstKind::as_*` methods take `self` (#5546)
(overlookmotel)
- fac5042 ast: Use `scope_id` etc methods (#7130) (overlookmotel)
- a297765 minifier: Use `map` and `and_then` instead of let else (#7178)
(7086cmd)
- fc86703 napi/transform: Change test files to TypeScript (#7221)
(Boshen)
- c5485ae semantic: Add `ancestor_kinds` iterator function (#7217)
(camchenry)
- abf1602 semantic: Rename `iter_parents` to `ancestors` (#7216)
(camchenry)
- 42171eb semantic: Rename `ancestors` to `ancestor_ids` (#7215)
(camchenry)
- de56083 transformer: Add `impl TryFrom<EngineTargets> for EnvOptions`
(#7191) (Boshen)
- 0a43c64 transformer: Move `ESTarget` to its own file (#7189) (Boshen)
- 0e1f12c transformer: Remove unimplemented `EnvOptions::bugfixes`
(#7162) (Boshen)
- a981caf transformer: Add `Engine` enum for `EngineTargets` (#7161)
(Boshen)
- 8340243 transformer: Rename `Query` to `BrowserslistQuery` (#7143)
(Boshen)
- 481f7e6 transformer: Change `Targets` to `EngineTargets` (#7142)
(Boshen)
- 55e6989 transformer: Deserialize engine target strings to specific
keys (#7139) (Boshen)
- fdfd9a4 transformer: Use `scope_id` etc methods (#7128)
(overlookmotel)
- ff8bd50 transformer: Move implementation of ArrowFunction to
common/ArrowFunctionConverter (#7107) (Dunqing)
- 4a515be transformer/arrow-function-coverter: Rename function name and
add some comments to explain confusing parts. (#7203) (Dunqing)
- c307e1b transformer/arrow-functions: Pass `ArenaBox` as function param
(#7169) (overlookmotel)
- 217d433 transformer/arrow-functions: Remove unused `&mut self`
function param (#7165) (overlookmotel)
- 426df71 transformer/arrow-functions: Use `scope_id` method (#7164)
(overlookmotel)
- 11c5e12 transformer/arrow-functions: Correct comments (#7163)
(overlookmotel)
- 1238506 transformer/async-generator-function: Remove inactive
`#[allow(clippy::unused_self)]` attrs (#7167) (overlookmotel)
- 84ee581 transformer/async-generator-functions: Simplify identifying
whether within an async generator function (#7170) (overlookmotel)
- 1b12328 transformer/async-generator-functions: Use `clone` not
`clone_in` on `LabelIdentifier` (#7172) (overlookmotel)
- cd1006f transformer/async-generator-functions: Do not transform yield
expression where inside generator function (#7134) (Dunqing)
- 2c5734d transformer/async-generator-functions: Do not transform await
expression where inside ArrowFunctionExpression (#7132) (Dunqing)
- 5ce83bd transformer/async-generator-functions: Remove dead code for
handle await expression (#7131) (Dunqing)
- e04ee97 transformer/async-generator-functions: Move handling of
`MethodDefinition`'s value to `exit_function` (#7106) (Dunqing)
- b57d5a5 transformer/async-to-generator: Remove unused `&self` function
param (#7166) (overlookmotel)
- f80085c transformer/async-to-generator: Move handling of
`MethodDefinition`'s value to `exit_function` (#7105) (Dunqing)
- e2241e6 transformer/jsx-self: Remove unused `&self` function params
(#7159) (overlookmotel)
- 1dfd241 transformer/optional-catch-binding: Remove inactive
`#[allow(clippy::unused_self)]` attr (#7158) (overlookmotel)
- fd9b44c transformer/typescript: Remove inactive
`#[allow(clippy::unused_self)]` attr (#7160) (overlookmotel)
- cacfb9b traverse: Use `symbol_id` etc methods (#7129) (overlookmotel)

### Styling

- 38a6df6 transformer/arrow-functions: Semicolon after return statements
(#7168) (overlookmotel)
- 64b7e3a transformer/async-generator-functions: Import
`oxc_allocator::Vec` as `ArenaVec` (#7173) (overlookmotel)

### Testing

- be819dd napi/transform: Add test for not default es transform (Boshen)

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0-merge Merge with Graphite Merge Queue A-transformer Area - Transformer / Transpiler C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant