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 {