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

fix(transformer/typescript): typescript syntax within SimpleAssignmentTarget with MemberExpressions is not stripped #4920

Conversation

Dunqing
Copy link
Member

@Dunqing Dunqing commented Aug 15, 2024

close: #4870

I added the move_identifier_reference and move_member_expression methods used to take ownership in ast_builder_impl. This way can let us get rid of ast.copy.

Another possible approach is to add get_expression_owner to SimpleAssignmentTarget and a get_inner_expression_owner method to Expression. And add an into_xxxxx method for inherit_variants macro

The implementation looks like this

let Some(expression) = self.get_expression_owner() else { return; }
match expr.get_inner_expression_owner() {
    Expression::Identifier(ident) => {
        *target = self.ctx.ast.simple_assignment_target_from_identifier_reference(ident);
    }
    inner_expr @ match_member_expression!(Expression) => {
        *target = SimpleAssignmentTarget::from(
            inner_expr.into_member_expression()
        );
    }
    _ => (),
}

@github-actions github-actions bot added A-ast Area - AST A-transformer Area - Transformer / Transpiler labels Aug 15, 2024
Copy link
Member Author

Dunqing commented Aug 15, 2024

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

Join @Dunqing and the rest of your teammates on Graphite Graphite

Copy link

codspeed-hq bot commented Aug 15, 2024

CodSpeed Performance Report

Merging #4920 will not alter performance

Comparing 08-16-feat_transformer_typescript_typescript_syntax_within_simpleassignmenttarget_with_memberexpressions_is_not_stripped (248a757) with main (508644a)

Summary

✅ 29 untouched benchmarks

@Dunqing Dunqing requested a review from overlookmotel August 15, 2024 17:49
@Dunqing Dunqing changed the title feat(transformer/typescript): typescript syntax within SimpleAssignmentTarget with MemberExpressions is not stripped fix(transformer/typescript): typescript syntax within SimpleAssignmentTarget with MemberExpressions is not stripped Aug 15, 2024
@Dunqing Dunqing force-pushed the 08-16-feat_transformer_typescript_typescript_syntax_within_simpleassignmenttarget_with_memberexpressions_is_not_stripped branch from 56296d4 to 1493b4f Compare August 15, 2024 17:53
@Dunqing Dunqing requested a review from Boshen August 15, 2024 17:58
@Dunqing Dunqing force-pushed the 08-16-feat_transformer_typescript_typescript_syntax_within_simpleassignmenttarget_with_memberexpressions_is_not_stripped branch from 1493b4f to 2549549 Compare August 16, 2024 04:21
@Dunqing Dunqing requested a review from Boshen August 16, 2024 04:21
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Aug 16, 2024
Copy link

graphite-app bot commented Aug 16, 2024

Merge activity

  • Aug 16, 1:04 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Aug 16, 1:04 AM EDT: Boshen added this pull request to the Graphite merge queue.
  • Aug 16, 1:08 AM EDT: Boshen merged this pull request with the Graphite merge queue.

