Skip to content

Commit be94bfd

Browse files
committed
fix(semantic): add scope tracking for with statements (#14652)
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
1 parent c69201c commit be94bfd

File tree

13 files changed

+177
-5
lines changed

13 files changed

+177
-5
lines changed

crates/oxc_ast/src/ast/js.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1384,12 +1384,15 @@ pub struct ReturnStatement<'a> {
13841384

13851385
/// With Statement
13861386
#[ast(visit)]
1387+
#[scope]
13871388
#[derive(Debug)]
13881389
#[generate_derive(CloneIn, Dummy, TakeIn, GetSpan, GetSpanMut, ContentEq, ESTree)]
13891390
pub struct WithStatement<'a> {
13901391
pub span: Span,
13911392
pub object: Expression<'a>,
1393+
#[scope(enter_before)]
13921394
pub body: Statement<'a>,
1395+
pub scope_id: Cell<Option<ScopeId>>,
13931396
}
13941397

13951398
/// Switch Statement

crates/oxc_ast/src/generated/assert_layouts.rs

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -461,12 +461,13 @@ const _: () = {
461461
assert!(offset_of!(ReturnStatement, span) == 0);
462462
assert!(offset_of!(ReturnStatement, argument) == 8);
463463

464-
// Padding: 0 bytes
465-
assert!(size_of::<WithStatement>() == 40);
464+
// Padding: 4 bytes
465+
assert!(size_of::<WithStatement>() == 48);
466466
assert!(align_of::<WithStatement>() == 8);
467467
assert!(offset_of!(WithStatement, span) == 0);
468468
assert!(offset_of!(WithStatement, object) == 8);
469469
assert!(offset_of!(WithStatement, body) == 24);
470+
assert!(offset_of!(WithStatement, scope_id) == 40);
470471

471472
// Padding: 4 bytes
472473
assert!(size_of::<SwitchStatement>() == 56);
@@ -2066,11 +2067,12 @@ const _: () = if cfg!(target_family = "wasm") || align_of::<u64>() == 8 {
20662067
assert!(offset_of!(ReturnStatement, argument) == 8);
20672068

20682069
// Padding: 0 bytes
2069-
assert!(size_of::<WithStatement>() == 24);
2070+
assert!(size_of::<WithStatement>() == 28);
20702071
assert!(align_of::<WithStatement>() == 4);
20712072
assert!(offset_of!(WithStatement, span) == 0);
20722073
assert!(offset_of!(WithStatement, object) == 8);
20732074
assert!(offset_of!(WithStatement, body) == 16);
2075+
assert!(offset_of!(WithStatement, scope_id) == 24);
20742076

20752077
// Padding: 0 bytes
20762078
assert!(size_of::<SwitchStatement>() == 36);

crates/oxc_ast/src/generated/ast_builder.rs

Lines changed: 65 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3742,6 +3742,28 @@ impl<'a> AstBuilder<'a> {
37423742
Statement::WithStatement(self.alloc_with_statement(span, object, body))
37433743
}
37443744

3745+
/// Build a [`Statement::WithStatement`] with `scope_id`.
3746+
///
3747+
/// This node contains a [`WithStatement`] that will be stored in the memory arena.
3748+
///
3749+
/// ## Parameters
3750+
/// * `span`: The [`Span`] covering this node
3751+
/// * `object`
3752+
/// * `body`
3753+
/// * `scope_id`
3754+
#[inline]
3755+
pub fn statement_with_with_scope_id(
3756+
self,
3757+
span: Span,
3758+
object: Expression<'a>,
3759+
body: Statement<'a>,
3760+
scope_id: ScopeId,
3761+
) -> Statement<'a> {
3762+
Statement::WithStatement(
3763+
self.alloc_with_statement_with_scope_id(span, object, body, scope_id),
3764+
)
3765+
}
3766+
37453767
/// Build a [`Directive`].
37463768
///
37473769
/// ## Parameters
@@ -5047,7 +5069,7 @@ impl<'a> AstBuilder<'a> {
50475069
object: Expression<'a>,
50485070
body: Statement<'a>,
50495071
) -> WithStatement<'a> {
5050-
WithStatement { span, object, body }
5072+
WithStatement { span, object, body, scope_id: Default::default() }
50515073
}
50525074

