Separate parser token inspection from consumption

[mununki]

Resolves: #8629

This PR separates token inspection from consumption, simplifying parser state management and allowing Diamond mode and the separate prev_end_pos field to be removed.

Correct rollback also uncovered invalid fixtures that previously passed because diagnostics were suppressed. Local benchmarks show roughly 5% lower parsing time and 2.9% less memory allocated.

Previously, make scanned the first token immediately, and next scanned its successor. Inspecting the following token required temporarily advancing the parser through lookahead:

let parser = Parser.make "let x = 1" "Example.res"
let token = parser.token
let following =
  Parser.lookahead parser (fun parser ->
    Parser.next parser;
    parser.token)

let () = Parser.next parser
let consumed_end = parser.prev_end_pos

Now, make does not scan tokens. peek and peek2 read and cache tokens without advancing the consumed position. next consumes the current token without scanning its successor:

let parser = Parser.make "let x = 1" "Example.res"
let token = Parser.peek parser
let following = Parser.peek2 parser

let () = Parser.next parser
let consumed_end = Parser.position parser

Main changes:

• Add lazy token inspection with a bounded two-token cache.
• Interpret angle-bracket operators in parser context, removing Diamond mode.
• Consolidate consumed source positions through Parser.position.
• Centralize speculative parsing and rollback, including diagnostics and error suppression.
• Improve shared delimiter recovery, preserve useful correction hints, and correct invalid fixtures.

Parsing time change compared with pre-refactor master (5357fe4ac), where negative means faster:

Input	Compiler parsing	Formatter parsing
Real sources, 239 files combined	−4.65%	−4.96%
Runtime	−3.99%	−3.83%
Belt	−3.32%	−3.52%
Nested generics (synthetic)	−8.43%	−8.52%
Operators (synthetic)	−10.13%	−10.32%

Measured locally with 500 paired runs per input and mode. Memory allocated for the combined real sources decreased by 2.92% in both modes. Measurements cover parsing only, excluding type checking, code generation, and formatted output.

Validation: make test, make test-syntax-roundtrip, and added cursor/recovery regression coverage.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-07T12:41:20.084844Z	b1102fa	Manual request

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1fb9db09a7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[mununki]

These fixtures previously passed because speculative lookahead left error reporting disabled. Quoting the fields makes these non-function external types valid while preserving the object-type AST the old parser produced.

[mununki]

Line 9 was already invalid, but this diagnostic was previously missing. The parser now reports the invalid opening parenthesis separately from the preceding line's error and suggests option<node<int>>.

[mununki]

The extra < is still reported as an error. Previously, << was read as a shift operator, so recovery split the type declaration into option and a malformed expression. Recovery now preserves the type structure as option<node<int>>.

[codecov]

Codecov Report

❌ Patch coverage is 98.25000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.91%. Comparing base (0b4bd0a) to head (b1102fa).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
compiler/syntax/src/res_parser.ml	96.36%	4 Missing ⚠️
tests/ounit_tests/ounit_parser_cursor_tests.ml	99.13%	2 Missing ⚠️
tests/ounit_tests/ounit_parser_recovery_tests.ml	97.67%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8633      +/-   ##
==========================================
+ Coverage   77.58%   77.91%   +0.33%     
==========================================
  Files         475      477       +2     
  Lines       64051    64609     +558     
==========================================
+ Hits        49693    50343     +650     
+ Misses      14358    14266      -92     

Files with missing lines	Coverage Δ
compiler/syntax/src/res_core.ml	91.85% <ø> (+0.25%)	⬆️
compiler/syntax/src/res_diagnostics.ml	80.43% <100.00%> (+2.65%)	⬆️
compiler/syntax/src/res_driver.ml	78.43% <ø> (ø)
compiler/syntax/src/res_grammar.ml	42.56% <100.00%> (+0.51%)	⬆️
compiler/syntax/src/res_scanner.ml	91.08% <100.00%> (+0.52%)	⬆️
...s/ounit_tests/ounit_constructor_arguments_tests.ml	86.66% <100.00%> (+0.36%)	⬆️
tests/ounit_tests/ounit_tests_main.ml	100.00% <ø> (ø)
tests/ounit_tests/ounit_parser_recovery_tests.ml	97.67% <97.67%> (ø)
tests/ounit_tests/ounit_parser_cursor_tests.ml	99.13% <99.13%> (ø)
compiler/syntax/src/res_parser.ml	94.95% <96.36%> (+1.73%)	⬆️

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[pkg-pr-new]

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@8633

@rescript/belt

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/belt@8633

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@8633

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@8633

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@8633

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@8633

@rescript/runtime

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/runtime@8633

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@8633

commit: b1102fa

[github-actions]

Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-8633

[mununki]

/codex review

[cknitt]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

rescript/tests/syntax_benchmarks/benchmark.ml

Lines 276 to 278 in 6b03ec6

	|> Seq.iteri (fun i json ->
	print_endline (if i == 0 then "[" else ",");
	print_string (Yojson.to_string json));

