Add numeric separator support in number literals#258
Merged
Conversation
- Accept underscores in decimal, hex, binary, octal, and exponent parts - Strip separators in the lexer and surface validation suggestions - Add language coverage for valid and invalid numeric separator forms
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds ES numeric separators ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
frostney
added
the
spec compliance
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/language/expressions/numeric-separators.js`:
- Around line 91-110: The tests in numeric-separators.js that call eval (e.g.,
the failing cases using eval("1e_10"), eval("1_000_"),
eval("0x_FF"/"0b_10"/"0o_77", eval("1__000"), eval("1_.5")) do not prove parser
syntax errors because eval is excluded at runtime; move these negative cases out
of runtime Jest tests and into parser-facing fixtures or the TestRunner path
that feeds source text to the parser (so the code is parsed rather than
executed). Replace the eval-based expect(() => eval(...)).toThrow() checks with
parser-level test files or harness invocations that assert a parse error for the
same literal inputs, and remove or adjust the runtime tests to avoid relying on
undefined eval.
In `@units/Goccia.Lexer.pas`:
- Around line 668-718: The fractional-part check currently only enters when
PeekNext is a digit, allowing inputs like "1._5" to be tokenized incorrectly;
update the logic around Peek/PeekNext in the fraction handling (the block that
uses Advance, Peek, PeekNext, HasSeparator) so that when a '.' is seen you also
detect and reject a numeric separator immediately following the dot (i.e., if
PeekNext = '_' then raise TGocciaLexerError with the same message/metadata and
SSuggestNumericSeparator), and if PeekNext is a digit proceed as before; ensure
the same numeric-separator validation used elsewhere (the existing
HasSeparator/Advance/check that follows underscores) applies to the fractional
loop so cases like "1._5" produce the error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2bfbe1e-842c-4edb-9e25-7f18f9ff49a7
📒 Files selected for processing (5)
AGENTS.mddocs/language-restrictions.mdtests/language/expressions/numeric-separators.jsunits/Goccia.Error.Suggestions.pasunits/Goccia.Lexer.pas
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results306 benchmarks Interpreted: 🟢 176 improved · 🔴 85 regressed · 45 unchanged · avg +3.3% arraybuffer.js — Interp: 🟢 2, 🔴 10, 2 unch. · avg -2.4% · Bytecode: 🟢 2, 🔴 2, 10 unch. · avg -0.0%
arrays.js — Interp: 🟢 1, 🔴 14, 4 unch. · avg -2.5% · Bytecode: 🟢 9, 🔴 4, 6 unch. · avg +0.6%
async-await.js — Interp: 🔴 6 · avg -4.3% · Bytecode: 🟢 1, 🔴 2, 3 unch. · avg -0.3%
base64.js — Interp: 🔴 9, 1 unch. · avg -6.9% · Bytecode: 🟢 3, 🔴 2, 5 unch. · avg +0.1%
classes.js — Interp: 🟢 14, 🔴 10, 7 unch. · avg +1.5% · Bytecode: 🟢 9, 🔴 1, 21 unch. · avg +1.7%
closures.js — Interp: 🟢 8, 🔴 3 · avg +1.9% · Bytecode: 🟢 8, 3 unch. · avg +3.1%
collections.js — Interp: 🟢 3, 🔴 9 · avg -1.1% · Bytecode: 🟢 8, 🔴 2, 2 unch. · avg +1.7%
destructuring.js — Interp: 🟢 20, 🔴 1, 1 unch. · avg +7.2% · Bytecode: 🟢 13, 🔴 1, 8 unch. · avg +3.4%
fibonacci.js — Interp: 🟢 3, 🔴 3, 2 unch. · avg -0.5% · Bytecode: 🟢 3, 🔴 1, 4 unch. · avg +1.4%
for-of.js — Interp: 🟢 6, 1 unch. · avg +5.4% · Bytecode: 🟢 5, 2 unch. · avg +2.7%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 35, 7 unch. · avg +7.8% · Bytecode: 🟢 32, 🔴 1, 9 unch. · avg +5.0%
json.js — Interp: 🟢 4, 🔴 10, 6 unch. · avg -1.5% · Bytecode: 🟢 14, 6 unch. · avg +3.5%
jsx.jsx — Interp: 🟢 21 · avg +7.5% · Bytecode: 🟢 20, 1 unch. · avg +3.4%
modules.js — Interp: 🟢 2, 🔴 3, 4 unch. · avg -0.3% · Bytecode: 🟢 1, 🔴 2, 6 unch. · avg -0.4%
numbers.js — Interp: 🟢 5, 🔴 4, 2 unch. · avg +1.9% · Bytecode: 🟢 9, 2 unch. · avg +7.9%
objects.js — Interp: 🟢 6, 1 unch. · avg +13.5% · Bytecode: 🟢 4, 3 unch. · avg +2.0%
promises.js — Interp: 🟢 12 · avg +9.9% · Bytecode: 🟢 1, 🔴 3, 8 unch. · avg -0.9%
regexp.js — Interp: 🟢 5, 🔴 2, 4 unch. · avg +2.4% · Bytecode: 🟢 4, 🔴 3, 4 unch. · avg +0.9%
strings.js — Interp: 🟢 9, 2 unch. · avg +4.2% · Bytecode: 🟢 3, 8 unch. · avg +0.8%
typed-arrays.js — Interp: 🟢 20, 🔴 1, 1 unch. · avg +7.7% · Bytecode: 🟢 4, 🔴 6, 12 unch. · avg +1.4%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
eval is excluded in GocciaScript, so the tests passed because eval is undefined (ReferenceError), not because the invalid literals were rejected. Invalid separator placement is a lex-time error that cannot be exercised from within JS — document the manual verification commands instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/language/expressions/numeric-separators.js`:
- Around line 77-79: The test case using the numeric literal 0_1 inside
test("separator with zero prefix decimal") is invalid under ES2021 and must be
removed from positive runtime tests; delete the expect(0_1).toBe(1) assertion
and instead add or move a lex-time rejection test that verifies the parser
rejects the token 0_1 (e.g., in the lex/parse-time failure suite), so leave
test("separator with zero prefix decimal") either removed or rewritten to assert
a syntax/lex error for the literal 0_1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fed341a1-eff9-4b2c-b701-d75a36c51f16
📒 Files selected for processing (1)
tests/language/expressions/numeric-separators.js
1._5 is a syntax error because the dot is parsed as part of the numeric literal, not as property access. Add an explicit check for underscore after the decimal point so the lexer rejects it instead of silently splitting the token. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ES2021 §12.9.3 defines 0 as a standalone DecimalIntegerLiteral production — numeric separators are not permitted after it. Reject 0_1 at lex time instead of treating it as decimal 1. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Invalid separator placement is a lex-time error that cannot be tested from within JS. Add a dedicated "Check numeric separator rejection" step to both ci.yml and pr.yml that pipes each invalid pattern through ScriptLoader and asserts exit code 1 with the expected error message. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
units/Goccia.Lexer.pas (1)
593-594: Use the full repo-required spec annotation format.
// ES2021 §12.9.3 NumericLiteralis only a section label; it does not include the exact spec method name and parameter list the repo standard requires, so it is harder to audit against future spec-aligned work.As per coding guidelines, "Apply ECMAScript spec annotations in the format
// ESYYYY §X.Y.Z SpecMethodName(specParams)immediately above each function body implementing spec-defined behavior, with the method name and parameter list matching the spec exactly."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Lexer.pas` around lines 593 - 594, Replace the terse section label above the TGocciaLexer.ScanNumber procedure with the repo-required full ECMAScript spec annotation format: include the year, section, the exact spec method name and parameter list that corresponds to numeric literal scanning (e.g., use the spec method that defines NumericLiteral parsing), so update the comment immediately above procedure TGocciaLexer.ScanNumber to follow "// ESYYYY §X.Y.Z SpecMethodName(specParams)" exactly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@units/Goccia.Lexer.pas`:
- Around line 593-594: Replace the terse section label above the
TGocciaLexer.ScanNumber procedure with the repo-required full ECMAScript spec
annotation format: include the year, section, the exact spec method name and
parameter list that corresponds to numeric literal scanning (e.g., use the spec
method that defines NumericLiteral parsing), so update the comment immediately
above procedure TGocciaLexer.ScanNumber to follow "// ESYYYY §X.Y.Z
SpecMethodName(specParams)" exactly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 021bdf93-1b7e-49de-83ff-af829bbb6cc3
📒 Files selected for processing (2)
tests/language/expressions/numeric-separators.jsunits/Goccia.Lexer.pas
- Refresh lexer annotation for `NumericLiteral` - Keeps spec cross-reference aligned with current ECMAScript edition
This was referenced Apr 29, 2026
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
Testing
./build.pas testrunner && ./build/TestRunner teststests/language/expressions/numeric-separators.js.