fix(codegen): avoid sourcemap panic on U+2028/U+2029#19548
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
6ea895a to
f2146ad
Compare
There was a problem hiding this comment.
Pull request overview
Adds regression coverage in the sourcemap builder test suite for JavaScript line-separator characters U+2028 (LS) and U+2029 (PS), which are valid line terminators and previously triggered sourcemap-related panics.
Changes:
- Extend
add_source_mappingtests to include inputs containingU+2028/U+2029. - Validate generated line/column tracking for LS/PS alone and when surrounded by other characters.
Merging this PR will not alter performance
Comparing Footnotes
|
f2146ad to
cb59c95
Compare
There was a problem hiding this comment.
I've pushed a commit which reverts most of the changes and just fixes the actual bug with minimum of changes.
The advantage of not introducing a separate line_break_byte_len var is it that increases the chances that all the calculations can be done in registers, without having to spill onto the stack. The most important thing in this hot loop is that the fast paths for ASCII chars and \n remain as streamlined as possible. The code which handles irregular line breaks will rarely be hit, so it's fine to add 1 operation there.
This is a micro-optimization, but as the previous PR which touched this code demonstrated (#13903), this code is very perf-sensitive.
There was one other good change in this PR. I'll split that out into a separate perf PR, with some other small improvements I noticed while reviewing this.
|
@camc314 Please take a look at my changes, and merge if you're happy with it. |
camc314
left a comment
There was a problem hiding this comment.
Thanks! - looks good to me
Merge activity
|
cb59c95 to
e316694
Compare
Follow-on after #19548. Remove a bounds check from hot loop in `SourcemapBuilder::update_generated_line_and_column` by doing only 1 bounds check to get next 2 bytes, instead of 2 bounds checks for each byte. This optimization was taken from @cam314's original version of PR #19548. While reviewing that PR I also thought I'd spied 2 other optimizations, but on reflection, I realized they were better left as is. Add comments to explain why.
### 🚀 Features - e814049 oxc_data_structure/rope: Add `get_offset_from_line_and_column` (#18133) (Sysix) ### 🐛 Bug Fixes - 7958b56 linter: Fix syntax error reporting in some output formatters. (#19590) (connorshea) - e316694 codegen: Avoid sourcemap panic on `U+2028`/`U+2029` (#19548) (camc314) - 933ff72 semantic: Emit correct error code for reserved type name (#19545) (camc314) ### ⚡ Performance - b5fa195 codegen: Remove bounds check from `SourcemapBuilder` (#19578) (overlookmotel) Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>

Fixes rolldown/rolldown#8388.