Skip to content

Conversation

gabritto
Copy link
Member

Fixes an out of bounds crash found when compiling an internal project.

We weren't setting (-1, -1) as a position for non-leaf nodes when deep cloning them. As a result, we'd later use those positions to access a file's text that wasn't the file for the cloned node, so the positions were out of the file text's bounds.

The diff for some compiler tests increased because we now seem to omit a comma during emit, but I think this is a problem that can be addressed later.

@gabritto gabritto requested review from Copilot and weswigham and removed request for Copilot August 19, 2025 20:17
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to copy the hasTrailingComma node list member?

@gabritto
Copy link
Member Author

Do we need to copy the hasTrailingComma node list member?

Uh, right, that's what's causing the diffs. That's a method now in Corsa, and it skips synthetic nodes and checks positions to determine the result. We could set up positions when cloning to preserve this (maybe set end to -2 on the last node of the list if it had a trailing comma?), I think. I'll try doing that and see if it works.

@Copilot Copilot AI review requested due to automatic review settings August 19, 2025 21:13
Copy link
Contributor

@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 fixes an out-of-bounds crash that occurred when compiling an internal project by ensuring that synthetic positions (-1, -1) are properly set when deep cloning AST nodes. The crash was happening because cloned nodes weren't getting synthetic positions, leading to attempts to access file text at invalid positions.

Key changes:

  • Modified deep clone visitor to set synthetic positions for all cloned nodes and lists
  • Updated HasTrailingComma() logic to properly handle synthetic positions
  • Added a test case to prevent regression of this cross-file node scenario

Reviewed Changes

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

Show a summary per file
File Description
testdata/tests/cases/compiler/declarationEmitNoCrashOnCrossFileNode.ts New test case demonstrating the crash scenario with cross-file imports and destructuring parameters
internal/ast/deepclone.go Enhanced deep clone visitor to set synthetic positions (-1, -1) for all cloned nodes and lists when syntheticLocation flag is true
internal/ast/ast.go Simplified HasTrailingComma() logic by removing synthetic position checks that were causing issues
testdata/baselines/reference/submodule/compiler/*.diff Test baseline updates showing changes in destructuring parameter comma handling
testdata/baselines/reference/compiler/declarationEmitNoCrashOnCrossFileNode.* New test baselines for the crash prevention test

@gabritto gabritto requested a review from weswigham August 19, 2025 21:14
@gabritto gabritto added this pull request to the merge queue Aug 20, 2025
Merged via the queue into main with commit 3f675f1 Aug 20, 2025
22 checks passed
@gabritto gabritto deleted the gabritto/fixdeepclone branch August 20, 2025 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants