walker: expression WARNING diagnostics (ADR-0027 step C, folds ADR-0026 §7)
Type-mismatched comparisons and `= NULL` / `!= NULL` in a WHERE expression now yield WARNING diagnostics — the command still parses and runs (the ADR-0026 §7 permissive posture is unchanged), but the validity indicator can flag it before submission. Computed post-walk from the built command's `Expr` against the table's column types: a Compare / Between / In with a column operand and a non-null literal whose type the column cannot hold, or a Compare with `=` / `!=` against NULL. New catalog keys `diagnostic.type_mismatch` / `diagnostic.eq_null`. This is ADR-0026's deferred step 5, folded into ADR-0027's diagnostics-severity model as the user requested.
This commit is contained in:
+230
-2
@@ -18,7 +18,9 @@ pub mod highlight;
|
|||||||
pub mod lex_helpers;
|
pub mod lex_helpers;
|
||||||
pub mod outcome;
|
pub mod outcome;
|
||||||
|
|
||||||
use crate::dsl::command::Command;
|
use crate::dsl::command::{
|
||||||
|
Command, CompareOp, Expr, Operand, Predicate, RowFilter,
|
||||||
|
};
|
||||||
use crate::dsl::grammar;
|
use crate::dsl::grammar;
|
||||||
use crate::dsl::walker::context::WalkContext;
|
use crate::dsl::walker::context::WalkContext;
|
||||||
use crate::dsl::walker::driver::{FailureKind, NodeWalkResult, walk_node};
|
use crate::dsl::walker::driver::{FailureKind, NodeWalkResult, walk_node};
|
||||||
@@ -421,6 +423,163 @@ fn schema_has_column(
|
|||||||
.is_some_and(|cols| cols.iter().any(|c| c.name.eq_ignore_ascii_case(column)))
|
.is_some_and(|cols| cols.iter().any(|c| c.name.eq_ignore_ascii_case(column)))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The WHERE expression of a filter command, if it has one.
|
||||||
|
const fn command_where_expr(command: &Command) -> Option<&Expr> {
|
||||||
|
match command {
|
||||||
|
Command::Update {
|
||||||
|
filter: RowFilter::Where(expr),
|
||||||
|
..
|
||||||
|
}
|
||||||
|
| Command::Delete {
|
||||||
|
filter: RowFilter::Where(expr),
|
||||||
|
..
|
||||||
|
}
|
||||||
|
| Command::ShowData {
|
||||||
|
filter: Some(expr), ..
|
||||||
|
} => Some(expr),
|
||||||
|
_ => None,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// 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.
|
||||||
|
fn expr_warnings(
|
||||||
|
expr: &Expr,
|
||||||
|
columns: &[crate::completion::TableColumn],
|
||||||
|
span: (usize, usize),
|
||||||
|
) -> Vec<outcome::Diagnostic> {
|
||||||
|
let mut out = Vec::new();
|
||||||
|
collect_expr_warnings(expr, columns, span, &mut out);
|
||||||
|
out
|
||||||
|
}
|
||||||
|
|
||||||
|
fn collect_expr_warnings(
|
||||||
|
expr: &Expr,
|
||||||
|
columns: &[crate::completion::TableColumn],
|
||||||
|
span: (usize, usize),
|
||||||
|
out: &mut Vec<outcome::Diagnostic>,
|
||||||
|
) {
|
||||||
|
match expr {
|
||||||
|
Expr::Or(terms) | Expr::And(terms) => {
|
||||||
|
for term in terms {
|
||||||
|
collect_expr_warnings(term, columns, span, out);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
Expr::Not(inner) => collect_expr_warnings(inner, columns, span, out),
|
||||||
|
Expr::Predicate(predicate) => {
|
||||||
|
predicate_warnings(predicate, columns, span, out);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
fn predicate_warnings(
|
||||||
|
predicate: &Predicate,
|
||||||
|
columns: &[crate::completion::TableColumn],
|
||||||
|
span: (usize, usize),
|
||||||
|
out: &mut Vec<outcome::Diagnostic>,
|
||||||
|
) {
|
||||||
|
use outcome::{Diagnostic, Severity};
|
||||||
|
let warn = |message: String| Diagnostic {
|
||||||
|
severity: Severity::Warning,
|
||||||
|
span,
|
||||||
|
message,
|
||||||
|
};
|
||||||
|
match predicate {
|
||||||
|
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) =
|
||||||
|
pair_type_mismatch(left, right, columns)
|
||||||
|
{
|
||||||
|
out.push(warn(message));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
Predicate::Between {
|
||||||
|
target, low, high, ..
|
||||||
|
} => {
|
||||||
|
for bound in [low, high] {
|
||||||
|
if let Some(message) =
|
||||||
|
pair_type_mismatch(target, bound, columns)
|
||||||
|
{
|
||||||
|
out.push(warn(message));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
Predicate::In { target, items, .. } => {
|
||||||
|
for item in items {
|
||||||
|
if let Some(message) =
|
||||||
|
pair_type_mismatch(target, item, columns)
|
||||||
|
{
|
||||||
|
out.push(warn(message));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
// `LIKE` is inherently a text-pattern test; flagging a
|
||||||
|
// non-text target is a future model extension.
|
||||||
|
// `IS [NOT] NULL` is the *correct* null test — never
|
||||||
|
// flagged.
|
||||||
|
Predicate::Like { .. } | Predicate::IsNull { .. } => {}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
const fn is_null_literal(operand: &Operand) -> bool {
|
||||||
|
matches!(operand, Operand::Literal(crate::dsl::value::Value::Null))
|
||||||
|
}
|
||||||
|
|
||||||
|
/// 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).
|
||||||
|
fn pair_type_mismatch(
|
||||||
|
a: &Operand,
|
||||||
|
b: &Operand,
|
||||||
|
columns: &[crate::completion::TableColumn],
|
||||||
|
) -> Option<String> {
|
||||||
|
let (column, literal) = match (a, b) {
|
||||||
|
(Operand::Column(c), Operand::Literal(v))
|
||||||
|
| (Operand::Literal(v), Operand::Column(c)) => (c, v),
|
||||||
|
_ => return None,
|
||||||
|
};
|
||||||
|
// `null` fits any column; `= NULL` is flagged separately.
|
||||||
|
if matches!(literal, crate::dsl::value::Value::Null) {
|
||||||
|
return None;
|
||||||
|
}
|
||||||
|
let ty = columns
|
||||||
|
.iter()
|
||||||
|
.find(|tc| tc.name.eq_ignore_ascii_case(column))?
|
||||||
|
.user_type;
|
||||||
|
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),
|
||||||
|
],
|
||||||
|
))
|
||||||
|
}
|
||||||
|
|
||||||
/// What the grammar would accept at the end of `source`
|
/// What the grammar would accept at the end of `source`
|
||||||
/// (ADR-0024 §architecture, Phase F walker-driven completion).
|
/// (ADR-0024 §architecture, Phase F walker-driven completion).
|
||||||
///
|
///
|
||||||
@@ -698,11 +857,25 @@ pub fn walk<'a>(
|
|||||||
// Schema-existence diagnostics (ADR-0027 §2) layer on top
|
// Schema-existence diagnostics (ADR-0027 §2) layer on top
|
||||||
// of a structurally-valid parse; a parse that already
|
// of a structurally-valid parse; a parse that already
|
||||||
// failed gets its ERROR verdict from `outcome`.
|
// failed gets its ERROR verdict from `outcome`.
|
||||||
let diagnostics = if matches!(final_outcome, WalkOutcome::Match { .. }) {
|
let mut diagnostics = if matches!(final_outcome, WalkOutcome::Match { .. }) {
|
||||||
schema_existence_diagnostics(&path, ctx.schema)
|
schema_existence_diagnostics(&path, ctx.schema)
|
||||||
} else {
|
} else {
|
||||||
Vec::new()
|
Vec::new()
|
||||||
};
|
};
|
||||||
|
// Expression WARNING diagnostics — type-mismatched
|
||||||
|
// comparisons and `= NULL` (ADR-0026 §7, surfaced through
|
||||||
|
// ADR-0027's model). Only a successfully-built command has
|
||||||
|
// a `where` expression to inspect.
|
||||||
|
if let Some(command) = &cmd
|
||||||
|
&& 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()),
|
||||||
|
));
|
||||||
|
}
|
||||||
let result = WalkResult {
|
let result = WalkResult {
|
||||||
outcome: final_outcome,
|
outcome: final_outcome,
|
||||||
matched_path: path,
|
matched_path: path,
|
||||||
@@ -1451,6 +1624,61 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn input_verdict_type_mismatch_is_warning() {
|
||||||
|
// `Age` is int; comparing it with a text literal runs,
|
||||||
|
// but is flagged (ADR-0026 §7).
|
||||||
|
let schema =
|
||||||
|
schema_with("Customers", &[("id", Type::Int), ("Age", Type::Int)]);
|
||||||
|
assert_eq!(
|
||||||
|
super::input_verdict(
|
||||||
|
"delete from Customers where Age = 'hello'",
|
||||||
|
Some(&schema),
|
||||||
|
),
|
||||||
|
Some(super::Severity::Warning),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn input_verdict_eq_null_is_warning() {
|
||||||
|
let schema =
|
||||||
|
schema_with("Customers", &[("id", Type::Int), ("Name", Type::Text)]);
|
||||||
|
assert_eq!(
|
||||||
|
super::input_verdict(
|
||||||
|
"delete from Customers where Name = null",
|
||||||
|
Some(&schema),
|
||||||
|
),
|
||||||
|
Some(super::Severity::Warning),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn input_verdict_compatible_comparison_is_clean() {
|
||||||
|
let schema =
|
||||||
|
schema_with("Customers", &[("id", Type::Int), ("Name", Type::Text)]);
|
||||||
|
assert_eq!(
|
||||||
|
super::input_verdict(
|
||||||
|
"delete from Customers where id = 5",
|
||||||
|
Some(&schema),
|
||||||
|
),
|
||||||
|
None,
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn input_verdict_error_outranks_warning() {
|
||||||
|
// An unknown column (ERROR) alongside `= NULL`
|
||||||
|
// (WARNING) — the indicator shows the higher severity.
|
||||||
|
let schema = schema_with("Customers", &[("id", Type::Int)]);
|
||||||
|
assert_eq!(
|
||||||
|
super::input_verdict(
|
||||||
|
"delete from Customers where NoSuchCol = null",
|
||||||
|
Some(&schema),
|
||||||
|
),
|
||||||
|
Some(super::Severity::Error),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn walker_parses_insert_with_explicit_column_list() {
|
fn walker_parses_insert_with_explicit_column_list() {
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
|
|||||||
@@ -38,6 +38,8 @@
|
|||||||
/// `(key, expected_placeholders)`. Sorted by key for grep-ability.
|
/// `(key, expected_placeholders)`. Sorted by key for grep-ability.
|
||||||
pub const KEYS_AND_PLACEHOLDERS: &[(&str, &[&str])] = &[
|
pub const KEYS_AND_PLACEHOLDERS: &[(&str, &[&str])] = &[
|
||||||
// ---- Pre-submit diagnostics (ADR-0027) ----
|
// ---- Pre-submit diagnostics (ADR-0027) ----
|
||||||
|
("diagnostic.eq_null", &[]),
|
||||||
|
("diagnostic.type_mismatch", &["column", "type"]),
|
||||||
("diagnostic.unknown_column", &["name", "table"]),
|
("diagnostic.unknown_column", &["name", "table"]),
|
||||||
("diagnostic.unknown_table", &["name"]),
|
("diagnostic.unknown_table", &["name"]),
|
||||||
// ---- Already-exists collisions (anchor: "already exists") ----
|
// ---- Already-exists collisions (anchor: "already exists") ----
|
||||||
|
|||||||
@@ -465,6 +465,10 @@ parse:
|
|||||||
diagnostic:
|
diagnostic:
|
||||||
unknown_table: "no such table: `{name}`"
|
unknown_table: "no such table: `{name}`"
|
||||||
unknown_column: "no such column `{name}` on table `{table}`"
|
unknown_column: "no such column `{name}` on table `{table}`"
|
||||||
|
# WARNING-severity flags (ADR-0026 §7) — the command still
|
||||||
|
# runs, but the comparison is very likely not intended.
|
||||||
|
type_mismatch: "`{column}` is {type} — this comparison uses a value of a different type"
|
||||||
|
eq_null: "`= NULL` is never true — use `IS NULL` or `IS NOT NULL`"
|
||||||
|
|
||||||
# ---- Project lifecycle event notes -----------------------------------
|
# ---- Project lifecycle event notes -----------------------------------
|
||||||
project:
|
project:
|
||||||
|
|||||||
Reference in New Issue
Block a user