Skip to content

Conversation

@delino
Copy link
Contributor

@delino delino bot commented Oct 23, 2025

Summary

This PR fixes the parser to correctly handle TypeScript non-null assertion operators (!) in destructuring assignment patterns, specifically in array element access expressions.

Problem

Previously, code like this would fail to parse:

[arrayCopy[currentIndex]!, arrayCopy[randomIndex]!] = [arrayCopy[randomIndex]!, arrayCopy[currentIndex]!];

The parser would generate a "Not a pattern" error (SyntaxError::InvalidPat), while the TypeScript compiler successfully parses this code.

Root Cause

The issue was in the reparse_expr_as_pat_inner function in crates/swc_ecma_lexer/src/common/parser/pat.rs. When checking if an expression can be used as an assignment pattern (PatType::AssignElement), the code listed specific expression types that are allowed, but Expr::TsNonNull was not in this list.

Solution

Added support for the following TypeScript expression types to the PatType::AssignElement match block:

  • Expr::TsNonNull (non-null assertion: expr!)
  • Expr::TsTypeAssertion (type assertion: <Type>expr)
  • Expr::TsInstantiation (type instantiation: expr<Type>)
  • Expr::TsSatisfies (satisfies expression: expr satisfies Type)

These TypeScript expression types are valid simple assignment targets when their inner expressions are valid. This aligns with how the ExprExt trait already correctly implements is_valid_simple_assignment_target for these expression types by delegating to the inner expression.

Testing

Added comprehensive test cases in crates/swc_ecma_parser/tests/typescript/issue-11142/ covering:

  • Array element swapping with non-null assertions (the original issue case)
  • Simple non-null assertions in array destructuring
  • Non-null assertions with member expressions
  • Non-null assertions in object destructuring
  • Complex nested cases with multiple non-null assertions

All existing tests continue to pass.

Related

Fixes #11142

🤖 Generated with Claude Code

…g assignment

This fixes the parser to correctly handle TypeScript non-null assertion operators (!)
in destructuring assignment patterns, specifically in array element access expressions.

Previously, code like:
```typescript
[arr[i]!, arr[j]!] = [arr[j]!, arr[i]!]
```

Would fail with "Not a pattern" error (SyntaxError::InvalidPat), while the TypeScript
compiler successfully parses this code.

The fix adds support for `Expr::TsNonNull`, `Expr::TsTypeAssertion`, `Expr::TsInstantiation`,
and `Expr::TsSatisfies` to the list of allowed expression types in the `PatType::AssignElement`
match block. These TypeScript expression types are valid simple assignment targets when their
inner expressions are valid.

This aligns with how the `ExprExt` trait already correctly implements
`is_valid_simple_assignment_target` for these expression types by delegating to the inner
expression.

Fixes #11142

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

Co-Authored-By: Claude <noreply@anthropic.com>
@changeset-bot
Copy link

changeset-bot bot commented Oct 23, 2025

⚠️ No Changeset found

Latest commit: 8bd0128

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Member

kdy1 commented Oct 23, 2025

🤖 This pull request has been linked to DevBird Task #870

View the task details and manage the automated development workflow in DevBird.

Copy link
Member

kdy1 commented Oct 23, 2025

📋 DevBird Task Prompt

Objective

Fix the SWC parser to correctly handle TypeScript non-null assertion operators (!) in destructuring assignment patterns, specifically in array element access expressions.

Problem Description

The SWC parser currently fails to parse code like:

[arrayCopy[currentIndex]!, arrayCopy[randomIndex]!] = [arrayCopy[randomIndex]!, arrayCopy[currentIndex]!]

It generates a "Not a pattern" error (SyntaxError::InvalidPat), while the TypeScript compiler successfully parses this code.

Root Cause

The issue is in the reparse_expr_as_pat_inner function in /home/runner/work/swc/swc/crates/swc_ecma_lexer/src/common/parser/pat.rs.

When checking if an expression can be used as an assignment pattern (PatType::AssignElement), the code lists specific expression types that are allowed (lines 131-154):

  • Expr::Array, Expr::Object
  • Expr::Member, Expr::SuperProp
  • Expr::Call, Expr::New
  • Expr::Lit, Expr::Ident
  • Expr::Fn, Expr::Class
  • Expr::Paren, Expr::Tpl
  • Expr::TsAs

However, Expr::TsNonNull is NOT in this list, so expressions with non-null assertions fall through to the default case at line 157 which emits the InvalidPat error.

Solution

Add Expr::TsNonNull to the list of allowed expression types in the PatType::AssignElement match block. This should be added alongside other TypeScript expression types like Expr::TsAs.

The fix should recognize that TypeScript non-null assertion expressions are valid simple assignment targets when their inner expression is valid. The ExprExt trait already correctly implements is_valid_simple_assignment_target for TsNonNullExpr by delegating to the inner expression (see /home/runner/work/swc/swc/crates/swc_ecma_lexer/src/common/parser/expr_ext.rs lines 66-72).

Implementation Details

  1. File to modify: /home/runner/work/swc/swc/crates/swc_ecma_lexer/src/common/parser/pat.rs

  2. Change location: In the reparse_expr_as_pat_inner function, around line 144, add Expr::TsNonNull(..) to the match arms that already include Expr::TsAs(..).

  3. Expected behavior: After the fix:

    • Non-null assertion expressions in destructuring assignments should be treated similarly to type assertions (TsAs)
    • The parser should validate that the inner expression is a valid simple assignment target
    • If valid, return the expression as a pattern (wrapped in Pat::Expr)

Documentation & References

TypeScript Non-Null Assertion Operator:

SWC AST Structure:

