Skip to content

feat(ecmascript): handle typeof guarded global access as side effect free #12981

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

Merged
merged 7 commits into from
Aug 12, 2025

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Aug 11, 2025

This PR integrates sophisticated side effect analysis for unbound identifier references directly into Oxc's MayHaveSideEffects trait implementations, rather than exposing it as a separate public function.

What this implements

The enhanced analysis recognizes guard patterns that make accessing potentially undefined global variables safe:

// Safe to access `x` in the right side since we know it's defined  
typeof x !== 'undefined' && x

// Safe to access `x` in the else branch since we know it's defined
typeof x === 'undefined' ? fallback : x

// Safe to access `x` in the right side (string comparison optimization)
typeof x < 'u' && x

Implementation approach

Following code review feedback, the logic is integrated directly into the trait implementations:

  • LogicalExpression: Enhanced to analyze AND/OR patterns with typeof guards
  • ConditionalExpression: Enhanced to analyze ternary patterns with typeof guards
  • Private helper function: Contains the core analysis logic, not exposed in the public API

Testing

Comprehensive tests have been added to crates/oxc_minifier/tests/ecmascript/may_have_side_effects.rs covering:

  • Logical AND patterns: typeof x !== 'undefined' && x
  • Logical OR patterns: typeof x === 'undefined' || x
  • Conditional patterns: typeof x === 'undefined' ? fallback : x
  • String comparison optimizations: typeof x < 'u' && x
  • Negative cases ensuring side effects are properly detected when guards don't apply

Use cases

This enhancement is particularly valuable for:

  • Tree-shaking optimizations - determining when global variable access can be safely eliminated
  • Dead code elimination - identifying unreachable code paths based on type guards
  • Bundler optimizations - similar to how Rolldown and esbuild analyze side effects

The implementation maintains full compatibility with Oxc's existing side effects analysis framework while providing enhanced capabilities for modern JavaScript bundling and optimization tools.

Ported from: https://github.com/evanw/esbuild/blob/d34e79e2a998c21bb71d57b92b0017ca11756912/internal/js_ast/js_ast_helpers.go#L2594-L2639


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Copy link
Contributor

graphite-app bot commented Aug 11, 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.

@Copilot Copilot AI changed the title [WIP] Port is_side_effect_free_unbound_identifier_ref check from @rolldown/rolldown/files/crates/rolldown/src/ast_scanner/side_effect_detector/utils.rs and @rolldown/rolldown/files/crates/rolldown/src/ast_scanner/side_effect_detector/mod.rs to @oxc-project... Port is_side_effect_free_unbound_identifier_ref from Rolldown to Oxc Aug 11, 2025
@Copilot Copilot AI requested a review from sapphi-red August 11, 2025 04:00
Copilot finished work on behalf of sapphi-red August 11, 2025 04:00
@Copilot Copilot AI changed the title Port is_side_effect_free_unbound_identifier_ref from Rolldown to Oxc Integrate side effect analysis into MayHaveSideEffects trait implementations Aug 11, 2025
@Copilot Copilot AI requested a review from sapphi-red August 11, 2025 04:25
Copilot finished work on behalf of sapphi-red August 11, 2025 04:25
@sapphi-red
Copy link
Member

Copy link
Contributor Author

Copilot AI commented Aug 11, 2025

@copilot add tests to https://github.com/oxc-project/oxc/blob/main/crates/oxc_minifier/tests/ecmascript/may_have_side_effects.rs

Added comprehensive tests for the typeof guard patterns in may_have_side_effects.rs. The tests cover all the enhanced side effect analysis patterns including logical AND/OR patterns, conditional expressions, and negative cases to ensure the optimization works correctly. Tests committed in 3b385d6.

Copilot finished work on behalf of sapphi-red August 11, 2025 04:37
@github-actions github-actions bot added the A-minifier Area - Minifier label Aug 11, 2025
Copy link

codspeed-hq bot commented Aug 11, 2025

CodSpeed Instrumentation Performance Report

Merging #12981 will not alter performance

Comparing copilot/fix-f1053a7e-2cd4-45ea-b4e3-78f8bb81fcb0 (a9e817c) with main (ca91a26)

Summary

✅ 34 untouched benchmarks

@Copilot Copilot AI requested a review from sapphi-red August 11, 2025 17:02
Copilot finished work on behalf of sapphi-red August 11, 2025 17:02
Copilot AI and others added 6 commits August 12, 2025 02:47
…tations

Instead of exposing is_side_effect_free_unbound_identifier_ref as a public function, integrate the logic directly into LogicalExpression and ConditionalExpression implementations as suggested in the review feedback.

Co-authored-by: sapphi-red <[email protected]>
@sapphi-red sapphi-red force-pushed the copilot/fix-f1053a7e-2cd4-45ea-b4e3-78f8bb81fcb0 branch from 2b96c5c to f3cea22 Compare August 11, 2025 18:01
Copy link
Member


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.

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

@sapphi-red sapphi-red changed the title Integrate side effect analysis into MayHaveSideEffects trait implementations feat(ecmascript): handle typeof guarded global access as side effect free Aug 11, 2025
@github-actions github-actions bot added the C-enhancement Category - New feature or request label Aug 11, 2025
@sapphi-red sapphi-red marked this pull request as ready for review August 11, 2025 18:06
@Copilot Copilot AI review requested due to automatic review settings August 11, 2025 18:06
Copy link

@Copilot 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 enhances Oxc's side effect analysis to recognize typeof guard patterns that make accessing potentially undefined global variables safe. The implementation integrates sophisticated guard pattern detection directly into the MayHaveSideEffects trait implementations.

Key changes:

  • Enhanced LogicalExpression and ConditionalExpression to analyze typeof guard patterns
  • Added comprehensive helper function for detecting safe unbound identifier access patterns
  • Extensive test coverage for various guard patterns including equality checks and string comparisons

Reviewed Changes

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

File Description
crates/oxc_ecmascript/src/side_effects/may_have_side_effects.rs Implements core guard pattern analysis logic in trait implementations and adds helper function
crates/oxc_minifier/tests/ecmascript/may_have_side_effects.rs Adds comprehensive test cases covering all supported guard patterns and edge cases

@Boshen Boshen merged commit 54d1750 into main Aug 12, 2025
30 checks passed
@Boshen Boshen deleted the copilot/fix-f1053a7e-2cd4-45ea-b4e3-78f8bb81fcb0 branch August 12, 2025 01:28
taearls pushed a commit to taearls/oxc that referenced this pull request Aug 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-minifier Area - Minifier C-enhancement Category - New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants