Ensure deep cloned AST nodes have synthetic positions #1174
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that deep cloned AST nodes receive synthetic position ranges to avoid unintended propagation of source locations, fixing issue #1092. Key changes include:
- Adding a new test case to catch declaration emit crashes when comments are copied.
- Updating baseline outputs for various compiler submodules to reflect the new synthetic positions.
- Modifying the deep clone implementation to explicitly set node locations as synthetic.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| testdata/tests/cases/compiler/declarationEmitNoCrashOnCommentCopiedFromOtherFile.ts | Added test case for ensuring no crash on comment copying. |
| testdata/baselines/reference/** | Updated baseline outputs reflecting synthetic node positions. |
| internal/ast/deepclone.go | Modified deep clone logic to set synthetic locations on cloned nodes. |
internal/ast/deepclone.go
Outdated
| // In strada, `factory.cloneNode` was dynamic and did _not_ clone positions for any "special cases", meanwhile | ||
| // Node.Clone in corsa reliably uses `Update` calls for all nodes and so copies locations by default. | ||
| // Deep clones are done to copy a node across files, so here, we explicitly make the location range synthetic on all cloned nodes | ||
| copy.Loc = core.NewTextRange(-1, -1) |
There was a problem hiding this comment.
Honestly, looking at this made me realize there's probably a latent bug in strada, too. While strada's cloneNode does strip locations from Identifier nodes (so won't trigger a crash in this scenario), it does copy location information on anything that falls through to the reflective copy case.
Which seems wrong. I, at least, have always used cloneNode to copy a node to copy a node for another location/a new file, and didn't realize that non-identifier node kinds retained a real .pos and .end after a clone... (see the comment in setTextRange in the checker immortalizing my thinking!)
|
|
||
| function f([, a, , b, , , , s, , , ] = results) { | ||
| ->f : ([, a, , b, , , , s, , ,]?: string[]) => void | ||
| +>f : ([, a, , b, , , , s, , ]?: string[]) => void |
There was a problem hiding this comment.
This is odd, what's the deal?
There was a problem hiding this comment.
Ah, it's mainly just losing the trailing comma, then.
There was a problem hiding this comment.
trailing comma preservation is done by looking at positions
Fixes #1092