fix(transformer): fix stacks in arrow function transform by overlookmotel · Pull Request #5828 · oxc-project/oxc

overlookmotel · 2024-09-17T15:28:00Z

Push/pop to stacks in matching enter_* / exit_* visitors.

The reason why there was a bug here is a little bit subtle.

Between enter_expression and exit_expression, another transform can replace the Expression with something else. Ditto enter_declaration + exit_declaration.

So pushing+popping from stacks in enter_expression + exit_expression can make the stack get out of sync. e.g.:

impl<'a> Traverse for TransformerWithStack {
    fn enter_expression(&mut self, expr: &mut Expression<'a>, ctx: TraverseCtx<'a>) {
        if let Expression::FunctionExpression(_) = expr {
            self.stack.push(true);
        }
    }

    fn exit_expression(&mut self, expr: &mut Expression<'a>, ctx: TraverseCtx<'a>) {
        if let Expression::FunctionExpression(_) = expr {
            self.stack.pop();
        }
    }
}

// Transformer that replaces `null` with a function expression (!)
impl<'a> Traverse for SomeOtherTransformer {
    fn enter_expression(&mut self, expr: &mut Expression<'a>, ctx: TraverseCtx<'a>) {
        if let Expression::NullLiteral(_) = expr {
            *expr = ctx.ast.expression_function( /* ... */ );
        }
    }
}

TransformerWithStack is assuming in exit_expression that it previously saw a FunctionExpression in enter_expression. But SomeOtherTransformer has created a new FunctionExpression after TransformerWithStack::enter_expression ran. So in TransformerWithStack, exit_expression is called 1 more time than enter_expression. When exit_expression calls self.stack.pop(), self.stack is empty, and it panics.

This example is silly, but real cases of this do exist. e.g. TS transformer creates a new functions when transforming enums.

enter_function + exit_function / enter_arrow_function_expression + exit_arrow_function_expression not have this problem. As we cannot mutate upwards, there are always the same number of calls to enter_* and exit_* (same for all enter_* / exit_* pairs which operate on a struct, not an enum).

overlookmotel · 2024-09-17T15:28:19Z

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-09-17T15:34:56Z

CodSpeed Performance Report

Merging #5828 will not alter performance

_{Comparing 09-17-fix_transformer_fix_stacks_in_arrow_function_transform (172fa03) with main (c6d97e9)}

Summary

✅ 29 untouched benchmarks

graphite-app · 2024-09-17T15:43:50Z

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.

graphite-app · 2024-09-18T02:22:50Z

Merge activity

Sep 17, 10:22 PM EDT: Dunqing added this pull request to the Graphite merge queue.
Sep 17, 10:32 PM EDT: Dunqing merged this pull request with the Graphite merge queue.

Push/pop to stacks in matching `enter_*` / `exit_*` visitors. The reason why there was a bug here is a little bit subtle. Between `enter_expression` and `exit_expression`, another transform can replace the `Expression` with something else. Ditto `enter_declaration` + `exit_declaration`. So pushing+popping from stacks in `enter_expression` + `exit_expression` can make the stack get out of sync. e.g.: ```rs impl<'a> Traverse for TransformerWithStack { fn enter_expression(&mut self, expr: &mut Expression<'a>, ctx: TraverseCtx<'a>) { if let Expression::FunctionExpression(_) = expr { self.stack.push(true); } } fn exit_expression(&mut self, expr: &mut Expression<'a>, ctx: TraverseCtx<'a>) { if let Expression::FunctionExpression(_) = expr { self.stack.pop(); } } } // Transformer that replaces `null` with a function expression (!) impl<'a> Traverse for SomeOtherTransformer { fn enter_expression(&mut self, expr: &mut Expression<'a>, ctx: TraverseCtx<'a>) { if let Expression::NullLiteral(_) = expr { *expr = ctx.ast.expression_function( /* ... */ ); } } } ``` `TransformerWithStack` is assuming in `exit_expression` that it previously saw a `FunctionExpression` in `enter_expression`. But `SomeOtherTransformer` has created a *new* `FunctionExpression` after `TransformerWithStack::enter_expression` ran. So in `TransformerWithStack`, `exit_expression` is called 1 more time than `enter_expression`. When `exit_expression` calls `self.stack.pop()`, `self.stack` is empty, and it panics. This example is silly, but real cases of this do exist. e.g. TS transformer creates a new functions when transforming `enum`s. `enter_function` + `exit_function` / `enter_arrow_function_expression` + `exit_arrow_function_expression` not have this problem. As we cannot mutate upwards, there are always the same number of calls to `enter_*` and `exit_*` (same for all `enter_*` / `exit_*` pairs which operate on a struct, *not an enum*).

