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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Nov 22, 2024
Merged

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

merged 1 commit into from
Nov 22, 2024

Conversation

Dunqing
Copy link
Member

@Dunqing Dunqing commented Nov 21, 2024 •
edited
Loading

close #7323

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

Changes

  1. The left-hand of AssignmentExpression is always ReferenceFlags::Write
let a = 0;
console.log(a = 0);
            ^ Write only
  1. 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.

@Dunqing Dunqing requested a review from Boshen as a code owner November 21, 2024 08:20
@graphite-app Graphite App
Copy link

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

@Dunqing Graphite App
Copy link
Member Author

Dunqing commented Nov 21, 2024 •
edited
Loading

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

@Dunqing Dunqing changed the title refactor(semantic): infer ReferenceFlags from Visit methods refactor(semantic): infer ReferenceFlags from Visit methods Nov 21, 2024
@Dunqing 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 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 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 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
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Nov 21, 2024 •
edited
Loading

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 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
@Dunqing
Copy link
Member Author

Dunqing commented Nov 21, 2024

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
Boshen requested changes Nov 21, 2024
View reviewed changes
Copy link
Member

@Boshen Boshen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const FOO = 1;

({
  files = FOO,
} = arg1);

Boshen
Boshen approved these changes Nov 21, 2024
View reviewed changes
crates/oxc_semantic/src/builder.rs Outdated Show resolved Hide resolved
@Boshen
Copy link
Member

Boshen commented Nov 21, 2024

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
Copy link
Member Author

Dunqing commented Nov 21, 2024

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 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 github-actions bot added the C-bug Category - Bug label Nov 21, 2024
@Dunqing 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 Dunqing mentioned this pull request Nov 22, 2024
Merged
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Nov 22, 2024
@Boshen Graphite App
Copy link
Member

Boshen commented Nov 22, 2024 •
edited by graphite-app bot
Loading

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.

Boshen pushed a commit that referenced this pull request Nov 22, 2024
@Dunqing
fix(semantic)!: correct all ReferenceFlags::Write according to the …
c28477b 
…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.
@Boshen Boshen force-pushed the 11-21-refactor_semantic_infer_referenceflags_from_visit_methods branch from 8c295bd to c28477b Compare November 22, 2024 06:06
@Dunqing
fix(semantic)!: correct all ReferenceFlags::Write according to the …
6f0fe38 
…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.
@Boshen 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 graphite-app bot merged commit 6f0fe38 into main Nov 22, 2024
27 checks passed
@graphite-app graphite-app bot deleted the 11-21-refactor_semantic_infer_referenceflags_from_visit_methods branch November 22, 2024 06:16
This was referenced Nov 22, 2024
Merged
Merged
Merged
Merged
@oxc-bot oxc-bot mentioned this pull request Nov 23, 2024
Merged
@overlookmotel
Copy link
Contributor

@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).

@oxc-bot oxc-bot mentioned this pull request Nov 26, 2024
Merged
@Dunqing
Copy link
Member Author

Dunqing commented Nov 26, 2024 •
edited by overlookmotel
Loading

@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.

@Dunqing Dunqing mentioned this pull request Nov 26, 2024
Merged
Dunqing added a commit that referenced this pull request Nov 27, 2024
@Dunqing
test(semantic): add a test for UpdateExpression (#7495)
fed8327 
related: #7388 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Boshen Boshen Boshen approved these changes

Assignees
No one assigned
Labels
0-merge Merge with Graphite Merge Queue A-linter Area - Linter A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler C-bug Category - Bug C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

semantic: ReferenceFlag::Write should only be assigned for assignment expressions
3 participants
@Dunqing @Boshen @overlookmotel