50535075
/// Build a [`WithStatement`], and store it in the memory arena.
@@ -5069,6 +5091,48 @@ impl<'a> AstBuilder<'a> {
50695091
Box::new_in(self.with_statement(span, object, body), self.allocator)
50705092
}
50715093

5094+
/// Build a [`WithStatement`] with `scope_id`.
5095+
///
5096+
/// If you want the built node to be allocated in the memory arena,
5097+
/// use [`AstBuilder::alloc_with_statement_with_scope_id`] instead.
5098+
///
5099+
/// ## Parameters
5100+
/// * `span`: The [`Span`] covering this node
5101+
/// * `object`
5102+
/// * `body`
5103+
/// * `scope_id`
5104+
#[inline]
5105+
pub fn with_statement_with_scope_id(
5106+
self,
5107+
span: Span,
5108+
object: Expression<'a>,
5109+
body: Statement<'a>,
5110+
scope_id: ScopeId,
5111+
) -> WithStatement<'a> {
5112+
WithStatement { span, object, body, scope_id: Cell::new(Some(scope_id)) }
5113+
}
5114+
5115+
/// Build a [`WithStatement`] with `scope_id`, and store it in the memory arena.
5116+
///
5117+
/// Returns a [`Box`] containing the newly-allocated node.
5118+
/// If you want a stack-allocated node, use [`AstBuilder::with_statement_with_scope_id`] instead.
5119+
///
5120+
/// ## Parameters
5121+
/// * `span`: The [`Span`] covering this node
5122+
/// * `object`
5123+
/// * `body`
5124+
/// * `scope_id`
5125+
#[inline]
5126+
pub fn alloc_with_statement_with_scope_id(
5127+
self,
5128+
span: Span,
5129+
object: Expression<'a>,
5130+
body: Statement<'a>,
5131+
scope_id: ScopeId,
5132+
) -> Box<'a, WithStatement<'a>> {
5133+
Box::new_in(self.with_statement_with_scope_id(span, object, body, scope_id), self.allocator)
5134+
}
5135+
50725136
/// Build a [`SwitchStatement`].
50735137
///
50745138
/// If you want the built node to be allocated in the memory arena,

crates/oxc_ast/src/generated/derive_clone_in.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3201,6 +3201,7 @@ impl<'new_alloc> CloneIn<'new_alloc> for WithStatement<'_> {
32013201
span: CloneIn::clone_in(&self.span, allocator),
32023202
object: CloneIn::clone_in(&self.object, allocator),
32033203
body: CloneIn::clone_in(&self.body, allocator),
3204+
scope_id: Default::default(),
32043205
}
32053206
}
32063207

@@ -3209,6 +3210,7 @@ impl<'new_alloc> CloneIn<'new_alloc> for WithStatement<'_> {
32093210
span: CloneIn::clone_in_with_semantic_ids(&self.span, allocator),
32103211
object: CloneIn::clone_in_with_semantic_ids(&self.object, allocator),
32113212
body: CloneIn::clone_in_with_semantic_ids(&self.body, allocator),
3213+
scope_id: CloneIn::clone_in_with_semantic_ids(&self.scope_id, allocator),
32123214
}
32133215
}
32143216
}

crates/oxc_ast/src/generated/derive_dummy.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -855,6 +855,7 @@ impl<'a> Dummy<'a> for WithStatement<'a> {
855855
span: Dummy::dummy(allocator),
856856
object: Dummy::dummy(allocator),
857857
body: Dummy::dummy(allocator),
858+
scope_id: Dummy::dummy(allocator),
858859
}
859860
}
860861
}

crates/oxc_ast/src/generated/get_id.rs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,25 @@ impl ForOfStatement<'_> {
138138
}
139139
}
140140