…ntTarget` with `MemberExpressions` is not stripped (#4920)

close: #4870

I added the `move_identifier_reference` and `move_member_expression` methods used to take ownership in `ast_builder_impl`.  This way can let us get rid of `ast.copy`.

Another possible approach is to add `get_expression_owner` to `SimpleAssignmentTarget` and a `get_inner_expression_owner` method to `Expression`. And add an `into_xxxxx` method for `inherit_variants` macro

The implementation looks like this
```rs
let Some(expression) = self.get_expression_owner() else { return; }
match expr.get_inner_expression_owner() {
    Expression::Identifier(ident) => {
        *target = self.ctx.ast.simple_assignment_target_from_identifier_reference(ident);
    }
    inner_expr @ match_member_expression!(Expression) => {
        *target = SimpleAssignmentTarget::from(
            inner_expr.into_member_expression()
        );
    }
    _ => (),
}
```
@Boshen Boshen force-pushed the 08-16-feat_transformer_typescript_typescript_syntax_within_simpleassignmenttarget_with_memberexpressions_is_not_stripped branch from 2549549 to 248a757 Compare August 16, 2024 05:05
@graphite-app graphite-app bot merged commit 248a757 into main Aug 16, 2024
24 checks passed
@graphite-app graphite-app bot deleted the 08-16-feat_transformer_typescript_typescript_syntax_within_simpleassignmenttarget_with_memberexpressions_is_not_stripped branch August 16, 2024 05:08
@oxc-bot oxc-bot mentioned this pull request Aug 18, 2024
Boshen added a commit that referenced this pull request Aug 18, 2024
## [0.24.3] - 2024-08-18

### Features

- d49fb16 oxc_codegen: Support generate range leading comments (#4898)
(IWANABETHATGUY)
- 80d0d1f semantic: Check for invalid interface heritage clauses (#4928)
(DonIsaac)
- 48821c0 semantic,syntax: Add SymbolFlags::ArrowFunction (#4946)
(DonIsaac)
- f1fcdde transformer: Support react fast refresh (#4587) (Dunqing)
- 0d79122 transformer: Support logical-assignment-operators plugin
(#4890) (Dunqing)
- ab1d08c transformer: Support `optional-catch-binding` plugin (#4885)
(Dunqing)
- 69da9fd transformer: Support nullish-coalescing-operator plugin
(#4884) (Dunqing)
- 3a66e58 transformer: Support exponentiation operator plugin (#4876)
(Dunqing)
- f88cbcd transformer: Add `BoundIdentifier::new_uid_in_current_scope`
method (#4903) (overlookmotel)
- 1e6d0fe transformer: Add methods to `BoundIdentifier` (#4897)
(overlookmotel)
- fd34640 traverse: Support `generate_uid_based_on_node` method in
`TraverseCtx` (#4940) (Dunqing)
- 72a37fc traverse: Support `clone_identifier_reference` method in
`TraverseCtx` (#4880) (Dunqing)

### Bug Fixes

- c0b26f4 ast: Do not include `scope_id` fields in JSON AST (#4858)
(overlookmotel)
- bbf9ec0 codegen: Add missing `declare` to `PropertyDefinition` (#4937)
(Boshen)
- f210cf7 codegen: Print `TSSatisfiesExpression` and
`TSInstantiationExpression` (#4936) (Boshen)
- 21f5762 codegen: Minify large numbers (#4889) (Boshen)
- e8de4bd codegen: Fix whitespace issue when minifying `x in new
Error()` (#4886) (Boshen)
- a226962 codegen: Print `TSNonNullExpression` (#4869) (Boshen)
- 3da33d3 codegen: Missing parenthesis for `PrivateInExpression` (#4865)
(Boshen)
- 1808529 codegen: Dedupe pure annotation comments (#4862)
(IWANABETHATGUY)
- d3bbc62 isolated-declarations: Declare modifier of PropertyDefinition
should not be retained (#4941) (Dunqing)
- 8e80f59 isolated_declarations: Class properties should still be lifted
from private constructors (#4934) (michaelm)
- b3ec9e5 isolated_declarations: Always emit module declarations that
perform augmentation (#4919) (michaelm)
- 0fb0b71 isolated_declarations: Always emit module declarations (#4911)
(michaelm)
- 4a16916 isolated_declarations: Support expando functions (#4910)
(michaelm)
- 508644a linter/tree-shaking: Correct the calculation of `>>`, `<<` and
`>>>` (#4932) (mysteryven)
- 46cb1c1 minifier: Handle `Object.definedPropert(exports` for
@babel/types/lib/index.js (#4933) (Boshen)
- 81fd637 minifier: Do not fold `0 && (module.exports = {})` for
`cjs-module-lexer` (#4878) (Boshen)
- 879a271 minifier: Do not join `require` calls for `cjs-module-lexer`
(#4875) (Boshen)
- 1bdde2c parser: Detect @flow in `/** @flow */ comment (#4861) (Boshen)
- 2476dce transformer: Remove an `ast.copy` from
`NullishCoalescingOperator` transform (#4913) (overlookmotel)
- 248a757 transformer/typescript: Typescript syntax within
`SimpleAssignmentTarget` with `MemberExpressions` is not stripped
(#4920) (Dunqing)

### Documentation

- 47c9552 ast, ast_macros, ast_tools: Better documentation for `Ast`
helper attributes. (#4856) (rzvxa)
- 0a01a47 semantic: Improve documentation (#4850) (DonIsaac)
- 9c700ed transformer: Add README including style guide (#4899)
(overlookmotel)

### Refactor

- a6967b3 allocator: Correct code comment (#4904) (overlookmotel)
- 90d0b2b allocator, ast, span, ast_tools: Use `allocator` as var name
for `Allocator` (#4900) (overlookmotel)
- 1eb59d2 ast, isolated_declarations, transformer: Mark
`AstBuilder::copy` as an unsafe function (#4907) (overlookmotel)
- 8e8fcd0 ast_tools: Rename `oxc_ast_codegen` to `oxc_ast_tools`.
(#4846) (rzvxa)
- 786bf07 index: Shorten code and correct comment (#4905)
(overlookmotel)
- ea1e64a semantic: Make SemanticBuilder opaque (#4851) (DonIsaac)
- 5fd1701 sourcemap: Lower the `msrv`. (#4873) (rzvxa)
- 48a1c32 syntax: Inline trivial bitflags methods (#4877)
(overlookmotel)
- 452187a transformer: Rename `BoundIdentifier::new_uid_in_root_scope`
(#4902) (overlookmotel)
- 707a01f transformer: Re-order `BoundIdentifier` methods (#4896)
(overlookmotel)
- 117dff2 transformer: Improve comments for `BoundIdentifier` helper
(#4895) (overlookmotel)

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Boshen pushed a commit that referenced this pull request Aug 19, 2024
…e empty string (#4977)

#4920 introduced `AstBuilder::move_identifier_reference`. Make this slightly cheaper by using a static empty `Atom` rather than relying on `"".into_in(allocator)` which allocates an empty string into arena.
Dunqing pushed a commit that referenced this pull request Aug 19, 2024
…#4982)

Follow on after #4920. #4920 removed a usage of `AstBuilder::copy` and replaced with 2 new methods `AstBuilder::move_identifier_reference` and `AstBuilder:: move_member_expression`.

We can instead use `AstBuilder::move_expression` earlier and then unpack the enum again. Hopefully the compiler can see that the 2 `unreachable!()` branches are indeed unreachable and elide them.

`move_expression` is preferable to `move_member_expression` as it only creates 1 temporary `Box<Expression::NullLiteral>` instead of 2.
Dunqing pushed a commit that referenced this pull request Aug 19, 2024
Remove one usage of unsound `AstBuilder::copy` method, using the new `get_inner_expression_mut` method introduced in #4920.
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-ast Area - AST A-transformer Area - Transformer / Transpiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

transformer: typescript syntax inside SimpleAssignmentTarget is not stripped
2 participants