From 5815918efb33471603f444c6530bb8f97227496b Mon Sep 17 00:00:00 2001 From: "claude@clouddev1" Date: Fri, 15 May 2026 18:58:28 +0000 Subject: [PATCH] Hint: surface ( as a branching candidate; stop red-flagging in-progress Form A values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related fixes from a user-reported snag: 1. After typing \`insert into Orders \`, the hint suggested only \`values\` even though the user could also choose \`(\` to open Form A (the explicit-column-list variant). The walker reports both \`Expectation::Word("values")\` and \`Expectation::Punct('(')\` at that position, but \`candidates_at_cursor\` had a blanket "no punctuation as Tab candidate" policy. Loosened the policy to surface branching punct (specifically \`(\` opening a sub-shape). Closing punct (\`)\`), separators (\`,\`), and content-trailing punct (\`:\`, \`=\`, \`.\`) stay out — the user types those naturally and advertising them in the Tab menu is noise. New \`CandidateKind::Punct\` so the renderer colors it as punct rather than mis-classifying as a keyword. 2. While typing \`insert into Orders (id, CustId, Total) values (42, 89, 17.59\` (no closing paren yet), the word \`values\` was rendered in \`tok_error\` red. The walker's \`Optional(Seq[values, '(', list, ')'])\` was rolling back on the partial inner match — treating \`(id, CustId, Total)\` as Form C (bare value list) followed by trailing junk starting at \`values\`. The classify_input call thus returned \`DefiniteErrorAt()\` and the renderer overlaid. Tightened \`walk_optional\`: roll back only when the inner reports NoMatch (or Incomplete / Mismatch without consuming anything). Once the inner has committed to at least one terminal (e.g. matched the \`values\` keyword), propagate Incomplete / Mismatch up — the user is mid-typing the optional's content and rolling back would lose their intent. The pre-existing chumsky-or_not-style aggressive rollback covered cases like \`save Customers\` (Optional(\`as\`) inner is a single Word that returns NoMatch without consuming, so rollback still fires). Those keep working. 3. Side effect: with \`Optional\` no longer hiding the in-progress Form A from the leading slice, the walker on \`create table T with \` correctly reports the next-expected keyword as \`pk\` — so cursor at the end of the complete command \`create table T with pk\` would now re-offer \`pk\` as a Tab candidate against the partial \"pk\". Added a final filter: when the full input is a valid parse AND the partial prefix is non-empty, drop candidates that equal the partial exactly. Preserves schema narrowing (\`show data Cu\` → \`Customers\` is not an exact match). Tests: - New \`in_progress_form_a_values_list_classifies_as_incomplete\` asserts the input-state for the user's exact scenario. - New \`open_paren_branching_punct_surfaces_after_insert_into_table\` and \`open_paren_candidate_is_classified_as_punct_kind\` cover the punct-as-candidate surface. - Renamed and rewrote \`punctuation_expected_does_not_produce_candidates\` to \`non_branching_punctuation_is_not_surfaced_as_candidate\` to document the new finer-grained policy. - Existing tests for \`save Tab → as\` and the schema- narrowing case continue to pass. Tests: 854 passing, 0 failing, 1 ignored. Clippy clean. --- src/completion.rs | 96 +++++++++++++++++++++++++++++++++++++--- src/dsl/walker/driver.rs | 41 +++++++++++------ src/input_render.rs | 23 ++++++++++ src/ui.rs | 1 + 4 files changed, 142 insertions(+), 19 deletions(-) diff --git a/src/completion.rs b/src/completion.rs index d86b18c..1800865 100644 --- a/src/completion.rs +++ b/src/completion.rs @@ -133,6 +133,13 @@ pub enum CandidateKind { /// A `--name`-style flag. Coloured with `tok_flag` so the /// hint matches the way it'll render in the input pane. Flag, + /// A single-char punctuation token the walker expects next + /// (e.g. `(` at the start of `insert into T (cols)`). Used + /// to surface branching alternatives the user might not + /// otherwise discover — at `insert into Orders ` the walker + /// expects either `values` or `(`, and surfacing both makes + /// the Form A path discoverable. + Punct, } #[derive(Debug, Clone, PartialEq, Eq)] @@ -203,6 +210,18 @@ pub fn candidates_at_cursor_with( let partial_prefix = input[start..cursor].to_string(); let leading = &input[..start]; + // When the full input already parses, the cursor is at + // the end of a "complete enough" command — but the leading + // slice still reports an expected set (the words the user + // just finished typing, or optional-suffix continuations). + // We let the completion engine run normally, then below + // filter out candidates that exactly equal the partial + // prefix the user already typed: it's not useful to suggest + // `pk` at the end of `create table T with pk`. The + // optional-suffix case (`save ` → `as`) is preserved + // because there `partial_prefix` is empty. + let input_parses_complete = parse_command(input).is_ok(); + let expected = expected_at(leading); if expected.is_empty() { return None; @@ -310,6 +329,27 @@ pub fn candidates_at_cursor_with( .filter(|s| matches_prefix(s)) .collect(); + // Source 1.7: branching-punct candidates. At positions + // where the walker expects a punct character that opens a + // sub-shape (notably `(` opening Form A or C of insert), + // surface the punct as a Tab candidate so the user + // discovers the option. Closing punct (`)`, `,`, etc.) and + // expected-after-content punct (`:`, `=`, `.`) are not + // surfaced — they're trailing terminals the user types + // naturally, not "shape branches" worth advertising in the + // hint panel. + let punct_candidates: Vec = if partial_prefix.is_empty() { + expected + .iter() + .filter_map(|e| match e { + Expectation::Punct('(') => Some("(".to_string()), + _ => None, + }) + .collect() + } else { + Vec::new() + }; + // Source 2: schema identifiers — accumulated across every // matching schema-listable `Ident { source }` expectation. // `NewName` / `Types` / `Free` sources don't query the @@ -337,11 +377,13 @@ pub fn candidates_at_cursor_with( // Keywords first (grammar parts read before content), // then type names (closed-set grammar — coloured as // keywords), then composite literals (`1:n`, …), then - // flags (own colour), then schema identifiers. + // branching punct (`(` opening a sub-shape), then flags + // (own colour), then schema identifiers. let mut candidates: Vec = Vec::with_capacity( keywords.len() + type_names.len() + composites.len() + + punct_candidates.len() + flags.len() + identifiers.len(), ); @@ -357,6 +399,10 @@ pub fn candidates_at_cursor_with( text, kind: CandidateKind::Keyword, })); + candidates.extend(punct_candidates.into_iter().map(|text| Candidate { + text, + kind: CandidateKind::Punct, + })); candidates.extend(flags.into_iter().map(|text| Candidate { text, kind: CandidateKind::Flag, @@ -370,6 +416,17 @@ pub fn candidates_at_cursor_with( return None; } + // When the input is already a valid complete command, drop + // candidates that exactly match the partial prefix — those + // are the words the user just finished typing (e.g. `pk` in + // `create table T with pk`), not useful suggestions. Keeps + // schema-narrowing intact (`show data Cu` → `Customers` is + // not an exact match; preserved). + if input_parses_complete && !partial_prefix.is_empty() { + let lowered_partial = partial_prefix.to_lowercase(); + candidates.retain(|c| c.text.to_lowercase() != lowered_partial); + } + let candidates = ranker(candidates); if candidates.is_empty() { return None; @@ -959,12 +1016,41 @@ mod tests { } #[test] - fn punctuation_expected_does_not_produce_candidates() { - // After `add column to table T` parser expects `:`. - // Tab should NOT offer the colon character. + fn non_branching_punctuation_is_not_surfaced_as_candidate() { + // After `add column to table T` the walker expects `:`. + // `:` is a "trailing-content" punct — the user types it + // naturally as they continue the command, so the hint + // panel doesn't surface it. Only branching punct (`(` + // opening a sub-shape) becomes a Tab candidate. let input = "add column to table T"; let cs = cands(input, input.len()); - assert!(cs.is_empty(), "punctuation should not be offered: {cs:?}"); + assert!(cs.is_empty(), "trailing-content punct should not surface: {cs:?}"); + } + + #[test] + fn open_paren_branching_punct_surfaces_after_insert_into_table() { + // After `insert into Orders ` the walker expects either + // `values` (Form B) or `(` (Forms A / C). Both surface + // as Tab candidates so the user discovers the column- + // list form. + let cs = cands("insert into Orders ", 19); + assert!(cs.contains(&"values".to_string()), "got {cs:?}"); + assert!(cs.contains(&"(".to_string()), "got {cs:?}"); + } + + #[test] + fn open_paren_candidate_is_classified_as_punct_kind() { + // The `(` candidate gets its own kind so the hint + // renderer can colour it as punctuation rather than + // mis-classifying it as a keyword. + let comp = candidates_at_cursor("insert into Orders ", 19, &SchemaCache::default()) + .expect("some completion"); + let paren = comp + .candidates + .iter() + .find(|c| c.text == "(") + .expect("( present"); + assert_eq!(paren.kind, CandidateKind::Punct); } #[test] diff --git a/src/dsl/walker/driver.rs b/src/dsl/walker/driver.rs index dcb83ec..fc603ce 100644 --- a/src/dsl/walker/driver.rs +++ b/src/dsl/walker/driver.rs @@ -693,11 +693,14 @@ fn walk_optional( ) -> NodeWalkResult { let saved_path_len = path.items.len(); let saved_byte_len = per_byte.len(); - match walk_node(source, position, child, ctx, path, per_byte) { + let result = walk_node(source, position, child, ctx, path, per_byte); + let inner_committed = path.items.len() > saved_path_len; + match result { m @ NodeWalkResult::Matched { .. } => m, NodeWalkResult::NoMatch { expected, .. } => { - // Skip the optional but carry the inner's expectations - // so the caller's expected-set sees them. + // Inner didn't engage at all — skip the Optional + // but carry the inner's expectations so the caller's + // expected-set sees them. path.items.truncate(saved_path_len); per_byte.truncate(saved_byte_len); NodeWalkResult::Matched { @@ -705,17 +708,13 @@ fn walk_optional( skipped: expected, } } - // Partial-match failure (mid-shape Incomplete or - // structural Mismatch). Match chumsky's `or_not` - // semantics: roll back any state the partial match - // pushed and treat the optional as skipped, carrying - // the partial's expected-set as `skipped` so callers - // can surface "what would have completed it" at the - // failure point. Validation errors do NOT backtrack — - // those are content failures the user means to fix. - NodeWalkResult::Incomplete { expected, .. } => { + NodeWalkResult::Incomplete { position: p, expected } if !inner_committed => { + // Inner reported Incomplete without consuming + // anything — same as NoMatch from the user's + // perspective. Roll back and skip. path.items.truncate(saved_path_len); per_byte.truncate(saved_byte_len); + let _ = p; NodeWalkResult::Matched { end: position, skipped: expected, @@ -724,7 +723,9 @@ fn walk_optional( NodeWalkResult::Failed { kind: FailureKind::Mismatch { expected }, .. - } => { + } if !inner_committed => { + // Inner reported Mismatch without consuming + // anything — roll back and skip. path.items.truncate(saved_path_len); per_byte.truncate(saved_byte_len); NodeWalkResult::Matched { @@ -732,7 +733,19 @@ fn walk_optional( skipped: expected, } } - validation_failure @ NodeWalkResult::Failed { .. } => validation_failure, + // Inner committed (consumed at least one terminal) but + // then ran out / hit a mismatch. Propagate the failure + // up — the user is mid-typing the optional's content and + // we'd lose their intent by rolling back. (Pre-fix + // behavior matched chumsky's `or_not` rollback, but + // that conflates "Form A in progress" with "Form C with + // trailing junk" — see e.g. `insert into T (a, b, c) + // values (1, 2, 3` losing the `values (…)` partial.) + // Validation failures already propagate as a separate + // branch below. + propagated @ (NodeWalkResult::Incomplete { .. } | NodeWalkResult::Failed { .. }) => { + propagated + } } } diff --git a/src/input_render.rs b/src/input_render.rs index 6c079fb..1b80d43 100644 --- a/src/input_render.rs +++ b/src/input_render.rs @@ -673,6 +673,29 @@ mod tests { } } + #[test] + fn in_progress_form_a_values_list_classifies_as_incomplete_not_definite_error() { + // Regression: typing `insert into T (a, b, c) values + // (1, 2, 3` (no closing paren yet) used to classify as + // DefiniteErrorAt() because the + // walker's Optional rolled back the partial values + // list, leaving the rest of the input as trailing junk + // — the renderer then overlaid `values` in red. After + // the walk_optional fix (only roll back when the inner + // hasn't committed), the Optional propagates Incomplete + // and the user sees no error overlay until they submit. + assert_eq!( + classify_input( + "insert into Orders (id, CustId, Total) values (42, 89, 17.59" + ), + InputState::IncompleteAtEof, + ); + assert_eq!( + classify_input("insert into Orders (id, CustId, Total) values (42, 89"), + InputState::IncompleteAtEof, + ); + } + #[test] fn ambient_hint_at_insert_first_value_mentions_column_name() { use crate::dsl::types::Type; diff --git a/src/ui.rs b/src/ui.rs index e53440f..3093e31 100644 --- a/src/ui.rs +++ b/src/ui.rs @@ -766,6 +766,7 @@ fn render_candidate_line( crate::completion::CandidateKind::Keyword => theme.tok_keyword, crate::completion::CandidateKind::Identifier => theme.tok_identifier, crate::completion::CandidateKind::Flag => theme.tok_flag, + crate::completion::CandidateKind::Punct => theme.tok_punct, }; let mut s = Style::default().fg(base_fg); if Some(i) == selected {