141+
impl WithStatement<'_> {
142+
/// Get [`ScopeId`] of [`WithStatement`].
143+
///
144+
/// Only use this method on a post-semantic AST where [`ScopeId`]s are always defined.
145+
///
146+
/// # Panics
147+
/// Panics if `scope_id` is [`None`].
148+
#[inline]
149+
pub fn scope_id(&self) -> ScopeId {
150+
self.scope_id.get().unwrap()
151+
}
152+
153+
/// Set [`ScopeId`] of [`WithStatement`].
154+
#[inline]
155+
pub fn set_scope_id(&self, scope_id: ScopeId) {
156+
self.scope_id.set(Some(scope_id));
157+
}
158+
}
159+
141160
impl SwitchStatement<'_> {
142161
/// Get [`ScopeId`] of [`SwitchStatement`].
143162
///

crates/oxc_ast_visit/src/generated/visit.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2256,7 +2256,9 @@ pub mod walk {
22562256
visitor.enter_node(kind);
22572257
visitor.visit_span(&it.span);
22582258
visitor.visit_expression(&it.object);
2259+
visitor.enter_scope(ScopeFlags::empty(), &it.scope_id);
22592260
visitor.visit_statement(&it.body);
2261+
visitor.leave_scope();
22602262
visitor.leave_node(kind);
22612263
}
22622264

crates/oxc_ast_visit/src/generated/visit_mut.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2314,7 +2314,9 @@ pub mod walk_mut {
23142314
visitor.enter_node(kind);
23152315
visitor.visit_span(&mut it.span);
23162316
visitor.visit_expression(&mut it.object);
2317+
visitor.enter_scope(ScopeFlags::empty(), &it.scope_id);
23172318
visitor.visit_statement(&mut it.body);
2319+
visitor.leave_scope();
23182320
visitor.leave_node(kind);
23192321
}
23202322

crates/oxc_semantic/src/builder.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1678,6 +1678,7 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {
16781678
/* cfg */
16791679

16801680
self.visit_expression(&stmt.object);
1681+
self.enter_scope(ScopeFlags::empty(), &stmt.scope_id);
16811682

16821683
/* cfg - body basic block */
16831684
#[cfg(feature = "cfg")]
@@ -1697,6 +1698,7 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {
16971698
});
16981699
/* cfg */
16991700

1701+
self.leave_scope();
17001702
self.leave_node(kind);
17011703
}
17021704

crates/oxc_semantic/tests/integration/scopes.rs

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -284,3 +284,54 @@ fn test_eval() {
284284
assert!(!semantic.scoping().root_scope_flags().contains_direct_eval());
285285
}
286286
}
287+
288+
#[test]
289+
fn test_with_statement() {
290+
// Test that with statement creates a scope
291+
let tester = SemanticTester::js(
292+
"
293+
const foo = { x: 1 };
294+
with (foo) x;
295+
",
296+
)
297+
.with_module(false)
298+
.with_scope_tree_child_ids(true);
299+
300+
let semantic = tester.build();
301+
let scopes = semantic.scoping();
302+
303+
// Should have root scope + with statement scope
304+
assert_eq!(scopes.scopes_len(), 2, "with statement should create a scope");
305+
306+
// Verify scope tree structure
307+
let root_id = semantic.scoping().root_scope_id();
308+
let child_ids = semantic.scoping().get_scope_child_ids(root_id);
309+
assert_eq!(child_ids.len(), 1, "with statement scope should be a child of root scope");
310+
311+
// Verify the child scope is for the with statement
312+
let with_scope_id = child_ids[0];
313+
let with_scope_node_id = scopes.get_node_id(with_scope_id);
314+
let with_node = semantic.nodes().get_node(with_scope_node_id);
315+
assert!(
316+
matches!(with_node.kind(), AstKind::WithStatement(_)),
317+
"Child scope should be associated with WithStatement"
318+
);
319+
320+
// Test with block statement as body
321+
let tester2 = SemanticTester::js(
322+
"
323+
const foo = { x: 1 };
324+
with (foo) {
325+
const y = 2;
326+
}
327+
",
328+
)
329+
.with_module(false)
330+
.with_scope_tree_child_ids(true);
331+
332+
let semantic2 = tester2.build();
333+
let scopes2 = semantic2.scoping();
334+
335+
// Should have root scope + with statement scope + block statement scope
336+
assert_eq!(scopes2.scopes_len(), 3, "with statement and its block should create scopes");
337+
}

0 commit comments

Comments
 (0)