fix(semantic)!: correct all `ReferenceFlags::Write` according to the spec by Dunqing · Pull Request #7388 · oxc-project/oxc

Dunqing · 2024-11-21T08:20:18Z

According to the specification re-design the JavaScript-part ReferenceFlags inferring approach.

Changes

The left-hand of AssignmentExpression is always ReferenceFlags::Write

let a = 0;
console.log(a = 0);
            ^ Write only

The argument of UpdateExpression is always ReferenceFlags::Read | Write

let a = 0;
a++;
^ Read and Write

This change might cause some trouble for Minfier to remove this code, because ‘a’ is not used elsewhere. I have taken a look at esbuild and Terser. Only the Terser can remove this code.

graphite-app · 2024-11-21T08:20:23Z

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.

Dunqing · 2024-11-21T08:20:34Z

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

codspeed-hq · 2024-11-21T08:34:08Z

CodSpeed Performance Report

Merging #7388 will not alter performance

_{Comparing 11-21-refactor_semantic_infer_referenceflags_from_visit_methods (6f0fe38) with main (6730e3e)}

Summary

✅ 30 untouched benchmarks

Dunqing · 2024-11-21T09:03:39Z

I prefer to fix transformer-related semantic errors in follow-up PR, but if you think should be fixed in this PR I am okay

Boshen

Can you add the test case to no-const-assign?

const FOO = 1;

({
  files = FOO,
} = arg1);

crates/oxc_semantic/src/builder.rs

Boshen · 2024-11-21T09:12:09Z

Can

Can you add the test case to no-const-assign?
const FOO = 1;

({
  files = FOO,
} = arg1);

Can you add all the assignment cases to no object assign?

Dunqing · 2024-11-21T09:17:17Z

Can you add the test case to no-const-assign?
const FOO = 1;

({
  files = FOO,
} = arg1);

Added

Can you add all the assignment cases to no object assign?

I have already added a lot of assignment-related tests in semantic.

Boshen · 2024-11-22T06:05:49Z

Merge activity

Nov 22, 1:05 AM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Nov 22, 1:05 AM EST: A user added this pull request to the Graphite merge queue.
Nov 22, 1:16 AM EST: A user merged this pull request with the Graphite merge queue.

