feat(semantic): ScopeFlags::With by aapoalas · Pull Request #16291 · oxc-project/oxc

aapoalas · 2025-11-30T11:34:36Z

Simple addition but unfortunately untested: I locally fail tsgo related tests, probably for reasons related to not having tsgo installed?

graphite-app · 2025-11-30T11:34:42Z

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

0-merge - adds this PR to the back of the merge queue
hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

_{An organization admin has enabled the Graphite Merge Queue in this repository.} _{Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.}

crates/oxc_syntax/src/scope.rs

Copilot

Pull request overview

This PR adds support for the ScopeFlags::With flag to mark scopes created by JavaScript with statements. The implementation adds the flag definition, a helper method to check for it, and applies it when entering with statement scopes during semantic analysis.

Added ScopeFlags::With flag definition with documentation
Added is_with() helper method for checking the flag
Applied the flag when entering with statement scopes

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
crates/oxc_syntax/src/scope.rs	Defines the new `With` scope flag and adds the `is_with()` helper method; also adds `#[inline]` to `is_ts_conditional()`
crates/oxc_semantic/src/builder.rs	Updates the semantic builder to use `ScopeFlags::With` instead of `ScopeFlags::empty()` when entering `with` statement scopes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

crates/oxc_syntax/src/scope.rs

crates/oxc_semantic/src/builder.rs

codspeed-hq · 2025-11-30T13:28:23Z

CodSpeed Performance Report

Merging #16291 will not alter performance

_{Comparing aapoalas:feat/semantic-scope-flags-with-statement (7653a33) with main (1da3dc1)¹}

Summary

✅ 42 untouched
⏩ 3 skipped²

No successful run was found on main (250feb3) during the generation of this report, so 1da3dc1 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

lilnasy

Looks good to me! I need to review oxc_semantic for an upcoming feature, and I figured this would be a good opportunity. I am not too familiar with it yet so take my comments with a grain of salt.

crates/oxc_semantic/src/builder.rs

crates/oxc_syntax/src/scope.rs

@aapoalas

…ditional` (#16309) Add `#[inline]` attr to this method, for consistency with all the rest. This omission was spotted by @aapoalas in #16291.

overlookmotel

with complicates symbol resolution a lot. Is there anything else we need to do to make Semantic make sense of it?

For example, I guess minifier should not mangle var names where they're accessed "through" a with, in the same way that it I think takes account of whether a var can be "seen" by an eval().

e.g. let foo = 1; with ({foo: 2}) foo = 3; would change behavior if foo var is renamed. Though maybe that's such an edge case, we shouldn't worry about it.

Out of interest, what's your use case for this in Nova? My sincere condolences if you actually need to make with behave correctly. Nightmare!

aapoalas · 2025-12-01T04:31:18Z

with complicates symbol resolution a lot. Is there anything else we need to do to make Semantic make sense of it?

For example, I guess minifier should not mangle var names where they're accessed "through" a with, in the same way that it I think takes account of whether a var can be "seen" by an eval().

e.g. let foo = 1; with ({foo: 2}) foo = 3; would change behavior if foo var is renamed. Though maybe that's such an edge case, we shouldn't worry about it.

Out of interest, what's your use case for this in Nova? My sincere condolences if you actually need to make with behave correctly. Nightmare!

with is actually really simple! That is, from an engine point of view if the engine is not interested in doing any optimisations. I have like 90% of with done and am only missing the bytecode instruction for it because I'm lazy and don't like it as a feature :D

The use-case I have is actually the same that you describe, basically: I am currently writing a sorts of "runtime minification" (or just an optimisation) probably better known as register allocation, wherein I'd like to rewrite eg. the foo variable in your example into a stack slot if the transformation is sound. In a with scope, such a transformation is not sound as your example illustrates, so if I had proper with support then I'd need to be able to detect them for this optimisation to be soundly applied.

Currently, I am applying this optimisation based on the Scoping information mainly, hence my interest in the flag :)

