Skip to content

Conversation

@kdy1
Copy link
Member

@kdy1 kdy1 commented Dec 18, 2025

Summary

Fixes #11375

The nullish coalescing operator (??) was missing the var declaration for the temporary variable when used in functions with spread parameters.

Root Cause

The bug was in the exit_stmt function in the nullish coalescing transformer. When exiting a statement, it was incorrectly restoring the statement pointer:

  • It popped the current statement from the stack
  • Then set stmt_ptr to the popped value (the statement we just exited)

This meant stmt_ptr was pointing to the wrong statement when trying to inject variable declarations for nested expressions.

The Fix

Changed the exit_stmt function to correctly restore the parent statement pointer by using last().copied() on the stack after popping.

Before

// Missing var declaration
(_props_children = props.children) !== null && _props_children !== void 0 ? _props_children : props.label

After

var _props_children;
(_props_children = props.children) !== null && _props_children !== void 0 ? _props_children : props.label

Test plan

🤖 Generated with Claude Code

kdy1 and others added 3 commits December 18, 2025 09:13
…g with spreads

The exit_stmt function was incorrectly restoring the statement pointer
when exiting a statement. It was popping the current statement and then
setting stmt_ptr to the popped value (the statement we just exited),
instead of restoring the parent statement pointer.

This caused the variable declaration for the temporary variable used in
nullish coalescing to not be injected when the operator was used inside
nested expressions.

Fixed #11375

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 (1M context) <[email protected]>
Copilot AI review requested due to automatic review settings December 18, 2025 01:07
@kdy1 kdy1 requested a review from a team as a code owner December 18, 2025 01:07
@changeset-bot
Copy link

changeset-bot bot commented Dec 18, 2025

🦋 Changeset detected

Latest commit: e9b9097

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@kdy1 kdy1 requested a review from a team as a code owner December 18, 2025 01:08
@kdy1 kdy1 enabled auto-merge (squash) December 18, 2025 01:08
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 where the nullish coalescing operator (??) was missing the var declaration for temporary variables when used in functions with spread parameters. The root cause was incorrect statement pointer restoration in the exit_stmt function.

  • Fixed the exit_stmt function to properly restore the parent statement pointer from the stack
  • Added a comprehensive test case demonstrating the fix with a real-world example using spread parameters and nullish coalescing

Reviewed changes

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

File Description
crates/swc_ecma_transformer/src/es2020/nullish_coalescing.rs Fixed exit_stmt to correctly restore parent statement pointer by using last().copied() after popping, instead of setting it to the popped value
crates/swc/tests/fixture/issues-11xxx/11375/input/index.tsx Added test input with function using spread parameters and nullish coalescing operator
crates/swc/tests/fixture/issues-11xxx/11375/input/.swcrc Added test configuration targeting ES5 with TypeScript and JSX support
crates/swc/tests/fixture/issues-11xxx/11375/output/index.tsx Added expected output showing correct var declaration for the temporary variable

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

@claude
Copy link
Contributor

claude bot commented Dec 18, 2025

Code Review

Summary

This PR fixes a critical bug in the nullish coalescing transformer where temporary variable declarations were missing when the operator was used inside functions with spread parameters. The fix correctly restores the statement pointer stack management.


Code Quality: Excellent

The Fix (lines 82-85 in nullish_coalescing.rs)
The change is minimal, surgical, and correct:

fn exit_stmt(&mut self, _stmt: &mut Stmt, _ctx: &mut TraverseCtx) {
    self.stmt_ptr_stack.pop();
    self.stmt_ptr = self.stmt_ptr_stack.last().copied();
}

Why this is correct:

  • The previous implementation was setting stmt_ptr to the current statement being exited instead of the parent statement
  • This caused variable declarations to be inserted at the wrong location in nested contexts
  • The fix properly restores the parent statement pointer by using last().copied() after popping

Pattern Consistency:
This implementation now matches the identical pattern used in other transformers:

  • es2021/logical_assignment_operators.rs:110-113
  • es2018/object_rest_spread.rs:115-118

This consistency is excellent and reduces the risk of similar bugs.


Bug Fix Validation

Root Cause Analysis: Accurate and well-explained in the PR description. The issue occurs when:

  1. A function has spread parameters (triggering object destructuring transform)
  2. Nullish coalescing is used inside nested expressions
  3. The wrong stmt_ptr causes the var declaration to be skipped

Test Coverage:


Performance Considerations

No performance concerns:

  • The fix only changes pointer assignment logic (O(1) operations)
  • Stack operations (pop, last) are already present, just reordered
  • Adheres to the project's "prefer performance" principle

Security Concerns

No security issues:

  • Does not introduce any unsafe operations
  • Properly manages the statement pointer stack lifecycle
  • No risk of use-after-free or memory issues

Best Practices & Conventions

Follows all CLAUDE.md rules:

  • ✅ Code is performant
  • ✅ Documentation is in English
  • ✅ No unstable/nightly features used
  • ✅ Changeset file properly added

Minor observations:

  1. The changeset file .changeset/eight-chicken-sneeze.md correctly marks this as a patch for both swc_core and swc_ecma_transformer
  2. The test file input/index.tsx is missing a newline at EOF, but this is cosmetic

🎯 Recommendations

1. Consider additional edge case tests (optional):
While the current test is good, consider adding tests for:

  • Multiple nested nullish coalescing operators
  • Nullish coalescing in arrow functions with spread params
  • Combinations with optional chaining

