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/class-properties): fix symbol clashes in instance prop initializers #7872

Conversation

overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented Dec 14, 2024

Instance property initializers are moved into constructor. If symbols they reference are shadowed within constructor, rename those symbols.

Input:

class C {
  prop = foo();
  constructor(foo) {
    console.log(foo);
  }
}

Output:

class C {
  constructor(_foo) { // <-- renamed
    this.prop = foo(); // <-- moved into constructor
    console.log(_foo); // <-- renamed
  }
}

Copy link
Contributor Author

overlookmotel commented Dec 14, 2024

Copy link

codspeed-hq bot commented Dec 14, 2024

CodSpeed Performance Report

Merging #7872 will not alter performance

Comparing 12-13-fix_transformer_class-properties_fix_symbol_clashes_in_instance_prop_initializers (e76fbb0) with main (3858221)

Summary

✅ 29 untouched benchmarks

@overlookmotel overlookmotel force-pushed the 12-14-feat_traverse_add_traversescoping_rename_symbol_method branch from 9e31c6c to 1a39db7 Compare December 14, 2024 03:25
@overlookmotel overlookmotel force-pushed the 12-13-fix_transformer_class-properties_fix_symbol_clashes_in_instance_prop_initializers branch from 3b8d0da to 8ecaa7c Compare December 14, 2024 03:25
@overlookmotel overlookmotel force-pushed the 12-14-feat_traverse_add_traversescoping_rename_symbol_method branch from 1a39db7 to c6d3847 Compare December 14, 2024 03:53
@overlookmotel overlookmotel force-pushed the 12-13-fix_transformer_class-properties_fix_symbol_clashes_in_instance_prop_initializers branch from 8ecaa7c to 3830050 Compare December 14, 2024 03:53
@overlookmotel overlookmotel force-pushed the 12-14-feat_traverse_add_traversescoping_rename_symbol_method branch from c6d3847 to de06015 Compare December 14, 2024 03:54
@overlookmotel overlookmotel force-pushed the 12-13-fix_transformer_class-properties_fix_symbol_clashes_in_instance_prop_initializers branch from 3830050 to 386c0ac Compare December 14, 2024 03:54
@Boshen Boshen changed the base branch from 12-14-feat_traverse_add_traversescoping_rename_symbol_method to graphite-base/7872 December 14, 2024 04:12
@Boshen Boshen force-pushed the graphite-base/7872 branch from de06015 to 53e2bc0 Compare December 14, 2024 04:56
@Boshen Boshen force-pushed the 12-13-fix_transformer_class-properties_fix_symbol_clashes_in_instance_prop_initializers branch from 386c0ac to ea9b831 Compare December 14, 2024 04:56
@Boshen Boshen changed the base branch from graphite-base/7872 to main December 14, 2024 04:57
@Boshen Boshen force-pushed the 12-13-fix_transformer_class-properties_fix_symbol_clashes_in_instance_prop_initializers branch from ea9b831 to 455c4a1 Compare December 14, 2024 04:57
@overlookmotel overlookmotel force-pushed the 12-13-fix_transformer_class-properties_fix_symbol_clashes_in_instance_prop_initializers branch from 455c4a1 to b12d26b Compare December 14, 2024 18:24
@overlookmotel overlookmotel marked this pull request as ready for review December 14, 2024 18:28
@overlookmotel overlookmotel force-pushed the 12-13-fix_transformer_class-properties_fix_symbol_clashes_in_instance_prop_initializers branch from b12d26b to 6fbf297 Compare December 14, 2024 18:29
@overlookmotel overlookmotel force-pushed the 12-13-fix_transformer_class-properties_fix_symbol_clashes_in_instance_prop_initializers branch 3 times, most recently from 80ad1cf to 21ebb97 Compare December 14, 2024 20:29
Copy link

graphite-app bot commented Dec 15, 2024

Merge activity

…p initializers (#7872)

Instance property initializers are moved into constructor. If symbols they reference are shadowed within constructor, rename those symbols.

Input:

```js
class C {
  prop = foo();
  constructor(foo) {
    console.log(foo);
  }
}
```

Output:

```js
class C {
  constructor(_foo) { // <-- renamed
    this.prop = foo(); // <-- moved into constructor
    console.log(_foo); // <-- renamed
  }
}
```
@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 pushed a commit that referenced this pull request Dec 15, 2024
…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.
@graphite-app graphite-app bot merged commit e76fbb0 into main Dec 15, 2024
26 checks passed
@graphite-app graphite-app bot deleted the 12-13-fix_transformer_class-properties_fix_symbol_clashes_in_instance_prop_initializers branch December 15, 2024 01:57
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-bug Category - Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant