feat: bring simple-mode insert arity diagnostics to parity with advanced
A wrong-count simple-mode insert now shows the friendly per-column arity message at typing time (instead of a bare "expected `,`/`)`") and is blocked from dispatch at submit — unifying simple and advanced mode onto the one ADR-0027 model (structural parse + ERROR diagnostic), where they had diverged. Grammar: a simple-mode-only arity gate (dsl_insert_value_list) routes a wrong-count DSL insert tuple to the type-blind fallback so it matches structurally and the per-tuple arity diagnostic fires. The gate is gated to simple mode, so advanced behaviour is unchanged. count_tuple_values and the target-column selection (insert_target_columns) are now shared by both grammars. Diagnostic: dml_insert_arity_diagnostics is mode-aware — advanced Form B expects all columns; simple Form B/C expects the user-fillable columns (serial/shortid auto-fill). It counts the DSL Form A role and scans the keyword-less Form C tuple. New catalog keys name the fillable/auto split and the all-auto-table case. Submit: a wrong-count DSL insert now parses Ok + carries the ERROR diagnostic, so a unified Ok-arm pre-flight (dsl_insert_count_mismatch_notes) blocks dispatch and teaches; the previous Err-arm note retires. advanced_alternative_note's gate now reads the validity verdict so it still fires for the parse-Ok-with-error shape. Docs: ADR-0036 Amendment 2 (+ README index) and requirements.md H1a.
This commit is contained in:
claude@clouddev1
+317
-43
@@ -1465,10 +1465,16 @@ fn dml_auto_column_diagnostics(
|
||||
fn dml_insert_arity_diagnostics(
|
||||
path: &MatchedPath,
|
||||
schema: Option<&crate::completion::SchemaCache>,
|
||||
mode: crate::mode::Mode,
|
||||
) -> Vec<outcome::Diagnostic> {
|
||||
use crate::dsl::grammar::IdentSource;
|
||||
use crate::dsl::types::Type;
|
||||
use outcome::{Diagnostic, MatchedKind, Severity};
|
||||
|
||||
// Form A column count: the explicit `(col, …)` list. The SQL
|
||||
// (advanced) grammar tags these `insert_column`; the DSL (simple)
|
||||
// grammar tags them `insert_first_item` (issue #17) — count both so
|
||||
// a DSL Form A insert isn't mis-classified as Form B.
|
||||
let col_arity = path
|
||||
.items
|
||||
.iter()
|
||||
@@ -1477,31 +1483,70 @@ fn dml_insert_arity_diagnostics(
|
||||
it.kind,
|
||||
MatchedKind::Ident {
|
||||
source: IdentSource::Columns,
|
||||
role: "insert_column",
|
||||
role: "insert_column" | "insert_first_item",
|
||||
}
|
||||
)
|
||||
})
|
||||
.count();
|
||||
|
||||
// Resolve the expected arity + which message template to use.
|
||||
// Form A: explicit column list → its own length.
|
||||
// Form B: no list → the target table's column count, *if* we know
|
||||
// it. Without a schema or a recognised target the pass goes
|
||||
// silent (the unknown-table case is flagged by the schema-
|
||||
// existence pass instead).
|
||||
let (expected, message_key): (usize, &'static str) = if col_arity > 0 {
|
||||
(col_arity, "diagnostic.insert_arity_mismatch")
|
||||
} else {
|
||||
let Some(schema) = schema else {
|
||||
return Vec::new();
|
||||
};
|
||||
let Some(target) = path.items.iter().find_map(|it| match it.kind {
|
||||
// Is this an INSERT? `into` is insert-exclusive in both grammars
|
||||
// (update uses `set`, delete `from`, show `data`), so it tells the
|
||||
// DSL insert apart from other commands that also tag a `table_name`.
|
||||
let is_insert = path
|
||||
.items
|
||||
.iter()
|
||||
.any(|it| matches!(it.kind, MatchedKind::Word("into")));
|
||||
// Insert target table. The SQL grammar tags it `insert_target_table`
|
||||
// (insert-specific); the DSL grammar reuses the generic `table_name`
|
||||
// role, so only trust that when `into` confirmed an insert (issue
|
||||
// #17).
|
||||
let target_table: Option<&str> = path
|
||||
.items
|
||||
.iter()
|
||||
.find_map(|it| match it.kind {
|
||||
MatchedKind::Ident {
|
||||
source: IdentSource::Tables,
|
||||
role: "insert_target_table",
|
||||
} => Some(it.text.as_str()),
|
||||
MatchedKind::Ident {
|
||||
source: IdentSource::Tables,
|
||||
role: "table_name",
|
||||
} if is_insert => Some(it.text.as_str()),
|
||||
_ => None,
|
||||
}) else {
|
||||
});
|
||||
|
||||
// Resolve the expected arity + a message builder. The builder
|
||||
// captures the per-case args because the message key (and its
|
||||
// placeholders) differ by form and mode.
|
||||
//
|
||||
// - **Form A** (both modes): the listed-column count, mode-neutral.
|
||||
// - **Advanced Form B**: every column needs a value (auto-fills
|
||||
// nothing, ADR-0036) → the full table count.
|
||||
// - **Simple Form B** (issue #17): the dispatch auto-fills
|
||||
// serial/shortid (ADR-0018 §3), so only the user-fillable columns
|
||||
// take values. When some columns are skipped the message names
|
||||
// both sets; when none are skipped the count equals the table's
|
||||
// and the plain Form B wording is accurate; when *every* column is
|
||||
// auto-generated no value belongs at all.
|
||||
type MsgFn<'a> = Box<dyn Fn(usize) -> String + 'a>;
|
||||
let (expected, make_message): (usize, MsgFn) = if col_arity > 0 {
|
||||
(
|
||||
col_arity,
|
||||
Box::new(move |actual| {
|
||||
crate::friendly::translate(
|
||||
"diagnostic.insert_arity_mismatch",
|
||||
&[
|
||||
("expected", &col_arity as &dyn std::fmt::Display),
|
||||
("actual", &actual as &dyn std::fmt::Display),
|
||||
],
|
||||
)
|
||||
}),
|
||||
)
|
||||
} else {
|
||||
let Some(schema) = schema else {
|
||||
return Vec::new();
|
||||
};
|
||||
let Some(target) = target_table else {
|
||||
return Vec::new();
|
||||
};
|
||||
let Some(cols) = schema.table_columns.get(target) else {
|
||||
@@ -1510,35 +1555,123 @@ fn dml_insert_arity_diagnostics(
|
||||
if cols.is_empty() {
|
||||
return Vec::new();
|
||||
}
|
||||
(cols.len(), "diagnostic.insert_arity_mismatch_form_b")
|
||||
let is_auto = |t: Type| matches!(t, Type::Serial | Type::ShortId);
|
||||
let fillable: Vec<&str> = cols
|
||||
.iter()
|
||||
.filter(|c| !is_auto(c.user_type))
|
||||
.map(|c| c.name.as_str())
|
||||
.collect();
|
||||
if mode == crate::mode::Mode::Advanced || fillable.len() == cols.len() {
|
||||
// Advanced Form B (all columns), or simple Form B over a
|
||||
// table with no auto-generated columns — "all N" is accurate.
|
||||
let expected = if mode == crate::mode::Mode::Advanced {
|
||||
cols.len()
|
||||
} else {
|
||||
fillable.len()
|
||||
};
|
||||
(
|
||||
expected,
|
||||
Box::new(move |actual| {
|
||||
crate::friendly::translate(
|
||||
"diagnostic.insert_arity_mismatch_form_b",
|
||||
&[
|
||||
("expected", &expected as &dyn std::fmt::Display),
|
||||
("actual", &actual as &dyn std::fmt::Display),
|
||||
],
|
||||
)
|
||||
}),
|
||||
)
|
||||
} else if fillable.is_empty() {
|
||||
// Simple Form B, every column auto-generated → no value
|
||||
// belongs at all.
|
||||
let table = target.to_string();
|
||||
(
|
||||
0,
|
||||
Box::new(move |actual| {
|
||||
crate::friendly::translate(
|
||||
"diagnostic.insert_arity_mismatch_all_auto",
|
||||
&[
|
||||
("table", &table as &dyn std::fmt::Display),
|
||||
("actual", &actual as &dyn std::fmt::Display),
|
||||
],
|
||||
)
|
||||
}),
|
||||
)
|
||||
} else {
|
||||
// Simple Form B with a mix — name the user-fillable and the
|
||||
// auto-generated columns so the learner understands the skip.
|
||||
let expected = fillable.len();
|
||||
let columns = fillable
|
||||
.iter()
|
||||
.map(|n| format!("`{n}`"))
|
||||
.collect::<Vec<_>>()
|
||||
.join(", ");
|
||||
let skipped = cols
|
||||
.iter()
|
||||
.filter(|c| is_auto(c.user_type))
|
||||
.map(|c| format!("`{}`", c.name))
|
||||
.collect::<Vec<_>>()
|
||||
.join(", ");
|
||||
(
|
||||
expected,
|
||||
Box::new(move |actual| {
|
||||
crate::friendly::translate(
|
||||
"diagnostic.insert_arity_mismatch_form_b_simple",
|
||||
&[
|
||||
("expected", &expected as &dyn std::fmt::Display),
|
||||
("columns", &columns as &dyn std::fmt::Display),
|
||||
("skipped", &skipped as &dyn std::fmt::Display),
|
||||
("actual", &actual as &dyn std::fmt::Display),
|
||||
],
|
||||
)
|
||||
}),
|
||||
)
|
||||
}
|
||||
};
|
||||
|
||||
// Index of the row-source keyword (first VALUES / SELECT / WITH).
|
||||
let Some(kw_idx) = path
|
||||
// Locate the row source to scan. Form A/B and INSERT…SELECT carry a
|
||||
// `values`/`select`/`with` keyword. The DSL Form C (`insert into T
|
||||
// (1, 2)` — bare value list, no `values`) has none: its single value
|
||||
// tuple follows the target table directly, with no column-name list
|
||||
// (col_arity == 0 here), so scan from just after the table and treat
|
||||
// it with the VALUES tuple logic (issue #17).
|
||||
let (kw, tail): (&str, &[outcome::MatchedItem]) = if let Some(kw_idx) = path
|
||||
.items
|
||||
.iter()
|
||||
.position(|it| matches!(it.kind, MatchedKind::Word("values" | "select" | "with")))
|
||||
else {
|
||||
return Vec::new();
|
||||
};
|
||||
let MatchedKind::Word(kw) = path.items[kw_idx].kind else {
|
||||
{
|
||||
let MatchedKind::Word(kw) = path.items[kw_idx].kind else {
|
||||
return Vec::new();
|
||||
};
|
||||
(kw, &path.items[kw_idx + 1..])
|
||||
} else if is_insert && mode == crate::mode::Mode::Simple {
|
||||
// DSL Form C (`insert into T (1, 2)` — bare value list, no
|
||||
// `values` keyword) is simple-mode only; advanced SQL always
|
||||
// carries a `values`/`select`/`with` keyword, so this branch
|
||||
// stays out of the advanced path (its arity is handled above).
|
||||
let Some(tbl_idx) = path.items.iter().position(|it| {
|
||||
matches!(
|
||||
it.kind,
|
||||
MatchedKind::Ident {
|
||||
source: IdentSource::Tables,
|
||||
role: "insert_target_table" | "table_name",
|
||||
}
|
||||
)
|
||||
}) else {
|
||||
return Vec::new();
|
||||
};
|
||||
("values", &path.items[tbl_idx + 1..])
|
||||
} else {
|
||||
return Vec::new();
|
||||
};
|
||||
|
||||
let mut diagnostics = Vec::new();
|
||||
let tail = &path.items[kw_idx + 1..];
|
||||
|
||||
let emit = |span: (usize, usize), actual: usize, diagnostics: &mut Vec<Diagnostic>| {
|
||||
diagnostics.push(Diagnostic {
|
||||
severity: Severity::Error,
|
||||
span,
|
||||
message: crate::friendly::translate(
|
||||
message_key,
|
||||
&[
|
||||
("expected", &expected as &dyn std::fmt::Display),
|
||||
("actual", &actual as &dyn std::fmt::Display),
|
||||
],
|
||||
),
|
||||
message: make_message(actual),
|
||||
});
|
||||
};
|
||||
|
||||
@@ -1600,7 +1733,8 @@ fn dml_insert_arity_diagnostics(
|
||||
}
|
||||
}
|
||||
if proj_arity != expected {
|
||||
emit(anchor.unwrap_or(path.items[kw_idx].span), proj_arity, &mut diagnostics);
|
||||
let fallback = tail.first().map_or((0, 0), |it| it.span);
|
||||
emit(anchor.unwrap_or(fallback), proj_arity, &mut diagnostics);
|
||||
}
|
||||
}
|
||||
diagnostics
|
||||
@@ -2872,7 +3006,7 @@ fn walk_one_command<'a>(
|
||||
// INSERT…SELECT projection. Form A uses the column list's
|
||||
// length; Form B uses the schema's column count for the
|
||||
// target table.
|
||||
d.extend(dml_insert_arity_diagnostics(&path, ctx.schema));
|
||||
d.extend(dml_insert_arity_diagnostics(&path, ctx.schema, ctx.mode));
|
||||
// ADR-0033 §8.3 — WARNING when an INSERT's column list omits
|
||||
// a NOT-NULL-no-default (non-auto-gen) column.
|
||||
d.extend(dml_not_null_missing_diagnostics(&path, ctx.schema));
|
||||
@@ -4252,23 +4386,30 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn phase_d_insert_form_b_skips_serial_column() {
|
||||
// Form B: `insert into <T> values (…)` excludes
|
||||
// auto-generated columns from the value list. Supplying
|
||||
// a value for the serial column is a count mismatch.
|
||||
// Form B: `insert into <T> values (…)` excludes auto-generated
|
||||
// columns from the value list. Supplying a value for the serial
|
||||
// column is a count mismatch. As of issue #17 such a wrong-count
|
||||
// tuple parses **structurally** (routed to the type-blind
|
||||
// fallback) so the friendly arity diagnostic can fire — the
|
||||
// mismatch is now reported as an ERROR diagnostic rather than a
|
||||
// bare parse error, matching advanced mode. Dispatch is gated by
|
||||
// the submit pre-flight, not by a parse failure.
|
||||
let schema = schema_with(
|
||||
"Customers",
|
||||
&[("id", Type::Serial), ("Name", Type::Text)],
|
||||
);
|
||||
// Two values where Form B expects one (Name only):
|
||||
let err = parse_command_with_schema(
|
||||
// Two values where Form B expects one (Name only): structurally
|
||||
// parses, but the simple-mode arity diagnostic flags it (Form B
|
||||
// expects 1 value for `Name`; `id` is auto-generated).
|
||||
let diags = diag_keys_simple(
|
||||
"insert into Customers values (1, 'Alice')",
|
||||
&schema,
|
||||
)
|
||||
.expect_err("Form B should reject user-supplied serial");
|
||||
match err {
|
||||
crate::dsl::ParseError::Invalid { .. } => {}
|
||||
other => panic!("expected Invalid, got {other:?}"),
|
||||
}
|
||||
);
|
||||
assert!(
|
||||
diags.iter().any(|d| d.contains("1 value(s)") && d.contains("2 given")),
|
||||
"Form B serial-skip count mismatch must fire the arity \
|
||||
diagnostic (expected 1, got 2); got {diags:?}",
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -4703,6 +4844,139 @@ mod tests {
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// Simple-mode counterpart of [`diag_keys`] — the DSL surface
|
||||
/// (ADR-0003). Issue #17: the arity diagnostic must fire in simple
|
||||
/// mode too, with the user-fillable (serial-skipped) Form B count.
|
||||
fn diag_keys_simple(source: &str, schema: &SchemaCache) -> Vec<String> {
|
||||
let mut ctx = super::context::WalkContext::with_schema(schema);
|
||||
ctx.mode = crate::mode::Mode::Simple;
|
||||
let (result, _cmd) =
|
||||
super::walk(source, super::outcome::WalkBound::EndOfInput, &mut ctx);
|
||||
result.map_or_else(Vec::new, |r| {
|
||||
r.diagnostics.into_iter().map(|d| d.message).collect()
|
||||
})
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn insert_arity_mismatch_simple_form_b_uses_user_fillable_count() {
|
||||
// Issue #17: simple-mode Form B skips serial/shortid (auto-
|
||||
// filled), so `Customers(id serial, Name, Age, SerNo)` expects
|
||||
// 2 values (Name, Age). Three values is a mismatch — and simple
|
||||
// mode must surface the friendly arity diagnostic (as advanced
|
||||
// mode already does), counted against the user-fillable columns,
|
||||
// not the full table.
|
||||
let schema = schema_with(
|
||||
"Customers",
|
||||
&[
|
||||
("id", Type::Serial),
|
||||
("Name", Type::Text),
|
||||
("Age", Type::Int),
|
||||
("SerNo", Type::Serial),
|
||||
],
|
||||
);
|
||||
let diags = diag_keys_simple(
|
||||
"insert into Customers values ('Oli', 52, 3)",
|
||||
&schema,
|
||||
);
|
||||
assert!(
|
||||
diags.iter().any(|d| d.contains("2 value(s)") && d.contains("3 given")),
|
||||
"simple Form B must fire arity diagnostic with user-fillable \
|
||||
count (2) and actual (3); got {diags:?}",
|
||||
);
|
||||
// Pedagogical: names the user-fillable and the auto-generated
|
||||
// columns so the learner understands the skip.
|
||||
assert!(
|
||||
diags.iter().any(|d| d.contains("Name")
|
||||
&& d.contains("Age")
|
||||
&& d.contains("id")
|
||||
&& d.contains("SerNo")),
|
||||
"message should name fillable (Name, Age) and auto-gen (id, \
|
||||
SerNo) columns; got {diags:?}",
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn insert_arity_mismatch_simple_form_a_uses_listed_count() {
|
||||
// Simple Form A: the explicit column list sets the expected
|
||||
// count (mode-neutral). `(Name, Age)` lists 2 columns; one value
|
||||
// is a mismatch. The DSL Form A column role is `insert_first_item`
|
||||
// (issue #17 — the diagnostic counts it as well as the SQL
|
||||
// `insert_column`).
|
||||
let schema = schema_with(
|
||||
"Customers",
|
||||
&[("id", Type::Serial), ("Name", Type::Text), ("Age", Type::Int)],
|
||||
);
|
||||
let diags = diag_keys_simple(
|
||||
"insert into Customers (Name, Age) values ('Oli')",
|
||||
&schema,
|
||||
);
|
||||
assert!(
|
||||
diags.iter().any(|d| d.contains("names 2 column(s)") && d.contains("1 value(s)")),
|
||||
"simple Form A must fire the column-list arity diagnostic; got {diags:?}",
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn insert_arity_mismatch_simple_form_c_uses_user_fillable_count() {
|
||||
// Simple Form C (`insert into T (vals)`, no `values` keyword)
|
||||
// shares Form B's count semantics — the diagnostic must fire even
|
||||
// though there is no `values` keyword to anchor on (issue #17).
|
||||
let schema = schema_with(
|
||||
"Customers",
|
||||
&[("id", Type::Serial), ("Name", Type::Text), ("Age", Type::Int)],
|
||||
);
|
||||
let diags = diag_keys_simple(
|
||||
"insert into Customers ('Oli', 52, 3)",
|
||||
&schema,
|
||||
);
|
||||
assert!(
|
||||
diags.iter().any(|d| d.contains("2 value(s)") && d.contains("3 given")),
|
||||
"simple Form C must fire the arity diagnostic (expected 2, got 3); got {diags:?}",
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn insert_arity_mismatch_simple_all_auto_table() {
|
||||
// Edge: every column auto-generated (all serial/shortid). The
|
||||
// user-fillable count is 0, so any supplied value is a mismatch —
|
||||
// the tailored "all auto-generated" message fires (issue #17).
|
||||
let schema = schema_with(
|
||||
"Counters",
|
||||
&[("id", Type::Serial), ("seq", Type::Serial)],
|
||||
);
|
||||
let diags = diag_keys_simple(
|
||||
"insert into Counters values (1)",
|
||||
&schema,
|
||||
);
|
||||
assert!(
|
||||
diags.iter().any(|d| d.contains("all auto-generated")
|
||||
|| d.contains("auto-generated, so no values")),
|
||||
"all-auto table must fire the tailored message; got {diags:?}",
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn insert_arity_advanced_form_b_still_uses_full_column_count() {
|
||||
// Guard: issue #17 made the diagnostic mode-aware, but advanced
|
||||
// Form B must keep the full-column-count semantics (auto-fills
|
||||
// nothing, ADR-0036) — `('Oli', 52, 3)` for a 4-column table
|
||||
// reports "all 4 column(s)", not the simple-mode user-fillable 2.
|
||||
let schema = schema_with(
|
||||
"Customers",
|
||||
&[
|
||||
("id", Type::Serial),
|
||||
("Name", Type::Text),
|
||||
("Age", Type::Int),
|
||||
("SerNo", Type::Serial),
|
||||
],
|
||||
);
|
||||
let diags = diag_keys("insert into Customers values ('Oli', 52, 3)", &schema);
|
||||
assert!(
|
||||
diags.iter().any(|d| d.contains("all 4 column(s)") && d.contains("3 value(s)")),
|
||||
"advanced Form B must keep the full-column count (4); got {diags:?}",
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn unknown_qualifier_in_qualified_ref_is_error() {
|
||||
let schema = two_table_schema();
|
||||
|
||||
Reference in New Issue
Block a user