Example:

function foo({...props}) {
  return props.a ?? props.b ?? props.c;
}

2. Documentation improvement (optional):
Consider adding a comment in exit_stmt explaining why we need to restore the parent pointer:

fn exit_stmt(&mut self, _stmt: &mut Stmt, _ctx: &mut TraverseCtx) {
    // Pop current statement and restore parent statement pointer
    // so variable declarations are inserted at the correct nesting level
    self.stmt_ptr_stack.pop();
    self.stmt_ptr = self.stmt_ptr_stack.last().copied();
}

However, given that this pattern is used consistently across multiple transformers, perhaps a comment in just one location (or in shared documentation) would be sufficient.


🚀 Verdict: LGTM

This is a high-quality bug fix that:

  • ✅ Correctly identifies and fixes the root cause
  • ✅ Follows established patterns in the codebase
  • ✅ Includes proper test coverage
  • ✅ Has no performance or security concerns
  • ✅ Adheres to all project conventions

The PR is ready to merge.


Review completed by Claude Code

@claude
Copy link
Contributor

claude bot commented Dec 18, 2025

Code Review

Summary

This PR successfully fixes issue #11375 where the nullish coalescing operator () was missing the declaration for temporary variables when used in functions with spread parameters. The fix is correct, minimal, and follows established patterns in the codebase.


✅ Code Quality & Best Practices

The Fix (crates/swc_ecma_transformer/src/es2020/nullish_coalescing.rs:82-85)

The bug was in the exit_stmt function. The previous implementation incorrectly restored stmt_ptr:

// BEFORE (incorrect)
self.stmt_ptr = None;                      // Reset to None
self.stmt_ptr = self.stmt_ptr_stack.pop(); // Set to the just-exited statement

This caused stmt_ptr to point to the statement being exited rather than its parent, preventing proper variable declaration injection for nested expressions.

The fix correctly restores the parent statement pointer:

// AFTER (correct)
self.stmt_ptr_stack.pop();                           // Remove current statement
self.stmt_ptr = self.stmt_ptr_stack.last().copied(); // Set to parent statement

Pattern Consistency: This implementation matches the exact pattern used in other transformers:

  • es2018/object_rest_spread.rs:50-52
  • es2021/logical_assignment_operators.rs:61-63

This consistency is excellent for maintainability.


✅ Test Coverage

The test case added is appropriate and comprehensive:

The test directly addresses the reported bug and verifies the fix works correctly.


✅ Performance Considerations

No performance impact: The fix only changes the order of operations:

  • Same number of stack operations
  • Same number of assignments
  • More correct pointer management

The change from pop() returning a value to pop() + last().copied() is negligible since:

  • Both are O(1) operations on Vec
  • The copied() is trivial for raw pointers
  • The stack depth is typically shallow (number of nested statements)

✅ Security Concerns

No security issues identified:

  • Uses raw pointers (*const Stmt) which is consistent with existing code
  • Pointers are only used for comparison/identification, not dereferencing
  • The statement injector handles actual modifications safely
  • No unsafe code introduced

✅ Correctness & Edge Cases

Root Cause Analysis: The bug only manifested when:

  1. Nullish coalescing was used
  2. Inside a nested statement context (like function parameters with spreads)
  3. Requiring a temporary variable declaration

The fix correctly handles this by ensuring stmt_ptr always points to the current statement's parent, not the statement itself.

Potential Edge Cases (all appear handled correctly):

  • ✅ Multiple nested statements
  • ✅ Multiple nullish coalescing in same scope
  • ✅ Nullish coalescing without spread parameters
  • ✅ Empty statement stack (returns None via last().copied())

📝 Minor Observations

  1. Documentation: The code has excellent inline documentation explaining the transformation logic
  2. PR Description: Very clear explanation of root cause and fix with before/after examples
  3. Commit Message: Follows conventional commit format

✅ Recommendation: APPROVE

This is a high-quality fix that:

  • Correctly resolves the reported bug
  • Follows established patterns in the codebase
  • Includes appropriate test coverage
  • Has no performance or security concerns
  • Is minimal and focused (2-line change)

The fix aligns perfectly with the repository's conventions for writing performant, well-documented code.


Reviewed with Claude Code 🤖

@github-actions
Copy link
Contributor

Binary Sizes

File Size
swc.linux-x64-gnu.node 31M (31768776 bytes)

Commit: f508b20

@codspeed-hq
Copy link

codspeed-hq bot commented Dec 18, 2025

CodSpeed Performance Report

Merging #11377 will not alter performance

Comparing kdy1/fix-11375 (e9b9097) with main (78a5327)1

Summary

✅ 138 untouched

Footnotes

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

@kdy1 kdy1 disabled auto-merge December 18, 2025 01:20
@kdy1 kdy1 enabled auto-merge (squash) December 18, 2025 01:20
@kdy1 kdy1 disabled auto-merge December 18, 2025 01:21
@kdy1 kdy1 merged commit 686d154 into main Dec 18, 2025
186 checks passed
@kdy1 kdy1 deleted the kdy1/fix-11375 branch December 18, 2025 01:21
@kdy1 kdy1 modified the milestones: Planned, 1.15.6 Dec 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

nullish coalescing operator missing var declaration when using spreads

2 participants