From 31dadd47f604b6965a5e2ba7e194a0d0f512df8e Mon Sep 17 00:00:00 2001 From: Jeremy Wall Date: Thu, 7 Sep 2017 14:29:32 -0500 Subject: [PATCH] Add a validation step for Macros. Ensure that only symbols declared in the macro arguments are used in the expressions. --- src/ast.rs | 176 +++++++++++++++++++++++++++++++++++++++++++++++++++ src/build.rs | 10 ++- 2 files changed, 183 insertions(+), 3 deletions(-) diff --git a/src/ast.rs b/src/ast.rs index 76b994e..d7c711c 100644 --- a/src/ast.rs +++ b/src/ast.rs @@ -11,6 +11,9 @@ // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. // See the License for the specific language governing permissions and // limitations under the License. +use std::collections::HashSet; +use std::borrow::Borrow; + pub type FieldList = Vec<(String, Expression)>; // str is expected to be a symbol pub type SelectorList = Vec; // str is expected to always be a symbol. @@ -90,6 +93,110 @@ pub struct MacroDef { pub fields: FieldList, } +impl MacroDef { + fn validate_value_symbols<'a>(&'a self, stack: &mut Vec<&'a Expression>, val: &'a Value) -> HashSet { + let mut bad_symbols = HashSet::new(); + if let &Value::Symbol(ref name) = val { + let mut ok = true; + for arg in self.argdefs.iter() { + ok &= arg == name + } + if !ok { + bad_symbols.insert(name.clone()); + } + } else if let &Value::Selector(ref list) = val { + let mut ok = true; + if list.len() > 0 { + // We only look to see if the first selector item exists. + // This is because only the first one is a symbol all of the + // rest of the items in the selector are fields in a tuple. + // But we don't know at this time of the value passed into + // this macro is a tuple since this isn't a callsite. + for arg in self.argdefs.iter() { + ok &= arg == &list[0] + } + if !ok { + bad_symbols.insert(list[0].clone()); + } + } + } else if let &Value::Tuple(ref fields) = val { + for &(_, ref expr) in fields.iter() { + stack.push(expr); + } + } + return bad_symbols; + } + + pub fn validate_symbols(&self) -> Result<(), HashSet> { + let mut bad_symbols = HashSet::new(); + for &(_, ref expr) in self.fields.iter() { + let mut stack = Vec::new(); + stack.push(expr); + while stack.len() > 0 { + match stack.pop().unwrap() { + &Expression::Add(ref bexpr) => { + let mut syms_set = self.validate_value_symbols(&mut stack, &bexpr.0); + bad_symbols.extend(syms_set.drain()); + stack.push(&bexpr.1); + }, + &Expression::Sub(ref bexpr) => { + let mut syms_set = self.validate_value_symbols(&mut stack, &bexpr.0); + bad_symbols.extend(syms_set.drain()); + stack.push(&bexpr.1); + }, + &Expression::Mul(ref bexpr) => { + let mut syms_set = self.validate_value_symbols(&mut stack, &bexpr.0); + bad_symbols.extend(syms_set.drain()); + stack.push(&bexpr.1); + }, + &Expression::Div(ref bexpr) => { + let mut syms_set = self.validate_value_symbols(&mut stack, &bexpr.0); + bad_symbols.extend(syms_set.drain()); + stack.push(&bexpr.1); + }, + &Expression::Grouped(ref expr) => { + stack.push(expr); + }, + &Expression::Format(_, ref exprs) => { + for arg_expr in exprs.iter() { + stack.push(arg_expr); + } + }, + &Expression::Select(ref def) => { + stack.push(def.default.borrow()); + stack.push(def.val.borrow()); + for &(_, ref expr) in def.tuple.iter() { + stack.push(expr); + } + }, + &Expression::Copy(_, ref fields) => { + for &(_, ref expr) in fields.iter() { + stack.push(expr); + } + }, + &Expression::Call(ref def) => { + for expr in def.arglist.iter() { + stack.push(expr); + } + } + &Expression::Simple(ref val) => { + let mut syms_set = self.validate_value_symbols(&mut stack, val); + bad_symbols.extend(syms_set.drain()); + }, + &Expression::Macro(_) => { + // noop + continue; + }, + } + } + } + if bad_symbols.len() > 0 { + return Err(bad_symbols); + } + return Ok(()) + } +} + /// BinaryExpression represents an expression with a left and a right side. #[derive(Debug,PartialEq,Clone)] pub struct BinaryExpression(pub Value, pub Box); @@ -137,3 +244,72 @@ pub enum Statement { name: String, }, } + +#[cfg(test)] +mod ast_test { + use super::*; + + #[test] + pub fn test_macro_validation_happy_path() { + let def = MacroDef{ + argdefs: vec![ + "foo".to_string() + ], + fields: vec![ + ("f1".to_string(), Expression::Add(BinaryExpression( + Value::Symbol("foo".to_string()), + Box::new(Expression::Simple(Value::Int(1)))))), + ], + }; + assert!(def.validate_symbols().unwrap() == ()); + } + + #[test] + pub fn test_macro_validation_fail() { + let def = MacroDef{ + argdefs: vec![ + "foo".to_string() + ], + fields: vec![ + ("f1".to_string(), Expression::Add(BinaryExpression( + Value::Symbol("bar".to_string()), + Box::new(Expression::Simple(Value::Int(1)))))), + ], + }; + let mut expected = HashSet::new(); + expected.insert("bar".to_string()); + assert_eq!(def.validate_symbols().err().unwrap(), expected); + } + + #[test] + pub fn test_macro_validation_selector_happy_path() { + let def = MacroDef{ + argdefs: vec![ + "foo".to_string() + ], + fields: vec![ + ("f1".to_string(), Expression::Add(BinaryExpression( + Value::Selector(vec!["foo".to_string(), "quux".to_string()]), + Box::new(Expression::Simple(Value::Int(1)))))), + ], + }; + assert!(def.validate_symbols().unwrap() == ()); + } + + #[test] + pub fn test_macro_validation_selector_fail() { + let def = MacroDef{ + argdefs: vec![ + "foo".to_string() + ], + fields: vec![ + ("f1".to_string(), Expression::Add(BinaryExpression( + Value::Selector(vec!["bar".to_string(), "quux".to_string()]), + Box::new(Expression::Simple(Value::Int(1)))))), + ], + }; + let mut expected = HashSet::new(); + expected.insert("bar".to_string()); + assert_eq!(def.validate_symbols().err().unwrap(), expected); + } +} diff --git a/src/build.rs b/src/build.rs index c6bce9e..10c988b 100644 --- a/src/build.rs +++ b/src/build.rs @@ -570,9 +570,12 @@ impl Builder { format!("{} is not a Macro", v)))) }, Expression::Macro(def) => { - // TODO(jwall): Walk the AST and verify that the symbols all - // exist as names in the arglist. - Ok(Rc::new(Val::Macro(def))) + match def.validate_symbols() { + Ok(()) => Ok(Rc::new(Val::Macro(def))), + Err(set) => Err(Box::new( + BuildError::NoSuchSymbol( + format!("Macro has the following undefined symbols: {:?}", set)))), + } }, Expression::Select(SelectDef{val: target, default: def_expr, tuple: mut fields}) => { // First resolve the target expression. @@ -984,4 +987,5 @@ mod test { ])), ], b); } + // TODO(jwall): Unit test for MacroDef Symbol Validation. }