fix(parser): don't pop from lexer errors to preserve error after rewind#13460
fix(parser): don't pop from lexer errors to preserve error after rewind#13460ulrichstark wants to merge 2 commits intooxc-project:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where lexer errors were being lost after parser rewind operations by changing from popping errors to referencing the last error. This ensures error messages are preserved and available even after the parser backtracks, leading to better error reporting.
- Changes error handling from destructive
pop()to non-destructivelast()access - Adds error cloning to maintain ownership semantics
- Preserves lexer errors for better diagnostic reporting after parser rewind operations
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
CodSpeed Instrumentation Performance ReportMerging #13460 will not alter performanceComparing Summary
|
Boshen
left a comment
There was a problem hiding this comment.
This is producing two identical errors:
e.g.:
× Unterminated string
╭─[test262/test/language/types/string/S8.4_A14_T1.js:16:11]
15 │
16 │ var str = ";
· ───
╰────
× Unterminated string
╭─[test262/test/language/types/string/S8.4_A14_T1.js:16:11]
15 │
16 │ var str = ";
· ───
╰────
Ok thanks! You are right. I missed that. My first approach to fix that issue was to clone all errors when creating a checkpoint and then re-assign them when rewinding instead of storing the count of errors in the snapshot. This way the expected behaviour stays the same and the error is preserved after rewinding even if it was popped from the Vec. The only disadvantage is that it has a slightly worse performance and memory footprint. Are you okay with cloning the lexer errors into the checkpoint or does it make more sense to figure out how to handle the duplicate errors? |
This is fixed by #13465. |
|
@ulrichstark I'm not sure, gut feeling is |
|
Closing this PR in favour of #13494, because the other PR is a cleaner fix and the performance impact is unexpectedly neutral instead of negative and even slightly positive on some benchmarks. |
Closes #12816. Alternative to #13460. This PR is the slightly cleaner way to fix the issue compared to the other PR, but comes at a potential performance cost because now instead of remembering how many errors there were, we are storing a clone of all errors and re-assign them when rewinding. This makes it possible to fix the issue because now even errors before a checkpoint, that were popped/removed after the checkpoint, are properly rewinded.
Closes #12816 and leads to more and better error messages as you can see in the updated coverage snapshots.
I'm not sure why the size of antd.js suddenly changed and I had to update the allocation snaphots as a result.