Emit an opening bracket for empty manifests

When --parse-manifest contains an empty JSON object, benchmarks and the flattened sequence are empty, so this callback never runs and the unconditional closing bracket produces only \n]. That is invalid JSON and breaks automation consuming the benchmark output; print [ independently of the first result or serialize the completed result list.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[mununki]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: b1102fa23d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[cristianoc]

Following up on the "invalid fixtures" point from the thread, with what the diff actually shows.

1. externalInlineRecordNonArrow.res is a breaking change, not an invalid fixture.

tests/syntax_tests/data/parsing/grammar/structure/externalInlineRecordNonArrow.res (and the printer/ copy) had its source rewritten from {x: int} to {"x": int}, with expected/ left untouched. That fixture was added by #8304 to pin exactly the unquoted form in a non-arrow external — it's the case the file is named after. On master it parses to < x: int >; on this branch it's an error, and the PR adds a unit test in ounit_constructor_arguments_tests.ml asserting that:

OUnit.assert_bool "unquoted fields are not inline records here"
  (parse "@val external defaults: {x: int} = \"defaults\"").invalid

So code that compiles today stops compiling, and the way it entered the PR was by editing the test input until it passed. Neither CHANGELOG line mentions it. This should either be reverted (rollback fix shouldn't change what's accepted) or split into its own PR with a breaking-change entry and a rationale.

General rule for the rest of the PR: when a snapshot breaks, updating expected/ is a review question; editing the fixture source is deleting the test.

2. Error locations in typeConstructorArgs collapsed from ranges to points (2:21-24 → 2:21, uniformly across the file). Part of that is the intended "report at the opening" change, but the uniform collapse looks like a side effect of the prev_end_pos → Parser.position consolidation rather than a decision. Please check each one.

3. Tests. The split is roughly:

• ounit_parser_cursor_tests.ml — peek not advancing, lookahead restoring positions/comments/diagnostics, nested checkpoints. These are invariants of the new internal API with no snapshot surface. Keep.
• ounit_parser_recovery_tests.ml — this is source → parsetree + diagnostics, i.e. the syntax fixture suite reimplemented in OCaml via assert_recovery, and it overlaps the new delimitedLists.res fixture added alongside it. The combinatorial table (4 delimiter pairs × 3 contexts) is the only thing fixtures can't express; the rest should be fixtures or dropped.
• tests/tests/src/angle_operators_test.res — redundant with the parse fixture; if the tree is right the shifts evaluate right.

The core refactor (lazy two-token cache, peek/peek2, removing Diamond mode and prev_end_pos) looks like the right shape. The issue is a behaviour change riding inside it.

[cristianoc]

Follow-up after an adversarial pass: whole-fixture-corpus differential of base vs PR res_cli (3,165 file×mode runs), plus targeted probes of rollback, Diamond removal, the two-token cache and diagnostics. The cursor machinery holds up — no case where a token, comment, diagnostic or position diverges from base, and the angle-bracket cases (>>=, t<'a>=int, option<option<int>>=?, JSX, poly-variant bounds, coercions) are all identical.

Apology for my previous comment on externalInlineRecordNonArrow. I said the fixture pinned a deliberate feature. It didn't: base accepted external d: {x: int} by accident. #8304's is_external_bare_arrow_type lookahead speculatively parses {x: int}, hits the forbidden-inline-record Parser.err, which flips the shared mutable region ref to Silent; lookahead restored diagnostics but not the region, so the real parse's error was swallowed and the snapshot recorded the accident. The same input errors everywhere else on both base and PR. So this PR restores the intended error — that's a bug fix, not a breaking change, and I was wrong to frame it that way.

Three things before merge:

1. Fixes go in the CHANGELOG, with fixtures. The region leak is real and reproducible: let a = 1\n@attr(\nlet b = 2 compiles on master with no diagnostic (emits @attr(let b = 2) %rescript.exprhole); this PR reports the missing ). That, and the {x: int} external case, are the actual bugs being fixed — name them in the changelog and add each as an errors/ fixture (move the external one there rather than editing its source). The "duplicate deprecation warnings" line I could not reproduce in 14 placements of (. x) — base and PR both emit one warning; add the repro as a fixture or drop the claim.

2. Regression to fix if possible: let r = /. (unterminated regex) — base recovers to /./ at 1:11, PR recovers to // at 1:10, losing the dot. scan_regex now restarts at the opening slash but the unterminated branch returns ("", "") instead of what was scanned so far.

3. Trim the unit tests to the ones that test something specific and non-snapshot-visible (peek not advancing, checkpoint restoring positions/comments/diagnostics/regions, nested rollback). ounit_parser_recovery_tests.ml is source→AST+diagnostics, i.e. the fixture suite reimplemented; keep the combinatorial delimiter table if you think it earns its place, drop the rest in favour of fixtures.

Approving — the core change is right and the remaining items are bookkeeping.