diff --git a/.openspec/adr/0044-bind-parameter-indices-assigned-at-parse-time.md b/.openspec/adr/0044-bind-parameter-indices-assigned-at-parse-time.md new file mode 100644 index 00000000..07289184 --- /dev/null +++ b/.openspec/adr/0044-bind-parameter-indices-assigned-at-parse-time.md @@ -0,0 +1,87 @@ +# ADR-0044: Bind-parameter indices are assigned at parse time, in text order + +**Date:** 2026-09-10 +**Status:** Accepted + +## Context + +A bare `?` has no index written in the SQL; something has to assign one. +SQLite does this while parsing, in `sqlite3ExprAssignVarNumber`: a bare `?` +takes one more than the highest index used so far, and an explicit `?NNN` +raises that high-water mark. The index is therefore a property of the SQL +text. + +This crate assigned it during code generation instead, from a `next_param` +counter on `RegAlloc` (`src/codegen.rs`), read by `compile_value`. That made +the index a property of *compilation*, and two things about compilation broke +it: + +- **Codegen does not visit expressions in text order.** `compile_update` + compiles the `WHERE` operand before the `SET` assignments, because the scan + has to be positioned before the row body is emitted. So + `UPDATE t SET v = ? WHERE k = ?` numbered the `WHERE` placeholder 1 and the + `SET` placeholder 2, and a caller binding in text order had its values + swapped. +- **There is more than one `RegAlloc` per statement.** Eight sites call + `RegAlloc::new()`, each starting the counter at zero. A plan that compiles + part of a statement through a second allocator — the covering-index seek + path, for instance — restarted numbering mid-statement, collapsing two + distinct placeholders onto index 1. + +Both were silent. The swapped `UPDATE` matched no row and returned `Ok(0)`, +which is also the rows-affected value an optimistic-concurrency check reads +as "someone else won the race" — so a compare-and-swap built on it failed +100% of the time while looking like ordinary contention. Both were reported +by the first consumer to drive the embedding API with `?` rather than `?NNN`. + +Explicit `?NNN` was unaffected, which is why the existing parameter tests +missed it: they were written with `?1`/`?2`, the form this repository's own +code writes. `sqlx` — and most drivers — emit bare `?`. + +## Decision + +Assign the index in the parser, in text order, and carry it on the AST: +`ParamKind::Anonymous(u32)`. `Parser` holds one `next_param` high-water mark, +which is per-statement by construction because every parse entry point builds +its own `Parser` for one statement's tokens. `?NNN` raises the mark; a +following bare `?` continues past it. Codegen reads the index and no longer +owns a counter, so `RegAlloc::anonymous_param` and +`RegAlloc::numbered_param` are deleted along with the field they mutated. + +## Alternatives rejected + +**A numbering pass over the AST before codegen.** Leaves the AST shape and +the parser untouched, and would fix both reported cases. Rejected because it +needs a visitor that reaches every expression position in every statement +type — `SET`, `WHERE`, `VALUES`, projections, `JOIN ON`, `HAVING`, `LIMIT`, +subqueries, CTEs — and a position the visitor misses is not a compile error. +It is this same bug, silently, in a shape nobody has tested yet. Assigning at +the point of parse makes "was this numbered?" unrepresentable rather than +merely tested. + +**Refusing bare `?` at prepare, the way named parameters are refused.** +Provably safe and two lines. Rejected because bare `?` is what drivers +generate: refusing it does not protect a consumer, it excludes them. Refusing +named parameters is defensible because there is no index to bind them to; +here the index exists and was simply computed in the wrong place. + +**Keeping the counter in codegen and making `compile_update` visit the `SET` +list first.** Fixes the one reported statement and leaves the mechanism — +plan-order-dependent numbering across eight allocators — in place for the +next plan to trip over. + +## Consequences + +- `ParamKind::Anonymous` carries a `u32`. Five codegen match sites that + already accepted `Numbered(_)` alongside it needed `Anonymous(_)`; the + printer still renders `?`, because the source form is what it round-trips. +- Numbering no longer depends on which plan the optimizer chose. This is the + substantive gain: it was previously possible for the same SQL to number its + parameters differently after an unrelated planner change, with no test + failing. +- `Program::param_count()` (max `P1` over `Opcode::Variable`) becomes + trustworthy for bare `?`. It was reporting 1 for a two-placeholder + statement whenever the indices collapsed. +- Parse-time assignment means a statement that never reaches codegen still + has its parameters numbered. That is what SQLite does and it is what + `sqlite3_bind_parameter_count` reports after `prepare`. diff --git a/.openspec/adr/index.md b/.openspec/adr/index.md index ab53a47a..e9d972b2 100644 --- a/.openspec/adr/index.md +++ b/.openspec/adr/index.md @@ -45,3 +45,4 @@ Specs record what the system must do; ADRs record **why it is shaped this way** | [0039](0039-value-payloads-are-arc-not-rc.md) | `Value`'s text and blob payloads are `Arc`, not `Rc` | 2026-09-04 | | [0040](0040-streaming-execution-with-batch-as-wrapper.md) | One streaming execution primitive, with the batch path as its wrapper | 2026-09-01 | | [0041](0041-embedding-api-owns-the-connection-driver-out-of-tree.md) | The embedding API owns the connection; the `sqlx` driver stays out of tree | 2026-08-28 | +| [0044](0044-bind-parameter-indices-assigned-at-parse-time.md) | Bind-parameter indices are assigned at parse time, in text order | 2026-09-10 | diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a35d73e..29639d58 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,24 @@ All notable changes to sqlite-rs. Format follows [Keep a Changelog](https://keep **Versioning policy:** one minor version per completed plan phase — the version number tells the plan's story, sub-steps stay inside a phase. V1 (READ CORE) = 0.1.0 through 0.4.0. *(History note: internal iterations briefly numbered 0.4.0–0.6.0 were renumbered into the phase scheme on 14 Aug 2026, before any tag or publication of those versions existed.)* +## [Unreleased] + +### Fixed + +- Bare `?` bind parameters were numbered during code generation rather than + at parse time, so their indices depended on the plan the optimizer chose + instead of on the order they appear in the SQL. Two consequences, both + silent: `UPDATE t SET v = ? WHERE k = ?` numbered its `WHERE` placeholder + before its `SET` one and bound the two values in the wrong order, matching + no row and reporting `Ok(0)` — which is also the value an + optimistic-concurrency check reads as a lost race; and a projection of + index columns only, on a table with a usable index, compiled its seek keys + through a second register allocator whose counter restarted, collapsing + two placeholders onto index 1 and reporting one parameter where there were + two. Indices are now assigned in the parser in text order, as SQLite does + it, so they no longer depend on the plan (ADR-0044). Explicit `?NNN` was + never affected. + ## [0.18.10] - 2026-08-31 ### Fixed diff --git a/Cargo.toml b/Cargo.toml index 99457437..3e3814b2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -84,6 +84,10 @@ path = "tests/unit/pragma_synchronous_repl.rs" name = "unit_introspection_pragmas" path = "tests/unit/introspection_pragmas.rs" +[[test]] +name = "unit_param_numbering" +path = "tests/unit/param_numbering_test.rs" + [[test]] name = "unit_codegen" path = "tests/unit/codegen.rs" diff --git a/src/codegen.rs b/src/codegen.rs index 177185c8..5fd3d295 100644 --- a/src/codegen.rs +++ b/src/codegen.rs @@ -221,10 +221,6 @@ impl Emitter { #[derive(Debug)] pub(crate) struct RegAlloc { next: i32, - /// Next bind-parameter index to hand out for a bare `?` - /// (`ParamKind::Anonymous`) — 1-based, matching SQLite's - /// `sqlite3_bind_*` convention and `Opcode::Variable`'s `P1`. - next_param: u32, /// Next cursor number to hand out for a subquery's own scan (#238) — /// started well above every fixed cursor constant this compiler's /// other features use (`TABLE_CURSOR`/`SORT_CURSOR`/`PSEUDO_CURSOR`/ @@ -259,7 +255,6 @@ impl Default for RegAlloc { fn default() -> Self { RegAlloc { next: 0, - next_param: 0, next_cursor: 1000, materialized_ctes: Vec::new(), } @@ -319,21 +314,6 @@ impl RegAlloc { pub(crate) fn peek(&self) -> i32 { self.next } - - /// Assigns register-independent parameter index for a bare `?`, - /// incrementing past any `?NNN` index already claimed via - /// [`RegAlloc::numbered_param`]. - pub(crate) fn anonymous_param(&mut self) -> u32 { - self.next_param = self.next_param.saturating_add(1); - self.next_param - } - - /// Claims an explicit `?NNN` parameter index, advancing - /// `next_param` past it so a later bare `?` doesn't collide. - pub(crate) fn numbered_param(&mut self, n: u32) -> u32 { - self.next_param = self.next_param.max(n); - n - } } pub(crate) fn p4_coll_seq( diff --git a/src/codegen/expr/value.rs b/src/codegen/expr/value.rs index 75a779a5..3251b48a 100644 --- a/src/codegen/expr/value.rs +++ b/src/codegen/expr/value.rs @@ -222,8 +222,7 @@ pub(crate) fn compile_value( ExprKind::Param(kind) => { let r = reg.alloc(); let index = match kind { - ParamKind::Anonymous => Some(reg.anonymous_param()), - ParamKind::Numbered(n) => Some(reg.numbered_param(*n)), + ParamKind::Anonymous(n) | ParamKind::Numbered(n) => Some(*n), ParamKind::Colon(_) | ParamKind::At(_) | ParamKind::Dollar(_) => None, }; if let Some(index) = index { diff --git a/src/codegen/select/aggregate.rs b/src/codegen/select/aggregate.rs index 605b3690..c1e94671 100644 --- a/src/codegen/select/aggregate.rs +++ b/src/codegen/select/aggregate.rs @@ -94,7 +94,7 @@ where let is_supported_operand = matches!( &operand.kind, ExprKind::Literal(Literal::Integer(_)) - | ExprKind::Param(ParamKind::Anonymous | ParamKind::Numbered(_)) + | ExprKind::Param(ParamKind::Anonymous(_) | ParamKind::Numbered(_)) ); if !is_supported_operand { return Ok(false); diff --git a/src/codegen/select/join_order.rs b/src/codegen/select/join_order.rs index a560e8ac..f54398d5 100644 --- a/src/codegen/select/join_order.rs +++ b/src/codegen/select/join_order.rs @@ -444,7 +444,7 @@ mod tests { Vec::::new() ); let param = Expr { - kind: ExprKind::Param(ParamKind::Anonymous), + kind: ExprKind::Param(ParamKind::Anonymous(1)), span: span(), }; assert_eq!( diff --git a/src/codegen/select/limit_scan.rs b/src/codegen/select/limit_scan.rs index 720d5b73..11933b6b 100644 --- a/src/codegen/select/limit_scan.rs +++ b/src/codegen/select/limit_scan.rs @@ -120,7 +120,7 @@ fn is_supported_seek_operand(expr: &Expr) -> bool { matches!( &expr.kind, ExprKind::Literal(Literal::Integer(_)) - | ExprKind::Param(ParamKind::Anonymous | ParamKind::Numbered(_)) + | ExprKind::Param(ParamKind::Anonymous(_) | ParamKind::Numbered(_)) ) } diff --git a/src/codegen/select/range_scan.rs b/src/codegen/select/range_scan.rs index f33e832b..69d194d3 100644 --- a/src/codegen/select/range_scan.rs +++ b/src/codegen/select/range_scan.rs @@ -86,7 +86,7 @@ pub(super) fn is_supported_operand(expr: &Expr) -> bool { matches!( &expr.kind, ExprKind::Literal(Literal::Integer(_) | Literal::Float(_) | Literal::Str(_)) - | ExprKind::Param(ParamKind::Anonymous | ParamKind::Numbered(_)) + | ExprKind::Param(ParamKind::Anonymous(_) | ParamKind::Numbered(_)) ) } diff --git a/src/codegen/stmt/delete.rs b/src/codegen/stmt/delete.rs index 15dfa325..de5f1cec 100644 --- a/src/codegen/stmt/delete.rs +++ b/src/codegen/stmt/delete.rs @@ -106,7 +106,7 @@ pub fn compile_delete_with_catalog( matches!( &operand.kind, ExprKind::Literal(Literal::Integer(_)) - | ExprKind::Param(ParamKind::Anonymous | ParamKind::Numbered(_)) + | ExprKind::Param(ParamKind::Anonymous(_) | ParamKind::Numbered(_)) ) }); diff --git a/src/codegen/stmt/update.rs b/src/codegen/stmt/update.rs index cdd1feca..fa1909cc 100644 --- a/src/codegen/stmt/update.rs +++ b/src/codegen/stmt/update.rs @@ -173,7 +173,7 @@ pub fn compile_update_with_catalog( matches!( &operand.kind, ExprKind::Literal(Literal::Integer(_)) - | ExprKind::Param(ParamKind::Anonymous | ParamKind::Numbered(_)) + | ExprKind::Param(ParamKind::Anonymous(_) | ParamKind::Numbered(_)) ) }); diff --git a/src/parser/ast.rs b/src/parser/ast.rs index 36a21d75..2b65e88a 100644 --- a/src/parser/ast.rs +++ b/src/parser/ast.rs @@ -506,8 +506,17 @@ pub enum Literal { /// A bind parameter's form. #[derive(Debug, Clone, PartialEq)] pub enum ParamKind { - /// Bare `?`. - Anonymous, + /// Bare `?`, carrying the 1-based index assigned at parse time. + /// + /// SQLite assigns this during parsing (`sqlite3ExprAssignVarNumber`), + /// in the order the placeholders appear in the SQL text: a bare `?` + /// takes one more than the highest index used so far, and a `?NNN` + /// raises that high-water mark. Carrying the index here rather than + /// deriving it in codegen is load-bearing — codegen visits + /// expressions in *plan* order, not text order, and uses more than + /// one register allocator per statement, so a codegen-time counter + /// numbers the same SQL differently depending on the plan chosen. + Anonymous(u32), /// `?NNN`. Numbered(u32), /// `:name`. diff --git a/src/parser/grammar.rs b/src/parser/grammar.rs index b2f37820..b8078f0d 100644 --- a/src/parser/grammar.rs +++ b/src/parser/grammar.rs @@ -35,6 +35,11 @@ pub struct Parser { tokens: Vec, pos: usize, depth: usize, + /// Highest parameter index handed out so far, so a bare `?` can take + /// the next one in text order. Per-statement, which it is by + /// construction: every parse entry point builds its own `Parser` for + /// one statement's tokens. + next_param: u32, } /// Recursion-depth cap for `expr`/`not_expr`/`unary_expr`, so pathological @@ -59,6 +64,7 @@ impl Parser { tokens, pos: 0, depth: 0, + next_param: 0, } } @@ -2212,8 +2218,14 @@ impl Parser { TokenKind::Param(p) => { self.advance(); let kind = match *p { - Param::Anonymous => ParamKind::Anonymous, - Param::Numbered(n) => ParamKind::Numbered(n), + Param::Anonymous => { + self.next_param = self.next_param.saturating_add(1); + ParamKind::Anonymous(self.next_param) + } + Param::Numbered(n) => { + self.next_param = self.next_param.max(n); + ParamKind::Numbered(n) + } Param::Colon(s) => ParamKind::Colon(s), Param::At(s) => ParamKind::At(s), Param::Dollar(s) => ParamKind::Dollar(s), diff --git a/src/parser/printer.rs b/src/parser/printer.rs index 6a974b54..0f131e38 100644 --- a/src/parser/printer.rs +++ b/src/parser/printer.rs @@ -704,7 +704,7 @@ impl fmt::Display for Rollback { impl fmt::Display for ParamKind { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - ParamKind::Anonymous => write!(f, "?"), + ParamKind::Anonymous(_) => write!(f, "?"), ParamKind::Numbered(n) => write!(f, "?{n}"), ParamKind::Colon(s) => write!(f, ":{s}"), ParamKind::At(s) => write!(f, "@{s}"), diff --git a/tests/unit/param_numbering_test.rs b/tests/unit/param_numbering_test.rs new file mode 100644 index 00000000..73c7007f --- /dev/null +++ b/tests/unit/param_numbering_test.rs @@ -0,0 +1,171 @@ +// Copyright 2026 Schuberg Philis +// SPDX-License-Identifier: Apache-2.0 +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + +//! Bare `?` placeholders are numbered in the order they appear in the SQL +//! text, at parse time. +//! +//! This is `sqlite3ExprAssignVarNumber`'s contract, and it is not a +//! stylistic choice. The indices used to be handed out by codegen, from a +//! counter on `RegAlloc`, which produced two silent wrong answers a +//! consumer reported against the embedding API: +//! +//! - `UPDATE t SET v = ? WHERE k = ?` — `compile_update` compiles the +//! `WHERE` operand before the `SET` assignments, so the two placeholders +//! were numbered backwards and the bindings arrived swapped. The +//! statement matched nothing and reported `Ok(0)` — which is also the +//! optimistic-concurrency "lost the race" signal, so a compare-and-swap +//! built on it failed every time while looking like a live conflict. +//! - `SELECT FROM t WHERE a = ? AND b = ?` on a table +//! with a usable index — the covering-index plan compiles its seek keys +//! through a second `RegAlloc`, whose counter started at zero, so both +//! placeholders became index 1 and the statement reported wanting one +//! parameter when it had two. +//! +//! There were eight `RegAlloc::new()` sites, so the numbering depended on +//! both visit order and plan shape. Asserting on the AST rather than on a +//! compiled program is deliberate: it pins the property at the point where +//! it is now decided, so no future plan can reintroduce the divergence. + +use sqlite_rs::parser::ast::{Expr, ExprKind, InsertSource, ParamKind}; +use sqlite_rs::parser::{parse_delete, parse_insert, parse_select, parse_update, ParseOutcome}; + +/// Every parameter index in `expr`, in the order the walk finds them. +fn params(expr: &Expr) -> Vec { + let mut found = Vec::new(); + walk(expr, &mut found); + found +} + +fn walk(expr: &Expr, out: &mut Vec) { + match &expr.kind { + ExprKind::Param(ParamKind::Anonymous(n) | ParamKind::Numbered(n)) => out.push(*n), + ExprKind::Binary { lhs, rhs, .. } => { + walk(lhs, out); + walk(rhs, out); + } + ExprKind::Unary { expr, .. } => walk(expr, out), + ExprKind::Paren(inner) => walk(inner, out), + _ => {} + } +} + +fn update(sql: &str) -> sqlite_rs::parser::ast::Update { + match parse_update(sql) { + ParseOutcome::Accepted(stmt) => *stmt, + other => panic!("{sql} did not parse: {other:?}"), + } +} + +/// The reported defect, at the layer that caused it: `SET` is written +/// first, so `SET` takes index 1. +#[test] +fn an_update_numbers_set_before_where() { + let stmt = update("UPDATE t SET v = ? WHERE k = ?"); + + assert_eq!( + params(&stmt.assignments.first().unwrap().value), + vec![1], + "the SET placeholder is written first, so it is parameter 1" + ); + assert_eq!( + params(stmt.where_clause.as_ref().unwrap()), + vec![2], + "the WHERE placeholder is written second, so it is parameter 2 — \ + even though codegen compiles the WHERE first" + ); +} + +/// Several assignments and several WHERE terms, to pin the whole sequence +/// rather than just the two-placeholder case. +#[test] +fn a_wider_update_numbers_strictly_left_to_right() { + let stmt = update("UPDATE t SET a = ?, b = ?, c = ? WHERE d = ? AND e = ?"); + + let set: Vec = stmt + .assignments + .iter() + .flat_map(|a| params(&a.value)) + .collect(); + assert_eq!(set, vec![1, 2, 3]); + assert_eq!(params(stmt.where_clause.as_ref().unwrap()), vec![4, 5]); +} + +/// `?NNN` raises the high-water mark and a later bare `?` continues past +/// it, which is SQLite's rule. Mixing the forms is legal and this is the +/// case a per-expression counter got wrong in both directions. +#[test] +fn an_explicit_index_raises_the_high_water_mark() { + let stmt = update("UPDATE t SET a = ?, b = ?7, c = ? WHERE d = ?"); + + let set: Vec = stmt + .assignments + .iter() + .flat_map(|a| params(&a.value)) + .collect(); + assert_eq!( + set, + vec![1, 7, 8], + "a bare ? after ?7 takes 8, not 3 — one past the highest so far" + ); + assert_eq!(params(stmt.where_clause.as_ref().unwrap()), vec![9]); +} + +/// The same `?NNN` twice is one parameter, bound once, and must not +/// advance the counter twice. +#[test] +fn a_repeated_explicit_index_is_one_parameter() { + let stmt = update("UPDATE t SET a = ?1, b = ?1 WHERE c = ?"); + + let set: Vec = stmt + .assignments + .iter() + .flat_map(|a| params(&a.value)) + .collect(); + assert_eq!(set, vec![1, 1]); + assert_eq!(params(stmt.where_clause.as_ref().unwrap()), vec![2]); +} + +/// The covering-index shape from the second report. The numbering is a +/// parse-time property, so it holds regardless of which plan codegen then +/// picks — which is the whole point of moving it here. +#[test] +fn a_select_numbers_its_where_terms_left_to_right() { + let ParseOutcome::Accepted(stmt) = parse_select("SELECT a, b FROM t WHERE b = ? AND a = ?") + else { + panic!("did not parse"); + }; + assert_eq!(params(stmt.where_clause.as_ref().unwrap()), vec![1, 2]); +} + +#[test] +fn an_insert_numbers_its_values_left_to_right() { + let ParseOutcome::Accepted(stmt) = parse_insert("INSERT INTO t VALUES (?, ?, ?7, ?)") else { + panic!("did not parse"); + }; + let InsertSource::Values(rows) = &stmt.source else { + panic!("expected a VALUES source"); + }; + let row = rows.first().expect("one VALUES row"); + let seen: Vec = row.iter().flat_map(params).collect(); + assert_eq!(seen, vec![1, 2, 7, 8]); +} + +#[test] +fn a_delete_numbers_its_where_terms_left_to_right() { + let ParseOutcome::Accepted(stmt) = parse_delete("DELETE FROM t WHERE a = ? AND b = ?") else { + panic!("did not parse"); + }; + assert_eq!(params(stmt.where_clause.as_ref().unwrap()), vec![1, 2]); +} + +/// Numbering restarts for each statement, so two statements parsed through +/// the same entry point cannot inherit each other's counter. +#[test] +fn numbering_is_per_statement() { + for _ in 0..2 { + let stmt = update("UPDATE t SET v = ? WHERE k = ?"); + assert_eq!(params(&stmt.assignments.first().unwrap().value), vec![1]); + assert_eq!(params(stmt.where_clause.as_ref().unwrap()), vec![2]); + } +}