Fix #445 and #446: make end of input unmissable to the parser, and stop IF backtracking exponentially

[geoHeil]

Fixes #445. Fixes #446.

You asked for no PR, and the three options in #445 are still yours to answer. This is
effectively the answer to that question — option 2 rather than the one-loop guard —
pushed as a diff you can read and close in one click if you would rather do it yourself.

Rebased onto b30c3d2 (v0.10.0). Numbers are from this machine (15 cores, other work
running), so treat them as orders of magnitude, not benchmarks.

---

1. A termination oracle with an enforced budget, first

termination_tests in parser.rs, so cargo test --lib runs it, which CI already does.
Every truncated prefix of a valid statement must reach a decision — parsed or rejected —
inside 5 s.

The version suggested in #445 asserted only that a prefix terminates. That is too weak:
it passes straight over finding 3 below, which terminates and merely takes seconds. The
budget is what does the work. A normal prefix decides in microseconds, so 5 s has six
orders of magnitude of headroom — it asserts something about the parser's complexity, not
about how fast the machine is.

The budget is enforced, not just awaited. Each parse runs in a worker process — this
test binary re-executed with --exact … --ignored, gated on an env var — which streams
one decision per input back over a pipe. Going over budget means kill() and then
wait(), so nothing is left behind consuming CPU, and a worker that exhausts its stack
aborts itself:

input	reported as
a parse that never returns	OverBudget, worker killed and reaped
a parse that exhausts 16 MiB of stack	Died("… exited with signal: 6 (SIGABRT)")
a parse that panics	Panicked
anything else	Parsed or Rejected, and the tests assert which

That is not theoretical. On main the earlier in-process version did not fail — it
aborted the test binary with fatal runtime error: stack overflow, taking the rest of
the suite with it. It also blocks asserting an input that is meant to pass:
("IF~" * 24) + "1" parses correctly, but 75 bytes of it exhausts a default libtest
stack in a debug build. The worker gets an explicit 16 MiB stack, the figure CI already
passes as RUST_MIN_STACK for the pretty-print and ClickHouse suites.

On origin/main, with only this module added, each failure now names its input and the
run survives:

prefix "CREATE TABLE t (a VARCHAR2(" did not decide within 5s: OverBudget
a malformed 24-link IF chain should be Rejected within 5s: "IF~IF~…~I?{"
  left: OverBudget    right: Rejected
an unclosed custom type argument list should be Rejected within 5s: "SELECT a.:S1("
  left: OverBudget    right: Rejected
test result: FAILED. 2 passed; 3 failed; 1 ignored

On this branch: 5 passed, 1 ignored (the worker), 0.04 s, debug build. A green run
spawns one child process per test; the 712-prefix sweep is a single child.

2. parse_data_type never terminating — fixed at the cause

Why a signature change rather than a fix inside advance. No change to advance alone
can terminate that loop. Returning a terminator token, or moving the cursor past the end,
still leaves

loop {
    if self.check(TokenType::RParen) { break; }
    let token = self.advance();
    …
}

spinning, because check is false at end of input too, so nothing in the loop observes that
the tokens ran out. The loop has to observe it, and the only thing that forces every such
loop to is the return type. That is why this is the fix and not a stylistic preference: it
converts "a loop might ignore end of input" from a thing you have to notice in review into a
compile error.

I used Result<Token> rather than Option<Token>. Both make it unmissable, but Option
would have required inventing an error message at each of the 469 propagating sites, whereas
Result lets them all be a bare ? and produces one real parse error with a span
("Unexpected end of input", positioned at the end of the last token). Say the word if you
would rather have Option.

advance() and advance_text() now return Result. At end of input they error instead of
re-returning the last token while leaving self.current where it was; that old fallback is
what made the loop both fail to progress and corrupt its output.

This is the change #445 said an outside contributor should not make unilaterally, so here
is what it actually cost, for review (counts on the rebased tree):

	count
call sites	497 (251 advance, 244 advance_text, 2 via a local parser binding)
propagated, no judgement — the enclosing fn already returned Result	476 (469 a bare ?, 7 a tail expression or return)
needed a decision	21
unwrap / expect / any new panic path	0

