diff --git a/docs/adr/0042-h1a-parse-error-pedagogy-grammar-tree.md b/docs/adr/0042-h1a-parse-error-pedagogy-grammar-tree.md index 6a84df6..834df55 100644 --- a/docs/adr/0042-h1a-parse-error-pedagogy-grammar-tree.md +++ b/docs/adr/0042-h1a-parse-error-pedagogy-grammar-tree.md @@ -223,6 +223,55 @@ on any §2 wording change (expected; reviewed via `cargo insta`), but the two substring suites above must stay green without edits to their assertions. +## Implementation outcome (2026-06-05) + +The baseline capture (§Implementation notes step 1) triaged four +gaps; all four are fixed test-first, locked by the near-miss matrix +in `tests/it/parse_error_pedagogy.rs`: + +- **G1** — the bare `1` cardinality literal opening `add 1:n + relationship …` rendered cryptically. Render it as + `` `1:n relationship` `` in `format_expectation` (error wording + only; completion still offers the literal `1`). +- **G2** — bare `select` dumped the 14-item expression first-set. + Collapse it to "a projection: `*`, a column, or an expression" + in `format_walker_error`, detected by the `distinct`+`all` + quantifier pair being *jointly* expectable — a signature unique + to a projection start (empirically verified not to misfire at + `count(`, `union`, `union all`, `select distinct`, or mid-list). + Render-only; the completion/hint layer still expands the full + set. +- **G3** — the usage block was mode-blind (`render_usage_block` + resolved shared entry words to the first-registered Simple node). + `usage_key(s)_for_input` gain mode-aware `_in_mode` variants. + + **Decision (user-confirmed, after the DA pass).** In advanced + mode the DSL forms remain *valid input* via fallback — verified: + `create table Foo with pk`, `drop column from table T: c`, + `drop relationship r`, `add column …` all parse and dispatch in + advanced mode. So the advanced usage block shows **every form + valid in the mode, mode-primary (SQL) first, then the DSL + fallback forms** — a usage hint must never hide input that works. + (An initial implementation that showed SQL-only was flagged by + the DA pass as hiding `create table … with pk` / `drop column …` + and corrected.) Simple mode shows DSL forms only — the SQL-only + forms hit the "this is SQL" rail and are unreachable. + +- **G4** — `with` borrowed `select`'s usage; it gains its own + `parse.usage.with` CTE template. + +Coverage: the matrix covers, in both modes, every entry word's bare +/ missing-clause / wrong-token near-misses, the app-lifecycle +trailing-junk cases, **and** the committed *multi-form* variants +(`add index` / `add constraint` / `add 1:n relationship`, `drop +index` / `drop constraint` / `drop relationship`, `show table`, +`change column …`, `create index`, `alter table … add` / `… drop`). +The committed forms were audited 2026-06-05 and each renders its own +form-specific missing-keyword message + usage (e.g. `add index` → +"expected `on` or `as`"; `drop constraint` → "expected `not`, +`unique`, `default`, or `check`"), regression-locked in +`near_miss_matrix_committed_multiforms`. + ## Out of scope 1. **Advanced-SQL engine-error sanitisation** — ADR-0019 §OOS-2. diff --git a/src/app.rs b/src/app.rs index d2313ae..54f42ec 100644 --- a/src/app.rs +++ b/src/app.rs @@ -1338,11 +1338,11 @@ impl App { }, ), AppCommand::Import { path, target } => { - // The path-bearing import goes through the - // pre-chumsky source-slice (parser.rs), which - // already validated non-empty path. Bare - // `import` returns from chumsky with an empty - // path string — surface the usage error. + // A path-bearing import carries a non-empty path + // from the walker. Bare `import` parses with an + // empty path string — surface the usage hint here + // at dispatch (not a parse error; ADR-0024 replaced + // the old chumsky source-slice path). if path.is_empty() { self.note_error(crate::t!("project.import_usage")); return Vec::new(); diff --git a/src/dsl/grammar/mod.rs b/src/dsl/grammar/mod.rs index c77ebec..d68fc3b 100644 --- a/src/dsl/grammar/mod.rs +++ b/src/dsl/grammar/mod.rs @@ -566,11 +566,6 @@ pub fn usage_keys_for_input_in_mode( if candidates.is_empty() { return None; } - let want = if mode == crate::mode::Mode::Advanced { - CommandCategory::Advanced - } else { - CommandCategory::Simple - }; let union = |nodes: &[(usize, &'static CommandNode, CommandCategory)]| -> Vec<&'static str> { let mut keys: Vec<&'static str> = Vec::new(); for (_, node, _) in nodes { @@ -582,23 +577,44 @@ pub fn usage_keys_for_input_in_mode( } keys }; - let matched: Vec<(usize, &'static CommandNode, CommandCategory)> = - candidates.iter().copied().filter(|(_, _, cat)| *cat == want).collect(); - // Prefer the mode-matching nodes' usage. But a shared SQL node - // (`SQL_INSERT` / `SQL_UPDATE` / `SQL_DELETE`) declares no - // `usage_ids` of its own — it reuses the DSL template. When the - // mode-preferred set yields no usage keys, fall back to every - // candidate so the entry word still shows a usage block rather - // than the available-commands fallback (regression-locked by - // the advanced near-miss matrix). - let mut keys = union(&matched); - if keys.is_empty() { - keys = union(&candidates); - } + // Advanced mode: every candidate form is reachable — the SQL + // nodes are primary, and the DSL nodes remain valid via fallback + // (verified: `create table … with pk` and `drop column …` both + // run in advanced mode). Show them all, mode-primary (Advanced) + // first, so the usage hint never hides input that works. Simple + // mode: only the DSL forms — the SQL-only forms hit the "this is + // SQL" rail and are not reachable. (ADR-0042 G3.) + let selected: Vec<(usize, &'static CommandNode, CommandCategory)> = + if mode == crate::mode::Mode::Advanced { + let mut v: Vec<_> = candidates + .iter() + .copied() + .filter(|(_, _, c)| *c == CommandCategory::Advanced) + .collect(); + v.extend( + candidates + .iter() + .copied() + .filter(|(_, _, c)| *c != CommandCategory::Advanced), + ); + v + } else { + candidates + .iter() + .copied() + .filter(|(_, _, c)| *c == CommandCategory::Simple) + .collect() + }; + // Degenerate guard: an advanced-only word in simple mode (not + // normally reachable — it hits the SQL rail first) leaves + // `selected` empty; fall back to all candidates so a usage block + // still renders rather than the available-commands fallback. + let pick = if selected.is_empty() { candidates } else { selected }; + let keys = union(&pick); if keys.is_empty() { return None; } - let entry = candidates[0].1.entry.primary; + let entry = pick[0].1.entry.primary; Some((entry, keys)) } diff --git a/tests/it/parse_error_pedagogy.rs b/tests/it/parse_error_pedagogy.rs index c975cfd..aedd920 100644 --- a/tests/it/parse_error_pedagogy.rs +++ b/tests/it/parse_error_pedagogy.rs @@ -64,17 +64,22 @@ fn dump(input: &str, lines: &[String]) -> String { /// near-correct input plus substrings that MUST appear across its /// rendered error lines — the structural "name the missing /// keyword/clause" message and the per-command usage template. -/// These are the cases triaged as already-good; the baseline dump -/// (`baseline_dump`, #[ignore]) shows the full rendering. Cases -/// flagged for wording fixes (bare `add` `1`, bare `select` -/// first-set, mode-blind usage) are deliberately NOT locked here -/// until their fixes land. #[test] fn near_miss_matrix_simple_mode() { // (input, required-substrings-across-error-lines) let matrix: &[(&str, &[&str])] = &[ - // app-lifecycle arg errors + // app-lifecycle arg errors. The arg-less commands all reject + // trailing junk with "expected end of input" + their usage + // (audited 2026-06-05); locked here as regression insurance. ("quit now", &["after `quit`, expected end of input", " quit"]), + ("help foo", &["after `help`, expected end of input", " help"]), + ("rebuild now", &["after `rebuild`, expected end of input", " rebuild"]), + ("new foo", &["after `new`, expected end of input", " new"]), + ("load foo", &["after `load`, expected end of input", " load"]), + ("undo foo", &["after `undo`, expected end of input", " undo"]), + ("redo foo", &["after `redo`, expected end of input", " redo"]), + ("export foo bar", &["after `export foo`, expected end of input", "export []"]), + ("import a b c", &["after `import a`, expected end of input", "import "]), ("save sideways", &["after `save`, expected end of input", "save | save as"]), ("mode sideways", &["unknown mode 'sideways'", "mode simple | mode advanced"]), ("messages louder", &["unknown messages mode 'louder'", "messages short"]), @@ -137,6 +142,61 @@ fn advanced_error_lines_for(input: &str) -> Vec { .collect() } +/// Committed multi-form near-misses (ADR-0042 §1). A user who has +/// already chosen a form (`add index …`, `drop constraint …`, +/// `alter table T add …`) must get that form's specific +/// missing-keyword/clause message and usage — not the whole family +/// generically. Audited 2026-06-05; these render well today and are +/// locked here as the residual systematic-pass tail. +#[test] +fn near_miss_matrix_committed_multiforms() { + // (input, advanced?, required-substrings) + let matrix: &[(&str, bool, &[&str])] = &[ + // add / drop multi-forms (simple) + ("add index", false, &["after `add index`, expected `on` or `as`", "add index [as ] on"]), + ("add index on T", false, &["after `add index on T`, expected `(`", "add index [as ] on"]), + ("add constraint", false, &["after `add constraint`, expected `not`, `unique`, `default`, or `check`", "add constraint not null to"]), + ("add constraint not null", false, &["after `add constraint not null`, expected `to`", "add constraint not null to"]), + ("add 1:n relationship", false, &["after `add 1:n relationship`, expected `from` or `as`", "add 1:n relationship"]), + ("add 1:n relationship from", false, &["after `add 1:n relationship from`, expected table name", "from ."]), + ("drop constraint", false, &["after `drop constraint`, expected `not`, `unique`, `default`, or `check`", "drop constraint (not null"]), + ("drop constraint not null", false, &["after `drop constraint not null`, expected `from`", "drop constraint (not null"]), + ("drop index", false, &["after `drop index`, expected `on` or index name", "drop index ", "drop index on "]), + ("drop index on T", false, &["after `drop index on T`, expected `(`", "drop index on
"]), + ("drop relationship", false, &["after `drop relationship`, expected `from` or relationship name", "drop relationship "]), + ("show table", false, &["after `show table`, expected table name", "show table
"]), + ("change column in table T: c", false, &["after `change column in table T: c`, expected `(`", "change column [in] [table]"]), + // advanced committed multi-forms + ("create index on", true, &["after `create index on`, expected table name", "create [unique] index"]), + ("create unique index", true, &["after `create unique index`, expected `on`, identifier, or `if`", "create [unique] index"]), + ("alter table T add", true, &["after `alter table T add`, expected `column`, `constraint`, `check`, `unique`, `foreign`, or `primary`", "alter table
add column"]), + ("alter table T drop", true, &["after `alter table T drop`, expected `column` or `constraint`", "alter table
drop column"]), + ]; + for (input, advanced, needles) in matrix { + let lines = if *advanced { + advanced_error_lines_for(input) + } else { + error_lines_for(input) + }; + let dump_msg = dump(input, &lines); + assert!( + lines.iter().any(|l| l.starts_with("parse error")), + "missing `parse error` line for {input:?}\n{dump_msg}", + ); + assert!( + !lines.iter().any(|l| l.starts_with("available commands:")), + "committed form {input:?} fell back to available-commands\n{dump_msg}", + ); + let joined = lines.join("\n"); + for needle in *needles { + assert!( + joined.contains(needle), + "committed form {input:?} missing expected substring {needle:?}\n{dump_msg}", + ); + } + } +} + #[test] fn advanced_bare_select_collapses_projection_first_set() { // ADR-0042 G2: bare `select` dumped the full 14-item @@ -163,12 +223,16 @@ fn advanced_bare_select_collapses_projection_first_set() { } #[test] -fn advanced_mode_usage_block_shows_sql_templates_not_dsl() { +fn advanced_mode_usage_block_shows_sql_and_dsl_forms() { // ADR-0042 G3: `render_usage_block` was mode-blind — it // resolved shared entry words to the first-registered (Simple) - // node, so advanced-mode `create` showed the DSL `create table - // … with pk …` template, which is not valid SQL. Mode-aware - // selection now shows the SQL forms. + // node, so advanced-mode `create` showed ONLY the DSL `create + // table … with pk …` template and none of the SQL forms. + // Mode-aware selection shows every form valid in the mode, + // SQL-primary first. In advanced mode the DSL forms remain + // valid input (verified: `create table Foo with pk` parses and + // runs in advanced mode), so they MUST still appear — a usage + // hint never hides input that works. let lines = advanced_error_lines_for("create"); let joined = lines.join("\n"); let dump_msg = dump("create", &lines); @@ -181,9 +245,14 @@ fn advanced_mode_usage_block_shows_sql_templates_not_dsl() { "advanced `create` should show the SQL create-index usage\n{dump_msg}", ); assert!( - !joined.contains("with pk"), - "advanced `create` must NOT show the DSL `with pk` template\n{dump_msg}", + joined.contains("create table with pk"), + "advanced `create` should ALSO show the DSL `with pk` form (valid in advanced mode)\n{dump_msg}", ); + // Ordering: the SQL form is listed before the DSL form + // (mode-primary first). + let sql_at = joined.find("create table [if not exists]").unwrap(); + let dsl_at = joined.find("create table with pk").unwrap(); + assert!(sql_at < dsl_at, "SQL form should precede the DSL form\n{dump_msg}"); } /// The advanced-mode near-miss matrix (ADR-0042 §1/§3). Mirrors @@ -197,11 +266,12 @@ fn near_miss_matrix_advanced_mode() { ("select", &["expected a projection: `*`, a column, or an expression", "select (* |"]), ("select * from", &["after `select * from`, expected table name", "select (* |"]), ("with", &["after `with`, expected identifier or `recursive`", "with [recursive]", "as ("]), - // create / drop / alter — SQL templates (G3) - ("create", &["after `create`, expected `table`", "create table [if not exists]", "create [unique] index"]), + // create / drop / alter — SQL forms AND the still-valid DSL + // fallback forms, SQL-primary first (G3). + ("create", &["after `create`, expected `table`", "create table [if not exists]", "create [unique] index", "create table with pk"]), ("create table", &["after `create table`, expected identifier or `if`", "create table [if not exists]"]), ("create index", &["after `create index`, expected `on`", "create [unique] index"]), - ("drop", &["after `drop`, expected `table`", "drop table [if exists]"]), + ("drop", &["after `drop`, expected `table`", "drop table [if exists]", "drop column [from]", "drop relationship"]), ("alter", &["after `alter`, expected `table`", "alter table
add column"]), ("alter table T", &["expected `add`, `drop`, `rename`, or `alter`", "alter table
"]), // shared insert/update/delete — must show usage, not the @@ -429,3 +499,7 @@ fn caret_aligns_under_offending_token() { "caret should sit at column 9 (under `f` of `frobulate` after the `running: ` prefix); got {leading_spaces} spaces in {caret:?}", ); } + + + +