Skip to content

Rewrite determinization as a Laurikari TDFA for correct capture semantics - #204

Merged
hhugo merged 10 commits into
ocaml-community:masterfrom
hhugo:pr4-tdfa-rewrite
Sep 28, 2026
Merged

hhugo merged 10 commits into
ocaml-community:masterfrom
hhugo:pr4-tdfa-rewrite

Conversation

@hhugo

@hhugo hhugo commented Sep 18, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes wrong sub-match positions for as bindings (#199) by rewriting determinization as a tagged DFA where each NFA path carries its own registers. Based on master (#203, the plumbing, is merged). Follow-ups stacked on this PR: #207 (register optimizations), #208 (no copy of the capture cells in mark/backtrack), #209 (TDFA(1)).

The bug. One flat tag list per transition, one register per tag shared by all live paths. When a tagged node is reachable from several paths at once, e.g. Star 'a', (('a', Plus 'b') as x), the loop path re-fires the write on every iteration and clobbers the position of the path already inside the capture: wrong, empty or out-of-bounds sub-matches.

The fix. A DFA state is an ordered list of configurations (NFA node, tag → register). Order is priority: the epsilon closure is a DFS along eps lists and the first path to a node wins, i.e. leftmost-greedy among the parses of the longest match. States are looked up modulo register renaming; reaching an existing state emits the moves realigning the registers, as a parallel move. An accepting state copies the accepting path's registers into the canonical cells read by the bindings, just before Sedlexing.mark. This is Laurikari's construction, a TDFA(0) in Trofimovich's terms. Patterns without captures generate byte-identical code.

Every tag write gets a fresh register and every tag is copied on accept: the simplest correct scheme, two cells per tag. #207 brings ordinary patterns back to one cell and no copy.

Commits

  1. Determinize over ordered configurations. Flips the loop_before_capture and capture_before_loop tests pinned in Pin known bugs with regression tests #200.
  2. Bounded repetition is greedy, like Star/Plus.
  3. Keep only nodes that matter in DFA states (no epsilon-only nodes): smaller keys, a few states merge.
  4. Simplify Seq lowering.
  5. Opt is greedy, so Opt p and Rep (p, 0 .. 1) agree.
  6. Plus is greedy when the body's first alternative is nullable (r, Star r).
  7. Codegen tests: counterparts of the as-binding tests whose positions cannot be computed statically.
  8. Remove a duplicate test.
  9. Changelog.
  10. Current/Goal notes of the codegen tests updated to the new cell counts.

opt_greedy and plus_nullable_first_alternative regress at 1 and are restored by 5 and 6. Every commit builds and passes dune runtest.

One change that is a decision, not a fix

rep_nullable_body (pinned in #200) flips: (Rep ((Opt 'd' | 'b'), 0 .. 1) as x), Star 'b' on "b" gave x="b", now gives x="". Both parses are maximal; under leftmost-greedy the body's first alternative, Opt 'd', matches empty and wins, exactly as for the spelled-out (Opt 'd' | 'b' | ""). All other flips replace captures that no parse can produce.

Verification

The #202 oracle, run locally against this tip (it needs small interpreter changes for Copy/accept, so it is not part of this PR): the 15 pinned disagreements clear, and 300k random cases agree.

🤖 Generated with Claude Code

@hhugo
hhugo changed the base branch from master to pr4a-tdfa-prep September 18, 2026 16:05
hhugo added a commit to hhugo/sedlex that referenced this pull request Sep 19, 2026
(cherry picked from commit abfb01d)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
hhugo added a commit to hhugo/sedlex that referenced this pull request Sep 19, 2026
(cherry picked from commit abfb01d)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
hhugo added a commit to hhugo/sedlex that referenced this pull request Sep 19, 2026
(cherry picked from commit abfb01d)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
hhugo added a commit to hhugo/sedlex that referenced this pull request Sep 19, 2026
(cherry picked from commit abfb01d)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
hhugo added a commit to hhugo/sedlex that referenced this pull request Sep 19, 2026
(cherry picked from commit abfb01d)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
hhugo added a commit to hhugo/sedlex that referenced this pull request Sep 19, 2026
(cherry picked from commit abfb01d)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
hhugo added a commit to hhugo/sedlex that referenced this pull request Sep 19, 2026
(cherry picked from commit abfb01d)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
hhugo added a commit to hhugo/sedlex that referenced this pull request Sep 19, 2026
(cherry picked from commit abfb01d)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
hhugo added a commit to hhugo/sedlex that referenced this pull request Sep 20, 2026
(cherry picked from commit abfb01d)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
hhugo added a commit to hhugo/sedlex that referenced this pull request Sep 20, 2026
(cherry picked from commit abfb01d)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@hhugo
hhugo changed the base branch from pr4a-tdfa-prep to master September 20, 2026 14:08
hhugo added a commit to hhugo/sedlex that referenced this pull request Sep 20, 2026
(cherry picked from commit abfb01d)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@hhugo
hhugo marked this pull request as ready for review September 20, 2026 14:34
hhugo added a commit to hhugo/sedlex that referenced this pull request Sep 20, 2026
(cherry picked from commit abfb01d)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@hhugo
hhugo requested a balanced review from Copilot September 20, 2026 19:28

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Transition seed accumulation introduces avoidable quadratic compilation cost, and several regression comments remain outdated.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity · 1 Low severity

Open (2)
What changed in this PR

Reworks determinization into a Laurikari TDFA to preserve correct capture positions and leftmost-greedy semantics.

Changes:

  • Tracks per-path tag registers and realigns them across DFA states.
  • Corrects greedy Opt, Plus, and bounded repetition behavior.
  • Expands capture regression and code-generation coverage.
File Description
src/​compiler/​sedlex.ml Implements tagged DFA construction and register handling.
src/​syntax/​ppx_sedlex.ml Makes Opt consume-first.
test/​basic.ml Updates capture and repetition regression expectations.
test/​codegen/​test_gen.ml Adds and updates generated-code expectations.
test/​codegen/​test_realistic.ml Updates realistic lexer register expectations.
CHANGES.md Documents the capture fix.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/compiler/sedlex.ml
Comment thread test/basic.ml
hhugo and others added 10 commits September 20, 2026 21:37
Tag operations were collected into one flat list per DFA transition,
with one register per tag shared by all live NFA paths. When a tagged
node is reachable from several paths at once, typically a Star/Plus
loop whose closure contains the start of the following capture, the
loop path re-fires the write on every iteration and clobbers the
position of the path already inside the capture: wrong, empty or
out-of-bounds submatches (ocaml-community#199).

A DFA state is now an ordered list of configurations (NFA node,
tag -> register map). Order is priority: the epsilon closure is a DFS
along eps lists and the first path to reach a node wins, which gives
leftmost-greedy disambiguation. States are looked up modulo register
renaming; reaching an existing state emits the moves realigning the
registers, as a parallel move. An accepting state copies the accepting
path's registers into the canonical cells just before Sedlexing.mark.

Every tag write gets a fresh register, so cell counts grow; reusing
registers is left to a follow-up PR. States are built depth first with
targets interned in character-set order, so patterns without captures
keep their state numbering and generated code.

The pinned loop_before_capture and capture_before_loop tests flip to
the expected captures. opt_greedy and plus_nullable_first_alternative
regress here and are restored by the greedy Opt and Plus commits that
follow.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
[Rep (r, 0, m)] unrolls to [r Rep(r, 0, m-1) | eps]; taking an iteration
now comes first, so bounded repetition prefers more iterations, like
Star and Plus. With ordered configurations the alternation order is
what decides capture positions among equal-length parses, so the two
Rep cases of capture_before_loop and the Rep half of opt_greedy flip
to the greedy capture.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The epsilon closure now records a configuration only for nodes with
outgoing character transitions and for rule-final nodes. Epsilon-only
nodes contribute nothing once visited; keeping them bloated state keys,
so states that behave identically were told apart by which epsilon
nodes led to them. A few states merge and some discriminator writes
that only the losing branch needed disappear.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
eps is the identity for seq, so the left-to-right fold can start from
(left, eps, []) and needs no special case for the first element. This
drops the three intermediate arrays, the imperative rights loop, and
the index threading.

(cherry picked from commit 21b8009)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Opt was lowered as `alt eps r` (empty branch first), so under leftmost-greedy
disambiguation the empty alternative won — while Star/Plus/Rep were made
iterate-first (greedy). That made the semantically identical `Opt p` and
`Rep (p, 0 .. 1)` produce different capture positions when the empty and
consuming parses have equal total length.

Lower Opt as `alt r eps` (consume-first) for greedy consistency, and add a
test pinning `Opt 'a', (Star 'a' as x)` == `Rep ('a', 0 .. 1), (Star 'a' as x)`.

(cherry picked from commit ac3deb8)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`plus r` built a single loop entered at the body, with the loop node's
epsilon list ordered [body; exit]. When the first iteration consumed
nothing, the closure walk reached the loop node from inside the body,
found the body already visited, and continued to the exit before the
body's remaining (consuming) alternatives — so `(Plus (Opt 'd' | 'b') as
x), Star 'b'` on "b" captured "" instead of "b", unlike `Star` and the
explicit `r, Star r`.

Build `plus r` as `r, Star r` with the body duplicated, as ocamllex does.
The DFA is unchanged for ordinary patterns.

(cherry picked from commit 46a9ff3)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Many as-binding tests compile to zero tags because both capture
boundaries are statically known (Start_plus/End_minus). Each such test
gains a counterpart with variable-length context where elimination
cannot happen, so the generated code exercises real memory cells:
position tags, tag-anchored offsets, and per-branch discriminators.

(cherry picked from commit 40ac620)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
"as binding: or-pattern with discriminator" was byte-identical to
"optim: discriminator elision" and misnamed: no discriminator is
emitted for that pattern since both branches yield the same static
positions. The dynamic counterpart now references the optim test.

(cherry picked from commit 32edf06)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
(cherry picked from commit abfb01d)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Every tag now costs its canonical cell plus a working register, copied
on accept, so the cell counts recorded in the optimization-tracking
notes double. Record them, with writing unambiguous tags directly to
their canonical cell as the goal.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@hhugo

hhugo commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator Author

@pmetzger, this is ready to go. Let me know if you want to give it some review. Otherwise, I think I'll merge

@pmetzger

Copy link
Copy Markdown
Member

If you feel it is ready, do it. I do still encourage eventually looking at the work done for re2c btw; they took Laurikari's work and advanced the state of the art significantly. They have papers explaining it.

@hhugo

hhugo commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator Author

If you feel it is ready, do it. I do still encourage eventually looking at the work done for re2c btw; they took Laurikari's work and advanced the state of the art significantly. They have papers explaining it.

This work is inspired by re2c already

@hhugo
hhugo merged commit 232ad40 into ocaml-community:master Sep 28, 2026
8 checks passed
hhugo added a commit that referenced this pull request Sep 28, 2026
(cherry picked from commit abfb01d)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@hhugo
hhugo deleted the pr4-tdfa-rewrite branch September 28, 2026 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants