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): create temp var for class where required #7516

Conversation

overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented Nov 27, 2024

Fix class properties transform to create a temp var for class when it's required.

Input:

class C {
  static getSelf = () => this;
}
const C2 = C;
C = 123;
assert(C2.getSelf() === C);

Output:

var _C;
class C {}
_C = C;
_defineProperty(C, "getSelf", () => _C);
const C2 = C;
C = 123;
assert(C2.getSelf() === C);

Previously, temp var wasn't used so code was _defineProperty(C, "getSelf", () => C);. C is altered later by C = 123, so C2.getSelf() returned 123, instead of reference to the class.

The logic around when a temp var is required and when it's not, and when/where it's referenced is ridiculously complicated. So add some debug assert mechanisms to double-check the logic.

Copy link

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

@github-actions github-actions bot added A-transformer Area - Transformer / Transpiler C-bug Category - Bug labels Nov 27, 2024
@overlookmotel overlookmotel marked this pull request as ready for review November 27, 2024 23:50
@overlookmotel overlookmotel marked this pull request as draft November 27, 2024 23:50
@overlookmotel overlookmotel removed the request for review from Dunqing November 27, 2024 23:50
@overlookmotel overlookmotel force-pushed the 11-27-fix_transformer_class-properties_create_temp_var_for_class_where_required branch 2 times, most recently from d7f5fd1 to 32d3ca9 Compare November 28, 2024 21:20
@overlookmotel overlookmotel changed the base branch from main to 11-28-refactor_transformer_class-properties_resolvedprivateprop_type November 28, 2024 21:20
Copy link

codspeed-hq bot commented Nov 28, 2024

CodSpeed Performance Report

Merging #7516 will not alter performance

Comparing 11-27-fix_transformer_class-properties_create_temp_var_for_class_where_required (0eadd9f) with 12-02-refactor_transformer_class-properties_privatepropsstack_type (a07f278)

Summary

✅ 30 untouched benchmarks

@overlookmotel overlookmotel force-pushed the 11-28-refactor_transformer_class-properties_resolvedprivateprop_type branch from d77528d to 2ea88cf Compare November 28, 2024 22:08
@overlookmotel overlookmotel force-pushed the 11-27-fix_transformer_class-properties_create_temp_var_for_class_where_required branch from 32d3ca9 to afcf033 Compare November 28, 2024 22:09
@overlookmotel overlookmotel force-pushed the 11-28-refactor_transformer_class-properties_resolvedprivateprop_type branch from 2ea88cf to 6b7c4f5 Compare November 28, 2024 22:23
@overlookmotel overlookmotel force-pushed the 11-27-fix_transformer_class-properties_create_temp_var_for_class_where_required branch from afcf033 to 0727af9 Compare November 28, 2024 22:23
@overlookmotel overlookmotel force-pushed the 11-28-refactor_transformer_class-properties_resolvedprivateprop_type branch from 6b7c4f5 to 2509562 Compare November 29, 2024 12:01
@overlookmotel overlookmotel force-pushed the 11-27-fix_transformer_class-properties_create_temp_var_for_class_where_required branch from 0727af9 to 0036051 Compare November 29, 2024 12:01
@overlookmotel overlookmotel force-pushed the 11-28-refactor_transformer_class-properties_resolvedprivateprop_type branch from 2509562 to 2728d4b Compare November 29, 2024 12:20
@overlookmotel overlookmotel force-pushed the 11-27-fix_transformer_class-properties_create_temp_var_for_class_where_required branch 2 times, most recently from 33e0cfc to 7b1b1df Compare November 29, 2024 14:49
@overlookmotel overlookmotel force-pushed the 11-28-refactor_transformer_class-properties_resolvedprivateprop_type branch from 2728d4b to 0282aa3 Compare November 29, 2024 17:26
@overlookmotel overlookmotel force-pushed the 11-27-fix_transformer_class-properties_create_temp_var_for_class_where_required branch 2 times, most recently from 1ce483f to 5c867b5 Compare November 30, 2024 12:00
@overlookmotel overlookmotel force-pushed the 11-28-refactor_transformer_class-properties_resolvedprivateprop_type branch from 0282aa3 to b9200ad Compare December 2, 2024 20:28
@overlookmotel overlookmotel force-pushed the 11-27-fix_transformer_class-properties_create_temp_var_for_class_where_required branch from c594277 to 0bb0da9 Compare December 2, 2024 20:28
@overlookmotel overlookmotel changed the base branch from 11-28-refactor_transformer_class-properties_resolvedprivateprop_type to 12-02-refactor_transformer_class-properties_move_creating_temp_var_out_of_main_loop December 2, 2024 20:29
@overlookmotel overlookmotel force-pushed the 11-27-fix_transformer_class-properties_create_temp_var_for_class_where_required branch from 0bb0da9 to 29fed4e Compare December 2, 2024 22:21
@overlookmotel overlookmotel changed the base branch from 12-02-refactor_transformer_class-properties_move_creating_temp_var_out_of_main_loop to 12-02-refactor_transformer_class-properties_privatepropsstack_type December 2, 2024 22:21
@overlookmotel overlookmotel force-pushed the 12-02-refactor_transformer_class-properties_privatepropsstack_type branch from 3547117 to 1a33bd2 Compare December 3, 2024 02:00
@overlookmotel overlookmotel force-pushed the 11-27-fix_transformer_class-properties_create_temp_var_for_class_where_required branch 2 times, most recently from efb778a to 66dc1b0 Compare December 3, 2024 02:12
@overlookmotel overlookmotel marked this pull request as ready for review December 3, 2024 02:37
@graphite-app graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Dec 3, 2024
Copy link

