fix(minifier): avoid incorrect logical assignment transformation when base object may be mutated

[camc314]

Fixes #16647

Use symbol_is_mutated() to check if the member expression base object may be reassigned, preventing incorrect transformations like x.y || (x = {}, x.y = 3) to x.y ||= (x = {}, 3).

🤖 generated with help from Claude Opus 4.5

[camc314]

• fix(minifier): avoid incorrect logical assignment transformation when base object may be mutated #16802 👈 (View in Graphite)
• main

---

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.

[Copilot]

Pull request overview

This PR fixes an incorrect minifier transformation where logical expressions with member expressions could be incorrectly transformed to logical assignment operators when the base object is mutated in a sequence expression. The fix adds a check to prevent transforming expressions like x.y || (x = {}, x.y = 3) to x.y ||= (x = {}, 3), which would have different semantics because the logical assignment operator captures x before evaluating the RHS.

• Adds a guard to check if member expression base objects may be mutated before applying the optimization
• Implements helper functions to recursively check if expressions reference mutated symbols
• Adds comprehensive test coverage for various mutation scenarios

Reviewed changes

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

File	Description
crates/oxc_minifier/src/peephole/minimize_logical_expression.rs	Adds member_object_may_be_mutated and expression_reference_may_be_mutated helper functions to check if member expression base objects may be mutated, and uses this check to guard the logical assignment optimization for sequence expressions
crates/oxc_minifier/src/peephole/minimize_conditions.rs	Adds comprehensive test cases covering various scenarios where the base object is mutated (direct assignment, nested properties, function mutations) and cases where transformation is still safe

---

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

[codspeed-hq]

CodSpeed Performance Report

Merging #16802 will not alter performance

_{Comparing c/12-13-fix_minifier_avoid_incorrect_logical_assignment_transformation_when_base_object_may_be_mutated (76b1dc7) with main (0f63e75)¹}

Summary

✅ 38 untouched
⏩ 7 skipped²

Footnotes

1. No successful run was found on main (b2b87c6) during the generation of this report, so 0f63e75 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

2. 7 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. ↩

[charliecreates]

The new guard correctly addresses the x.y || (x = {}, x.y = 3) mis-optimization, but the safety check may be under-conservative for this roots and is easy to misread due to naming that doesn’t reflect its “object identity stability” intent. Tightening the conservatism for this and documenting the invariant would reduce the chance of future incorrect rewrites. The added regression tests are strong and cover the reported issue well.

Additional notes (2)

• Maintainability | crates/oxc_minifier/src/peephole/minimize_logical_expression.rs:296-329
  The helper name expression_reference_may_be_mutated is doing two different things:

1. For identifiers, it checks symbol_is_mutated() (good and precise).
2. For all other expressions, it returns true (unknown), which is broader than “reference may be mutated” (it’s more like “object identity may differ between evaluations”).

This matters because the call site is specifically about capturing the base object for member assignment LHS under ||=/&&=/??=. The current logic is conservative (good for correctness), but the naming/doc doesn’t make that conservatism obvious, and future maintainers may “optimize” it incorrectly.

A short doc comment explaining the intended safety property (object identity stability between LHS evaluation and RHS) would prevent regressions.

• Maintainability | crates/oxc_minifier/src/peephole/minimize_logical_expression.rs:230-241
  The mutation check is only applied to the LHS assignment target (i.e., the member expression being assigned) via member_object_may_be_mutated(&assignment_expr.left, ctx). That’s aligned with the reported bug, but it leaves a potential blind spot if the logical-expression left operand is a member expression with a different base than the assignment target (even if has_no_side_effect_for_evaluation_same_target passes due to structural equivalence rules). If that helper allows some equivalence cases where bases are “effectively same target” but can still diverge after mutation (e.g., via aliasing), the guard might be too narrow.

Given this is a correctness fix, it’s worth double-checking the contract of has_no_side_effect_for_evaluation_same_target: does it guarantee the left operand and assignment target share the same base object identity in a way that makes checking only assignment_expr.left sufficient?

Summary of changes

What changed

✅ Minifier correctness guard

• Added a conservative bail-out in minimize_logical_expression.rs to avoid transforming patterns like x.y || (x = {}, x.y = 3) into x.y ||= (x = {}, 3) when the base object of the member expression may be mutated before the RHS finishes.
• Implemented helper checks:
  • member_object_may_be_mutated() extracts the member-expression object from an AssignmentTarget.
  • expression_reference_may_be_mutated() recursively determines whether the object ultimately refers to a symbol that symbol_is_mutated() marks as mutated (or is otherwise conservatively treated as mutable).

🧪 Expanded regression coverage

• Added multiple new tests in minimize_conditions.rs covering:
  • Reassignment of the base object in RHS sequences.
  • Safe transformations when only the property chain is mutated (not the base binding).
  • Conservative no-transform cases when a symbol is mutated anywhere in scope (including inside functions).
  • Deep member chains like x.y.z.w.

Links:

• Regression: #16647

[sapphi-red]

Thanks!

[graphite-app]

Merge activity

• Dec 21, 2:09 AM UTC: sapphi-red added this pull request to the Graphite merge queue.
• Dec 21, 2:15 AM UTC: Merged by the Graphite merge queue.