Skip to content

fix(minifier): prevent incorrect ??= transformation when member base is mutated#17472

Merged
graphite-app[bot] merged 1 commit intomainfrom
copilot/add-tests-for-feature
Dec 30, 2025
Merged

fix(minifier): prevent incorrect ??= transformation when member base is mutated#17472
graphite-app[bot] merged 1 commit intomainfrom
copilot/add-tests-for-feature

Conversation

Copy link
Contributor

Copilot AI commented Dec 30, 2025

Extends the mutation check from #16802 to remove_unused_expression.rs. The transformation x.y != null || (x = {}, x.y = 3)x.y ??= (x = {}, 3) changes semantics when x is reassigned because ??= captures the reference before evaluating the RHS.

Changes

  • Add member_object_may_be_mutated() check before ??= transformation in nullish coalescing optimization
  • Transformation now correctly:
    • Allows x.y != null || (x = {}, x.y = 3)x.y ?? (x = {}, x.y = 3) (safe)
    • Blocks x.y != null || (x = {}, x.y = 3)x.y ??= (x = {}, 3) (unsafe)
    • Permits x.y != null || (foo(), x.y = 3)x.y ??= (foo(), 3) when no mutation

Example

var x = {};
x.y != null || (x = {}, x.y = 3)  // x becomes { y: 3 }
x.y ??= (x = {}, 3)                // x becomes {} (incorrect!)
x.y ?? (x = {}, x.y = 3)           // x becomes { y: 3 } (correct)
Original prompt

Add tests for #16802 (comment)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI changed the title [WIP] Add tests for discussed feature in PR 16802 fix(minifier): prevent incorrect ??= transformation when member base is mutated Dec 30, 2025
Copilot AI requested a review from sapphi-red December 30, 2025 03:00
@github-actions github-actions bot added A-minifier Area - Minifier C-bug Category - Bug labels Dec 30, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Dec 30, 2025

CodSpeed Performance Report

Merging #17472 will not alter performance

Comparing copilot/add-tests-for-feature (1e6ccda) with main (435e955)

Summary

✅ 38 untouched
⏩ 7 skipped1

Footnotes

  1. 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.

@sapphi-red sapphi-red marked this pull request as ready for review December 30, 2025 07:31
Copilot AI review requested due to automatic review settings December 30, 2025 07:31
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 prevents incorrect transformation of logical expressions to nullish coalescing assignment (??=) when the member expression's base object is mutated in the RHS. It extends the mutation detection from PR #16802 to the nullish coalescing optimization in remove_unused_expression.rs.

Key changes:

  • Added member_object_may_be_mutated() check before transforming to ??= operator
  • Added comprehensive test cases covering both safe and unsafe transformation scenarios
  • Ensures transformations fall back to safe ?? operator when mutation is detected

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

@sapphi-red sapphi-red requested a review from camc314 December 30, 2025 07:35
@sapphi-red sapphi-red assigned camc314 and unassigned sapphi-red Dec 30, 2025
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Dec 30, 2025
Copy link
Member

Boshen commented Dec 30, 2025

Merge activity

…is mutated (#17472)

Extends the mutation check from #16802 to `remove_unused_expression.rs`. The transformation `x.y != null || (x = {}, x.y = 3)` → `x.y ??= (x = {}, 3)` changes semantics when `x` is reassigned because `??=` captures the reference before evaluating the RHS.

### Changes

- Add `member_object_may_be_mutated()` check before `??=` transformation in nullish coalescing optimization
- Transformation now correctly:
  - Allows `x.y != null || (x = {}, x.y = 3)` → `x.y ?? (x = {}, x.y = 3)` (safe)
  - Blocks `x.y != null || (x = {}, x.y = 3)` → `x.y ??= (x = {}, 3)` (unsafe)
  - Permits `x.y != null || (foo(), x.y = 3)` → `x.y ??= (foo(), 3)` when no mutation

### Example

```js
var x = {};
x.y != null || (x = {}, x.y = 3)  // x becomes { y: 3 }
x.y ??= (x = {}, 3)                // x becomes {} (incorrect!)
x.y ?? (x = {}, x.y = 3)           // x becomes { y: 3 } (correct)
```

<!-- START COPILOT CODING AGENT SUFFIX -->

<!-- START COPILOT ORIGINAL PROMPT -->

<details>

<summary>Original prompt</summary>

> Add tests for #16802 (comment)

</details>

<!-- START COPILOT CODING AGENT TIPS -->
---

💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey).
@graphite-app graphite-app bot force-pushed the copilot/add-tests-for-feature branch from 1e6ccda to 68b2e54 Compare December 30, 2025 08:50
@graphite-app graphite-app bot merged commit 68b2e54 into main Dec 30, 2025
20 checks passed
@graphite-app graphite-app bot deleted the copilot/add-tests-for-feature branch December 30, 2025 08:57
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Dec 30, 2025
graphite-app bot pushed a commit that referenced this pull request Jan 5, 2026
### 🚀 Features

- 659c23e linter: Init note field boilerplate  (#17589) (Shrey Sudhir)
- 6870b64 parser: Add TS1363 error code (#17609) (Sysix)
- 23680a3 mangler: Skip mangling only in scopes affected by direct eval (#17612) (camc314)
- a7e1643 parser: Add TS2528 error code to duplicate_default_export diagnostic (#17558) (camc314)

### 🐛 Bug Fixes

- 1044116 ecmascript: Mark `new Symbol` as non side-effect free (#17568) (camc314)
- ab5e4ca isolated-declarations: Strip default values from rest parameter binding patterns (#17602) (camc314)
- 68b2e54 minifier: Prevent incorrect ??= transformation when member base is mutated (#17472) (copilot-swe-agent)

### ⚡ Performance

- 6067143 semantic: Remove hash when checking identifier (#17564) (camchenry)
- a28ab3d semantic: Avoid bounds check when checking string literal (#17545) (camc314)
- 04809d1 semantic: Use SIMD for finding backslashes in `check_string_literal` (#17534) (camchenry)
- 49ad2f0 semantic: Mark all diagnostic functions as `#[cold]` (#17487) (camc314)
- ea82b50 transformer: Mark all diagnostic functions as `#[cold]` (#17486) (camc314)
- d968e51 semantic: Mark `checker::check` as `inline(always)` (#17459) (camc314)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-minifier Area - Minifier C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants