diff --git a/.openspec/specs/002-parser/spec.md b/.openspec/specs/002-parser/spec.md index 0df43d55..7a20c497 100644 --- a/.openspec/specs/002-parser/spec.md +++ b/.openspec/specs/002-parser/spec.md @@ -410,6 +410,14 @@ The tokenizer MUST convert SQL text into a stream of tokens. Each token MUST car **Tests:** `src/parser/tokenizer.rs::test_tokenize_parameters` +#### Scenario: A leading or trailing comment is trivia, not syntax + +- GIVEN `-- c\nCREATE TABLE t(a);`, `/* c */ CREATE TABLE t(a);`, `CREATE TABLE t(a); -- c`, or a comment-only input +- WHEN parsed +- THEN the comment is skipped and the statement parses (a comment-only input succeeds as a no-op), matching what the pinned oracle accepts + +**Tests:** `tests/corpus/comment_trivia_test.rs::leading_line_comment_before_statement_is_accepted`, `tests/corpus/comment_trivia_test.rs::leading_block_comment_before_statement_is_accepted`, `tests/corpus/comment_trivia_test.rs::trailing_line_comment_after_statement_is_accepted`, `tests/corpus/comment_trivia_test.rs::comment_only_input_is_a_successful_no_op`, `src/parser/tokenizer.rs::skip_leading_trivia_skips_whitespace_and_both_comment_styles` + ### Requirement 2: Grammar Compatibility [MUST] The parser MUST accept all SQL that SQLite accepts, and reject all SQL that SQLite rejects. diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a35d73e..b7e3b492 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,14 @@ All notable changes to sqlite-rs. Format follows [Keep a Changelog](https://keep ### Fixed +- A SQL comment before or after a statement (`-- c\nCREATE TABLE t(a);`, + `CREATE TABLE t(a); -- c`, or comment-only input) was a parse error, + even though `split_statements` already grouped a leading comment with + its statement — the statement dispatchers keyword-sniffed the raw + text and expected the first token to be a keyword. `codegen::dispatch` + and the `exec`/`query` CLI entry points now skip leading trivia via + `parser::skip_leading_trivia` before sniffing; comment-only input is a + successful no-op, matching the oracle (#698, spec 002 Req 1). - `hash_agg_find` allocated a fresh `Vec` key buffer and `Vec` key-values buffer per row. It now reuses `HashAggState`-held scratch buffers via take/give-back, cloning into `GroupSlot`/the index diff --git a/src/bin/sqlite-rs/exec.rs b/src/bin/sqlite-rs/exec.rs index 51d33ed5..cf0c3982 100644 --- a/src/bin/sqlite-rs/exec.rs +++ b/src/bin/sqlite-rs/exec.rs @@ -103,7 +103,7 @@ pub fn run_exec(path: &Path, sql: &str) -> ExitCode { /// any statement starting with `CREATE`/`DROP`/`ALTER` invalidates, /// even one that ends up failing or being a no-op. fn is_schema_changing(stmt: &str) -> bool { - let head = stmt.trim_start(); + let head = sqlite_rs::parser::skip_leading_trivia(stmt); ["CREATE", "DROP", "ALTER"].iter().any(|kw| { head.get(..kw.len()) .is_some_and(|h| h.eq_ignore_ascii_case(kw)) diff --git a/src/bin/sqlite-rs/query.rs b/src/bin/sqlite-rs/query.rs index a9a4744f..c328d006 100644 --- a/src/bin/sqlite-rs/query.rs +++ b/src/bin/sqlite-rs/query.rs @@ -192,8 +192,7 @@ pub fn run_query(raw_args: Vec) -> ExitCode { // point (`parse_explain`, grammar V4) rather than `parse_select` — // only checked when the statement actually starts with `EXPLAIN`, // so an ordinary `SELECT` never pays for the extra parse attempt. - let starts_with_explain = sql - .trim_start() + let starts_with_explain = sqlite_rs::parser::skip_leading_trivia(&sql) .get(..7) .is_some_and(|head| head.eq_ignore_ascii_case("explain")); let (select, eqp_mode) = if starts_with_explain { diff --git a/src/codegen/dispatch.rs b/src/codegen/dispatch.rs index d9c7f3ed..6b8b8186 100644 --- a/src/codegen/dispatch.rs +++ b/src/codegen/dispatch.rs @@ -13,6 +13,7 @@ use crate::parser::error::{ parse_create_view, parse_delete, parse_drop_index, parse_drop_table, parse_insert, parse_pragma, parse_rollback, parse_update, }; +use crate::parser::skip_leading_trivia; use crate::schema::{TableSchema, ViewSchema}; use crate::vdbe::Program; @@ -83,7 +84,8 @@ impl From for DispatchError { /// there. `compile_statement` below instead does its own borrowed, /// non-allocating scan for the hot dispatch path. pub fn leading_keywords(sql: &str) -> Vec { - sql.split_whitespace() + skip_leading_trivia(sql) + .split_whitespace() .take(3) .map(|w| w.to_ascii_uppercase()) .collect() @@ -142,7 +144,7 @@ pub fn compile_statement( .ok_or_else(|| DispatchError::NoSuchIndex(name.to_string())) }; - let mut words = sql.split_whitespace(); + let mut words = skip_leading_trivia(sql).split_whitespace(); let first_word = words.next().unwrap_or(""); let head = canonical(first_word); let second = canonical(words.next().unwrap_or("")); diff --git a/src/parser.rs b/src/parser.rs index 92ac2c36..9d30edbe 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -16,4 +16,4 @@ pub use error::{ parse_delete, parse_drop_index, parse_drop_table, parse_drop_view, parse_explain, parse_insert, parse_pragma, parse_rollback, parse_select, parse_update, ParseOutcome, }; -pub use tokenizer::{ends_with_semicolon, split_statements}; +pub use tokenizer::{ends_with_semicolon, skip_leading_trivia, split_statements}; diff --git a/src/parser/tokenizer.rs b/src/parser/tokenizer.rs index 224fcf24..b95e31c6 100644 --- a/src/parser/tokenizer.rs +++ b/src/parser/tokenizer.rs @@ -1179,8 +1179,8 @@ pub fn ends_with_semicolon(sql: &str) -> bool { matches!(last_real, Some(tok) if tok.kind == TokenKind::Semicolon) } -/// Empty statements (a bare `;`, leading/trailing whitespace-only) are -/// dropped, matching `sqlite3`'s own script handling. +/// Empty statements (a bare `;`, leading/trailing whitespace-only, or +/// comment-only) are dropped, matching `sqlite3`'s own script handling. pub fn split_statements(sql: &str) -> Vec { let tokens = Tokenizer::tokenize(sql); let mut statements = Vec::new(); @@ -1201,11 +1201,26 @@ pub fn split_statements(sql: &str) -> Vec { statements } +/// Skips leading whitespace and comments in `sql`, returning the rest of +/// the source starting at the first real token (or the empty string if +/// `sql` is nothing but trivia). Comments are trivia, not syntax — code +/// that keyword-sniffs a raw statement string (e.g. `codegen::dispatch`) +/// needs to look past a leading comment the same way the tokenizer does, +/// rather than treating the comment's own text as the "first word". +pub fn skip_leading_trivia(sql: &str) -> &str { + let offset = Tokenizer::tokenize(sql) + .first() + .map(|tok| tok.span.offset as usize) + .unwrap_or(0); + sql.get(offset..).unwrap_or("") +} + fn push_trimmed(statements: &mut Vec, slice: &str) { let trimmed = slice.trim(); - if !trimmed.is_empty() { - statements.push(trimmed.to_string()); + if trimmed.is_empty() || skip_leading_trivia(trimmed).is_empty() { + return; } + statements.push(trimmed.to_string()); } /// The remaining unconsumed source, from the given byte cursor. @@ -1256,6 +1271,45 @@ mod tests { assert_eq!(stmts, vec!["BEGIN", "ROLLBACK"]); } + #[test] + fn split_statements_keeps_a_leading_comment_attached_to_its_statement() { + // #698: `split_statements` already groups a leading comment with + // the statement that follows it (matching `sqlite3`'s own script + // handling) — the bug was downstream, in code that keyword-sniffs + // that returned text. This test pins the grouping behavior this + // ticket must not disturb. + let stmts = split_statements("-- c\nCREATE TABLE t(a);"); + assert_eq!(stmts, vec!["-- c\nCREATE TABLE t(a)"]); + } + + #[test] + fn split_statements_drops_a_comment_only_statement() { + // #698: a comment-only statement must not survive as a + // "statement" for the dispatcher to choke on — it's a no-op. + let stmts = split_statements("-- just a comment"); + assert!(stmts.is_empty()); + + let stmts = split_statements("BEGIN; /* comment only */; COMMIT"); + assert_eq!(stmts, vec!["BEGIN", "COMMIT"]); + } + + #[test] + fn skip_leading_trivia_skips_whitespace_and_both_comment_styles() { + assert_eq!( + skip_leading_trivia("-- c\nCREATE TABLE t(a)"), + "CREATE TABLE t(a)" + ); + assert_eq!( + skip_leading_trivia("/* c */ CREATE TABLE t(a)"), + "CREATE TABLE t(a)" + ); + assert_eq!( + skip_leading_trivia(" CREATE TABLE t(a)"), + "CREATE TABLE t(a)" + ); + assert_eq!(skip_leading_trivia("-- only a comment"), ""); + } + #[test] fn ends_with_semicolon_true_for_a_trailing_top_level_semicolon() { assert!(ends_with_semicolon("SELECT * FROM t;")); diff --git a/tests/corpus/comment_trivia_test.rs b/tests/corpus/comment_trivia_test.rs new file mode 100644 index 00000000..ecfea052 --- /dev/null +++ b/tests/corpus/comment_trivia_test.rs @@ -0,0 +1,201 @@ +// Copyright 2026 Schuberg Philis +// SPDX-License-Identifier: Apache-2.0 +//! Oracle-diffed regression tests for #698: a SQL comment before or +//! after a statement must not be a parse error — comments are trivia, +//! not syntax, and the tokenizer already skips them everywhere except +//! at the statement-dispatch boundary (`codegen::dispatch`'s raw +//! keyword-sniffing, and `split_statements`'s comment-only-statement +//! handling). +//! +//! The oracle is invoked via stdin, not argv: a leading `--` in an argv +//! element is read as a CLI option by `sqlite3` itself (confirmed: +//! `sqlite3 db.db "-- c\nCREATE TABLE t(a)"` fails with `Error: unknown +//! option: - c` before it ever reaches the SQL parser), independent of +//! how the argument reached the process. `sqlite-rs exec`'s own +//! argument parsing is purely positional (`src/bin/sqlite-rs/main.rs`), +//! so it has no such restriction and is fed the same text via argv. + +use std::io::Write; +use std::path::{Path, PathBuf}; +use std::process::{Command, Output, Stdio}; +use std::sync::atomic::{AtomicU64, Ordering}; + +use crate::oracle::{pinned_oracle, skip_no_oracle}; + +const CLI: &str = env!("CARGO_BIN_EXE_sqlite-rs"); + +fn scratch_db(label: &str) -> PathBuf { + static COUNTER: AtomicU64 = AtomicU64::new(0); + let n = COUNTER.fetch_add(1, Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!( + "sqlite-rs-comment-trivia-{label}-{}-{n}", + std::process::id() + )); + std::fs::remove_dir_all(&dir).ok(); + std::fs::create_dir_all(&dir).unwrap(); + dir.join("scratch.db") +} + +fn run_exec(db: &Path, sql: &str) -> Output { + Command::new(CLI) + .arg("exec") + .arg(db) + .arg(sql) + .output() + .unwrap_or_else(|e| panic!("running {CLI} exec {} {sql:?}: {e}", db.display())) +} + +fn run_query(db: &Path, sql: &str) -> Output { + Command::new(CLI) + .arg("query") + .arg(db) + .arg(sql) + .output() + .unwrap_or_else(|e| panic!("running {CLI} query {} {sql:?}: {e}", db.display())) +} + +/// Runs `sql` against a fresh oracle-created `db` via stdin (never +/// argv — see the module doc for why), returning stdout as text. +fn oracle_via_stdin(oracle: &Path, db: &Path, sql: &str) -> String { + let mut child = Command::new(oracle) + .arg(db) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap_or_else(|e| panic!("spawning oracle on {}: {e}", db.display())); + child + .stdin + .take() + .unwrap() + .write_all(sql.as_bytes()) + .unwrap(); + let output = child.wait_with_output().unwrap(); + assert!( + output.status.success(), + "oracle rejected {sql:?}: {}", + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8_lossy(&output.stdout).into_owned() +} + +/// Asserts our `exec` accepts `sql` and, when the pinned oracle is +/// available, that the two engines agree on the resulting stored DDL +/// text for `t` (`expected_ddl`, e.g. `"CREATE TABLE t(a)"` — a +/// statement's own leading/trailing comment must not leak into the +/// stored text, but a *mid*-statement comment, being part of the +/// statement's own span, must). +fn assert_accepted_and_matches_oracle(label: &str, sql: &str, expected_ddl: &str) { + let db = scratch_db(label); + let output = run_exec(&db, sql); + assert!( + output.status.success(), + "{label}: our exec rejected {sql:?}: {}", + String::from_utf8_lossy(&output.stderr) + ); + let our_schema = Command::new(CLI) + .arg("dump") + .arg(&db) + .output() + .unwrap_or_else(|e| panic!("running {CLI} dump {}: {e}", db.display())); + assert!(our_schema.status.success()); + let our_schema = String::from_utf8_lossy(&our_schema.stdout).into_owned(); + assert!( + our_schema.contains(expected_ddl), + "our schema for {label}: {our_schema:?}" + ); + + let Some(oracle) = pinned_oracle() else { + skip_no_oracle(label); + return; + }; + let oracle_db = scratch_db(&format!("{label}-oracle")); + let oracle_schema = oracle_via_stdin(&oracle, &oracle_db, &format!("{sql}\n.schema")); + // `.schema`'s own trailing `;` differs from `dump`'s; compare only + // the table's actual DDL text, which is what #698 is about (whether + // a comment leaks into or blocks the statement), not dump-format + // fidelity (covered by `dump_oracle_test.rs`). + assert!( + oracle_schema.contains(expected_ddl), + "oracle schema for {label}: {oracle_schema:?}" + ); +} + +#[test] +fn leading_line_comment_before_statement_is_accepted() { + assert_accepted_and_matches_oracle( + "leading-line", + "-- c\nCREATE TABLE t(a);", + "CREATE TABLE t(a)", + ); +} + +#[test] +fn leading_block_comment_before_statement_is_accepted() { + assert_accepted_and_matches_oracle( + "leading-block", + "/* c */ CREATE TABLE t(a);", + "CREATE TABLE t(a)", + ); +} + +#[test] +fn trailing_line_comment_after_statement_is_accepted() { + assert_accepted_and_matches_oracle("trailing", "CREATE TABLE t(a); -- c", "CREATE TABLE t(a)"); +} + +#[test] +fn mid_statement_comment_still_works() { + assert_accepted_and_matches_oracle( + "mid-statement", + "CREATE TABLE t(a /* col */);", + "CREATE TABLE t(a /* col */)", + ); +} + +#[test] +fn comment_only_input_is_a_successful_no_op() { + let db = scratch_db("comment-only"); + let output = run_exec(&db, "-- just a comment"); + assert!( + output.status.success(), + "comment-only input should be a no-op, not an error: {}", + String::from_utf8_lossy(&output.stderr) + ); + + if let Some(oracle) = pinned_oracle() { + let oracle_db = scratch_db("comment-only-oracle"); + // Succeeding at all (no error/panic from `wait_with_output`'s + // status assertion) is the acceptance bar here. + oracle_via_stdin(&oracle, &oracle_db, "-- just a comment"); + } else { + skip_no_oracle("comment_only_input_is_a_successful_no_op (oracle cross-check)"); + } +} + +#[test] +fn comment_like_text_inside_a_string_literal_stays_literal() { + let db = scratch_db("string-literal"); + assert!(run_exec(&db, "CREATE TABLE t(a)").status.success()); + + let output = run_query(&db, "SELECT '-- not a comment'"); + assert!( + output.status.success(), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!( + String::from_utf8_lossy(&output.stdout), + "-- not a comment\n" + ); + + if let Some(oracle) = pinned_oracle() { + let oracle_db = scratch_db("string-literal-oracle"); + let oracle_out = oracle_via_stdin(&oracle, &oracle_db, "SELECT '-- not a comment';"); + assert_eq!(oracle_out, "-- not a comment\n"); + } else { + skip_no_oracle( + "comment_like_text_inside_a_string_literal_stays_literal (oracle cross-check)", + ); + } +} diff --git a/tests/corpus/main.rs b/tests/corpus/main.rs index e0a60865..a80be8c6 100644 --- a/tests/corpus/main.rs +++ b/tests/corpus/main.rs @@ -25,6 +25,7 @@ mod btree_insert_test; mod btree_test; mod cli_e2e_test; mod cli_write_test; +mod comment_trivia_test; mod crash_torture_test; mod cte_test; mod declared_collate_test;