oli/rdbms-playground
1
0
Fork 0
Code Issues 4 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix: INSERT Form B value-count UX (ADR-0033 Amendment 5)

Browse Source 
Three layered fixes for advanced/simple-mode positional INSERT
value-count mismatches (e.g. `insert into T values (...)` with the
wrong number of values for T's column count), plus ADR-0033
Amendment 5 recording the gate refinement.

Walker diagnostic (dml_insert_arity_diagnostics): the function's own
doc-comment recorded the no-column-list (Form B) case as deferred.
This commit closes that gap. Form B mismatches now emit a new
diagnostic.insert_arity_mismatch_form_b ERROR per offending tuple,
keyed off the target table's column count from the schema cache. The
[ERR] validity indicator (ADR-0027) lights up at typing time for the
reported scenario, no longer needing a submit.

Cross-mode pointer gate (advanced_alternative_note): refactored from
a hand-rolled Form B count check to a single input_verdict_in_mode(
input, schema, Mode::Advanced) call. The pointer fires only when the
verdict is None — the ADR-0027 sense of "valid". Any future static
check added to the verdict pipeline participates automatically; no
per-feature maintenance.

Teaching notes for the value-count cases users can hit before the
indicator turns red:
  - simple-mode submit: insert.form_b_extra_values_note covers under-,
    in-window, and over-supply against the Form B contract; suppressed
    when the cross-mode pointer fires (to avoid parallel advice).
  - advanced-mode dispatch pre-flight:
    insert.form_b_positional_count_mismatch_note catches a submitted
    mismatch with a teaching message before the engine produces its
    raw NOT-NULL / type error.

The advanced_mode.also_valid_sql pointer wording was reworked to
"trying to write SQL? switch with `mode advanced`, or prefix `:` to
run once". One insta snapshot regenerated.

ADR-0033 Amendment 5 records the gate change: Amendment 3's "would
parse in advanced mode" now reads as "valid in advanced mode" in the
explicit verdict-is-None sense, with the precise definition spelled
out so future readers can't drift back to the syntactic-only reading.
ADR-0000 index entry updated; docs/requirements.md H1a citation added
listing the three new pedagogical strings.

Tests added (8): four walker arity tests (under-supply, over-supply,
match, unknown-table); two app-level teaching-note tests for the
sibling cases (under-supply, over-supply beyond total); one pointer-
gate unit test pinning the bug-case suppression; one gate-precedence
test ensuring only one advice line per error. Existing
simple_mode_submit_of_sql_construct_appends_advanced_pointer updated
to use a known schema (the new validity gate requires it).

Full suite: 2031 passed, 0 failed, 0 unexpected skips. Clippy clean.
This commit is contained in:
claude@clouddev1
2026-05-28 16:38:33 +00:00
parent 9468324d56
commit c12ed1da9a
9 changed files with 898 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/input_render.rs
+271 -10
View File
@@ -300,11 +300,24 @@ pub fn ambient_hint_in_mode(
/// The `advanced_mode.also_valid_sql` pointer string, or `None`.
///
/// Returns the pointer when a simple-mode line is a *definite* DSL
/// error (not merely incomplete) yet parses in advanced mode. Used to
/// combine the DSL fix with the mode hint — both while typing
/// (`ambient_hint_in_mode`) and on submit (`App::dispatch_dsl`) — so
/// the pointer reaches SQL constructs that surface only on submit,
/// e.g. `delete … returning` (ADR-0033 Amendment 3).
/// error (not merely incomplete) yet would be **valid** in advanced
/// mode (ADR-0033 Amendment 5).
///
/// "Valid" is the ADR-0027 sense: the advanced-mode input-validity
/// verdict comes back as `None` (no Warning, no Error) after every
/// static check — parse, schema existence, type slots, INSERT arity,
/// predicate warnings, and any future addition. The pointer fires only
/// when switching modes is **actually** the next step the user can
/// take to make the line valid: a `Severity::Warning` or
/// `Severity::Error` from the advanced-mode pipeline means switching
/// wouldn't resolve the line, so the pointer would mislead.
///
/// Issue #1 origin: previously the gate was a syntactic "would parse"
/// check, which fired for the user's reported case (4-col table,
/// 3 positional values) where the line parses but fails at the engine.
/// Now the gate reads `input_verdict_in_mode` so any static
/// rejection — including the Form B arity diagnostic added in this
/// same change (issue #1) — suppresses the pointer automatically.
#[must_use]
pub fn advanced_alternative_note(
input: &str,
@@ -317,10 +330,228 @@ pub fn advanced_alternative_note(
if !definite_dsl_error {
return None;
}
if parse_command_with_schema_in_mode(input, cache, Mode::Advanced).is_ok() {
Some(crate::t!("advanced_mode.also_valid_sql"))
// The validity-verdict-driven gate (ADR-0033 Amendment 5): the
// line must be fully valid (verdict `None`) in advanced mode.
if crate::dsl::walker::input_verdict_in_mode(input, Some(cache), Mode::Advanced).is_some()
{
return None;
}
Some(crate::t!("advanced_mode.also_valid_sql"))
}
/// Education note for the simple-mode INSERT Form B count-mismatch
/// case (issue #1 sub-task 2).
///
/// When a simple-mode `insert into <T> values (…)` (Form B) supplies
/// more values than the non-auto-generated columns of `<T>` — but no
/// more than every column — the bare "expected `)`" parse error
/// doesn't explain the contract that excluded the serial / shortid
/// columns. This helper returns a note that names the columns Form B
/// expects values for, names the auto-generated ones it skips, and
/// shows the column-list (Form A) override.
///
/// Returns `None` when the input doesn't match the pattern (e.g. the
/// advanced parse fails, the schema isn't known, the table has no
/// auto-generated columns, the value count is at or below the non-auto
/// count, or the value count exceeds the total column count — which is
/// a different error class the engine surfaces directly).
///
/// Also `None` when the cross-mode `advanced_alternative_note` pointer
/// would fire for the same input: those two pieces of advice overlap
/// (both are escape hatches from the Form-B mismatch — one to advanced
/// mode, one to Form A) and showing both would clutter the error
/// without adding pedagogy. The cross-mode pointer wins because it
/// only fires when switching modes actually works (issue #1 sub-task
/// 1's gate); when it doesn't fire, this note steps in.
#[must_use]
pub fn form_b_extra_values_note(
input: &str,
cache: &crate::completion::SchemaCache,
) -> Option<String> {
use crate::dsl::command::Command;
use crate::dsl::types::Type;
if advanced_alternative_note(input, cache).is_some() {
return None;
}
let parsed = parse_command_with_schema_in_mode(input, cache, Mode::Advanced).ok()?;
let Command::SqlInsert {
target_table,
listed_columns,
literal_rows,
..
} = parsed
else {
return None;
};
if !listed_columns.is_empty() || literal_rows.is_empty() {
return None;
}
let table_cols = cache.table_columns.get(&target_table)?;
let is_auto = |t: Type| matches!(t, Type::Serial | Type::ShortId);
let auto: Vec<&str> = table_cols
.iter()
.filter(|c| is_auto(c.user_type))
.map(|c| c.name.as_str())
.collect();
if auto.is_empty() {
return None;
}
let non_auto: Vec<&str> = table_cols
.iter()
.filter(|c| !is_auto(c.user_type))
.map(|c| c.name.as_str())
.collect();
// Defence in depth: a table that is all-auto has no non-auto
// column to talk about. The cross-mode-pointer gate above would
// normally shield us (the line works in advanced — the pointer
// fires and we return early), but the helper is `pub` and should
// not produce "expects 0 values for " on its own.
if non_auto.is_empty() {
return None;
}
// Fire whenever the supplied count differs from Form B's expected
// count — the teaching message is forward-looking (it states what
// Form B expects + the override path) so it works for under-supply,
// over-supply within the column range, and over-supply past the
// total column count alike (issue #1 siblings task — over- /
// under-supply previously fell through to the bare parse error).
let value_count = literal_rows[0].len();
if value_count == non_auto.len() {
return None;
}
let expected_phrase = if non_auto.len() == 1 {
format!("1 value for {}", quote_join_and(&non_auto))
} else {
None
format!(
"{count} values for {names}",
count = non_auto.len(),
names = quote_join_and(&non_auto),
)
};
let auto_phrase = if auto.len() == 1 {
format!(
"{name} is auto-generated and filled automatically",
name = quote_join_and(&auto),
)
} else {
format!(
"{names} are auto-generated and filled automatically",
names = quote_join_and(&auto),
)
};
let all_cols = table_cols
.iter()
.map(|c| c.name.as_str())
.collect::<Vec<_>>()
.join(", ");
Some(crate::t!(
"insert.form_b_extra_values_note",
table = target_table,
expected_phrase = expected_phrase,
auto_phrase = auto_phrase,
all_cols = all_cols,
))
}
/// Pre-flight note for advanced-mode `insert into <T> values (…)`
/// (positional, no column list) when the value count doesn't match
/// the table's column count (issue #1 sub-task 3).
///
/// The advanced-mode SQL grammar accepts any positional value count;
/// the engine then rejects the line with a NOT-NULL / type-mismatch
/// error that doesn't tell the user about the column-list override.
/// This helper detects the case at dispatch time and returns a note
/// that names the rule, lists the target table's columns, and shows
/// the column-list (Form A) override.
///
/// Returns `None` when the input doesn't match the pattern (the parse
/// isn't a Form B `SqlInsert`, the schema isn't known, the table has
/// no auto-generated columns or no non-auto columns, the literal-row
/// shape is empty, or every row's value count already matches the
/// column count).
///
/// Conservative on multi-row VALUES: fires only when **every** row's
/// value count is the same wrong number. Mixed-length rows fall
/// through to the engine error — they're a different shape of mistake.
#[must_use]
pub fn form_b_positional_count_mismatch_note(
parsed: &crate::dsl::command::Command,
cache: &crate::completion::SchemaCache,
) -> Option<String> {
use crate::dsl::command::Command;
use crate::dsl::types::Type;
let Command::SqlInsert {
target_table,
listed_columns,
literal_rows,
..
} = parsed
else {
return None;
};
if !listed_columns.is_empty() || literal_rows.is_empty() {
return None;
}
let table_cols = cache.table_columns.get(target_table)?;
let col_count = table_cols.len();
let row_lens: Vec<usize> = literal_rows.iter().map(Vec::len).collect();
// Only fire when every row is the same (wrong) length.
let first_len = row_lens[0];
if !row_lens.iter().all(|n| *n == first_len) {
return None;
}
if first_len == col_count {
return None;
}
let is_auto = |t: Type| matches!(t, Type::Serial | Type::ShortId);
// The override only makes sense when (a) there's something to
// skip and (b) something to list. A table that is all-auto can't
// omit anything via the column list; a table with no autos
// doesn't benefit from the column-list form for this purpose.
let has_auto = table_cols.iter().any(|c| is_auto(c.user_type));
if !has_auto {
return None;
}
let non_auto: Vec<&str> = table_cols
.iter()
.filter(|c| !is_auto(c.user_type))
.map(|c| c.name.as_str())
.collect();
if non_auto.is_empty() {
return None;
}
let all_cols_list = quote_join_and(
&table_cols
.iter()
.map(|c| c.name.as_str())
.collect::<Vec<_>>(),
);
let non_auto_csv = non_auto.join(", ");
Some(crate::t!(
"insert.form_b_positional_count_mismatch_note",
table = target_table,
col_count = col_count,
col_list = all_cols_list,
supplied = first_len,
non_auto_csv = non_auto_csv,
))
}
/// Format `items` as an English-prose list with backtick-quoted
/// identifiers: `["a"]` → `` `a` ``, `["a","b"]` → `` `a` and `b` ``,
/// `["a","b","c"]` → `` `a`, `b`, and `c` `` (Oxford comma).
fn quote_join_and(items: &[&str]) -> String {
match items {
[] => String::new(),
[only] => format!("`{only}`"),
[a, b] => format!("`{a}` and `{b}`"),
rest => {
let head: Vec<String> = rest[..rest.len() - 1]
.iter()
.map(|s| format!("`{s}`"))
.collect();
format!("{}, and `{}`", head.join(", "), rest[rest.len() - 1])
}
}
}
@@ -1124,8 +1355,10 @@ mod tests {
// The DSL detail survives …
assert!(p.contains("Name"), "expected DSL slot detail, got: {p:?}");
// … and the advanced-mode pointer is appended.
// Substring "mode advanced" is the actionable fragment
// (the switch command) that survives wording revisions.
assert!(
p.contains("advanced mode"),
p.contains("mode advanced"),
"expected the advanced-mode pointer, got: {p:?}",
);
}
@@ -1145,12 +1378,40 @@ mod tests {
let input = "insert into Customers values ('Alice')";
if let Some(AmbientHint::Prose(p)) = ambient_hint(input, input.len(), None, &cache) {
assert!(
!p.contains("advanced mode"),
!p.contains("mode advanced"),
"a valid DSL command must not carry the advanced pointer, got: {p:?}",
);
}
}
#[test]
fn ambient_hint_omits_advanced_pointer_when_form_b_value_count_wouldnt_match() {
// Issue #1: simple-mode rejects `insert into Customers values
// ('Oli', 52, 3)` because Form B skips both serials and expects
// 2 values for `Name`, `Age`. The same line ALSO fails in
// advanced mode: positional VALUES requires every column (4
// total) but only 3 were supplied. The cross-mode pointer would
// be misleading — switching modes wouldn't help — so it must
// not be appended.
use crate::dsl::types::Type;
let cache = schema_with_columns(
"Customers",
&[
("id", Type::Serial),
("Name", Type::Text),
("Age", Type::Int),
("SerNo", Type::Serial),
],
);
let input = "insert into Customers values ('Oli', 52, 3)";
let note = advanced_alternative_note(input, &cache);
assert!(
note.is_none(),
"Form B mismatch where advanced mode would also reject — \
the cross-mode pointer must be suppressed; got: {note:?}",
);
}
#[test]
fn ambient_hint_at_insert_second_value_shows_text_prose() {
use crate::dsl::types::Type;