src: fix crash in GetErrorSource() for invalid using syntax#62770
src: fix crash in GetErrorSource() for invalid using syntax#62770semimikoh wants to merge 2 commits intonodejs:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62770 +/- ##
==========================================
- Coverage 91.55% 89.66% -1.89%
==========================================
Files 355 706 +351
Lines 149381 218268 +68887
Branches 23364 41778 +18414
==========================================
+ Hits 136765 195716 +58951
- Misses 12354 14460 +2106
- Partials 262 8092 +7830
🚀 New features to boost your workflow:
|
| 'async function f() { { await using x=null, []=null; } }', | ||
| 'async function f() { { await using x=null, {}=null; } }', |
There was a problem hiding this comment.
| 'async function f() { { await using x=null, []=null; } }', | |
| 'async function f() { { await using x=null, {}=null; } }', | |
| 'async function f() { await using x=null, []=null; }', | |
| 'async function f() { await using x=null, {}=null; }', |
or even
| 'async function f() { { await using x=null, []=null; } }', | |
| 'async function f() { { await using x=null, {}=null; } }', | |
| '{ await using x=null, []=null; }', | |
| '{ await using x=null, {}=null; }', |
There was a problem hiding this comment.
V8 doesn't guarantee that these will behave in an ordered way, so this looks like a worthwhile change.
However, the test case is probably a genuine V8 parser bug (the message's end_position ends up being one less than its start_position, hence the previous assertion failure), so I'm not sure that it's worthwhile hard-coding this as a Node.js test; the quirky behaviour it relies upon is liable to be patched upstream.
Agreed. This test case could be a clunky failure when V8 is upgraded. We can remove the test. |
| @@ -150,8 +150,7 @@ static std::string GetErrorSource(Isolate* isolate, | |||
| : 0; | |||
| int start = message->GetStartColumn(context).FromMaybe(0); | |||
| int end = message->GetEndColumn(context).FromMaybe(0); | |||
| if (start >= script_start) { | |||
| CHECK_GE(end, start); | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
Thanks for the reviews! |
Fixes: #62767
V8 can report an invalid source range for some malformed
usingandawait usingdeclarations, with the end column preceding the startcolumn.
GetErrorSource()currently asserts that the end column isgreater than or equal to the start column before reaching the existing
bounds check, so formatting the SyntaxError aborts the process.
Remove the assertion and only apply the script start offset when both
columns can be adjusted. Invalid ranges now fall through to the existing
fallback path, which prints the error source without an underline instead
of crashing.
Add regression coverage for the four invalid
using/await usingcases reported from test262.
Refs: https://github.com/tc39/test262
Verification