diff --git a/src/completion.rs b/src/completion.rs index 31fa2aa..5bb1580 100644 --- a/src/completion.rs +++ b/src/completion.rs @@ -1123,6 +1123,21 @@ pub fn invalid_ident_at_cursor_in_mode( if expected.is_empty() { return None; } + // Issue #6 follow-on: at a SQL expression position the partial + // could resolve to either a column reference *or* a function-call + // name (the grammar admits the call shape structurally — see + // sql_expr.rs's `CALL_ARGS` comment, "it does not know which + // names are aggregates"). Without lookahead for a trailing `(`, + // we can't tell here, so we don't flag — submit-time validation + // still catches a genuine column typo via the `unknown_column` + // diagnostic, which only skips when the ident *is* followed by + // `(` (i.e. it really is a function call). + let has_sql_expr_slot = expected.iter().any(|e| { + matches!(e, Expectation::Ident { role: "sql_expr_ident", .. }) + }); + if has_sql_expr_slot { + return None; + } // Find every schema-listable source in the expected list. let sources: Vec = expected .iter() diff --git a/src/dsl/walker/mod.rs b/src/dsl/walker/mod.rs index 5eba5b8..e44dd68 100644 --- a/src/dsl/walker/mod.rs +++ b/src/dsl/walker/mod.rs @@ -1034,6 +1034,18 @@ fn schema_existence_diagnostics( // check on the next iteration. pending_qualifier = Some((item.text.clone(), item.span)); + } else if role == "sql_expr_ident" + && is_followed_by_call_args(&path.items, i) + { + // Issue #6: this ident is the *name* of a function + // call (immediately followed by `(`), not a column + // reference. The grammar admits the call shape + // structurally (sql_expr.rs's `CALL_ARGS` comment: + // "it does not know which names are aggregates"), + // so the validator skips here. The arguments + // *inside* the call are ordinary expressions and + // their idents flow through the bare-column check + // on subsequent iterations. } else if let Some(target) = (role == "sql_expr_ident") .then(|| { bare_ref_insert_target( @@ -2015,6 +2027,25 @@ fn is_followed_by_qualified_ref( ) } +/// True when the ident at `items[i]` is the *name* of a function call — +/// the immediately following item is `(`. Mirrors the qualified-ref +/// guard and lets the bare-column check in +/// `schema_existence_diagnostics` skip validating a function-call name +/// against the schema (issue #6). The grammar admits the call shape +/// structurally (sql_expr.rs's CALL_ARGS comment: "it does not know +/// which names are aggregates"), so this validator must match: an +/// ident followed by `(` is a function-call name, not a column ref. +fn is_followed_by_call_args( + items: &[outcome::MatchedItem], + i: usize, +) -> bool { + use outcome::MatchedKind; + matches!( + items.get(i + 1).map(|it| &it.kind), + Some(MatchedKind::Punct('(')) + ) +} + fn schema_has_table(schema: &crate::completion::SchemaCache, name: &str) -> bool { schema.tables.iter().any(|t| t.eq_ignore_ascii_case(name)) } @@ -5399,6 +5430,85 @@ mod tests { ); } + #[test] + fn select_aggregate_function_name_not_flagged_as_unknown_column() { + // Issue #6: `select sum(Age) from Customers` runs cleanly at + // the engine but the schema-existence pass was treating `sum` + // as a bare column reference and flagging "no such column + // `sum` on table `Customers`". The grammar admits the + // function-call shape structurally (sql_expr.rs comment at + // CALL_ARGS, ADR-0031 §1: "it does not know which names are + // aggregates"), so the validator must match: an ident + // followed by `(` is a function-call name, not a column + // reference. + let schema = schema_with("Customers", &[("Age", Type::Int)]); + for input in [ + // The five standard SQL aggregates … + "select sum(Age) from Customers", + "select avg(Age) from Customers", + "select count(*) from Customers", + "select count(Age) from Customers", + "select min(Age) from Customers", + "select max(Age) from Customers", + // … `count(distinct …)` (ADR-0031 §1 — the one place + // DISTINCT leads the argument list) … + "select count(distinct Age) from Customers", + // … and non-aggregate functions, which get the same + // treatment by the same fix because the grammar already + // accepts them and the engine evaluates them. + "select length(Age) from Customers", + "select coalesce(Age, 0) from Customers", + // Nested calls — the outer name is followed by `(` (skip), + // the inner name is followed by `(` (skip), the innermost + // bare ident is followed by `)` and gets validated. + "select upper(lower(Age)) from Customers", + // Functions in WHERE: same `sql_expr_ident` role, same + // skip guard applies. + "select Age from Customers where length(Age) > 0", + ] { + let diags = diag_keys(input, &schema); + assert!( + !diags.iter().any(|d| d.contains("no such column")), + "function-call name must not trip `no such column` for {input:?}; got {diags:?}", + ); + } + } + + #[test] + fn select_unknown_column_inside_distinct_arg_still_flagged() { + // ADR-0031 §1 calls out `count(distinct col)` as a special + // argument shape. The argument is still an ordinary column + // reference and a genuinely unknown one must still flag — + // the DISTINCT keyword doesn't shield the column-ref + // validation downstream. + let schema = schema_with("Customers", &[("Age", Type::Int)]); + let diags = diag_keys( + "select count(distinct nonexistent) from Customers", + &schema, + ); + assert!( + diags.iter().any(|d| d.contains("no such column")), + "unknown column inside `count(distinct …)` must still fire; got {diags:?}", + ); + } + + #[test] + fn select_unknown_argument_inside_function_call_still_flagged() { + // Symmetric guard: the fix only skips validation of the + // function-call *name*; the argument expressions are still + // ordinary expressions and a genuinely unknown column inside + // a call must still fire `no such column`. + let schema = schema_with("Customers", &[("Age", Type::Int)]); + let diags = diag_keys( + "select sum(nonexistent) from Customers", + &schema, + ); + assert!( + diags.iter().any(|d| d.contains("no such column")), + "unknown column inside the call args must still fire; got {diags:?}", + ); + } + #[test] fn insert_select_unknown_projection_column_is_error() { // ADR-0033 sub-phase 3c cross-cut: the Phase-2 diff --git a/src/input_render.rs b/src/input_render.rs index f7286c0..dfc2b7f 100644 --- a/src/input_render.rs +++ b/src/input_render.rs @@ -1169,6 +1169,87 @@ mod tests { } } + #[test] + fn genuine_column_typo_in_complete_select_still_hints_via_diagnostic() { + // Issue #6 trade-off lockdown: dropping the typing-time + // `invalid_ident_at_cursor` flag at `sql_expr_ident` positions + // (to avoid the false positive on function names like `sum`) + // must not silently kill the typo signal for *genuine* column + // typos. Once the SELECT is structurally complete (FROM is in + // scope), the schema-existence pass fires `unknown_column` + // and the ambient hint surfaces that diagnostic via + // `pick_hint_diagnostic`. The user still gets the typing-time + // warning, just through a different path. + use crate::completion::{SchemaCache, TableColumn}; + use crate::dsl::types::Type; + let mut cache = SchemaCache::default(); + cache.tables.push("Customers".to_string()); + let tc = vec![TableColumn { + name: "Age".to_string(), + user_type: Type::Int, + not_null: false, + has_default: false, + }]; + for c in &tc { + cache.columns.push(c.name.clone()); + } + cache.table_columns.insert("Customers".to_string(), tc); + for input in [ + "select Agx from Customers", + "select * from Customers where Agx = 5", + "select * from Customers where Agx", + ] { + match ambient_hint_in_mode(input, input.len(), None, &cache, Mode::Advanced) { + Some(AmbientHint::Prose(p)) => assert!( + p.contains("no such column") && p.contains("Agx"), + "complete SELECT with column typo must surface the diagnostic hint for {input:?}; got: {p:?}", + ), + other => panic!( + "complete SELECT with column typo must produce a Prose hint for {input:?}; got: {other:?}", + ), + } + } + } + + #[test] + fn advanced_select_partial_function_name_not_flagged_as_invalid_column() { + // Issue #6 follow-on: while the user is typing + // `select sum` (no `(` yet), the ambient hint must not + // pre-emptively show "No such column: `sum`". At a SQL + // expression position the partial could resolve to either a + // column reference *or* a function-call name; the typing-time + // `invalid_ident_at_cursor` check would otherwise mislead + // because it only knows about schema columns. Submit-time + // validation still flags a genuine column typo (the + // `unknown_column` diagnostic only skips function-call names, + // which require a trailing `(` — so a bare unknown ident + // still trips at submit). + use crate::completion::{SchemaCache, TableColumn}; + use crate::dsl::types::Type; + let mut cache = SchemaCache::default(); + cache.tables.push("Customers".to_string()); + let tc = vec![ + TableColumn { + name: "Age".to_string(), + user_type: Type::Int, + not_null: false, + has_default: false, + }, + ]; + for c in &tc { + cache.columns.push(c.name.clone()); + } + cache.table_columns.insert("Customers".to_string(), tc); + let input = "select sum"; + let hint = ambient_hint_in_mode(input, input.len(), None, &cache, Mode::Advanced); + if let Some(AmbientHint::Prose(p)) = &hint { + assert!( + !p.contains("No such column"), + "`select sum` mid-typing must not pre-emptively flag `sum` as an invalid column; got: {p:?}", + ); + } + } + #[test] fn advanced_partial_typing_does_not_leak_bare_double_in_prose() { // Issue #5 (prose half): at `create table Orders (count` (no