All 21 are in private helpers whose signature is not Result, and every one is already
guarded by a check, is_at_end or is_identifier_token that proves a token is present —
so none of them can actually reach the error. They are resolved without introducing a panic:

• let Ok(tok) = … else { break } / else { return … } in a scan loop or an opportunistic
  lookahead that already has a "found nothing" return — 14
• self.advance_text().ok() where the target is already Option<String> — 6
• .ok()? in a try_parse_* that returns Option — 1

Two of these are small improvements rather than no-ops: WITH (a= and CREATE VIEW v UUID
with nothing after now report end of input instead of silently re-reading the previous token.

Nothing in the public API changes — advance, advance_text, peek, check and
is_at_end are all private.

3. IF backtracking exponentially — a second, different defect (#446)

Found while writing the oracle and tracked separately as #446. parse_primary tries
IF as an if-expression and, on None, rewinds and re-reads the IF as an identifier.
Nothing records the rejection, so every enclosing expression parse repeats the attempt and a
chain of IFs parses the same suffix twice per link.

("IF~" * k) + "I?{", release profile, no sanitizer, measured at 44ab8f9:

k	bytes	main	this branch
12	39	8.9 ms	0.61 ms
16	51	136 ms	0.25 ms
20	63	2.14 s	0.31 ms
22	69	8.90 s	0.38 ms
24	75	37.9 s	0.47 ms
40	123	not measured	0.93 ms
100	303	not measured	4.70 ms

(The k=12 figure on the right carries this harness's warm-up; k=16 onward is the real
shape. Both columns are [profile.release], i.e. opt-level = "z". At opt-level = 3
the main column is roughly 2.8x faster and still doubles.) The defect is unchanged on
b30c3d2: the oracle above reports OverBudget there for all three separators.

Cleanly base-2: +2 IFs is ~4x. k = 24 is 75 bytes for ~38 s. The same chain with a
parsing tail, ("IF~" * 24) + "1", takes 175 µs on main — a successful parse commits, so
the blowup is only on the failure path.

Three things I checked rather than assumed, because each could have made a different fix the
right one:

• ~ is not special — the separator has to be a prefix-unary operator. On main at
  k=16: ~ 584 ms, + 439 ms, - 473 ms, against 74–606 µs for space, ,, *, =,
  /, ||, AND, OR. What matters is that IF <unary> IF <unary> … keeps nesting as
  one expression. All three are now in the regression test.
• The trigger is a tail that cannot parse as an expression, not merely one that errors.
  On main at k=16, tails that parse as an expression stay fast even though the statement
  still fails — 1 918 µs, 1) 768 µs, ? 577 µs (a parameter placeholder) — while )
  243 ms, { 306 ms, FROM 278 ms and SELECT 285 ms all blow up.
• No other keyword retains a superlinear path. On this branch, all 16 keywords I tried
  in the same chain shape are flat between k=12 and k=20 (15–65 µs), IF included at
  149/285 µs. Had any of the other 15 shared the defect it would still be slow here.

The fix is one HashSet<usize> of positions where IF has already been ruled out — packrat
memoization of exactly that one decision. Its soundness rests on the outcome being a
function of the token stream (it is decided by parse_disjunction failing), so the memo is
dropped whenever the stream it describes changes: swapped out and back in
parse_data_type_from_text, cleared in expect_gt where a >> is rewritten in place.

The one thing that could have differed, checked rather than argued: a failing parse_if
attempt can touch self.pending_leading_comments on its way out, and a memoised skip skips
that too — so comment placement was the plausible behavioural difference. It is an
assignment rather than an append, which is why it comes out the same, but I diffed it instead
of reasoning about it: 30 comment-bearing IF statements (block and line comments,
before/after/between the IFs, in IF(...) argument position, and in WHERE/CASE
context) transpiled across Generic, PostgreSQL, Exasol, T-SQL and ClickHouse, plus the parsed
AST — 180 outputs, byte-identical between origin/main and this branch.
test_comments_around_a_ruled_out_if_are_unchanged pins twelve of them, including the
interior comments the parser drops, so it asserts unchanged rather than ideal.

4. TokenType::Eof — comparisons kept and covered

An earlier revision removed the five comparisons against the variant as dead code, on the
grounds that the tokenizer never constructs it. That was wrong, and @tobilg caught it:
Parser::new is public, so a caller can hand the parser a stream that ends with an Eof
token, and the comparisons are what make such a stream parse the same as one without it.
That commit is gone. Nothing about TokenType::Eof changes here except a doc comment
saying the tokenizer never emits it — so the next reader does not take the comparisons for
dead code either.

New explicit_eof_token_tests asserts the invariant rather than the examples: appending an
Eof token must not change the parse, checked against the same statement parsed without
one, over ten statements that reach all five comparison sites. Three sites are observable:

Input	without the comparisons	with them
SELECT BINARY	Unexpected token: Eof	parses
SELECT a OVERLAPS	Unexpected token: Eof	parses as an aliased projection
ALTER TABLE t UNSET prop	Raw { sql: "UNSET prop" }	UnsetProperty { properties: ["prop"] }

The other two are alternatives in parse_show's lists of clause-starting tokens to stop at.
TokenType::Eof is not in is_keyword(), so both scans stop at their trailing
else { break } either way — those two are behaviour-neutral, which is why a sweep over
SHOW forms shows no difference. Kept regardless, and now covered so they stay that way.

---

What this does not fix

• A trailing Eof is still not fully transparent. ALTER TABLE t UNSET PROJECTION POLICY yields Raw { sql: "UNSET PROJECTION POLICY " } — note the trailing space —
  because the scan that collects a multi-word UNSET clause stops at is_at_end and at
  ; but has no Eof comparison, so it takes the token as another word. Byte-identical on
  origin/main, so it predates this branch, and closing it would mean adding a sixth
  comparison rather than keeping five. test_a_raw_unset_clause_absorbs_an_explicit_eof
  pins it so it is visible rather than surprising. Happy to fix it here or in a follow-up.
• The oracle is empirical, not a type-level guarantee. skip() — 769 call sites —
  still does nothing at end of input, so loop { if check(X) { break } self.skip(); } would
  still spin. No such loop exists today (the oracle finds none), and making skip fallible
  would touch 769 sites for a hazard with no live instance, so I did not. The oracle is the
  mechanism covering it, which is why it is in --lib where CI runs it.
• Finding 3 is fixed at one site, not as a class. parse_if() has exactly one call
  site, so the cause behind this blowup is fully covered — and the keyword sweep above
  shows no other keyword retaining a superlinear path. But there are ~92 other
  self.current = saved backtrack points and none of them is memoized; I did not
  characterise them, only observed that the 16-keyword sweep does not reach them. General
  packrat memoization would be a parser-architecture change and is not attempted here. The
  budget in the oracle is what would catch the next one.
• Recursion depth is untouched. The IF chain is now fast, but depth is still linear in
  the input, so a long enough chain exhausts the stack rather than the clock — 12 kB of it
  overruns even the worker's 16 MiB. The watchdog now contains that (it is reported as a
  failed input rather than an aborted test run) but does not prevent it. That is the
  territory of your existing ComplexityGuardOptions (max_ast_depth and friends), and no
  guard option is added or changed here.
