Surface lexer errors instead of misreporting them as EOF#281
Closed
git-hulk wants to merge 3 commits into
Closed
Conversation
Dozens of parser call sites discard consumeToken's error
(`_ = p.lexer.consumeToken()`). When the lexer failed mid-statement
(invalid number, unterminated string, unclosed quoted identifier), the
current token stayed nil and the parser read that as end of input,
producing misleading errors:
SELECT 0x => line 1:8 unexpected token kind: <eof>
or, worse, silent success when the statement happened to look complete
at the point of failure: `SELECT 1 'oops` parsed cleanly as SELECT 1.
Rather than threading error returns through every call site, the lexer
now records its first failure (error + offset) in lexerState:
- consumeToken stashes the error; the existing save/restore backtracking
clears failures that were only hit during rolled-back lookahead, since
the fields live in lexerState.
- wrapError prefers the stashed lexer error over whatever downstream
symptom the parser tripped on, reporting the real cause at the offset
it happened:
SELECT 0x => line 1:8 invalid number
- Statements that previously succeeded despite a trailing lexer error
now fail via the re-lex at the top of the ParseStmts loop.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82319d4a9c
ℹ️ 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".
CI's golangci-lint predates Go 1.21 builtins and fails typecheck on min() even though go.mod declares go 1.21. Clamp with a plain comparison instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
aftership-abas
suggested changes
Jul 6, 2026
aftership-abas
left a comment
aftership-abas
left a comment
There was a problem hiding this comment.
Changes Requested. The main correctness risk is already covered by the existing inline thread: recorded lexer failures still need to be checked before accepting an otherwise successful parse, so malformed trailing input cannot be silently accepted. I reviewed the lexer/parser error propagation changes, the regression tests, existing PR feedback, and CI status; I did not run local tests because CI owns automated checks for this flash review.
Review follow-up: lastError was only consulted through wrapError, which only runs when the parser itself fails. A failed lex can leave the offset advanced - consumeIdent skips the opening backtick before discovering the quote is unclosed - so with `SELECT 1 `bad` the retained input re-lexes cleanly to EOF, parsing reaches the success path, and the recorded error was silently dropped. - ParseStmts now fails if lexerState still carries an error when parsing reaches end of input. - peekToken restores the saved state on the error path too, so a failed lookahead neither leaves the offset advanced nor records an error for input the parser has not actually reached. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Dozens of parser call sites discard
consumeToken's error (_ = p.lexer.consumeToken()). When the lexer failed mid-statement, the current token stayedniland the parser read that as end of input:SELECT 0xline 1:8 unexpected token kind: <eof>line 1:8 invalid numberSELECT 'unterminatedinvalid stringSELECT \unclosed`unclosed quoted identifierSELECT 1 'oopsSELECT 1invalid stringSELECT 1; SELECT 'bad; SELECT 2invalid stringApproach
Rather than threading error returns through every call site (huge churn), the lexer records its first failure + offset in
lexerState:consumeTokenis now a thin wrapper that stashes the error before returning it.lexerState, the existingsaveState/restoreStatebacktracking automatically clears failures that were only hit during rolled-back lookahead — no false positives on successful parses.wrapErrorprefers the stashed lexer error over the downstream symptom the parser tripped on, reporting the real cause at the offset it happened (correct line/column/caret).ParseStmtsloop.No golden fixture changed; new tests in
error_test.gocover the messages, the reported position, and lookahead rollback.Complements #280 (which makes the lexer produce errors for unterminated comments / non-ASCII bytes; this PR makes the parser report them).
🤖 Generated with Claude Code