…spec (#7388) close #7323 According to the specification re-design the JavaScript-part ReferenceFlags inferring approach. * https://tc39.es/ecma262/#sec-assignment-operators-runtime-semantics-evaluation * https://tc39.es/ecma262/#sec-postfix-increment-operator-runtime-semantics-evaluation * https://tc39.es/ecma262/#sec-runtime-semantics-restdestructuringassignmentevaluation * ... See references of https://tc39.es/ecma262/#sec-putvalue ### Changes 1. The left-hand of `AssignmentExpression` is always `ReferenceFlags::Write` ```js let a = 0; console.log(a = 0); ^ Write only ``` 2. The `argument` of `UpdateExpression` is always `ReferenceFlags::Read | Write` ```js let a = 0; a++; ^ Read and Write ``` This change might cause some trouble for `Minfier` to remove this code, because ‘a’ is not used elsewhere. I have taken a look at `esbuild` and `Terser`. Only the `Terser` can remove this code.

overlookmotel · 2024-11-24T14:52:37Z

@Dunqing Is x in x++ now Read | Write? I can't see tests for this (if I remember right, previously it was Write only which is incorrect).

Dunqing · 2024-11-26T15:35:11Z

@Dunqing Is x in x++ now Read | Write? I can't see tests for this (if I remember right, previously it was Write only which is incorrect).

Yes, you are right. The test is added in #7495.

related: #7388 (comment)

Dunqing requested a review from Boshen as a code owner November 21, 2024 08:20

Dunqing changed the title ~~refactor(semantic): infer ReferenceFlags from Visit methods~~ refactor(semantic): infer ReferenceFlags from Visit methods Nov 21, 2024

Dunqing force-pushed the 11-21-refactor_semantic_infer_referenceflags_from_visit_methods branch from f3b83b8 to edcf14b Compare November 21, 2024 08:21

github-actions bot added A-linter Area - Linter A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Nov 21, 2024

Dunqing changed the title ~~refactor(semantic): infer ReferenceFlags from Visit methods~~ refactor(semantic): re-design the JavaScript-part ReferenceFlags inferring approach Nov 21, 2024

Dunqing changed the title ~~refactor(semantic): re-design the JavaScript-part ReferenceFlags inferring approach~~ refactor(semantic): re-design the JavaScript-part ReferenceFlags inferring approach Nov 21, 2024

Dunqing force-pushed the 11-21-refactor_semantic_infer_referenceflags_from_visit_methods branch 2 times, most recently from bab338b to 419b321 Compare November 21, 2024 08:58

Boshen requested changes Nov 21, 2024

View reviewed changes

Boshen approved these changes Nov 21, 2024

View reviewed changes

crates/oxc_semantic/src/builder.rs Outdated Show resolved Hide resolved

Boshen changed the title ~~refactor(semantic): re-design the JavaScript-part ReferenceFlags inferring approach~~ fix(semantic)!: correct all ReferenceFlags::Write according to the spec Nov 21, 2024

github-actions bot added the C-bug Category - Bug label Nov 21, 2024

Dunqing force-pushed the 11-21-refactor_semantic_infer_referenceflags_from_visit_methods branch from b3a05fa to 8c295bd Compare November 22, 2024 05:22

Dunqing mentioned this pull request Nov 22, 2024

fix(transformer): correct all ReferenceFlags #7410

Merged

Boshen added the 0-merge Merge with Graphite Merge Queue label Nov 22, 2024

Boshen force-pushed the 11-21-refactor_semantic_infer_referenceflags_from_visit_methods branch from 8c295bd to c28477b Compare November 22, 2024 06:06

Boshen force-pushed the 11-21-refactor_semantic_infer_referenceflags_from_visit_methods branch from c28477b to 6f0fe38 Compare November 22, 2024 06:08

graphite-app bot merged commit 6f0fe38 into main Nov 22, 2024

graphite-app bot deleted the 11-21-refactor_semantic_infer_referenceflags_from_visit_methods branch November 22, 2024 06:16

oxc-bot mentioned this pull request Nov 23, 2024

release(oxlint): v0.13.1 #7441

Merged

oxc-bot mentioned this pull request Nov 26, 2024

release(crates): v0.38.0 #7490

Merged

Dunqing mentioned this pull request Nov 26, 2024

test(semantic): add a test for UpdateExpression #7495

Merged

Dunqing added a commit that referenced this pull request Nov 27, 2024

test(semantic): add a test for UpdateExpression (#7495)

fed8327

related: #7388 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

fix(semantic)!: correct all `ReferenceFlags::Write` according to the spec#7388

fix(semantic)!: correct all `ReferenceFlags::Write` according to the spec#7388
graphite-app[bot] merged 1 commit intomainfrom
11-21-refactor_semantic_infer_referenceflags_from_visit_methods

Dunqing commented Nov 21, 2024 •

edited

Loading

Uh oh!

graphite-app bot commented Nov 21, 2024

Uh oh!

Dunqing commented Nov 21, 2024 •

edited

Loading

Uh oh!

codspeed-hq bot commented Nov 21, 2024 •

edited

Loading

Uh oh!

Dunqing commented Nov 21, 2024

Uh oh!

Boshen left a comment

Uh oh!

Uh oh!

Boshen commented Nov 21, 2024

Uh oh!

Dunqing commented Nov 21, 2024

Uh oh!

Boshen commented Nov 22, 2024 •

edited by graphite-app bot

Loading

Uh oh!

overlookmotel commented Nov 24, 2024

Uh oh!

Dunqing commented Nov 26, 2024 •

edited by overlookmotel

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Comments

Conversation

Dunqing commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

graphite-app bot commented Nov 21, 2024

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

Uh oh!

Dunqing commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

CodSpeed Performance Report

Merging #7388 will not alter performance

Summary

Uh oh!

Dunqing commented Nov 21, 2024

Uh oh!

Boshen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Boshen commented Nov 21, 2024

Uh oh!

Dunqing commented Nov 21, 2024

Uh oh!

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

Uh oh!

Merge activity

Uh oh!

overlookmotel commented Nov 24, 2024

Uh oh!

Dunqing commented Nov 26, 2024 • edited by overlookmotel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Dunqing commented Nov 21, 2024 •

edited

Loading

Dunqing commented Nov 21, 2024 •

edited

Loading

codspeed-hq bot commented Nov 21, 2024 •

edited

Loading

Boshen commented Nov 22, 2024 •

edited by graphite-app bot

Loading

Dunqing commented Nov 26, 2024 •

edited by overlookmotel

Loading