• peek and peek_text still return the last token at end of input. 102 and 168 sites.
  They do not move the cursor, so they cannot cause non-termination on their own, but a
  peek_text().eq_ignore_ascii_case("…") at end of input compares against a token that was
  already consumed, which can misparse rather than hang. Out of scope here; flagging it
  because the same fallback is what made finding 2 unbounded.
• The oracle is only as good as its corpus — 18 statements, 712 prefixes. It is a
  regression net for this shape of defect, not a proof of absence. A fuzzer found the
  original; the oracle is what keeps it from coming back.
• No in-parse work budget, and the existing guards cannot substitute for one.
  Every measurement above already ran through ComplexityGuardOptions::default() —
  Parser::parse_sql calls enforce_input, and the parser calls ensure_complexity_guards.
  The 75-byte input passes all of them comfortably: 75 bytes against a 16 MiB limit, ~51
  tokens against 1e6, zero parens, zero function calls; and max_ast_nodes / max_ast_depth
  are never reached because the parse errors and no AST is built. Every guard measures
  input size or output shape, and what is unbounded here is work. A work budget would
  be the backstop for the next superlinear path, but it would also reject valid input, so it
  is a design decision for you rather than something I should add.

Test results

cargo fmt --all -- --check — clean. cargo clippy -p polyglot-sql --lib --tests — no new
warnings from this branch.

