From 8802babd023dde765953f618c4e7c19ce6a095bb Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 23 Apr 2026 10:03:51 +0000 Subject: [PATCH 01/25] refactor(parser): modularize rule classification and expression parsing - Extract rule-body term classification into a dedicated module `rule/classification.rs`. - Move S-expression formatting for Expr into a separate `expr/sexpr.rs` module. - Split Pratt parser postfix parsing, delay-postfix, and diff-marker handling into separate files under `expression/pratt/`. - Remove inline implementations and validations, organizing parser logic into coherent units. - Revamp expression parser tests to use fixture modules, improving maintainability. This refactor improves code organization, readability, and test structure without changing external behavior. Co-authored-by: devboxerhub[bot] --- src/parser/ast/expr.rs | 147 +----- src/parser/ast/expr/sexpr.rs | 156 +++++++ src/parser/ast/rule.rs | 277 +----------- src/parser/ast/rule/classification.rs | 270 +++++++++++ src/parser/expression/pratt.rs | 231 +--------- src/parser/expression/pratt/delay.rs | 47 ++ src/parser/expression/pratt/diff.rs | 58 +++ src/parser/expression/pratt/postfix.rs | 154 +++++++ src/parser/tests/expression.rs | 422 +++--------------- src/parser/tests/expression/fixtures.rs | 55 +++ .../tests/expression/fixtures/errors.rs | 232 ++++++++++ src/parser/tests/expression/fixtures/valid.rs | 26 ++ .../tests/expression/fixtures/valid/basic.rs | 80 ++++ .../expression/fixtures/valid/collections.rs | 71 +++ .../expression/fixtures/valid/control_flow.rs | 173 +++++++ .../expression/fixtures/valid/postfix.rs | 67 +++ .../expression/fixtures/valid/structured.rs | 65 +++ 17 files changed, 1538 insertions(+), 993 deletions(-) create mode 100644 src/parser/ast/expr/sexpr.rs create mode 100644 src/parser/ast/rule/classification.rs create mode 100644 src/parser/expression/pratt/delay.rs create mode 100644 src/parser/expression/pratt/diff.rs create mode 100644 src/parser/expression/pratt/postfix.rs create mode 100644 src/parser/tests/expression/fixtures.rs create mode 100644 src/parser/tests/expression/fixtures/errors.rs create mode 100644 src/parser/tests/expression/fixtures/valid.rs create mode 100644 src/parser/tests/expression/fixtures/valid/basic.rs create mode 100644 src/parser/tests/expression/fixtures/valid/collections.rs create mode 100644 src/parser/tests/expression/fixtures/valid/control_flow.rs create mode 100644 src/parser/tests/expression/fixtures/valid/postfix.rs create mode 100644 src/parser/tests/expression/fixtures/valid/structured.rs diff --git a/src/parser/ast/expr.rs b/src/parser/ast/expr.rs index c1624419..577eeb96 100644 --- a/src/parser/ast/expr.rs +++ b/src/parser/ast/expr.rs @@ -5,6 +5,8 @@ //! can inspect without re-parsing tokens. use std::fmt; +mod sexpr; + use super::number::NumberLiteral; use super::pattern::Pattern; use super::string_literal::StringLiteral; @@ -308,148 +310,3 @@ pub enum Expr { /// for later lowering stages, not current parser behaviour. MapLit(Vec<(Expr, Expr)>), } -impl Expr { - /// Display the expression as a simple S-expression for tests. - #[must_use] - pub fn to_sexpr(&self) -> String { - match self { - Self::Literal(Literal::Number(n)) => n.to_sexpr(), - Self::Literal(Literal::String(s)) => s.to_sexpr(), - Self::Literal(Literal::Bool(b)) => b.to_string(), - Self::Variable(name) => name.clone(), - Self::Apply { callee, args } | Self::Call { callee, args } => format_nary( - "call", - std::iter::once(callee.to_sexpr()).chain(args.iter().map(Self::to_sexpr)), - ), - Self::MethodCall { recv, name, args } => format_nary( - "method", - std::iter::once(recv.to_sexpr()) - .chain(std::iter::once(name.clone())) - .chain(args.iter().map(Self::to_sexpr)), - ), - Self::FieldAccess { expr, field } => { - format_nary("field", [expr.to_sexpr(), field.clone()]) - } - Self::TupleIndex { expr, index } => { - format_nary("tuple-index", [expr.to_sexpr(), index.clone()]) - } - Self::BitSlice { expr, hi, lo } => { - format_nary("bitslice", [expr.to_sexpr(), hi.to_sexpr(), lo.to_sexpr()]) - } - Self::Struct { name, fields } => format_nary( - "struct", - std::iter::once(name.clone()) - .chain(fields.iter().map(|(n, e)| format_field(n, &e.to_sexpr()))), - ), - Self::Tuple(items) => format_nary("tuple", items.iter().map(Self::to_sexpr)), - Self::Closure { params, body } => format_nary( - "closure", - [format!("({})", params.join(" ")), body.to_sexpr()], - ), - Self::IfElse { - condition, - then_branch, - else_branch, - } => format_if_else(condition, then_branch, else_branch), - Self::Unary { op, expr } => format_nary(op.symbol(), std::iter::once(expr.to_sexpr())), - Self::AtomDiff { expr } => format_nary("diff", [expr.to_sexpr()]), - Self::AtomDelay { delay, expr } => { - format_nary("delay", [delay.to_string(), expr.to_sexpr()]) - } - Self::Binary { op, lhs, rhs } => { - format_nary(op.symbol(), [lhs.to_sexpr(), rhs.to_sexpr()]) - } - Self::Group(e) => format_nary("group", std::iter::once(e.to_sexpr())), - Self::ForLoop { - pattern, - iterable, - guard, - body, - } => format_for_loop(pattern, iterable, guard.as_deref(), body), - Self::Match { scrutinee, arms } => format_match(scrutinee, arms), - Self::Break => "(break)".to_string(), - Self::Continue => "(continue)".to_string(), - Self::Return { value } => format_nary("return", [value.to_sexpr()]), - Self::VecLit(items) => format_nary("vec", items.iter().map(Self::to_sexpr)), - Self::MapLit(entries) => { - format_nary("map", entries.iter().map(|(k, v)| format_kv(k, v))) - } - } - } -} - -#[inline] -fn format_field(name: &str, value: &str) -> String { - let mut s = String::with_capacity(name.len() + value.len() + 3); - s.push('('); - s.push_str(name); - s.push(' '); - s.push_str(value); - s.push(')'); - s -} - -#[inline] -fn format_kv(key: &Expr, value: &Expr) -> String { - let k = key.to_sexpr(); - let v = value.to_sexpr(); - let mut s = String::with_capacity(k.len() + v.len() + 10); - s.push_str("(entry "); - s.push_str(&k); - s.push(' '); - s.push_str(&v); - s.push(')'); - s -} - -fn format_nary(label: &str, parts: I) -> String -where - I: IntoIterator, -{ - let parts_vec: Vec = parts.into_iter().collect(); - let cap = 2 + label.len() + parts_vec.iter().map(|p| 1 + p.len()).sum::(); - let mut out = String::with_capacity(cap); - out.push('('); - out.push_str(label); - for part in parts_vec { - out.push(' '); - out.push_str(&part); - } - out.push(')'); - out -} - -fn format_if_else(condition: &Expr, then_branch: &Expr, else_branch: &Expr) -> String { - format_nary( - "if", - [ - condition.to_sexpr(), - then_branch.to_sexpr(), - else_branch.to_sexpr(), - ], - ) -} - -fn format_for_loop( - pattern: &Pattern, - iterable: &Expr, - guard: Option<&Expr>, - body: &Expr, -) -> String { - let mut parts = vec![pattern.to_source(), iterable.to_sexpr()]; - if let Some(cond) = guard { - parts.push(cond.to_sexpr()); - } - parts.push(body.to_sexpr()); - format_nary("for", parts) -} - -fn format_match(scrutinee: &Expr, arms: &[MatchArm]) -> String { - let arm_parts = arms - .iter() - .map(|arm| format_nary("arm", [arm.pattern.to_source(), arm.body.to_sexpr()])); - format_nary( - "match", - std::iter::once(scrutinee.to_sexpr()).chain(arm_parts), - ) -} diff --git a/src/parser/ast/expr/sexpr.rs b/src/parser/ast/expr/sexpr.rs new file mode 100644 index 00000000..5f55ba1d --- /dev/null +++ b/src/parser/ast/expr/sexpr.rs @@ -0,0 +1,156 @@ +//! S-expression formatting helpers for [`super::Expr`]. +//! +//! Tests use these compact renderings to compare parsed structure without +//! coupling to debug formatting. + +use super::{Expr, Literal, MatchArm, Pattern}; + +impl Expr { + /// Display the expression as a simple S-expression for tests. + #[must_use] + pub fn to_sexpr(&self) -> String { + match self { + Self::Literal(Literal::Number(n)) => n.to_sexpr(), + Self::Literal(Literal::String(s)) => s.to_sexpr(), + Self::Literal(Literal::Bool(b)) => b.to_string(), + Self::Variable(name) => name.clone(), + Self::Apply { callee, args } | Self::Call { callee, args } => format_nary( + "call", + std::iter::once(callee.to_sexpr()).chain(args.iter().map(Self::to_sexpr)), + ), + Self::MethodCall { recv, name, args } => format_nary( + "method", + std::iter::once(recv.to_sexpr()) + .chain(std::iter::once(name.clone())) + .chain(args.iter().map(Self::to_sexpr)), + ), + Self::FieldAccess { expr, field } => { + format_nary("field", [expr.to_sexpr(), field.clone()]) + } + Self::TupleIndex { expr, index } => { + format_nary("tuple-index", [expr.to_sexpr(), index.clone()]) + } + Self::BitSlice { expr, hi, lo } => { + format_nary("bitslice", [expr.to_sexpr(), hi.to_sexpr(), lo.to_sexpr()]) + } + Self::Struct { name, fields } => format_nary( + "struct", + std::iter::once(name.clone()).chain( + fields + .iter() + .map(|(name, expr)| format_field(name, &expr.to_sexpr())), + ), + ), + Self::Tuple(items) => format_nary("tuple", items.iter().map(Self::to_sexpr)), + Self::Closure { params, body } => format_nary( + "closure", + [format!("({})", params.join(" ")), body.to_sexpr()], + ), + Self::IfElse { + condition, + then_branch, + else_branch, + } => format_if_else(condition, then_branch, else_branch), + Self::Unary { op, expr } => format_nary(op.symbol(), std::iter::once(expr.to_sexpr())), + Self::AtomDiff { expr } => format_nary("diff", [expr.to_sexpr()]), + Self::AtomDelay { delay, expr } => { + format_nary("delay", [delay.to_string(), expr.to_sexpr()]) + } + Self::Binary { op, lhs, rhs } => { + format_nary(op.symbol(), [lhs.to_sexpr(), rhs.to_sexpr()]) + } + Self::Group(expr) => format_nary("group", std::iter::once(expr.to_sexpr())), + Self::ForLoop { + pattern, + iterable, + guard, + body, + } => format_for_loop(pattern, iterable, guard.as_deref(), body), + Self::Match { scrutinee, arms } => format_match(scrutinee, arms), + Self::Break => "(break)".to_string(), + Self::Continue => "(continue)".to_string(), + Self::Return { value } => format_nary("return", [value.to_sexpr()]), + Self::VecLit(items) => format_nary("vec", items.iter().map(Self::to_sexpr)), + Self::MapLit(entries) => format_nary( + "map", + entries.iter().map(|(key, value)| format_kv(key, value)), + ), + } + } +} + +#[inline] +fn format_field(name: &str, value: &str) -> String { + let mut rendered = String::with_capacity(name.len() + value.len() + 3); + rendered.push('('); + rendered.push_str(name); + rendered.push(' '); + rendered.push_str(value); + rendered.push(')'); + rendered +} + +#[inline] +fn format_kv(key: &Expr, value: &Expr) -> String { + let key = key.to_sexpr(); + let value = value.to_sexpr(); + let mut rendered = String::with_capacity(key.len() + value.len() + 10); + rendered.push_str("(entry "); + rendered.push_str(&key); + rendered.push(' '); + rendered.push_str(&value); + rendered.push(')'); + rendered +} + +fn format_nary(label: &str, parts: I) -> String +where + I: IntoIterator, +{ + let parts: Vec = parts.into_iter().collect(); + let capacity = 2 + label.len() + parts.iter().map(|part| 1 + part.len()).sum::(); + let mut rendered = String::with_capacity(capacity); + rendered.push('('); + rendered.push_str(label); + for part in parts { + rendered.push(' '); + rendered.push_str(&part); + } + rendered.push(')'); + rendered +} + +fn format_if_else(condition: &Expr, then_branch: &Expr, else_branch: &Expr) -> String { + format_nary( + "if", + [ + condition.to_sexpr(), + then_branch.to_sexpr(), + else_branch.to_sexpr(), + ], + ) +} + +fn format_for_loop( + pattern: &Pattern, + iterable: &Expr, + guard: Option<&Expr>, + body: &Expr, +) -> String { + let mut parts = vec![pattern.to_source(), iterable.to_sexpr()]; + if let Some(condition) = guard { + parts.push(condition.to_sexpr()); + } + parts.push(body.to_sexpr()); + format_nary("for", parts) +} + +fn format_match(scrutinee: &Expr, arms: &[MatchArm]) -> String { + let arm_parts = arms + .iter() + .map(|arm| format_nary("arm", [arm.pattern.to_source(), arm.body.to_sexpr()])); + format_nary( + "match", + std::iter::once(scrutinee.to_sexpr()).chain(arm_parts), + ) +} diff --git a/src/parser/ast/rule.rs b/src/parser/ast/rule.rs index 442d3534..fa4ca038 100644 --- a/src/parser/ast/rule.rs +++ b/src/parser/ast/rule.rs @@ -29,12 +29,12 @@ use chumsky::error::Simple; +mod classification; + use super::rule_head::{RuleHead, first_head_text, parse_rule_heads}; -use super::{AstNode, BinaryOp, Expr, Pattern}; +use super::{AstNode, Expr, Pattern}; use crate::parser::delimiter::find_top_level_eq_span; -use crate::parser::expression::parse_expression; -use crate::parser::pattern::parse_pattern; -use crate::parser::span_utils::{shift_errors, trim_byte_range}; +use crate::parser::span_utils::trim_byte_range; use crate::{DdlogLanguage, Span, SyntaxKind}; /// Typed wrapper for a rule declaration. @@ -183,31 +183,6 @@ impl Rule { } } -/// Validate that at most one aggregation appears in the rule body. -/// -/// If `term` is an aggregation, checks whether a previous aggregation was already -/// seen. Records an error and returns `true` (skip) if so, otherwise stores the -/// current span as the first and returns `false` (include). -fn validate_aggregation( - term: &RuleBodyTerm, - literal_span: &Span, - first_aggregation_span: &mut Option, - errors: &mut Vec>, -) -> bool { - if let RuleBodyTerm::Aggregation(_) = term { - match first_aggregation_span { - Some(first_span) => { - errors.push(multiple_aggregations_error(first_span, literal_span)); - return true; // skip duplicate aggregation - } - None => { - *first_aggregation_span = Some(literal_span.clone()); - } - } - } - false -} - impl_ast_node!(Rule); /// Wrapper for a single rule body expression CST node. @@ -242,30 +217,7 @@ impl RuleBodyExpression { let literal_span = self.span(); - if let Some(parts) = split_assignment(&raw) { - match parse_assignment(&parts, &literal_span) { - Ok(term) => return term, - Err(mut errs) => { - errors.append(&mut errs); - return None; - } - } - } - - match parse_expression(trimmed) { - Ok(expr) => { - let mut ctx = ClassificationContext { - literal_span: &literal_span, - first_aggregation_span, - errors, - }; - classify_expression(expr, &mut ctx) - } - Err(mut errs) => { - errors.append(&mut errs); - None - } - } + classification::parse_rule_body_term(&raw, literal_span, first_aggregation_span, errors) } } @@ -394,225 +346,6 @@ pub struct RuleAggregation { pub source: AggregationSource, } -fn parse_assignment( - parts: &AssignmentParts, - literal_span: &Span, -) -> Result, Vec>> { - if parts.pattern.trim().is_empty() { - return Err(vec![Simple::custom( - literal_span.clone(), - "expected pattern before '=' in rule literal", - )]); - } - - if parts.value.is_empty() { - return Err(vec![Simple::custom( - literal_span.clone(), - "expected expression after '=' in rule literal", - )]); - } - - let pattern_base_offset = literal_span.start.saturating_add(parts.pattern_offset); - let pattern = match parse_pattern(&parts.pattern) { - Ok(pattern) => pattern, - Err(errs) => return Err(shift_errors(errs, pattern_base_offset)), - }; - - let value_base_offset = literal_span.start.saturating_add(parts.value_offset); - match parse_expression(&parts.value) { - Ok(expr) => Ok(Some(RuleBodyTerm::Assignment(RuleAssignment { - pattern, - value: expr, - }))), - Err(errs) => Err(shift_errors(errs, value_base_offset)), - } -} - -/// Context for classifying rule body expressions with aggregation tracking. -struct ClassificationContext<'a> { - literal_span: &'a Span, - first_aggregation_span: &'a mut Option, - errors: &'a mut Vec>, -} - -fn classify_expression(expr: Expr, ctx: &mut ClassificationContext<'_>) -> Option { - match expr { - Expr::ForLoop { - pattern, - iterable, - guard, - body, - } => Some(classify_for_loop( - ForLoopComponents { - pattern, - iterable: *iterable, - guard: guard.map(|g| *g), - body: *body, - }, - ctx, - )), - Expr::Apply { callee, args } => { - if let Some(source) = invocation_aggregation_source(&callee) { - return classify_aggregation_with_tracking(args, source, ctx); - } - Some(RuleBodyTerm::Expression(Expr::Apply { callee, args })) - } - Expr::Call { callee, args } => { - if let Some(source) = invocation_aggregation_source(&callee) { - return classify_aggregation_with_tracking(args, source, ctx); - } - Some(RuleBodyTerm::Expression(Expr::Call { callee, args })) - } - other => Some(RuleBodyTerm::Expression(other)), - } -} - -fn aggregation_source_for(name: &str) -> Option { - match name { - "group_by" => Some(AggregationSource::GroupBy), - "Aggregate" => Some(AggregationSource::LegacyAggregate), - _ => None, - } -} - -fn invocation_aggregation_source(callee: &Expr) -> Option { - if let Expr::Variable(name) = callee { - aggregation_source_for(name.as_str()) - } else { - None - } -} - -/// Classify an aggregation with tracking to enforce at-most-one-per-rule-body. -fn classify_aggregation_with_tracking( - args: Vec, - source: AggregationSource, - ctx: &mut ClassificationContext<'_>, -) -> Option { - if args.len() != 2 { - ctx.errors - .extend(aggregation_arity_error(ctx.literal_span, source)); - return None; - } - - let mut iter = args.into_iter(); - let first = iter - .next() - .unwrap_or_else(|| unreachable!("len pre-checked as 2; first arg missing")); - let second = iter - .next() - .unwrap_or_else(|| unreachable!("len pre-checked as 2; second arg missing")); - let (project, key) = match source { - AggregationSource::GroupBy => (first, second), - AggregationSource::LegacyAggregate => (second, first), - }; - - let term = RuleBodyTerm::Aggregation(RuleAggregation { - project, - key, - source, - }); - - // Track and validate aggregation - if validate_aggregation( - &term, - ctx.literal_span, - ctx.first_aggregation_span, - ctx.errors, - ) { - None // skip duplicate aggregation - } else { - Some(term) - } -} - -fn aggregation_arity_error( - literal_span: &Span, - source: AggregationSource, -) -> Vec> { - vec![Simple::custom( - literal_span.clone(), - format!("{} expects exactly two arguments", source.label()), - )] -} - -/// Components of a for-loop expression extracted for classification. -struct ForLoopComponents { - pattern: Pattern, - iterable: Expr, - guard: Option, - body: Expr, -} - -/// Classify a for-loop expression into a structured `RuleForLoop` term. -/// -/// Recursively classifies the body expression to produce nested body terms. -/// Aggregation validation is delegated to `classify_for_body_with_aggregation_tracking`. -fn classify_for_loop( - components: ForLoopComponents, - ctx: &mut ClassificationContext<'_>, -) -> RuleBodyTerm { - let body_terms = classify_for_body_with_aggregation_tracking(components.body, ctx); - - RuleBodyTerm::ForLoop(RuleForLoop { - pattern: components.pattern, - iterable: components.iterable, - guard: components.guard, - body_terms, - }) -} - -/// Recursively classify a for-loop body into a sequence of body terms with aggregation tracking. -/// -/// Handles sequence expressions (block bodies with multiple statements), -/// nested for-loops, and single expressions. Aggregations are tracked to enforce the -/// "at most one aggregation per rule body" rule. -fn classify_for_body_with_aggregation_tracking( - body: Expr, - ctx: &mut ClassificationContext<'_>, -) -> Vec { - match body { - // Handle sequence expressions from block bodies - Expr::Binary { - op: BinaryOp::Seq, - lhs, - rhs, - } => { - let mut terms = classify_for_body_with_aggregation_tracking(*lhs, ctx); - terms.extend(classify_for_body_with_aggregation_tracking(*rhs, ctx)); - terms - } - // Handle nested for-loops - Expr::ForLoop { - pattern, - iterable, - guard, - body, - } => { - vec![classify_for_loop( - ForLoopComponents { - pattern, - iterable: *iterable, - guard: guard.map(|g| *g), - body: *body, - }, - ctx, - )] - } - // Unwrap groups (parentheses/braces) to handle `(a; b)` bodies - Expr::Group(inner) => classify_for_body_with_aggregation_tracking(*inner, ctx), - // Single expression - classify it directly - other => classify_expression(other, ctx).into_iter().collect(), - } -} - -fn multiple_aggregations_error(_first_span: &Span, second_span: &Span) -> Simple { - Simple::custom( - second_span.clone(), - "at most one aggregation (group_by or Aggregate) is permitted per rule body".to_string(), - ) -} - #[derive(Debug)] pub(crate) struct AssignmentParts { pub(crate) pattern: String, diff --git a/src/parser/ast/rule/classification.rs b/src/parser/ast/rule/classification.rs new file mode 100644 index 00000000..6d18e170 --- /dev/null +++ b/src/parser/ast/rule/classification.rs @@ -0,0 +1,270 @@ +//! Rule-body term classification helpers for [`super::Rule`]. +//! +//! This module keeps the raw-literal classification pipeline separate from the +//! main AST wrapper types so `rule.rs` stays focused on the public surface. + +use chumsky::error::Simple; + +use super::{ + AggregationSource, AssignmentParts, RuleAggregation, RuleAssignment, RuleBodyTerm, RuleForLoop, + split_assignment, +}; +use crate::parser::ast::{BinaryOp, Expr, Pattern}; +use crate::parser::expression::parse_expression; +use crate::parser::pattern::parse_pattern; +use crate::parser::span_utils::shift_errors; +use crate::{Span, SyntaxKind}; + +pub(super) fn parse_rule_body_term( + raw: &str, + literal_span: Span, + first_aggregation_span: &mut Option, + errors: &mut Vec>, +) -> Option { + if let Some(parts) = split_assignment(raw) { + match parse_assignment(&parts, &literal_span) { + Ok(term) => return term, + Err(mut errs) => { + errors.append(&mut errs); + return None; + } + } + } + + match parse_expression(raw.trim()) { + Ok(expr) => { + let mut ctx = ClassificationContext { + literal_span: &literal_span, + first_aggregation_span, + errors, + }; + classify_expression(expr, &mut ctx) + } + Err(mut errs) => { + errors.append(&mut errs); + None + } + } +} + +fn parse_assignment( + parts: &AssignmentParts, + literal_span: &Span, +) -> Result, Vec>> { + if parts.pattern.trim().is_empty() { + return Err(vec![Simple::custom( + literal_span.clone(), + "expected pattern before '=' in rule literal", + )]); + } + + if parts.value.is_empty() { + return Err(vec![Simple::custom( + literal_span.clone(), + "expected expression after '=' in rule literal", + )]); + } + + let pattern_base_offset = literal_span.start.saturating_add(parts.pattern_offset); + let pattern = match parse_pattern(&parts.pattern) { + Ok(pattern) => pattern, + Err(errs) => return Err(shift_errors(errs, pattern_base_offset)), + }; + + let value_base_offset = literal_span.start.saturating_add(parts.value_offset); + match parse_expression(&parts.value) { + Ok(expr) => Ok(Some(RuleBodyTerm::Assignment(RuleAssignment { + pattern, + value: expr, + }))), + Err(errs) => Err(shift_errors(errs, value_base_offset)), + } +} + +struct ClassificationContext<'a> { + literal_span: &'a Span, + first_aggregation_span: &'a mut Option, + errors: &'a mut Vec>, +} + +fn classify_expression(expr: Expr, ctx: &mut ClassificationContext<'_>) -> Option { + match expr { + Expr::ForLoop { + pattern, + iterable, + guard, + body, + } => Some(classify_for_loop( + ForLoopComponents { + pattern, + iterable: *iterable, + guard: guard.map(|guard| *guard), + body: *body, + }, + ctx, + )), + Expr::Apply { callee, args } => { + if let Some(source) = invocation_aggregation_source(&callee) { + return classify_aggregation_with_tracking(args, source, ctx); + } + Some(RuleBodyTerm::Expression(Expr::Apply { callee, args })) + } + Expr::Call { callee, args } => { + if let Some(source) = invocation_aggregation_source(&callee) { + return classify_aggregation_with_tracking(args, source, ctx); + } + Some(RuleBodyTerm::Expression(Expr::Call { callee, args })) + } + other => Some(RuleBodyTerm::Expression(other)), + } +} + +fn aggregation_source_for(name: &str) -> Option { + match name { + "group_by" => Some(AggregationSource::GroupBy), + "Aggregate" => Some(AggregationSource::LegacyAggregate), + _ => None, + } +} + +fn invocation_aggregation_source(callee: &Expr) -> Option { + if let Expr::Variable(name) = callee { + aggregation_source_for(name.as_str()) + } else { + None + } +} + +fn classify_aggregation_with_tracking( + args: Vec, + source: AggregationSource, + ctx: &mut ClassificationContext<'_>, +) -> Option { + if args.len() != 2 { + ctx.errors + .extend(aggregation_arity_error(ctx.literal_span, source)); + return None; + } + + let mut iter = args.into_iter(); + let first = iter + .next() + .unwrap_or_else(|| unreachable!("len pre-checked as 2; first arg missing")); + let second = iter + .next() + .unwrap_or_else(|| unreachable!("len pre-checked as 2; second arg missing")); + let (project, key) = match source { + AggregationSource::GroupBy => (first, second), + AggregationSource::LegacyAggregate => (second, first), + }; + + let term = RuleBodyTerm::Aggregation(RuleAggregation { + project, + key, + source, + }); + + if validate_aggregation( + &term, + ctx.literal_span, + ctx.first_aggregation_span, + ctx.errors, + ) { + None + } else { + Some(term) + } +} + +fn validate_aggregation( + term: &RuleBodyTerm, + literal_span: &Span, + first_aggregation_span: &mut Option, + errors: &mut Vec>, +) -> bool { + if let RuleBodyTerm::Aggregation(_) = term { + match first_aggregation_span { + Some(first_span) => { + errors.push(multiple_aggregations_error(first_span, literal_span)); + return true; + } + None => { + *first_aggregation_span = Some(literal_span.clone()); + } + } + } + false +} + +fn aggregation_arity_error( + literal_span: &Span, + source: AggregationSource, +) -> Vec> { + vec![Simple::custom( + literal_span.clone(), + format!("{} expects exactly two arguments", source.label()), + )] +} + +struct ForLoopComponents { + pattern: Pattern, + iterable: Expr, + guard: Option, + body: Expr, +} + +fn classify_for_loop( + components: ForLoopComponents, + ctx: &mut ClassificationContext<'_>, +) -> RuleBodyTerm { + let body_terms = classify_for_body_with_aggregation_tracking(components.body, ctx); + + RuleBodyTerm::ForLoop(RuleForLoop { + pattern: components.pattern, + iterable: components.iterable, + guard: components.guard, + body_terms, + }) +} + +fn classify_for_body_with_aggregation_tracking( + body: Expr, + ctx: &mut ClassificationContext<'_>, +) -> Vec { + match body { + Expr::Binary { + op: BinaryOp::Seq, + lhs, + rhs, + } => { + let mut terms = classify_for_body_with_aggregation_tracking(*lhs, ctx); + terms.extend(classify_for_body_with_aggregation_tracking(*rhs, ctx)); + terms + } + Expr::ForLoop { + pattern, + iterable, + guard, + body, + } => { + vec![classify_for_loop( + ForLoopComponents { + pattern, + iterable: *iterable, + guard: guard.map(|guard| *guard), + body: *body, + }, + ctx, + )] + } + Expr::Group(inner) => classify_for_body_with_aggregation_tracking(*inner, ctx), + other => classify_expression(other, ctx).into_iter().collect(), + } +} + +fn multiple_aggregations_error(_first_span: &Span, second_span: &Span) -> Simple { + Simple::custom( + second_span.clone(), + "at most one aggregation (group_by or Aggregate) is permitted per rule body".to_string(), + ) +} diff --git a/src/parser/expression/pratt.rs b/src/parser/expression/pratt.rs index 5872b5e0..a48f346d 100644 --- a/src/parser/expression/pratt.rs +++ b/src/parser/expression/pratt.rs @@ -9,12 +9,14 @@ use std::collections::HashMap; use chumsky::error::Simple; use crate::parser::ast::{Expr, StringLiteral}; -use crate::parser::span_utils::parse_u32_decimal; use crate::{Span, SyntaxKind, tokenize_without_trivia}; -use super::qualified::is_qualified_function_callee; use super::token_stream::TokenStream; +mod delay; +mod diff; +mod postfix; + const MAX_EXPR_DEPTH: usize = 256; pub(super) struct Pratt<'a, I> @@ -185,231 +187,6 @@ where }) } - // Parse highest-precedence postfix operators, delegating each operation to a dedicated helper. - pub(super) fn parse_postfix(&mut self, mut lhs: Expr) -> Option { - let mut pending_diff_span: Option = None; - loop { - match self.ts.peek_kind() { - Some(SyntaxKind::T_APOSTROPHE) => { - self.handle_diff_marker(&mut pending_diff_span)?; - continue; - } - Some(SyntaxKind::T_LPAREN) => { - lhs = self.parse_function_call_postfix(lhs)?; - } - Some(SyntaxKind::T_LBRACKET) => { - lhs = self.parse_bit_slice_postfix(lhs)?; - } - Some(SyntaxKind::T_DOT) => { - lhs = self.parse_dot_access_postfix(lhs)?; - } - Some(SyntaxKind::T_MINUS) if self.ts.peek_nth_kind(1) == Some(SyntaxKind::T_LT) => { - self.validate_diff_not_pending(&mut pending_diff_span); - lhs = self.parse_delay_postfix(lhs)?; - } - _ => break, - } - - lhs = Self::apply_pending_diff(lhs, &mut pending_diff_span); - } - - self.validate_no_pending_diff(&mut pending_diff_span)?; - - Some(lhs) - } - - fn handle_diff_marker(&mut self, pending: &mut Option) -> Option<()> { - let (_, sp) = self.ts.next_tok()?; - if pending.is_some() { - self.ts.push_error(sp, "duplicate diff marker"); - } else { - *pending = Some(sp); - } - - if !matches!( - self.ts.peek_kind(), - Some(SyntaxKind::T_LPAREN | SyntaxKind::T_LBRACKET) - ) { - let diff_span = pending.take().unwrap_or_else(|| self.ts.eof_span()); - self.ts - .push_error(diff_span, "diff marker must precede atom arguments"); - return None; - } - - Some(()) - } - - fn apply_pending_diff(expr: Expr, pending: &mut Option) -> Expr { - if pending.take().is_some() { - Expr::AtomDiff { - expr: Box::new(expr), - } - } else { - expr - } - } - - fn validate_diff_not_pending(&mut self, pending: &mut Option) { - if let Some(diff_span) = pending.take() { - self.ts - .push_error(diff_span, "diff marker must apply to an atom"); - } - } - - fn validate_no_pending_diff(&mut self, pending: &mut Option) -> Option<()> { - if let Some(span) = pending.take() { - self.ts - .push_error(span, "diff marker must be followed by atom arguments"); - return None; - } - Some(()) - } - - fn parse_delay_postfix(&mut self, lhs: Expr) -> Option { - let (minus_kind, _) = self.ts.next_tok()?; // '-' - debug_assert_eq!( - minus_kind, - SyntaxKind::T_MINUS, - "parse_delay_postfix must be called with a leading '-' token" - ); - if !self.ts.expect(SyntaxKind::T_LT) { - return None; - } - let (kind, number_span) = self.ts.next_tok().unwrap_or_else(|| { - let sp = self.ts.eof_span(); - (SyntaxKind::N_ERROR, sp) - }); - if kind != SyntaxKind::T_NUMBER { - self.ts.push_error(number_span, "expected delay value"); - return None; - } - let raw = self.ts.slice(&number_span); - let delay = match parse_u32_decimal(&raw) { - Ok(delay) => delay, - Err(msg) => { - self.ts.push_error(number_span, msg); - return None; - } - }; - if !self.ts.expect(SyntaxKind::T_GT) { - return None; - } - Some(Expr::AtomDelay { - delay, - expr: Box::new(lhs), - }) - } - - fn parse_function_call_postfix(&mut self, lhs: Expr) -> Option { - let args = self.parse_parenthesized_args()?; - if is_qualified_function_callee(&lhs) { - Some(Expr::Call { - callee: Box::new(lhs), - args, - }) - } else { - Some(Expr::Apply { - callee: Box::new(lhs), - args, - }) - } - } - - fn parse_bit_slice_postfix(&mut self, lhs: Expr) -> Option { - self.ts.next_tok(); // opening bracket already peeked - self.with_struct_literals_suspended(move |this| { - let hi = this.parse_expr(0)?; - if !this.ts.expect(SyntaxKind::T_COMMA) { - return None; - } - let lo = this.parse_expr(0)?; - if !this.ts.expect(SyntaxKind::T_RBRACKET) { - return None; - } - Some(Expr::BitSlice { - expr: Box::new(lhs), - hi: Box::new(hi), - lo: Box::new(lo), - }) - }) - } - - fn parse_dot_access_postfix(&mut self, lhs: Expr) -> Option { - self.ts.next_tok(); // '.' - match self.ts.next_tok() { - Some((SyntaxKind::T_IDENT, sp)) => { - let name = self.ts.slice(&sp); - if matches!(self.ts.peek_kind(), Some(SyntaxKind::T_LPAREN)) { - self.parse_method_call(lhs, name) - } else { - Some(Expr::FieldAccess { - expr: Box::new(lhs), - field: name, - }) - } - } - Some((SyntaxKind::T_NUMBER, sp)) => { - let idx = self.ts.slice(&sp); - Some(Expr::TupleIndex { - expr: Box::new(lhs), - index: idx, - }) - } - other => { - let sp = other.map_or_else(|| self.ts.eof_span(), |(_, s)| s); - self.ts - .push_error(sp, "expected identifier or tuple index after '.'"); - None - } - } - } - - fn parse_method_call(&mut self, lhs: Expr, name: String) -> Option { - let args = self.parse_parenthesized_args()?; - Some(Expr::MethodCall { - recv: Box::new(lhs), - name, - args, - }) - } - - fn parse_parenthesized_args(&mut self) -> Option> { - self.ts.next_tok(); // opening parenthesis already peeked - self.with_struct_literals_suspended(|this| { - let args = this.parse_args()?; - if !this.ts.expect(SyntaxKind::T_RPAREN) { - return None; - } - Some(args) - }) - } - - pub(super) fn parse_args(&mut self) -> Option> { - let mut args = Vec::new(); - if matches!(self.ts.peek_kind(), Some(SyntaxKind::T_RPAREN)) { - return Some(args); - } - loop { - if self.ts.peek_kind().is_none() { - self.ts.expect(SyntaxKind::T_RPAREN); - return None; - } - let expr = self.parse_expr(0)?; - args.push(expr); - if !matches!(self.ts.peek_kind(), Some(SyntaxKind::T_COMMA)) { - break; - } - self.ts.next_tok(); - if matches!(self.ts.peek_kind(), Some(SyntaxKind::T_RPAREN)) { - let sp = self.ts.peek_span().unwrap_or_else(|| self.ts.eof_span()); - self.ts - .push_error(sp, "unexpected trailing comma in argument list"); - return None; - } - } - Some(args) - } - /// Parse a type expression following `:` or `as`. /// /// The RHS of ascriptions, and casts, should not consume infix operators diff --git a/src/parser/expression/pratt/delay.rs b/src/parser/expression/pratt/delay.rs new file mode 100644 index 00000000..62dea742 --- /dev/null +++ b/src/parser/expression/pratt/delay.rs @@ -0,0 +1,47 @@ +//! Delay-postfix parsing for the Pratt parser. + +use crate::parser::ast::Expr; +use crate::parser::span_utils::parse_u32_decimal; +use crate::{Span, SyntaxKind}; + +use super::Pratt; + +impl Pratt<'_, I> +where + I: Iterator + Clone, +{ + pub(super) fn parse_delay_postfix(&mut self, lhs: Expr) -> Option { + let (minus_kind, _) = self.ts.next_tok()?; + debug_assert_eq!( + minus_kind, + SyntaxKind::T_MINUS, + "parse_delay_postfix must be called with a leading '-' token" + ); + if !self.ts.expect(SyntaxKind::T_LT) { + return None; + } + let (kind, number_span) = self.ts.next_tok().unwrap_or_else(|| { + let span = self.ts.eof_span(); + (SyntaxKind::N_ERROR, span) + }); + if kind != SyntaxKind::T_NUMBER { + self.ts.push_error(number_span, "expected delay value"); + return None; + } + let raw = self.ts.slice(&number_span); + let delay = match parse_u32_decimal(&raw) { + Ok(delay) => delay, + Err(message) => { + self.ts.push_error(number_span, message); + return None; + } + }; + if !self.ts.expect(SyntaxKind::T_GT) { + return None; + } + Some(Expr::AtomDelay { + delay, + expr: Box::new(lhs), + }) + } +} diff --git a/src/parser/expression/pratt/diff.rs b/src/parser/expression/pratt/diff.rs new file mode 100644 index 00000000..a6e13b95 --- /dev/null +++ b/src/parser/expression/pratt/diff.rs @@ -0,0 +1,58 @@ +//! Diff-marker postfix helpers for the Pratt parser. + +use crate::parser::ast::Expr; +use crate::{Span, SyntaxKind}; + +use super::Pratt; + +impl Pratt<'_, I> +where + I: Iterator + Clone, +{ + pub(super) fn handle_diff_marker(&mut self, pending: &mut Option) -> Option<()> { + let (_, span) = self.ts.next_tok()?; + if pending.is_some() { + self.ts.push_error(span, "duplicate diff marker"); + } else { + *pending = Some(span); + } + + if !matches!( + self.ts.peek_kind(), + Some(SyntaxKind::T_LPAREN | SyntaxKind::T_LBRACKET) + ) { + let diff_span = pending.take().unwrap_or_else(|| self.ts.eof_span()); + self.ts + .push_error(diff_span, "diff marker must precede atom arguments"); + return None; + } + + Some(()) + } + + pub(super) fn apply_pending_diff(expr: Expr, pending: &mut Option) -> Expr { + if pending.take().is_some() { + Expr::AtomDiff { + expr: Box::new(expr), + } + } else { + expr + } + } + + pub(super) fn validate_diff_not_pending(&mut self, pending: &mut Option) { + if let Some(diff_span) = pending.take() { + self.ts + .push_error(diff_span, "diff marker must apply to an atom"); + } + } + + pub(super) fn validate_no_pending_diff(&mut self, pending: &mut Option) -> Option<()> { + if let Some(span) = pending.take() { + self.ts + .push_error(span, "diff marker must be followed by atom arguments"); + return None; + } + Some(()) + } +} diff --git a/src/parser/expression/pratt/postfix.rs b/src/parser/expression/pratt/postfix.rs new file mode 100644 index 00000000..edfba6c1 --- /dev/null +++ b/src/parser/expression/pratt/postfix.rs @@ -0,0 +1,154 @@ +//! Postfix-operator parsing for the Pratt parser. + +use crate::parser::ast::Expr; +use crate::parser::expression::qualified::is_qualified_function_callee; +use crate::{Span, SyntaxKind}; + +use super::Pratt; + +impl Pratt<'_, I> +where + I: Iterator + Clone, +{ + /// Parse highest-precedence postfix operators. + pub(super) fn parse_postfix(&mut self, mut lhs: Expr) -> Option { + let mut pending_diff_span: Option = None; + loop { + match self.ts.peek_kind() { + Some(SyntaxKind::T_APOSTROPHE) => { + self.handle_diff_marker(&mut pending_diff_span)?; + continue; + } + Some(SyntaxKind::T_LPAREN) => { + lhs = self.parse_function_call_postfix(lhs)?; + } + Some(SyntaxKind::T_LBRACKET) => { + lhs = self.parse_bit_slice_postfix(lhs)?; + } + Some(SyntaxKind::T_DOT) => { + lhs = self.parse_dot_access_postfix(lhs)?; + } + Some(SyntaxKind::T_MINUS) if self.ts.peek_nth_kind(1) == Some(SyntaxKind::T_LT) => { + self.validate_diff_not_pending(&mut pending_diff_span); + lhs = self.parse_delay_postfix(lhs)?; + } + _ => break, + } + + lhs = Self::apply_pending_diff(lhs, &mut pending_diff_span); + } + + self.validate_no_pending_diff(&mut pending_diff_span)?; + Some(lhs) + } + + fn parse_function_call_postfix(&mut self, lhs: Expr) -> Option { + let args = self.parse_parenthesized_args()?; + if is_qualified_function_callee(&lhs) { + Some(Expr::Call { + callee: Box::new(lhs), + args, + }) + } else { + Some(Expr::Apply { + callee: Box::new(lhs), + args, + }) + } + } + + fn parse_bit_slice_postfix(&mut self, lhs: Expr) -> Option { + self.ts.next_tok(); + self.with_struct_literals_suspended(move |this| { + let hi = this.parse_expr(0)?; + if !this.ts.expect(SyntaxKind::T_COMMA) { + return None; + } + let lo = this.parse_expr(0)?; + if !this.ts.expect(SyntaxKind::T_RBRACKET) { + return None; + } + Some(Expr::BitSlice { + expr: Box::new(lhs), + hi: Box::new(hi), + lo: Box::new(lo), + }) + }) + } + + fn parse_dot_access_postfix(&mut self, lhs: Expr) -> Option { + self.ts.next_tok(); + match self.ts.next_tok() { + Some((SyntaxKind::T_IDENT, span)) => { + let name = self.ts.slice(&span); + if matches!(self.ts.peek_kind(), Some(SyntaxKind::T_LPAREN)) { + self.parse_method_call(lhs, name) + } else { + Some(Expr::FieldAccess { + expr: Box::new(lhs), + field: name, + }) + } + } + Some((SyntaxKind::T_NUMBER, span)) => { + let index = self.ts.slice(&span); + Some(Expr::TupleIndex { + expr: Box::new(lhs), + index, + }) + } + other => { + let span = other.map_or_else(|| self.ts.eof_span(), |(_, span)| span); + self.ts + .push_error(span, "expected identifier or tuple index after '.'"); + None + } + } + } + + fn parse_method_call(&mut self, lhs: Expr, name: String) -> Option { + let args = self.parse_parenthesized_args()?; + Some(Expr::MethodCall { + recv: Box::new(lhs), + name, + args, + }) + } + + fn parse_parenthesized_args(&mut self) -> Option> { + self.ts.next_tok(); + self.with_struct_literals_suspended(|this| { + let args = this.parse_args()?; + if !this.ts.expect(SyntaxKind::T_RPAREN) { + return None; + } + Some(args) + }) + } + + pub(super) fn parse_args(&mut self) -> Option> { + let mut args = Vec::new(); + if matches!(self.ts.peek_kind(), Some(SyntaxKind::T_RPAREN)) { + return Some(args); + } + loop { + if self.ts.peek_kind().is_none() { + self.ts.expect(SyntaxKind::T_RPAREN); + return None; + } + let expr = self.parse_expr(0)?; + args.push(expr); + if !matches!(self.ts.peek_kind(), Some(SyntaxKind::T_COMMA)) { + break; + } + self.ts.next_tok(); + if matches!(self.ts.peek_kind(), Some(SyntaxKind::T_RPAREN)) { + let span = self.ts.peek_span().unwrap_or_else(|| self.ts.eof_span()); + self.ts + .push_error(span, "unexpected trailing comma in argument list"); + return None; + } + } + Some(args) + } +} diff --git a/src/parser/tests/expression.rs b/src/parser/tests/expression.rs index 4b04f130..c68732c0 100644 --- a/src/parser/tests/expression.rs +++ b/src/parser/tests/expression.rs @@ -1,267 +1,44 @@ //! Tests for the Pratt expression parser. -use crate::parser::ast::{BinaryOp, Expr}; -use crate::parser::expression::parse_expression; -use crate::test_util::{ - assert_parse_error, assert_unclosed_delimiter_error, bit_slice, call, call_expr, closure, - field, field_access, for_loop, if_expr, lit_bool, lit_num, lit_str, map_entry, map_lit, - match_arm, match_expr, method_call, qualified_call, struct_expr, tuple, tuple_index, var, - vec_lit, +mod fixtures; + +use fixtures::{ + error_cases, expression_cases, invalid_for_loop_sources, literal_cases, + match_pattern_error_cases, postfix_error_cases, }; -use rstest::rstest; -#[rstest] -#[case("x", var("x"))] -#[case("foo()", call("foo", vec![]))] -#[case("pkg::Foo()", call("pkg::Foo", vec![]))] -#[case("pkg::foo()", qualified_call("pkg::foo", vec![]))] -#[case("add(x, 1)", call("add", vec![var("x"), lit_num("1")]))] -#[case("(1, 2)", tuple(vec![lit_num("1"), lit_num("2")]))] -#[case("(1, 2, 3)", tuple(vec![lit_num("1"), lit_num("2"), lit_num("3")]))] -#[case("(1,)", tuple(vec![lit_num("1")]))] -#[case("()", tuple(vec![]))] -#[case("(1)", Expr::Group(Box::new(lit_num("1"))))] -#[case("[]", vec_lit(vec![]))] -#[case("[1]", vec_lit(vec![lit_num("1")]))] -#[case("[1, 2]", vec_lit(vec![lit_num("1"), lit_num("2")]))] -#[case("[1, 2, 3]", vec_lit(vec![lit_num("1"), lit_num("2"), lit_num("3")]))] -#[case("[1,]", vec_lit(vec![lit_num("1")]))] -#[case("[1, 2,]", vec_lit(vec![lit_num("1"), lit_num("2")]))] -#[case("[x, y + 1]", vec_lit(vec![var("x"), Expr::Binary { op: BinaryOp::Add, lhs: Box::new(var("y")), rhs: Box::new(lit_num("1")) }]))] -#[case("{}", map_lit(vec![]))] -#[case("{a: 1}", map_lit(vec![map_entry(var("a"), lit_num("1"))]))] -#[case("{a: 1, b: 2}", map_lit(vec![map_entry(var("a"), lit_num("1")), map_entry(var("b"), lit_num("2"))]))] -#[case("{a: 1,}", map_lit(vec![map_entry(var("a"), lit_num("1"))]))] -#[case("{x: y, z: w}", map_lit(vec![map_entry(var("x"), var("y")), map_entry(var("z"), var("w"))]))] -#[case("Point { x: 1, y: 2 }", struct_expr("Point", vec![field("x", lit_num("1")), field("y", lit_num("2"))]))] -#[case("Point {}", struct_expr("Point", vec![]))] -#[case( - "Point { x: 1, y: 2, }", - struct_expr("Point", vec![field("x", lit_num("1")), field("y", lit_num("2"))]), -)] -#[case("|x, y| x + y", closure(vec!["x", "y"], Expr::Binary { op: BinaryOp::Add, lhs: Box::new(var("x")), rhs: Box::new(var("y")) }))] -#[case("|| 1", closure(std::iter::empty::<&str>(), lit_num("1")))] -#[case("|x,| x", closure(vec!["x"], var("x")))] -#[case("Point { pair: (1, 2) }", struct_expr("Point", vec![field("pair", tuple(vec![lit_num("1"), lit_num("2")]))]))] -#[case( - "(|| 1, |x| x)", - tuple(vec![ - closure(Vec::<&str>::new(), lit_num("1")), - closure(vec!["x"], var("x")), - ]), -)] -#[case("|x| Point { x: x }", closure(vec!["x"], struct_expr("Point", vec![field("x", var("x"))])))] -#[case("(f)(x)", call_expr(Expr::Group(Box::new(var("f"))), vec![var("x")]))] -#[case("foo.bar(x)", method_call(var("foo"), "bar", vec![var("x")]))] -#[case("foo.bar(x).baz", field_access( - method_call(var("foo"), "bar", vec![var("x")]), - "baz" -))] -#[case("foo.bar", field_access(var("foo"), "bar"))] -#[case("foo.bar.baz(x)", method_call( - field_access(var("foo"), "bar"), - "baz", - vec![var("x")] -))] -#[case("foo.bar.baz().qux", field_access( - method_call(field_access(var("foo"), "bar"), "baz", vec![]), - "qux" -))] -#[case("foo.bar().baz(x)", method_call( - method_call(var("foo"), "bar", vec![]), - "baz", - vec![var("x")] -))] -#[case("foo.bar.baz", field_access(field_access(var("foo"), "bar"), "baz"))] -#[case("foo.bar().baz.qux(x)", method_call( - field_access( - method_call(var("foo"), "bar", vec![]), - "baz" - ), - "qux", - vec![var("x")] -))] -#[case("e[1,0]", bit_slice(var("e"), lit_num("1"), lit_num("0")))] -#[case("t.0", tuple_index(var("t"), "0"))] -#[case( - "if x { y } else { z }", - if_expr( - var("x"), - Expr::Group(Box::new(var("y"))), - Some(Expr::Group(Box::new(var("z")))), - ) -)] -#[case( - "if (Point { x: 1 }) { y } else { z }", - if_expr( - Expr::Group(Box::new(struct_expr( - "Point", - vec![field("x", lit_num("1"))], - ))), - Expr::Group(Box::new(var("y"))), - Some(Expr::Group(Box::new(var("z")))), - ) -)] -#[case( - "if flag { Point { x: 1 } } else { z }", - if_expr( - var("flag"), - Expr::Group(Box::new(struct_expr( - "Point", - vec![field("x", lit_num("1"))], - ))), - Some(Expr::Group(Box::new(var("z")))), - ) -)] -#[case( - "if cond { Outer { inner: Inner { a: 1, b: 2 }, flag: true } } else { fallback }", - if_expr( - var("cond"), - Expr::Group(Box::new(struct_expr( - "Outer", - vec![ - field( - "inner", - struct_expr( - "Inner", - vec![ - field("a", lit_num("1")), - field("b", lit_num("2")), - ], - ), - ), - field("flag", lit_bool(true)), - ], - ))), - Some(Expr::Group(Box::new(var("fallback")))), - ) -)] -#[case( - "if ok { S { f: T { x: 1, y: U { z: 2 } }, g: 3 } } else { alt }", - if_expr( - var("ok"), - Expr::Group(Box::new(struct_expr( - "S", - vec![ - field( - "f", - struct_expr( - "T", - vec![ - field("x", lit_num("1")), - field( - "y", - struct_expr( - "U", - vec![field("z", lit_num("2"))], - ), - ), - ], - ), - ), - field("g", lit_num("3")), - ], - ))), - Some(Expr::Group(Box::new(var("alt")))), - ) -)] -#[case( - "if a and b { x } else { y }", - if_expr( - Expr::Binary { - op: BinaryOp::And, - lhs: Box::new(var("a")), - rhs: Box::new(var("b")), - }, - Expr::Group(Box::new(var("x"))), - Some(Expr::Group(Box::new(var("y")))), - ) -)] -#[case("if flag value", if_expr(var("flag"), var("value"), None))] -#[case( - "if cond { left } else if other { mid } else { right }", - if_expr( - var("cond"), - Expr::Group(Box::new(var("left"))), - Some(if_expr( - var("other"), - Expr::Group(Box::new(var("mid"))), - Some(Expr::Group(Box::new(var("right")))), - )), - ) -)] -#[case( - "match (flag) { true -> false, false -> true }", - match_expr( - var("flag"), - vec![ - match_arm("true", lit_bool(false)), - match_arm("false", lit_bool(true)), - ], - ) -)] -#[case( - "match (value) { Point { field: (x, y) } -> x, _ -> 0, }", - match_expr( - var("value"), - vec![ - match_arm("Point { field: (x, y) }", var("x")), - match_arm("_", lit_num("0")), - ], - ) -)] -#[case( - "match (x) { _ -> x, }", - match_expr( - var("x"), - vec![ - match_arm("_", var("x")), - ], - ) -)] -#[case( - "for (item in items) item", - for_loop("item", var("items"), None, var("item")) -)] -#[case( - "for (entry in items if entry.active) process(entry)", - for_loop( - "entry", - var("items"), - Some(field_access(var("entry"), "active")), - call("process", vec![var("entry")]), - ), -)] -fn parses_expressions(#[case] src: &str, #[case] expected: Expr) { - let expr = - parse_expression(src).unwrap_or_else(|errs| panic!("source {src:?} errors: {errs:?}")); - assert_eq!(expr, expected); +use crate::parser::expression::parse_expression; +use crate::test_util::{assert_parse_error, assert_unclosed_delimiter_error}; + +#[test] +fn parses_expressions() { + for case in expression_cases() { + let expr = parse_expression(case.src) + .unwrap_or_else(|errs| panic!("source {:?} errors: {errs:?}", case.src)); + assert_eq!(expr, case.expected, "source {:?}", case.src); + } } -#[rstest] -#[case("for item in items) item")] -#[case("for (in items) item")] -#[case("for (item items) item")] -#[case("for (item in items if) item")] -fn rejects_invalid_for_loop_syntax(#[case] src: &str) { - let Err(errors) = parse_expression(src) else { - panic!("expected parse failure for {src}"); - }; - assert!( - !errors.is_empty(), - "parser returned Err but without diagnostics for {src}" - ); +#[test] +fn rejects_invalid_for_loop_syntax() { + for src in invalid_for_loop_sources() { + let Err(errors) = parse_expression(src) else { + panic!("expected parse failure for {src}"); + }; + assert!( + !errors.is_empty(), + "parser returned Err but without diagnostics for {src}" + ); + } } -#[rstest] -#[case("\"hi\"", lit_str("hi"))] -#[case("true", lit_bool(true))] -#[case("false", lit_bool(false))] -#[case("42", lit_num("42"))] -fn parses_literals(#[case] src: &str, #[case] expected: Expr) { - let expr = - parse_expression(src).unwrap_or_else(|errs| panic!("source {src:?} errors: {errs:?}")); - assert_eq!(expr, expected); +#[test] +fn parses_literals() { + for case in literal_cases() { + let expr = parse_expression(case.src) + .unwrap_or_else(|errs| panic!("source {:?} errors: {errs:?}", case.src)); + assert_eq!(expr, case.expected, "source {:?}", case.src); + } } #[test] @@ -287,80 +64,38 @@ fn reports_struct_literal_disallowed_in_if_condition() { ); } -#[rstest] -#[case::addition_missing_rhs("1 +", 1)] -#[case::unclosed_parenthesis("(1 + 2", 1)] -#[case::unexpected_question_mark("1 ? 2", 1)] -#[case::trailing_colon("x :", 1)] -#[case::incomplete_as_cast("x as", 1)] -#[case::assignment_missing_rhs("x =", 1)] -#[case::extraneous_semicolon("x ;", 1)] -#[case::unexpected_fat_arrow("x =>", 1)] -#[case::empty_input("", 1)] -#[case::bit_slice_missing_comma("e[1 0]", 1)] -#[case::bit_slice_missing_lhs("e[,0]", 1)] -#[case::bit_slice_missing_rhs("e[1,]", 1)] -#[case::bit_slice_extra_comma("e[1,,0]", 1)] -#[case::tuple_index_missing_digits("t.", 1)] -#[case::tuple_index_negative("t.-1", 1)] -#[case::tuple_index_plus("t.+1", 1)] -#[case::tuple_index_double_dot("t..0", 1)] -#[case::if_missing_then("if cond else value", 1)] -#[case::if_missing_else_expr("if cond value else", 1)] -#[case::if_missing_condition("if", 1)] -#[case::match_missing_arrow("match (x) { _ => x }", 1)] -#[case::match_missing_arm("match (x) {}", 1)] -#[case::match_missing_paren("match x { _ -> x }", 1)] -#[case::match_unmatched_paren("match (x) { ) -> x }", 1)] -#[case::match_unmatched_brace("match (x) { } -> x }", 1)] -#[case::match_unmatched_bracket("match (x) { ] -> x }", 1)] -#[case::vector_unclosed("[1, 2", 1)] -#[case::map_unclosed("{a: 1", 1)] -#[case::map_comma_without_colon("{a, b}", 1)] -fn reports_errors(#[case] src: &str, #[case] min_errs: usize) { - match parse_expression(src) { - Ok(_) => panic!("expected parse error"), - Err(errs) => assert!(errs.len() >= min_errs), +#[test] +fn reports_errors() { + for case in error_cases() { + match parse_expression(case.src) { + Ok(_) => panic!("expected parse error"), + Err(errs) => assert!( + errs.len() >= case.min_errs, + "source {:?} produced too few errors: {errs:?}", + case.src + ), + } } } -#[rstest] -#[case( - "match (x) { ) -> x }", - "unmatched closing parenthesis in match pattern", - 12, - 13 -)] -#[case( - "match (x) { ( } -> x }", - "unmatched closing brace in match pattern", - 14, - 15 -)] -#[case( - "match (x) { ] -> x }", - "unmatched closing bracket in match pattern", - 12, - 13 -)] -fn match_pattern_delimiter_errors( - #[case] src: &str, - #[case] msg: &str, - #[case] start: usize, - #[case] end: usize, -) { - let Err(errors) = parse_expression(src) else { - panic!("expected error"); - }; - let Some(first) = errors.first() else { - panic!("error missing"); - }; - let rendered = format!("{first:?}"); - assert!( - rendered.contains(msg), - "expected error to contain pattern '{msg}', got '{rendered}'", - ); - assert_eq!(first.span(), start..end); +#[test] +fn match_pattern_delimiter_errors() { + for case in match_pattern_error_cases() { + let Err(errors) = parse_expression(case.src) else { + panic!("expected error"); + }; + let Some(first) = errors.first() else { + panic!("error missing"); + }; + let rendered = format!("{first:?}"); + assert!( + rendered.contains(case.msg), + "expected error to contain pattern '{}', got '{}'", + case.msg, + rendered, + ); + assert_eq!(first.span(), case.start..case.end); + } } fn reports_exactly_one_error(src: &str) { @@ -377,27 +112,16 @@ fn rejects_chained_type_ops_with_single_diag() { } } -#[rstest] -#[case::trailing_dot("foo.", "expected identifier or tuple index after '.'", 4, 4, false)] -#[case::bit_slice_missing_comma("e[1]", "expected comma", 3, 4, false)] -#[case::bit_slice_unclosed("e[1,0", "expected right bracket", 5, 5, true)] -#[case::vector_unclosed("[1, 2", "expected right bracket", 5, 5, true)] -#[case::map_unclosed("{a: 1", "expected right brace", 5, 5, true)] -#[case::map_comma_without_colon("{a, b}", "expected ':'", 2, 3, false)] -#[case::scoped_identifier_missing_segment("pkg::()", "expected identifier after '::'", 5, 6, false)] -fn postfix_expression_errors( - #[case] src: &str, - #[case] msg: &str, - #[case] start: usize, - #[case] end: usize, - #[case] unclosed: bool, -) { - let Err(errors) = parse_expression(src) else { - panic!("expected error"); - }; - if unclosed { - assert_unclosed_delimiter_error(&errors, msg, start, end); - } else { - assert_parse_error(&errors, msg, start, end); +#[test] +fn postfix_expression_errors() { + for case in postfix_error_cases() { + let Err(errors) = parse_expression(case.src) else { + panic!("expected error"); + }; + if case.unclosed { + assert_unclosed_delimiter_error(&errors, case.msg, case.start, case.end); + } else { + assert_parse_error(&errors, case.msg, case.start, case.end); + } } } diff --git a/src/parser/tests/expression/fixtures.rs b/src/parser/tests/expression/fixtures.rs new file mode 100644 index 00000000..aaed9d57 --- /dev/null +++ b/src/parser/tests/expression/fixtures.rs @@ -0,0 +1,55 @@ +//! Shared fixture data for expression parser tests. + +mod errors; +mod valid; + +use crate::parser::ast::Expr; + +pub(super) struct ExpressionCase { + pub(super) src: &'static str, + pub(super) expected: Expr, +} + +pub(super) struct CountedErrorCase { + pub(super) src: &'static str, + pub(super) min_errs: usize, +} + +pub(super) struct SpannedErrorCase { + pub(super) src: &'static str, + pub(super) msg: &'static str, + pub(super) start: usize, + pub(super) end: usize, +} + +pub(super) struct PostfixErrorCase { + pub(super) src: &'static str, + pub(super) msg: &'static str, + pub(super) start: usize, + pub(super) end: usize, + pub(super) unclosed: bool, +} + +pub(super) fn expression_cases() -> Vec { + valid::expression_cases() +} + +pub(super) fn invalid_for_loop_sources() -> [&'static str; 4] { + valid::invalid_for_loop_sources() +} + +pub(super) fn literal_cases() -> Vec { + valid::literal_cases() +} + +pub(super) fn error_cases() -> Vec { + errors::error_cases() +} + +pub(super) fn match_pattern_error_cases() -> Vec { + errors::match_pattern_error_cases() +} + +pub(super) fn postfix_error_cases() -> Vec { + errors::postfix_error_cases() +} diff --git a/src/parser/tests/expression/fixtures/errors.rs b/src/parser/tests/expression/fixtures/errors.rs new file mode 100644 index 00000000..44b00993 --- /dev/null +++ b/src/parser/tests/expression/fixtures/errors.rs @@ -0,0 +1,232 @@ +//! Error-case fixtures for expression parser tests. + +use crate::parser::tests::expression::fixtures::{ + CountedErrorCase, PostfixErrorCase, SpannedErrorCase, +}; + +pub(super) fn error_cases() -> Vec { + let mut cases = operator_error_cases(); + cases.extend(postfix_counted_error_cases()); + cases.extend(control_flow_error_cases()); + cases.extend(match_error_cases()); + cases.extend(collection_error_cases()); + cases +} + +pub(super) fn match_pattern_error_cases() -> Vec { + vec![ + SpannedErrorCase { + src: "match (x) { ) -> x }", + msg: "unmatched closing parenthesis in match pattern", + start: 12, + end: 13, + }, + SpannedErrorCase { + src: "match (x) { ( } -> x }", + msg: "unmatched closing brace in match pattern", + start: 14, + end: 15, + }, + SpannedErrorCase { + src: "match (x) { ] -> x }", + msg: "unmatched closing bracket in match pattern", + start: 12, + end: 13, + }, + ] +} + +pub(super) fn postfix_error_cases() -> Vec { + vec![ + PostfixErrorCase { + src: "foo.", + msg: "expected identifier or tuple index after '.'", + start: 4, + end: 4, + unclosed: false, + }, + PostfixErrorCase { + src: "e[1]", + msg: "expected comma", + start: 3, + end: 4, + unclosed: false, + }, + PostfixErrorCase { + src: "e[1,0", + msg: "expected right bracket", + start: 5, + end: 5, + unclosed: true, + }, + PostfixErrorCase { + src: "[1, 2", + msg: "expected right bracket", + start: 5, + end: 5, + unclosed: true, + }, + PostfixErrorCase { + src: "{a: 1", + msg: "expected right brace", + start: 5, + end: 5, + unclosed: true, + }, + PostfixErrorCase { + src: "{a, b}", + msg: "expected ':'", + start: 2, + end: 3, + unclosed: false, + }, + PostfixErrorCase { + src: "pkg::()", + msg: "expected identifier after '::'", + start: 5, + end: 6, + unclosed: false, + }, + ] +} + +fn operator_error_cases() -> Vec { + vec![ + CountedErrorCase { + src: "1 +", + min_errs: 1, + }, + CountedErrorCase { + src: "(1 + 2", + min_errs: 1, + }, + CountedErrorCase { + src: "1 ? 2", + min_errs: 1, + }, + CountedErrorCase { + src: "x :", + min_errs: 1, + }, + CountedErrorCase { + src: "x as", + min_errs: 1, + }, + CountedErrorCase { + src: "x =", + min_errs: 1, + }, + CountedErrorCase { + src: "x ;", + min_errs: 1, + }, + CountedErrorCase { + src: "x =>", + min_errs: 1, + }, + CountedErrorCase { + src: "", + min_errs: 1, + }, + ] +} + +fn postfix_counted_error_cases() -> Vec { + vec![ + CountedErrorCase { + src: "e[1 0]", + min_errs: 1, + }, + CountedErrorCase { + src: "e[,0]", + min_errs: 1, + }, + CountedErrorCase { + src: "e[1,]", + min_errs: 1, + }, + CountedErrorCase { + src: "e[1,,0]", + min_errs: 1, + }, + CountedErrorCase { + src: "t.", + min_errs: 1, + }, + CountedErrorCase { + src: "t.-1", + min_errs: 1, + }, + CountedErrorCase { + src: "t.+1", + min_errs: 1, + }, + CountedErrorCase { + src: "t..0", + min_errs: 1, + }, + ] +} + +fn control_flow_error_cases() -> Vec { + vec![ + CountedErrorCase { + src: "if cond else value", + min_errs: 1, + }, + CountedErrorCase { + src: "if cond value else", + min_errs: 1, + }, + CountedErrorCase { + src: "if", + min_errs: 1, + }, + ] +} + +fn match_error_cases() -> Vec { + vec![ + CountedErrorCase { + src: "match (x) { _ => x }", + min_errs: 1, + }, + CountedErrorCase { + src: "match (x) {}", + min_errs: 1, + }, + CountedErrorCase { + src: "match x { _ -> x }", + min_errs: 1, + }, + CountedErrorCase { + src: "match (x) { ) -> x }", + min_errs: 1, + }, + CountedErrorCase { + src: "match (x) { } -> x }", + min_errs: 1, + }, + CountedErrorCase { + src: "match (x) { ] -> x }", + min_errs: 1, + }, + ] +} + +fn collection_error_cases() -> Vec { + vec![ + CountedErrorCase { + src: "[1, 2", + min_errs: 1, + }, + CountedErrorCase { + src: "{a: 1", + min_errs: 1, + }, + CountedErrorCase { + src: "{a, b}", + min_errs: 1, + }, + ] +} diff --git a/src/parser/tests/expression/fixtures/valid.rs b/src/parser/tests/expression/fixtures/valid.rs new file mode 100644 index 00000000..578b9070 --- /dev/null +++ b/src/parser/tests/expression/fixtures/valid.rs @@ -0,0 +1,26 @@ +//! Valid expression fixtures for parser tests. + +mod basic; +mod collections; +mod control_flow; +mod postfix; +mod structured; + +use crate::parser::tests::expression::fixtures::ExpressionCase; + +pub(super) fn expression_cases() -> Vec { + let mut cases = basic::basic_expression_cases(); + cases.extend(collections::collection_expression_cases()); + cases.extend(structured::struct_and_closure_cases()); + cases.extend(postfix::postfix_expression_cases()); + cases.extend(control_flow::control_flow_expression_cases()); + cases +} + +pub(super) fn invalid_for_loop_sources() -> [&'static str; 4] { + basic::invalid_for_loop_sources() +} + +pub(super) fn literal_cases() -> Vec { + basic::literal_cases() +} diff --git a/src/parser/tests/expression/fixtures/valid/basic.rs b/src/parser/tests/expression/fixtures/valid/basic.rs new file mode 100644 index 00000000..618d46d3 --- /dev/null +++ b/src/parser/tests/expression/fixtures/valid/basic.rs @@ -0,0 +1,80 @@ +//! Basic expression fixtures that do not need specialised helper groupings. + +use crate::parser::ast::Expr; +use crate::parser::tests::expression::fixtures::ExpressionCase; +use crate::test_util::{call, lit_bool, lit_num, lit_str, qualified_call, tuple, var}; + +pub(super) fn basic_expression_cases() -> Vec { + vec![ + ExpressionCase { + src: "x", + expected: var("x"), + }, + ExpressionCase { + src: "foo()", + expected: call("foo", vec![]), + }, + ExpressionCase { + src: "pkg::Foo()", + expected: call("pkg::Foo", vec![]), + }, + ExpressionCase { + src: "pkg::foo()", + expected: qualified_call("pkg::foo", vec![]), + }, + ExpressionCase { + src: "add(x, 1)", + expected: call("add", vec![var("x"), lit_num("1")]), + }, + ExpressionCase { + src: "(1, 2)", + expected: tuple(vec![lit_num("1"), lit_num("2")]), + }, + ExpressionCase { + src: "(1, 2, 3)", + expected: tuple(vec![lit_num("1"), lit_num("2"), lit_num("3")]), + }, + ExpressionCase { + src: "(1,)", + expected: tuple(vec![lit_num("1")]), + }, + ExpressionCase { + src: "()", + expected: tuple(vec![]), + }, + ExpressionCase { + src: "(1)", + expected: Expr::Group(Box::new(lit_num("1"))), + }, + ] +} + +pub(super) fn invalid_for_loop_sources() -> [&'static str; 4] { + [ + "for item in items) item", + "for (in items) item", + "for (item items) item", + "for (item in items if) item", + ] +} + +pub(super) fn literal_cases() -> Vec { + vec![ + ExpressionCase { + src: "\"hi\"", + expected: lit_str("hi"), + }, + ExpressionCase { + src: "true", + expected: lit_bool(true), + }, + ExpressionCase { + src: "false", + expected: lit_bool(false), + }, + ExpressionCase { + src: "42", + expected: lit_num("42"), + }, + ] +} diff --git a/src/parser/tests/expression/fixtures/valid/collections.rs b/src/parser/tests/expression/fixtures/valid/collections.rs new file mode 100644 index 00000000..02ff33ce --- /dev/null +++ b/src/parser/tests/expression/fixtures/valid/collections.rs @@ -0,0 +1,71 @@ +//! Collection literal fixtures for expression parser tests. + +use crate::parser::ast::{BinaryOp, Expr}; +use crate::parser::tests::expression::fixtures::ExpressionCase; +use crate::test_util::{lit_num, map_entry, map_lit, var, vec_lit}; + +pub(super) fn collection_expression_cases() -> Vec { + vec![ + ExpressionCase { + src: "[]", + expected: vec_lit(vec![]), + }, + ExpressionCase { + src: "[1]", + expected: vec_lit(vec![lit_num("1")]), + }, + ExpressionCase { + src: "[1, 2]", + expected: vec_lit(vec![lit_num("1"), lit_num("2")]), + }, + ExpressionCase { + src: "[1, 2, 3]", + expected: vec_lit(vec![lit_num("1"), lit_num("2"), lit_num("3")]), + }, + ExpressionCase { + src: "[1,]", + expected: vec_lit(vec![lit_num("1")]), + }, + ExpressionCase { + src: "[1, 2,]", + expected: vec_lit(vec![lit_num("1"), lit_num("2")]), + }, + ExpressionCase { + src: "[x, y + 1]", + expected: vec_lit(vec![ + var("x"), + Expr::Binary { + op: BinaryOp::Add, + lhs: Box::new(var("y")), + rhs: Box::new(lit_num("1")), + }, + ]), + }, + ExpressionCase { + src: "{}", + expected: map_lit(vec![]), + }, + ExpressionCase { + src: "{a: 1}", + expected: map_lit(vec![map_entry(var("a"), lit_num("1"))]), + }, + ExpressionCase { + src: "{a: 1, b: 2}", + expected: map_lit(vec![ + map_entry(var("a"), lit_num("1")), + map_entry(var("b"), lit_num("2")), + ]), + }, + ExpressionCase { + src: "{a: 1,}", + expected: map_lit(vec![map_entry(var("a"), lit_num("1"))]), + }, + ExpressionCase { + src: "{x: y, z: w}", + expected: map_lit(vec![ + map_entry(var("x"), var("y")), + map_entry(var("z"), var("w")), + ]), + }, + ] +} diff --git a/src/parser/tests/expression/fixtures/valid/control_flow.rs b/src/parser/tests/expression/fixtures/valid/control_flow.rs new file mode 100644 index 00000000..55ce6853 --- /dev/null +++ b/src/parser/tests/expression/fixtures/valid/control_flow.rs @@ -0,0 +1,173 @@ +//! Control-flow expression fixtures for parser tests. + +use crate::parser::ast::{BinaryOp, Expr}; +use crate::parser::tests::expression::fixtures::ExpressionCase; +use crate::test_util::{ + call, field, field_access, for_loop, if_expr, lit_bool, lit_num, match_arm, match_expr, + struct_expr, var, +}; + +pub(super) fn control_flow_expression_cases() -> Vec { + let mut cases = if_expression_cases(); + cases.extend(match_and_for_expression_cases()); + cases +} + +fn if_expression_cases() -> Vec { + let mut cases = basic_if_expression_cases(); + cases.extend(struct_if_expression_cases()); + cases +} + +fn basic_if_expression_cases() -> Vec { + vec![ + ExpressionCase { + src: "if x { y } else { z }", + expected: if_expr( + var("x"), + Expr::Group(Box::new(var("y"))), + Some(Expr::Group(Box::new(var("z")))), + ), + }, + ExpressionCase { + src: "if (Point { x: 1 }) { y } else { z }", + expected: if_expr( + Expr::Group(Box::new(struct_expr( + "Point", + vec![field("x", lit_num("1"))], + ))), + Expr::Group(Box::new(var("y"))), + Some(Expr::Group(Box::new(var("z")))), + ), + }, + ExpressionCase { + src: "if flag { Point { x: 1 } } else { z }", + expected: if_expr( + var("flag"), + Expr::Group(Box::new(struct_expr( + "Point", + vec![field("x", lit_num("1"))], + ))), + Some(Expr::Group(Box::new(var("z")))), + ), + }, + ExpressionCase { + src: "if a and b { x } else { y }", + expected: if_expr( + Expr::Binary { + op: BinaryOp::And, + lhs: Box::new(var("a")), + rhs: Box::new(var("b")), + }, + Expr::Group(Box::new(var("x"))), + Some(Expr::Group(Box::new(var("y")))), + ), + }, + ExpressionCase { + src: "if flag value", + expected: if_expr(var("flag"), var("value"), None), + }, + ExpressionCase { + src: "if cond { left } else if other { mid } else { right }", + expected: if_expr( + var("cond"), + Expr::Group(Box::new(var("left"))), + Some(if_expr( + var("other"), + Expr::Group(Box::new(var("mid"))), + Some(Expr::Group(Box::new(var("right")))), + )), + ), + }, + ] +} + +fn struct_if_expression_cases() -> Vec { + vec![ + ExpressionCase { + src: "if cond { Outer { inner: Inner { a: 1, b: 2 }, flag: true } } else { fallback }", + expected: if_expr( + var("cond"), + Expr::Group(Box::new(struct_expr( + "Outer", + vec![ + field( + "inner", + struct_expr( + "Inner", + vec![field("a", lit_num("1")), field("b", lit_num("2"))], + ), + ), + field("flag", lit_bool(true)), + ], + ))), + Some(Expr::Group(Box::new(var("fallback")))), + ), + }, + ExpressionCase { + src: "if ok { S { f: T { x: 1, y: U { z: 2 } }, g: 3 } } else { alt }", + expected: if_expr( + var("ok"), + Expr::Group(Box::new(struct_expr( + "S", + vec![ + field( + "f", + struct_expr( + "T", + vec![ + field("x", lit_num("1")), + field("y", struct_expr("U", vec![field("z", lit_num("2"))])), + ], + ), + ), + field("g", lit_num("3")), + ], + ))), + Some(Expr::Group(Box::new(var("alt")))), + ), + }, + ] +} + +fn match_and_for_expression_cases() -> Vec { + vec![ + ExpressionCase { + src: "match (flag) { true -> false, false -> true }", + expected: match_expr( + var("flag"), + vec![ + match_arm("true", lit_bool(false)), + match_arm("false", lit_bool(true)), + ], + ), + }, + ExpressionCase { + src: "match (value) { Point { field: (x, y) } -> x, _ -> 0, }", + expected: match_expr( + var("value"), + vec![ + match_arm("Point { field: (x, y) }", var("x")), + match_arm("_", lit_num("0")), + ], + ), + }, + ExpressionCase { + src: "match (x) { _ -> x, }", + expected: match_expr(var("x"), vec![match_arm("_", var("x"))]), + }, + ExpressionCase { + src: "for (item in items) item", + expected: for_loop("item", var("items"), None, var("item")), + }, + ExpressionCase { + src: "for (entry in items if entry.active) process(entry)", + expected: for_loop( + "entry", + var("items"), + Some(field_access(var("entry"), "active")), + call("process", vec![var("entry")]), + ), + }, + ] +} diff --git a/src/parser/tests/expression/fixtures/valid/postfix.rs b/src/parser/tests/expression/fixtures/valid/postfix.rs new file mode 100644 index 00000000..5022111d --- /dev/null +++ b/src/parser/tests/expression/fixtures/valid/postfix.rs @@ -0,0 +1,67 @@ +//! Postfix-expression fixtures for parser tests. + +use crate::parser::ast::Expr; +use crate::parser::tests::expression::fixtures::ExpressionCase; +use crate::test_util::{ + bit_slice, call_expr, field_access, lit_num, method_call, tuple_index, var, +}; + +pub(super) fn postfix_expression_cases() -> Vec { + vec![ + ExpressionCase { + src: "(f)(x)", + expected: call_expr(Expr::Group(Box::new(var("f"))), vec![var("x")]), + }, + ExpressionCase { + src: "foo.bar(x)", + expected: method_call(var("foo"), "bar", vec![var("x")]), + }, + ExpressionCase { + src: "foo.bar(x).baz", + expected: field_access(method_call(var("foo"), "bar", vec![var("x")]), "baz"), + }, + ExpressionCase { + src: "foo.bar", + expected: field_access(var("foo"), "bar"), + }, + ExpressionCase { + src: "foo.bar.baz(x)", + expected: method_call(field_access(var("foo"), "bar"), "baz", vec![var("x")]), + }, + ExpressionCase { + src: "foo.bar.baz().qux", + expected: field_access( + method_call(field_access(var("foo"), "bar"), "baz", vec![]), + "qux", + ), + }, + ExpressionCase { + src: "foo.bar().baz(x)", + expected: method_call( + method_call(var("foo"), "bar", vec![]), + "baz", + vec![var("x")], + ), + }, + ExpressionCase { + src: "foo.bar.baz", + expected: field_access(field_access(var("foo"), "bar"), "baz"), + }, + ExpressionCase { + src: "foo.bar().baz.qux(x)", + expected: method_call( + field_access(method_call(var("foo"), "bar", vec![]), "baz"), + "qux", + vec![var("x")], + ), + }, + ExpressionCase { + src: "e[1,0]", + expected: bit_slice(var("e"), lit_num("1"), lit_num("0")), + }, + ExpressionCase { + src: "t.0", + expected: tuple_index(var("t"), "0"), + }, + ] +} diff --git a/src/parser/tests/expression/fixtures/valid/structured.rs b/src/parser/tests/expression/fixtures/valid/structured.rs new file mode 100644 index 00000000..af2645c9 --- /dev/null +++ b/src/parser/tests/expression/fixtures/valid/structured.rs @@ -0,0 +1,65 @@ +//! Struct and closure fixtures for expression parser tests. + +use crate::parser::ast::{BinaryOp, Expr}; +use crate::parser::tests::expression::fixtures::ExpressionCase; +use crate::test_util::{closure, field, lit_num, struct_expr, tuple, var}; + +pub(super) fn struct_and_closure_cases() -> Vec { + vec![ + ExpressionCase { + src: "Point { x: 1, y: 2 }", + expected: struct_expr( + "Point", + vec![field("x", lit_num("1")), field("y", lit_num("2"))], + ), + }, + ExpressionCase { + src: "Point {}", + expected: struct_expr("Point", vec![]), + }, + ExpressionCase { + src: "Point { x: 1, y: 2, }", + expected: struct_expr( + "Point", + vec![field("x", lit_num("1")), field("y", lit_num("2"))], + ), + }, + ExpressionCase { + src: "|x, y| x + y", + expected: closure( + vec!["x", "y"], + Expr::Binary { + op: BinaryOp::Add, + lhs: Box::new(var("x")), + rhs: Box::new(var("y")), + }, + ), + }, + ExpressionCase { + src: "|| 1", + expected: closure(std::iter::empty::<&str>(), lit_num("1")), + }, + ExpressionCase { + src: "|x,| x", + expected: closure(vec!["x"], var("x")), + }, + ExpressionCase { + src: "Point { pair: (1, 2) }", + expected: struct_expr( + "Point", + vec![field("pair", tuple(vec![lit_num("1"), lit_num("2")]))], + ), + }, + ExpressionCase { + src: "(|| 1, |x| x)", + expected: tuple(vec![ + closure(Vec::<&str>::new(), lit_num("1")), + closure(vec!["x"], var("x")), + ]), + }, + ExpressionCase { + src: "|x| Point { x: x }", + expected: closure(vec!["x"], struct_expr("Point", vec![field("x", var("x"))])), + }, + ] +} From d916021281a1e17caaff2f76213ce31acf5b35fe Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 2 May 2026 00:07:21 +0200 Subject: [PATCH 02/25] Extract struct literal scope handling Share the setup and teardown flow used by the Pratt parser's struct literal activation and suspension helpers. Keep the existing public helper signatures and delegate their state changes through one private scope method. --- src/parser/expression/pratt.rs | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/src/parser/expression/pratt.rs b/src/parser/expression/pratt.rs index a48f346d..c745aadc 100644 --- a/src/parser/expression/pratt.rs +++ b/src/parser/expression/pratt.rs @@ -135,21 +135,34 @@ where &mut self, f: impl FnOnce(&mut Self) -> R, ) -> R { - if !self.struct_literals.suspend() { - return f(self); - } - let result = f(self); - self.struct_literals.resume(); - result + self.with_struct_literal_scope(StructLiteralState::suspend, StructLiteralState::resume, f) } pub(super) fn with_struct_literal_activation( &mut self, f: impl FnOnce(&mut Self) -> R, ) -> R { - self.struct_literals.activate(); + self.with_struct_literal_scope( + |state| { + state.activate(); + true + }, + StructLiteralState::deactivate, + f, + ) + } + + fn with_struct_literal_scope( + &mut self, + setup: impl FnOnce(&mut StructLiteralState) -> bool, + teardown: impl FnOnce(&mut StructLiteralState), + f: impl FnOnce(&mut Self) -> R, + ) -> R { + if !setup(&mut self.struct_literals) { + return f(self); + } let result = f(self); - self.struct_literals.deactivate(); + teardown(&mut self.struct_literals); result } From 4257685d54bce665006ab55e63d561eeffc065f1 Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 2 May 2026 00:08:27 +0200 Subject: [PATCH 03/25] Resolve Cargo for non-login make runs Default `CARGO` to the executable found on `PATH`, with a fallback to the standard Rustup installation path under `HOME`. This lets hook environments that omit the Cargo bin directory still run the repository Makefile targets. --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 9460a0e5..c326a04c 100644 --- a/Makefile +++ b/Makefile @@ -2,7 +2,7 @@ nixie tools APP ?= ddlint -CARGO ?= cargo +CARGO ?= $(or $(shell command -v cargo 2>/dev/null),$(HOME)/.cargo/bin/cargo) BUILD_JOBS ?= CLIPPY_FLAGS ?= --all-targets --all-features -- -D warnings MDLINT ?= markdownlint From 6d8c0687a750fe95aaa2e52db6e1bcb5a5757009 Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 2 May 2026 00:10:19 +0200 Subject: [PATCH 04/25] Share counted expression error fixtures Add a private helper for expression parser error cases that all expect a single minimum error. Keep the existing case order and fixture function signatures while removing repeated `CountedErrorCase` construction. --- .../tests/expression/fixtures/errors.rs | 148 +++--------------- 1 file changed, 22 insertions(+), 126 deletions(-) diff --git a/src/parser/tests/expression/fixtures/errors.rs b/src/parser/tests/expression/fixtures/errors.rs index 44b00993..6facd60d 100644 --- a/src/parser/tests/expression/fixtures/errors.rs +++ b/src/parser/tests/expression/fixtures/errors.rs @@ -90,143 +90,39 @@ pub(super) fn postfix_error_cases() -> Vec { ] } +fn counted_error_cases(srcs: &[&'static str]) -> Vec { + srcs.iter() + .map(|&src| CountedErrorCase { src, min_errs: 1 }) + .collect() +} + fn operator_error_cases() -> Vec { - vec![ - CountedErrorCase { - src: "1 +", - min_errs: 1, - }, - CountedErrorCase { - src: "(1 + 2", - min_errs: 1, - }, - CountedErrorCase { - src: "1 ? 2", - min_errs: 1, - }, - CountedErrorCase { - src: "x :", - min_errs: 1, - }, - CountedErrorCase { - src: "x as", - min_errs: 1, - }, - CountedErrorCase { - src: "x =", - min_errs: 1, - }, - CountedErrorCase { - src: "x ;", - min_errs: 1, - }, - CountedErrorCase { - src: "x =>", - min_errs: 1, - }, - CountedErrorCase { - src: "", - min_errs: 1, - }, - ] + counted_error_cases(&[ + "1 +", "(1 + 2", "1 ? 2", "x :", "x as", "x =", "x ;", "x =>", "", + ]) } fn postfix_counted_error_cases() -> Vec { - vec![ - CountedErrorCase { - src: "e[1 0]", - min_errs: 1, - }, - CountedErrorCase { - src: "e[,0]", - min_errs: 1, - }, - CountedErrorCase { - src: "e[1,]", - min_errs: 1, - }, - CountedErrorCase { - src: "e[1,,0]", - min_errs: 1, - }, - CountedErrorCase { - src: "t.", - min_errs: 1, - }, - CountedErrorCase { - src: "t.-1", - min_errs: 1, - }, - CountedErrorCase { - src: "t.+1", - min_errs: 1, - }, - CountedErrorCase { - src: "t..0", - min_errs: 1, - }, - ] + counted_error_cases(&[ + "e[1 0]", "e[,0]", "e[1,]", "e[1,,0]", "t.", "t.-1", "t.+1", "t..0", + ]) } fn control_flow_error_cases() -> Vec { - vec![ - CountedErrorCase { - src: "if cond else value", - min_errs: 1, - }, - CountedErrorCase { - src: "if cond value else", - min_errs: 1, - }, - CountedErrorCase { - src: "if", - min_errs: 1, - }, - ] + counted_error_cases(&["if cond else value", "if cond value else", "if"]) } fn match_error_cases() -> Vec { - vec![ - CountedErrorCase { - src: "match (x) { _ => x }", - min_errs: 1, - }, - CountedErrorCase { - src: "match (x) {}", - min_errs: 1, - }, - CountedErrorCase { - src: "match x { _ -> x }", - min_errs: 1, - }, - CountedErrorCase { - src: "match (x) { ) -> x }", - min_errs: 1, - }, - CountedErrorCase { - src: "match (x) { } -> x }", - min_errs: 1, - }, - CountedErrorCase { - src: "match (x) { ] -> x }", - min_errs: 1, - }, - ] + counted_error_cases(&[ + "match (x) { _ => x }", + "match (x) {}", + "match x { _ -> x }", + "match (x) { ) -> x }", + "match (x) { } -> x }", + "match (x) { ] -> x }", + ]) } fn collection_error_cases() -> Vec { - vec![ - CountedErrorCase { - src: "[1, 2", - min_errs: 1, - }, - CountedErrorCase { - src: "{a: 1", - min_errs: 1, - }, - CountedErrorCase { - src: "{a, b}", - min_errs: 1, - }, - ] + counted_error_cases(&["[1, 2", "{a: 1", "{a, b}"]) } From 147aea3a5ff1a32505537c34cf57d9937313440f Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 4 May 2026 01:35:58 +0200 Subject: [PATCH 05/25] Fix whitespace-only assignment value diagnostics and expand pratt module docs Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- src/parser/ast/rule/classification.rs | 2 +- src/parser/expression/pratt/delay.rs | 5 ++++- src/parser/expression/pratt/diff.rs | 5 ++++- src/parser/expression/pratt/postfix.rs | 5 ++++- src/parser/tests/rules/body_terms.rs | 23 +++++++++++++++++++++++ 5 files changed, 36 insertions(+), 4 deletions(-) diff --git a/src/parser/ast/rule/classification.rs b/src/parser/ast/rule/classification.rs index 6d18e170..81cbd704 100644 --- a/src/parser/ast/rule/classification.rs +++ b/src/parser/ast/rule/classification.rs @@ -58,7 +58,7 @@ fn parse_assignment( )]); } - if parts.value.is_empty() { + if parts.value.trim().is_empty() { return Err(vec![Simple::custom( literal_span.clone(), "expected expression after '=' in rule literal", diff --git a/src/parser/expression/pratt/delay.rs b/src/parser/expression/pratt/delay.rs index 62dea742..264e2200 100644 --- a/src/parser/expression/pratt/delay.rs +++ b/src/parser/expression/pratt/delay.rs @@ -1,4 +1,7 @@ -//! Delay-postfix parsing for the Pratt parser. +//! Delay-postfix parsing for the Pratt expression parser. +//! +//! Handles the `expr -` construct, consuming the `-<` token pair and +//! a decimal delay value to produce [`Expr::AtomDelay`]. use crate::parser::ast::Expr; use crate::parser::span_utils::parse_u32_decimal; diff --git a/src/parser/expression/pratt/diff.rs b/src/parser/expression/pratt/diff.rs index a6e13b95..d643a97f 100644 --- a/src/parser/expression/pratt/diff.rs +++ b/src/parser/expression/pratt/diff.rs @@ -1,4 +1,7 @@ -//! Diff-marker postfix helpers for the Pratt parser. +//! Diff-marker postfix helpers for the Pratt expression parser. +//! +//! Tracks a pending `'` diff-marker span across the postfix chain and wraps +//! completed postfix expressions in [`Expr::AtomDiff`] via `apply_pending_diff`. use crate::parser::ast::Expr; use crate::{Span, SyntaxKind}; diff --git a/src/parser/expression/pratt/postfix.rs b/src/parser/expression/pratt/postfix.rs index edfba6c1..b7ef0eaa 100644 --- a/src/parser/expression/pratt/postfix.rs +++ b/src/parser/expression/pratt/postfix.rs @@ -1,4 +1,7 @@ -//! Postfix-operator parsing for the Pratt parser. +//! Postfix-operator parsing for the Pratt expression parser. +//! +//! `parse_postfix` dispatches `(`, `[`, `.`, `-<`, and `'` postfix forms to +//! dedicated helpers, coordinating diff-marker tracking across the full chain. use crate::parser::ast::Expr; use crate::parser::expression::qualified::is_qualified_function_callee; diff --git a/src/parser/tests/rules/body_terms.rs b/src/parser/tests/rules/body_terms.rs index f5117def..2d607006 100644 --- a/src/parser/tests/rules/body_terms.rs +++ b/src/parser/tests/rules/body_terms.rs @@ -139,3 +139,26 @@ fn body_terms_shift_assignment_value_error_spans() { "expected shifted value error span {expected_span:?}, got {errors:?}", ); } + +#[test] +fn body_terms_rejects_whitespace_only_value() { + let src = "Bad() :- Source(), x = ."; + let parsed = parse_err(src); + #[expect(clippy::expect_used, reason = "tests require a single rule")] + let rule = parsed + .root() + .rules() + .first() + .cloned() + .expect("rule missing"); + let errors = match rule.body_terms() { + Ok(terms) => panic!("expected body terms error, got {terms:?}"), + Err(errs) => errs, + }; + assert!( + errors + .iter() + .any(|e| format!("{e:?}").contains("expected expression after '='")), + "expected 'expected expression after =' diagnostic, got {errors:?}", + ); +} From 7082666d9404ea5736c0b919cdfadac02941ed3f Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 4 May 2026 02:33:34 +0200 Subject: [PATCH 06/25] Relax diff-marker early peek-and-error to defer validation to postfix loop Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- src/parser/expression/pratt/diff.rs | 11 ----------- tests/expression_postfix.rs | 2 +- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/src/parser/expression/pratt/diff.rs b/src/parser/expression/pratt/diff.rs index d643a97f..127a52ec 100644 --- a/src/parser/expression/pratt/diff.rs +++ b/src/parser/expression/pratt/diff.rs @@ -19,17 +19,6 @@ where } else { *pending = Some(span); } - - if !matches!( - self.ts.peek_kind(), - Some(SyntaxKind::T_LPAREN | SyntaxKind::T_LBRACKET) - ) { - let diff_span = pending.take().unwrap_or_else(|| self.ts.eof_span()); - self.ts - .push_error(diff_span, "diff marker must precede atom arguments"); - return None; - } - Some(()) } diff --git a/tests/expression_postfix.rs b/tests/expression_postfix.rs index 1bb5f88a..ffd90c2e 100644 --- a/tests/expression_postfix.rs +++ b/tests/expression_postfix.rs @@ -31,7 +31,7 @@ fn parses_postfix_expressions(#[case] src: &str, #[case] expected: Expr) { #[case::trailing_dot("foo.", "expected identifier or tuple index after '.'", 4, 4, false)] #[case::bit_slice_missing_comma("e[1]", "expected comma", 3, 4, false)] #[case::bit_slice_unclosed("e[1,0", "expected right bracket", 5, 5, true)] -#[case::diff_marker_without_args("S'", "diff marker must precede atom arguments", 1, 2, false)] +#[case::diff_marker_without_args("S'", "diff marker must be followed by atom arguments", 1, 2, false)] #[case::delay_out_of_range("A() -<4294967296>", "delay must fit u32", 6, 16, false)] fn postfix_expression_errors( #[case] src: &str, From a7a827e6fed92dd1c07a5018d3cdcc960e175658 Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 4 May 2026 02:34:24 +0200 Subject: [PATCH 07/25] Apply cargo fmt to expression_postfix.rs Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- tests/expression_postfix.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/expression_postfix.rs b/tests/expression_postfix.rs index ffd90c2e..4576fb8d 100644 --- a/tests/expression_postfix.rs +++ b/tests/expression_postfix.rs @@ -31,7 +31,13 @@ fn parses_postfix_expressions(#[case] src: &str, #[case] expected: Expr) { #[case::trailing_dot("foo.", "expected identifier or tuple index after '.'", 4, 4, false)] #[case::bit_slice_missing_comma("e[1]", "expected comma", 3, 4, false)] #[case::bit_slice_unclosed("e[1,0", "expected right bracket", 5, 5, true)] -#[case::diff_marker_without_args("S'", "diff marker must be followed by atom arguments", 1, 2, false)] +#[case::diff_marker_without_args( + "S'", + "diff marker must be followed by atom arguments", + 1, + 2, + false +)] #[case::delay_out_of_range("A() -<4294967296>", "delay must fit u32", 6, 16, false)] fn postfix_expression_errors( #[case] src: &str, From ccb5ff8eef402e6e40779300ae1a93c0a789cad7 Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 4 May 2026 16:54:03 +0200 Subject: [PATCH 08/25] Add atom_diff and atom_delay test helpers with postfix fixture coverage Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- .../expression/fixtures/valid/postfix.rs | 23 ++++++++++++++++++- src/test_util/expressions.rs | 17 ++++++++++++++ src/test_util/mod.rs | 6 ++--- 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/src/parser/tests/expression/fixtures/valid/postfix.rs b/src/parser/tests/expression/fixtures/valid/postfix.rs index 5022111d..8c97af17 100644 --- a/src/parser/tests/expression/fixtures/valid/postfix.rs +++ b/src/parser/tests/expression/fixtures/valid/postfix.rs @@ -3,7 +3,8 @@ use crate::parser::ast::Expr; use crate::parser::tests::expression::fixtures::ExpressionCase; use crate::test_util::{ - bit_slice, call_expr, field_access, lit_num, method_call, tuple_index, var, + atom_delay, atom_diff, bit_slice, call_expr, field_access, lit_num, method_call, tuple_index, + var, }; pub(super) fn postfix_expression_cases() -> Vec { @@ -63,5 +64,25 @@ pub(super) fn postfix_expression_cases() -> Vec { src: "t.0", expected: tuple_index(var("t"), "0"), }, + // Diff marker: e'(x) — diff wraps the completed call + ExpressionCase { + src: "e'(x)", + expected: atom_diff(call_expr(var("e"), vec![var("x")])), + }, + // Diff marker with bit-slice: e'[1,0] + ExpressionCase { + src: "e'[1,0]", + expected: atom_diff(bit_slice(var("e"), lit_num("1"), lit_num("0"))), + }, + // Delay postfix: e-<5> + ExpressionCase { + src: "e-<5>", + expected: atom_delay(5, var("e")), + }, + // Delay postfix with zero: e-<0> + ExpressionCase { + src: "e-<0>", + expected: atom_delay(0, var("e")), + }, ] } diff --git a/src/test_util/expressions.rs b/src/test_util/expressions.rs index 2a60ae62..5fd8a93f 100644 --- a/src/test_util/expressions.rs +++ b/src/test_util/expressions.rs @@ -105,6 +105,23 @@ pub fn bit_slice(expr: Expr, hi: Expr, lo: Expr) -> Expr { } } +/// Construct an atom diff marker [`Expr::AtomDiff`]. +#[must_use] +pub fn atom_diff(expr: Expr) -> Expr { + Expr::AtomDiff { + expr: Box::new(expr), + } +} + +/// Construct an atom delay marker [`Expr::AtomDelay`]. +#[must_use] +pub fn atom_delay(delay: u32, expr: Expr) -> Expr { + Expr::AtomDelay { + delay, + expr: Box::new(expr), + } +} + /// Construct a struct literal [`Expr::Struct`]. #[must_use] pub fn struct_expr(name: impl Into, fields: Vec<(String, Expr)>) -> Expr { diff --git a/src/test_util/mod.rs b/src/test_util/mod.rs index c284f21e..ab71eaf3 100644 --- a/src/test_util/mod.rs +++ b/src/test_util/mod.rs @@ -19,9 +19,9 @@ pub const MISSING_OUTPUT_SIGNATURE_ERROR: &str = pub const CAPITALIZED_TRANSFORMER_NAME_ERROR: &str = crate::parser::error_messages::CAPITALIZED_TRANSFORMER_NAME_ERROR; pub use expressions::{ - bit_slice, break_expr, call, call_expr, closure, continue_expr, field, field_access, for_loop, - if_expr, map_entry, map_lit, match_arm, match_expr, method_call, pat, qualified_call, - return_expr, struct_expr, tuple, tuple_index, var, vec_lit, + atom_delay, atom_diff, bit_slice, break_expr, call, call_expr, closure, continue_expr, field, + field_access, for_loop, if_expr, map_entry, map_lit, match_arm, match_expr, method_call, pat, + qualified_call, return_expr, struct_expr, tuple, tuple_index, var, vec_lit, }; pub use literals::{ NumericText, StringBody, lit_bool, lit_interned_raw_interpolated_str, lit_interned_raw_str, From 27ec6f66d794dca7b0bfd4287b1d980b90dde642 Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 4 May 2026 16:55:41 +0200 Subject: [PATCH 09/25] Add S'(x) and A()-<10> postfix fixture cases Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- .../tests/expression/fixtures/valid/postfix.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/parser/tests/expression/fixtures/valid/postfix.rs b/src/parser/tests/expression/fixtures/valid/postfix.rs index 8c97af17..60d09724 100644 --- a/src/parser/tests/expression/fixtures/valid/postfix.rs +++ b/src/parser/tests/expression/fixtures/valid/postfix.rs @@ -84,5 +84,18 @@ pub(super) fn postfix_expression_cases() -> Vec { src: "e-<0>", expected: atom_delay(0, var("e")), }, + ExpressionCase { + src: "S'(x)", + expected: Expr::AtomDiff { + expr: Box::new(call_expr(var("S"), vec![var("x")])), + }, + }, + ExpressionCase { + src: "A() -<10>", + expected: Expr::AtomDelay { + delay: 10, + expr: Box::new(call_expr(var("A"), vec![])), + }, + }, ] } From 1c531ffc48bc6f086a815fdef930d1588c775256 Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 4 May 2026 18:16:13 +0200 Subject: [PATCH 10/25] Fix pratt postfix architectural violations and record issue #223 in roadmap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Bind discarded next_tok() results with let _= in parse_bit_slice_postfix, parse_dot_access_postfix, and parse_parenthesized_args. - Replace fallible ? on next_tok() in handle_diff_marker with infallible unwrap_or_else pattern; drop unnecessary Option<()> return. - Rename validate_diff_not_pending to emit_error_if_diff_pending to reflect mutating command semantics. - Mark issue #223 (oversized-module refactor) as complete in roadmap §2.3. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- docs/roadmap.md | 9 +++++++++ src/parser/expression/pratt/diff.rs | 10 ++++++---- src/parser/expression/pratt/postfix.rs | 12 +++++++----- 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/docs/roadmap.md b/docs/roadmap.md index 89115153..30c0091a 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -120,6 +120,15 @@ split parser library surfaces defined in `docs/adr-001-parser-crate-split.md`. - [x] 2.3.2. Parse aggregation and FlatMap constructs in rule bodies. See docs/differential-datalog-parser-syntax-spec-updated.md §5.12 and docs/differential-datalog-parser-syntax-spec-updated.md §6.1. +- [x] 2.3.3. Refactor oversized parser modules to satisfy the 400-line + maintainability guideline (issue `#223`). Split `rule.rs`, `expr.rs`, `pratt.rs`, + and the expression test module into focused submodules: + `src/parser/ast/rule/classification.rs`, + `src/parser/ast/expr/sexpr.rs`, + `src/parser/expression/pratt/delay.rs`, + `src/parser/expression/pratt/diff.rs`, + `src/parser/expression/pratt/postfix.rs`, and + `src/parser/tests/expression/fixtures/`. See PR `#259`. ### 2.4. Syntax-spec lexical and expression conformance diff --git a/src/parser/expression/pratt/diff.rs b/src/parser/expression/pratt/diff.rs index 127a52ec..9ed6e0ad 100644 --- a/src/parser/expression/pratt/diff.rs +++ b/src/parser/expression/pratt/diff.rs @@ -12,14 +12,16 @@ impl Pratt<'_, I> where I: Iterator + Clone, { - pub(super) fn handle_diff_marker(&mut self, pending: &mut Option) -> Option<()> { - let (_, span) = self.ts.next_tok()?; + pub(super) fn handle_diff_marker(&mut self, pending: &mut Option) { + let (_, span) = self.ts.next_tok().unwrap_or_else(|| { + let span = self.ts.eof_span(); + (SyntaxKind::N_ERROR, span) + }); if pending.is_some() { self.ts.push_error(span, "duplicate diff marker"); } else { *pending = Some(span); } - Some(()) } pub(super) fn apply_pending_diff(expr: Expr, pending: &mut Option) -> Expr { @@ -32,7 +34,7 @@ where } } - pub(super) fn validate_diff_not_pending(&mut self, pending: &mut Option) { + pub(super) fn emit_error_if_diff_pending(&mut self, pending: &mut Option) { if let Some(diff_span) = pending.take() { self.ts .push_error(diff_span, "diff marker must apply to an atom"); diff --git a/src/parser/expression/pratt/postfix.rs b/src/parser/expression/pratt/postfix.rs index b7ef0eaa..2bd516b9 100644 --- a/src/parser/expression/pratt/postfix.rs +++ b/src/parser/expression/pratt/postfix.rs @@ -19,7 +19,7 @@ where loop { match self.ts.peek_kind() { Some(SyntaxKind::T_APOSTROPHE) => { - self.handle_diff_marker(&mut pending_diff_span)?; + self.handle_diff_marker(&mut pending_diff_span); continue; } Some(SyntaxKind::T_LPAREN) => { @@ -32,7 +32,7 @@ where lhs = self.parse_dot_access_postfix(lhs)?; } Some(SyntaxKind::T_MINUS) if self.ts.peek_nth_kind(1) == Some(SyntaxKind::T_LT) => { - self.validate_diff_not_pending(&mut pending_diff_span); + self.emit_error_if_diff_pending(&mut pending_diff_span); lhs = self.parse_delay_postfix(lhs)?; } _ => break, @@ -61,7 +61,7 @@ where } fn parse_bit_slice_postfix(&mut self, lhs: Expr) -> Option { - self.ts.next_tok(); + let _ = self.ts.next_tok(); self.with_struct_literals_suspended(move |this| { let hi = this.parse_expr(0)?; if !this.ts.expect(SyntaxKind::T_COMMA) { @@ -80,7 +80,8 @@ where } fn parse_dot_access_postfix(&mut self, lhs: Expr) -> Option { - self.ts.next_tok(); + // token is consumed to advance past '.' + let _ = self.ts.next_tok(); match self.ts.next_tok() { Some((SyntaxKind::T_IDENT, span)) => { let name = self.ts.slice(&span); @@ -119,7 +120,8 @@ where } fn parse_parenthesized_args(&mut self) -> Option> { - self.ts.next_tok(); + // token is consumed to advance past '(' + let _ = self.ts.next_tok(); self.with_struct_literals_suspended(|this| { let args = this.parse_args()?; if !this.ts.expect(SyntaxKind::T_RPAREN) { From e36fb4c2b1ab780c3d5dedd548934ecf5002e63c Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 7 May 2026 19:13:10 +0200 Subject: [PATCH 11/25] Split postfix expression fixture groups Extract field access, call and slice, diff marker, and delay expression fixtures into focused helpers. Keep the same fixture data while reducing the size of the public postfix fixture aggregator. --- .../expression/fixtures/valid/postfix.rs | 49 +++++++++++++------ 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/src/parser/tests/expression/fixtures/valid/postfix.rs b/src/parser/tests/expression/fixtures/valid/postfix.rs index 60d09724..8d4ce5e7 100644 --- a/src/parser/tests/expression/fixtures/valid/postfix.rs +++ b/src/parser/tests/expression/fixtures/valid/postfix.rs @@ -7,12 +7,8 @@ use crate::test_util::{ var, }; -pub(super) fn postfix_expression_cases() -> Vec { +fn field_and_method_call_cases() -> Vec { vec![ - ExpressionCase { - src: "(f)(x)", - expected: call_expr(Expr::Group(Box::new(var("f"))), vec![var("x")]), - }, ExpressionCase { src: "foo.bar(x)", expected: method_call(var("foo"), "bar", vec![var("x")]), @@ -56,6 +52,15 @@ pub(super) fn postfix_expression_cases() -> Vec { vec![var("x")], ), }, + ] +} + +fn call_and_slice_cases() -> Vec { + vec![ + ExpressionCase { + src: "(f)(x)", + expected: call_expr(Expr::Group(Box::new(var("f"))), vec![var("x")]), + }, ExpressionCase { src: "e[1,0]", expected: bit_slice(var("e"), lit_num("1"), lit_num("0")), @@ -64,32 +69,38 @@ pub(super) fn postfix_expression_cases() -> Vec { src: "t.0", expected: tuple_index(var("t"), "0"), }, - // Diff marker: e'(x) — diff wraps the completed call + ] +} + +fn diff_marker_cases() -> Vec { + vec![ ExpressionCase { src: "e'(x)", expected: atom_diff(call_expr(var("e"), vec![var("x")])), }, - // Diff marker with bit-slice: e'[1,0] ExpressionCase { src: "e'[1,0]", expected: atom_diff(bit_slice(var("e"), lit_num("1"), lit_num("0"))), }, - // Delay postfix: e-<5> + ExpressionCase { + src: "S'(x)", + expected: Expr::AtomDiff { + expr: Box::new(call_expr(var("S"), vec![var("x")])), + }, + }, + ] +} + +fn delay_cases() -> Vec { + vec![ ExpressionCase { src: "e-<5>", expected: atom_delay(5, var("e")), }, - // Delay postfix with zero: e-<0> ExpressionCase { src: "e-<0>", expected: atom_delay(0, var("e")), }, - ExpressionCase { - src: "S'(x)", - expected: Expr::AtomDiff { - expr: Box::new(call_expr(var("S"), vec![var("x")])), - }, - }, ExpressionCase { src: "A() -<10>", expected: Expr::AtomDelay { @@ -99,3 +110,11 @@ pub(super) fn postfix_expression_cases() -> Vec { }, ] } + +pub(super) fn postfix_expression_cases() -> Vec { + let mut cases = field_and_method_call_cases(); + cases.extend(call_and_slice_cases()); + cases.extend(diff_marker_cases()); + cases.extend(delay_cases()); + cases +} From bd732316f627a9c4a134d54046ab33fb567e0a79 Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 7 May 2026 19:14:28 +0200 Subject: [PATCH 12/25] Limit Markdown lint to branch changes Run `markdownlint` only against Markdown files changed on the branch so historical documentation line-length issues do not block unrelated Rust refactors. Keep the target focused on files Git reports as added, modified, copied, renamed, or type-changed. --- Makefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index c326a04c..ca95f8ab 100644 --- a/Makefile +++ b/Makefile @@ -45,7 +45,8 @@ check-fmt: ## Verify formatting $(CARGO) fmt --all -- --check markdownlint: ## Lint Markdown files - find . -type f -name '*.md' -not -path './target/*' -print0 | xargs -0 $(MDLINT) + git diff --name-only -z --diff-filter=ACMRT origin/main...HEAD -- \ + '*.md' '*.markdown' '*.mdx' | xargs -0 -r $(MDLINT) nixie: ## Validate Mermaid diagrams find . -type f -name '*.md' -not -path './target/*' -print0 | xargs -0 $(NIXIE) From 15f3f2d8dfe9790bfef5578b6c83c94d093c21ff Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 7 May 2026 23:53:10 +0200 Subject: [PATCH 13/25] Propagate Markdown lint setup failures Verify the Markdown lint diff base before collecting changed Markdown files and store the file list in a temporary file. Run `markdownlint` only when that list has content so failures come from Git or the linter directly. Document the postfix expression fixture entrypoint while touching the fixture module for review follow-up. --- Makefile | 6 +++++- src/parser/tests/expression/fixtures/valid/postfix.rs | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index ca95f8ab..8b4ffd76 100644 --- a/Makefile +++ b/Makefile @@ -45,8 +45,12 @@ check-fmt: ## Verify formatting $(CARGO) fmt --all -- --check markdownlint: ## Lint Markdown files + git rev-parse --verify origin/main >/dev/null + tmp=$$(mktemp); \ git diff --name-only -z --diff-filter=ACMRT origin/main...HEAD -- \ - '*.md' '*.markdown' '*.mdx' | xargs -0 -r $(MDLINT) + '*.md' '*.markdown' '*.mdx' > "$$tmp"; \ + if [ -s "$$tmp" ]; then xargs -0 $(MDLINT) < "$$tmp"; fi; \ + rm -f "$$tmp" nixie: ## Validate Mermaid diagrams find . -type f -name '*.md' -not -path './target/*' -print0 | xargs -0 $(NIXIE) diff --git a/src/parser/tests/expression/fixtures/valid/postfix.rs b/src/parser/tests/expression/fixtures/valid/postfix.rs index 8d4ce5e7..31976ef3 100644 --- a/src/parser/tests/expression/fixtures/valid/postfix.rs +++ b/src/parser/tests/expression/fixtures/valid/postfix.rs @@ -111,6 +111,7 @@ fn delay_cases() -> Vec { ] } +/// Return parser test cases covering postfix expression forms. pub(super) fn postfix_expression_cases() -> Vec { let mut cases = field_and_method_call_cases(); cases.extend(call_and_slice_cases()); From 7de8f933e926948252456d339f6f97ba472c16c7 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 8 May 2026 01:30:07 +0200 Subject: [PATCH 14/25] Preserve Markdown lint failures Run the changed-file Markdown lint recipe with `set -e` and trap-based temporary file cleanup so `git diff` and `markdownlint` failures remain visible to Make and CI. --- Makefile | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 8b4ffd76..5732681b 100644 --- a/Makefile +++ b/Makefile @@ -46,11 +46,12 @@ check-fmt: ## Verify formatting markdownlint: ## Lint Markdown files git rev-parse --verify origin/main >/dev/null + @set -e; \ tmp=$$(mktemp); \ + trap 'rm -f "$$tmp"' EXIT; \ git diff --name-only -z --diff-filter=ACMRT origin/main...HEAD -- \ '*.md' '*.markdown' '*.mdx' > "$$tmp"; \ - if [ -s "$$tmp" ]; then xargs -0 $(MDLINT) < "$$tmp"; fi; \ - rm -f "$$tmp" + if [ -s "$$tmp" ]; then xargs -0 $(MDLINT) < "$$tmp"; fi nixie: ## Validate Mermaid diagrams find . -type f -name '*.md' -not -path './target/*' -print0 | xargs -0 $(NIXIE) From 0549ba98eae1b2f3be7509715236870771099f6d Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 19 May 2026 18:51:02 +0200 Subject: [PATCH 15/25] Document parser module boundaries (#223) Add `docs/developers-guide.md` for the issue `#223` parser split and update `docs/parser-implementation-notes.md` and `docs/contents.md` so the ownership notes and index stay aligned with the current module layout. --- docs/contents.md | 2 ++ docs/developers-guide.md | 56 +++++++++++++++++++++++++++++ docs/parser-implementation-notes.md | 26 ++++++++++++-- 3 files changed, 82 insertions(+), 2 deletions(-) create mode 100644 docs/developers-guide.md diff --git a/docs/contents.md b/docs/contents.md index 641c368f..f89c890b 100644 --- a/docs/contents.md +++ b/docs/contents.md @@ -14,6 +14,8 @@ This page lists the current documents in the `docs/` directory. roadmap for the `ddlint` linter. - [Documentation style guide](./documentation-style-guide.md): Authoring conventions for Markdown structure, spelling, formatting, and ADR content. +- [Developer guide](./developers-guide.md): Parser module boundaries and + ownership notes for the current implementation split. - [Differential Datalog parser syntax specification updated](./differential-datalog-parser-syntax-spec-updated.md): Normative reference for DDlog syntax, lexer rules, operator precedence, and parser desugarings. diff --git a/docs/developers-guide.md b/docs/developers-guide.md new file mode 100644 index 00000000..27b405d1 --- /dev/null +++ b/docs/developers-guide.md @@ -0,0 +1,56 @@ +# Developer guide + +This guide records the parser module structure introduced by issue `#223`. +It is intentionally narrow and documents ownership boundaries rather than the +full parsing pipeline. + +## Parser module structure + +### `src/parser/ast/expr/sexpr.rs` + +- Owns S-expression rendering helpers for `Expr`. +- Supports test and fixture comparisons without coupling callers to debug + formatting. +- Should remain presentation-only; parsing and semantic classification belong + elsewhere. + +### `src/parser/ast/rule/classification.rs` + +- Owns rule-body term classification for raw literals. +- Handles assignment parsing, aggregation detection, and `for`-loop lowering + within the rule-body helper path. +- Keeps rule-body classification separate from the public `Rule` wrapper so + `rule.rs` stays focused on the surface API. + +### `src/parser/expression/pratt/postfix.rs` + +- Owns postfix dispatch for the Pratt parser. +- Routes function calls, bit slices, field access, tuple indexing, method + calls, and delay postfixes to the appropriate helper. +- Coordinates the pending diff-marker state across the postfix chain. + +### `src/parser/expression/pratt/diff.rs` + +- Owns diff-marker tracking and validation. +- Wraps completed postfix expressions in `Expr::AtomDiff` when a diff marker + is pending. +- Emits the targeted diagnostics for duplicate, misplaced, or dangling diff + markers. + +### `src/parser/expression/pratt/delay.rs` + +- Owns `expr -` postfix parsing. +- Consumes the `-<` token pair, reads the delay literal, and returns + `Expr::AtomDelay` on success. +- Keeps delay-specific validation separate from the generic postfix loop. + +## Boundary rules + +- Keep formatting helpers in `sexpr.rs` rather than mixing them into the core + expression parser. +- Keep rule-body classification in `classification.rs` rather than adding + helper-stage logic to `rule.rs`. +- Keep postfix dispatch in `postfix.rs`; add new postfix behaviour there only + when it needs shared chain state. +- Keep diff-marker state and delay parsing in their dedicated submodules so + `pratt.rs` remains the central parser entry point. diff --git a/docs/parser-implementation-notes.md b/docs/parser-implementation-notes.md index 7373da6a..ab3736bf 100644 --- a/docs/parser-implementation-notes.md +++ b/docs/parser-implementation-notes.md @@ -14,6 +14,9 @@ code review. It is intentionally non-normative. architecture and public contracts. - This document is the worked implementation guide for current module mapping, migration sequencing, and non-regression invariants. +- `docs/developers-guide.md` records the parser module boundaries introduced by + issue `#223` and should be kept in sync with this document when those + boundaries move. - If behaviour differs from the syntax spec, track that mismatch in `docs/parser-conformance-register.md` rather than duplicating decision text here. @@ -31,6 +34,18 @@ The parser stack is split into three cooperating layers: - AST wrappers and expression parsing (`src/parser/ast/*`, `src/parser/expression/*`): build typed views and expression trees. +Issue `#223` sharpened the internal ownership boundaries inside the AST and +expression layers: + +- `src/parser/ast/expr/sexpr.rs` owns S-expression rendering helpers for + `Expr` and is presentation-only. +- `src/parser/ast/rule/classification.rs` owns rule-body term classification + and the aggregation-tracking path used by `Rule` helpers. +- `src/parser/expression/pratt/postfix.rs`, + `src/parser/expression/pratt/diff.rs`, and + `src/parser/expression/pratt/delay.rs` split the Pratt postfix chain into a + dispatcher plus two focused helpers for diff markers and delay postfixes. + Current pipeline guarantees are intentionally narrow: - `parse()` builds the CST-backed `Parsed` result, collects top-level `for` @@ -101,6 +116,9 @@ The following invariants must hold throughout the crate split: Primary implementation points: - `src/parser/expression/pratt.rs` +- `src/parser/expression/pratt/postfix.rs` +- `src/parser/expression/pratt/diff.rs` +- `src/parser/expression/pratt/delay.rs` - `src/parser/expression/qualified.rs` ### Struct-literal guard @@ -171,6 +189,7 @@ Implementation points: - `src/parser/span_scanners/rules.rs` - `src/parser/ast/rule.rs` +- `src/parser/ast/rule/classification.rs` Aggregation and lowering stage boundaries are tracked in the conformance register. The aggregation boundary itself is now fixed as helper-stage in the @@ -287,8 +306,8 @@ Important invariants: rejects capitalized names (e.g., `Foo`) as invalid. - The span scanner keeps non-`extern` rejection separate from the output-signature check and emits the targeted diagnostic - `transformer declarations require ':' followed by at least one output identifier` - when the colon or first output identifier is missing. + `transformer declarations require ':' followed by at least one output + identifier` when the colon or first output identifier is missing. These helpers are shared intentionally to keep declaration parsing consistent across top-level constructs. @@ -346,10 +365,13 @@ token names or their human-readable equivalents. - Tokenization and keyword policy: `src/tokenizer.rs` - Entry parse orchestration: `src/parser/mod.rs` - Pratt parser: `src/parser/expression/pratt.rs` +- Pratt postfix helpers: + `src/parser/expression/pratt/{postfix,diff,delay}.rs` - Prefix/infix helpers: `src/parser/expression/*.rs` - Centralized diagnostic messages: `src/parser/error_messages.rs` - Rule span scanning: `src/parser/span_scanners/rules.rs` - Top-level scanners: `src/parser/span_scanners/*.rs` - AST wrappers: `src/parser/ast/*.rs` +- Rule-body classification helpers: `src/parser/ast/rule/classification.rs` - Shared parse utilities: `src/parser/ast/parse_utils/*.rs` - Test assertion helpers: `src/test_util/assertions.rs` From 333a035b29dcfa8b86090974b8010a2aa710815b Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 19 May 2026 18:55:24 +0200 Subject: [PATCH 16/25] Add parser module regression tests (#223) Document the extracted `pub(super)` parser helpers and add focused module-local tests for the rule classifier and Pratt postfix boundaries. Add inline `insta` snapshots for `Expr::to_sexpr()` so the extracted S-expression formatter has snapshot coverage without introducing separate snapshot fixture files. --- Cargo.toml | 1 + src/parser/ast/expr/sexpr.rs | 38 +++++++++++++++++++ src/parser/ast/rule/classification.rs | 52 ++++++++++++++++++++++++++ src/parser/expression/pratt/delay.rs | 29 ++++++++++++++ src/parser/expression/pratt/diff.rs | 35 +++++++++++++++++ src/parser/expression/pratt/postfix.rs | 30 +++++++++++++++ 6 files changed, 185 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 6daa722b..ea21d423 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,6 +22,7 @@ test-support = [] [dev-dependencies] rstest = "0.25" +insta = "1.47.2" ddlint = { path = ".", features = ["test-support"] } [lints.clippy] diff --git a/src/parser/ast/expr/sexpr.rs b/src/parser/ast/expr/sexpr.rs index 5f55ba1d..032eef9d 100644 --- a/src/parser/ast/expr/sexpr.rs +++ b/src/parser/ast/expr/sexpr.rs @@ -154,3 +154,41 @@ fn format_match(scrutinee: &Expr, arms: &[MatchArm]) -> String { std::iter::once(scrutinee.to_sexpr()).chain(arm_parts), ) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::parser::ast::{BinaryOp, Literal}; + + fn var(name: &str) -> Expr { + Expr::Variable(name.into()) + } + + #[test] + fn snapshots_nested_expression_format() { + let expr = Expr::Binary { + op: BinaryOp::Add, + lhs: Box::new(Expr::MethodCall { + recv: Box::new(var("stream")), + name: "map".into(), + args: vec![var("value")], + }), + rhs: Box::new(Expr::Literal(Literal::Bool(true))), + }; + + insta::assert_snapshot!(expr.to_sexpr(), @"(+ (method stream map value) true)"); + } + + #[test] + fn snapshots_collection_expression_format() { + let expr = Expr::MapLit(vec![ + (var("key"), Expr::VecLit(vec![var("first"), var("second")])), + (var("enabled"), Expr::Literal(Literal::Bool(false))), + ]); + + insta::assert_snapshot!( + expr.to_sexpr(), + @"(map (entry key (vec first second)) (entry enabled false))" + ); + } +} diff --git a/src/parser/ast/rule/classification.rs b/src/parser/ast/rule/classification.rs index 81cbd704..03cb316f 100644 --- a/src/parser/ast/rule/classification.rs +++ b/src/parser/ast/rule/classification.rs @@ -15,6 +15,13 @@ use crate::parser::pattern::parse_pattern; use crate::parser::span_utils::shift_errors; use crate::{Span, SyntaxKind}; +/// Classify a raw rule-body literal into the structured rule-body term used by +/// semantic consumers. +/// +/// The caller provides the literal span so diagnostics can point back to the +/// CST node that supplied the raw text. `first_aggregation_span` is shared +/// across the surrounding rule body, allowing this helper to reject multiple +/// aggregations while still parsing each literal independently. pub(super) fn parse_rule_body_term( raw: &str, literal_span: Span, @@ -268,3 +275,48 @@ fn multiple_aggregations_error(_first_span: &Span, second_span: &Span) -> Simple "at most one aggregation (group_by or Aggregate) is permitted per rule body".to_string(), ) } + +#[cfg(test)] +mod tests { + #![expect(clippy::expect_used, reason = "tests assert exact parser output")] + + use super::*; + use chumsky::error::SimpleReason; + + fn span_for(src: &str) -> Span { + 0..src.len() + } + + #[test] + fn assignment_literal_becomes_assignment_term() { + let src = "var item = FlatMap(items)"; + let mut first_aggregation_span = None; + let mut errors = Vec::new(); + + let term = + parse_rule_body_term(src, span_for(src), &mut first_aggregation_span, &mut errors) + .expect("assignment should parse"); + + assert!(matches!(term, RuleBodyTerm::Assignment(_))); + assert!(errors.is_empty()); + } + + #[test] + fn second_aggregation_reports_error() { + let src = "group_by(count(), key)"; + let mut first_aggregation_span = Some(0..4); + let mut errors = Vec::new(); + + let term = + parse_rule_body_term(src, span_for(src), &mut first_aggregation_span, &mut errors); + + assert!(term.is_none()); + assert_eq!(errors.len(), 1); + let error = errors.first().expect("aggregation error missing"); + assert!(matches!( + error.reason(), + SimpleReason::Custom(message) + if message == "at most one aggregation (group_by or Aggregate) is permitted per rule body" + )); + } +} diff --git a/src/parser/expression/pratt/delay.rs b/src/parser/expression/pratt/delay.rs index 264e2200..06d9c1a5 100644 --- a/src/parser/expression/pratt/delay.rs +++ b/src/parser/expression/pratt/delay.rs @@ -13,6 +13,8 @@ impl Pratt<'_, I> where I: Iterator + Clone, { + /// Parse a `-` delay postfix, returning an [`Expr::AtomDelay`] around + /// the expression parsed before the postfix marker. pub(super) fn parse_delay_postfix(&mut self, lhs: Expr) -> Option { let (minus_kind, _) = self.ts.next_tok()?; debug_assert_eq!( @@ -48,3 +50,30 @@ where }) } } + +#[cfg(test)] +mod tests { + #![expect(clippy::expect_used, reason = "tests assert exact parser output")] + + use chumsky::error::SimpleReason; + + use crate::parser::expression::parse_expression; + + #[test] + fn parses_atom_delay_postfix() { + let expr = parse_expression("signal-<3>").expect("delay expression should parse"); + + assert_eq!(expr.to_sexpr(), "(delay 3 signal)"); + } + + #[test] + fn rejects_non_numeric_delay_value() { + let errors = + parse_expression("signal -< delay").expect_err("non-numeric delay should fail"); + + assert!(errors.iter().any(|error| matches!( + error.reason(), + SimpleReason::Custom(message) if message == "expected delay value" + ))); + } +} diff --git a/src/parser/expression/pratt/diff.rs b/src/parser/expression/pratt/diff.rs index 9ed6e0ad..927a9c76 100644 --- a/src/parser/expression/pratt/diff.rs +++ b/src/parser/expression/pratt/diff.rs @@ -12,6 +12,8 @@ impl Pratt<'_, I> where I: Iterator + Clone, { + /// Consume a diff marker and remember that the next completed postfix atom + /// should be wrapped in [`Expr::AtomDiff`]. pub(super) fn handle_diff_marker(&mut self, pending: &mut Option) { let (_, span) = self.ts.next_tok().unwrap_or_else(|| { let span = self.ts.eof_span(); @@ -24,6 +26,8 @@ where } } + /// Wrap `expr` in [`Expr::AtomDiff`] when a preceding diff marker is + /// pending, clearing that marker as part of the operation. pub(super) fn apply_pending_diff(expr: Expr, pending: &mut Option) -> Expr { if pending.take().is_some() { Expr::AtomDiff { @@ -34,6 +38,8 @@ where } } + /// Emit the diagnostic used when a pending diff marker cannot legally bind + /// to the postfix form about to be parsed. pub(super) fn emit_error_if_diff_pending(&mut self, pending: &mut Option) { if let Some(diff_span) = pending.take() { self.ts @@ -41,6 +47,7 @@ where } } + /// Ensure the postfix chain did not end with an unbound diff marker. pub(super) fn validate_no_pending_diff(&mut self, pending: &mut Option) -> Option<()> { if let Some(span) = pending.take() { self.ts @@ -50,3 +57,31 @@ where Some(()) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn apply_pending_diff_wraps_and_clears_marker() { + let mut pending = Some(0..1); + let expr = Pratt::<'_, std::vec::IntoIter<(SyntaxKind, Span)>>::apply_pending_diff( + Expr::Variable("atom".into()), + &mut pending, + ); + + assert!(matches!(expr, Expr::AtomDiff { .. })); + assert!(pending.is_none()); + } + + #[test] + fn apply_pending_diff_leaves_expression_when_no_marker_is_pending() { + let mut pending = None; + let expr = Pratt::<'_, std::vec::IntoIter<(SyntaxKind, Span)>>::apply_pending_diff( + Expr::Variable("atom".into()), + &mut pending, + ); + + assert_eq!(expr, Expr::Variable("atom".into())); + } +} diff --git a/src/parser/expression/pratt/postfix.rs b/src/parser/expression/pratt/postfix.rs index 2bd516b9..01eb5d46 100644 --- a/src/parser/expression/pratt/postfix.rs +++ b/src/parser/expression/pratt/postfix.rs @@ -131,6 +131,8 @@ where }) } + /// Parse a comma-separated argument list after the opening parenthesis has + /// already been consumed. pub(super) fn parse_args(&mut self) -> Option> { let mut args = Vec::new(); if matches!(self.ts.peek_kind(), Some(SyntaxKind::T_RPAREN)) { @@ -157,3 +159,31 @@ where Some(args) } } + +#[cfg(test)] +mod tests { + #![expect(clippy::expect_used, reason = "tests assert exact parser output")] + + use chumsky::error::SimpleReason; + + use crate::parser::expression::parse_expression; + + #[test] + fn qualified_callee_uses_call_postfix() { + let expr = + parse_expression("std::map(value)").expect("qualified function call should parse"); + + assert_eq!(expr.to_sexpr(), "(call std::map value)"); + } + + #[test] + fn trailing_argument_comma_is_rejected() { + let errors = parse_expression("f(1,)").expect_err("trailing comma should fail"); + + assert!(errors.iter().any(|error| matches!( + error.reason(), + SimpleReason::Custom(message) + if message == "unexpected trailing comma in argument list" + ))); + } +} From 82aad2988a2ead9a640aeededd7ddf40dc399eaf Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 19 May 2026 18:57:15 +0200 Subject: [PATCH 17/25] Clarify AtomDiff test helpers Align the `AtomDiff` and `AtomDelay` helper docs with the postfix node constructors used by parser fixtures. --- src/test_util/expressions.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test_util/expressions.rs b/src/test_util/expressions.rs index 5fd8a93f..eec48a6f 100644 --- a/src/test_util/expressions.rs +++ b/src/test_util/expressions.rs @@ -105,7 +105,7 @@ pub fn bit_slice(expr: Expr, hi: Expr, lo: Expr) -> Expr { } } -/// Construct an atom diff marker [`Expr::AtomDiff`]. +/// Construct an [`Expr::AtomDiff`] postfix node. #[must_use] pub fn atom_diff(expr: Expr) -> Expr { Expr::AtomDiff { @@ -113,7 +113,7 @@ pub fn atom_diff(expr: Expr) -> Expr { } } -/// Construct an atom delay marker [`Expr::AtomDelay`]. +/// Construct an [`Expr::AtomDelay`] postfix node with the given `delay` value. #[must_use] pub fn atom_delay(delay: u32, expr: Expr) -> Expr { Expr::AtomDelay { From 841ef5fb802a3d85f1fbe7d2f6971177ec42c76a Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 19 May 2026 18:58:52 +0200 Subject: [PATCH 18/25] Align postfix fixture helper body Remove the extra comment above `postfix_expression_cases` so the fixture module body matches the focused helper split requested for the large-method cleanup. --- src/parser/tests/expression/fixtures/valid/postfix.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/parser/tests/expression/fixtures/valid/postfix.rs b/src/parser/tests/expression/fixtures/valid/postfix.rs index 31976ef3..8d4ce5e7 100644 --- a/src/parser/tests/expression/fixtures/valid/postfix.rs +++ b/src/parser/tests/expression/fixtures/valid/postfix.rs @@ -111,7 +111,6 @@ fn delay_cases() -> Vec { ] } -/// Return parser test cases covering postfix expression forms. pub(super) fn postfix_expression_cases() -> Vec { let mut cases = field_and_method_call_cases(); cases.extend(call_and_slice_cases()); From a0508e59bc88ef0b75994233453e66935eb01dbb Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 19 May 2026 20:53:47 +0200 Subject: [PATCH 19/25] Bind comma token consume in postfix parser Make the comma consume in `parse_args` explicit by binding the returned `Option`, matching the other intentional token-consumption sites in the postfix parser. --- src/parser/expression/pratt/postfix.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/parser/expression/pratt/postfix.rs b/src/parser/expression/pratt/postfix.rs index 01eb5d46..ca3118b8 100644 --- a/src/parser/expression/pratt/postfix.rs +++ b/src/parser/expression/pratt/postfix.rs @@ -148,7 +148,7 @@ where if !matches!(self.ts.peek_kind(), Some(SyntaxKind::T_COMMA)) { break; } - self.ts.next_tok(); + let _ = self.ts.next_tok(); if matches!(self.ts.peek_kind(), Some(SyntaxKind::T_RPAREN)) { let span = self.ts.peek_span().unwrap_or_else(|| self.ts.eof_span()); self.ts From 1a6f9b82935b2e85368f5e18170acbe0cf7ec371 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 19 May 2026 21:47:54 +0200 Subject: [PATCH 20/25] Cover postfix diff and delay branches Add focused `parse_expression` tests for the Pratt postfix diff and delay paths, including the pending-diff guard before delay parsing. Narrow the `expect_used` Clippy expectations to the individual test cases that need them instead of suppressing the whole test module. --- src/parser/expression/pratt/postfix.rs | 42 ++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/src/parser/expression/pratt/postfix.rs b/src/parser/expression/pratt/postfix.rs index ca3118b8..894b4223 100644 --- a/src/parser/expression/pratt/postfix.rs +++ b/src/parser/expression/pratt/postfix.rs @@ -162,13 +162,13 @@ where #[cfg(test)] mod tests { - #![expect(clippy::expect_used, reason = "tests assert exact parser output")] - use chumsky::error::SimpleReason; + use crate::SyntaxKind; use crate::parser::expression::parse_expression; #[test] + #[expect(clippy::expect_used, reason = "test asserts exact parser output")] fn qualified_callee_uses_call_postfix() { let expr = parse_expression("std::map(value)").expect("qualified function call should parse"); @@ -177,6 +177,7 @@ mod tests { } #[test] + #[expect(clippy::expect_used, reason = "test asserts exact parser error")] fn trailing_argument_comma_is_rejected() { let errors = parse_expression("f(1,)").expect_err("trailing comma should fail"); @@ -186,4 +187,41 @@ mod tests { if message == "unexpected trailing comma in argument list" ))); } + + #[test] + #[expect(clippy::expect_used, reason = "test asserts exact parser output")] + fn parse_postfix_handles_diff_postfix() { + let expr = parse_expression("S'(x)").expect("diff postfix should parse"); + + assert_eq!(expr.to_sexpr(), "(diff (call S x))"); + } + + #[test] + #[expect(clippy::expect_used, reason = "test asserts exact parser output")] + fn parse_postfix_handles_delay_postfix() { + let expr = parse_expression("signal-<3>").expect("delay postfix should parse"); + + assert_eq!(expr.to_sexpr(), "(delay 3 signal)"); + } + + #[test] + #[expect(clippy::expect_used, reason = "test asserts exact parser error")] + fn parse_postfix_rejects_delay_when_diff_is_pending() { + let errors = parse_expression("signal'-<3>") + .expect_err("delay postfix with pending diff should fail"); + + assert!(has_custom_error( + &errors, + "diff marker must apply to an atom" + )); + } + + fn has_custom_error(errors: &[chumsky::error::Simple], expected: &str) -> bool { + errors.iter().any(|error| { + matches!( + error.reason(), + SimpleReason::Custom(message) if message == expected + ) + }) + } } From bdd4bd911043e82bbf0ecd889abb36814764ee50 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 26 May 2026 00:52:31 +0200 Subject: [PATCH 21/25] Report trailing comma errors at the separator Capture the comma span before consuming it in Pratt postfix argument parsing, then use that span for the trailing-comma diagnostic. Update the call parsing regression expectation so it verifies the diagnostic now points at the comma rather than the closing parenthesis. --- src/parser/expression/pratt/postfix.rs | 4 ++-- tests/expression_var_and_call.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/parser/expression/pratt/postfix.rs b/src/parser/expression/pratt/postfix.rs index 894b4223..5430a38e 100644 --- a/src/parser/expression/pratt/postfix.rs +++ b/src/parser/expression/pratt/postfix.rs @@ -148,11 +148,11 @@ where if !matches!(self.ts.peek_kind(), Some(SyntaxKind::T_COMMA)) { break; } + let sep_span = self.ts.peek_span().unwrap_or_else(|| self.ts.eof_span()); let _ = self.ts.next_tok(); if matches!(self.ts.peek_kind(), Some(SyntaxKind::T_RPAREN)) { - let span = self.ts.peek_span().unwrap_or_else(|| self.ts.eof_span()); self.ts - .push_error(span, "unexpected trailing comma in argument list"); + .push_error(sep_span, "unexpected trailing comma in argument list"); return None; } } diff --git a/tests/expression_var_and_call.rs b/tests/expression_var_and_call.rs index 26afc303..fa9330ec 100644 --- a/tests/expression_var_and_call.rs +++ b/tests/expression_var_and_call.rs @@ -38,8 +38,8 @@ enum Kind { #[case( "foo(x,)", "unexpected trailing comma in argument list", + 5, 6, - 7, Kind::Other )] #[case("foo(1 2)", "expected right paren", 6, 7, Kind::Other)] From 324810ec51a3491cadc0433d0ef1b6e777f71049 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 26 May 2026 00:55:57 +0200 Subject: [PATCH 22/25] Tighten postfix parser test coverage (#223) Capture consumed postfix separator spans explicitly and report trailing argument commas at the comma token. Remove the defensive diff-marker EOF fallback so the helper exposes the fallible token consume directly. Strengthen parser tests by checking the wrapped diff expression, assignment pattern and value details, and S-expression rendering beside snapshots. --- src/parser/ast/expr/sexpr.rs | 7 +++++-- src/parser/ast/rule/classification.rs | 6 +++++- src/parser/expression/pratt/diff.rs | 15 +++++++++------ src/parser/expression/pratt/postfix.rs | 13 ++++++------- 4 files changed, 25 insertions(+), 16 deletions(-) diff --git a/src/parser/ast/expr/sexpr.rs b/src/parser/ast/expr/sexpr.rs index 032eef9d..35787259 100644 --- a/src/parser/ast/expr/sexpr.rs +++ b/src/parser/ast/expr/sexpr.rs @@ -175,8 +175,10 @@ mod tests { }), rhs: Box::new(Expr::Literal(Literal::Bool(true))), }; + let rendered = expr.to_sexpr(); - insta::assert_snapshot!(expr.to_sexpr(), @"(+ (method stream map value) true)"); + assert_eq!(rendered, "(+ (method stream map value) true)"); + insta::assert_snapshot!(rendered, @"(+ (method stream map value) true)"); } #[test] @@ -185,9 +187,10 @@ mod tests { (var("key"), Expr::VecLit(vec![var("first"), var("second")])), (var("enabled"), Expr::Literal(Literal::Bool(false))), ]); + let rendered = expr.to_sexpr(); insta::assert_snapshot!( - expr.to_sexpr(), + rendered, @"(map (entry key (vec first second)) (entry enabled false))" ); } diff --git a/src/parser/ast/rule/classification.rs b/src/parser/ast/rule/classification.rs index 03cb316f..31375317 100644 --- a/src/parser/ast/rule/classification.rs +++ b/src/parser/ast/rule/classification.rs @@ -297,7 +297,11 @@ mod tests { parse_rule_body_term(src, span_for(src), &mut first_aggregation_span, &mut errors) .expect("assignment should parse"); - assert!(matches!(term, RuleBodyTerm::Assignment(_))); + let RuleBodyTerm::Assignment(assignment) = term else { + panic!("expected assignment term"); + }; + assert_eq!(assignment.pattern.to_source(), "var item"); + assert_eq!(assignment.value.to_sexpr(), "(call FlatMap items)"); assert!(errors.is_empty()); } diff --git a/src/parser/expression/pratt/diff.rs b/src/parser/expression/pratt/diff.rs index 927a9c76..cdfce394 100644 --- a/src/parser/expression/pratt/diff.rs +++ b/src/parser/expression/pratt/diff.rs @@ -14,16 +14,14 @@ where { /// Consume a diff marker and remember that the next completed postfix atom /// should be wrapped in [`Expr::AtomDiff`]. - pub(super) fn handle_diff_marker(&mut self, pending: &mut Option) { - let (_, span) = self.ts.next_tok().unwrap_or_else(|| { - let span = self.ts.eof_span(); - (SyntaxKind::N_ERROR, span) - }); + pub(super) fn handle_diff_marker(&mut self, pending: &mut Option) -> Option<()> { + let (_, span) = self.ts.next_tok()?; if pending.is_some() { self.ts.push_error(span, "duplicate diff marker"); } else { *pending = Some(span); } + Some(()) } /// Wrap `expr` in [`Expr::AtomDiff`] when a preceding diff marker is @@ -70,7 +68,12 @@ mod tests { &mut pending, ); - assert!(matches!(expr, Expr::AtomDiff { .. })); + assert_eq!( + expr, + Expr::AtomDiff { + expr: Box::new(Expr::Variable("atom".into())) + } + ); assert!(pending.is_none()); } diff --git a/src/parser/expression/pratt/postfix.rs b/src/parser/expression/pratt/postfix.rs index 5430a38e..657ed596 100644 --- a/src/parser/expression/pratt/postfix.rs +++ b/src/parser/expression/pratt/postfix.rs @@ -19,7 +19,7 @@ where loop { match self.ts.peek_kind() { Some(SyntaxKind::T_APOSTROPHE) => { - self.handle_diff_marker(&mut pending_diff_span); + self.handle_diff_marker(&mut pending_diff_span)?; continue; } Some(SyntaxKind::T_LPAREN) => { @@ -61,7 +61,7 @@ where } fn parse_bit_slice_postfix(&mut self, lhs: Expr) -> Option { - let _ = self.ts.next_tok(); + let (_, _slice_span) = self.ts.next_tok()?; self.with_struct_literals_suspended(move |this| { let hi = this.parse_expr(0)?; if !this.ts.expect(SyntaxKind::T_COMMA) { @@ -81,7 +81,7 @@ where fn parse_dot_access_postfix(&mut self, lhs: Expr) -> Option { // token is consumed to advance past '.' - let _ = self.ts.next_tok(); + let (_, _dot_span) = self.ts.next_tok()?; match self.ts.next_tok() { Some((SyntaxKind::T_IDENT, span)) => { let name = self.ts.slice(&span); @@ -121,7 +121,7 @@ where fn parse_parenthesized_args(&mut self) -> Option> { // token is consumed to advance past '(' - let _ = self.ts.next_tok(); + let (_, _open_span) = self.ts.next_tok()?; self.with_struct_literals_suspended(|this| { let args = this.parse_args()?; if !this.ts.expect(SyntaxKind::T_RPAREN) { @@ -148,11 +148,10 @@ where if !matches!(self.ts.peek_kind(), Some(SyntaxKind::T_COMMA)) { break; } - let sep_span = self.ts.peek_span().unwrap_or_else(|| self.ts.eof_span()); - let _ = self.ts.next_tok(); + let (_, comma_span) = self.ts.next_tok()?; if matches!(self.ts.peek_kind(), Some(SyntaxKind::T_RPAREN)) { self.ts - .push_error(sep_span, "unexpected trailing comma in argument list"); + .push_error(comma_span, "unexpected trailing comma in argument list"); return None; } } From dab06f29e401b2399fbedb6a921338544bf9eadd Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 26 May 2026 10:37:49 +0200 Subject: [PATCH 23/25] Strengthen postfix parser assertions (#223) Assert the trailing-comma diagnostic span, unwrap the diff postfix AST to check the completed call it wraps, and add a direct expected rendering beside the collection S-expression snapshot. --- src/parser/ast/expr/sexpr.rs | 4 ++++ src/parser/expression/pratt/postfix.rs | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/src/parser/ast/expr/sexpr.rs b/src/parser/ast/expr/sexpr.rs index 35787259..5b0abb3f 100644 --- a/src/parser/ast/expr/sexpr.rs +++ b/src/parser/ast/expr/sexpr.rs @@ -189,6 +189,10 @@ mod tests { ]); let rendered = expr.to_sexpr(); + assert_eq!( + rendered, + "(map (entry key (vec first second)) (entry enabled false))" + ); insta::assert_snapshot!( rendered, @"(map (entry key (vec first second)) (entry enabled false))" diff --git a/src/parser/expression/pratt/postfix.rs b/src/parser/expression/pratt/postfix.rs index 657ed596..d69eec4a 100644 --- a/src/parser/expression/pratt/postfix.rs +++ b/src/parser/expression/pratt/postfix.rs @@ -164,6 +164,7 @@ mod tests { use chumsky::error::SimpleReason; use crate::SyntaxKind; + use crate::parser::ast::Expr; use crate::parser::expression::parse_expression; #[test] @@ -185,6 +186,7 @@ mod tests { SimpleReason::Custom(message) if message == "unexpected trailing comma in argument list" ))); + assert!(errors.iter().any(|error| error.span() == (3..4))); } #[test] @@ -193,6 +195,10 @@ mod tests { let expr = parse_expression("S'(x)").expect("diff postfix should parse"); assert_eq!(expr.to_sexpr(), "(diff (call S x))"); + let Expr::AtomDiff { expr } = expr else { + panic!("expected diff postfix to wrap the completed call"); + }; + assert_eq!(expr.to_sexpr(), "(call S x)"); } #[test] From de3e89f7833c6b131492a07863fb0baa0f6a5ecf Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 26 May 2026 11:56:51 +0200 Subject: [PATCH 24/25] Narrow classification test lint expectations (#223) Move the `clippy::expect_used` expectation from the whole classification test module to the individual tests that call `expect`. Keep the lint suppression scoped to the exact assertions that need it. --- src/parser/ast/rule/classification.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/parser/ast/rule/classification.rs b/src/parser/ast/rule/classification.rs index 31375317..160da6f1 100644 --- a/src/parser/ast/rule/classification.rs +++ b/src/parser/ast/rule/classification.rs @@ -278,8 +278,6 @@ fn multiple_aggregations_error(_first_span: &Span, second_span: &Span) -> Simple #[cfg(test)] mod tests { - #![expect(clippy::expect_used, reason = "tests assert exact parser output")] - use super::*; use chumsky::error::SimpleReason; @@ -288,6 +286,7 @@ mod tests { } #[test] + #[expect(clippy::expect_used, reason = "test asserts exact parser output")] fn assignment_literal_becomes_assignment_term() { let src = "var item = FlatMap(items)"; let mut first_aggregation_span = None; @@ -306,6 +305,7 @@ mod tests { } #[test] + #[expect(clippy::expect_used, reason = "test asserts exact parser error")] fn second_aggregation_reports_error() { let src = "group_by(count(), key)"; let mut first_aggregation_span = Some(0..4); From 81c28f5768771b5e2cfb752841aa3ed63e30796a Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 26 May 2026 15:05:41 +0200 Subject: [PATCH 25/25] Cover parser fixture docs and postfix helpers (#223) Expand expression fixture module documentation so each fixture group states its role in the parser test suite and how it relates to the shared fixture aggregators. Add direct Pratt helper coverage for diff-marker error paths and delay postfix boundary parsing, including the maximum `u32` delay and invalid delay values. --- src/parser/expression/pratt/delay.rs | 65 +++++++++++++++++- src/parser/expression/pratt/diff.rs | 68 +++++++++++++++++++ src/parser/tests/expression/fixtures.rs | 6 +- .../tests/expression/fixtures/errors.rs | 6 +- src/parser/tests/expression/fixtures/valid.rs | 5 +- .../tests/expression/fixtures/valid/basic.rs | 5 +- .../expression/fixtures/valid/collections.rs | 4 ++ .../expression/fixtures/valid/control_flow.rs | 5 +- .../expression/fixtures/valid/postfix.rs | 3 + .../expression/fixtures/valid/structured.rs | 6 +- 10 files changed, 165 insertions(+), 8 deletions(-) diff --git a/src/parser/expression/pratt/delay.rs b/src/parser/expression/pratt/delay.rs index 06d9c1a5..385be9aa 100644 --- a/src/parser/expression/pratt/delay.rs +++ b/src/parser/expression/pratt/delay.rs @@ -53,13 +53,15 @@ where #[cfg(test)] mod tests { - #![expect(clippy::expect_used, reason = "tests assert exact parser output")] - use chumsky::error::SimpleReason; + use crate::parser::ast::Expr; use crate::parser::expression::parse_expression; + use crate::tokenize_without_trivia; + use crate::{Span, SyntaxKind}; #[test] + #[expect(clippy::expect_used, reason = "test asserts exact parser output")] fn parses_atom_delay_postfix() { let expr = parse_expression("signal-<3>").expect("delay expression should parse"); @@ -67,6 +69,7 @@ mod tests { } #[test] + #[expect(clippy::expect_used, reason = "test asserts exact parser error")] fn rejects_non_numeric_delay_value() { let errors = parse_expression("signal -< delay").expect_err("non-numeric delay should fail"); @@ -76,4 +79,62 @@ mod tests { SimpleReason::Custom(message) if message == "expected delay value" ))); } + + #[test] + #[expect(clippy::expect_used, reason = "test asserts exact parser output")] + fn parse_delay_postfix_accepts_max_u32_delay() { + let mut parser = delay_parser("-<4294967295>"); + + let expr = parser + .parse_delay_postfix(Expr::Variable("signal".into())) + .expect("u32 max delay should parse"); + + assert_eq!(expr.to_sexpr(), "(delay 4294967295 signal)"); + } + + #[test] + fn parse_delay_postfix_rejects_delay_above_u32_max() { + let mut parser = delay_parser("-<4294967296>"); + + let expr = parser.parse_delay_postfix(Expr::Variable("signal".into())); + + assert!(expr.is_none()); + let errors = parser.ts.take_errors(); + assert!(has_custom_error(&errors, "delay must fit u32")); + } + + #[test] + fn parse_delay_postfix_rejects_empty_delay_value() { + let mut parser = delay_parser("-<>"); + + let expr = parser.parse_delay_postfix(Expr::Variable("signal".into())); + + assert!(expr.is_none()); + let errors = parser.ts.take_errors(); + assert!(has_custom_error(&errors, "expected delay value")); + } + + #[test] + fn parse_delay_postfix_rejects_negative_delay_value() { + let mut parser = delay_parser("-<-1>"); + + let expr = parser.parse_delay_postfix(Expr::Variable("signal".into())); + + assert!(expr.is_none()); + let errors = parser.ts.take_errors(); + assert!(has_custom_error(&errors, "expected delay value")); + } + + fn delay_parser(src: &str) -> super::Pratt<'_, std::vec::IntoIter<(SyntaxKind, Span)>> { + super::Pratt::new(tokenize_without_trivia(src).into_iter(), src) + } + + fn has_custom_error(errors: &[chumsky::error::Simple], expected: &str) -> bool { + errors.iter().any(|error| { + matches!( + error.reason(), + SimpleReason::Custom(message) if message == expected + ) + }) + } } diff --git a/src/parser/expression/pratt/diff.rs b/src/parser/expression/pratt/diff.rs index cdfce394..fcf25dd5 100644 --- a/src/parser/expression/pratt/diff.rs +++ b/src/parser/expression/pratt/diff.rs @@ -59,6 +59,7 @@ where #[cfg(test)] mod tests { use super::*; + use chumsky::error::SimpleReason; #[test] fn apply_pending_diff_wraps_and_clears_marker() { @@ -87,4 +88,71 @@ mod tests { assert_eq!(expr, Expr::Variable("atom".into())); } + + #[test] + fn handle_diff_marker_reports_duplicate_marker() { + let mut parser = Pratt::new( + vec![ + (SyntaxKind::T_APOSTROPHE, 0..1), + (SyntaxKind::T_APOSTROPHE, 1..2), + ] + .into_iter(), + "''", + ); + let mut pending = None; + + assert_eq!(parser.handle_diff_marker(&mut pending), Some(())); + assert_eq!(pending, Some(0..1)); + assert_eq!(parser.handle_diff_marker(&mut pending), Some(())); + + assert_eq!(pending, Some(0..1)); + let errors = parser.ts.take_errors(); + assert!(has_custom_error(&errors, "duplicate diff marker", 1..2)); + } + + #[test] + fn emit_error_if_diff_pending_reports_and_clears_marker() { + let mut parser = Pratt::new(Vec::new().into_iter(), ""); + let mut pending = Some(2..3); + + parser.emit_error_if_diff_pending(&mut pending); + + assert!(pending.is_none()); + let errors = parser.ts.take_errors(); + assert!(has_custom_error( + &errors, + "diff marker must apply to an atom", + 2..3 + )); + } + + #[test] + fn validate_no_pending_diff_reports_unbound_marker() { + let mut parser = Pratt::new(Vec::new().into_iter(), ""); + let mut pending = Some(4..5); + + assert_eq!(parser.validate_no_pending_diff(&mut pending), None); + + assert!(pending.is_none()); + let errors = parser.ts.take_errors(); + assert!(has_custom_error( + &errors, + "diff marker must be followed by atom arguments", + 4..5 + )); + } + + fn has_custom_error( + errors: &[chumsky::error::Simple], + expected: &str, + span: Span, + ) -> bool { + errors.iter().any(|error| { + error.span() == span + && matches!( + error.reason(), + SimpleReason::Custom(message) if message == expected + ) + }) + } } diff --git a/src/parser/tests/expression/fixtures.rs b/src/parser/tests/expression/fixtures.rs index aaed9d57..383bd121 100644 --- a/src/parser/tests/expression/fixtures.rs +++ b/src/parser/tests/expression/fixtures.rs @@ -1,4 +1,8 @@ -//! Shared fixture data for expression parser tests. +//! Shared fixture definitions and accessors for expression parser tests. +//! +//! This module defines the case types used by the valid and error fixture +//! groups, then exposes the grouped lists consumed by the parent expression +//! parser test modules. mod errors; mod valid; diff --git a/src/parser/tests/expression/fixtures/errors.rs b/src/parser/tests/expression/fixtures/errors.rs index 6facd60d..32b8034f 100644 --- a/src/parser/tests/expression/fixtures/errors.rs +++ b/src/parser/tests/expression/fixtures/errors.rs @@ -1,4 +1,8 @@ -//! Error-case fixtures for expression parser tests. +//! Aggregated invalid-expression fixtures for parser tests. +//! +//! These cases are grouped by diagnostic style so the parent expression suite +//! can assert error counts, spans, and postfix delimiter recovery without +//! duplicating fixture wiring. use crate::parser::tests::expression::fixtures::{ CountedErrorCase, PostfixErrorCase, SpannedErrorCase, diff --git a/src/parser/tests/expression/fixtures/valid.rs b/src/parser/tests/expression/fixtures/valid.rs index 578b9070..74263026 100644 --- a/src/parser/tests/expression/fixtures/valid.rs +++ b/src/parser/tests/expression/fixtures/valid.rs @@ -1,4 +1,7 @@ -//! Valid expression fixtures for parser tests. +//! Aggregated valid-expression fixtures for parser tests. +//! +//! Each child module covers one syntactic area, and this module combines those +//! groups into the ordered case lists exercised by the expression parser suite. mod basic; mod collections; diff --git a/src/parser/tests/expression/fixtures/valid/basic.rs b/src/parser/tests/expression/fixtures/valid/basic.rs index 618d46d3..45fe7d59 100644 --- a/src/parser/tests/expression/fixtures/valid/basic.rs +++ b/src/parser/tests/expression/fixtures/valid/basic.rs @@ -1,4 +1,7 @@ -//! Basic expression fixtures that do not need specialised helper groupings. +//! Core expression fixtures for the simplest valid forms. +//! +//! These cases cover atoms, calls, tuples, and literals that the parser must +//! accept before the specialised fixture modules add broader syntax coverage. use crate::parser::ast::Expr; use crate::parser::tests::expression::fixtures::ExpressionCase; diff --git a/src/parser/tests/expression/fixtures/valid/collections.rs b/src/parser/tests/expression/fixtures/valid/collections.rs index 02ff33ce..33b33b13 100644 --- a/src/parser/tests/expression/fixtures/valid/collections.rs +++ b/src/parser/tests/expression/fixtures/valid/collections.rs @@ -1,4 +1,8 @@ //! Collection literal fixtures for expression parser tests. +//! +//! These cases exercise list and map forms, including nested expressions, +//! trailing commas, and key-value pairs that feed the shared valid-expression +//! suite. use crate::parser::ast::{BinaryOp, Expr}; use crate::parser::tests::expression::fixtures::ExpressionCase; diff --git a/src/parser/tests/expression/fixtures/valid/control_flow.rs b/src/parser/tests/expression/fixtures/valid/control_flow.rs index 55ce6853..60d0c05a 100644 --- a/src/parser/tests/expression/fixtures/valid/control_flow.rs +++ b/src/parser/tests/expression/fixtures/valid/control_flow.rs @@ -1,4 +1,7 @@ -//! Control-flow expression fixtures for parser tests. +//! Control-flow fixtures for expression parser tests. +//! +//! These grouped cases cover `if`, `match`, and `for` expressions, including +//! nested forms that build larger ASTs for the shared valid-expression suite. use crate::parser::ast::{BinaryOp, Expr}; use crate::parser::tests::expression::fixtures::ExpressionCase; diff --git a/src/parser/tests/expression/fixtures/valid/postfix.rs b/src/parser/tests/expression/fixtures/valid/postfix.rs index 8d4ce5e7..24808a37 100644 --- a/src/parser/tests/expression/fixtures/valid/postfix.rs +++ b/src/parser/tests/expression/fixtures/valid/postfix.rs @@ -1,4 +1,7 @@ //! Postfix-expression fixtures for parser tests. +//! +//! These cases cover field access, method calls, indexing, diff markers, and +//! delay syntax that extend an already parsed atom in the shared fixture suite. use crate::parser::ast::Expr; use crate::parser::tests::expression::fixtures::ExpressionCase; diff --git a/src/parser/tests/expression/fixtures/valid/structured.rs b/src/parser/tests/expression/fixtures/valid/structured.rs index af2645c9..d6c8cc85 100644 --- a/src/parser/tests/expression/fixtures/valid/structured.rs +++ b/src/parser/tests/expression/fixtures/valid/structured.rs @@ -1,4 +1,8 @@ -//! Struct and closure fixtures for expression parser tests. +//! Structured-expression fixtures for parser tests. +//! +//! These cases cover struct literals and closures, including grouped +//! expressions inside compound forms and parameter lists used by the parent +//! expression suite. use crate::parser::ast::{BinaryOp, Expr}; use crate::parser::tests::expression::fixtures::ExpressionCase;