Re: [PATCH proxmox 01/29] add new proxmox-match-expression crate

["Lukas Wagner" <l.wagner@proxmox.com>]

On Fri Jul 24, 2026 at 8:46 AM CEST, Arthur Bied-Charreton wrote:
> On Thu, Jul 09, 2026 at 01:56:48PM +0200, Lukas Wagner wrote:
>> This new crate is a generic implementation of a serializable expression
>> language. It supports basic combinators, such as AnyOf, OneOf and AllOf,
>> constants, Not, as well as custom matcher leave nodes that are injected
>> via a generic type parameter.
>> 
>> The first user of this implementation will be the notification stack.
>> 
> i really like this crate, great job! 

Thanks!

> beeing able to serialize evaluated
> expressions is a cool feature for notifications history. some comments
> inline. 

[...]

>> +
>> +/// Evaluated expression resulting from [`Expression::evaluate`].
>> +///
>> +/// This type contains the original expression, augmented with per-node
>> +/// results. This is useful for recording 'traces', e.g. for recording
>> +/// exactly why a notification matcher matched a notification or not.
>> +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)]
>> +pub struct EvaluatedExpressionWithResult<M> {
>> +    /// The actual expression.
>> +    #[serde(flatten)]
>> +    expression: EvaluatedExpression<M>,
>> +    /// The evaluated expression matched the notification.
> since this crate is meant to be generic, we might want to drop
> 'notification' in favor of 'input' or something like that
>> +    matches: bool,
>> +}
>> +
>> +impl<M> EvaluatedExpressionWithResult<M> {
>> +    /// Return whether the expression matched the notification.
> here as well

Done, thanks. This was a leftover from when this was still inside
proxmox-notify.

>> +    pub fn is_match(&self) -> bool {
>> +        self.matches
>> +    }
>> +
>> +    /// Borrow the contained [`EvaluatedExpression`].
>> +    pub fn expression(&self) -> &EvaluatedExpression<M> {
>> +        &self.expression
>> +    }
>> +
>> +    /// Return the contained [`EvaluatedExpression`], consuming `self`.
>> +    pub fn into_expression(self) -> EvaluatedExpression<M> {
>> +        self.expression
>> +    }
> nit: is there any reason why this is not an impl From<A> for B? if not i
> would personally find such an API more idiomatic

Will add a From implementation in v2, but I will keep into_expression
and have `from` call just that

>> +
>> +    /// The direct child nodes, uniform across all combinator kinds.
>> +    ///
>> +    /// 'not' yields its single operand as a one-element slice; leaf nodes
>> +    /// ('constant', 'match') yield an empty slice. This lets a generic tree
>> +    /// walker recurse without special-casing each variant.
>> +    pub fn children(&self) -> &[EvaluatedExpressionWithResult<M>] {
>> +        match &self.expression {
>> +            EvaluatedExpression::AllOf(expressions)
>> +            | EvaluatedExpression::AnyOf(expressions)
>> +            | EvaluatedExpression::OneOf(expressions) => expressions,
>> +            EvaluatedExpression::Not(expression) => std::slice::from_ref(&**expression),
>> +            EvaluatedExpression::Constant(_) | EvaluatedExpression::Match(_) => &[],
>> +        }
>> +    }
>> +}
>> +
>> +impl<M, D, E> Expression<M>
>> +where
>> +    M: Clone + MatchExpression<Data = D, Error = E>,
>> +{
>> +    /// Evaluate this expression against a provided variable.
>> +    ///
>> +    /// This function returns an `EvaluatedExpression`, replicating the exact structure
>> +    /// of `self`, but annotated with a per-subexpression trace to record which of
>> +    /// the subexpressions matched or not.
>> +    ///
>> +    /// # Note
>> +    /// This evaluates the expression and *all* sub-expressions fully and does not return
>> +    /// early if the final result is already determined by the already evaluated sub-expressions
>> +    /// (for instance, [`Expression::AnyOf`] and the first sub-expression matched).
>> +    /// This is done so that we can return the full trace via the [`EvaluatedExpressionWithResult`]
>> +    /// type.
>> +    ///
>> +    /// If you need fast evaluation and do not care about the trace, feel free to add a
>> +    /// `evaluate_fast` (or similar), which does early returns as appropriate and simply returns
>> +    /// `bool`.
>> +    pub fn evaluate(&self, data: &D) -> Result<EvaluatedExpressionWithResult<M>, E> {
>> +        let evaluated_expression = match self {
>> +            Expression::AllOf(expressions) => {
>> +                if expressions.is_empty() {
>> +                    // 'all-of' without any sub-expressions is *not* a match
>> +                    EvaluatedExpressionWithResult {
>> +                        expression: EvaluatedExpression::AllOf(Vec::new()),
>> +                        matches: false,
>> +                    }
> this probaly does not matter too much as long as it's documented, but
> intuitively (and based on implementations for all-expressions in 
> other languages like rust and python) i would expect all([]) to evaluate to
> true.

Yeah, I see where you are coming from, but after some consideration I've
decided that I'd like to keep it this way.

For non-coders, all([]) being true is not very intuitive, and I'd prefer
to just say "hey, all combinators eval to false if they have no
children" in the docs.

>> +                } else {
>> +                    let (matches, evaluated_expressions) =
>> +                        Self::evaluate_subexpressions(data, expressions, true, |a, b| a && b)?;
>> +
>> +                    EvaluatedExpressionWithResult {
>> +                        expression: EvaluatedExpression::AllOf(evaluated_expressions),
>> +                        matches,
>> +                    }
>> +                }
>> +            }

[...]

>> +
>> +#[cfg(test)]
>> +mod test {
>> +    use super::*;
>> +
>> +    #[derive(Serialize, Deserialize, Debug, Clone)]
>> +    #[serde(rename_all = "kebab-case", tag = "type")]
>> +    pub enum TestMatcher {
>> +        CustomFailure,
>> +        CustomTrue,
>> +        CustomFalse,
>> +    }
> nit: this enum does not pass clippy because of the common prefix

fixed, thanks!

>> +
>> +    impl MatchExpression for TestMatcher {
>> +        type Data = ();
>> +        type Error = ();
>> +
>> +        fn evaluate(&self, _data: &Self::Data) -> Result<bool, ()> {
>> +            match self {
>> +                TestMatcher::CustomFailure => Err(()),
>> +                TestMatcher::CustomTrue => Ok(true),
>> +                TestMatcher::CustomFalse => Ok(false),
>> +            }
>> +        }
>> +    }
>> +
> [...]