Related SWC Code:

  • ExprExt trait already handles TsNonNullExpr correctly in is_valid_simple_assignment_target
  • Other TypeScript expressions like TsAs, TsTypeAssertion, TsSatisfies, and TsInstantiation are already supported

Testing Requirements

  1. Create a test case for the reported issue in the TypeScript parser tests

    • Test file should contain the exact code from the issue: array element swapping with non-null assertions
    • Verify the parser successfully parses the code without errors
  2. Add additional test cases covering:

    • Simple non-null assertion in destructuring: [a!] = [b]
    • Object destructuring with non-null assertions: {x: obj.prop!} = {x: value}
    • Nested patterns with non-null assertions
    • Combined with other TypeScript features (type assertions, optional chaining, etc.)
  3. Verify existing tests still pass with cargo test

Success Criteria

  • The parser successfully parses destructuring assignments with non-null assertions
  • The code [arrayCopy[currentIndex]!, arrayCopy[randomIndex]!] = [arrayCopy[randomIndex]!, arrayCopy[currentIndex]!] parses without errors
  • All existing tests continue to pass
  • New test cases validate the fix

Project-Specific Guidelines

  • Follow the CLAUDE.md guidelines:
    • Write performant code (prefer performance)
    • Write comments and documentation in English
    • Do not use unstable/nightly Rust features
    • Write unit tests for the code
    • Do not remove or modify existing tests when fixing tests
    • Run cargo fmt --all before committing

GitHub Issue


This comment was automatically added by DevBird. You can disable this feature in DevBird Settings.

@claude
Copy link
Contributor

claude bot commented Oct 23, 2025

Claude finished @DeLino[bot]'s task —— View job


Code Review Complete ✅

I've thoroughly reviewed this pull request that fixes TypeScript non-null assertion in destructuring assignments. Here's my comprehensive feedback:

Review Checklist

  • Analyze parser code changes in pat.rs
  • Review test coverage and test cases
  • Check adherence to project guidelines
  • Assess code quality and potential issues
  • Evaluate performance and security considerations
  • Provide comprehensive feedback

Overall Assessment: ✅ LOOKS GOOD

This is a well-implemented fix that correctly addresses the parser issue while maintaining consistency with existing code patterns.

Code Quality & Best Practices ✅

Strengths:

  • Consistent with existing patterns: The fix follows the exact same pattern as other TypeScript expression types like TsAs
  • Comprehensive solution: Adds support for all relevant TS expression types (TsNonNull, TsTypeAssertion, TsInstantiation, TsSatisfies)
  • Proper delegation: Correctly relies on ExprExt::is_valid_simple_assignment_target for validation
  • Clean code: Simple, focused change that doesn't introduce complexity

Location: crates/swc_ecma_lexer/src/common/parser/pat.rs:144-148

Performance Considerations ✅

  • Minimal overhead: The change only adds pattern matching cases - no additional runtime cost
  • Efficient validation: Leverages existing is_valid_simple_assignment_target implementation
  • No regressions: Existing expressions continue to follow the same code paths

Security Considerations ✅

  • No security implications: Parser changes are safe and don't introduce vulnerabilities
  • Input validation maintained: All validation logic is preserved through the ExprExt trait

Test Coverage ✅

Excellent test coverage in crates/swc_ecma_parser/tests/typescript/issue-11142/:

Original issue case: Array element swapping with non-null assertions
Simple cases: Basic non-null assertions in destructuring
Member expressions: Non-null assertions with property access
Object destructuring: Non-null assertions in object patterns
Complex nested cases: Multiple levels of non-null assertions

The test file demonstrates the parser correctly produces TsNonNullExpression AST nodes in all the expected locations.

Technical Implementation ✅

The fix correctly addresses the root cause identified in the DevBird task:

  1. Problem: Expr::TsNonNull was missing from the PatType::AssignElement match block
  2. Solution: Added TsNonNull alongside other TS expression types
  3. Validation: Uses existing ExprExt::is_valid_simple_assignment_target which properly delegates to inner expressions

This aligns perfectly with how the ExprExt trait already handles these expressions in expr_ext.rs:66-72.

Adherence to Project Guidelines ✅

Follows all CLAUDE.md requirements:

  • ✅ Performant code (minimal changes, no performance impact)
  • ✅ Comprehensive unit tests included
  • ✅ No unstable Rust features used
  • ✅ Focused change that doesn't modify existing tests

Minor Suggestions

Documentation: Consider adding a brief comment explaining why these TS expression types are valid assignment targets:

// TypeScript expression types that wrap valid assignment targets
| Expr::TsAs(..)
| Expr::TsNonNull(..)        // Non-null assertion: expr!
| Expr::TsTypeAssertion(..)  // Type assertion: <Type>expr  
| Expr::TsInstantiation(..)  // Type instantiation: expr<Type>
| Expr::TsSatisfies(..) => { // Satisfies expression: expr satisfies Type

However, this is optional since the existing code doesn't document the other expression types either.

Changeset Status ⚠️

The changeset bot indicates no changeset was found. Since this is a bug fix for the parser, you'll likely want to add a changeset for proper version tracking.


Recommendation: ✅ APPROVE

This is a solid fix that correctly implements the missing functionality while maintaining code quality and following established patterns. The comprehensive test coverage gives confidence that the solution works correctly.


@codspeed-hq
Copy link

codspeed-hq bot commented Oct 23, 2025

CodSpeed Performance Report

Merging #11168 will not alter performance

Comparing autodev/fix-ts-non-null-in-destructuring (8bd0128) with main (146c77c)

Summary

✅ 140 untouched

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.

Non-null assertions fails to parse with Not a pattern

2 participants