Reject incomplete SQL and preserve lexical errors - #322
Merged
Conversation
## Problem
ParseStmts checks the byte offset before consuming the final token, and
SELECT permits an empty projection at EOF. Ignored token errors and failed
lookahead can also discard lexical failures or replace them with a later
grammar error. The shared sign branch incorrectly recognizes +> as ->.
## Reproduction
```go
parser.NewParser("SELECT").ParseStmts() // succeeds with no statements
parser.NewParser("SELECT 1; SELECT").ParseStmts() // loses the final SELECT
parser.NewParser("SELECT 1 /*").ParseStmts() // accepts an unclosed comment
parser.NewParser("SELECT 1 +> 2").ParseStmts() // formats +> as ->
```
## Fix
Retain the first lexical error and its byte position outside cursor
checkpoints, restore lookahead on every outcome, and return the original
failure from ParseStmts. Test token exhaustion, require a SELECT
expression, and recognize an arrow only when it starts with a minus.
Public APIs and existing golden output are unchanged.
## Test
Add regressions for final-token boundaries, lexical failures across
lookahead/backtracking, original error locations, and cursor restoration.
The focused tests failed before the fix. Add AST and formatting fixtures
for valid arrow syntax and a final statement without a semicolon.
Validate the reproductions with ClickHouse 26.7.1.1315. A separate probe
also rejects malformed suffixes in 684 variations of the existing SQL
fixtures. `make`, `make test`, and `make lint` pass with Go 1.21.13 and
the CI-pinned golangci-lint v1.53.3. Local macOS verification uses the
external linker and an ad-hoc signature for the temporary linter binary.
git-hulk
changed the title
Replace the separate lexer error and position fields with a private lexerError wrapper. Capture comment positions at the error source and preserve already-positioned failures when token consumption returns. Extend the lookahead regression to inspect the returned error position. Build, lint, and the full race/compatibility suite pass with Go 1.21.13.
Remove the duplicate lexer error check after parseStmt. The next loop iteration always calls consumeToken before checking EOF, so retained lexical failures already return through that path. wrapError preserves the lexical cause when the grammar returns a secondary error first.
Return (*Token, error) from tryConsumeTokenKind and check the error in expectTokenKind and every optional-token caller. Preserve non-matching and EOF behavior while returning lexical failures at consumption. Add regressions for direct error propagation, original error positions, non-matching tokens, and final-token consumption. make test, make lint, and make pass with Go 1.21.13 and the CI-pinned linter.
Remove Lexer.err and propagate consumption and lookahead failures through parser helpers. Keep error positions in returned lexerError values and retry only grammar errors during backtracking. Preserve keyword mismatch rollback and normal EOF. Cover direct keyword, lookahead, and backtracking errors, plus scanning valid input after a failed scan is rewound. make test, make lint, and make pass with Go 1.21.13 and the CI-pinned linter; 684 malformed fixture suffixes are rejected.
This reverts commit 03b5f95.
git-hulk
marked this pull request as ready for review
September 10, 2026 05:51
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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
1 participant
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.
Problem
ParseStmts checks the byte offset before consuming the final token, and
SELECT permits an empty projection at EOF. Ignored token errors and failed
lookahead can also discard lexical failures or replace them with a later
grammar error. The shared sign branch incorrectly recognizes +> as ->.
Reproduction
Fix
Retain the first lexical failure in a private lexerError that wraps its
cause and byte position outside cursor checkpoints. Restore lookahead on
every outcome and return the original failure through ParseError. Return
errors from tryConsumeTokenKind and propagate them through expectTokenKind
and every optional-token caller. Preserve no-match and normal EOF behavior.
Test token exhaustion, require a SELECT expression, and recognize an arrow
only when it starts with a minus.
Public APIs and existing golden output are unchanged.
Test
Add regressions for final-token boundaries, lexical failures across
lookahead/backtracking, original error locations, and cursor restoration.
Check direct error propagation through required tokens, optional tokens,
and list separators without relying on wrapError. The focused tests failed
before the fix. Add AST and formatting fixtures
for valid arrow syntax and a final statement without a semicolon.
Validate the reproductions with ClickHouse 26.7.1.1315. A separate probe
also rejects malformed suffixes in 684 variations of the existing SQL
fixtures.
make,make test, andmake lintpass with Go 1.21.13 andthe CI-pinned golangci-lint v1.53.3. Local macOS verification uses the
external linker and an ad-hoc signature for the temporary linter binary.