Skip to content

bug: a SQL comment before or after a statement is a parse error (oracle accepts all five forms) #698

Description

@dpsiderius

Description

A SQL comment before or after a statement is a parse error. Only comments
inside a statement work.

Measured against the pinned 3.53.4 oracle (fed via stdin, because a leading
-- in argv is read as a CLI option by both binaries):

input oracle ours
-- c\nCREATE TABLE t(a); ok FAIL
/* c */ CREATE TABLE t(a); ok FAIL
CREATE TABLE t(a); -- c ok FAIL
CREATE TABLE t(a /* col */); ok ok
-- just a comment (nothing else) ok FAIL

So the tokenizer handles comments once it is inside a statement, but a
statement that begins with one, or trailing text after the terminating
semicolon, is rejected.

Why it matters beyond neatness

Every hand-written schema file, migration and fixture is commented. A
consumer that reads its DDL from a .sql file — which is the normal way to
ship a schema — cannot feed it to this crate. It also makes our own corpus
fixtures awkward: tests/corpus/fixtures/consumers/sqe/catalog.sql has a
header comment explaining what it is, and that header alone is enough to make
the file unusable through sqlite-rs exec.

split_statements (src/parser/tokenizer.rs:1184) already does the right
thing — it keeps a leading comment attached to the statement that follows it,
which is what SQLite does. The failure is downstream, in the statement
parsers, which expect the first token to be a keyword.

Likely shape of the fix

Comments are trivia, not syntax. Either the tokenizer should not emit them as
tokens at statement level, or each statement parser should skip leading trivia
before dispatching on the first keyword. The second is narrower but has to be
done in every entry point; the first is one place but needs checking against
the mid-statement cases that already work, so that CREATE TABLE t(a /* col */) does not regress.

A comment-only input should succeed as a no-op, matching the oracle, rather
than erroring — worth stating explicitly because "empty program" is an easy
thing to get wrong in the other direction.

Acceptance Criteria

  • All five rows in the table above match the oracle
  • tests/corpus/fixtures/consumers/sqe/catalog.sql runs through
    sqlite-rs exec verbatim, comments included — this un-#[ignore]s
    consumer_sqe_test::catalog_sql_file_runs_verbatim
  • Mid-statement comments still work (CREATE TABLE t(a /* col */),
    SELECT 1 /* x */ + 1)
  • A comment-only statement is a successful no-op, not an error
  • Comments inside string literals are still literal text, not comments
    (SELECT '-- not a comment')
  • Full suite green, make lint both clippy passes

Complexity

Estimate: small
Reasoning: Comments are already tokenized correctly for the
mid-statement case, and split_statements already groups them correctly, so
the change is skipping leading trivia at the parser entry points. The care is
in the two directions that must not regress: comments inside string literals
must stay literal, and a comment-only input must succeed rather than produce
an error or an invalid empty program.

Found while building spec 013 Requirement 6's SQE consumer fixture family: the
fixture's own header comment made it unusable.

Refs: 002/Req-1, 013/Req-6, #695

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions