From 43c49f4d1b5cd82820a964b2f769579e9ee2818a Mon Sep 17 00:00:00 2001 From: "claude@clouddev1" Date: Thu, 21 May 2026 20:52:20 +0000 Subject: [PATCH] =?UTF-8?q?walker:=20F5=20=E2=80=94=20drop=20preceding-cla?= =?UTF-8?q?use=20keywords=20from=20committed-child=20Incomplete=20sets?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit walk_seq's Incomplete arm unconditionally merged the accumulated skipped-Optional expectations (pending_skipped) into the child's expected set. When a child committed terminals before going Incomplete (e.g. `order by` consumed, now awaiting a sort item), this leaked ~13 clause keywords from clauses positioned *before* the committed child — WHERE/GROUP BY/HAVING, the FROM's JOIN options, set-ops — into the ORDER BY completion list, shoving the actual columns off-screen. Merge pending_skipped only when the Incomplete-producing child consumed nothing (path length unchanged): the cursor still sits at the optional boundary, so those optionals are genuine alternatives. A committed child means the cursor is past them. Tests: walker expected-set guard (+ over-correction guard) and a full-stack completion-layer regression test. --- src/completion.rs | 32 +++++++++++++++++++++ src/dsl/walker/driver.rs | 21 ++++++++++++-- src/dsl/walker/mod.rs | 61 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 111 insertions(+), 3 deletions(-) diff --git a/src/completion.rs b/src/completion.rs index d504871..5d09fcc 100644 --- a/src/completion.rs +++ b/src/completion.rs @@ -1474,6 +1474,38 @@ mod tests { assert!(!cs.contains(&"Stock".to_string()), "got {cs:?}"); } + #[test] + fn order_by_completion_omits_preceding_clause_keywords() { + use crate::dsl::types::Type; + // F5 (handoff 30 §3.3): at `… order by ` the candidate + // list is the start of a sort item — the table's columns + // plus expression-start keywords. It must NOT be padded + // with clause keywords belonging to clauses positioned + // *before* ORDER BY (the FROM's JOIN options, WHERE / + // GROUP BY / HAVING, set-ops). Those used to shove the + // columns off-screen. + let cache = schema_with_table( + "Things", + &[("Name", Type::Text), ("Qty", Type::Int)], + ); + let input = "select Name from Things order by "; + let cs = cands_with(input, input.len(), &cache); + // The columns the user wants are offered: + assert!(cs.contains(&"Name".to_string()), "got {cs:?}"); + assert!(cs.contains(&"Qty".to_string()), "got {cs:?}"); + // Preceding-clause keywords must not leak in: + for kw in [ + "where", "group", "having", "join", "union", "intersect", + "except", "left", "right", "full", "cross", "inner", "as", + ] { + assert!( + !cs.contains(&kw.to_string()), + "preceding-clause keyword `{kw}` leaked into ORDER BY \ + completion; got {cs:?}", + ); + } + } + #[test] fn update_where_offers_only_current_table_columns() { use crate::dsl::types::Type; diff --git a/src/dsl/walker/driver.rs b/src/dsl/walker/driver.rs index f7b8d3e..b4d5119 100644 --- a/src/dsl/walker/driver.rs +++ b/src/dsl/walker/driver.rs @@ -850,6 +850,7 @@ fn walk_seq( // engine see optional connectives that haven't been typed. let mut pending_skipped: Vec = Vec::new(); for child in children { + let path_before = path.items.len(); match walk_node(source, cur, child, ctx, path, per_byte) { NodeWalkResult::Matched { end, skipped } => { if end == cur { @@ -900,9 +901,23 @@ fn walk_seq( position, mut expected, } => { - for e in std::mem::take(&mut pending_skipped) { - if !expected.contains(&e) { - expected.push(e); + // Only merge the skipped-Optional expectations when + // the Incomplete-producing child consumed nothing + // (path didn't grow): the cursor still sits at the + // optional boundary, so those optionals are genuine + // alternatives. If the child committed terminals + // (e.g. `order by` consumed, now awaiting a sort + // item) the cursor has moved *past* the skipped + // optionals — clauses positioned before this child + // are no longer valid continuations, so dropping + // `pending_skipped` keeps them out of the expected + // set (handoff 30 §3.3, F5). + let child_consumed = path.items.len() > path_before; + if !child_consumed { + for e in std::mem::take(&mut pending_skipped) { + if !expected.contains(&e) { + expected.push(e); + } } } return NodeWalkResult::Incomplete { position, expected }; diff --git a/src/dsl/walker/mod.rs b/src/dsl/walker/mod.rs index 85eb9c2..71833ea 100644 --- a/src/dsl/walker/mod.rs +++ b/src/dsl/walker/mod.rs @@ -4801,3 +4801,64 @@ mod dispatch_3a_tests { assert!(matches!(outcome, WalkOutcome::Match { .. }), "got {outcome:?}"); } } + +#[cfg(test)] +mod order_by_expected_set_tests { + //! F5 (handoff 30 §3.3) — when ORDER BY has consumed `order + //! by` and is awaiting a sort item, the expected set must not + //! be padded with clause keywords belonging to clauses that + //! sit *before* ORDER BY (the FROM's JOIN options, WHERE / + //! GROUP BY / HAVING, set-ops). Those optionals were skipped + //! earlier in the seq; once ORDER BY commits past them they + //! are no longer valid continuations at the cursor. + use super::*; + use crate::dsl::walker::outcome::Expectation; + use crate::mode::Mode; + + fn expected_words(source: &str) -> Vec<&'static str> { + expected_at_input_in_mode(source, Mode::Advanced) + .iter() + .filter_map(|e| match e { + Expectation::Word(w) => Some(*w), + _ => None, + }) + .collect() + } + + #[test] + fn order_by_excludes_preceding_clause_keywords() { + let words = expected_words("select Name from T order by "); + let preceding_clause_kw = [ + "where", "group", "having", "join", "union", "intersect", + "except", "left", "right", "full", "cross", "inner", "as", + ]; + let leaked: Vec<&str> = preceding_clause_kw + .iter() + .copied() + .filter(|k| words.contains(k)) + .collect(); + assert!( + leaked.is_empty(), + "ORDER BY expected set leaked preceding-clause keywords \ + {leaked:?}; full word set: {words:?}", + ); + } + + #[test] + fn order_by_still_offers_a_sort_item() { + // Guard against over-correction: the legitimate sort-item + // continuation (a column identifier) must survive the + // pending-skipped suppression. + let expected = expected_at_input_in_mode( + "select Name from T order by ", + Mode::Advanced, + ); + assert!( + expected.iter().any(|e| matches!( + e, + Expectation::Ident { .. } | Expectation::NumberLit + )), + "ORDER BY must still offer a sort item; got {expected:?}", + ); + } +}