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/class-properties): only rename symbols if necessary #7896

Conversation

overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented Dec 14, 2024

#7872 implements renaming symbols in constructor which shadow references in instance property initializers. But we don't need to rename where the reference in initializer references a symbol which is bound within the initializer itself.

Input:

class C {
  double = n => n * 2;
  constructor(n) {
    console.log(n);
  }
}

Output:

class C {
  constructor(n) { // <-- not renamed
    this.double = n => n * 2; // <-- moved into constructor
    console.log(n); // <-- not renamed
  }
}

This produces better output, and avoids a traversal of constructor's AST renaming symbols.

Copy link
Contributor Author

overlookmotel commented Dec 14, 2024

Copy link

codspeed-hq bot commented Dec 14, 2024

CodSpeed Performance Report

Merging #7896 will not alter performance

Comparing 12-14-feat_transformer_class-properties_only_rename_symbols_if_necessary (feac02e) with main (3858221)

Summary

✅ 29 untouched benchmarks

@overlookmotel overlookmotel force-pushed the 12-13-fix_transformer_class-properties_fix_symbol_clashes_in_instance_prop_initializers branch from 6fbf297 to 92729ef Compare December 14, 2024 19:21
@overlookmotel overlookmotel force-pushed the 12-14-feat_transformer_class-properties_only_rename_symbols_if_necessary branch from 0024a9e to d73ac1e Compare December 14, 2024 19:21
@overlookmotel overlookmotel marked this pull request as draft December 14, 2024 19:31
@overlookmotel overlookmotel force-pushed the 12-14-feat_transformer_class-properties_only_rename_symbols_if_necessary branch from d73ac1e to 90ad409 Compare December 14, 2024 19:50
@overlookmotel overlookmotel marked this pull request as ready for review December 14, 2024 19:50
@overlookmotel overlookmotel force-pushed the 12-14-feat_transformer_class-properties_only_rename_symbols_if_necessary branch from 90ad409 to 506077d Compare December 14, 2024 19:56
@overlookmotel overlookmotel force-pushed the 12-13-fix_transformer_class-properties_fix_symbol_clashes_in_instance_prop_initializers branch from 92729ef to 80ad1cf Compare December 14, 2024 20:04
@overlookmotel overlookmotel force-pushed the 12-14-feat_transformer_class-properties_only_rename_symbols_if_necessary branch from 506077d to 29b8651 Compare December 14, 2024 20:04
@overlookmotel overlookmotel force-pushed the 12-13-fix_transformer_class-properties_fix_symbol_clashes_in_instance_prop_initializers branch from 80ad1cf to 21ebb97 Compare December 14, 2024 20:29
@overlookmotel overlookmotel force-pushed the 12-14-feat_transformer_class-properties_only_rename_symbols_if_necessary branch from 29b8651 to 2885b93 Compare December 14, 2024 20:29
@graphite-app graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Dec 15, 2024
Copy link

graphite-app bot commented Dec 15, 2024

Merge activity

…7896)

#7872 implements renaming symbols in constructor which shadow references in instance property initializers. But we don't need to rename where the reference in initializer references a symbol which is bound within the initializer itself.

Input:

```js
class C {
  double = n => n * 2;
  constructor(n) {
    console.log(n);
  }
}
```

Output:

```js
class C {
  constructor(n) { // <-- not renamed
    this.double = n => n * 2; // <-- moved into constructor
    console.log(n); // <-- not renamed
  }
}
```

This produces better output, and avoids a traversal of constructor's AST renaming symbols.
@Dunqing Dunqing force-pushed the 12-13-fix_transformer_class-properties_fix_symbol_clashes_in_instance_prop_initializers branch from 21ebb97 to e76fbb0 Compare December 15, 2024 01:53
@Dunqing Dunqing force-pushed the 12-14-feat_transformer_class-properties_only_rename_symbols_if_necessary branch from 2885b93 to feac02e Compare December 15, 2024 01:53
Base automatically changed from 12-13-fix_transformer_class-properties_fix_symbol_clashes_in_instance_prop_initializers to main December 15, 2024 01:57
@graphite-app graphite-app bot merged commit feac02e into main Dec 15, 2024
27 checks passed
@graphite-app graphite-app bot deleted the 12-14-feat_transformer_class-properties_only_rename_symbols_if_necessary branch December 15, 2024 02:02
Dunqing added a commit that referenced this pull request Dec 18, 2024
## [0.42.0] - 2024-12-18

