-
-
Notifications
You must be signed in to change notification settings - Fork 721
fix(semantic): add scope tracking for with statements
#14652
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug in the semantic analysis where with statements were not properly creating scopes, causing incorrect global reference detection and potential minification issues. The fix ensures that with statements create proper lexical scopes according to the ECMAScript specification.
Key changes:
- Added scope tracking to
WithStatementin the AST definition - Modified the semantic builder to create and manage scopes for
withstatements - Added comprehensive test coverage to verify proper scope creation and tree structure
Reviewed Changes
Copilot reviewed 3 out of 13 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
crates/oxc_ast/src/ast/js.rs |
Added #[scope] attribute and scope_id field to WithStatement struct |
crates/oxc_semantic/src/builder.rs |
Modified visit_with_statement to enter/leave scopes around the body |
crates/oxc_semantic/tests/integration/scopes.rs |
Added comprehensive test coverage for with statement scope creation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
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. |
CodSpeed Performance ReportMerging #14652 will not alter performanceComparing Summary
|
overlookmotel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the scope should not cover object - scope should be entered just before body. This is similar to SwitchStatement.
Merge activity
|
Fixes #8365 ## Summary The `with` statement was not creating a scope in the semantic analysis, causing `is_global_reference` to incorrectly return `true` for references inside `with` blocks. This could lead to incorrect minification. ## Problem According to the ECMAScript specification, `with` statements should create an Object Environment Record that extends the lexical environment. Without proper scope tracking: - `is_global_reference()` incorrectly returns `true` for identifiers inside `with` blocks - This can cause minification to incorrectly treat local references as global - The scope tree doesn't accurately represent the program structure ## Solution ### Changes Made 1. **AST Definition** (`crates/oxc_ast/src/ast/js.rs`): - Added `#[scope]` attribute to `WithStatement` struct - Added `scope_id: Cell<Option<ScopeId>>` field 2. **Semantic Builder** (`crates/oxc_semantic/src/builder.rs`): - Modified `visit_with_statement` to call `enter_scope()` before visiting the body - Added `leave_scope()` call after visiting the body - Uses `ScopeFlags::empty()` similar to block statements 3. **Test Coverage** (`crates/oxc_semantic/tests/integration/scopes.rs`): - Added `test_with_statement` to verify scope creation - Tests both simple expressions and block statement bodies - Verifies proper scope tree structure ### Example ```javascript const foo = { Object: class { constructor() { console.log('Constructor') } } } with (foo) { console.log(new Object()) // should print "Constructor" } ``` Before this fix, `Object` inside the `with` block would be incorrectly identified as a global reference, potentially causing minification errors. ## Implementation Notes While this implementation doesn't fully model the complex object environment semantics (where property lookups can dynamically affect identifier resolution at runtime), it ensures: - References inside `with` blocks are no longer incorrectly marked as global - The scope tree properly represents the lexical structure - Provides a foundation for future improvements to object environment handling ## Testing All existing tests pass, plus the new test verifies: - `with` statements create scopes - Scopes are properly nested in the scope tree - Works correctly with both simple and block statement bodies
3787b46 to
be94bfd
Compare
Fixes #8365
Summary
The
withstatement was not creating a scope in the semantic analysis, causingis_global_referenceto incorrectly returntruefor references insidewithblocks. This could lead to incorrect minification.Problem
According to the ECMAScript specification,
withstatements should create an Object Environment Record that extends the lexical environment. Without proper scope tracking:is_global_reference()incorrectly returnstruefor identifiers insidewithblocksSolution
Changes Made
AST Definition (
crates/oxc_ast/src/ast/js.rs):#[scope]attribute toWithStatementstructscope_id: Cell<Option<ScopeId>>fieldSemantic Builder (
crates/oxc_semantic/src/builder.rs):visit_with_statementto callenter_scope()before visiting the bodyleave_scope()call after visiting the bodyScopeFlags::empty()similar to block statementsTest Coverage (
crates/oxc_semantic/tests/integration/scopes.rs):test_with_statementto verify scope creationExample
Before this fix,
Objectinside thewithblock would be incorrectly identified as a global reference, potentially causing minification errors.Implementation Notes
While this implementation doesn't fully model the complex object environment semantics (where property lookups can dynamically affect identifier resolution at runtime), it ensures:
withblocks are no longer incorrectly marked as globalTesting
All existing tests pass, plus the new test verifies:
withstatements create scopes