make test-rust-verify, exit 0, every step green (fixtures already extracted):

step	result
Lib unit tests	1225 passed, 0 failed, 2 ignored
Generic identity	977/977
Dialect identity	4086/4086
Transpilation	6058/6058 (4 known failures)
Transpile generic	154/154
Parser	32/32
Pretty-print (release)	23/23
Custom dialect	276/276 identity, 347/347 transpilation
ClickHouse parser (release)	9417 parsed, 0 failed (9474 files, 57 skipped as non-UTF8/empty/out-of-scope)
ClickHouse coverage (release)	100% on every group
FFI	72 passed

The 2 ignored lib tests are one pre-existing diagnostic helper and the oracle's own worker,
which is #[ignore]d because the watchdog drives it as a subprocess rather than libtest
running it directly.

Also green: cargo test --test error_handling 62 passed. Each of the three commits was
checked on its own — 1219, 1222 and 1225 lib tests passing respectively, so the series
bisects cleanly.

Happy to split this into separate PRs, shrink it, or drop it.

---

🤖 Generated with Claude Code

[tobilg]

Thanks for the detailed investigation and the work on both parser paths. I reviewed revision 8237f82f458a96eaaabd3d35c1a7df8231b23d9e. Both underlying fixes worked in my checks: the malformed custom-type input now terminates with an error, and a 24-link malformed IF chain completed in approximately 3 ms in a debug probe.

The fallible advancement helpers and the negative-result cache look like a useful approach. I would keep that approach, with the following focused changes before merging.

1. Preserve handling of explicitly supplied EOF tokens

Although the tokenizer does not emit TokenType::Eof, callers can supply it through the public Parser::new(tokens) API. Removing the existing EOF checks changes that API's behavior.

Using tokenized SQL followed by an explicit TokenType::Eof, then calling parse_statement(), I reproduced:

Input	Before this PR	This PR
SELECT BINARY	Succeeds	Unexpected token: Eof
SELECT a OVERLAPS	Succeeds as an aliased projection	Unexpected token: Eof

Could we restore the existing EOF checks, retain the public enum variant, and add regression coverage for caller-supplied EOF tokens? Removing these checks is not necessary for either fix. Relevant paths: BINARY lookahead and OVERLAPS handling.

2. Make the timeout enforceable

The timeout helper limits how long the receiver waits, but does not stop the parser worker. If non-termination returns, the detached worker can continue consuming CPU and memory until the test process exits; a stack overflow could also abort that process.

Could we use a subprocess watchdog that kills and reaps a timed-out worker, or another enforceable execution budget? Please keep the coverage in an existing test file and in the --lib verification path. A more generous timeout would also make the tests less sensitive to loaded CI machines.

3. Assert the regression's result as well as its completion

The helper currently checks whether a result was received, but discards the success/failure value. Consequently, the malformed IF regression would also pass if the input were incorrectly accepted.

Could we assert that malformed chains both finish within the budget and return a parse error? Please also add focused checks for accepted IF forms, comment preservation, and representative unary +/- variants alongside ~. Returning the worker's result would avoid reparsing inputs outside the watchdog, as the custom-type regression currently does.

Verification and integration

On this revision, all 1,176 library tests passed (one additional test was ignored), all 62 error-handling integration tests passed, and formatting passed. I also found no output differences in 28 focused comparisons across Generic, Exasol, T-SQL, and ClickHouse. The relevant CI jobs are green. I have not run the full make test-rust-verify target locally for this review; that should be run after the revisions and again on the integrated result.