… of sync (#5941) Fix tiny bug in arrow function transform. Similar to #5828 - cannot assume that other transforms haven't altered AST between `enter_*` and `exit_*`. In this case, pushing/popping to the stack cannot depend on `func.body.is_some()`, as that could change in between `enter_function` and `exit_function`.

## [0.30.0] - 2024-09-23 - c96b712 syntax: [**BREAKING**] Remove `SymbolFlags::ArrowFunction` (#5857) (overlookmotel) ### Features - ae89145 ast: Revert `#[non_exhaustive]` change (#5885) (Boshen) - e8bf30a ast: Add `Comment::real_span` (#5764) (Boshen) - d901772 codegen: Implement minify number from terser (#5929) (Boshen) - 9f6696a codegen: Add new lines to `TSTypeParameterDeclaration` (#5853) (Boshen) - bcdbba3 codegen: Print jsdoc comments that are attached to statements and class elements (#5845) (Boshen) - 26386da codegen: Have `with_source_text` reserve memory for code buffer (#5823) (DonIsaac) - 4a62703 isolated-declarations: Handle `export` in the `namespace` correctly (#5950) (Dunqing) - 84a5816 isolated_declarations: Add `stripInternal` (#5878) (Boshen) - dfbde2c isolated_declarations: Print jsdoc comments (#5858) (Boshen) - 3bf7b24 linter: Make `typescript/no-duplicate-enum-values` a `correctness` rule (#5810) (DonIsaac) - 9076dee minifier: Implement part of `StatementFusion` (#5936) (Boshen) - a111bb6 oxc_wasm: Add `verbse` option to `debug_dot` (#5879) (IWANABETHATGUY) - 8e7556f parser: Calculate leading and trailing position for comments (#5785) (Boshen) - 65c337a prettier: Improve ts compatibility (#5900) (Alexander S.) - 6d9ccdd prettier: Support TSMappedType (#5834) (Alexander S.) - b5ac5a6 prettier: Support TSModuleDeclaration (#5813) (Alexander S.) - 74d8714 semantic: Add help message for invalid `let x?: number` (#5969) (DonIsaac) - 3230ae5 semantic: Add `SemanticBuilder::with_excess_capacity` (#5762) (overlookmotel) - a5f2e9a span: Impl `From<Atom<'a>>` for `Atom` (#5809) (DonIsaac) - a07f03a transformer: Sync `Program::source_type` after transform (#5887) (Boshen) - 635e918 traverse: `generate_uid_name` method (#5839) (overlookmotel) ### Bug Fixes - 66e919e ast: Correct TS types for JSX (#5884) (overlookmotel) - 0d10521 ast: Serialize `JSXMemberExpressionObject` to estree (#5883) (overlookmotel) - a822c9d ast: Serialize `JSXElementName` to estree (#5882) (Boshen) - f4aefb5 codegen: Print `let[0]` as `(let)[0]` (#5947) (Boshen) - cee9d0b codegen: Fix spacing of `for await (x of y)` (#5890) (Boshen) - 5901d2a codegen: Various spacing issues (#5820) (Boshen) - fd1c46c isolated-declarations: Infer failed if there are two setter/getter methods that need to be inferred (#5967) (Dunqing) - 6df82ee isolated-declarations: False positive for class private method that has arguments without type annotations (#5964) (Dunqing) - 6a9e71d isolated-declarations: Wrap TSFunctionType in parentheses if it is inside the `TSUnionType` (#5963) (Dunqing) - ea32d5b isolated-declarations: Should print constructor assignments first (#5934) (Dunqing) - 0f96b59 isolated-declarations: Missing print comments in class's private method (#5931) (Dunqing) - 8780c54 isolated-declarations: Do not union a undefined when the param type is any or unknown (#5930) (Dunqing) - f07ff14 isolated-declarations: Should not transform signature that has type annotation (#5927) (Dunqing) - b6a9178 isolated-declarations: Don't collect references when `ExportNamedDeclaration` has source (#5926) (Dunqing) - 756a571 isolated-declarations: Missing empty export when has an export declare (#5925) (Dunqing) - e148c80 isolated_declarations: Try fix fixtures (Boshen) - 9b3f763 isolated_declarations: Try fix new line issue (Boshen) - ee748b0 isolated_declarations: Fix fixture spacing (Boshen) - 362c427 mangler,codegen: Do not mangle top level symbols (#5965) (Boshen) - 127c881 napi/transform: Fix jsdoc links (#5886) (Boshen) - 6c04fa1 napi/transform: Make isolated_declaration options optional (#5880) (Boshen) - 42dcadf parser: Hashbang comment should not keep the end newline char (#5844) (Boshen) - f1551d6 semantic: `?` on variable declaration type annotations is a syntax error (#5956) (DonIsaac) - a23879c semantic: Analyze `ReferenceFlags` incorrectly when there are nested `AssignmentTarget` (#5847) (Dunqing) - 87323c6 transformer: Arrow function transform: prevent stack getting out of sync (#5941) (overlookmotel) - 4e9e838 transformer: Fix arrow function transform (#5933) (overlookmotel) - 4d5c4f6 transformer: Fix reference flags in logical assignment operator transform (#5903) (overlookmotel) - d335a67 transformer: Fix references in logical assignment operator transform (#5896) (overlookmotel) - 9758c1a transformer: JSX source: add `var _jsxFileName` statement (#5894) (overlookmotel) - 49ee1dc transformer: Arrow function transform handle `this` in arrow function in class static block (#5848) (overlookmotel) - 172fa03 transformer: Fix stacks in arrow function transform (#5828) (overlookmotel) - d74c7fa transformer: Remove `AstBuilder::copy` from arrow functions transform (#5825) (overlookmotel) - 3cc38df transformer/react: React refresh panics when encounter `use` hook (#5768) (Dunqing) ### Performance - cd34f07 isolated-declarations: Combine type/value bindings and type/value references into one (#5968) (Dunqing) - c477424 mangler: Use `sort_unstable_by_key` instead of `sort_by` (#5948) (Boshen) - c3e0fb6 semantic: Simplify resetting ReferenceFlags in `AssignmentExpression` (#5846) (Dunqing) - ff7d9c1 transformer: Arrow function transform: calculate whether `this` is in arrow function lazily (#5850) (Dunqing) - fd70c4b transformer: Arrow function transform more efficient scope search (#5842) (overlookmotel) - 56703a3 transformer: Make branch more predictable in arrow function transform (#5833) (overlookmotel) - 36e698b transformer: Call `transform_jsx` in `exit_expression` rather than `enter_expression` (#5751) (Dunqing) - aac8316 transformer/react: Improve `is_componentish_name`'s implementation (#5769) (Dunqing) ### Documentation - acc2d16 ast: Document most TypeScript AST nodes (#5983) (DonIsaac) - 47c2faa ast: Document TryStatement and related nodes (#5970) (DonIsaac) - 83ca7f5 diagnostics: Fully document `oxc_diagnostics` (#5865) (DonIsaac) - bacfbb8 oxc: Add submodule documentation (#5984) (DonIsaac) - 3120c6c parser: Add module and struct level documentation (#5831) (DonIsaac) - 1ccf290 semantic: Document `AstNode` and `AstNodes` (#5872) (DonIsaac) - e04841c syntax: Add ModuleRecord documentation (#5818) (DonIsaac) - 7085829 transformer: Arrow function transform: comment about incomplete implementation (#5945) (overlookmotel) - 66b4688 transformer: React: convert docs to standard format (#5891) (overlookmotel) - 7f05eed transformer: Add comment about missing features in arrow function transform (#5855) (overlookmotel) - 8770647 transformer: Correct docs for arrow function transform (#5854) (overlookmotel) ### Refactor - f4fac0f ast: Remove `.iter()` where not needed (#5904) (camchenry) - 17cd903 ast: Move functions to top level in `ast` macro (#5793) (overlookmotel) - cf97f6d ast: Import `syn` types in `ast` macro (#5792) (overlookmotel) - dc10eaf ast: Split `ast` macro into multiple files (#5791) (overlookmotel) - 6dd6f7c ast: Change `Comment` struct (#5783) (Boshen) - bb95306 codegen: Change annotation comment tests to snapshot (#5800) (Boshen) - e613a3d codegen: Prepare to add leading comments by adding a template method pattern (#5784) (Boshen) - 7caae5b codegen: Add `GetSpan` requirement to `Gen` trait (#5772) (Boshen) - c84bd28 isolated-declarations: Simplify to infer the getter and setter methods (#5966) (Dunqing) - 67b4220 isolated-declarations: Simplify handling VariableDeclaration transform (#5916) (Dunqing) - 2fd5c2a isolated-declarations: Pre-filter statements that do not need to be transformed (#5909) (Dunqing) - 943bd76 minifier: Move tests to their src files (#5912) (Boshen) - cbaeea6 minifier: Clean up some tests (#5910) (Boshen) - 144611e minifier: Align ast pass names with closure compiler (#5908) (Boshen) - 31e9db4 parser: Shorten `UniquePromise` code (#5805) (overlookmotel) - 2322b8b parser: Remove dead code warning when running tests (#5804) (overlookmotel) - 4abfa76 parser: Add `--ast` and `--comments` to example (Boshen) - a4b55bf parser: Use AstBuilder (#5743) (Boshen) - d910304 semantic: Rename lifetime on `impl IntoIterator for &AstNodes` (#5881) (overlookmotel) - f360e2c semantic: Remove redundunt is_leading check for JSDoc (#5877) (leaysgur) - 9115dd9 semantic: Use `Comment::attached_to` for jsdoc attachment (#5876) (Boshen) - db4f16a semantic: Call `with_trivias` before `build_with_jsdoc` (#5875) (Boshen) - 3d13c6d semantic: Impl `IntoIterator` for `&AstNodes` (#5873) (DonIsaac) - 47d9ad8 semantic: Remove unused vars warning in release mode (#5803) (overlookmotel) - 155d7fc transformer: Arrow function transform: ignore type fields when finding enclosing arrow function (#5944) (overlookmotel) - 2cf5607 transformer: Split up logical assignment operator transform into functions (#5902) (overlookmotel) - 41fbe15 transformer: Internal functions not `pub` in logical assignment operator transform (#5898) (overlookmotel) - b11d91c transformer: Remove nested match in logical assignment operator transform (#5897) (overlookmotel) - 52c9903 transformer: JSX: use `AstBuilder::vec_from_iter` (#5862) (overlookmotel) - 74364ad transformer: JSX: merge `transform_jsx_attribute_item` into `transform_jsx` (#5861) (overlookmotel) - d2eaa7d transformer: Reorder match arms in JSX transform (#5860) (overlookmotel) - 58a8327 transformer: Simplify match in JSX transform (#5859) (overlookmotel) - b9c4564 transformer: Transformer example output semantic + transformer errors (#5852) (overlookmotel) - 03e02a0 transformer: Comment about potential improvement to arrow function transform (#5841) (overlookmotel) - 40cdad5 transformer: Remove repeat code in arrow function transform (#5837) (overlookmotel) - 3dd188c transformer: Deref `SymbolId` immediately (#5836) (overlookmotel) - 03a9e1a transformer: Reorder methods in arrow function transform (#5830) (overlookmotel) - 4d97184 transformer: Rename vars in arrow function transform (#5827) (overlookmotel) - 01c5b7c transformer: Shorten code in arrow functions transform (#5826) (overlookmotel) - 85ac3f7 transformer: Arrow functions transform do not wrap function expressions in parentheses (#5824) (overlookmotel) - 1c1353b transformer: Use AstBuilder instead of using struct constructor (#5778) (Boshen) ### Testing - 84b7d1a index: Add unit tests to `oxc_index` (#5979) (DonIsaac) - d6cbbe7 isolated-declarations: Arrow function unions in return signature (#5973) (DonIsaac) --------- Co-authored-by: Boshen <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

github-actions bot added the A-transformer Area - Transformer / Transpiler label Sep 17, 2024

This was referenced Sep 17, 2024

fix(transformer): remove AstBuilder::copy from arrow functions transform #5825

Merged

refactor(transformer): shorten code in arrow functions transform #5826

Merged

overlookmotel mentioned this pull request Sep 17, 2024

refactor(transformer): rename vars in arrow function transform #5827

Merged

overlookmotel marked this pull request as ready for review September 17, 2024 15:51

overlookmotel force-pushed the 09-17-fix_transformer_fix_stacks_in_arrow_function_transform branch from 9cfa043 to 07af4cb Compare September 17, 2024 16:59

This was referenced Sep 17, 2024

refactor(transformer): reorder methods in arrow function transform #5830

Merged

perf(transformer): make branch more predictable in arrow function transform #5833

Merged

overlookmotel force-pushed the 09-17-fix_transformer_fix_stacks_in_arrow_function_transform branch from 07af4cb to 71c2f2c Compare September 17, 2024 17:21

Dunqing force-pushed the 09-17-refactor_transformer_rename_vars_in_arrow_function_transform branch from b730ea1 to 4d97184 Compare September 18, 2024 02:24

Dunqing force-pushed the 09-17-fix_transformer_fix_stacks_in_arrow_function_transform branch from 71c2f2c to 172fa03 Compare September 18, 2024 02:25

Base automatically changed from 09-17-refactor_transformer_rename_vars_in_arrow_function_transform to main September 18, 2024 02:32

graphite-app bot merged commit 172fa03 into main Sep 18, 2024

graphite-app bot deleted the 09-17-fix_transformer_fix_stacks_in_arrow_function_transform branch September 18, 2024 02:32

overlookmotel mentioned this pull request Sep 20, 2024

fix(transformer): arrow function transform: prevent stack getting out of sync #5941

Merged

oxc-bot mentioned this pull request Sep 23, 2024

release(crates): v0.30.0 #5986

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

fix(transformer): fix stacks in arrow function transform#5828

fix(transformer): fix stacks in arrow function transform#5828
graphite-app[bot] merged 1 commit intomainfrom
09-17-fix_transformer_fix_stacks_in_arrow_function_transform

overlookmotel commented Sep 17, 2024 •

edited

Loading

Uh oh!

overlookmotel commented Sep 17, 2024 •

edited

Loading

Uh oh!

codspeed-hq bot commented Sep 17, 2024 •

edited

Loading

Uh oh!

graphite-app bot commented Sep 17, 2024

Uh oh!

graphite-app bot commented Sep 18, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Comments

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

CodSpeed Performance Report

Merging #5828 will not alter performance

Summary

Uh oh!

graphite-app bot commented Sep 17, 2024

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

Uh oh!

graphite-app bot commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

overlookmotel commented Sep 17, 2024 •

edited

Loading

overlookmotel commented Sep 17, 2024 •

edited

Loading

codspeed-hq bot commented Sep 17, 2024 •

edited

Loading

graphite-app bot commented Sep 18, 2024 •

edited

Loading