Add a column offset to JSEvalOptions - #1656
Open
andreasrosdal wants to merge 3 commits into
Open
Conversation
An embedder that evaluates a fragment carved out of a larger document -- an
inline <script>, an event-handler attribute, a snippet extracted from a
bundle -- can already tell the engine which line the fragment starts on, but
not which column. The first line's columns therefore come out relative to
the fragment, and stack traces for it do not line up with the document the
user is looking at.
Add JSEvalOptions.col_num alongside line_num. js_parse_init() backs both
column origins (line_start, eol) up by col - 1, so the first line is
numbered from that column and every later line is unaffected -- both are
reset at the first line terminator. token.line_num also starts at `line`
rather than 1, which it should have all along.
This is the first use of the ABI-compatible extension the header describes,
so JS_EVAL_OPTIONS_VERSION goes to 2 and the version check accepts anything
in [1, JS_EVAL_OPTIONS_VERSION] instead of demanding the newest. A caller
built against version 1 keeps working with its shorter struct; col_num is
only read when the caller says version >= 2. Rejecting an unknown-but-higher
version stays as it was.
JSEvalOptions o = { .version = JS_EVAL_OPTIONS_VERSION,
.filename = "doc.html",
.line_num = 1, .col_num = 24 };
JS_EvalThis2(ctx, global, "nope()", 6, &o);
// at <eval> (doc.html:1:24)
Verified that a version-1 struct still evaluates and reports 1:1, that
col_num = 1 is byte-for-byte the old behaviour, and that a fragment
containing a newline numbers its second line from column 1. tests.conf,
which includes parse-error-column.js and global-reference-column.js, is
unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014mv33YvfHz7t9mmkituBnn
Covers the new field end to end: the unset/1/explicit-offset cases against both a runtime and a parse error, that only the first line is shifted, that it composes with line_num, and that a large offset survives. Also pins the version handling the field rides on: a version 1 caller has no col_num field, so a value in that position must not be read, and a version outside the supported range is still refused. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K6eRbuuuCujKgQkrHgvMrc
Column numbers are ints, and js_parse_init() derives them by subtracting a
line origin that col_num shifts backwards, so a col_num close to INT_MAX
overflowed and reported a negative column:
JSEvalOptions o = { .version = 2, .col_num = INT_MAX };
// at <eval> (f.js:1:-2147483647)
Take the offset only while every column of the source still fits an int,
and number from 1 otherwise, which is what a non-positive col_num already
did.
The api-test covers the boundary from both sides -- the largest offset
that fits is used as given, one past it is not -- along with negative
offsets, a module, a stack with more than one frame, a source whose first
line is empty, and JSON parsing, which shares the tokenizer.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An embedder that evaluates a fragment carved out of a larger document — an inline
<script>, an event-handler attribute, a snippet extracted from a bundle — can already tell the engine which line the fragment starts on, but not which column. The first line's columns therefore come out relative to the fragment, and stack traces for it do not line up with the document the user is looking at.Add
JSEvalOptions.col_numalongsideline_num.js_parse_init()backs both column origins (line_start,eol) up bycol - 1, so the first line is numbered from that column and every later line is unaffected — both are reset at the first line terminator.token.line_numalso starts atlinerather than1, which it should have all along.On the version bump
This is the first use of the ABI-compatible extension the header describes, so
JS_EVAL_OPTIONS_VERSIONgoes to 2. For that to actually be ABI-compatible the check has to stop demanding the newest version:A caller built against version 1 then keeps working with its shorter struct, and
col_numis only read when the caller saysversion >= 2. Rejecting an unknown-but-higher version stays as it was. Happy to drop the bump and putcol_numin version 1 instead if you would rather not commit to that policy yet — it changes only these few lines.Testing
Verified with a small C harness that a version-1 struct still evaluates and reports
1:1, thatcol_num = 1is byte-for-byte the old behaviour, and that a fragment containing a newline numbers its second line from column 1:tests.conf— which includesparse-error-column.jsandglobal-reference-column.js— is unchanged, andapi-testpasses.These are 16 PRs from Nordstjernen web browser:
https://github.com/nordstjernen-web/nordstjernen-browser
test262 conformance (the real metric):
┌────────────────────┬────────────────┬─────────────┐
│ │ Known failures │ Conformance │
├────────────────────┼────────────────┼─────────────┤
│ Before │ 52 │ ~99.93% │
├────────────────────┼────────────────┼─────────────┤
│ After all 16 merge │ 18 │ ~99.98% │
└────────────────────┴────────────────┴─────────────┘