From 6f87ad18421365e8069578df4fb9d87863d84b96 Mon Sep 17 00:00:00 2001 From: "claude@clouddev1" Date: Thu, 28 May 2026 18:56:13 +0000 Subject: [PATCH] fix: advanced CREATE TABLE completion cluster MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three completion / hint bugs in the same advanced-mode grammar + walker path: 1. `create table T ` offered only `with` (the DSL fallback) — the `(` continuation for the SQL column-def list (ADR-0035 §4) was missing because the shared-entry-word completion merge in `completion_probe_in_mode` only fired at the entry-word boundary. Broadened to fire at any cursor depth and to handle `Expectation::Punct` continuations alongside `Word`/`Literal`. A shared-entry-word candidate whose grammar has already diverged (e.g. SQL `CREATE INDEX` past `create table …`) returns Mismatch and is naturally skipped — the viability check stays the gate, not the cursor depth. 2. `create table T (` showed only the table-level constraint keywords (`primary`, `unique`, `check`, `constraint`, `foreign`) in the ambient hint, leaving the column-name role invisible because COLUMN_DEF starts with an `Ident::NewName` slot that produces no concrete candidate. Added a new `HintMode::IntroProse( &'static str)` variant that surfaces catalog prose at slot entry without suppressing Tab completion (unlike `ProseOnly`) and without requiring `typing_name_at_cursor` to fire (unlike `ForceProse`). Wrapped ELEMENT in `Node::Hinted { mode: IntroProse( "hint.create_table_element"), … }`, with prose "Type a column name, or a table-level constraint: `primary`, `unique`, `check`, `constraint`, `foreign`". Tab still cycles every keyword. 3. The SQL_TYPE position leaked the bare keyword `double` (the first token of the dedicated `double precision` Choice branch per ADR-0035 §6.3) alongside the playground's regular type list. Added `("double", "double precision")` to `COMPOSITE_CANDIDATES` and extended the keyword filter to drop composite openers so the composite phrase replaces the bare opener instead of appearing alongside it. Tab now offers `double precision` as a single coherent candidate; the partial-typing prose at the same slot is subsumed by item 2's IntroProse (the user reads "Type a column name…" while mid-typing, then advances to the clean type list). Tests added (4): pinning each behavioural promise above plus the no-leakage assertion at the partial-typing prose position. Full suite 2035 passed / 0 failed / 0 unexpected skips. Clippy clean. The new `HintMode::IntroProse` variant is an additive extension to the ADR-0024 HintMode-per-node model; no behaviour change to existing modes. An ADR-0024 amendment recording it can follow later if desired — flagged but not written. --- src/completion.rs | 27 ++++- src/dsl/grammar/mod.rs | 10 ++ src/dsl/grammar/sql_create_table.rs | 13 ++- src/dsl/walker/mod.rs | 88 +++++++++++------ src/friendly/keys.rs | 4 + src/friendly/strings/en-US.yaml | 4 + src/input_render.rs | 148 ++++++++++++++++++++++++++++ 7 files changed, 259 insertions(+), 35 deletions(-) diff --git a/src/completion.rs b/src/completion.rs index 5053a59..31fa2aa 100644 --- a/src/completion.rs +++ b/src/completion.rs @@ -25,11 +25,22 @@ use crate::mode::Mode; /// one token but which the user types as a single fluent piece. /// Pairs of (walker-expected-literal, full-composite-text). /// -/// When the walker reports `Expectation::Literal(opener)` at the -/// cursor, the engine surfaces the full composite text as a Tab -/// candidate. Today the only entry is `1:n` (the opener for -/// `add 1:n relationship`) — adding more is a one-line edit. -const COMPOSITE_CANDIDATES: &[(&str, &str)] = &[("1", "1:n")]; +/// When the walker reports `Expectation::Literal(opener)` or +/// `Expectation::Word(opener)` at the cursor, the engine surfaces the +/// full composite text as a Tab candidate. Used for multi-token +/// fragments the user thinks of as a single phrase: +/// +/// - `1:n` — the opener for `add 1:n relationship`. +/// - `double precision` — the lone two-word SQL type alias +/// (ADR-0035 §6.3; the grammar has a dedicated branch so the per-word +/// `Ident` validator never has to make sense of `double` alone). +/// Surfacing it as a composite stops bare `double` from appearing in +/// the type candidate list alongside `int`/`text`/etc. (issue #5). +/// Source 1's keyword filter drops openers that appear here so the +/// composite replaces the bare opener rather than appearing +/// alongside it. +const COMPOSITE_CANDIDATES: &[(&str, &str)] = + &[("1", "1:n"), ("double", "double precision")]; /// Per-project schema lookup cache (ADR-0022 §9, ADR-0024 §Phase D). /// @@ -525,6 +536,11 @@ pub fn candidates_at_cursor_with_in_mode( // Declaration order is preserved (matches the canonical // command shape, e.g. `to` before `table` for // `add column [to] [table] …`). + // + // Composite openers (e.g. `double` for `double precision`) are + // filtered out here so Source 1.6 (the composite pipeline) can + // surface the full multi-word candidate without the bare opener + // also appearing alongside (issue #5). let mut keywords: Vec = expected .iter() .filter_map(|e| match e { @@ -532,6 +548,7 @@ pub fn candidates_at_cursor_with_in_mode( _ => None, }) .filter(|w| !w.is_empty() && w.chars().all(|c| c.is_ascii_alphabetic())) + .filter(|w| !COMPOSITE_CANDIDATES.iter().any(|(opener, _)| opener == w)) .map(str::to_string) .filter(|name| matches_prefix(name)) .collect(); diff --git a/src/dsl/grammar/mod.rs b/src/dsl/grammar/mod.rs index b07d96f..89533b0 100644 --- a/src/dsl/grammar/mod.rs +++ b/src/dsl/grammar/mod.rs @@ -149,6 +149,15 @@ impl IdentSource { /// - `ForceProse(catalog_key)` — force this prose at the /// catalog key regardless of candidates. Used today by /// `NewName` ident slots ("Type a name, then `(`"). +/// - `IntroProse(catalog_key)` — show prose at slot entry to +/// *introduce* a position whose first-class candidate is an +/// ident slot (which would be invisible in a pure-candidate +/// render) but whose keyword alternatives are also available. +/// Unlike `ProseOnly`, Tab candidates remain available — the +/// user still cycles through the keyword set. Used at the +/// advanced-mode CREATE TABLE element slot, where the +/// column-name `NewName` slot would otherwise be invisible +/// alongside the table-level constraint keywords (issue #4). /// - `SuppressProse` — show only candidates; never fall back /// to a prose ladder. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -156,6 +165,7 @@ pub enum HintMode { Default, ForceProse(&'static str), ProseOnly(&'static str), + IntroProse(&'static str), SuppressProse, } diff --git a/src/dsl/grammar/sql_create_table.rs b/src/dsl/grammar/sql_create_table.rs index 633f901..c30e24c 100644 --- a/src/dsl/grammar/sql_create_table.rs +++ b/src/dsl/grammar/sql_create_table.rs @@ -390,7 +390,18 @@ const TABLE_FK_NAMED: Node = Node::Seq(TABLE_FK_NAMED_NODES); // the same trade real SQL makes with its reserved words.) static ELEMENT_CHOICES: &[Node] = &[TABLE_PK, TABLE_UNIQUE, TABLE_CHECK, TABLE_FK_NAMED, TABLE_FK, COLUMN_DEF]; -const ELEMENT: Node = Node::Choice(ELEMENT_CHOICES); +const ELEMENT_INNER: Node = Node::Choice(ELEMENT_CHOICES); +// Issue #4: wrap the element slot in `IntroProse` so a fresh element +// position (`create table T (` and after every `,`) surfaces a prose +// hint that names the column-name role *and* the table-level +// constraint keywords. The bare candidate render shows only the +// constraint keywords because the `COLUMN_DEF` branch starts with a +// `NewName` ident that has no concrete candidate to offer; the prose +// makes the dominant first move visible without suppressing Tab. +const ELEMENT: Node = Node::Hinted { + mode: crate::dsl::grammar::HintMode::IntroProse("hint.create_table_element"), + inner: &ELEMENT_INNER, +}; static COLUMN_LIST_NODES: &[Node] = &[ Node::Punct('('), diff --git a/src/dsl/walker/mod.rs b/src/dsl/walker/mod.rs index 4ee5135..5eba5b8 100644 --- a/src/dsl/walker/mod.rs +++ b/src/dsl/walker/mod.rs @@ -147,13 +147,15 @@ pub fn hint_resolution_at_input_in_mode( // value-literal fallback slot; `ForceProse` covers `NewName` // ident slots ("Type a name"). match snap.pending_hint_mode { - Some(mode @ (HintMode::ProseOnly(_) | HintMode::ForceProse(_))) => { - Some(HintResolution { - mode, - column: None, - form_b_autogen_skipped: Vec::new(), - }) - } + Some( + mode @ (HintMode::ProseOnly(_) + | HintMode::ForceProse(_) + | HintMode::IntroProse(_)), + ) => Some(HintResolution { + mode, + column: None, + form_b_autogen_skipped: Vec::new(), + }), Some(HintMode::SuppressProse | HintMode::Default) | None => None, } } @@ -386,28 +388,39 @@ pub fn completion_probe_in_mode( } (top_from, ctes) }; - // ADR-0035 §4i (d/e): shared-entry-word completion merge. In advanced - // mode an entry word like `create` / `drop` has several candidate - // nodes (SQL forms + the DSL fallback), but the walk above committed - // to ONE, so only that node's continuations are in `expected`. At the - // entry-word boundary (nothing typed after the entry word yet — the - // divergence point), walk every candidate, keep the viable - // (Incomplete) ones, and union their next-keyword continuations, - // tagging each by the producing category — so completion offers all - // valid continuations and can colour/order them by mode. Deeper - // positions keep the committed walk's `expected` untouched. + // ADR-0035 §4i (d/e) + issue #3 (2026-05-28): shared-entry-word + // completion merge. In advanced mode an entry word like `create` / + // `drop` has several candidate nodes (SQL forms + the DSL fallback), + // but the walk above committed to ONE, so only that node's + // continuations are in `expected`. Walk every viable + // (`Incomplete`) candidate from the entry word to the cursor, union + // their next-token continuations, tag each by the producing category + // — so completion offers all valid continuations at the current + // depth and can colour/order them by mode. + // + // Originally limited to the entry-word boundary; broadened to every + // depth so that e.g. `create table T ` in advanced mode surfaces + // both `with` (DSL, ADR-0009) and `(` (SQL, ADR-0035 §4) as + // continuations. A candidate whose grammar has already diverged + // (e.g. SQL `CREATE INDEX` after `create table …`) returns + // Mismatch and is naturally skipped — the viability check is the + // gate, not the cursor depth. let mut expected_modes = vec![crate::completion::ModeClass::Both; expected.len()]; if mode == crate::mode::Mode::Advanced { let s = skip_whitespace(source, 0); - if let Some((kw_start, kw_end)) = consume_ident(source, s) - && skip_whitespace(source, kw_end) >= source.len() - { + if let Some((kw_start, kw_end)) = consume_ident(source, s) { let entry = &source[kw_start..kw_end]; let candidates = grammar::commands_for_entry_word(entry); if candidates.len() > 1 { use crate::dsl::grammar::CommandCategory; // (continuation word, produced-by-simple, produced-by-advanced) let mut tally: Vec<(&'static str, bool, bool)> = Vec::new(); + // Continuations that aren't keyword/literal-shaped + // (notably `Punct`, used for `(` at the SQL column-list + // boundary — issue #3). Tracked separately because the + // tally is keyed by `&'static str`; mode classification + // for punctuation defaults to `Both`. + let mut punct_tally: Vec = Vec::new(); for (_, node, category) in candidates { let mut sctx = context::WalkContext::with_schema(schema); sctx.mode = mode; @@ -419,21 +432,28 @@ pub fn completion_probe_in_mode( }; let advanced = category == CommandCategory::Advanced; for e in &cont { - if let outcome::Expectation::Word(w) | outcome::Expectation::Literal(w) = e { - match tally.iter_mut().find(|(kw, _, _)| kw == w) { - Some(rec) => { - if advanced { - rec.2 = true; - } else { - rec.1 = true; + match e { + outcome::Expectation::Word(w) + | outcome::Expectation::Literal(w) => { + match tally.iter_mut().find(|(kw, _, _)| kw == w) { + Some(rec) => { + if advanced { + rec.2 = true; + } else { + rec.1 = true; + } } + None => tally.push((w, !advanced, advanced)), } - None => tally.push((w, !advanced, advanced)), } + outcome::Expectation::Punct(c) if !punct_tally.contains(c) => { + punct_tally.push(*c); + } + _ => {} } } } - if !tally.is_empty() { + if !tally.is_empty() || !punct_tally.is_empty() { // Augment `expected` with merged continuations the // committed node lacked. for &(w, _, _) in &tally { @@ -446,7 +466,17 @@ pub fn completion_probe_in_mode( expected.push(outcome::Expectation::Word(w)); } } + for &c in &punct_tally { + let present = expected + .iter() + .any(|e| matches!(e, outcome::Expectation::Punct(x) if *x == c)); + if !present { + expected.push(outcome::Expectation::Punct(c)); + } + } // Classify every expectation by the merged tally. + // Word/Literal route through the tally; Punct (and + // other expectation shapes) default to `Both`. expected_modes = expected .iter() .map(|e| match e { diff --git a/src/friendly/keys.rs b/src/friendly/keys.rs index d54e3f3..45ed99a 100644 --- a/src/friendly/keys.rs +++ b/src/friendly/keys.rs @@ -211,6 +211,10 @@ pub const KEYS_AND_PLACEHOLDERS: &[(&str, &[&str])] = &[ &["kind", "found"], ), ("hint.ambient_typing_name", &[]), + // Issue #4: introduce the advanced-mode CREATE TABLE element + // slot (`create table T (`) so the otherwise-invisible + // column-name role reads as the dominant first move. + ("hint.create_table_element", &[]), ("hint.value_literal_slot", &[]), ( "hint.ambient_typing_name_then", diff --git a/src/friendly/strings/en-US.yaml b/src/friendly/strings/en-US.yaml index cfd3710..14087e1 100644 --- a/src/friendly/strings/en-US.yaml +++ b/src/friendly/strings/en-US.yaml @@ -357,6 +357,10 @@ hint: # falls through to `ambient_typing_name` instead. ambient_typing_name: "Type a name" ambient_typing_name_then: "Type a name, then {next}" + # Issue #4 — advanced-mode CREATE TABLE element slot. Surfaced + # at `create table T (` so the column-name role is visible + # alongside the table-level constraint keywords. + create_table_element: "Type a column name, or a table-level constraint: `primary`, `unique`, `check`, `constraint`, `foreign`" # Value-literal slot — `insert ... values (`, `update ... set # col=`, `where col=`. Replaces the misleading "null true # false" keyword candidate list with format guidance for all diff --git a/src/input_render.rs b/src/input_render.rs index b05bb3e..f7286c0 100644 --- a/src/input_render.rs +++ b/src/input_render.rs @@ -693,6 +693,17 @@ fn ambient_hint_core_in_mode( return Some(AmbientHint::Prose(text)); } } + Some(crate::dsl::grammar::HintMode::IntroProse(key)) => { + // Slot entry: surface the catalog prose so an + // invisible-by-default ident slot (the column-name + // `NewName` at the CREATE TABLE element position, + // issue #4) reads as the dominant first move with + // the keyword alternatives folded into the prose. + // Tab candidates remain available via the parallel + // completion surface; the user still cycles the + // keyword set. + return Some(AmbientHint::Prose(crate::friendly::translate(key, &[]))); + } Some(crate::dsl::grammar::HintMode::SuppressProse | crate::dsl::grammar::HintMode::Default) | None => {} } @@ -1111,6 +1122,143 @@ mod tests { assert!(ambient_hint(" ", 3, None, &empty_cache()).is_none()); } + #[test] + fn advanced_create_table_element_position_introduces_column_name() { + // Issue #4: at `create table T (`, the user is at the + // ELEMENT slot of the column-def list. The current candidate + // list shows only table-level constraint keywords (`primary`, + // `unique`, `check`, `constraint`, `foreign`); a new column + // is the dominant first move and is currently invisible + // because the COLUMN_DEF branch starts with an `Ident::NewName` + // slot which produces no concrete candidate. + // + // The fix wraps the ELEMENT choice in a `Hinted::IntroProse` + // that surfaces a prose hint mentioning the column name first, + // with the constraint keywords as the alternative. Tab + // candidates remain available. + let cache = crate::completion::SchemaCache::default(); + let input = "create table Orders ("; + match ambient_hint_in_mode(input, input.len(), None, &cache, Mode::Advanced) { + Some(AmbientHint::Prose(p)) => { + assert!( + p.to_lowercase().contains("column name"), + "prose must mention `column name`; got: {p:?}", + ); + // Constraint alternatives should still be mentioned. + assert!( + p.contains("primary") && p.contains("unique"), + "prose should mention constraint alternatives; got: {p:?}", + ); + } + other => panic!("expected Prose hint at ELEMENT slot; got: {other:?}"), + } + // Tab candidates should remain available (the keywords still cycle). + let comp = crate::completion::candidates_at_cursor_in_mode( + input, + input.len(), + &cache, + Mode::Advanced, + ) + .expect("completion must remain available"); + let texts: Vec<&str> = comp.candidates.iter().map(|c| c.text.as_str()).collect(); + for kw in &["primary", "unique"] { + assert!( + texts.contains(kw), + "Tab candidate `{kw}` must remain; got {texts:?}", + ); + } + } + + #[test] + fn advanced_partial_typing_does_not_leak_bare_double_in_prose() { + // Issue #5 (prose half): at `create table Orders (count` (no + // trailing space), the user is mid-typing what's + // grammatically a column name (`count` could be the start of + // `counterparty`). The bare `Word("double")` from the + // DOUBLE_PRECISION_NODES branch must not appear in the + // ambient hint at this position — the new IntroProse hint + // from issue #4 already covers this position by introducing + // the element slot ("Type a column name, or a table-level + // constraint: …"), and the user discovers the type list + // (with `double precision` as a single composite, not bare + // `double`) when they advance to the SQL_TYPE slot. + let cache = crate::completion::SchemaCache::default(); + let input = "create table Orders (count"; + match ambient_hint_in_mode(input, input.len(), None, &cache, Mode::Advanced) { + Some(AmbientHint::Prose(p)) => { + assert!( + !p.contains("`double`"), + "bare `double` must not appear in the prose; got: {p:?}", + ); + } + other => panic!("expected Prose hint at partial column name; got: {other:?}"), + } + } + + #[test] + fn advanced_type_position_offers_double_precision_not_bare_double() { + // Issue #5: at the SQL_TYPE position (`create table Orders + // (count `), the candidate list previously surfaced `double` + // as a peer of the playground's regular types — the user + // sees a leading "double" alongside int/text/etc. and has + // to know it's the start of the two-word `double precision` + // alias. The fix surfaces `double precision` as a single + // composite candidate and suppresses the bare `double`. + let cache = crate::completion::SchemaCache::default(); + let input = "create table Orders (count "; + let comp = crate::completion::candidates_at_cursor_in_mode( + input, + input.len(), + &cache, + Mode::Advanced, + ) + .expect("completion expected at the SQL_TYPE position"); + let texts: Vec<&str> = comp.candidates.iter().map(|c| c.text.as_str()).collect(); + assert!( + !texts.contains(&"double"), + "bare `double` must NOT appear as a type candidate; got {texts:?}", + ); + assert!( + texts.contains(&"double precision"), + "`double precision` should appear as a composite type candidate; got {texts:?}", + ); + // The regular type vocabulary still appears. + for t in &["int", "text", "real", "serial"] { + assert!( + texts.iter().any(|x| x == t), + "regular type `{t}` must remain a candidate; got {texts:?}", + ); + } + } + + #[test] + fn advanced_create_table_offers_open_paren_after_name() { + // Issue #3: typing `create table Orders ` in advanced mode + // should offer both `with` (DSL form, ADR-0009) and `(` (SQL + // form, ADR-0035 §4) as the next-step continuation. Today + // only `with` surfaces — the shared-entry-word completion + // merge only fires at the entry-word boundary, so deeper + // positions show only the committed node's continuations. + let cache = crate::completion::SchemaCache::default(); + let input = "create table Orders "; + let comp = crate::completion::candidates_at_cursor_in_mode( + input, + input.len(), + &cache, + Mode::Advanced, + ) + .expect("completion expected for advanced create-table after name"); + let texts: Vec<&str> = comp.candidates.iter().map(|c| c.text.as_str()).collect(); + assert!( + texts.contains(&"("), + "advanced mode must offer `(` for the SQL column-def list; got {texts:?}", + ); + assert!( + texts.contains(&"with"), + "advanced mode must keep `with` for the DSL form; got {texts:?}", + ); + } + #[test] fn advanced_mode_ambient_offers_sql_from_slot_candidate() { // ADR-0022 Amendment 1: advanced-mode ambient assistance