From d145a77a2d6550fa3e1ce8a2098b0f426979562c Mon Sep 17 00:00:00 2001 From: Rain Date: Sun, 29 Mar 2020 13:40:37 -0700 Subject: [PATCH] Fix nested any/all evaluation The current evaluator pops everything on the stack in order to evaluate `any` or `all` queries. That breaks in nested situations like: ``` all(any(unix, target_arch="x86"), not(any(target_os="android", target_os="emscripten"))) ``` Fix this by tracking the number of predicates in the `Any` and `All` variants. This unfortunately breaks semver because the definition of `Func` changes. `Func` doesn't seem to be exported anywhere so maybe in the upcoming release it could be made private? --- src/error.rs | 2 +- src/expr/mod.rs | 22 ++++++++++++++-------- src/expr/parser.rs | 19 +++++++++++-------- tests/eval.rs | 11 +++++++++++ 4 files changed, 37 insertions(+), 17 deletions(-) diff --git a/src/error.rs b/src/error.rs index 35f3683..3f4dbe5 100644 --- a/src/error.rs +++ b/src/error.rs @@ -16,7 +16,7 @@ pub struct ParseError<'a> { #[derive(Debug, PartialEq)] pub enum Reason { /// not() takes exactly 1 predicate, unlike all() and any() - InvalidNot(u8), + InvalidNot(usize), /// The characters are not valid in an SDPX license expression InvalidCharacters, /// An opening parens was unmatched with a closing parens diff --git a/src/expr/mod.rs b/src/expr/mod.rs index 3321a24..66f3039 100644 --- a/src/expr/mod.rs +++ b/src/expr/mod.rs @@ -13,11 +13,15 @@ pub enum Func { /// `all()` with a comma separated list of configuration predicates. It /// is false if at least one predicate is false. If there are no predicates, /// it is true. - All, + /// + /// The associated `usize` is the number of predicates inside the `all()`. + All(usize), /// `any()` with a comma separated list of configuration predicates. It /// is true if at least one predicate is true. If there are no predicates, /// it is false. - Any, + /// + /// The associated `usize` is the number of predicates inside the `any()`. + Any(usize), } use crate::targets as targ; @@ -192,26 +196,28 @@ impl Expression { let pred = pred.to_pred(&self.original); result_stack.push(eval_predicate(&pred)); } - ExprNode::Fn(Func::All) => { + ExprNode::Fn(Func::All(count)) => { // all() with a comma separated list of configuration predicates. // It is false if at least one predicate is false. // If there are no predicates, it is true. let mut result = true; - while let Some(rs) = result_stack.pop() { - result = result && rs; + for _ in 0..*count { + let r = result_stack.pop().unwrap(); + result = result && r; } result_stack.push(result); } - ExprNode::Fn(Func::Any) => { + ExprNode::Fn(Func::Any(count)) => { // any() with a comma separated list of configuration predicates. // It is true if at least one predicate is true. // If there are no predicates, it is false. let mut result = false; - while let Some(rs) = result_stack.pop() { - result = result || rs; + for _ in 0..*count { + let r = result_stack.pop().unwrap(); + result = result || r; } result_stack.push(result); diff --git a/src/expr/parser.rs b/src/expr/parser.rs index 884427a..2f097d8 100644 --- a/src/expr/parser.rs +++ b/src/expr/parser.rs @@ -21,6 +21,7 @@ impl Expression { // the string before we start walking tokens let original = lexer.inner; + #[derive(Debug)] struct FuncAndSpan { func: Func, parens_index: usize, @@ -248,8 +249,10 @@ impl Expression { Token::All | Token::Any | Token::Not => match last_token { None | Some(Token::OpenParen) | Some(Token::Comma) => { let new_fn = match lt.token { - Token::All => Func::All, - Token::Any => Func::Any, + // the 0 is a dummy value -- it will be substituted for the real + // number of predicates in the `CloseParen` branch below. + Token::All => Func::All(0), + Token::Any => Func::Any(0), Token::Not => Func::Not, _ => unreachable!(), }; @@ -284,16 +287,16 @@ impl Expression { let key = pred_key.take(); let val = pred_val.take(); + let num_predicates = top.predicates.len() + + if key.is_some() { 1 } else { 0 } + + top.nest_level as usize; + let func = match top.func { - Func::All => Func::All, - Func::Any => Func::Any, + Func::All(_) => Func::All(num_predicates), + Func::Any(_) => Func::Any(num_predicates), Func::Not => { // not() doesn't take a predicate list, but only a single predicate, // so ensure we have exactly 1 - let num_predicates = top.predicates.len() as u8 - + if key.is_some() { 1 } else { 0 } - + top.nest_level; - if num_predicates != 1 { return Err(ParseError { original, diff --git a/tests/eval.rs b/tests/eval.rs index 11f3bce..9590934 100644 --- a/tests/eval.rs +++ b/tests/eval.rs @@ -127,6 +127,17 @@ fn complex() { assert!(complex.eval(|pred| tg_match!(pred, windows_msvc))); assert!(complex.eval(|pred| tg_match!(pred, mac))); assert!(!complex.eval(|pred| tg_match!(pred, android))); + + let complex = Expression::parse(r#"all(any(unix, target_arch="x86"), not(any(target_os="android", target_os="emscripten")))"#).unwrap(); + + // Should match linuxes and mac + assert!(complex.eval(|pred| tg_match!(pred, linux_gnu))); + assert!(complex.eval(|pred| tg_match!(pred, linux_musl))); + assert!(complex.eval(|pred| tg_match!(pred, mac))); + + // Should *not* match x86_64 windows or android + assert!(!complex.eval(|pred| tg_match!(pred, windows_msvc))); + assert!(!complex.eval(|pred| tg_match!(pred, android))); } #[test]