Please also add a closing reference for #446 alongside the existing Fixes #445. Both issues are reproducible parser defects and should remain open until the corrected implementation lands. These requests do not require a general parser rewrite or a new public work-budget API; addressing the focused compatibility and test-safety points above should put this in a good position to merge.

[geoHeil]

nice how our klankers are talking now ;) how should we proceed? will you want to take it over? should I tell mine to perform some refinements based on your feedback?

[tobilg]

Usally the PR would get updated upon the feedback, so it'd be great if you could eventually address the topics that have been mentioned in the comment. Thanks!

[geoHeil]

on it.

[geoHeil]

Thanks — all three are addressed, and point 1 was right in a way my own check had been too
narrow to see. The branch is also rebased onto b30c3d2, which moved ~1.6k lines of
parser.rs underneath it, and Fixes #446 is now in the description alongside Fixes #445.

1. EOF handling restored, and covered

That commit is dropped rather than patched: all five comparisons are back and nothing about
TokenType::Eof changes in this PR except a doc comment saying the tokenizer never emits
it — so the next reader does not take the comparisons for dead code either. My
"behaviour-neutral by construction" claim only held for streams Tokenizer produces, and
Parser::new is public, so it was the wrong claim to make.

New explicit_eof_token_tests in parser.rs asserts the invariant rather than the two
examples: appending an Eof token must not change the parse, checked against the same
statement parsed without one, over ten statements reaching all five comparison sites. Both
of your cases reproduce, and there is a third:

Input	without the comparisons	with them
SELECT BINARY	Unexpected token: Eof	parses
SELECT a OVERLAPS	Unexpected token: Eof	parses as an aliased projection
ALTER TABLE t UNSET prop	Raw { sql: "UNSET prop" }	UnsetProperty { properties: ["prop"] }

The other two sites are parse_show's lists of clause-starting tokens to stop at.
TokenType::Eof is not in is_keyword(), so both scans stop at their trailing
else { break } either way — genuinely behaviour-neutral, which is why a sweep over ten
SHOW forms shows no difference. Kept regardless, and now covered so they stay that way.

One gap I found writing that test and did not close. A trailing Eof is not fully
transparent today: ALTER TABLE t UNSET PROJECTION POLICY yields
Raw { sql: "UNSET PROJECTION POLICY " } — trailing space — because the scan that collects
a multi-word UNSET clause stops at is_at_end and at ; but has no Eof comparison, so
it takes the token as another word. Byte-identical on origin/main, so it predates this
branch, and closing it would mean adding a sixth comparison rather than keeping five.
test_a_raw_unset_clause_absorbs_an_explicit_eof pins it so it is visible rather than
surprising. Say the word and I will fix it here or in a follow-up.

2. The budget is enforced now

Replaced the detached thread with a subprocess watchdog. Each parse runs in a worker
process — this test binary re-executed with --exact … --ignored, gated on an env var —
which streams one decision per input back over a pipe. Over budget means kill() then
wait(), so nothing survives the failure, and a worker that exhausts its stack aborts
itself:

input	reported as
a parse that never returns	OverBudget, worker killed and reaped
a parse that exhausts 16 MiB of stack	Died("… exited with signal: 6 (SIGABRT)")
a parse that panics	Panicked
anything else	Parsed or Rejected, and the tests assert which

You were right that the stack case mattered, and it is also why the in-process version could
not just be patched. On origin/main the old oracle did not fail — it aborted the test
binary with fatal runtime error: stack overflow, taking the rest of the run with it. It
also blocked asserting an input that is meant to pass: ("IF~" * 24) + "1" parses
correctly, but 75 bytes of it exhausts a default libtest stack in a debug build, so it can
only be asserted from inside the watchdog. Hence the worker's explicit 16 MiB stack — the
figure CI already passes as RUST_MIN_STACK for the pretty-print and ClickHouse suites.

I verified the containment rather than assuming it: a 12 kB chain overruns even 16 MiB, and
comes back as Died("nothing more from a worker that exited with signal: 6 (SIGABRT)")
with the test process still running.