- 84b75a0 semantic: [**BREAKING**] Remove `ScopeFlags::Modifiers`
(#7935) (overlookmotel)

- c071494 semantic: [**BREAKING**] Remove `SymbolTable::rename` method
(#7868) (overlookmotel)

### Features

- 8b7c5ae ast: Add `AstBuilder::atom_from_cow` (#7974) (overlookmotel)
- 46e2e27 data_structures: Implement `Default` for `NonEmptyStack`
(#7946) (overlookmotel)
- db9e93b mangler: Mangle top level variables (#7907) (翠 / green)
- 075bd16 minifier: Fold bitwise operation (#7908) (翠 / green)
- c16a851 napi/transform: Add `jsx: 'preserve'` option (#7965) (Boshen)
- 81eedb1 parser: 'readonly' type modifier is only permitted on array
and tuple literal types. (#7880) (Boshen)
- b9322c6 semantic: Re-export all flags and ID types (#7886)
(overlookmotel)
- c30a982 span: Add `impl From<ArenaString> for Atom` (#7973)
(overlookmotel)
- 02b653c transformer/class-properties: Do not create temp var for
template literal computed key (#7919) (overlookmotel)
- feac02e transformer/class-properties: Only rename symbols if necessary
(#7896) (overlookmotel)
- 6bc530d transformer/class-properties: Transform super call expression
that is inside static prop initializer (#7831) (Dunqing)
- 53e2bc0 traverse: Add `TraverseScoping::rename_symbol` method (#7871)
(overlookmotel)

### Bug Fixes

- 3659e6d cfg: Include export default code in CFG instructions (#7862)
(Jan Olaf Martin)
- 850dd43 codegen: Missing `,` when generating type parameters with jsx
(#7929) (Dunqing)
- 4799471 minfier: Bigint bitwise operation only works with bigint
(#7937) (Boshen)
- de8a86e minifier: Incorrect minification in `try_fold_left_child_op`
(#7949) (翠 / green)
- 9a30910 oxc_transformer: Inject_global_variables should considering
string imported name (#7768) (IWANABETHATGUY)
- 111dc52 parser: Include export token in spans of
TSNamespaceExportDeclaration (#7963) (branchseer)
- 14c51ff semantic: Remove inherting `ScopeFlags::Modifier` from parent
scope (#7932) (Dunqing)
- 596aead semantic: Reset references flags when resolved (#7923)
(Dunqing)
- 4924073 semantic: `ScopeTree::rename_binding` preserve order of
bindings (#7870) (overlookmotel)
- bb38065 transformer/class-properties: Do not transform `super.prop` in
nested method within static prop initializer (#7978) (overlookmotel)
- e76fbb0 transformer/class-properties: Fix symbol clashes in instance
prop initializers (#7872) (overlookmotel)
- c0576fa transformer/class-properties: Use UID for `args` in created
class constructor (#7866) (overlookmotel)
- d660d8d transformer/optional-chaining: Do not create unused reference
when `noDocumentAll` assumption (#7847) (overlookmotel)
- 4920c6a transformer/optional-chaining: Avoid creating a useless
reference when `noDocumentAll` is true (#7832) (Dunqing)

### Performance

- a5f04a7 ast: Faster `Comment::is_jsdoc` (#7905) (overlookmotel)
- 4b24335 codegen: Improve printing of statement comments (#7857)
(Boshen)
- 71a40a2 codegen: Guard comment printing comments when there are no
comments (#7856) (Boshen)
- b31f123 transformer/class-properties: Do not re-generate same method
key (#7915) (overlookmotel)
- 8ca8fce transformer/class-properties: Reduce work updating scopes when
transforming static prop initializers (#7904) (overlookmotel)
- 80d0b3e transformer/class-properties: Fast path for instance prop
initializer scope re-parenting (#7901) (overlookmotel)
- 38aafa2 transformer/class-properties: Reduce size of
`transform_call_expression_for_super_member_expr` (#7859)
(overlookmotel)

### Documentation

- e49de81 ast: Document `Expression::is_*` methods (#7853)
(overlookmotel)
- 10a86b9 transformer: Fix comments (#7925) (overlookmotel)
- f4cb5d3 transformer: Clarify comment (#7918) (overlookmotel)
- 41a1456 transformer/class-properties: Correct doc comments (#7966)
(overlookmotel)
- 18441af transformer/class-properties: Remove oudated todo for
assignment expression (#7955) (Dunqing)
- 1317c00 transformer/class-properties: Clarify doc comments (#7914)
(overlookmotel)
- 9989b58 transformer/class-properties: Re-order file list in doc
comment (#7911) (overlookmotel)
- 7390048 transformer/class-properties: Reformat doc comment (#7909)
(overlookmotel)

### Refactor

- beb982a ast: Use exhaustive match for `Argument` to
`ArrayExpressionElement` conversion (#7848) (overlookmotel)
- 3858221 global: Sort imports (#7883) (overlookmotel)
- 1314c97 minifier: Expose dce as an API instead of an option (#7957)
(Boshen)
- 6551dfe semantic: Pass `&str` instead of `Cow` (#7972) (overlookmotel)
- b8d2bd2 semantic: Move determining references flags for export
specifier to `visit_export_named_declaration` (#7924) (Dunqing)
- 98d7946 semantic: Import flags and ID types from `oxc_syntax` (#7887)
(overlookmotel)
- 1cf8f8f semantic: `SymbolTable::set_name` return old name (#7869)
(overlookmotel)
- 5d42df8 semantic: Use `Expression::is_super` (#7851) (overlookmotel)
- 8cf9766 semantic, syntax, wasm: Remove `#![allow(non_snake_case)]`
(#7863) (overlookmotel)
- d59bbae transformer: Remove unneeded lint `#[allow]` (#7971)
(overlookmotel)
- 2c94236 transformer: Improve encapsulation of transforms (#7888)
(overlookmotel)
- 34091b2 transformer: Use `Expression::is_super` (#7852)
(overlookmotel)
- d4d7bc0 transformer/async-to-generator: Avoid allocating unnecessary
`Atom`s (#7975) (overlookmotel)
- 2e5ffd3 transformer/class-properties: Store `temp_var_is_created` on
`ClassBindings` (#7981) (overlookmotel)
- 27cc6da transformer/class-properties: Store `is_declaration` only on
`ClassDetails` (#7980) (overlookmotel)
- ee282f8 transformer/class-properties: Remove `move_expression`s
(#7979) (overlookmotel)
- 94b376a transformer/class-properties: Simplify logic for when to
create temp binding (#7977) (overlookmotel)
- ff9d1b3 transformer/class-properties: Comments about shorter output
(#7976) (overlookmotel)
- 6fc40f0 transformer/class-properties: Pass `BoundIdentifier`s by
reference (#7968) (overlookmotel)
- 69eeeea transformer/class-properties: Methods take `&self` where
possible (#7967) (overlookmotel)
- 98340bb transformer/class-properties: Use stack of `ClassDetails`
(#7947) (overlookmotel)
- 088dd48 transformer/class-properties: Shorten code (#7913)
(overlookmotel)
- 544ffbf transformer/class-properties: Split up code into multiple
files (#7912) (overlookmotel)
- dcaf674 transformer/class-properties: Rename file (#7910)
(overlookmotel)
- 6243980 transformer/class-properties: Instance prop inits visitor use
`Visit` (#7867) (overlookmotel)
- eb47d43 transformer/class-properties: Re-use existing `Vec` (#7854)
(overlookmotel)
- 1380b7b transformer/class-properties: Reduce visibility of method
(#7858) (overlookmotel)
- 0f5e078 transformer/class-properties: Rename `*_owner` to `owned_*`
(#7855) (Dunqing)
- 4ea90d4 transformer/react-refresh: Calculate signature key once
(#7970) (Dunqing)
- 15b9bff transformer/typescript: Reuse `Atom` (#7969) (overlookmotel)

### Styling

- fb897f6 data_structures: Add line break (#7882) (overlookmotel)
- 7fb9d47 rust: `cargo +nightly fmt` (#7877) (Boshen)

### Testing

- 523d48c transformer: Move named test to exports folder (#7922)
(Dunqing)
- e766051 transformer: Skip test which uses filesystem under miri
(#7874) (overlookmotel)
- f39e65e transformer: Prevent lint error when running miri (#7873)
(overlookmotel)

Co-authored-by: Dunqing <[email protected]>
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-enhancement Category - New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant