From 7ad19ccebdef7854392017561f651facf1990893 Mon Sep 17 00:00:00 2001 From: Keavon Chambers Date: Mon, 21 Sep 2026 04:01:02 -0700 Subject: [PATCH] Require exact truth values in math expression logic and read comparison chains as one predicate --- libraries/math-parser/src/ast.rs | 41 ++++++++++++++++++--- libraries/math-parser/src/executer.rs | 49 +++++++++++++++++++++++-- libraries/math-parser/src/lib.rs | 39 +++++++++++++++++--- libraries/math-parser/src/parser.rs | 53 +++++++++++++++++++++++---- libraries/math-parser/src/value.rs | 19 +++++++--- 5 files changed, 173 insertions(+), 28 deletions(-) diff --git a/libraries/math-parser/src/ast.rs b/libraries/math-parser/src/ast.rs index 97076312f8..51dd09d51b 100644 --- a/libraries/math-parser/src/ast.rs +++ b/libraries/math-parser/src/ast.rs @@ -17,10 +17,10 @@ pub enum BinaryOp { Add, Sub, Mul, - /// Logical AND (nonzero treated as true, returns 1. or 0.) + /// Logical AND over operands that must each be exactly 0 or 1, returning 0 or 1. And, Div, - /// Logical OR (nonzero treated as true, returns 1. or 0.) + /// Logical OR over operands that must each be exactly 0 or 1, returning 0 or 1. Or, Modulo, Pow, @@ -32,6 +32,16 @@ pub enum BinaryOp { Eq, } +impl BinaryOp { + /// Whether a chain of comparisons reads in one direction: `<`/`<=`/`==` ascending, `>`/`>=`/`==` descending, or `!=` alone. + pub fn chain_in_one_direction(ops: &[BinaryOp]) -> bool { + let ascending = ops.iter().all(|op| matches!(op, BinaryOp::Lt | BinaryOp::Leq | BinaryOp::Eq)); + let descending = ops.iter().all(|op| matches!(op, BinaryOp::Gt | BinaryOp::Geq | BinaryOp::Eq)); + let distinct = ops.iter().all(|op| matches!(op, BinaryOp::Neq)); + ascending || descending || distinct + } +} + #[derive(Debug, PartialEq, Clone, Copy)] pub enum UnaryOp { Pos, @@ -46,8 +56,27 @@ pub enum UnaryOp { pub enum Node { Lit(Literal), Var(String), - FnCall { name: String, expr: Vec }, - BinOp { lhs: Box, op: BinaryOp, rhs: Box }, - UnaryOp { expr: Box, op: UnaryOp }, - Conditional { condition: Box, if_block: Box, else_block: Box }, + FnCall { + name: String, + expr: Vec, + }, + BinOp { + lhs: Box, + op: BinaryOp, + rhs: Box, + }, + UnaryOp { + expr: Box, + op: UnaryOp, + }, + /// A chain of two or more comparisons like `a < b < c`, each operator paired with the operand after it: one predicate over each adjacent pair, or over every pair for `!=`. + Comparison { + first: Box, + rest: Vec<(BinaryOp, Node)>, + }, + Conditional { + condition: Box, + if_block: Box, + else_block: Box, + }, } diff --git a/libraries/math-parser/src/executer.rs b/libraries/math-parser/src/executer.rs index e88421db40..fbd17ae3b3 100644 --- a/libraries/math-parser/src/executer.rs +++ b/libraries/math-parser/src/executer.rs @@ -1,4 +1,4 @@ -use crate::ast::{BinaryOp, Literal, Node}; +use crate::ast::{BinaryOp, Literal, Node, UnaryOp}; use crate::constants::{builtin_function, suffixed_function}; use crate::context::{EvalContext, FunctionProvider, ValueProvider}; use crate::lexer::Constant; @@ -19,6 +19,9 @@ pub enum EvalError { #[error("Unsupported operand types for operator")] OperatorTypeError, + #[error("Logic requires values of exactly 0 (false) or 1 (true)")] + NotATruthValue, + #[error("Indeterminate result, like `0/0` or `∞ - ∞`")] Indeterminate, @@ -71,11 +74,47 @@ impl Node { }, Node::BinOp { lhs, op, rhs } => match (lhs.eval(context)?, rhs.eval(context)?) { - (Value::Number(lhs), Value::Number(rhs)) => settle(Value::Number(lhs.binary_op(*op, rhs).ok_or(EvalError::OperatorTypeError)?)), + (Value::Number(lhs), Value::Number(rhs)) => { + // Logic rejects operands that aren't truth values, while the other operators reject operand types they don't support + let rejected = if matches!(op, BinaryOp::And | BinaryOp::Or) { + EvalError::NotATruthValue + } else { + EvalError::OperatorTypeError + }; + settle(Value::Number(lhs.binary_op(*op, rhs).ok_or(rejected)?)) + } }, Node::UnaryOp { expr, op } => match expr.eval(context)? { - Value::Number(num) => settle(Value::Number(num.unary_op(*op).ok_or(EvalError::OperatorTypeError)?)), + Value::Number(num) => { + let rejected = if *op == UnaryOp::Not { EvalError::NotATruthValue } else { EvalError::OperatorTypeError }; + settle(Value::Number(num.unary_op(*op).ok_or(rejected)?)) + } }, + Node::Comparison { first, rest } => { + let Value::Number(first) = first.eval(context)?; + let rest = rest + .iter() + .map(|(op, operand)| operand.eval(context).map(|Value::Number(number)| (*op, number))) + .collect::, EvalError>>()?; + + // A `!=` chain asserts every pair distinct, while the ordered chains assert each adjacent pair's relation; every pair is checked so an unsupported comparison errors regardless of the others + let holds = if rest.iter().all(|(op, _)| *op == BinaryOp::Neq) { + let numbers: Vec = std::iter::once(first).chain(rest.iter().map(|(_, number)| *number)).collect(); + numbers + .iter() + .enumerate() + .all(|(index, a)| numbers[index + 1..].iter().all(|b| a.binary_op(BinaryOp::Neq, *b) == Some(Number::Real(1.)))) + } else { + let mut holds = true; + let mut previous = first; + for (op, number) in rest { + holds &= previous.binary_op(op, number).ok_or(EvalError::OperatorTypeError)? == Number::Real(1.); + previous = number; + } + holds + }; + Ok(Value::from_f64(holds as u8 as f64)) + } Node::Var(name) => { let value = resolve_value(context, name).ok_or_else(|| EvalError::MissingValue(name.clone()))?; canonical_host_value(name, value) @@ -120,7 +159,9 @@ impl Node { } Node::Conditional { condition, if_block, else_block } => { let Value::Number(number) = condition.eval(context)?; - if number.as_bool() { if_block.eval(context) } else { else_block.eval(context) } + let Some(condition) = number.as_bool() else { return Err(EvalError::NotATruthValue) }; + + if condition { if_block.eval(context) } else { else_block.eval(context) } } } } diff --git a/libraries/math-parser/src/lib.rs b/libraries/math-parser/src/lib.rs index d44dd25dc9..32d4f9652f 100644 --- a/libraries/math-parser/src/lib.rs +++ b/libraries/math-parser/src/lib.rs @@ -108,6 +108,23 @@ mod tests { } } + #[test] + fn comparison_chains_stay_in_one_direction() { + for input in ["1 < 2 > 1", "1 < 2 != 3", "1 == 2 != 2"] { + let error = evaluate(input).unwrap_err().to_string(); + let expected = "A comparison chain must read in one direction: all ascending (`<`, `<=`, `==`), all descending (`>`, `>=`, `==`), or all `!=`"; + assert_eq!(error, format!("{expected} at 0..{}", input.len()), "`{input}`"); + } + } + + #[test] + fn logic_requires_truth_values() { + // A logical operand must be exactly 0 or 1, so a general number becomes a truth value only through a comparison + for input in ["!5", "2 && 1", "0.5 || 0", "if(3, 1, 0)", "1 && i", "if(sqrt(-1), 1, 2)"] { + assert!(matches!(evaluate(input).unwrap(), Err(EvalError::NotATruthValue)), "expected `{input}` to need a truth value"); + } + } + #[test] fn indeterminate_forms_are_errors() { // No operation returns NaN: an indeterminate form is an evaluation error, as is a domain failure with no complex answer @@ -469,7 +486,7 @@ mod tests { if_arithmetic_false: "if(3*2-5, 1, 0)" => 1., // Nested arithmetic - if_complex_arithmetic: "if((5+3)*(2-1), 10, 20)" => 10., + if_complex_arithmetic: "if((5+3)*(2-1) > 0, 10, 20)" => 10., if_with_division: "if(8/4-2 == 0, 15, 25)" => 15., if_with_division_ne: "if(8/4-2 ≠ 0, 15, 25)" => 25., @@ -483,7 +500,7 @@ mod tests { // Logical NOT (prefix !) logical_not_zero: "!0" => 1., - logical_not_nonzero: "!5" => 0., + logical_not_one: "!1" => 0., logical_not_expression: "!(2 - 2)" => 1., // Log / exp / pow / root @@ -591,7 +608,7 @@ mod tests { if_zero: "if(0.0, 1, 2)" => 2., // Complex nested expressions - if_nested_expr: "if((sqrt(16) + 2) * (sin(pi) + 1), 3 + 4 * 2, 5 - 2 / 1)" => 11., + if_nested_expr: "if((sqrt(16) + 2) * (sin(pi) + 1) > 5, 3 + 4 * 2, 5 - 2 / 1)" => 11., // Overflow-safe evaluation factorial_overflows_to_infinity: "171!" => f64::INFINITY, @@ -605,10 +622,9 @@ mod tests { root_negative_odd_reciprocal: "root(-8, -3)" => -0.5, root_negative_even: "root(-4, 2)" => Complex::new(0., 2.), - // Logic and equality span real and complex operands + // Equality spans real and complex operands mixed_equality: "1 == i" => 0., complex_equality: "i == i" => 1., - mixed_and: "1 && i" => 1., // Value identity: a zero imaginary part or a signed zero never changes a result, so a result landing on the real line is real value_identity_product: "sqrt(-4) * sqrt(-4)" => -4., @@ -742,6 +758,15 @@ mod tests { not_inside_magnitude: "|!0|" => 1., not_inside_magnitude_with_or: "| !0 || 0 |" => 1., + // Ordered chains are one predicate over adjacent pairs, like interval notation, rather than `(a < b) < c`, while `!=` chains require every pair to differ + chain_interval: "0 <= 0.5 < 1" => 1., + chain_fails_on_one_pair: "1 < 2 < 2" => 0., + chain_not_c_style: "3 < 2 < 1" => 0., + chain_descending: "3 > 2 >= 2" => 1., + chain_equality: "1 == 1 == 1" => 1., + chain_distinct: "1 != 2 != 3" => 1., + chain_distinct_all_pairs: "1 != 2 != 1" => 0., + // Correctly rounded literals via std parsing seventeen_digit_literal: "999999999999999999" => 1e18, long_fraction_literal: "0.1111111111111111111111111111111111111111" => 1. / 9., @@ -819,5 +844,9 @@ mod tests { assert_eq!(evaluate("300").unwrap().unwrap().as_u8(), None); assert_eq!(evaluate("i").unwrap().unwrap().as_i64(), None); assert_eq!(evaluate("inf").unwrap().unwrap().as_u64(), None); + + // A truth value is exactly 0 or 1 + assert_eq!(evaluate("2 > 1").unwrap().unwrap().as_bool(), Some(true)); + assert_eq!(evaluate("0.5").unwrap().unwrap().as_bool(), None); } } diff --git a/libraries/math-parser/src/parser.rs b/libraries/math-parser/src/parser.rs index 8488e3bdf3..59ceed6e30 100644 --- a/libraries/math-parser/src/parser.rs +++ b/libraries/math-parser/src/parser.rs @@ -1,6 +1,6 @@ use crate::ast::{BinaryOp, Literal, Node, UnaryOp}; use crate::lexer::{Lexer, Span, Token}; -use chumsky::error::LabelError; +use chumsky::error::{EmptyErr, LabelError}; use chumsky::input::ValueInput; use chumsky::{Parser, prelude::*}; use std::fmt; @@ -23,6 +23,23 @@ impl fmt::Display for ParseError { impl std::error::Error for ParseError {} +/// Builds a parse error from a plain message, for a failure that no "expected ..., found ..." phrasing describes. +pub trait CustomError { + fn custom(span: Span, message: &'static str) -> Self; +} + +impl CustomError for EmptyErr { + fn custom(_: Span, _: &'static str) -> Self { + EmptyErr::default() + } +} + +impl<'src> CustomError for Rich<'src, Token<'src>, Span> { + fn custom(span: Span, message: &'static str) -> Self { + Rich::custom(span, message) + } +} + impl Node { pub fn try_parse_from_str(src: &str) -> Result { // Parse with zero-cost errors first (several times faster), then re-parse invalid input with rich errors to build the messages @@ -41,7 +58,7 @@ pub fn parser<'src, I, E>() -> impl Parser<'src, I, Node, E> where I: ValueInput<'src, Token = Token<'src>, Span = Span>, E: extra::ParserExtra<'src, I>, - E::Error: LabelError<'src, I, &'static str>, + E::Error: LabelError<'src, I, &'static str> + CustomError, { recursive(|expr| { let constant = select! { Token::Float(f) => Node::Lit(Literal::Float(f)) }; @@ -130,11 +147,33 @@ where rhs: Box::new(rhs), }); - let cmp = add.clone().foldl(cmp_op.then(add).repeated(), |lhs: Node, (op, rhs)| Node::BinOp { - lhs: Box::new(lhs), - op, - rhs: Box::new(rhs), - }); + // A chain like `0 <= x < 1` is one predicate over its adjacent pairs, not an implicit `(0 <= x) < 1` (which is only read that way when its parentheses are written out), and must read in one direction + let cmp = add + .clone() + .then(cmp_op.then(add).repeated().collect::>()) + .try_map(|(first, mut rest): (Node, Vec<(BinaryOp, Node)>), span| { + // A lone comparison is an ordinary binary operation + if rest.len() <= 1 { + return Ok(match rest.pop() { + Some((op, second)) => Node::BinOp { + lhs: Box::new(first), + op, + rhs: Box::new(second), + }, + None => first, + }); + } + + let ops: Vec = rest.iter().map(|(op, _)| *op).collect(); + if !BinaryOp::chain_in_one_direction(&ops) { + return Err(CustomError::custom( + span, + "A comparison chain must read in one direction: all ascending (`<`, `<=`, `==`), all descending (`>`, `>=`, `==`), or all `!=`", + )); + } + + Ok(Node::Comparison { first: Box::new(first), rest }) + }); let and = cmp.clone().foldl(and_op.then(cmp).repeated(), |lhs, (op, rhs)| Node::BinOp { lhs: Box::new(lhs), diff --git a/libraries/math-parser/src/value.rs b/libraries/math-parser/src/value.rs index f5ef02d328..0664f5d94c 100644 --- a/libraries/math-parser/src/value.rs +++ b/libraries/math-parser/src/value.rs @@ -38,6 +38,12 @@ impl Value { self.as_real().map(|real| real as f32) } + /// Reads the value as a truth value, or `None` unless it is exactly 0 or 1. + pub fn as_bool(&self) -> Option { + let Self::Number(number) = self; + number.as_bool() + } + integer_accessors! { as_u8: u8, as_u16: u16, @@ -96,11 +102,12 @@ impl Number { } } - /// The value's truthiness for conditions and logic operators, where any nonzero number is true. - pub fn as_bool(self) -> bool { + /// The truth value of a logical operand, which must be exactly 0 or 1: any other number is not a truth value, so logic on it is an error rather than a guess. + pub fn as_bool(self) -> Option { match self { - Number::Real(real) => real != 0., - Number::Complex(complex) => complex != Complex::ZERO, + Number::Real(0.) => Some(false), + Number::Real(1.) => Some(true), + _ => None, } } @@ -126,7 +133,7 @@ impl Number { // Logic and equality work uniformly across real and complex operands match op { BinaryOp::And | BinaryOp::Or => { - let (lhs, rhs) = (self.as_bool(), other.as_bool()); + let (Some(lhs), Some(rhs)) = (self.as_bool(), other.as_bool()) else { return None }; let result = if matches!(op, BinaryOp::And) { lhs && rhs } else { lhs || rhs }; return Some(Number::Real(result as u8 as f64)); } @@ -218,7 +225,7 @@ impl Number { Number::Real(real) => Number::Real(-real), Number::Complex(complex) => Number::Complex(-complex), }), - UnaryOp::Not => Some(Number::Real(!self.as_bool() as u8 as f64)), + UnaryOp::Not => self.as_bool().map(|boolean| Number::Real(!boolean as u8 as f64)), UnaryOp::Magnitude => Some(Number::Real(match self { Number::Real(real) => real.abs(), Number::Complex(complex) => complex.norm(),