@aapoalas

…ditional` (oxc-project#16309) Add `#[inline]` attr to this method, for consistency with all the rest. This omission was spotted by @aapoalas in oxc-project#16291.

graphite-app · 2025-12-12T06:28:30Z

Merge activity

Dec 12, 6:28 AM UTC: @aapoalas we removed the merge queue label because we could not find a Graphite account associated with your GitHub profile.

You must have a Graphite account in order to use the merge queue. Create an account and try again using this link

Boshen · 2025-12-12T06:29:06Z

Didn't find any problems from PR review, merge so we can get a release out next week.

feat(semantic): ScopeFlags::With

d8af2fe

Copilot AI review requested due to automatic review settings November 30, 2025 11:34

aapoalas requested a review from Dunqing as a code owner November 30, 2025 11:34

github-actions bot added A-semantic Area - Semantic C-enhancement Category - New feature or request labels Nov 30, 2025

Copilot started reviewing on behalf of aapoalas November 30, 2025 11:35 View session

aapoalas commented Nov 30, 2025

View reviewed changes

crates/oxc_syntax/src/scope.rs Show resolved Hide resolved

Boshen requested a review from overlookmotel November 30, 2025 11:37

Boshen assigned overlookmotel Nov 30, 2025

Copilot finished reviewing on behalf of aapoalas November 30, 2025 11:38

Copilot AI reviewed Nov 30, 2025

View reviewed changes

crates/oxc_syntax/src/scope.rs Show resolved Hide resolved

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

fix: add ScopeFlags::With to WithStatement scope attribute

72c022d

github-actions bot added the A-ast Area - AST label Nov 30, 2025

fix visitor tests

9cfc4a9

lilnasy reviewed Nov 30, 2025

View reviewed changes

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

crates/oxc_syntax/src/scope.rs Show resolved Hide resolved

overlookmotel mentioned this pull request Nov 30, 2025

refactor(syntax): add #[inline] attribute to ScopeFlags::is_ts_conditional #16309

Merged

overlookmotel reviewed Dec 1, 2025

View reviewed changes

Boshen mentioned this pull request Dec 3, 2025

semantics: scopes for with statements are not created (Object Environment Record) #8365

Closed

Boshen added the 0-merge Merge with Graphite Merge Queue label Dec 12, 2025

graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Dec 12, 2025

Merge branch 'main' into feat/semantic-scope-flags-with-statement

7653a33

Boshen merged commit d221921 into oxc-project:main Dec 12, 2025
28 checks passed

Boshen mentioned this pull request Dec 15, 2025

release(crates): oxc v0.103.0 #16887

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(semantic): ScopeFlags::With#16291

feat(semantic): ScopeFlags::With#16291
Boshen merged 4 commits intooxc-project:mainfrom
aapoalas:feat/semantic-scope-flags-with-statement

aapoalas commented Nov 30, 2025

Uh oh!

graphite-app bot commented Nov 30, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

codspeed-hq bot commented Nov 30, 2025 •

edited

Loading

Uh oh!

lilnasy left a comment

Uh oh!

Uh oh!

Uh oh!

overlookmotel left a comment •

edited

Loading

Uh oh!

aapoalas commented Dec 1, 2025

Uh oh!

graphite-app bot commented Dec 12, 2025

Uh oh!

Boshen commented Dec 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

aapoalas commented Nov 30, 2025

Uh oh!

graphite-app bot commented Nov 30, 2025

How to use the Graphite Merge Queue

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

CodSpeed Performance Report

Merging #16291 will not alter performance

Summary

Footnotes

Uh oh!

lilnasy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

overlookmotel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aapoalas commented Dec 1, 2025

Uh oh!

graphite-app bot commented Dec 12, 2025

Merge activity

Uh oh!

Boshen commented Dec 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codspeed-hq bot commented Nov 30, 2025 •

edited

Loading

overlookmotel left a comment •

edited

Loading