graphite-app bot commented Dec 3, 2024

Merge activity

…quired (#7516)

Fix class properties transform to create a temp var for class when it's required.

Input:

```js
class C {
  static getSelf = () => this;
}
const C2 = C;
C = 123;
assert(C2.getSelf() === C);
```

Output:

```js
var _C;
class C {}
_C = C;
_defineProperty(C, "getSelf", () => _C);
const C2 = C;
C = 123;
assert(C2.getSelf() === C);
```

Previously, temp var wasn't used so code was `_defineProperty(C, "getSelf", () => C);`. `C` is altered later by `C = 123`, so `C2.getSelf()` returned `123`, instead of reference to the class.

The logic around when a temp var is required and when it's not, and when/where it's referenced is ridiculously complicated. So add some debug assert mechanisms to double-check the logic.
Dunqing pushed a commit that referenced this pull request Dec 3, 2024
… main loop (#7587)

Small optimization. Move code out of the loop which determines if class property transform has nothing to do and can bail out early. This also clears the way for correcting the logic around when temp vars are/aren't created in #7516.
@Dunqing Dunqing force-pushed the 12-02-refactor_transformer_class-properties_privatepropsstack_type branch from 1a33bd2 to a07f278 Compare December 3, 2024 07:49
@Dunqing Dunqing force-pushed the 11-27-fix_transformer_class-properties_create_temp_var_for_class_where_required branch from 66dc1b0 to 0eadd9f Compare December 3, 2024 07:50
Dunqing pushed a commit that referenced this pull request Dec 3, 2024
#7516 removed the need for a special method to handle `export default class {}`. Delete it.
Base automatically changed from 12-02-refactor_transformer_class-properties_privatepropsstack_type to main December 3, 2024 08:32
@graphite-app graphite-app bot merged commit 0eadd9f into main Dec 3, 2024
29 checks passed
@graphite-app graphite-app bot deleted the 11-27-fix_transformer_class-properties_create_temp_var_for_class_where_required branch December 3, 2024 08:37
Dunqing pushed a commit that referenced this pull request Dec 3, 2024
…ith temp var in static prop initializers (#7610)

Similar to #7516. Fix class properties transform to replace references to class name in static prop initializers with temp var.

Input:

```js
class C {
  static getSelf = () => C;
}
const C2 = C;
C = 123;
assert(C2.getSelf() === C);
```

Output:

```js
var _C;
class C {}
_C = C;
_defineProperty(C, "getSelf", () => _C);
const C2 = C;
C = 123;
assert(C2.getSelf() === C);
```

Previously, temp var wasn't used so code was `_defineProperty(C, "getSelf", () => C);`. `C` is altered later by `C = 123`, so `C2.getSelf()` returned `123`, instead of reference to the class.
@oxc-bot oxc-bot mentioned this pull request Dec 4, 2024
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.

2 participants