Skip to content

Conversation

@Boshen
Copy link
Member

@Boshen Boshen commented Oct 16, 2025

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

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

Copilot AI review requested due to automatic review settings October 16, 2025 07:29
Copy link
Contributor

Copilot AI left a 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 WithStatement in the AST definition
  • Modified the semantic builder to create and manage scopes for with statements
  • 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.

@graphite-app
Copy link
Contributor

graphite-app bot commented Oct 16, 2025

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.

@github-actions github-actions bot added A-semantic Area - Semantic A-ast Area - AST C-bug Category - Bug labels Oct 16, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Oct 16, 2025

CodSpeed Performance Report

Merging #14652 will not alter performance

Comparing fix/with-statement-scope (be94bfd) with main (c69201c)

Summary

✅ 37 untouched

Copy link
Member

@overlookmotel overlookmotel left a 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.

@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Oct 16, 2025
Copy link
Member Author

Boshen commented Oct 16, 2025

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
@graphite-app graphite-app bot force-pushed the fix/with-statement-scope branch from 3787b46 to be94bfd Compare October 16, 2025 10:02
@graphite-app graphite-app bot merged commit be94bfd into main Oct 16, 2025
29 checks passed
@graphite-app graphite-app bot deleted the fix/with-statement-scope branch October 16, 2025 10:07
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Oct 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ast Area - AST A-semantic Area - Semantic C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

3 participants