From 53a964fa0376406cc7911c3765855c4905106f10 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 18:59:29 +0000 Subject: [PATCH 1/3] LL(1): match with an explicit stack and a shared-input cursor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The LL(1) matcher recursed once per rule in a dispatched chain, and a right-recursive rule — how `repeat0Plus` encodes repetition — puts one such chain per repetition, so match depth grew with input length rather than grammar size. `parser(repeat0Plus(set(' \t')))` overflowed the JS call stack on a few thousand spaces, and deep bracket nesting hit the same limit the same way. The sibling descent backend was fixed in #1303; this ports that fix. `f` in `parserRuleSet` is now an explicit-stack machine: suspended rules-chain frames live on an immutable cons-cell stack, and each iteration either dispatches the current task's rule or feeds the pending result into the innermost frame. Positions are now cursors into one shared input array over `0 .. cp.length + 1`, where `cp.length + 1` means the synthesized EOF has been consumed — the `(idx, eofConsumed)` pair as one number, replacing the remainder slice plus threaded flag. Consuming a code point no longer re-slices the rest of the input (which made a match quadratic); the `MatchResult` remainder is materialized once, when the match returns. `MatchRule` and `_MatchResultEof` described the recursive matcher's signature and are removed; nothing outside this module used them. `_DispatchRuleCollection.rules` becomes `readonly string[]`: the builder only ever appends rule names, so the `_DispatchRule | string` union was dead and its unreachable branch was the module's one coverage gap. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014tCLFXucKxrz85w3D8MjQW --- fjs/bnf/ll1/README.md | 37 ++- fjs/bnf/ll1/module.f.mjs | 225 +++++++++++++----- fjs/bnf/ll1/proof.f.mjs | 21 ++ fjs/bnf/ll1/todo/stack-recursive-matching.md | 72 ------ fjs/bnf/ll1/types.ts | 27 +-- fjs/bnf/todo/proof-recognizer-and-fixtures.md | 4 +- 6 files changed, 236 insertions(+), 150 deletions(-) delete mode 100644 fjs/bnf/ll1/todo/stack-recursive-matching.md diff --git a/fjs/bnf/ll1/README.md b/fjs/bnf/ll1/README.md index 96a6ce9ce..ab35946d8 100644 --- a/fjs/bnf/ll1/README.md +++ b/fjs/bnf/ll1/README.md @@ -7,15 +7,44 @@ An LL(1) dispatch/matcher backend built over the BNF data `parserRuleSet()` match input into an AST. The builder throws at build time (`can not merge …`) when the grammar is not LL(1) — a first/first conflict. -## Logical EOF +## Logical EOF and the complete cursor The caller passes physical symbols only; the matcher synthesizes the one logical EOF after them ([`../README.md`](../README.md#logical-eof-in-parser-input)). A -rule that dispatches on `eof` consumes it at the physical end of input, once — -the match threads an `eofConsumed` flag alongside the remainder, which is the -`(idx, eofConsumed)` cursor in this backend's remainder-shaped form. +rule that dispatches on `eof` consumes it at the physical end of input, once. +Internally a position is therefore a cursor over `0 .. cp.length + 1`, where +`cp.length + 1` means the synthesized EOF has been consumed — the +`(idx, eofConsumed)` pair of the shared design written as one number, because +`eofConsumed` can only be true at the physical end. The same cursor used to be a +remainder slice paired with a threaded `eofConsumed` flag; it says the same +thing, and being a number is what lets the input array be shared instead of +copied (see below). Remainders stay physical, so consuming EOF leaves an empty remainder rather than the `null` this backend reports when a match runs out of input. Since EOF sits below every ordinary symbol, its dispatch entry cuts at `-2`; the dispatch map holds decoded terminals, never stored endpoint codes. + +## Matching without the JS call stack + +The matcher is an explicit-stack machine, like the sibling +[`../descent`](../descent) backend's: it keeps suspended rules-chain frames on +an immutable cons-cell stack and loops, either dispatching the rule the current +task names or feeding the pending result into the innermost frame. + +The JS call stack cannot do this job here. Matching recursed once per rule in a +dispatched chain, and a right-recursive rule — which is how `repeat0Plus` +encodes repetition — puts one such chain per repetition, so the depth grew with +*input length*, not grammar size: a few thousand code points were enough for +`RangeError: Maximum call stack size exceeded`. Deeply nested input reached the +same limit through the same mechanism. The explicit stack grows on the heap +instead, so depth is bounded by memory (see the `longInput` proof group). + +Positions are cursors into one shared input array for the second half of the +same reason. Consuming a code point by re-slicing the remainder +(`const [, ...restCp] = cp`) copied the whole rest of the input at every step, +making a match quadratic in input length; the remainder slice a `MatchResult` +carries is materialized once, when the match returns. + +LL(1) never backtracks, so a cursor only ever moves forward — which is why the +machine needs no rewind state per frame, unlike the descent backend. diff --git a/fjs/bnf/ll1/module.f.mjs b/fjs/bnf/ll1/module.f.mjs index 2ba412eab..9c2fc4aa0 100644 --- a/fjs/bnf/ll1/module.f.mjs +++ b/fjs/bnf/ll1/module.f.mjs @@ -21,7 +21,7 @@ * @import { StringSet } from '../../types/string_set/types.ts' * @import { EmptyTag, RuleSet } from '../data/types.ts' * @import { Rule as FRule } from '../types.ts' - * @import { AstSequence, AstTag, Match, MatchResult, MatchRule, Remainder, _Dispatch, _DispatchMap, _DispatchResult, _DispatchRule, _DispatchRuleCollection, _MatchResultEof } from './types.ts' + * @import { AstSequence, AstTag, Match, MatchResult, Remainder, _AstRule, _Dispatch, _DispatchMap, _DispatchResult, _DispatchRule } from './types.ts' */ import { strictEqual } from '../../types/function/operator/module.f.mjs' @@ -150,13 +150,101 @@ export const parser = fr => { return parserRuleSet(data[0]) } -/** @type {(tag: AstTag, sequence: AstSequence, r: Remainder) => MatchResult} */ -const mrSuccess = (tag, sequence, r) => - [{tag, sequence}, true, r] +/** + * A match position over the physical input: `0 .. cp.length` are the physical + * positions, and `cp.length + 1` is where the one synthesized EOF has been + * consumed. + * + * This is the `(idx, eofConsumed)` cursor written as one number: `eofConsumed` + * can only be true at the physical end, so the pair and the extended position + * hold the same information. + * + * @typedef {number} _Cursor + */ + +/** + * Where a match stopped: a {@link _Cursor}, or `null` when it ran out of input + * — the `null` {@link Remainder} this backend reports for that. + * + * @typedef {_Cursor|null} _Position + */ + +/** + * The machine's own result: a {@link MatchResult} positioned by a cursor + * instead of by a materialized remainder. + * + * @typedef {{ + * readonly ast: _AstRule + * readonly success: boolean + * readonly pos: _Position + * }} _Result + */ + +/** + * A suspended rules-chain match: `rules[ruleIndex]` is being matched by the + * current task, and `seq` holds the ASTs matched so far — starting with the + * dispatched symbol's leaf, empty when that symbol was the synthesized EOF. + * + * @typedef {{ + * readonly tag: AstTag + * readonly rules: readonly string[] + * readonly ruleIndex: number + * readonly seq: AstSequence + * }} _Frame + */ + +/** + * Immutable cons-cell stack: O(1) push/pop, no array copying per step. + * + * @typedef {null | { + * readonly top: _Frame + * readonly rest: _Stack + * }} _Stack + */ + +/** + * The rule invocation about to be evaluated, or `null` when a result is ready + * to resume the innermost frame instead. + * + * @typedef {{ + * readonly rule: _DispatchRule + * readonly pos: _Cursor + * }} _Task + */ + +/** @type {(tag: AstTag, sequence: AstSequence, pos: _Position) => _Result} */ +const mrSuccess = (tag, sequence, pos) => + ({ast: {tag, sequence}, success: true, pos}) -/** @type {(tag: AstTag, sequence: AstSequence, r: Remainder) => MatchResult} */ -const mrFail = (tag, sequence, r) => - [{tag, sequence}, false, r] +/** @type {(tag: AstTag, sequence: AstSequence, pos: _Position) => _Result} */ +const mrFail = (tag, sequence, pos) => + ({ast: {tag, sequence}, success: false, pos}) + +/** + * The semantic symbol a cursor points at: a code point inside the physical + * input, and the synthesized {@link eofSymbol} at its end. Only meaningful + * where the cursor still has a symbol, `pos <= cp.length`. + * + * @type {(cp: readonly CodePoint[], pos: _Cursor) => number} + */ +const symbolAt = (cp, pos) => pos < cp.length ? cp[pos] : eofSymbol + +/** + * What consuming the symbol at a cursor contributes to the AST: the code point + * itself, and nothing for the synthesized EOF — it has no physical source + * element. + * + * @type {(cp: readonly CodePoint[], pos: _Cursor) => AstSequence} + */ +const leafAt = (cp, pos) => pos < cp.length ? [cp[pos]] : [] + +/** + * The public remainder of a position. It stays physical, so consuming EOF + * leaves it empty; the slice is materialized once, at the end of a match. + * + * @type {(cp: readonly CodePoint[], pos: _Position) => Remainder} + */ +const remainderAt = (cp, pos) => pos === null ? null : cp.slice(Math.min(pos, cp.length)) /** * Creates an LL(1) parser from an already materialized {@link RuleSet}. @@ -166,57 +254,88 @@ const mrFail = (tag, sequence, r) => export const parserRuleSet = ruleSet => { const map = dispatchMap(ruleSet) - // Matches the rules the dispatched symbol selected, one after another, - // starting from `seq0` — the AST of that symbol, empty when it was the - // synthesized EOF. - /** @type {(d: _DispatchRuleCollection, seq0: AstSequence, cp: readonly CodePoint[], eofConsumed: boolean) => _MatchResultEof} */ - const items = ({tag, rules}, seq0, cp, eofConsumed) => { - let seq = seq0 - let r = cp - let eofDone = eofConsumed - for (const i of rules) { - const rule = typeof i === 'string' ? /** @type {_DispatchRule} */ (map[i]) : i - const [res, itemEof] = f(rule, r, eofDone) - const [astRule, success, newR] = res - if (success === false) { - return [res, itemEof] + /** @type {(name: string) => _DispatchRule} */ + const dispatched = name => /** @type {_DispatchRule} */ (map[name]) + + // The matcher as an explicit-stack machine: each iteration either starts the + // current task (dispatching one rule and pushing a frame for the rules chain + // that symbol selected) or feeds the pending result into the innermost frame. + // Semantics are identical to the former recursive matcher, but the JS call + // stack stays O(1) however deep the grammar recurses: a right-recursive rule + // (e.g. a `repeat0Plus` chain) descends once per consumed code point, so the + // depth used to grow with input length and overflow (see the longInput proof + // group). Positions are cursors into the shared input array for the same + // reason: re-slicing the remainder per step made a match quadratic. + /** @type {(dr0: _DispatchRule, cp: readonly CodePoint[]) => MatchResult} */ + const f = (dr0, cp) => { + /** @type {_Stack} */ + let stack = null + /** @type {_Task|null} */ + let task = {rule: dr0, pos: 0} + /** @type {_Result} */ + let result = mrFail(undefined, [], 0) + + while (true) { + if (task !== null) { + // The explicit annotation cuts a control-flow inference cycle + // (TS7022): `task`'s narrowed type feeds `pos`, which feeds the + // `task` assignment below that the narrowing depends on. + /** @type {_Task} */ + const current = task + const {rule: {emptyTag, rangeMap}, pos} = current + task = null + // The one logical EOF is available at the physical end, and only + // there: a rule that dispatches on it consumes it, once. Past it + // no symbol is left to dispatch on. + const dr = pos > cp.length ? null : dispatchOp.get(rangeMap)(symbolAt(cp, pos)) + if (dr === null) { + if (emptyTag !== undefined) { + result = mrSuccess(emptyTag, [], pos) + } else if (pos >= cp.length) { + // Nothing left to reject: the match ran out of input. + result = mrSuccess(emptyTag, [], null) + } else { + result = mrFail(emptyTag, [], pos) + } + continue + } + const {tag, rules} = dr + const seq = leafAt(cp, pos) + const next = pos + 1 + if (rules.length === 0) { + result = mrSuccess(tag, seq, next) + } else { + stack = {top: {tag, rules, ruleIndex: 0, seq}, rest: stack} + task = {rule: dispatched(rules[0]), pos: next} + } + continue } - seq = [...seq, astRule] - eofDone = itemEof - if (newR === null) { - return [mrSuccess(tag, seq, null), eofDone] + + if (stack === null) { + const {ast, success, pos} = result + return [ast, success, remainderAt(cp, pos)] } - r = newR - } - return [mrSuccess(tag, seq, r), eofDone] - } + const frame = stack.top + stack = stack.rest - /** @type {MatchRule} */ - const f = ({emptyTag, rangeMap}, cp, eofConsumed) => { - if (cp.length === 0) { - // The one logical EOF is available at the physical end, and only - // there: a rule that dispatches on it consumes it, once. - const eofDr = eofConsumed ? null : dispatchOp.get(rangeMap)(eofSymbol) - if (eofDr === null) { - return [mrSuccess(emptyTag, [], emptyTag === undefined ? null : cp), eofConsumed] + const {ast, success, pos} = result + // A failing item's own result propagates unchanged, exactly as the + // recursive version returned it through the chain. + if (success === false) { continue } + const seq = [...frame.seq, ast] + if (pos === null) { + result = mrSuccess(frame.tag, seq, null) + continue + } + const ruleIndex = frame.ruleIndex + 1 + if (ruleIndex < frame.rules.length) { + stack = {top: {...frame, ruleIndex, seq}, rest: stack} + task = {rule: dispatched(frame.rules[ruleIndex]), pos} + } else { + result = mrSuccess(frame.tag, seq, pos) } - // The synthesized EOF has no physical source element, so it adds no - // AST leaf, and the remainder stays physical — already empty here. - return items(eofDr, [], cp, true) - } - const [cp0] = cp - const dr = dispatchOp.get(rangeMap)(cp0) - if (dr === null) { - return [ - emptyTag === undefined - ? mrFail(emptyTag, [], cp) - : mrSuccess(emptyTag, [], cp), - eofConsumed, - ] } - const [, ...restCp] = cp - return items(dr, [cp0], restCp, eofConsumed) } - return (name, cp) => f(/** @type {_DispatchRule} */ (map[name]), cp, false)[0] + return (name, cp) => f(dispatched(name), cp) } diff --git a/fjs/bnf/ll1/proof.f.mjs b/fjs/bnf/ll1/proof.f.mjs index 3b4de5a6d..bd7591613 100644 --- a/fjs/bnf/ll1/proof.f.mjs +++ b/fjs/bnf/ll1/proof.f.mjs @@ -274,6 +274,27 @@ export const proof = { expect(' [{ "q": [ 12, false, [}], "a"] }] ', false) } ], + longInput: [ + () => { + // Long right-recursive repetition: one `repeat0Plus` chain across the + // whole input. This is the shape that overflowed the JS call stack + // when the matcher recursed once per consumed code point. + const m = parser(repeat0Plus(set(' \n\r\t'))) + const [, success, remainder] = m('', toArray(stringToCodePointList(' '.repeat(10000)))) + assertEq(success, true) + assertEq(remainder?.length, 0) + }, + () => { + // Deep non-repetition nesting: 5000 bracket levels in the JSON-like + // test grammar — a shape a repetition-specific fix would not cover. + const m = parser(deterministic()) + const n = 5000 + const cp = toArray(stringToCodePointList('['.repeat(n) + ']'.repeat(n))) + const [, success, remainder] = m('', cp) + assertEq(success, true) + assertEq(remainder?.length, 0) + }, + ], logicalEof: [ () => { // EOF dispatches below every ordinary symbol, so its cut point is diff --git a/fjs/bnf/ll1/todo/stack-recursive-matching.md b/fjs/bnf/ll1/todo/stack-recursive-matching.md deleted file mode 100644 index b7206c23f..000000000 --- a/fjs/bnf/ll1/todo/stack-recursive-matching.md +++ /dev/null @@ -1,72 +0,0 @@ -## stack-recursive-matching. LL(1) matcher recurses once per grammar step, overflowing on long input - -**Priority:** P3 -**Status:** open - -### Problem - -`fjs/bnf/ll1/module.f.mjs`'s matcher `f` (inside `parserRuleSet`) recurses -natively: after consuming a code point it walks the dispatched `rules` chain -with one nested `f` call per rule. For a right-recursive rule — which is how -`repeat0Plus` encodes repetition — every additional repetition adds another -JS call-stack frame, so match depth grows with *input length*, not grammar -size. Deeply nested input (e.g. thousands of bracket levels) hits the same -limit through the same mechanism. - -Confirmed repro: `parser(repeat0Plus(set(' \t')))` matching 10,000 spaces -throws `RangeError: Maximum call stack size exceeded` (5,000 still passes — -the threshold is higher than the descent backend's old ~2,000–3,000 because -LL(1)'s per-repetition frame chain is shallower, but the failure mode is -identical). - -This is the same bug that was fixed in the sibling descent backend in PR -[#1303](https://github.com/functionalscript/functionalscript/pull/1303) (see -its CHANGELOG entry for the history): `fjs/bnf/descent/module.f.mjs`'s matcher -now runs as an explicit-stack machine and handles 100 KB+ inputs; the LL(1) -matcher was not touched. Today nothing outside `fjs/bnf/ll1`'s own proofs consumes this -parser (hence P3, not P1), but any future consumer with realistic input -sizes will hit it. - -### Proposal - -Port the descent backend's fix: rewrite `f` as an explicit-stack machine. -`fjs/bnf/descent/module.f.mjs` is the template — its matcher keeps two -suspended-frame kinds on an immutable cons-cell stack and loops, either -starting the current rule invocation or feeding the pending result into the -innermost frame. The LL(1) version is simpler: `f` has only one recursion -site (the `rules`-chain loop), so a single frame kind suffices — roughly -`{ tag, rules, ruleIndex, seq }` plus the pending remainder. - -While rewriting, also fix the quadratic input handling in the same function: -`const [, ...restCp] = cp` copies the entire remaining input on **every** -consumed code point (O(n²) time/allocation overall). The descent backend -threads an index `idx` into a shared immutable array instead; `Remainder` -being part of the public `MatchResult` type means the index can be converted -back to a slice once at the end (or the result type generalized), rather -than per step. - -Note `dispatchMap`'s build-time recursion (`dispatchRule`) is bounded by -grammar size, not input size — it does not need to change. - -### Tasks - -- [ ] Rewrite `f` in `parserRuleSet` as an explicit-stack loop (single frame - kind: dispatched rules chain + accumulated sequence + tag). -- [ ] Replace per-step rest-spread with an index into the input array; - materialize the `Remainder` slice only at the boundary. -- [ ] Add a `longInput` proof group mirroring - `fjs/bnf/descent/proof.f.mjs` — long `repeat0Plus` repetition (10,000+ - code points) and deep bracket nesting via `deterministic()`. -- [ ] `npx tsc`, `node ./fjs/module.mjs t`. - -### Related - -- `fjs/bnf/descent/module.f.mjs` — the ported fix to mirror (explicit frame - stack; see the `longInput` proof group in its `proof.f.mjs`), landed in PR - [#1303](https://github.com/functionalscript/functionalscript/pull/1303), - whose CHANGELOG entry records the history of the same bug in the descent - backend and why the fix belongs in the matcher, not the grammar. -- [../../todo/667-bnf-repeat-flatten.md](../../todo/667-bnf-repeat-flatten.md) - — the `repeat` data-node proposal; would reduce repetition depth for - detected shapes but is not a complete fix (deep nesting, undetected - right-recursive shapes), and is tracked for data-shape reasons. diff --git a/fjs/bnf/ll1/types.ts b/fjs/bnf/ll1/types.ts index 48477fc32..6e67f8940 100644 --- a/fjs/bnf/ll1/types.ts +++ b/fjs/bnf/ll1/types.ts @@ -21,13 +21,16 @@ export type _Dispatch = RangeMapArray<_DispatchResult> /** @internal */ export type _DispatchResult = _DispatchRuleCollection | null -/** @internal */ -export type _DispatchRuleOrName = _DispatchRule | string - -/** @internal */ +/** + * The rules a dispatched symbol selects, to be matched one after another. They + * are names into the {@link _DispatchMap}: the builder only ever appends rule + * names, so the matcher resolves each one there. + * + * @internal + */ export type _DispatchRuleCollection = { readonly tag: string | undefined, - readonly rules: _DispatchRuleOrName[] + readonly rules: readonly string[] } /** @internal */ @@ -72,17 +75,3 @@ export type MatchResult = readonly[_AstRule, boolean, Remainder] * end-of-input symbol after them. */ export type Match = (name: string, s: readonly CodePoint[]) => MatchResult - -/** - * A {@link MatchResult} paired with the matcher's own progress past the - * physical input: `true` once a rule has consumed the synthesized end-of-input - * symbol, so no later rule can consume it a second time. - * - * @internal - */ -export type _MatchResultEof = readonly[MatchResult, boolean] - -/** - * Internal match function signature used by compiled dispatch rules. - */ -export type MatchRule = (dr: _DispatchRule, s: readonly CodePoint[], eofConsumed: boolean) => _MatchResultEof diff --git a/fjs/bnf/todo/proof-recognizer-and-fixtures.md b/fjs/bnf/todo/proof-recognizer-and-fixtures.md index e44478ee4..7fe121611 100644 --- a/fjs/bnf/todo/proof-recognizer-and-fixtures.md +++ b/fjs/bnf/todo/proof-recognizer-and-fixtures.md @@ -177,8 +177,8 @@ explicit named override list for the rows where token-stream acceptance differs. moves the grammars themselves; this issue moves the proof harness/fixtures. - [65Y-proof-assertEq-adoption](../../emergent_testing/todo/65y-proof-asserteq-adoption.md) — orthogonal assertion cleanup. -- [stack-recursive-matching](../ll1/todo/stack-recursive-matching.md) — separate - long-input regression corpus. +- `fjs/bnf/ll1/proof.f.mjs` / `fjs/bnf/descent/proof.f.mjs` `longInput` — the + separate long-input regression corpus both matchers already carry. - [new-parser](./new-parser.md) — token-symbol alphabet needs its own recognizer adapter, but can share `Case` / `assertRecognizes`. - `fjs/bnf/descent/types.ts` `DescentFailure` — failure diagnostics compose From 1b7a34f244d34f0674fd0d7699c4e0e8deb6dcfc Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 19:00:24 +0000 Subject: [PATCH 2/3] Add CHANGELOG entry for #1531 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014tCLFXucKxrz85w3D8MjQW --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 957cb4fc3..41b95d48c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,12 @@ history. ## Unreleased +- **BREAKING CHANGES:** `fjs/bnf/ll1`'s matcher runs as an explicit-stack + machine over a cursor into the shared input, so long or deeply nested input + no longer overflows the JS call stack and matching is no longer quadratic. + The `MatchRule` type described the recursive matcher and is removed; match + results are unchanged + [#1531](https://github.com/functionalscript/functionalscript/pull/1531) - `types/bigfloat`: `decToBin` no longer returns a 54-bit mantissa when rounding carries out of the top bit; the result is always the 53-bit IEEE-754 significand From 5b4214133458b9a66a916bc96f8a5e8ff19936e2 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 19:23:58 +0000 Subject: [PATCH 3/3] Propose one owner for the matcher cursor, AST, and result constructors Rewrites 669-bnf-data-shared-helpers against today's code and renames it: the backends moved out of `bnf/data`, both result constructors are records now, and the duplication has grown past the `mrSuccess`/`mrFail` pair the issue described. `ll1` and `descent` each carry the complete cursor, the AST family, and the constructors, and `AstTag` is declared byte-identically in both `types.ts`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014tCLFXucKxrz85w3D8MjQW --- fjs/bnf/todo/669-bnf-data-shared-helpers.md | 75 ------- fjs/bnf/todo/669-bnf-matcher-shared-core.md | 222 ++++++++++++++++++++ fjs/bnf/todo/terminal-range-shared-type.md | 6 +- 3 files changed, 226 insertions(+), 77 deletions(-) delete mode 100644 fjs/bnf/todo/669-bnf-data-shared-helpers.md create mode 100644 fjs/bnf/todo/669-bnf-matcher-shared-core.md diff --git a/fjs/bnf/todo/669-bnf-data-shared-helpers.md b/fjs/bnf/todo/669-bnf-data-shared-helpers.md deleted file mode 100644 index 55034b29f..000000000 --- a/fjs/bnf/todo/669-bnf-data-shared-helpers.md +++ /dev/null @@ -1,75 +0,0 @@ -## 669-bnf-data-shared-helpers. Hoist and share repeated helpers in `bnf/data` - -**Priority:** P4 -**Status:** open - -### Problem - -`fjs/bnf/data/module.f.mjs` carries a DRY / hoisting smell in its parser -machinery, not covered by the existing fold-children work in -[i665-bnf-data-fold-children](todo.md) (that issue is -about the `sequence` / `variant` AST-fold helpers, a different pair of -functions). - -(A second smell used to be listed here — duplicated `emptyTagMapAdd` branches -in `fjs/bnf/descent` — but that function was deleted and replaced by a single -shared `emptyTagMap` fixpoint in `fjs/bnf/data/module.f.mjs` while fixing -nullable-analysis-shared, so it no longer applies.) - -#### 1. `mrSuccess` / `mrFail` match-result constructors - -Both `descentParser` (lines 384-385) and `parserRuleSet` (lines 452-453) define -a local pair of match-result constructors *inside* their recursive `f` -callback: - -```ts -// descentParser, inside f (383-385) -const mrSuccess = (tag: AstTag, sequence: AstSequenceMeta, idx: number): DescentMatchResult => [{tag, sequence}, true, idx] -const mrFail = (tag: AstTag, sequence: AstSequenceMeta, idx: number): DescentMatchResult => [{tag, sequence}, false, idx] - -// parserRuleSet, inside f (452-453) -const mrSuccess = (tag: AstTag, sequence: AstSequence, r: Remainder): MatchResult => [{tag, sequence}, true, r] -const mrFail = (tag: AstTag, sequence: AstSequence, r: Remainder): MatchResult => [{tag, sequence}, false, r] -``` - -Two problems: - -- **Hoisting.** These helpers capture no local state — they are pure functions - of their arguments — yet they are redeclared on *every* recursive call of `f`. - `AGENTS.md` says: "Hoist helpers to module scope when they don't capture local - state — don't redeclare them inside another function on every call." -- **DRY.** All four constructors are the same shape: `[{ tag, sequence }, - success, third]`. They differ only in the type of the third tuple element - (`number` index vs `Remainder`) and the sequence type — both of which a - generic parameter captures. - -### Proposal - -**Hoist a single match-result constructor** to module scope, generic over the -sequence and third-element types, and derive `mrSuccess` / `mrFail` (or call -it directly with the success flag) in both parsers: - -```ts -const mr = (success: boolean) => - (tag: AstTag, sequence: S, r: R): readonly [{ readonly tag: AstTag, readonly sequence: S }, boolean, R] => - [{ tag, sequence }, success, r] -``` - -`descentParser` uses `mr, number>`; `parserRuleSet` uses -`mr`. Both `f` bodies drop their four local -declarations. - -This is a local, single-module refactor with no cross-module coordination. - -### Tasks - -- [ ] Hoist the shared `mr` constructor to module scope; update both `f` bodies. -- [ ] Run `npx tsc`, `fjs t`, and confirm `fjs/bnf/data/proof.f.mjs` still passes - with full coverage. - -### Related - -- [i665-bnf-data-fold-children](todo.md) — the adjacent - `sequence` / `variant` AST-fold extraction (different functions, same module). -- [i667-bnf-repeat-flatten](todo.md) — other `bnf/data` - cleanups. diff --git a/fjs/bnf/todo/669-bnf-matcher-shared-core.md b/fjs/bnf/todo/669-bnf-matcher-shared-core.md new file mode 100644 index 000000000..8c92e3f3b --- /dev/null +++ b/fjs/bnf/todo/669-bnf-matcher-shared-core.md @@ -0,0 +1,222 @@ +## 669-bnf-matcher-shared-core. One owner for the matcher cursor, AST, and result constructors + +**Priority:** P3 +**Status:** open + +> Renamed from `669-bnf-data-shared-helpers.md`. That issue described the +> `mrSuccess` / `mrFail` pair as a `fjs/bnf/data` hoisting smell, quoting a +> code state that no longer exists: the backends have since moved into +> `fjs/bnf/ll1` and `fjs/bnf/descent`, both constructors build records rather +> than tuples, and neither is declared inside the matcher's `f` any more. The +> duplication it pointed at is real and has grown — it is now the smallest of +> three things the two backends each own a copy of. Item 3 below is the +> original issue, corrected. + +### Problem + +`fjs/bnf/ll1` and `fjs/bnf/descent` are two matchers over one contract. +[`fjs/bnf/README.md`](../README.md#logical-eof-in-parser-input) states that +contract normatively — callers supply physical symbols only, each backend +synthesizes exactly one logical EOF after them, public positions stay physical, +and internally a backend tracks the complete cursor `(idx, eofConsumed)` +because consuming EOF is progress even though `idx` does not move. Nothing owns +it in code. Each backend derives it separately, and since +[#1531](https://github.com/functionalscript/functionalscript/pull/1531) both +derive it *identically*, as the extended position `0 .. cp.length + 1`. + +Three things are duplicated. + +#### 1. The cursor and its accessors + +```js +// fjs/bnf/descent/module.f.mjs // fjs/bnf/ll1/module.f.mjs +/** @typedef {number} _Cursor */ /** @typedef {number} _Cursor */ +const symbolAt = (cp, pos) => const symbolAt = (cp, pos) => + pos < cp.length ? cp[pos][0] pos < cp.length ? cp[pos] + : eofSymbol : eofSymbol +const leafAt = (cp, pos) => const leafAt = (cp, pos) => + pos < cp.length ? [cp[pos]] : [] pos < cp.length ? [cp[pos]] : [] +const physicalIdx = length => pos => const remainderAt = (cp, pos) => pos === null ? null + Math.min(pos, length) : cp.slice(Math.min(pos, cp.length)) +``` + +`leafAt` is the same function twice, already generic in the leaf type. +`symbolAt` differs only in how it reads a symbol out of a leaf — `cp[pos]` +against `cp[pos][0]`. `physicalIdx` is the clamp `remainderAt` slices at. + +The prose is duplicated with the code: the same explanation of why +`cp.length + 1` is a position, and why consuming EOF counts as progress, now +appears in both modules and in the README. When one copy is corrected the +others silently drift — and this is a contract every future backend +([recognizer-backend](./recognizer-backend.md), +[new-parser](./new-parser.md)) must also implement. + +#### 2. The AST shape + +`ll1`'s `_AstRule` / `AstSequence` and `descent`'s `AstRuleMeta` / +`AstSequenceMeta` are one type family parameterized by leaf type: `ll1`'s +leaf is `CodePoint`, `descent`'s is `CodePointMeta`. `AstTag` is declared +byte-identically in both `types.ts` files: + +```ts +export type AstTag = string|true|undefined +``` + +#### 3. The result constructors + +Both modules define the pair the original issue named, now record-shaped and at +module scope in `ll1` and inside `descentParser` in `descent`: + +```js +const mrSuccess = (tag, sequence, pos) => ({ast: {tag, sequence}, success: true, pos}) +const mrFail = (tag, sequence, pos) => ({ast: {tag, sequence}, success: false, pos}) +``` + +They differ only in the leaf type and in `pos` — `_Cursor` in `descent`, +`_Cursor|null` in `ll1`, whose extra `null` is the "ran out of input" state +its public `Remainder` reports. + +#### What must not be shared + +The machines themselves. `descent` backtracks: two frame kinds, a per-frame +rewind to the sequence's start, and a furthest-failure high-water mark that +outlives the rewinds. `ll1` is predictive: one frame kind, and a cursor that +never moves backwards. Their public results are different types on purpose — +`{ ast, success, idx, failure? }` against `readonly [ast, success, Remainder]`. +One matcher covering both would be worse than two, and this issue does not +propose it. + +### Proposal + +A new `fjs/bnf/matcher/` owning the shared layer — the cursor, the AST it +builds, and the constructor that pairs them. One module rather than three: the +three are interdependent (`leafAt` produces AST leaves, `mr` builds AST nodes +positioned by a cursor), and splitting them would put one concept behind three +imports. + +`fjs/bnf/matcher/types.ts`: + +```ts +/** Tag of an AST node ... */ +export type AstTag = string|true|undefined + +/** An AST over leaves of type `L`. */ +export type Ast = { + readonly tag: AstTag + readonly sequence: AstSequence +} + +export type AstSequence = readonly(Ast|L)[] + +/** + * A match position over input of `length` leaves: `0 .. length` are the + * physical positions, and `length + 1` is where the one synthesized EOF has + * been consumed. <...the (idx, eofConsumed) explanation, once...> + */ +export type Cursor = number + +/** + * A matcher's own result: an AST, whether it matched, and where it stopped. + * `P` is the position type — `Cursor` for a backend whose match always has + * one, `Cursor|null` for one that also reports running out of input. + */ +export type AstResult = { + readonly ast: Ast + readonly success: boolean + readonly pos: P +} +``` + +`fjs/bnf/matcher/module.f.mjs`: + +```js +/** @type {(input: readonly L[], pos: Cursor) => readonly L[]} */ +export const leafAt = (input, pos) => pos < input.length ? [input[pos]] : [] + +/** @type {(symbolOf: (leaf: L) => number) => (input: readonly L[], pos: Cursor) => number} */ +export const symbolAt = symbolOf => (input, pos) => + pos < input.length ? symbolOf(input[pos]) : eofSymbol + +/** @type {(length: number) => (pos: Cursor) => number} */ +export const physicalIdx = length => pos => Math.min(pos, length) + +/** @type {(success: boolean) => (tag: AstTag, sequence: AstSequence, pos: P) => AstResult} */ +const mr = success => (tag, sequence, pos) => ({ast: {tag, sequence}, success, pos}) + +export const mrSuccess = mr(true) +export const mrFail = mr(false) +``` + +`symbolOf` is the one place the two leaf shapes differ, so each backend binds +its partial application once at module scope, per +[AGENTS.md §6.3](../../../AGENTS.md#place-curried-partial-applications-at-their-dependencys-scope): +`symbolAt(identity)` in `ll1` (`identity` from `fjs/types/function`) and +`symbolAt(([symbol]) => symbol)` in `descent`. + +Call sites: + +- `ll1`: `_AstRule` becomes `Ast`, `AstSequence` becomes + `AstSequence`, `_Result` becomes + `AstResult`; `AstTag`, `symbolAt`, `leafAt`, + `mrSuccess`, `mrFail` come from the shared module, and `remainderAt` keeps + only the part that is genuinely `ll1`'s — `pos === null ? null : + cp.slice(physicalIdx(cp.length)(pos))`. +- `descent`: `AstRuleMeta` becomes `Ast>`, + `AstSequenceMeta` becomes `AstSequence>`, `_Result` + becomes `AstResult, _Cursor>`; `physicalIdx`, `symbolAt`, + `leafAt`, and the constructors come from the shared module. +- Delete the `AstRuleMeta` / `AstSequenceMeta` / `AstTag` declarations rather + than aliasing the new names to the old ones + ([AGENTS.md §5.2](../../../AGENTS.md#52-the-api-is-the-most-important-part-of-quality): + two spellings for one concept is the last resort, not the convenient path). + There is exactly one external importer to update, `fjs/djs/tokenizer`, which + takes `AstRuleMeta`, `AstSequenceMeta`, `AstTag`, and `CodePointMeta` from + `descent/types.ts`. `CodePointMeta` stays in `descent` — a leaf carrying + metadata is that backend's own concept. + +The README stops describing the cursor twice: `fjs/bnf/README.md` keeps the +normative statement, each backend's README keeps only what is true of *it* +(`ll1`: no backtracking, so the cursor never rewinds; `descent`: rewind and +the furthest-failure mark), and the shared JSDoc holds the mechanics. + +### Optional, decide when implementing + +Both machines now carry the same immutable cons-cell stack: + +```js +/** @typedef {null | { readonly top: _Frame, readonly rest: _Stack }} _Stack */ +``` + +It is a typedef with no functions — push and pop are object literals at the +use site — and its frame types have nothing in common, so sharing it buys one +line each. If it is shared, it is a general immutable stack and belongs in +`fjs/types`, not in `fjs/bnf`. Flagged, not proposed. + +### Tasks + +- [ ] Create `fjs/bnf/matcher/` (`types.ts`, `module.f.mjs`, `proof.f.mjs`, + `README.md`) with the cursor, the AST family, and the result + constructors. +- [ ] Convert `fjs/bnf/ll1` to the shared module; delete its copies. +- [ ] Convert `fjs/bnf/descent` to the shared module; delete its copies, + including the `AstTag` declaration. +- [ ] Update `fjs/djs/tokenizer`'s imports to the new type names. +- [ ] Move the cursor prose: one normative statement in `fjs/bnf/README.md`, + mechanics in the shared JSDoc, backend-specific facts in each backend's + README. +- [ ] `npx tsc`, `fjs t`; both backends' proofs pass unchanged, and the new + module ships 100% proof coverage. + +### Related + +- [terminal-range-shared-type](./terminal-range-shared-type.md) — the same + one-owner move for `TerminalRange`, which `bnf` and `bnf/data` both declare. +- [665-bnf-data-fold-children](./665-bnf-data-fold-children.md) and + [667-bnf-repeat-flatten](./667-bnf-repeat-flatten.md) — other `bnf/data` + cleanups; different functions, and unaffected by this one. +- [recognizer-backend](./recognizer-backend.md), + [new-parser](./new-parser.md) — future backends that would consume this + layer rather than re-deriving the cursor a third time. +- `fjs/bnf/ll1/README.md` "Logical EOF and the complete cursor" and + `fjs/bnf/descent/README.md` "Logical EOF and the complete cursor" — the two + copies of the prose this issue gives one owner. diff --git a/fjs/bnf/todo/terminal-range-shared-type.md b/fjs/bnf/todo/terminal-range-shared-type.md index 13275a351..005c201e5 100644 --- a/fjs/bnf/todo/terminal-range-shared-type.md +++ b/fjs/bnf/todo/terminal-range-shared-type.md @@ -43,5 +43,7 @@ module already has the type you need, import it" instead of duplicating. ### Related -- `fjs/bnf/todo/669-bnf-data-shared-helpers.md` — other `bnf/data` DRY cleanups - (different functions; this type duplication is not covered there). +- [669-bnf-matcher-shared-core](./669-bnf-matcher-shared-core.md) — the same + one-owner move for the matcher backends' cursor, AST, and result + constructors (different declarations; this type duplication is not covered + there).