fix(parser): make speculative paren-group probes template-aware#901
Merged
Conversation
The four speculative parenthesized-group probes — arrow-vs-paren
disambiguation (SkipExpressionWithLexicalGoal / SkipDestructuringPattern),
IsMatchExpressionAhead, and LooksLikeTraditionalForHeader — counted braces
flatly. A template `${...}` substitution's closing `}` is lexed as a plain
gttRightBrace under any non-template goal, so the probes miscounted it as a
structural brace and the following backtick started a runaway template/regex,
producing a spurious "Unterminated template literal" or "Unterminated regular
expression literal" parse error.
Route templates through SkipTemplateLiteral so the substitution closer is
re-lexed under the template-tail goal, and make its substitution scan
operand-aware so a `/` inside `${...}` is classified as division vs regex
exactly as the real parse would (otherwise a runaway regex swallowed the `}`).
Promote the duplicated TokenRequiresFollowingOperand to a shared method.
Pre-existing (fails on origin/main); independent of #896.
Add JS regression tests for arrow and destructuring defaults, match
discriminants, parenthesized expressions, and the compat-gated traditional-for
header, plus operand-awareness edge cases (regex/division in substitution,
nested templates, tagged-template defaults).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthrough
ChangesTemplate literal substitution brace safety in speculative parsers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.0)tests/language/expressions/string/template-interpolation-speculative-probes.jsFile contains syntax errors that prevent linting: Line 25: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 26: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 27: Expected a statement but instead found 'default: "other"'.; Line 33: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 34: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 35: Expected a statement but instead found 'default: "no"'.; Line 41: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 42: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 43: Expected a statement but instead found 'default: "miss"'.; Line 49: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 50: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 51: Expected a statement but instead found 'default: "no"'. Comment |
coderabbitai
Bot
added
bug
Contributor
Suite TimingTest Runner (interpreted: 11,024 passed; bytecode: 11,024 passed)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Test runner worker shutdown frees thread-local heaps in bulk; that shutdown reclamation is not counted as GC collections or collected objects.
Benchmarks (interpreted: 436; bytecode: 436)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Benchmark runner performs explicit between-file collections, so collection and collected-object counts can be much higher than the test runner.
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results436 benchmarks · PR vs same-runner Interpreted: 🟢 16 improved · 🔴 27 regressed · 393 unchanged · avg -0.2% Typical per-run noise (median variance): interpreted ±3.3%, bytecode ±2.8%. Deltas within noise overlap and read as unchanged. arraybuffer.js — Interp: 🔴 1, 13 unch. · avg +0.2% · Bytecode: 🔴 1, 13 unch. · avg -0.0%
arrays.js — Interp: 🔴 1, 18 unch. · avg -1.3% · Bytecode: 🟢 1, 🔴 1, 17 unch. · avg +0.9%
async-await.js — Interp: 6 unch. · avg +1.6% · Bytecode: 6 unch. · avg +2.7%
async-generators.js — Interp: 2 unch. · avg -10.0% · Bytecode: 🔴 1, 1 unch. · avg -1.8%
atomics.js — Interp: 6 unch. · avg -2.8% · Bytecode: 🟢 1, 🔴 2, 3 unch. · avg -0.7%
base64.js — Interp: 🟢 1, 9 unch. · avg +1.0% · Bytecode: 10 unch. · avg +1.6%
classes.js — Interp: 🟢 2, 🔴 1, 28 unch. · avg +0.4% · Bytecode: 🟢 2, 🔴 3, 26 unch. · avg +0.5%
closures.js — Interp: 11 unch. · avg -1.6% · Bytecode: 🔴 1, 10 unch. · avg -1.8%
collections.js — Interp: 🔴 2, 10 unch. · avg -0.3% · Bytecode: 12 unch. · avg -1.2%
csv.js — Interp: 🔴 1, 12 unch. · avg -1.4% · Bytecode: 🔴 1, 12 unch. · avg +1.7%
destructuring.js — Interp: 🔴 1, 21 unch. · avg -0.1% · Bytecode: 🔴 3, 19 unch. · avg -0.7%
fibonacci.js — Interp: 🔴 2, 6 unch. · avg +0.1% · Bytecode: 🟢 1, 7 unch. · avg -1.2%
float16array.js — Interp: 🟢 1, 31 unch. · avg -0.2% · Bytecode: 🟢 8, 24 unch. · avg +5.0%
for-in/for-in.js — Interp: 3 unch. · avg +3.1% · Bytecode: 3 unch. · avg -4.9%
for-of.js — Interp: 🔴 1, 6 unch. · avg -1.5% · Bytecode: 🔴 3, 4 unch. · avg -6.6%
generators.js — Interp: 4 unch. · avg +4.9% · Bytecode: 4 unch. · avg +0.8%
intl.js — Interp: 🔴 1, 5 unch. · avg -2.0% · Bytecode: 🟢 1, 5 unch. · avg +2.4%
iterators.js — Interp: 🔴 1, 41 unch. · avg -0.5% · Bytecode: 🟢 3, 🔴 2, 37 unch. · avg +0.8%
json.js — Interp: 🟢 3, 🔴 1, 19 unch. · avg +1.2% · Bytecode: 🔴 4, 19 unch. · avg -1.8%
jsx.jsx — Interp: 21 unch. · avg +0.4% · Bytecode: 🔴 2, 19 unch. · avg -1.8%
modules.js — Interp: 9 unch. · avg -0.5% · Bytecode: 9 unch. · avg -0.1%
numbers.js — Interp: 🟢 1, 10 unch. · avg +5.5% · Bytecode: 11 unch. · avg -1.1%
objects.js — Interp: 🔴 1, 6 unch. · avg +2.4% · Bytecode: 7 unch. · avg +4.6%
promises.js — Interp: 12 unch. · avg -3.5% · Bytecode: 12 unch. · avg +2.5%
property-access.js — Interp: 5 unch. · avg -3.7% · Bytecode: 5 unch. · avg -5.2%
regexp.js — Interp: 🟢 1, 10 unch. · avg -2.4% · Bytecode: 🔴 2, 9 unch. · avg -1.5%
strings.js — Interp: 19 unch. · avg -3.1% · Bytecode: 🔴 1, 18 unch. · avg -0.5%
temporal.js — Interp: 6 unch. · avg +1.0% · Bytecode: 🔴 2, 4 unch. · avg -3.2%
tsv.js — Interp: 🔴 1, 8 unch. · avg +0.3% · Bytecode: 🔴 1, 8 unch. · avg -0.3%
typed-arrays.js — Interp: 🟢 2, 🔴 6, 14 unch. · avg -5.7% · Bytecode: 🔴 11, 11 unch. · avg -12.2%
uint8array-encoding.js — Interp: 🟢 5, 🔴 2, 11 unch. · avg +14.7% · Bytecode: 🟢 3, 🔴 2, 13 unch. · avg +10.8%
weak-collections.js — Interp: 🔴 4, 11 unch. · avg -7.7% · Bytecode: 🟢 3, 🔴 4, 8 unch. · avg +1.9%
Deterministic profile diffDeterministic profile diff: no significant changes. Measured on ubuntu-latest x64. Each PR run also builds the |
Contributor
test262 Conformance
Areas closest to 100%
Per-test deltas (+2 / -0)Newly passing (2):
Steady-state failures are non-blocking; regressions vs the cached main baseline (lower total pass count, or any PASS → non-PASS transition) fail the conformance gate. Measured on ubuntu-latest x64, bytecode mode. Areas grouped by the first two test262 path components; minimum 25 attempted tests, areas already at 100% excluded. Δ vs main compares against the most recent cached |
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
1 participant
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.
Summary
`…${expr}…`) raised a spurious "Unterminated template literal" or "Unterminated regular expression literal" error when they appeared inside a speculatively-probed parenthesized group.${…}substitution's closing}is lexed as a plaingttRightBraceunder any non-template goal, so the probes miscounted it as a structural brace; the following backtick then started a runaway template/regex. Affected probes: arrow-vs-paren disambiguation (SkipExpressionWithLexicalGoal/SkipDestructuringPattern),IsMatchExpressionAhead, andLooksLikeTraditionalForHeader.SkipTemplateLiteral, which re-lexes the substitution closer under the template-tail goal (matching the parser-owned lexical-goal model indocs/architecture.md). Its substitution scan is now operand-aware so a/inside${…}is classified as division vs. regex exactly as the real parse would — otherwise a runaway regex swallowed the}(this was a second goal-sensitivity beyond the}itself, surfaced by thematch/arrow-with-division repros). The duplicatedTokenRequiresFollowingOperandis promoted to a shared method.Failing examples (all green after this change):
Constraints / non-goals: No change to the #896 goal-sensitivity contract — a template-tail
}is goal-sensitive, so these probes still drop their cached tokens and let the real parse re-lex the template (this PR fixes the brace-counting within the probe scan). Pre-existing bug; reproduces onorigin/main, independent of #896.Testing
Full suite 11024/11024 passing in both modes:
New regression tests (22 tests) cover arrow/destructuring defaults, match discriminants, parenthesized expressions, and the compat-gated traditional-
forheader, plus operand-awareness edge cases (regex- and division-in-substitution, nested templates, tagged-template defaults). Documentation unchanged: this fixes parser behavior to match the existing lexical-goal model rather than introducing new surface. No AST/scope/evaluator/value-type changes, so native Pascal tests N/A; no expected benchmark impact.🤖 Generated with Claude Code