From 3912fb5a9b959b0ae2b0846246323d43f88fce63 Mon Sep 17 00:00:00 2001 From: "claude@clouddev1" Date: Tue, 19 May 2026 09:24:44 +0000 Subject: [PATCH] walker: precise per-literal spans for expression WARNINGs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Expression WARNING diagnostics (type mismatch, = NULL) carried a coarse span — the whole WHERE clause, from the `where` keyword to end of input. They now span exactly the offending literal operand, read from the Operand source span added in the previous commit. predicate_warnings derives the span per warning; pair_type_mismatch returns (message, literal-span); the dead where_clause_span helper is removed. 5 walker tests assert the spans cover exactly the literal / identifier (type mismatch, = NULL, BETWEEN bounds, IN item, unknown-column ERROR). 1105 passing, clippy clean. --- src/dsl/walker/mod.rs | 178 +++++++++++++++++++++++++++++------------- 1 file changed, 125 insertions(+), 53 deletions(-) diff --git a/src/dsl/walker/mod.rs b/src/dsl/walker/mod.rs index 56a84f5..240009e 100644 --- a/src/dsl/walker/mod.rs +++ b/src/dsl/walker/mod.rs @@ -441,45 +441,35 @@ const fn command_where_expr(command: &Command) -> Option<&Expr> { } } -/// A coarse span covering the WHERE clause — from the `where` -/// keyword to the end of input. The validity indicator reads -/// only the severity; the span is for the (eventual) -/// highlight overlay. -fn where_clause_span(path: &MatchedPath, source_len: usize) -> (usize, usize) { - path.items - .iter() - .find(|i| matches!(&i.kind, outcome::MatchedKind::Word("where"))) - .map_or((0, source_len), |w| (w.span.0, source_len)) -} - /// WARNING diagnostics for a WHERE expression (ADR-0026 §7): /// a type-mismatched comparison, or `= NULL` / `!= NULL`. /// Both are valid and runnable — the warning is advisory. +/// +/// Each diagnostic's span is the offending **literal operand** +/// — precise enough for a per-literal highlight (ADR-0027). fn expr_warnings( expr: &Expr, columns: &[crate::completion::TableColumn], - span: (usize, usize), ) -> Vec { let mut out = Vec::new(); - collect_expr_warnings(expr, columns, span, &mut out); + collect_expr_warnings(expr, columns, &mut out); out } fn collect_expr_warnings( expr: &Expr, columns: &[crate::completion::TableColumn], - span: (usize, usize), out: &mut Vec, ) { match expr { Expr::Or(terms) | Expr::And(terms) => { for term in terms { - collect_expr_warnings(term, columns, span, out); + collect_expr_warnings(term, columns, out); } } - Expr::Not(inner) => collect_expr_warnings(inner, columns, span, out), + Expr::Not(inner) => collect_expr_warnings(inner, columns, out), Expr::Predicate(predicate) => { - predicate_warnings(predicate, columns, span, out); + predicate_warnings(predicate, columns, out); } } } @@ -487,11 +477,10 @@ fn collect_expr_warnings( fn predicate_warnings( predicate: &Predicate, columns: &[crate::completion::TableColumn], - span: (usize, usize), out: &mut Vec, ) { use outcome::{Diagnostic, Severity}; - let warn = |message: String| Diagnostic { + let warn = |message: String, span: (usize, usize)| Diagnostic { severity: Severity::Warning, span, message, @@ -500,36 +489,40 @@ fn predicate_warnings( Predicate::Compare { left, op, right } => { // `= NULL` / `!= NULL`: valid syntax that is never // true — the user almost certainly means IS NULL. - if matches!(op, CompareOp::Eq | CompareOp::NotEq) - && (is_null_literal(left) || is_null_literal(right)) - { - out.push(warn(crate::friendly::translate( - "diagnostic.eq_null", - &[], - ))); - } else if let Some(message) = + // The highlight points at the `null` literal itself. + let null_operand = if matches!(op, CompareOp::Eq | CompareOp::NotEq) { + [left, right].into_iter().find(|&o| is_null_literal(o)) + } else { + None + }; + if let Some(operand) = null_operand { + out.push(warn( + crate::friendly::translate("diagnostic.eq_null", &[]), + operand.span(), + )); + } else if let Some((message, span)) = pair_type_mismatch(left, right, columns) { - out.push(warn(message)); + out.push(warn(message, span)); } } Predicate::Between { target, low, high, .. } => { for bound in [low, high] { - if let Some(message) = + if let Some((message, span)) = pair_type_mismatch(target, bound, columns) { - out.push(warn(message)); + out.push(warn(message, span)); } } } Predicate::In { target, items, .. } => { for item in items { - if let Some(message) = + if let Some((message, span)) = pair_type_mismatch(target, item, columns) { - out.push(warn(message)); + out.push(warn(message, span)); } } } @@ -552,20 +545,24 @@ const fn is_null_literal(operand: &Operand) -> bool { } /// If one operand is a known column and the other a non-null -/// literal whose type the column cannot hold, the message for -/// a type-mismatch WARNING; otherwise `None` (column-to-column, -/// literal-to-literal, an unknown column — already an ERROR — -/// or a compatible pair). +/// literal whose type the column cannot hold, the type-mismatch +/// WARNING message paired with the **literal operand's span**; +/// otherwise `None` (column-to-column, literal-to-literal, an +/// unknown column — already an ERROR — or a compatible pair). fn pair_type_mismatch( a: &Operand, b: &Operand, columns: &[crate::completion::TableColumn], -) -> Option { - let (column, literal) = match (a, b) { - (Operand::Column { name: c, .. }, Operand::Literal { value: v, .. }) - | (Operand::Literal { value: v, .. }, Operand::Column { name: c, .. }) => { - (c, v) - } +) -> Option<(String, (usize, usize))> { + let (column, literal, span) = match (a, b) { + ( + Operand::Column { name, .. }, + Operand::Literal { value, span }, + ) + | ( + Operand::Literal { value, span }, + Operand::Column { name, .. }, + ) => (name, value, *span), _ => return None, }; // `null` fits any column; `= NULL` is flagged separately. @@ -579,12 +576,15 @@ fn pair_type_mismatch( if literal.bind_for_column(column, ty).is_ok() { return None; } - Some(crate::friendly::translate( - "diagnostic.type_mismatch", - &[ - ("column", column as &dyn std::fmt::Display), - ("type", &ty.keyword() as &dyn std::fmt::Display), - ], + Some(( + crate::friendly::translate( + "diagnostic.type_mismatch", + &[ + ("column", column as &dyn std::fmt::Display), + ("type", &ty.keyword() as &dyn std::fmt::Display), + ], + ), + span, )) } @@ -878,11 +878,7 @@ pub fn walk<'a>( && let Some(expr) = command_where_expr(command) { let columns = ctx.current_table_columns.as_deref().unwrap_or(&[]); - diagnostics.extend(expr_warnings( - expr, - columns, - where_clause_span(&path, effective_source.len()), - )); + diagnostics.extend(expr_warnings(expr, columns)); } let result = WalkResult { outcome: final_outcome, @@ -1744,6 +1740,82 @@ mod tests { } } + // ---- precise diagnostic spans (ADR-0027 highlight wiring) - + + /// Walk `input` with `schema` and return the diagnostics the + /// walk produced. + fn diagnostics( + input: &str, + schema: &SchemaCache, + ) -> Vec { + let mut ctx = super::context::WalkContext::with_schema(schema); + let (result, _cmd) = + super::walk(input, super::outcome::WalkBound::EndOfInput, &mut ctx); + result.map_or_else(Vec::new, |r| r.diagnostics) + } + + #[test] + fn type_mismatch_warning_span_covers_only_the_literal() { + // The coarse pre-ADR-0027 span was the whole WHERE + // clause; it is now exactly the offending literal. + let schema = schema_with("Customers", &[("Age", Type::Int)]); + let input = "delete from Customers where Age = 'hello'"; + let diags = diagnostics(input, &schema); + assert_eq!(diags.len(), 1); + assert_eq!(diags[0].severity, super::Severity::Warning); + let (s, e) = diags[0].span; + assert_eq!( + &input[s..e], + "'hello'", + "the WARNING span should cover exactly the literal", + ); + } + + #[test] + fn eq_null_warning_span_covers_the_null_literal() { + let schema = schema_with("Customers", &[("Name", Type::Text)]); + let input = "delete from Customers where Name = null"; + let diags = diagnostics(input, &schema); + assert_eq!(diags.len(), 1); + let (s, e) = diags[0].span; + assert_eq!(&input[s..e], "null"); + } + + #[test] + fn between_warning_spans_each_offending_bound() { + // `Age` is int; both text bounds mismatch — two + // distinct WARNINGs, each spanning its own bound. + let schema = schema_with("Customers", &[("Age", Type::Int)]); + let input = "delete from Customers where Age between 'a' and 'z'"; + let diags = diagnostics(input, &schema); + assert_eq!(diags.len(), 2); + let spans: Vec<&str> = + diags.iter().map(|d| &input[d.span.0..d.span.1]).collect(); + assert_eq!(spans, vec!["'a'", "'z'"]); + } + + #[test] + fn in_warning_spans_only_the_mismatched_item() { + // `Age` is int; of `(1, 'two', 3)` only `'two'` is wrong. + let schema = schema_with("Customers", &[("Age", Type::Int)]); + let input = "delete from Customers where Age in (1, 'two', 3)"; + let diags = diagnostics(input, &schema); + assert_eq!(diags.len(), 1); + let (s, e) = diags[0].span; + assert_eq!(&input[s..e], "'two'"); + } + + #[test] + fn unknown_column_error_span_covers_the_identifier() { + let schema = schema_with("Customers", &[("id", Type::Int)]); + let input = "delete from Customers where NoSuchCol = 1"; + let diags = diagnostics(input, &schema); + assert_eq!(diags.len(), 1); + assert_eq!(diags[0].severity, super::Severity::Error); + let (s, e) = diags[0].span; + assert_eq!(&input[s..e], "NoSuchCol"); + } + #[test] fn walker_parses_insert_with_explicit_column_list() { assert_eq!(