Budget is 5 s per input, up from 100 ms, with process start-up charged to a separate
120 s allowance so it is not part of any input's budget. A prefix in this corpus decides in
microseconds, so that is six orders of magnitude of slack for a loaded CI machine while
still catching a 38 s blowup.

Coverage stayed where you asked — termination_tests in parser.rs, an existing file in
the --lib path CI runs. On origin/main, with only that module added, every failure now
names its input and the run survives:

prefix "CREATE TABLE t (a VARCHAR2(" did not decide within 5s: OverBudget
a malformed 24-link IF chain should be Rejected within 5s: … left: OverBudget  right: Rejected
an unclosed custom type argument list should be Rejected within 5s: "SELECT a.:S1("
test result: FAILED. 2 passed; 3 failed; 1 ignored; finished in 5.02s

On this branch: 5 passed, 1 ignored (the worker itself, #[ignore]d since the watchdog
drives it), 0.04 s. A red run costs one budget, not one per remaining input, because the
sweep stops at the first input the worker does not survive.

3. Results asserted, and the extra cases added

decide_all returns the worker's outcome, so nothing is re-parsed outside the watchdog —
the custom-type regression no longer calls Parser::parse_sql a second time.

test	asserts
test_every_truncated_prefix_decides_within_budget	all 712 prefixes reach Parsed or Rejected — a Panicked, OverBudget or Died fails
test_unclosed_custom_type_args_are_rejected	Rejected, all five routes into parse_data_type
test_malformed_if_chains_are_rejected_within_budget	Rejected, 24-link chains through all three prefix-unary separators (~, +, -)
test_accepted_if_forms_still_parse	Parsed: IF(a,1,2), IF … THEN … ELSE … END, nested IF, IF as an identifier bare and dotted, ~IF / +IF / -IF, and ("IF~" * 24) + "1" — the chain with a tail that parses
test_comments_around_a_ruled_out_if_are_unchanged	twelve pinned outputs

On comments: the pinned values are what the parser produced before the memo existed,
including the interior comments it drops, so the test asserts unchanged rather than
ideal. I re-ran the full comparison against the new base — 30 comment-bearing IF
statements across Generic, PostgreSQL, Exasol, T-SQL and ClickHouse, plus the parsed AST:
180 outputs, byte-identical between origin/main and this branch.

The rebase

All 35 conflicts in parser.rs were the same shape — 0.10.0 added span tracking and the
identifier_from_token / parsed_column helpers exactly where this branch added a ? —
so each was resolved by taking your line and re-applying the ?. The compiler then found
the residue: three new self.advance() call sites in parse_id_var / parse_identifier,
and parse_mysql_numeric_identifier, which 0.10.0 also changed. That is the property the
signature change buys — a conflict resolution that drops a ? cannot compile. Recount on
the rebased tree: 497 call sites, 476 propagating with no judgement, 21 needing
a decision, still 0 new panic paths.

Verification

cargo fmt --all -- --check — clean. cargo clippy -p polyglot-sql --lib --tests — no new
warnings from this branch.

make test-rust-verify, exit 0, every step green (fixtures already extracted):

step	result
Lib unit tests	1225 passed, 0 failed, 2 ignored
Generic identity	977/977
Dialect identity	4086/4086
Transpilation	6058/6058 (4 known failures)
Transpile generic	154/154
Parser	32/32
Pretty-print (release)	23/23
Custom dialect	276/276 identity, 347/347 transpilation
ClickHouse parser (release)	9417 parsed, 0 failed (9474 files, 57 skipped as non-UTF8/empty/out-of-scope)
ClickHouse coverage (release)	100% on every group
FFI	72 passed

The 2 ignored lib tests are one pre-existing diagnostic helper and the oracle's own worker,
which is #[ignore]d because the watchdog drives it as a subprocess rather than libtest
running it directly.

Also green: cargo test --test error_handling 62 passed. Each of the three commits was
checked on its own — 1219, 1222 and 1225 lib tests passing respectively, so the series
bisects cleanly.

cargo fmt --all -- --check clean. Commits are three now: the two fixes, each with its own
tests, and the Eof coverage.

[tobilg]

Thanks, merged! There will need to be some follow-up work that was not covered by this PR.