diff --git a/bindings/nodejs/dts-header.d.ts b/bindings/nodejs/dts-header.d.ts index 648ff297..7028322a 100644 --- a/bindings/nodejs/dts-header.d.ts +++ b/bindings/nodejs/dts-header.d.ts @@ -69,9 +69,11 @@ export type PolicyDiagnosticCode = | 'UNRESOLVED_FUNCTION_TYPE' | 'IMPLICIT_ANY' | 'UNCHECKED_NODE' + | 'NULLABILITY_DIVERGENCE' | 'REDUNDANT_NULLISH' | 'REPEATED_DERIVATION' | 'PREFER_MATCH' + | 'PREFER_DICTIONARY' | 'REDUNDANT_TABLE_ROW' | 'NON_DISCRIMINATING_COLUMN' | 'REDUNDANT_PARENTHESES'; diff --git a/bindings/nodejs/index.d.ts b/bindings/nodejs/index.d.ts index b2b23b15..83b8022c 100644 --- a/bindings/nodejs/index.d.ts +++ b/bindings/nodejs/index.d.ts @@ -69,9 +69,11 @@ export type PolicyDiagnosticCode = | 'UNRESOLVED_FUNCTION_TYPE' | 'IMPLICIT_ANY' | 'UNCHECKED_NODE' + | 'NULLABILITY_DIVERGENCE' | 'REDUNDANT_NULLISH' | 'REPEATED_DERIVATION' | 'PREFER_MATCH' + | 'PREFER_DICTIONARY' | 'REDUNDANT_TABLE_ROW' | 'NON_DISCRIMINATING_COLUMN' | 'REDUNDANT_PARENTHESES'; diff --git a/core/engine/src/policy/blocks/context.rs b/core/engine/src/policy/blocks/context.rs index b87501bf..7a7d56b1 100644 --- a/core/engine/src/policy/blocks/context.rs +++ b/core/engine/src/policy/blocks/context.rs @@ -233,6 +233,22 @@ impl AnalysisContext { .push(Diagnostic::error(code, location, message)); } + pub fn hint_with_target( + &mut self, + code: DiagnosticCode, + expression_id: Option>, + span: Option<(u32, u32)>, + target: Option, + message: impl Into, + ) { + let mut location = self.location_with(expression_id, span); + if let Some(t) = target { + location = location.with_target(t); + } + self.diagnostics + .push(Diagnostic::hint(code, location, message)); + } + fn location_with( &self, expression_id: Option>, diff --git a/core/engine/src/policy/blocks/decision_table.rs b/core/engine/src/policy/blocks/decision_table.rs index f0fe7b13..3909f39a 100644 --- a/core/engine/src/policy/blocks/decision_table.rs +++ b/core/engine/src/policy/blocks/decision_table.rs @@ -510,6 +510,45 @@ impl DecisionTableIr { } } + if cx.is_enriched() { + for col in &self.inputs { + let Some(field) = col.field.as_ref().filter(|f| !f.is_empty()) else { + continue; + }; + let Some(field_type) = input_field_types.get(&col.id) else { + continue; + }; + if !matches!(field_type.unwrap_nullable().0, VariableType::String) { + continue; + } + let mut tests: Vec = Vec::new(); + for rule in &self.rules { + let Some(cell) = rule.get(&col.id).filter(|c| !c.is_empty()) else { + continue; + }; + if cell.trim() == "_" { + continue; + } + tests.push(cx.cell_test(cell)); + } + if let Some(values) = DictionaryCandidate::from_literal_tests(&tests) { + cx.hint_with_target( + DiagnosticCode::PreferDictionary, + Some(col.id.clone()), + None, + Some(CursorTarget::DecisionTableHead { + col: col.id.clone(), + }), + format!( + "conditions on '{}' only test the fixed strings {} — define a dictionary and type the field with it for membership checking and labeled editing", + field, + DictionaryCandidate::format_values(&values) + ), + ); + } + } + } + for col in &self.outputs { if col.field.is_empty() { continue; @@ -548,6 +587,25 @@ impl DecisionTableIr { } } + if cx.is_enriched() { + if let Some(values) = DictionaryCandidate::from_const_cells(&cell_types) { + cx.hint_with_target( + DiagnosticCode::PreferDictionary, + Some(col.id.clone()), + None, + Some(CursorTarget::DecisionTableHead { + col: col.id.clone(), + }), + format!( + "output column '{}' only produces the fixed strings {} — define a dictionary with these values and type the column with it ('out {}: ') for membership checking and labeled editing", + col.field, + DictionaryCandidate::format_values(&values), + col.field + ), + ); + } + } + let covered = col.collect || !cx.is_enriched() || self.column_covered(col, cx, &input_field_types); @@ -1263,3 +1321,76 @@ impl DecisionTableIr { } } } + +pub(crate) struct DictionaryCandidate; + +impl DictionaryCandidate { + const MAX_SHOWN: usize = 6; + + pub(crate) fn from_const_cells(cell_types: &[VariableType]) -> Option>> { + if cell_types.len() < 2 { + return None; + } + let mut values: Vec> = Vec::new(); + for cell in cell_types { + let VariableType::Const(value) = cell else { + return None; + }; + if !values.iter().any(|seen| seen == value) { + values.push(value.clone()); + } + } + (values.len() >= 2 && !Self::all_date_like(&values)).then_some(values) + } + + pub(crate) fn from_literal_tests(tests: &[ArmTest]) -> Option>> { + let mut values: Vec> = Vec::new(); + let mut literal_cells = 0usize; + for test in tests { + match test { + ArmTest::Enum { + values: cell_values, + .. + } => { + literal_cells += 1; + for value in cell_values { + if !values.iter().any(|seen| seen == value) { + values.push(value.clone()); + } + } + } + ArmTest::Default => {} + _ => return None, + } + } + (literal_cells >= 2 && values.len() >= 2 && !Self::all_date_like(&values)).then_some(values) + } + + /// Literal-date value sets are calendars, not enums. + fn all_date_like(values: &[std::rc::Rc]) -> bool { + let is_date_like = |value: &str| { + let bytes = value.as_bytes(); + bytes.len() >= 10 + && bytes[..4].iter().all(u8::is_ascii_digit) + && bytes[4] == b'-' + && bytes[5..7].iter().all(u8::is_ascii_digit) + && bytes[7] == b'-' + && bytes[8..10].iter().all(u8::is_ascii_digit) + }; + values.iter().all(|value| is_date_like(value)) + } + + pub(crate) fn format_values(values: &[std::rc::Rc]) -> String { + let shown = values + .iter() + .take(Self::MAX_SHOWN) + .map(|value| format!("\"{value}\"")) + .collect::>() + .join(" | "); + if values.len() > Self::MAX_SHOWN { + format!("{shown} | …") + } else { + shown + } + } +} diff --git a/core/engine/src/policy/blocks/mod.rs b/core/engine/src/policy/blocks/mod.rs index 4fc865de..e7552727 100644 --- a/core/engine/src/policy/blocks/mod.rs +++ b/core/engine/src/policy/blocks/mod.rs @@ -31,8 +31,8 @@ pub use context::{ AnalysisContext, AnalysisSummary, ExecutionContext, ExecutionError, ExpressionLocation, InstanceSource, PropertyRead, SharedDictionaryTypes, SharedIntelliSense, WriteTarget, }; -pub(crate) use decision_table::TableSelection; pub use decision_table::{DecisionTableDoc, DecisionTableIr, DeclaredType}; +pub(crate) use decision_table::{DictionaryCandidate, TableSelection}; pub use expression::{ExpressionDoc, ExpressionIr}; pub(crate) use match_block::MatchSelection; pub use match_block::{MatchDoc, MatchIr}; diff --git a/core/engine/src/workspace/graph/analysis.rs b/core/engine/src/workspace/graph/analysis.rs index 5b10c005..83365d64 100644 --- a/core/engine/src/workspace/graph/analysis.rs +++ b/core/engine/src/workspace/graph/analysis.rs @@ -13,13 +13,15 @@ use zen_types::decision::{ use zen_expression::intellisense::ArmTest; use crate::model::GraphContent; -use crate::policy::blocks::{DecisionTableIr, DeclaredType, IntelliSenseSource, ReadFlattener}; +use crate::policy::blocks::{ + DecisionTableIr, DeclaredType, DictionaryCandidate, IntelliSenseSource, ReadFlattener, +}; use crate::policy::linter::{AstOps, RedundantParentheses}; use crate::policy::queries::scope::VariableTypeScope; use crate::workspace::db::Db; use crate::workspace::graph::function::FunctionTypeOutcome; use crate::workspace::types::{ - CursorTarget, Diagnostic, DiagnosticCode, DiagnosticLocation, ExpressionKind, + CursorTarget, Diagnostic, DiagnosticCode, DiagnosticLocation, ExpressionKind, Severity, }; const NODES_KEY: &str = "$nodes"; @@ -132,6 +134,7 @@ impl<'a> GraphAnalyzer<'a> { let output = Self::terminal_output(self.content, &topology, &nodes); let inferred_inputs = self.inferred_inputs(&topology, &nodes, &graph_input); + self.lint_output_any(&topology, &nodes, &graph_input); self.lint_unreachable(&topology); self.lint_expressions(); self.sort_diagnostics(&topology); @@ -413,6 +416,19 @@ impl<'a> GraphAnalyzer<'a> { } } + fn check_schema_enum_candidates(&mut self, node: &DecisionNode, schema: &serde_json::Value) { + let paths = super::SchemaType::inline_enum_paths(schema); + for path in paths.iter().take(8) { + self.diagnostics.push(Diagnostic::hint( + DiagnosticCode::PreferDictionary, + DiagnosticLocation::block(self.path.clone(), node.id.clone()), + format!( + "schema property `{path}` declares an inline enum — reference a dictionary instead ({{\"$dictionary\": \"\"}}) so the value set is defined once, labeled, and membership-checked" + ), + )); + } + } + fn analyze_node( &mut self, node: &'a DecisionNode, @@ -444,6 +460,7 @@ impl<'a> GraphAnalyzer<'a> { DecisionNodeKind::InputNode { content } => { if let Some(schema) = content.schema.as_ref() { self.check_schema_dictionaries(node, schema); + self.check_schema_enum_candidates(node, schema); } analysis.output = graph_input.shallow_clone(); if matches!(graph_input, VariableType::Any) { @@ -466,11 +483,24 @@ impl<'a> GraphAnalyzer<'a> { ), )); } + if let Some(schema) = content.schema.as_ref() { + let divergent = super::SchemaType::nullability_divergences(schema); + for path in divergent.iter().take(8) { + self.diagnostics.push(Diagnostic::warning( + DiagnosticCode::NullabilityDivergence, + DiagnosticLocation::block(self.path.clone(), node.id.clone()), + format!( + "optional property `{path}` reads as nullable, but its schema does not allow null — a payload carrying `{path}: null` fails validation at runtime; add \"null\" to its type if null is a real value, or ignore this if the field is strictly absent-or-present" + ), + )); + } + } } } DecisionNodeKind::OutputNode { content } => { if let Some(schema) = content.schema.as_ref() { self.check_schema_dictionaries(node, schema); + self.check_schema_enum_candidates(node, schema); } if let Some(schema) = content.schema.as_ref().filter(|_| self.validate) { let expected = @@ -692,7 +722,7 @@ impl<'a> GraphAnalyzer<'a> { } let mut output = handler(self, &element); if attributes.pass_through { - output = element.merge(&output); + output = Self::merge_patch_type(&element, &output); } (element, output.array()) } @@ -704,22 +734,35 @@ impl<'a> GraphAnalyzer<'a> { output = wrapped; } if attributes.pass_through { - output = match &output { - VariableType::Array(_) => output, - VariableType::Object(_) => scope_input.merge(&output), - VariableType::Nullable(inner) - if matches!(inner.as_ref(), VariableType::Object(_)) => - { - scope_input.merge(inner) - } - VariableType::Any => VariableType::Any, - _ => scope_input.shallow_clone(), - }; + output = Self::merge_patch_type(scope_input, &output); } (handler_scope, output) } + /// Type-level mirror of the runtime pass-through merge (`Variable::merge_clone`). + fn merge_patch_type(base: &VariableType, patch: &VariableType) -> VariableType { + match patch { + VariableType::Any => VariableType::Any, + VariableType::Array(_) => patch.shallow_clone(), + VariableType::Object(_) => base.merge(patch), + VariableType::Nullable(inner) => match inner.as_ref() { + VariableType::Object(fields) => { + let optional = VariableType::empty_object(); + if let VariableType::Object(target) = &optional { + let mut map = target.borrow_mut(); + for (key, value) in fields.borrow().iter() { + map.insert(key.clone(), super::wrap_optional(value.shallow_clone())); + } + } + base.merge(&optional) + } + _ => base.shallow_clone(), + }, + _ => base.shallow_clone(), + } + } + fn check_expression_rows( &mut self, node: &DecisionNode, @@ -829,6 +872,51 @@ impl<'a> GraphAnalyzer<'a> { } } + for col in content.inputs.iter() { + let Some(field) = &col.field else { + continue; + }; + let Some(field_type) = input_field_types.get(&col.id) else { + continue; + }; + if !matches!(field_type.unwrap_nullable().0, VariableType::String) { + continue; + } + let intellisense = self.db.graph_intellisense(); + let mut tests: Vec = Vec::new(); + for rule in content.rules.iter() { + let Some(cell) = rule.get(&col.id).filter(|c| !c.is_empty()) else { + continue; + }; + if cell.trim() == "_" { + continue; + } + tests.push(IntelliSenseSource::cell_test( + &mut intellisense.borrow_mut(), + cell, + )); + } + if let Some(values) = DictionaryCandidate::from_literal_tests(&tests) { + self.diagnostics.push(Diagnostic::hint( + DiagnosticCode::PreferDictionary, + DiagnosticLocation::expression( + self.path.clone(), + node.id.clone(), + col.id.clone(), + None, + ) + .with_target(CursorTarget::DecisionTableHead { + col: col.id.clone(), + }), + format!( + "conditions on '{}' only test the fixed strings {} — define a dictionary in an imported policy and type the field with it for membership checking and labeled editing", + field, + DictionaryCandidate::format_values(&values) + ), + )); + } + } + let output = VariableType::empty_object(); for col in content.outputs.iter() { if col.field.is_empty() { @@ -873,6 +961,28 @@ impl<'a> GraphAnalyzer<'a> { None => cell_types.push(resolved), } } + if declared.is_none() { + if let Some(values) = DictionaryCandidate::from_const_cells(&cell_types) { + self.diagnostics.push(Diagnostic::hint( + DiagnosticCode::PreferDictionary, + DiagnosticLocation::expression( + self.path.clone(), + node.id.clone(), + col.id.clone(), + None, + ) + .with_target(CursorTarget::DecisionTableHead { + col: col.id.clone(), + }), + format!( + "output column '{}' only produces the fixed strings {} — define a dictionary with these values in an imported policy and type the column with it ('out {}: ') for membership checking and labeled editing", + col.field, + DictionaryCandidate::format_values(&values), + col.field + ), + )); + } + } let has_empty_cell = content .rules .iter() @@ -1109,6 +1219,69 @@ impl<'a> GraphAnalyzer<'a> { } } + fn lint_output_any( + &mut self, + topology: &GraphTopology, + nodes: &HashMap, GraphNodeAnalysis>, + graph_input: &VariableType, + ) { + if matches!(graph_input, VariableType::Any) { + return; + } + if self + .diagnostics + .iter() + .any(|d| d.severity == Severity::Error) + { + return; + } + let Some(reachable) = Self::reachable_from_inputs(self.content, topology) else { + return; + }; + let Some(order) = &topology.order else { + return; + }; + let mut input_any = Vec::new(); + Self::collect_any_paths(graph_input, String::new(), &mut input_any); + let mut seen: HashSet = HashSet::default(); + for &idx in order { + let node = &self.content.nodes[idx]; + if !reachable[idx] || matches!(node.kind, DecisionNodeKind::InputNode { .. }) { + continue; + } + let Some(analysis) = nodes.get(&node.id) else { + continue; + }; + if analysis.unchecked || analysis.opaque || analysis.open { + continue; + } + if matches!(analysis.output, VariableType::Any) { + self.diagnostics.push(Diagnostic::error( + DiagnosticCode::ImplicitAny, + DiagnosticLocation::block(self.path.clone(), node.id.clone()), + format!( + "output of node '{}' resolves to `any` — the graph's result type becomes unknown; type the producing expression or give the called sub-decision an input schema", + node.name + ), + )); + continue; + } + let mut any_paths = Vec::new(); + Self::collect_any_paths(&analysis.output, String::new(), &mut any_paths); + any_paths.retain(|path| !input_any.contains(path) && !seen.contains(path)); + for path in any_paths.iter().take(8) { + self.diagnostics.push(Diagnostic::error( + DiagnosticCode::ImplicitAny, + DiagnosticLocation::block(self.path.clone(), node.id.clone()), + format!( + "output `{path}` resolves to `any` — everything reading it degrades to `any`; give it a concrete type where it is produced" + ), + )); + } + seen.extend(any_paths); + } + } + fn lint_unreachable(&mut self, topology: &GraphTopology) { let input_indices: Vec = self .content @@ -1376,23 +1549,79 @@ impl<'a> GraphAnalyzer<'a> { DiagnosticCode::TypeMismatch, DiagnosticLocation::block(self.path.clone(), node.id.clone()), format!( - "decision '{}' requires input '{path}' of type `{expected_type}`, but it is not provided", - content.key + "decision '{}' requires input '{path}' of type `{}`, but it is not provided", + content.key, + Self::type_sketch(&expected_type, 0) ), )); } for (path, actual_type, expected_type) in mismatched { + let nullability_only = actual_type.is_nullable() && !expected_type.is_nullable() && { + let (actual_inner, _) = actual_type.unwrap_nullable(); + actual_inner.satisfies(&expected_type) + }; + let message = if nullability_only { + format!( + "input '{path}' for decision '{}' may be null (`{actual_type}`), but a non-null `{expected_type}` is required", + content.key + ) + } else { + format!( + "input '{path}' for decision '{}' has type `{}`, but `{}` is expected", + content.key, + Self::type_sketch(&actual_type, 0), + Self::type_sketch(&expected_type, 0) + ) + }; self.diagnostics.push(Diagnostic::error( DiagnosticCode::TypeMismatch, DiagnosticLocation::block(self.path.clone(), node.id.clone()), - format!( - "input '{path}' for decision '{}' has type `{actual_type}`, but `{expected_type}` is expected", - content.key - ), + message, )); } } + /// Unlike `Display`, expands object fields so two different types never print identically. + fn type_sketch(variable_type: &VariableType, depth: usize) -> String { + const MAX_DEPTH: usize = 3; + const MAX_FIELDS: usize = 8; + match variable_type { + VariableType::Nullable(inner) => format!("{}?", Self::type_sketch(inner, depth)), + VariableType::Array(items) => { + let inner = Self::type_sketch(items, depth); + if inner.ends_with('?') { + format!("({inner})[]") + } else { + format!("{inner}[]") + } + } + VariableType::Object(fields) => { + let map = fields.borrow(); + if map.is_empty() { + return "{}".to_string(); + } + if depth >= MAX_DEPTH { + return "object".to_string(); + } + let mut keys: Vec<_> = map.keys().cloned().collect(); + keys.sort(); + let mut parts: Vec = keys + .iter() + .take(MAX_FIELDS) + .filter_map(|key| { + map.get(key.as_ref()) + .map(|field| format!("{key}: {}", Self::type_sketch(field, depth + 1))) + }) + .collect(); + if keys.len() > MAX_FIELDS { + parts.push(format!("…+{} more", keys.len() - MAX_FIELDS)); + } + format!("{{ {} }}", parts.join(", ")) + } + other => other.to_string(), + } + } + fn diff_required( prefix: String, expected: &HashMap, VariableType>, @@ -1419,16 +1648,60 @@ impl<'a> GraphAnalyzer<'a> { } } Some(actual_type) => { - let (actual_inner, _) = actual_type.unwrap_nullable(); + let (actual_inner, actual_nullable) = actual_type.unwrap_nullable(); if matches!(actual_inner, VariableType::Any) { continue; } + if actual_nullable && !optional { + mismatched.push(( + path, + actual_type.shallow_clone(), + expected_type.shallow_clone(), + )); + continue; + } if let (VariableType::Object(e), VariableType::Object(a)) = (expected_inner, actual_inner) { Self::diff_required(path, &e.borrow(), &a.borrow(), missing, mismatched); continue; } + if let (VariableType::Array(e_item), VariableType::Array(a_item)) = + (expected_inner, actual_inner) + { + let (e_it, item_optional) = e_item.unwrap_nullable(); + let (a_it, item_nullable) = a_item.unwrap_nullable(); + let item_path = format!("{path}[]"); + if matches!(a_it, VariableType::Any) { + continue; + } + if item_nullable && !item_optional { + mismatched.push(( + item_path, + a_item.shallow_clone(), + e_item.shallow_clone(), + )); + continue; + } + if let (VariableType::Object(e), VariableType::Object(a)) = (e_it, a_it) { + Self::diff_required( + item_path, + &e.borrow(), + &a.borrow(), + missing, + mismatched, + ); + continue; + } + if !a_it.satisfies(e_it) { + mismatched.push(( + item_path, + a_it.shallow_clone(), + e_it.shallow_clone(), + )); + } + continue; + } if !actual_type.satisfies(expected_type) { mismatched.push(( path, @@ -1454,6 +1727,14 @@ impl<'a> GraphAnalyzer<'a> { let analysis = IntelliSenseSource::analyze(&mut intellisense.borrow_mut(), source, kind, scope); for diagnostic in &analysis.diagnostics { + if !self.validate + && matches!( + diagnostic.source, + zen_expression::intellisense::diagnostic::DiagnosticSource::TypeCheck + ) + { + continue; + } let location = DiagnosticLocation { policy_path: self.path.clone(), block_id: Some(node_id.clone()), diff --git a/core/engine/src/workspace/graph/schema.rs b/core/engine/src/workspace/graph/schema.rs index d2141c41..ae7b1441 100644 --- a/core/engine/src/workspace/graph/schema.rs +++ b/core/engine/src/workspace/graph/schema.rs @@ -61,6 +61,112 @@ impl SchemaType { } } + /// Optional properties resolve statically to `T?`, yet the runtime validator + /// rejects an explicit `null` unless the type admits it. + pub(crate) fn nullability_divergences(schema: &Value) -> Vec { + let mut out = Vec::new(); + Self::collect_divergences(schema, String::new(), &mut out); + out + } + + fn collect_divergences(schema: &Value, path: String, out: &mut Vec) { + let Some(object) = schema.as_object() else { + return; + }; + if let Some(items) = object.get("items") { + let item_path = if path.is_empty() { + "[]".to_string() + } else { + format!("{path}[]") + }; + Self::collect_divergences(items, item_path, out); + } + let Some(properties) = object.get("properties").and_then(Value::as_object) else { + return; + }; + let required: Vec<&str> = object + .get("required") + .and_then(Value::as_array) + .map(|list| list.iter().filter_map(Value::as_str).collect()) + .unwrap_or_default(); + for (name, prop_schema) in properties { + let child_path = if path.is_empty() { + name.clone() + } else { + format!("{path}.{name}") + }; + if !required.contains(&name.as_str()) && !Self::admits_null(prop_schema) { + out.push(child_path.clone()); + } + Self::collect_divergences(prop_schema, child_path, out); + } + } + + fn admits_null(schema: &Value) -> bool { + let Some(object) = schema.as_object() else { + return true; + }; + if object.get("$dictionary").is_some() { + return false; + } + if let Some(cases) = object + .get("anyOf") + .or_else(|| object.get("oneOf")) + .and_then(Value::as_array) + { + return cases.iter().any(Self::admits_null); + } + if let Some(values) = object.get("enum").and_then(Value::as_array) { + return values.iter().any(Value::is_null); + } + match object.get("type") { + Some(Value::String(kind)) => kind == "null", + Some(Value::Array(kinds)) => kinds + .iter() + .filter_map(Value::as_str) + .any(|kind| kind == "null"), + _ => true, + } + } + + pub(crate) fn inline_enum_paths(schema: &Value) -> Vec { + let mut out = Vec::new(); + Self::collect_inline_enums(schema, String::new(), &mut out); + out + } + + fn collect_inline_enums(schema: &Value, path: String, out: &mut Vec) { + let Some(object) = schema.as_object() else { + return; + }; + if object.get("$dictionary").is_none() && !path.is_empty() { + if let Some(values) = object.get("enum").and_then(Value::as_array) { + let strings = values.iter().filter(|value| value.is_string()).count(); + if strings == values.len() && values.len() >= 2 { + out.push(path.clone()); + } + } + } + if let Some(items) = object.get("items") { + let item_path = if path.is_empty() { + "[]".to_string() + } else { + format!("{path}[]") + }; + Self::collect_inline_enums(items, item_path, out); + } + if let Some(properties) = object.get("properties").and_then(Value::as_object) { + for (name, prop_schema) in properties { + let child_path = if path.is_empty() { + name.clone() + } else { + format!("{path}.{name}") + }; + Self::collect_inline_enums(prop_schema, child_path, out); + } + } + } + pub(crate) fn dictionary_names(schema: &Value, out: &mut Vec>) { match schema { Value::Object(map) => { diff --git a/core/engine/src/workspace/types/diagnostic.rs b/core/engine/src/workspace/types/diagnostic.rs index f80dac42..631a6506 100644 --- a/core/engine/src/workspace/types/diagnostic.rs +++ b/core/engine/src/workspace/types/diagnostic.rs @@ -173,10 +173,12 @@ pub enum DiagnosticCode { UnresolvedFunctionType, ImplicitAny, UncheckedNode, + NullabilityDivergence, RedundantNullish, RepeatedDerivation, PreferMatch, + PreferDictionary, RedundantTableRow, NonDiscriminatingColumn, RedundantParentheses, diff --git a/core/engine/tests/data/policy/diagnostics.toml b/core/engine/tests/data/policy/diagnostics.toml index 1b5945d7..a53b159b 100644 --- a/core/engine/tests/data/policy/diagnostics.toml +++ b/core/engine/tests/data/policy/diagnostics.toml @@ -403,8 +403,8 @@ content = ''' } ''' no_errors = true -hint_codes = ["RedundantTableRow", "NonDiscriminatingColumn"] -hint_count = 2 +hint_codes = ["RedundantTableRow", "NonDiscriminatingColumn", "PreferDictionary"] +hint_count = 3 # Multi-policy @@ -500,7 +500,8 @@ content = ''' } ''' no_errors = true -hint_count = 0 +hint_codes = ["PreferDictionary"] +hint_count = 1 [[test]] name = "values yields union of field types" @@ -1298,3 +1299,35 @@ content = ''' no_errors = true hint_codes = ["RedundantParentheses"] hint_count = 1 + +[[test]] +name = "static string output column hints a dictionary candidate" +content = ''' +{ + "blocks": [ + { + "id": "schema", + "type": "dataModel", + "props": { "data": { "name": "inputs", "scope": "global", "properties": [ + { "id": "prop1", "name": "amount", "type": "number", "array": false, "optional": false } + ] } } + }, + { + "id": "table1", + "type": "decisionTable", + "props": { "data": { + "hitPolicy": "first", + "inputs": [ { "id": "col1", "name": "Amount", "field": "amount" } ], + "outputs": [ { "id": "out1", "name": "Method", "field": "method" } ], + "rules": [ + { "_id": "row1", "col1": "> 100", "out1": "\"wire\"" }, + { "_id": "row2", "col1": "", "out1": "\"card\"" } + ] + } } + } + ] +} +''' +no_errors = true +hint_codes = ["PreferDictionary"] +hint_count = 1 diff --git a/core/engine/tests/policy.rs b/core/engine/tests/policy.rs index 874f88bc..d22203fb 100644 --- a/core/engine/tests/policy.rs +++ b/core/engine/tests/policy.rs @@ -3222,7 +3222,11 @@ fn unrelated_policies_do_not_leak_computed_global_types() { ws.set_policy("tier-writer", serde_json::from_value(writer).unwrap()); ws.set_policy("tier-reader", serde_json::from_value(reader).unwrap()); - let diags = ws.diagnostics("tier-reader"); + let diags: Vec<_> = ws + .diagnostics("tier-reader") + .into_iter() + .filter(|d| d.severity != Severity::Hint) + .collect(); assert!( diags.is_empty(), "tier-reader declares its own global tier:string and never imports tier-writer; \ diff --git a/core/engine/tests/workspace_graph.rs b/core/engine/tests/workspace_graph.rs index ed35d0af..77b204ec 100644 --- a/core/engine/tests/workspace_graph.rs +++ b/core/engine/tests/workspace_graph.rs @@ -326,7 +326,10 @@ fn decision_table_cells_are_checked() { document(linear_graph(Some(person_schema()), vec![table])), ); let diagnostics = ws.diagnostics("g"); - assert!(diagnostics.is_empty(), "{diagnostics:?}"); + assert!( + diagnostics.iter().all(|d| d.severity == Severity::Hint), + "{diagnostics:?}" + ); let outputs = ws.outputs(&ScopeRequest::for_policy("g")); let result = outputs.iter().find(|o| o.path.as_ref() == "result"); @@ -1665,7 +1668,10 @@ fn switch_first_hit_narrows_branches_and_default() { let mut ws = Workspace::new(); ws.set_document("g", document(switch_graph("first"))); let diagnostics = ws.diagnostics("g"); - assert!(diagnostics.is_empty(), "{diagnostics:?}"); + assert!( + diagnostics.iter().all(|d| d.severity == Severity::Hint), + "{diagnostics:?}" + ); assert!( matches!(node_input_kind(&ws, "g", "nShip"), VariableType::Const(ref c) if c.as_ref() == "shipping") @@ -2660,3 +2666,604 @@ fn graph_signature_excludes_unreachable_sinks() { "{outputs:?}" ); } + +fn rates_loop_graph(hit_policy: &str, rules: Value, pass_through: bool) -> Value { + let schema = json!({ + "type": "object", + "properties": { + "rates": { + "type": "array", + "items": { + "type": "object", + "properties": { "region": { "type": "string" }, "amount": { "type": "number" } }, + "required": ["region", "amount"] + } + } + }, + "required": ["rates"] + }); + let table = node( + "dt", + "decisionTableNode", + json!({ + "hitPolicy": hit_policy, + "passThrough": pass_through, + "inputField": "rates", + "executionMode": "loop", + "outputPath": "results", + "inputs": [{ "id": "c1", "name": "Amount", "field": "amount" }], + "outputs": [{ "id": "o1", "name": "Rate", "field": "rate" }], + "rules": rules + }), + ); + let reader = node( + "read", + "expressionNode", + json!({ + "passThrough": true, + "expressions": [ + { "id": "e1", "key": "phantom", "value": "map(results, #.inclusive ?? false)" }, + { "id": "e2", "key": "real", "value": "map(results, #.rate ?? 0)" }, + { "id": "e3", "key": "carried", "value": "map(results, #.amount)" } + ] + }), + ); + json!({ + "nodes": [ + node("in", "inputNode", json!({ "schema": schema.to_string() })), + table, + reader, + node("out", "outputNode", json!({})), + ], + "edges": [ + edge("g1", "in", "dt"), + edge("g2", "dt", "read"), + edge("g3", "read", "out"), + ] + }) +} + +#[test] +fn loop_table_output_columns_propagate_into_element_type() { + let mut ws = Workspace::new(); + ws.set_document( + "g", + document(rates_loop_graph( + "first", + json!([ + { "_id": "r1", "c1": "> 100", "o1": "0.12" }, + { "_id": "r2", "c1": "", "o1": "0.02" } + ]), + true, + )), + ); + let diagnostics = ws.diagnostics("g"); + let phantom: Vec<_> = diagnostics + .iter() + .filter(|d| d.code == DiagnosticCode::UndefinedVariable) + .collect(); + assert_eq!(phantom.len(), 1, "{diagnostics:?}"); + assert!( + phantom[0].message.contains("inclusive"), + "the never-produced field must be the one flagged: {diagnostics:?}" + ); +} + +#[test] +fn loop_collect_table_elements_are_row_arrays() { + let mut ws = Workspace::new(); + ws.set_document( + "g", + document(rates_loop_graph( + "collect", + json!([{ "_id": "r1", "c1": "> 100", "o1": "0.12" }]), + true, + )), + ); + let analysis = ws.graph_analysis("g").expect("analysis"); + let dt = analysis.nodes.get("dt").expect("dt node"); + let results = dt.output.get("results"); + let element = match &results { + VariableType::Array(inner) => inner.as_ref().shallow_clone(), + other => panic!("results must be an array, got {other:?}"), + }; + assert!( + matches!(element, VariableType::Array(_)), + "collect in loop must produce row arrays per element, got {element:?}" + ); + let member_errors = analysis + .diagnostics + .iter() + .filter(|d| d.location.block_id.as_deref() == Some("read") && d.severity == Severity::Error) + .count(); + assert!( + member_errors >= 3, + "member reads on row arrays must be flagged: {:?}", + analysis.diagnostics + ); +} + +#[test] +fn pass_through_nullable_patch_merges_fields_as_optional() { + let mut ws = Workspace::new(); + let child_table = simple_table(json!([ + { "_id": "r1", "c1": "> 18", "o1": "\"adult\"" } + ])); + let mut child = linear_graph(Some(person_schema()), vec![child_table]); + child["nodes"].as_array_mut().unwrap()[1]["content"]["passThrough"] = json!(false); + ws.set_document("child", document(child)); + + let decision = node( + "call", + "decisionNode", + json!({ "key": "child", "passThrough": true }), + ); + ws.set_document( + "parent", + document(linear_graph(Some(person_schema()), vec![decision])), + ); + + let outputs = ws.outputs(&ScopeRequest::for_policy("parent")); + let result = outputs + .iter() + .find(|o| o.path.as_ref() == "result") + .expect("result output"); + assert!( + matches!(result.resolved_type, VariableType::Nullable(_)), + "a nullable pass-through patch must merge its fields as optional, got {:?}", + result.resolved_type + ); +} + +fn line_items_child(required_item_fields: Value) -> Value { + let schema = json!({ + "type": "object", + "properties": { + "lineItems": { + "type": "array", + "items": { + "type": "object", + "properties": { + "amount": { "type": "number" }, + "inclusive": { "type": "boolean" } + }, + "required": required_item_fields + } + } + }, + "required": ["lineItems"] + }); + linear_graph( + Some(schema), + vec![expression_node( + "calc", + &[("total", "sum(map(lineItems, #.amount))")], + )], + ) +} + +fn line_items_parent() -> Value { + let schema = json!({ + "type": "object", + "properties": { + "source": { + "type": "array", + "items": { + "type": "object", + "properties": { "amount": { "type": "number" } }, + "required": ["amount"] + } + } + }, + "required": ["source"] + }); + let mut mk = expression_node("mk", &[("lineItems", "map(source, { amount: #.amount })")]); + mk["content"]["passThrough"] = json!(true); + let decision = node("call", "decisionNode", json!({ "key": "child" })); + linear_graph(Some(schema), vec![mk, decision]) +} + +#[test] +fn decision_boundary_reports_item_level_diff() { + let mut ws = Workspace::new(); + ws.set_document( + "child", + document(line_items_child(json!(["amount", "inclusive"]))), + ); + ws.set_document("parent", document(line_items_parent())); + let diagnostics = ws.diagnostics("parent"); + let errors: Vec<&str> = diagnostics + .iter() + .filter(|d| d.severity == Severity::Error) + .map(|d| d.message.as_str()) + .collect(); + assert_eq!(errors.len(), 1, "{errors:?}"); + assert!( + errors[0].contains("lineItems[].inclusive") && errors[0].contains("`bool`"), + "the diff must name the item field, not flatten to object[]: {errors:?}" + ); +} + +#[test] +fn decision_boundary_allows_missing_optional_item_field() { + let mut ws = Workspace::new(); + ws.set_document("child", document(line_items_child(json!(["amount"])))); + ws.set_document("parent", document(line_items_parent())); + let diagnostics = ws.diagnostics("parent"); + assert!( + diagnostics.is_empty(), + "an optional item field the parent never produces must not break the boundary: {diagnostics:?}" + ); +} + +#[test] +fn optional_property_without_null_type_warns_of_divergence() { + let mut ws = Workspace::new(); + let schema = json!({ + "type": "object", + "properties": { + "age": { "type": "number" }, + "name": { "type": "string" }, + "alias": { "type": ["string", "null"] }, + "tags": { + "type": "array", + "items": { + "type": "object", + "properties": { "label": { "type": "string" }, "weight": { "type": "number" } }, + "required": ["label"] + } + } + }, + "required": ["age", "tags"] + }); + ws.set_document( + "g", + document(linear_graph( + Some(schema), + vec![expression_node("calc", &[("x", "age * 2")])], + )), + ); + let diagnostics = ws.diagnostics("g"); + let divergent: Vec<&str> = diagnostics + .iter() + .filter(|d| d.code == DiagnosticCode::NullabilityDivergence) + .map(|d| d.message.as_str()) + .collect(); + assert_eq!(divergent.len(), 2, "{divergent:?}"); + assert!( + divergent.iter().any(|m| m.contains("`name`")), + "{divergent:?}" + ); + assert!( + divergent.iter().any(|m| m.contains("`tags[].weight`")), + "{divergent:?}" + ); + assert!( + !divergent.iter().any(|m| m.contains("`alias`")), + "a type that allows null must not warn: {divergent:?}" + ); +} + +#[test] +fn decision_boundary_names_nullability_delta() { + let mut ws = Workspace::new(); + ws.set_document( + "child", + document(line_items_child(json!(["amount", "inclusive"]))), + ); + let parent_schema = json!({ + "type": "object", + "properties": { + "lineItems": { + "type": ["array", "null"], + "items": { + "type": "object", + "properties": { + "amount": { "type": "number" }, + "inclusive": { "type": "boolean" } + }, + "required": ["amount", "inclusive"] + } + } + }, + "required": ["lineItems"] + }); + let decision = node("call", "decisionNode", json!({ "key": "child" })); + ws.set_document( + "parent", + document(linear_graph(Some(parent_schema), vec![decision])), + ); + let diagnostics = ws.diagnostics("parent"); + let errors: Vec<&str> = diagnostics + .iter() + .filter(|d| d.severity == Severity::Error) + .map(|d| d.message.as_str()) + .collect(); + assert_eq!(errors.len(), 1, "{errors:?}"); + assert!( + errors[0].contains("lineItems") && errors[0].contains("may be null"), + "the nullability delta must be stated, not flattened: {errors:?}" + ); +} + +#[test] +fn any_typed_graph_output_is_an_error_in_strict_graphs() { + let mut ws = Workspace::new(); + let graph = json!({ + "nodes": [ + node("in", "inputNode", json!({ "schema": person_schema().to_string() })), + expression_node("a", &[("x", "$nodes.b.marker")]), + expression_node("b", &[("marker", "1")]), + ], + "edges": [edge("e1", "in", "a"), edge("e2", "in", "b")] + }); + ws.set_document("g", document(graph)); + let diagnostics = ws.diagnostics("g"); + let implicit: Vec<&str> = diagnostics + .iter() + .filter(|d| d.code == DiagnosticCode::ImplicitAny && d.severity == Severity::Error) + .map(|d| d.message.as_str()) + .collect(); + assert_eq!(implicit.len(), 1, "{diagnostics:?}"); + assert!( + implicit[0].contains("`x`"), + "the any-typed output path must be named: {implicit:?}" + ); +} + +#[test] +fn recursive_decision_any_output_is_an_error() { + let mut ws = Workspace::new(); + let decision = node("call", "decisionNode", json!({ "key": "g" })); + ws.set_document( + "g", + document(linear_graph(Some(person_schema()), vec![decision])), + ); + let diagnostics = ws.diagnostics("g"); + let implicit: Vec<&str> = diagnostics + .iter() + .filter(|d| d.code == DiagnosticCode::ImplicitAny && d.severity == Severity::Error) + .map(|d| d.message.as_str()) + .collect(); + assert!( + implicit.iter().any(|m| m.contains("resolves to `any`")), + "a recursive sub-decision degrades the result to any and must error: {diagnostics:?}" + ); +} + +#[test] +fn schemaless_graph_output_any_stays_warning_only() { + let mut ws = Workspace::new(); + ws.set_document( + "g", + document(linear_graph( + None, + vec![expression_node("calc", &[("double", "value * 2")])], + )), + ); + let diagnostics = ws.diagnostics("g"); + assert!( + diagnostics.iter().all(|d| d.severity != Severity::Error), + "without an input schema the graph stays warning-only: {diagnostics:?}" + ); + assert!( + diagnostics + .iter() + .any(|d| d.code == DiagnosticCode::MissingInputSchema), + "{diagnostics:?}" + ); +} + +#[test] +fn string_literal_columns_hint_dictionary_candidates() { + let mut ws = Workspace::new(); + let table = node( + "dt", + "decisionTableNode", + json!({ + "hitPolicy": "first", + "inputs": [{ "id": "c1", "name": "Name", "field": "name" }], + "outputs": [{ "id": "o1", "name": "Verdict", "field": "verdict" }], + "rules": [ + { "_id": "r1", "c1": "\"gold\"", "o1": "\"approve\"" }, + { "_id": "r2", "c1": "\"silver\"", "o1": "\"review\"" }, + { "_id": "r3", "c1": "", "o1": "\"reject\"" } + ] + }), + ); + ws.set_document( + "g", + document(linear_graph(Some(person_schema()), vec![table])), + ); + let diagnostics = ws.diagnostics("g"); + let hints: Vec<&str> = diagnostics + .iter() + .filter(|d| d.code == DiagnosticCode::PreferDictionary) + .map(|d| d.message.as_str()) + .collect(); + assert_eq!(hints.len(), 2, "{diagnostics:?}"); + assert!( + hints + .iter() + .any(|m| m.contains("'name'") && m.contains("\"gold\" | \"silver\"")), + "{hints:?}" + ); + assert!( + hints + .iter() + .any(|m| m.contains("'verdict'") && m.contains("\"approve\" | \"review\" | \"reject\"")), + "{hints:?}" + ); +} + +#[test] +fn inline_schema_enum_hints_dictionary() { + let mut ws = Workspace::new(); + let schema = json!({ + "type": "object", + "properties": { + "status": { "type": "string", "enum": ["active", "suspended"] }, + "tier": { "$dictionary": "customerTier" } + }, + "required": ["status"] + }); + ws.set_document( + "g", + document(linear_graph( + Some(schema), + vec![expression_node("calc", &[("s", "status")])], + )), + ); + let diagnostics = ws.diagnostics("g"); + let hints: Vec<&str> = diagnostics + .iter() + .filter(|d| d.code == DiagnosticCode::PreferDictionary) + .map(|d| d.message.as_str()) + .collect(); + assert_eq!(hints.len(), 1, "{diagnostics:?}"); + assert!(hints[0].contains("`status`"), "{hints:?}"); +} + +#[test] +fn date_literal_cells_do_not_hint_dictionary() { + let mut ws = Workspace::new(); + let table = node( + "dt", + "decisionTableNode", + json!({ + "hitPolicy": "first", + "inputs": [{ "id": "c1", "name": "Name", "field": "name" }], + "outputs": [{ "id": "o1", "name": "Rate", "field": "rate" }], + "rules": [ + { "_id": "r1", "c1": "\"2024-01-01\"", "o1": "0.1" }, + { "_id": "r2", "c1": "\"2018-01-01\"", "o1": "0.2" } + ] + }), + ); + ws.set_document( + "g", + document(linear_graph(Some(person_schema()), vec![table])), + ); + let diagnostics = ws.diagnostics("g"); + assert!( + !diagnostics + .iter() + .any(|d| d.code == DiagnosticCode::PreferDictionary), + "date-keyed columns are calendars, not enums: {diagnostics:?}" + ); +} + +#[test] +fn schemaless_graph_suppresses_type_derived_expression_errors() { + let mut ws = Workspace::new(); + let graph = json!({ + "nodes": [ + node("in", "inputNode", json!({})), + expression_node("first", &[("count", "len(items)")]), + expression_node("second", &[("label", "$nodes.first.count + \"!\"")]), + ], + "edges": [edge("e1", "in", "first"), edge("e2", "first", "second")] + }); + ws.set_document("g", document(graph)); + let diagnostics = ws.diagnostics("g"); + assert!( + diagnostics.iter().all(|d| d.severity != Severity::Error), + "an unchecked graph must not raise type-derived errors: {diagnostics:?}" + ); + assert!( + diagnostics + .iter() + .any(|d| d.code == DiagnosticCode::MissingInputSchema), + "{diagnostics:?}" + ); +} + +#[test] +fn strict_graph_still_reports_nodes_scope_type_errors() { + let mut ws = Workspace::new(); + let graph = json!({ + "nodes": [ + node("in", "inputNode", json!({ "schema": person_schema().to_string() })), + expression_node("first", &[("count", "age * 2")]), + expression_node("second", &[("label", "$nodes.first.count + \"!\"")]), + ], + "edges": [edge("e1", "in", "first"), edge("e2", "first", "second")] + }); + ws.set_document("g", document(graph)); + let codes = error_codes(&ws, "g"); + assert!( + codes.contains(&DiagnosticCode::TypeMismatch), + "strict graphs keep type checking: {codes:?}" + ); +} + +fn grouped_items_schema() -> Value { + json!({ + "type": "object", + "properties": { + "items": { "type": "array", "items": { + "type": "object", + "properties": { + "grp": { "type": ["string", "null"] }, + "lines": { "type": ["array", "null"], "items": { + "type": "object", "properties": { "idx": { "type": ["number", "null"] } } } } + } + } } + }, + "required": ["items"] + }) +} + +fn single_expression_graph(schema: Value, expr: &str) -> Value { + json!({ + "nodes": [ + node("in", "inputNode", json!({ "schema": schema.to_string() })), + node("ex", "expressionNode", json!({ "expressions": [{ "id": "e1", "key": "out", "value": expr }] })), + ], + "edges": [edge("e1", "in", "ex")] + }) +} + +#[test] +fn assignment_bound_locals_keep_element_types_in_closures() { + let cases = [ + r#"map(items as m, (gp = filter(items as x, x.grp == m.grp)[0]; mw = filter(gp.lines ?? [] as c, c.idx == 0)[0]; mw))"#, + r#"(loc = filter(items as x, x.grp == "a"); map(loc as e, e.grp))"#, + r#"map(items as m, (gp = filter(items as x, x.grp == m.grp); len(gp) + (gp[0].grp == "a" ? 1 : 0)))"#, + ]; + for expr in cases { + let mut ws = Workspace::new(); + ws.set_document( + "g", + document(single_expression_graph(grouped_items_schema(), expr)), + ); + let codes = error_codes(&ws, "g"); + assert!( + codes.is_empty(), + "closure over a ;-bound local must type-check: {expr}\n{codes:?}" + ); + } +} + +#[test] +fn missing_member_through_local_names_the_member_not_the_alias() { + let mut ws = Workspace::new(); + ws.set_document( + "g", + document(single_expression_graph( + grouped_items_schema(), + r#"(loc = filter(items as x, x.grp == "a"); map(loc as e, e.doesNotExist))"#, + )), + ); + let diagnostics = ws.diagnostics("g"); + let errors: Vec<&str> = diagnostics + .iter() + .filter(|d| d.severity == Severity::Error) + .map(|d| d.message.as_str()) + .collect(); + assert_eq!(errors.len(), 1, "{diagnostics:?}"); + assert!( + errors[0].contains("doesNotExist") && !errors[0].contains("'e'"), + "the specific member must be blamed, not the alias: {errors:?}" + ); +} diff --git a/core/expression/src/intellisense/dependency.rs b/core/expression/src/intellisense/dependency.rs index 452616e4..3a30ebc7 100644 --- a/core/expression/src/intellisense/dependency.rs +++ b/core/expression/src/intellisense/dependency.rs @@ -682,10 +682,16 @@ impl<'a> DependencyResolutionWalker<'a> { match (alias, collection_source.as_ref()) { (Some(alias_name), Some(source)) => { - inner_scope.aliases.insert( - Rc::from(*alias_name), - scope.expand_alias_root(source), - ); + let expanded = scope.expand_alias_root(source); + if scope.is_local(&expanded) { + inner_scope + .unresolved_aliases + .insert(Rc::from(*alias_name)); + } else { + inner_scope + .aliases + .insert(Rc::from(*alias_name), expanded); + } } (Some(alias_name), None) => { inner_scope diff --git a/core/types/src/variable_type/util.rs b/core/types/src/variable_type/util.rs index 686eb897..34fb7f4e 100644 --- a/core/types/src/variable_type/util.rs +++ b/core/types/src/variable_type/util.rs @@ -61,8 +61,13 @@ impl VariableType { let o1 = o1.borrow(); let o2 = o2.borrow(); - o2.iter() - .all(|(k, v)| o1.get(k).is_some_and(|tv| tv.satisfies(v))) + o2.iter().all(|(k, v)| match o1.get(k) { + Some(tv) => tv.satisfies(v), + None => matches!( + v, + VariableType::Any | VariableType::Null | VariableType::Nullable(_) + ), + }) } (VariableType::Const(c1), VariableType::Const(c2)) => c1 == c2,