From 0f9df9589146900877c0508eb684ca9d4ab1d137 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Wed, 20 Mar 2024 07:25:29 -0700 Subject: [PATCH] Implement RFC 57 (#702) Signed-off-by: Craig Disselkoen --- cedar-policy-core/src/ast/expr.rs | 52 +- cedar-policy-core/src/ast/expr_iterator.rs | 3 - cedar-policy-core/src/ast/ops.rs | 6 + cedar-policy-core/src/ast/policy.rs | 4 + cedar-policy-core/src/ast/restricted_expr.rs | 15 +- cedar-policy-core/src/ast/value.rs | 1 - cedar-policy-core/src/est/err.rs | 4 +- cedar-policy-core/src/est/expr.rs | 29 +- cedar-policy-core/src/evaluator.rs | 50 +- cedar-policy-core/src/evaluator/err.rs | 11 +- cedar-policy-core/src/parser/cst_to_ast.rs | 781 +++++++++++------- cedar-policy-core/src/parser/err.rs | 15 +- cedar-policy-validator/src/typecheck.rs | 39 +- .../src/typecheck/test_expr.rs | 4 +- .../src/typecheck/test_type_annotation.rs | 4 +- cedar-policy/CHANGELOG.md | 5 +- cedar-policy/src/tests.rs | 41 + 17 files changed, 607 insertions(+), 457 deletions(-) diff --git a/cedar-policy-core/src/ast/expr.rs b/cedar-policy-core/src/ast/expr.rs index fd005a0bf..c3fe8eb54 100644 --- a/cedar-policy-core/src/ast/expr.rs +++ b/cedar-policy-core/src/ast/expr.rs @@ -93,17 +93,6 @@ pub enum ExprKind { /// Second arg arg2: Arc>, }, - /// Multiplication by constant - /// - /// This isn't just a BinaryOp because its arguments aren't both expressions. - /// (Similar to how `like` isn't a BinaryOp and has its own AST node as well.) - MulByConst { - /// first argument, which may be an arbitrary expression, but must - /// evaluate to Long type - arg: Arc>, - /// second argument, which must be an integer constant - constant: Integer, - }, /// Application of an extension function to n arguments /// INVARIANT (MethodStyleArgs): /// if op.style is MethodStyle then args _cannot_ be empty. @@ -384,9 +373,9 @@ impl Expr { ExprBuilder::new().sub(e1, e2) } - /// Create a 'mul' expression. First argument must evaluate to Long type. - pub fn mul(e: Expr, c: Integer) -> Self { - ExprBuilder::new().mul(e, c) + /// Create a 'mul' expression. Arguments must evaluate to Long type + pub fn mul(e1: Expr, e2: Expr) -> Self { + ExprBuilder::new().mul(e1, e2) } /// Create a 'neg' expression. `e` must evaluate to Long type. @@ -589,9 +578,6 @@ impl Expr { Ok(Expr::record(map) .expect("cannot have a duplicate key because the input was already a BTreeMap")) } - ExprKind::MulByConst { arg, constant } => { - Ok(Expr::mul(arg.substitute(definitions)?, *constant)) - } ExprKind::Is { expr, entity_type } => Ok(Expr::is_entity_type( expr.substitute(definitions)?, entity_type.clone(), @@ -862,11 +848,12 @@ impl ExprBuilder { }) } - /// Create a 'mul' expression. First argument must evaluate to Long type. - pub fn mul(self, e: Expr, c: Integer) -> Expr { - self.with_expr_kind(ExprKind::MulByConst { - arg: Arc::new(e), - constant: c, + /// Create a 'mul' expression. Arguments must evaluate to Long type + pub fn mul(self, e1: Expr, e2: Expr) -> Expr { + self.with_expr_kind(ExprKind::BinaryApp { + op: BinaryOp::Mul, + arg1: Arc::new(e1), + arg2: Arc::new(e2), }) } @@ -1077,12 +1064,12 @@ impl ExprBuilder { } /// Errors when constructing an `Expr` -#[derive(Debug, PartialEq, Diagnostic, Error)] +#[derive(Debug, PartialEq, Eq, Clone, Diagnostic, Error)] pub enum ExprConstructionError { - /// A key occurred twice (or more) in a record literal + /// The same key occurred two or more times in a single record literal #[error("duplicate key `{key}` in record literal")] DuplicateKeyInRecordLiteral { - /// The key which occurred twice (or more) in the record literal + /// The key which occurred two or more times in the record literal key: SmolStr, }, } @@ -1178,13 +1165,6 @@ impl Expr { arg2: arg21, }, ) => op == op1 && arg1.eq_shape(arg11) && arg2.eq_shape(arg21), - ( - MulByConst { arg, constant }, - MulByConst { - arg: arg1, - constant: constant1, - }, - ) => constant == constant1 && arg.eq_shape(arg1), ( ExtensionFunctionApp { fn_name, args }, ExtensionFunctionApp { @@ -1274,10 +1254,6 @@ impl Expr { arg1.hash_shape(state); arg2.hash_shape(state); } - ExprKind::MulByConst { arg, constant } => { - arg.hash_shape(state); - constant.hash(state); - } ExprKind::ExtensionFunctionApp { fn_name, args } => { fn_name.hash(state); state.write_usize(args.len()); @@ -1654,8 +1630,8 @@ mod test { Expr::sub(Expr::val(1), Expr::val(1)), ), ( - ExprBuilder::with_data(1).mul(temp.clone(), 1), - Expr::mul(Expr::val(1), 1), + ExprBuilder::with_data(1).mul(temp.clone(), temp.clone()), + Expr::mul(Expr::val(1), Expr::val(1)), ), ( ExprBuilder::with_data(1).neg(temp.clone()), diff --git a/cedar-policy-core/src/ast/expr_iterator.rs b/cedar-policy-core/src/ast/expr_iterator.rs index 1b0a90fa2..c06ef340c 100644 --- a/cedar-policy-core/src/ast/expr_iterator.rs +++ b/cedar-policy-core/src/ast/expr_iterator.rs @@ -71,9 +71,6 @@ impl<'a, T> Iterator for ExprIterator<'a, T> { self.expression_stack.push(arg1); self.expression_stack.push(arg2); } - ExprKind::MulByConst { arg, .. } => { - self.expression_stack.push(arg); - } ExprKind::ExtensionFunctionApp { args, .. } => { for arg in args.as_ref() { self.expression_stack.push(arg); diff --git a/cedar-policy-core/src/ast/ops.rs b/cedar-policy-core/src/ast/ops.rs index 07842a0e9..7163c0f76 100644 --- a/cedar-policy-core/src/ast/ops.rs +++ b/cedar-policy-core/src/ast/ops.rs @@ -61,6 +61,11 @@ pub enum BinaryOp { /// Arguments must have Long type Sub, + /// Integer multiplication + /// + /// Arguments must have Long type + Mul, + /// Hierarchy membership. Specifically, is the first arg a member of the /// second. /// @@ -103,6 +108,7 @@ impl std::fmt::Display for BinaryOp { BinaryOp::LessEq => write!(f, "_<=_"), BinaryOp::Add => write!(f, "_+_"), BinaryOp::Sub => write!(f, "_-_"), + BinaryOp::Mul => write!(f, "_*_"), BinaryOp::In => write!(f, "_in_"), BinaryOp::Contains => write!(f, "contains"), BinaryOp::ContainsAll => write!(f, "containsAll"), diff --git a/cedar-policy-core/src/ast/policy.rs b/cedar-policy-core/src/ast/policy.rs index a9783a92b..882a35d20 100644 --- a/cedar-policy-core/src/ast/policy.rs +++ b/cedar-policy-core/src/ast/policy.rs @@ -29,6 +29,8 @@ extern crate tsify; /// Top level structure for a policy template. /// Contains both the AST for template, and the list of open slots in the template. +/// +/// Note that this "template" may have no slots, in which case this `Template` represents a static policy #[derive(Clone, Hash, Eq, PartialEq, Debug, Serialize, Deserialize)] #[serde(from = "TemplateBody")] #[serde(into = "TemplateBody")] @@ -36,6 +38,8 @@ pub struct Template { body: TemplateBody, /// INVARIANT (slot cache correctness): This Vec must contain _all_ of the open slots in `body` /// This is maintained by the only two public constructors, `new` and `instantiate_inline_policy` + /// + /// Note that `slots` may be empty, in which case this `Template` represents a static policy slots: Vec, } diff --git a/cedar-policy-core/src/ast/restricted_expr.rs b/cedar-policy-core/src/ast/restricted_expr.rs index e34230d6b..4febc2d81 100644 --- a/cedar-policy-core/src/ast/restricted_expr.rs +++ b/cedar-policy-core/src/ast/restricted_expr.rs @@ -23,7 +23,7 @@ use crate::parser::err::ParseErrors; use crate::parser::{self, Loc}; use miette::Diagnostic; use serde::{Deserialize, Serialize}; -use smol_str::SmolStr; +use smol_str::{SmolStr, ToSmolStr}; use std::hash::{Hash, Hasher}; use std::ops::Deref; use std::sync::Arc; @@ -477,15 +477,11 @@ fn is_restricted(expr: &Expr) -> Result<(), RestrictedExprError> { expr: expr.clone(), }), ExprKind::UnaryApp { op, .. } => Err(RestrictedExprError::InvalidRestrictedExpression { - feature: op.to_string().into(), + feature: op.to_smolstr(), expr: expr.clone(), }), ExprKind::BinaryApp { op, .. } => Err(RestrictedExprError::InvalidRestrictedExpression { - feature: op.to_string().into(), - expr: expr.clone(), - }), - ExprKind::MulByConst { .. } => Err(RestrictedExprError::InvalidRestrictedExpression { - feature: "multiplication".into(), + feature: op.to_smolstr(), expr: expr.clone(), }), ExprKind::GetAttr { .. } => Err(RestrictedExprError::InvalidRestrictedExpression { @@ -654,6 +650,7 @@ pub enum RestrictedExprParseError { #[cfg(test)] mod test { use super::*; + use crate::ast::ExprConstructionError; use crate::parser::err::{ParseError, ToASTError, ToASTErrorKind}; use crate::parser::Loc; use std::str::FromStr; @@ -706,7 +703,9 @@ mod test { RestrictedExpr::from_str(str), Err(RestrictedExprParseError::Parse(ParseErrors(vec![ ParseError::ToAST(ToASTError::new( - ToASTErrorKind::DuplicateKeyInRecordLiteral { key: "foo".into() }, + ToASTErrorKind::ExprConstructionError( + ExprConstructionError::DuplicateKeyInRecordLiteral { key: "foo".into() } + ), Loc::new(0..32, Arc::from(str)) )) ]))), diff --git a/cedar-policy-core/src/ast/value.rs b/cedar-policy-core/src/ast/value.rs index 26bae906f..c68da5d03 100644 --- a/cedar-policy-core/src/ast/value.rs +++ b/cedar-policy-core/src/ast/value.rs @@ -248,7 +248,6 @@ impl TryFrom for ValueKind { ExprKind::Or { .. } => Err(NotValue::NotValue { loc }), ExprKind::UnaryApp { .. } => Err(NotValue::NotValue { loc }), ExprKind::BinaryApp { .. } => Err(NotValue::NotValue { loc }), - ExprKind::MulByConst { .. } => Err(NotValue::NotValue { loc }), ExprKind::ExtensionFunctionApp { .. } => Err(NotValue::NotValue { loc }), ExprKind::GetAttr { .. } => Err(NotValue::NotValue { loc }), ExprKind::HasAttr { .. } => Err(NotValue::NotValue { loc }), diff --git a/cedar-policy-core/src/est/err.rs b/cedar-policy-core/src/est/err.rs index cecb7fc45..e66f38585 100644 --- a/cedar-policy-core/src/est/err.rs +++ b/cedar-policy-core/src/est/err.rs @@ -14,7 +14,7 @@ * limitations under the License. */ -use crate::ast::{self, SlotId}; +use crate::ast; use crate::entities::JsonDeserializationError; use crate::parser::err::ParseErrors; use crate::parser::unescape; @@ -49,7 +49,7 @@ pub enum FromJsonError { #[diagnostic(help("slots are currently unsupported in `{clausetype}` clauses"))] SlotsInConditionClause { /// Slot that was found in a when/unless clause - slot: SlotId, + slot: ast::SlotId, /// Clause type, e.g. "when" or "unless" clausetype: &'static str, }, diff --git a/cedar-policy-core/src/est/expr.rs b/cedar-policy-core/src/est/expr.rs index f4e6c34af..3886c8d68 100644 --- a/cedar-policy-core/src/est/expr.rs +++ b/cedar-policy-core/src/est/expr.rs @@ -620,26 +620,10 @@ impl Expr { (*left).clone().try_into_ast(id.clone())?, (*right).clone().try_into_ast(id)?, )), - Expr::ExprNoExt(ExprNoExt::Mul { left, right }) => { - let left: ast::Expr = (*left).clone().try_into_ast(id.clone())?; - let right: ast::Expr = (*right).clone().try_into_ast(id)?; - let left_c = match left.expr_kind() { - ast::ExprKind::Lit(ast::Literal::Long(c)) => Some(c), - _ => None, - }; - let right_c = match right.expr_kind() { - ast::ExprKind::Lit(ast::Literal::Long(c)) => Some(c), - _ => None, - }; - match (left_c, right_c) { - (_, Some(c)) => Ok(ast::Expr::mul(left, *c)), - (Some(c), _) => Ok(ast::Expr::mul(right, *c)), - (None, None) => Err(FromJsonError::MultiplicationByNonConstant { - arg1: left, - arg2: right, - })?, - } - } + Expr::ExprNoExt(ExprNoExt::Mul { left, right }) => Ok(ast::Expr::mul( + (*left).clone().try_into_ast(id.clone())?, + (*right).clone().try_into_ast(id)?, + )), Expr::ExprNoExt(ExprNoExt::Contains { left, right }) => Ok(ast::Expr::contains( (*left).clone().try_into_ast(id.clone())?, (*right).clone().try_into_ast(id)?, @@ -780,15 +764,12 @@ impl From for Expr { ast::BinaryOp::LessEq => Expr::lesseq(arg1, arg2), ast::BinaryOp::Add => Expr::add(arg1, arg2), ast::BinaryOp::Sub => Expr::sub(arg1, arg2), + ast::BinaryOp::Mul => Expr::mul(arg1, arg2), ast::BinaryOp::Contains => Expr::contains(Arc::new(arg1), arg2), ast::BinaryOp::ContainsAll => Expr::contains_all(Arc::new(arg1), arg2), ast::BinaryOp::ContainsAny => Expr::contains_any(Arc::new(arg1), arg2), } } - ast::ExprKind::MulByConst { arg, constant } => Expr::mul( - Arc::unwrap_or_clone(arg).into(), - Expr::lit(CedarValueJson::Long(constant as InputInteger)), - ), ast::ExprKind::ExtensionFunctionApp { fn_name, args } => { let args = Arc::unwrap_or_clone(args) .into_iter() diff --git a/cedar-policy-core/src/evaluator.rs b/cedar-policy-core/src/evaluator.rs index d068e44f6..14dae9e6c 100644 --- a/cedar-policy-core/src/evaluator.rs +++ b/cedar-policy-core/src/evaluator.rs @@ -408,7 +408,11 @@ impl<'e> Evaluator<'e> { match op { BinaryOp::Eq => Ok((arg1 == arg2).into()), // comparison and arithmetic operators, which only work on Longs - BinaryOp::Less | BinaryOp::LessEq | BinaryOp::Add | BinaryOp::Sub => { + BinaryOp::Less + | BinaryOp::LessEq + | BinaryOp::Add + | BinaryOp::Sub + | BinaryOp::Mul => { let i1 = arg1.get_as_long()?; let i2 = arg2.get_as_long()?; match op { @@ -436,6 +440,17 @@ impl<'e> Evaluator<'e> { loc.cloned(), )), }, + BinaryOp::Mul => match i1.checked_mul(i2) { + Some(prod) => Ok(prod.into()), + None => Err(EvaluationError::integer_overflow( + IntegerOverflowError::BinaryOp { + op: *op, + arg1, + arg2, + }, + loc.cloned(), + )), + }, // PANIC SAFETY `op` is checked to be one of the above #[allow(clippy::unreachable)] _ => { @@ -530,22 +545,6 @@ impl<'e> Evaluator<'e> { } } } - ExprKind::MulByConst { arg, constant } => match self.partial_interpret(arg, slots)? { - PartialValue::Value(arg) => { - let i1 = arg.get_as_long()?; - match i1.checked_mul(*constant) { - Some(prod) => Ok(prod.into()), - None => Err(EvaluationError::integer_overflow( - IntegerOverflowError::Multiplication { - arg, - constant: *constant, - }, - loc.cloned(), - )), - } - } - PartialValue::Residual(r) => Ok(PartialValue::Residual(Expr::mul(r, *constant))), - }, ExprKind::ExtensionFunctionApp { fn_name, args } => { let args = args .iter() @@ -2986,17 +2985,17 @@ pub mod test { ); // 5 * (-3) assert_eq!( - eval.interpret_inline_policy(&Expr::mul(Expr::val(5), -3)), + eval.interpret_inline_policy(&Expr::mul(Expr::val(5), Expr::val(-3))), Ok(Value::from(-15)) ); // 5 * 0 assert_eq!( - eval.interpret_inline_policy(&Expr::mul(Expr::val(5), 0)), + eval.interpret_inline_policy(&Expr::mul(Expr::val(5), Expr::val(0))), Ok(Value::from(0)) ); // "5" * 0 assert_matches!( - eval.interpret_inline_policy(&Expr::mul(Expr::val("5"), 0)), + eval.interpret_inline_policy(&Expr::mul(Expr::val("5"), Expr::val(0))), Err(e) => assert_eq!(e.error_kind(), &EvaluationErrorKind::TypeError { expected: nonempty![Type::Long], @@ -3006,11 +3005,12 @@ pub mod test { ); // overflow assert_eq!( - eval.interpret_inline_policy(&Expr::mul(Expr::val(Integer::MAX - 1), 3)), + eval.interpret_inline_policy(&Expr::mul(Expr::val(Integer::MAX - 1), Expr::val(3))), Err(EvaluationError::integer_overflow( - IntegerOverflowError::Multiplication { - arg: Value::from(Integer::MAX - 1), - constant: 3, + IntegerOverflowError::BinaryOp { + op: BinaryOp::Mul, + arg1: Value::from(Integer::MAX - 1), + arg2: Value::from(3), }, None )) @@ -5550,7 +5550,7 @@ pub mod test { let exts = Extensions::none(); let eval = Evaluator::new(empty_request(), &es, &exts); - let e = Expr::mul(Expr::unknown(Unknown::new_untyped("a")), 32); + let e = Expr::mul(Expr::unknown(Unknown::new_untyped("a")), Expr::val(32)); let r = eval.partial_interpret(&e, &HashMap::new()).unwrap(); assert_eq!(r, PartialValue::Residual(e)); } diff --git a/cedar-policy-core/src/evaluator/err.rs b/cedar-policy-core/src/evaluator/err.rs index 87ccc6ce0..7c1af37d9 100644 --- a/cedar-policy-core/src/evaluator/err.rs +++ b/cedar-policy-core/src/evaluator/err.rs @@ -392,7 +392,7 @@ fn pretty_type_error(expected: &NonEmpty, actual: &Type) -> String { #[derive(Debug, PartialEq, Eq, Clone, Diagnostic, Error)] pub enum IntegerOverflowError { /// Overflow during a binary operation - #[error("integer overflow while attempting to {} the values `{arg1}` and `{arg2}`", match .op { BinaryOp::Add => "add", BinaryOp::Sub => "subtract", _ => "perform an operation on" })] + #[error("integer overflow while attempting to {} the values `{arg1}` and `{arg2}`", match .op { BinaryOp::Add => "add", BinaryOp::Sub => "subtract", BinaryOp::Mul => "multiply", _ => "perform an operation on" })] BinaryOp { /// overflow while evaluating this operator op: BinaryOp, @@ -402,15 +402,6 @@ pub enum IntegerOverflowError { arg2: Value, }, - /// Overflow during multiplication - #[error("integer overflow while attempting to multiply `{arg}` by `{constant}`")] - Multiplication { - /// first argument, which wasn't necessarily a constant in the policy - arg: Value, - /// second argument, which was a constant in the policy - constant: Integer, - }, - /// Overflow during a unary operation #[error("integer overflow while attempting to {} the value `{arg}`", match .op { UnaryOp::Neg => "negate", _ => "perform an operation on" })] UnaryOp { diff --git a/cedar-policy-core/src/parser/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs index aae1a393b..3a638e0c1 100644 --- a/cedar-policy-core/src/parser/cst_to_ast.rs +++ b/cedar-policy-core/src/parser/cst_to_ast.rs @@ -43,12 +43,12 @@ use super::loc::Loc; use super::node::Node; use super::unescape::{to_pattern, to_unescaped_string}; use crate::ast::{ - self, ActionConstraint, CallStyle, EntityReference, EntityType, EntityUID, - ExprConstructionError, Integer, PatternElem, PolicySetError, PrincipalConstraint, - PrincipalOrResourceConstraint, ResourceConstraint, + self, ActionConstraint, CallStyle, EntityReference, EntityType, EntityUID, Integer, + PatternElem, PolicySetError, PrincipalConstraint, PrincipalOrResourceConstraint, + ResourceConstraint, }; use crate::est::extract_single_argument; -use itertools::Either; +use itertools::{Either, Itertools}; use smol_str::SmolStr; use std::cmp::Ordering; use std::collections::{BTreeMap, HashSet}; @@ -827,6 +827,7 @@ impl Node> { /// During conversion it is useful to keep track of expression that may be used /// as function names, record names, or record attributes. This prevents parsing these /// terms to a general Expr expression and then immediately unwrapping them. +#[derive(Debug)] pub(crate) enum ExprOrSpecial<'a> { /// Any expression except a variable, name, or string literal Expr { expr: ast::Expr, loc: Loc }, @@ -1450,76 +1451,26 @@ impl Node> { let mult = self.as_inner()?; let maybe_first = mult.initial.to_expr_or_special(errs); - // collect() preforms all the conversions, generating any errors - let more: Vec<(cst::MultOp, _)> = mult + let more = mult .extended .iter() - .filter_map(|&(op, ref i)| i.to_expr(errs).map(|e| (op, e))) - .collect(); + .filter_map(|&(op, ref i)| i.to_expr(errs).map(|e| (op, e))); + let (more, new_errs): (Vec<_>, Vec<_>) = more + .map(|(op, expr)| match op { + cst::MultOp::Times => Ok(expr), + cst::MultOp::Divide => Err(self.to_ast_err(ToASTErrorKind::UnsupportedDivision)), + cst::MultOp::Mod => Err(self.to_ast_err(ToASTErrorKind::UnsupportedModulo)), + }) + .partition_result(); + errs.extend(new_errs); if !more.is_empty() { + // in this case, `first` must be an expr, we should collect any errors there as well let first = maybe_first?.into_expr(errs)?; - // enforce that division and remainder/modulo are not supported - for (op, _) in &more { - match op { - cst::MultOp::Times => {} - cst::MultOp::Divide => { - errs.push(self.to_ast_err(ToASTErrorKind::UnsupportedDivision)); - return None; - } - cst::MultOp::Mod => { - errs.push(self.to_ast_err(ToASTErrorKind::UnsupportedModulo)); - return None; - } - } - } - // split all the operands into constantints and nonconstantints. - // also, remove the opcodes -- from here on we assume they're all - // `Times`, having checked above that this is the case - let (constantints, nonconstantints): (Vec, Vec) = - std::iter::once(first) - .chain(more.into_iter().map(|(_, e)| e)) - .partition(|e| { - matches!(e.expr_kind(), ast::ExprKind::Lit(ast::Literal::Long(_))) - }); - let constantints = constantints - .into_iter() - .map(|e| match e.expr_kind() { - ast::ExprKind::Lit(ast::Literal::Long(i)) => *i, - // PANIC SAFETY Checked the match above via the call to `partition` - #[allow(clippy::unreachable)] - _ => unreachable!( - "checked it matched ast::ExprKind::Lit(ast::Literal::Long(_)) above" - ), - }) - .collect::>(); - if nonconstantints.len() > 1 { - // at most one of the operands in `a * b * c * d * ...` can be a nonconstantint - errs.push(self.to_ast_err(ToASTErrorKind::NonConstantMultiplication)); - None - } else if nonconstantints.is_empty() { - // PANIC SAFETY If nonconstantints is empty then constantints must have at least one value - #[allow(clippy::indexing_slicing)] - Some(ExprOrSpecial::Expr { - expr: construct_expr_mul( - construct_expr_num(constantints[0], self.loc.clone()), - constantints[1..].iter().copied(), - &self.loc, - ), - loc: self.loc.clone(), - }) - } else { - // PANIC SAFETY Checked above that `nonconstantints` has at least one element - #[allow(clippy::expect_used)] - let nonconstantint: ast::Expr = nonconstantints - .into_iter() - .next() - .expect("already checked that it's not empty"); - Some(ExprOrSpecial::Expr { - expr: construct_expr_mul(nonconstantint, constantints, &self.loc), - loc: self.loc.clone(), - }) - } + Some(ExprOrSpecial::Expr { + expr: construct_expr_mul(first, more, &self.loc), + loc: self.loc.clone(), + }) } else { maybe_first } @@ -2494,14 +2445,14 @@ fn construct_expr_add( /// used for a chain of multiplication only (no division or mod) fn construct_expr_mul( f: ast::Expr, - chained: impl IntoIterator, + chained: impl IntoIterator, loc: &Loc, ) -> ast::Expr { let mut expr = f; for next_expr in chained { expr = ast::ExprBuilder::new() .with_source_loc(loc.clone()) - .mul(expr, next_expr as Integer) + .mul(expr, next_expr); } expr } @@ -2561,11 +2512,7 @@ fn construct_expr_record( ast::ExprBuilder::new() .with_source_loc(loc.clone()) .record(kvs) - .map_err(|e| match e { - ExprConstructionError::DuplicateKeyInRecordLiteral { key } => { - ToASTError::new(ToASTErrorKind::DuplicateKeyInRecordLiteral { key }, loc) - } - }) + .map_err(|e| ToASTError::new(e.into(), loc)) } // PANIC SAFETY: Unit Test Code @@ -2593,7 +2540,12 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| { + panic!( + "failed convert to AST:\n{:?}", + miette::Report::new(errs.clone()) + ) + }); assert!(errs.is_empty()); // manual check at test defn println!("{:?}", expr); @@ -2609,7 +2561,7 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); // manual check at test defn println!("{:?}", expr); } @@ -2625,14 +2577,18 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); match expr.expr_kind() { ast::ExprKind::GetAttr { attr, .. } => { assert_eq!(attr, "some_ident"); } _ => panic!("should be a get expr"), } + } + #[test] + fn show_expr4() { + let mut errs = ParseErrors::new(); let expr = text_to_cst::parse_expr( r#" 1.some_ident @@ -2640,14 +2596,18 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); match expr.expr_kind() { ast::ExprKind::GetAttr { attr, .. } => { assert_eq!(attr, "some_ident"); } _ => panic!("should be a get expr"), } + } + #[test] + fn show_expr5() { + let mut errs = ParseErrors::new(); let expr = text_to_cst::parse_expr( r#" "first"["some string"] @@ -2655,7 +2615,7 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); match expr.expr_kind() { ast::ExprKind::GetAttr { attr, .. } => { assert_eq!(attr, "some string"); @@ -2665,7 +2625,7 @@ mod tests { } #[test] - fn show_expr4() { + fn show_expr6() { let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( r#" @@ -2674,7 +2634,7 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); match expr.expr_kind() { ast::ExprKind::HasAttr { attr, .. } => { @@ -2685,7 +2645,7 @@ mod tests { } #[test] - fn show_expr5() { + fn show_expr7() { let mut errs = ParseErrors::new(); let expr = text_to_cst::parse_expr( r#" @@ -2694,7 +2654,7 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); match expr.expr_kind() { ast::ExprKind::GetAttr { attr, .. } => { @@ -2702,7 +2662,11 @@ mod tests { } _ => panic!("should be a get expr"), } + } + #[test] + fn show_expr8() { + let mut errs = ParseErrors::new(); // parses to the same AST expression as above let expr: ast::Expr = text_to_cst::parse_expr( r#" @@ -2711,7 +2675,7 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); match expr.expr_kind() { ast::ExprKind::GetAttr { attr, .. } => { @@ -2719,7 +2683,10 @@ mod tests { } _ => panic!("should be a get expr"), } + } + #[test] + fn show_expr9() { // accessing a record with a non-identifier attribute let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( @@ -2729,7 +2696,7 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); match expr.expr_kind() { ast::ExprKind::GetAttr { attr, .. } => { @@ -2740,7 +2707,7 @@ mod tests { } #[test] - fn show_expr6_idents() { + fn show_expr10() { let mut errs = ParseErrors::new(); let expr = text_to_cst::parse_expr( r#" @@ -2751,13 +2718,16 @@ mod tests { .expect("failed parser") .to_expr(&mut errs); - println!("{:?}", errs); - assert!(expr.is_none()); - // a,b,a,b: unsupported variables - // if .. then .. else are invalid attributes - assert!(errs.len() == 6); + assert_matches!(expr, None => { + // four errors of unsupported-variable for a,b,a,b + // two errors of invalid record attribute + assert_eq!(errs.len(), 6, "{:?}", miette::Report::new(errs)); + }); + } - errs.clear(); + #[test] + fn show_expr11() { + let mut errs = ParseErrors::new(); let expr = text_to_cst::parse_expr( r#" {principal:"principal"} @@ -2765,15 +2735,17 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); - println!("{:?}", expr); match expr.expr_kind() { ast::ExprKind::Record { .. } => {} - _ => panic!("should be record"), + _ => panic!("should be record, got: {expr:?}"), } + } - errs.clear(); + #[test] + fn show_expr12() { + let mut errs = ParseErrors::new(); let expr = text_to_cst::parse_expr( r#" {"principal":"principal"} @@ -2781,12 +2753,11 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); - println!("{:?}", expr); match expr.expr_kind() { ast::ExprKind::Record { .. } => {} - _ => panic!("should be record"), + _ => panic!("should be record, got: {expr:?}"), } } @@ -2801,115 +2772,104 @@ mod tests { .expect("failed parse"); let convert = parse.to_expr(&mut errs); - println!("{:?}", errs); // uses true and false: - assert!(errs.len() == 2); - assert!(convert.is_none()); + assert_matches!(convert, None => { + assert_eq!(errs.len(), 2, "{:?}", miette::Report::new(errs)); + }); + } + #[test] + fn reserved_idents2() { let mut errs = ParseErrors::new(); - let parse = text_to_cst::parse_expr( - r#" + let src = r#" if {if: true}.if then {"if":false}["if"] else {when:true}.permit - "#, - ) - .expect("failed parse"); + "#; + let parse = text_to_cst::parse_expr(src).expect("failed parse"); - let convert = parse.to_expr(&mut errs); - println!("{:?}", errs); - // uses if twice, one of those triggers an invalid attr - assert!(errs.len() == 3); - assert!(convert.is_none()); + assert_matches!(parse.to_expr(&mut errs), None => { + expect_some_error_matches(src, &errs, &ExpectedErrorMessage::error("this identifier is reserved and cannot be used: `if`")); + expect_some_error_matches(src, &errs, &ExpectedErrorMessage::error("record literal has invalid attributes")); + assert_eq!(errs.len(), 3, "{:?}", miette::Report::new(errs)); // the `if` error occurs twice in that expression, so the total number of errors is 3 + }); } #[test] - fn reserved_idents2() { + fn reserved_idents3() { let mut errs = ParseErrors::new(); - let parse = text_to_cst::parse_expr( - r#" + let src = r#" if {where: true}.like || {has:false}.in then {"like":false}["in"] else {then:true}.else - "#, - ) - .expect("failed parse"); - - let convert = parse.to_expr(&mut errs); - println!("{:?}", errs); - // uses 5x reserved idents, 2 of those trigger an invalid attr - assert!(errs.len() == 7); - assert!(convert.is_none()); + "#; + let parse = text_to_cst::parse_expr(src).expect("failed parse"); + + assert_matches!(parse.to_expr(&mut errs), None => { + expect_some_error_matches(src, &errs, &ExpectedErrorMessage::error("this identifier is reserved and cannot be used: `has`")); + expect_some_error_matches(src, &errs, &ExpectedErrorMessage::error("this identifier is reserved and cannot be used: `like`")); + expect_some_error_matches(src, &errs, &ExpectedErrorMessage::error("this identifier is reserved and cannot be used: `in`")); + expect_some_error_matches(src, &errs, &ExpectedErrorMessage::error("this identifier is reserved and cannot be used: `then`")); + expect_some_error_matches(src, &errs, &ExpectedErrorMessage::error("this identifier is reserved and cannot be used: `else`")); + expect_some_error_matches(src, &errs, &ExpectedErrorMessage::error("record literal has invalid attributes")); + assert_eq!(errs.len(), 7, "{:?}", miette::Report::new(errs)); // the "record literal has invalid attributes" error occurs twice in that expression + }); } #[test] fn show_policy1() { let mut errs = ParseErrors::new(); - let parse = text_to_cst::parse_policy( - r#" + let src = r#" permit(principal:p,action:a,resource:r)when{w}unless{u}advice{"doit"}; - "#, - ) - .expect("failed parse"); + "#; + let parse = text_to_cst::parse_policy(src).expect("failed parse"); println!("{:#}", parse.as_inner().expect("internal parse error")); - let convert = parse.to_policy(ast::PolicyID::from_string("id"), &mut errs); - println!("{:?}", errs); - // 3x type constraints, 2x arbitrary vars, advice block - assert!(errs.len() == 6); - assert!(convert.is_none()); - // manual check at test defn - println!("{:?}", convert); + assert_matches!(parse.to_policy(ast::PolicyID::from_string("id"), &mut errs), None => { + // TODO: check the source snippet pointed to in each of these three errors is `p`, `a`, and `r` respectively + expect_some_error_matches(src, &errs, &ExpectedErrorMessage::error_and_help("type constraints using `:` are not supported", "try using `is` instead")); + expect_some_error_matches(src, &errs, &ExpectedErrorMessage::error_and_help("type constraints using `:` are not supported", "try using `is` instead")); + expect_some_error_matches(src, &errs, &ExpectedErrorMessage::error_and_help("type constraints using `:` are not supported", "try using `is` instead")); + expect_some_error_matches(src, &errs, &ExpectedErrorMessage::error_and_help("arbitrary variables are not supported; the valid Cedar variables are `principal`, `action`, `resource`, and `context`", "did you mean to enclose `w` in quotes to make a string?")); + expect_some_error_matches(src, &errs, &ExpectedErrorMessage::error_and_help("arbitrary variables are not supported; the valid Cedar variables are `principal`, `action`, `resource`, and `context`", "did you mean to enclose `u` in quotes to make a string?")); + expect_some_error_matches(src, &errs, &ExpectedErrorMessage::error_and_help("not a valid policy condition: `advice`", "condition must be either `when` or `unless`")); + }); } #[test] fn show_policy2() { let mut errs = ParseErrors::new(); - let parse = text_to_cst::parse_policy( - r#" + let src = r#" permit(principal,action,resource)when{true}; - "#, - ) - .expect("failed parse"); - println!("{}", parse.as_inner().expect("internal parse error")); - println!("{:?}", parse.as_inner().expect("internal parse error")); - let convert = parse.to_policy(ast::PolicyID::from_string("id"), &mut errs); - assert!(convert.is_some()); - // manual check at test defn - println!("{:?}", convert); + "#; + let parse = text_to_cst::parse_policy(src).expect("failed parse"); + println!("{:#}", parse.as_inner().expect("internal parse error")); + assert_matches!(parse.to_policy(ast::PolicyID::from_string("id"), &mut errs), Some(_) => { + assert!(errs.is_empty(), "{:?}", miette::Report::new(errs)); + }); } #[test] fn show_policy3() { let mut errs = ParseErrors::new(); - let parse = text_to_cst::parse_policy( - r#" + let src = r#" permit(principal in User::"jane",action,resource); - "#, - ) - .expect("failed parse"); - println!("{}", parse.as_inner().expect("internal parse error")); - println!("{:?}", parse.as_inner().expect("internal parse error")); - let convert = parse - .to_policy(ast::PolicyID::from_string("id"), &mut errs) - .expect("failed convert"); - assert!(errs.is_empty()); - // manual check at test defn - println!("{:?}", convert); + "#; + let parse = text_to_cst::parse_policy(src).expect("failed parse"); + println!("{:#}", parse.as_inner().expect("internal parse error")); + assert_matches!(parse.to_policy(ast::PolicyID::from_string("id"), &mut errs), Some(_) => { + assert!(errs.is_empty(), "{:?}", miette::Report::new(errs)); + }); } #[test] fn show_policy4() { let mut errs = ParseErrors::new(); - let parse = text_to_cst::parse_policy( - r#" + let src = r#" forbid(principal in User::"jane",action,resource)unless{ context.group != "friends" }; - "#, - ) - .expect("failed parse"); - let convert = parse - .to_policy(ast::PolicyID::from_string("id"), &mut errs) - .expect("failed convert"); - assert!(errs.is_empty()); - // manual check at test defn - println!("\n{:?}", convert); + "#; + let parse = text_to_cst::parse_policy(src).expect("failed parse"); + println!("{:#}", parse.as_inner().expect("internal parse error")); + assert_matches!(parse.to_policy(ast::PolicyID::from_string("id"), &mut errs), Some(_) => { + assert!(errs.is_empty(), "{:?}", miette::Report::new(errs)); + }); } #[test] @@ -2923,7 +2883,7 @@ mod tests { ) .expect("should parse") .to_policy(ast::PolicyID::from_string("id"), &mut errs) - .expect("should be valid"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); assert_matches!( policy.annotation(&ast::AnyId::new_unchecked("anno")), Some(ast::Annotation { val, .. }) => assert_eq!(val.as_ref(), "good annotation") @@ -2941,9 +2901,10 @@ mod tests { ) .expect("should parse") .to_policy(ast::PolicyID::from_string("id"), &mut errs); - assert!(policy.is_none()); - // annotation duplication (anno) - assert!(errs.len() == 1); + assert_matches!(policy, None => { + // annotation duplication (anno) + assert_eq!(errs.len(), 1, "{:?}", miette::Report::new(errs)); + }); // can have multiple annotations let mut errs = ParseErrors::new(); @@ -2962,7 +2923,7 @@ mod tests { ) .expect("should parse") .to_policyset(&mut errs) - .expect("should be valid"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); assert_matches!( policyset .get(&ast::PolicyID::from_string("policy0")) @@ -3052,7 +3013,7 @@ mod tests { "#, ).expect("should parse") .to_policyset(&mut errs) - .expect("should be valid"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); let policy0 = policyset .get(&ast::PolicyID::from_string("policy0")) .expect("should be the right policy ID"); @@ -3113,9 +3074,9 @@ mod tests { .expect("failed parse"); println!("\n{:#}", parse.as_inner().expect("internal parse error")); let convert = parse.to_policy(ast::PolicyID::from_string("id"), &mut errs); - println!("{:?}", errs); - assert!(errs.len() == 1); - assert!(convert.is_none()); + assert_matches!(convert, None => { + assert_eq!(errs.len(), 1, "{:?}", miette::Report::new(errs)); + }); } #[test] @@ -3133,9 +3094,9 @@ mod tests { .expect("failed parse"); println!("{:#}", parse.as_inner().expect("internal parse error")); let convert = parse.to_policy(ast::PolicyID::from_string("id"), &mut errs); - println!("{:?}", errs); - assert!(errs.len() == 1); - assert!(convert.is_none()); + assert_matches!(convert, None => { + assert_eq!(errs.len(), 1, "{:?}", miette::Report::new(errs)); + }); } #[test] @@ -3148,8 +3109,9 @@ mod tests { ) .expect("failed parse"); let convert = parse.to_policy(ast::PolicyID::from_string("id"), &mut errs); - assert!(errs.len() == 1); - assert!(convert.is_none()); + assert_matches!(convert, None => { + assert_eq!(errs.len(), 1, "{:?}", miette::Report::new(errs)); + }); } #[test] @@ -3164,8 +3126,11 @@ mod tests { .expect("parse error") .to_expr(&mut errs); // ast should be acceptable - println!("{:?}", errs); - assert!(e.is_some()); + assert!( + e.is_some(), + "{:?}", // the Debug representation of `miette::Report` is the pretty one + miette::Report::new(errs.clone()), + ); assert!(errs.is_empty()); let e = text_to_cst::parse_expr( @@ -3177,13 +3142,13 @@ mod tests { .expect("parse error") .to_expr(&mut errs); // ast should be error, since "contains" is used inappropriately - println!("{:?}", errs); - assert!(e.is_none()); - assert!(errs.len() == 1); + assert_matches!(e, None => { + assert_eq!(errs.len(), 1, "{:?}", miette::Report::new(errs)); + }); } #[test] - fn construct_record1() { + fn construct_record_1() { let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" @@ -3193,7 +3158,7 @@ mod tests { // the cst should be acceptable .expect("parse error") .to_expr(&mut errs) - .expect("convert fail"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); // ast should be acceptable, with record construction if let ast::ExprKind::Record { .. } = e.expr_kind() { // good @@ -3201,7 +3166,11 @@ mod tests { panic!("not a record") } println!("{e}"); + } + #[test] + fn construct_record_2() { + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" {"one":"one"} @@ -3210,7 +3179,7 @@ mod tests { // the cst should be acceptable .expect("parse error") .to_expr(&mut errs) - .expect("convert fail"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); // ast should be acceptable, with record construction if let ast::ExprKind::Record { .. } = e.expr_kind() { // good @@ -3218,7 +3187,11 @@ mod tests { panic!("not a record") } println!("{e}"); + } + #[test] + fn construct_record_3() { + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" {"one":"one",two:"two"} @@ -3227,7 +3200,7 @@ mod tests { // the cst should be acceptable .expect("parse error") .to_expr(&mut errs) - .expect("convert fail"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); // ast should be acceptable, with record construction if let ast::ExprKind::Record { .. } = e.expr_kind() { // good @@ -3235,7 +3208,11 @@ mod tests { panic!("not a record") } println!("{e}"); + } + #[test] + fn construct_record_4() { + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" {one:"one","two":"two"} @@ -3244,7 +3221,7 @@ mod tests { // the cst should be acceptable .expect("parse error") .to_expr(&mut errs) - .expect("convert fail"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); // ast should be acceptable, with record construction if let ast::ExprKind::Record { .. } = e.expr_kind() { // good @@ -3252,7 +3229,11 @@ mod tests { panic!("not a record") } println!("{e}"); + } + #[test] + fn construct_record_5() { + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" {one:"b\"","b\"":2} @@ -3261,7 +3242,7 @@ mod tests { // the cst should be acceptable .expect("parse error") .to_expr(&mut errs) - .expect("convert fail"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); // ast should be acceptable, with record construction if let ast::ExprKind::Record { .. } = e.expr_kind() { // good @@ -3272,7 +3253,7 @@ mod tests { } #[test] - fn construct_invalid_get() { + fn construct_invalid_get_1() { let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" @@ -3282,10 +3263,14 @@ mod tests { .expect("failed parser") .to_expr(&mut errs); // ast should be error: 0 is not a string literal - println!("{:?}", errs); - assert!(e.is_none()); - assert!(errs.len() == 1); + assert_matches!(e, None => { + assert_eq!(errs.len(), 1, "{:?}", miette::Report::new(errs)); + }); + } + #[test] + fn construct_invalid_get_2() { + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" {"one":1, "two":"two"}[-1] @@ -3294,9 +3279,14 @@ mod tests { .expect("failed parser") .to_expr(&mut errs); // ast should be error: -1 is not a string literal - println!("{:?}", errs); - assert!(e.is_none()); + assert_matches!(e, None => { + assert_eq!(errs.len(), 1, "{:?}", miette::Report::new(errs)); + }); + } + #[test] + fn construct_invalid_get_3() { + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" {"one":1, "two":"two"}[true] @@ -3305,9 +3295,14 @@ mod tests { .expect("failed parser") .to_expr(&mut errs); // ast should be error: true is not a string literal - println!("{:?}", errs); - assert!(e.is_none()); + assert_matches!(e, None => { + assert_eq!(errs.len(), 1, "{:?}", miette::Report::new(errs)); + }); + } + #[test] + fn construct_invalid_get_4() { + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" {"one":1, "two":"two"}[one] @@ -3316,12 +3311,13 @@ mod tests { .expect("failed parser") .to_expr(&mut errs); // ast should be error: one is not a string literal - println!("{:?}", errs); - assert!(e.is_none()); + assert_matches!(e, None => { + assert_eq!(errs.len(), 1, "{:?}", miette::Report::new(errs)); + }); } #[test] - fn construct_has() { + fn construct_has_1() { let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( r#" @@ -3330,7 +3326,7 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); match expr.expr_kind() { ast::ExprKind::HasAttr { attr, .. } => { @@ -3338,7 +3334,10 @@ mod tests { } _ => panic!("should be a has expr"), } + } + #[test] + fn construct_has_2() { let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" @@ -3348,13 +3347,13 @@ mod tests { .expect("failed parser") .to_expr(&mut errs); // ast should be error - println!("{:?}", errs); - assert!(e.is_none()); - assert!(errs.len() == 1); + assert_matches!(e, None => { + assert_eq!(errs.len(), 1, "{:?}", miette::Report::new(errs)); + }); } #[test] - fn construct_like() { + fn construct_like_1() { let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( r#" @@ -3363,14 +3362,18 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); match expr.expr_kind() { ast::ExprKind::Like { pattern, .. } => { assert_eq!(pattern.to_string(), "*5*"); } _ => panic!("should be a like expr"), } + } + #[test] + fn construct_like_2() { + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" "354 hams" like 354 @@ -3379,10 +3382,13 @@ mod tests { .expect("failed parser") .to_expr(&mut errs); // ast should be error - println!("{:?}", errs); - assert!(e.is_none()); - assert!(errs.len() == 1); + assert_matches!(e, None => { + assert_eq!(errs.len(), 1, "{:?}", miette::Report::new(errs)); + }); + } + #[test] + fn construct_like_3() { let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( r#" @@ -3391,14 +3397,18 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); match expr.expr_kind() { ast::ExprKind::Like { pattern, .. } => { assert_eq!(pattern.to_string(), r"string\\with\\backslashes"); } _ => panic!("should be a like expr"), } + } + #[test] + fn construct_like_4() { + let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( r#" "string\\with\\backslashes" like "string\*with\*backslashes" @@ -3406,14 +3416,18 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); match expr.expr_kind() { ast::ExprKind::Like { pattern, .. } => { assert_eq!(pattern.to_string(), r"string\*with\*backslashes"); } _ => panic!("should be a like expr"), } + } + #[test] + fn construct_like_5() { + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" "string\*with\*escaped\*stars" like "string\*with\*escaped\*stars" @@ -3422,10 +3436,14 @@ mod tests { .expect("failed parser") .to_expr(&mut errs); // ast should be error, \* is not a valid string character - println!("{:?}", errs); - assert!(e.is_none()); - assert!(errs.len() == 3); // 3 invalid escapes in the first argument + assert_matches!(e, None => { + assert_eq!(errs.len(), 3, "{:?}", miette::Report::new(errs)); // 3 invalid escapes in the first argument + }); + } + #[test] + fn construct_like_6() { + let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( r#" "string*with*stars" like "string\*with\*stars" @@ -3433,14 +3451,17 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); match expr.expr_kind() { ast::ExprKind::Like { pattern, .. } => { assert_eq!(pattern.to_string(), "string\\*with\\*stars"); } _ => panic!("should be a like expr"), } + } + #[test] + fn construct_like_7() { let mut errs = ParseErrors::new(); let expr: ast::Expr = text_to_cst::parse_expr( r#" @@ -3449,7 +3470,7 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); match expr.expr_kind() { ast::ExprKind::Like { pattern, .. } => { assert_eq!( @@ -3459,7 +3480,11 @@ mod tests { } _ => panic!("should be a like expr"), } - // round trip test + } + + #[test] + fn pattern_roundtrip() { + let mut errs = ParseErrors::new(); let test_pattern = &vec![ PatternElem::Char('h'), PatternElem::Char('e'), @@ -3479,7 +3504,7 @@ mod tests { let e2 = text_to_cst::parse_expr(&s1) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); match e2.expr_kind() { ast::ExprKind::Like { pattern, .. } => { assert_eq!(pattern.get_elems(), test_pattern); @@ -3518,7 +3543,7 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); match expr.expr_kind() { ast::ExprKind::HasAttr { attr, .. } => { assert_eq!(attr, "age"); @@ -3535,7 +3560,7 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); match expr.expr_kind() { ast::ExprKind::HasAttr { attr, .. } => { assert_eq!(attr, "arbitrary+ _string"); @@ -3552,8 +3577,9 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs); - assert!(e.is_none()); - assert!(errs.len() == 1); + assert_matches!(e, None => { + assert_eq!(errs.len(), 1, "{:?}", miette::Report::new(errs)); + }); // ok let mut errs = ParseErrors::new(); @@ -3564,7 +3590,7 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); match expr.expr_kind() { ast::ExprKind::GetAttr { attr, .. } => { assert_eq!(attr, "age"); @@ -3581,7 +3607,7 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs) - .expect("failed convert"); + .unwrap_or_else(|| panic!("failed convert to AST:\n{:?}", miette::Report::new(errs))); match expr.expr_kind() { ast::ExprKind::GetAttr { attr, .. } => { assert_eq!(attr, "arbitrary+ _string"); @@ -3598,8 +3624,9 @@ mod tests { ) .expect("failed parser") .to_expr(&mut errs); - assert!(e.is_none()); - assert!(errs.len() == 1); + assert_matches!(e, None => { + assert_eq!(errs.len(), 1, "{:?}", miette::Report::new(errs)); + }); } #[test] @@ -3614,8 +3641,14 @@ mod tests { .expect("parse error") .to_expr(&mut errs); // conversion should fail, too many relational ops - assert!(e.is_none()); + assert_matches!(e, None => { + assert_eq!(errs.len(), 1, "{:?}", miette::Report::new(errs)); + }); + } + #[test] + fn relational_ops2() { + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" 3 >= ("dad" in "dad") @@ -3625,8 +3658,17 @@ mod tests { .expect("parse error") .to_expr(&mut errs); // conversion should succeed, only one relational op - assert!(e.is_some()); + assert!( + e.is_some(), + "{:?}", // the Debug representation of `miette::Report` is the pretty one + miette::Report::new(errs.clone()), + ); + assert!(errs.is_empty()); + } + #[test] + fn relational_ops3() { + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" (3 >= 2) == true @@ -3636,8 +3678,17 @@ mod tests { .expect("parse error") .to_expr(&mut errs); // conversion should succeed, parentheses provided - assert!(e.is_some()); + assert!( + e.is_some(), + "{:?}", // the Debug representation of `miette::Report` is the pretty one + miette::Report::new(errs.clone()), + ); + assert!(errs.is_empty()); + } + #[test] + fn relational_ops4() { + let mut errs = ParseErrors::new(); let e = text_to_cst::parse_expr( r#" if 4 < 3 then 4 != 3 else 4 == 3 < 4 @@ -3647,7 +3698,9 @@ mod tests { .expect("parse error") .to_expr(&mut errs); // conversion should fail, too many relational ops - assert!(e.is_none()); + assert_matches!(e, None => { + assert_eq!(errs.len(), 1, "{:?}", miette::Report::new(errs)); + }); } #[test] @@ -3658,98 +3711,168 @@ mod tests { .expect("parse error") .to_expr(&mut errs); // conversion should succeed - assert!(e.is_some()); + assert!( + e.is_some(), + "{:?}", // the Debug representation of `miette::Report` is the pretty one + miette::Report::new(errs.clone()), + ); + assert!(errs.is_empty()); let e = text_to_cst::parse_expr(r#" 2 + -5 "#) // the cst should be acceptable .expect("parse error") .to_expr(&mut errs); // conversion should succeed - assert!(e.is_some()); + assert!( + e.is_some(), + "{:?}", // the Debug representation of `miette::Report` is the pretty one + miette::Report::new(errs.clone()), + ); + assert!(errs.is_empty()); let e = text_to_cst::parse_expr(r#" 2 - 5 "#) // the cst should be acceptable .expect("parse error") .to_expr(&mut errs); // conversion should succeed - assert!(e.is_some()); + assert!( + e.is_some(), + "{:?}", // the Debug representation of `miette::Report` is the pretty one + miette::Report::new(errs.clone()), + ); + assert!(errs.is_empty()); let e = text_to_cst::parse_expr(r#" 2 * 5 "#) // the cst should be acceptable .expect("parse error") .to_expr(&mut errs); // conversion should succeed - assert!(e.is_some()); + assert!( + e.is_some(), + "{:?}", // the Debug representation of `miette::Report` is the pretty one + miette::Report::new(errs.clone()), + ); + assert!(errs.is_empty()); let e = text_to_cst::parse_expr(r#" 2 * -5 "#) // the cst should be acceptable .expect("parse error") .to_expr(&mut errs); // conversion should succeed - assert!(e.is_some()); + assert!( + e.is_some(), + "{:?}", // the Debug representation of `miette::Report` is the pretty one + miette::Report::new(errs.clone()), + ); + assert!(errs.is_empty()); let e = text_to_cst::parse_expr(r#" context.size * 4 "#) // the cst should be acceptable .expect("parse error") .to_expr(&mut errs); // conversion should succeed - assert!(e.is_some()); + assert!( + e.is_some(), + "{:?}", // the Debug representation of `miette::Report` is the pretty one + miette::Report::new(errs.clone()), + ); + assert!(errs.is_empty()); let e = text_to_cst::parse_expr(r#" 4 * context.size "#) // the cst should be acceptable .expect("parse error") .to_expr(&mut errs); // conversion should succeed - assert!(e.is_some()); + assert!( + e.is_some(), + "{:?}", // the Debug representation of `miette::Report` is the pretty one + miette::Report::new(errs.clone()), + ); + assert!(errs.is_empty()); let e = text_to_cst::parse_expr(r#" context.size * context.scale "#) // the cst should be acceptable .expect("parse error") .to_expr(&mut errs); - // conversion should fail: only multiplication by a constant is allowed - assert!(e.is_none()); + // conversion should succeed + assert!( + e.is_some(), + "{:?}", // the Debug representation of `miette::Report` is the pretty one + miette::Report::new(errs.clone()), + ); + assert!(errs.is_empty()); let e = text_to_cst::parse_expr(r#" 5 + 10 + 90 "#) // the cst should be acceptable .expect("parse error") .to_expr(&mut errs); // conversion should succeed - assert!(e.is_some()); + assert!( + e.is_some(), + "{:?}", // the Debug representation of `miette::Report` is the pretty one + miette::Report::new(errs.clone()), + ); + assert!(errs.is_empty()); let e = text_to_cst::parse_expr(r#" 5 + 10 - 90 * -2 "#) // the cst should be acceptable .expect("parse error") .to_expr(&mut errs); // conversion should succeed - assert!(e.is_some()); + assert!( + e.is_some(), + "{:?}", // the Debug representation of `miette::Report` is the pretty one + miette::Report::new(errs.clone()), + ); + assert!(errs.is_empty()); let e = text_to_cst::parse_expr(r#" 5 + 10 * 90 - 2 "#) // the cst should be acceptable .expect("parse error") .to_expr(&mut errs); // conversion should succeed - assert!(e.is_some()); + assert!( + e.is_some(), + "{:?}", // the Debug representation of `miette::Report` is the pretty one + miette::Report::new(errs.clone()), + ); + assert!(errs.is_empty()); let e = text_to_cst::parse_expr(r#" 5 - 10 - 90 - 2 "#) // the cst should be acceptable .expect("parse error") .to_expr(&mut errs); // conversion should succeed - assert!(e.is_some()); + assert!( + e.is_some(), + "{:?}", // the Debug representation of `miette::Report` is the pretty one + miette::Report::new(errs.clone()), + ); + assert!(errs.is_empty()); let e = text_to_cst::parse_expr(r#" 5 * context.size * 10 "#) // the cst should be acceptable .expect("parse error") .to_expr(&mut errs); // conversion should succeed - assert!(e.is_some()); + assert!( + e.is_some(), + "{:?}", // the Debug representation of `miette::Report` is the pretty one + miette::Report::new(errs.clone()), + ); + assert!(errs.is_empty()); let e = text_to_cst::parse_expr(r#" context.size * 3 * context.scale "#) // the cst should be acceptable .expect("parse error") .to_expr(&mut errs); - // conversion should fail: only multiplication by a constant is allowed - assert!(e.is_none()); + // conversion should succeed + assert!( + e.is_some(), + "{:?}", // the Debug representation of `miette::Report` is the pretty one + miette::Report::new(errs.clone()), + ); + assert!(errs.is_empty()); } const CORRECT_TEMPLATES: [&str; 7] = [ @@ -3787,7 +3910,13 @@ mod tests { .expect("parse error") .to_policy(ast::PolicyID::from_string("0"), &mut errs); // conversion should succeed, it's just permit all - assert!(e.is_some()); + assert!( + e.is_some(), + "{:?}", // the Debug representation of `miette::Report` is the pretty one + miette::Report::new(errs.clone()), + ); + assert!(errs.is_empty()); + let e = text_to_cst::parse_policy( r#" permit(principal:User,action,resource); @@ -3797,7 +3926,9 @@ mod tests { .expect("parse error") .to_policy(ast::PolicyID::from_string("1"), &mut errs); // conversion should fail, variable types are not supported - assert!(e.is_none()); + assert_matches!(e, None => { + assert_eq!(errs.len(), 1, "{:?}", miette::Report::new(errs)); + }); } #[test] fn string_escapes() { @@ -3823,7 +3954,7 @@ mod tests { let test_invalid = |s: &str, en: usize| { let r = parse_literal(&format!("\"{}\"", s)); assert!(r.is_err()); - assert!(r.unwrap_err().len() == en); + assert_eq!(r.unwrap_err().len(), en); }; // invalid escape `\a` test_invalid("\\a", 1); @@ -3953,51 +4084,110 @@ mod tests { #[test] fn test_mul() { - for (es, expr) in [ - ("--2*3", Expr::mul(Expr::neg(Expr::val(-2)), 3)), + for (str, expected) in [ + ("--2*3", Expr::mul(Expr::neg(Expr::val(-2)), Expr::val(3))), ( "1 * 2 * false", - Expr::mul(Expr::mul(Expr::val(false), 1), 2), + Expr::mul(Expr::mul(Expr::val(1), Expr::val(2)), Expr::val(false)), ), ( "0 * 1 * principal", - Expr::mul(Expr::mul(Expr::var(ast::Var::Principal), 0), 1), + Expr::mul( + Expr::mul(Expr::val(0), Expr::val(1)), + Expr::var(ast::Var::Principal), + ), ), ( "0 * (-1) * principal", - Expr::mul(Expr::mul(Expr::var(ast::Var::Principal), 0), -1), + Expr::mul( + Expr::mul(Expr::val(0), Expr::val(-1)), + Expr::var(ast::Var::Principal), + ), + ), + ( + "0 * 6 * context.foo", + Expr::mul( + Expr::mul(Expr::val(0), Expr::val(6)), + Expr::get_attr(Expr::var(ast::Var::Context), "foo".into()), + ), + ), + ( + "(0 * 6) * context.foo", + Expr::mul( + Expr::mul(Expr::val(0), Expr::val(6)), + Expr::get_attr(Expr::var(ast::Var::Context), "foo".into()), + ), + ), + ( + "0 * (6 * context.foo)", + Expr::mul( + Expr::val(0), + Expr::mul( + Expr::val(6), + Expr::get_attr(Expr::var(ast::Var::Context), "foo".into()), + ), + ), + ), + ( + "0 * (context.foo * 6)", + Expr::mul( + Expr::val(0), + Expr::mul( + Expr::get_attr(Expr::var(ast::Var::Context), "foo".into()), + Expr::val(6), + ), + ), + ), + ( + "1 * 2 * 3 * context.foo * 4 * 5 * 6", + Expr::mul( + Expr::mul( + Expr::mul( + Expr::mul( + Expr::mul(Expr::mul(Expr::val(1), Expr::val(2)), Expr::val(3)), + Expr::get_attr(Expr::var(ast::Var::Context), "foo".into()), + ), + Expr::val(4), + ), + Expr::val(5), + ), + Expr::val(6), + ), + ), + ( + "principal * (1 + 2)", + Expr::mul( + Expr::var(ast::Var::Principal), + Expr::add(Expr::val(1), Expr::val(2)), + ), + ), + ( + "principal * -(-1)", + Expr::mul(Expr::var(ast::Var::Principal), Expr::neg(Expr::val(-1))), + ), + ( + "principal * --1", + Expr::mul(Expr::var(ast::Var::Principal), Expr::neg(Expr::val(-1))), + ), + ( + r#"false * "bob""#, + Expr::mul(Expr::val(false), Expr::val("bob")), ), ] { let mut errs = ParseErrors::new(); - let e = text_to_cst::parse_expr(es) + let e = text_to_cst::parse_expr(str) .expect("should construct a CST") .to_expr(&mut errs) - .expect("should convert to AST"); + .unwrap_or_else(|| { + panic!( + "failed convert to AST:\n{:?}", + miette::Report::new(errs.clone()) + ) + }); + assert!(errs.is_empty()); assert!( - e.eq_shape(&expr), - "{:?} and {:?} should have the same shape.", - e, - expr - ); - } - - for es in [ - r#"false * "bob""#, - "principal * (1 + 2)", - "principal * -(-1)", - // --1 is parsed as Expr::neg(Expr::val(-1)) and thus is not - // considered as a constant. - "principal * --1", - ] { - let mut errs = ParseErrors::new(); - let e = text_to_cst::parse_expr(es) - .expect("should construct a CST") - .to_expr(&mut errs); - assert!(e.is_none()); - expect_some_error_matches( - es, - &errs, - &ExpectedErrorMessage::error("multiplication must be by an integer literal"), + e.eq_shape(&expected), + "{e:?} and {expected:?} should have the same shape", ); } } @@ -4045,7 +4235,9 @@ mod tests { let e = text_to_cst::parse_expr(es) .expect("should construct a CST") .to_expr(&mut errs) - .expect("should convert to AST"); + .unwrap_or_else(|| { + panic!("failed convert to AST:\n{:?}", miette::Report::new(errs)) + }); assert!( e.eq_shape(&expr), "{:?} and {:?} should have the same shape.", @@ -4082,7 +4274,9 @@ mod tests { let e = text_to_cst::parse_expr(es) .expect("should construct a CST") .to_expr(&mut errs) - .expect("should convert to AST"); + .unwrap_or_else(|| { + panic!("failed convert to AST:\n{:?}", miette::Report::new(errs)) + }); assert!( e.eq_shape(&expr), "{:?} and {:?} should have the same shape.", @@ -4115,8 +4309,9 @@ mod tests { let e = text_to_cst::parse_expr(es) .expect("should construct a CST") .to_expr(&mut errs); - assert_matches!(e, None); - expect_err(es, &errs, &em); + assert_matches!(e, None => { + expect_err(es, &errs, &em); + }); } } diff --git a/cedar-policy-core/src/parser/err.rs b/cedar-policy-core/src/parser/err.rs index 625eae693..96e837449 100644 --- a/cedar-policy-core/src/parser/err.rs +++ b/cedar-policy-core/src/parser/err.rs @@ -26,7 +26,7 @@ use miette::{Diagnostic, LabeledSpan, SourceSpan}; use smol_str::SmolStr; use thiserror::Error; -use crate::ast::{self, InputInteger, PolicyID, RestrictedExprError, Var}; +use crate::ast::{self, ExprConstructionError, InputInteger, PolicyID, RestrictedExprError, Var}; use crate::parser::fmt::join_with_conjunction; use crate::parser::loc::Loc; use crate::parser::node::Node; @@ -305,9 +305,10 @@ pub enum ToASTErrorKind { /// Returned when a policy uses the remainder/modulo operator (`%`), which is not supported #[error("remainder/modulo is not supported")] UnsupportedModulo, - /// Returned when a policy attempts to multiply by a non-constant integer - #[error("multiplication must be by an integer literal")] - NonConstantMultiplication, + /// Any `ExprConstructionError` can also happen while converting CST to AST + #[error(transparent)] + #[diagnostic(transparent)] + ExprConstructionError(#[from] ExprConstructionError), /// Returned when a policy contains an integer literal that is out of range #[error("integer literal `{0}` is too large")] #[diagnostic(help("maximum allowed integer literal is `{}`", InputInteger::MAX))] @@ -343,12 +344,6 @@ pub enum ToASTErrorKind { /// Returned when the contents of an indexing expression is not a string literal #[error("the contents of an index expression must be a string literal")] NonStringIndex, - /// Returned when the same key appears two or more times in a single record literal - #[error("duplicate key `{key}` in record literal")] - DuplicateKeyInRecordLiteral { - /// The key that appeared two or more times - key: SmolStr, - }, /// Returned when a user attempts to use type-constraint `:` syntax. This /// syntax was not adopted, but `is` can be used to write type constraints /// in the policy scope. diff --git a/cedar-policy-validator/src/typecheck.rs b/cedar-policy-validator/src/typecheck.rs index 203813c67..9650764db 100644 --- a/cedar-policy-validator/src/typecheck.rs +++ b/cedar-policy-validator/src/typecheck.rs @@ -949,10 +949,6 @@ impl<'a> Typechecker<'a> { // INVARIANT: typecheck_binary requires a `BinaryApp`, we've just ensured this self.typecheck_binary(request_env, prior_eff, e, type_errors) } - ExprKind::MulByConst { .. } => { - // INVARIANT: typecheck_mul requires a `MulByConst`, we've just ensured this - self.typecheck_mul(request_env, prior_eff, e, type_errors) - } ExprKind::ExtensionFunctionApp { .. } => { // INVARIANT: typecheck_extension requires a `ExtensionFunctionApp`, we've just ensured this self.typecheck_extension(request_env, prior_eff, e, type_errors) @@ -1413,7 +1409,7 @@ impl<'a> Typechecker<'a> { }) } - BinaryOp::Add | BinaryOp::Sub => { + BinaryOp::Add | BinaryOp::Sub | BinaryOp::Mul => { let help_builder = |actual: &Type| match (op, actual) { ( BinaryOp::Add, @@ -1607,39 +1603,6 @@ impl<'a> Typechecker<'a> { } } - /// Like `typecheck_binary()`, but for multiplication, which isn't - /// technically a `BinaryOp` - /// INVARIANT: must be called `mul_expr` being a `MulByConst` - fn typecheck_mul<'b>( - &self, - request_env: &RequestEnv, - prior_eff: &EffectSet<'b>, - mul_expr: &'b Expr, - type_errors: &mut Vec, - ) -> TypecheckAnswer<'b> { - // PANIC SAFETY: maintained by invariant on this function - #[allow(clippy::panic)] - let ExprKind::MulByConst { arg, constant } = mul_expr.expr_kind() else { - panic!("`typecheck_mul` called with an expression kind other than `MulByConst`"); - }; - - let ans_arg = self.expect_type( - request_env, - prior_eff, - arg, - Type::primitive_long(), - type_errors, - |_| None, - ); - ans_arg.then_typecheck(|arg_expr_ty, _| { - TypecheckAnswer::success({ - ExprBuilder::with_data(Some(Type::primitive_long())) - .with_same_source_loc(mul_expr) - .mul(arg_expr_ty, *constant) - }) - }) - } - /// Get the type for an `==` expression given the input types. fn type_of_equality<'b>( &self, diff --git a/cedar-policy-validator/src/typecheck/test_expr.rs b/cedar-policy-validator/src/typecheck/test_expr.rs index 80157b0d4..8f525253a 100644 --- a/cedar-policy-validator/src/typecheck/test_expr.rs +++ b/cedar-policy-validator/src/typecheck/test_expr.rs @@ -1072,13 +1072,13 @@ fn neg_typecheck_fails() { #[test] fn mul_typechecks() { - let neg_expr = Expr::mul(Expr::val(1), 2); + let neg_expr = Expr::mul(Expr::val(1), Expr::val(2)); assert_typechecks_empty_schema(neg_expr, Type::primitive_long()); } #[test] fn mul_typecheck_fails() { - let neg_expr = Expr::mul(Expr::val("foo"), 2); + let neg_expr = Expr::mul(Expr::val("foo"), Expr::val(2)); assert_typecheck_fails_empty_schema( neg_expr, Type::primitive_long(), diff --git a/cedar-policy-validator/src/typecheck/test_type_annotation.rs b/cedar-policy-validator/src/typecheck/test_type_annotation.rs index 2c77ba1fb..ac6c50135 100644 --- a/cedar-policy-validator/src/typecheck/test_type_annotation.rs +++ b/cedar-policy-validator/src/typecheck/test_type_annotation.rs @@ -100,10 +100,10 @@ fn expr_typechecks_with_correct_annotation() { .not(ExprBuilder::with_data(Some(Type::singleton_boolean(false))).val(false)), ); assert_expr_has_annotated_ast( - &Expr::mul(Expr::val(3), 4), + &Expr::mul(Expr::val(3), Expr::val(4)), &ExprBuilder::with_data(Some(Type::primitive_long())).mul( ExprBuilder::with_data(Some(Type::primitive_long())).val(3), - 4, + ExprBuilder::with_data(Some(Type::primitive_long())).val(4), ), ); assert_expr_has_annotated_ast( diff --git a/cedar-policy/CHANGELOG.md b/cedar-policy/CHANGELOG.md index a5248ee2f..83a08fb4b 100644 --- a/cedar-policy/CHANGELOG.md +++ b/cedar-policy/CHANGELOG.md @@ -16,6 +16,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Implement [RFC 57](https://github.com/cedar-policy/rfcs/pull/57): policies can + now include multiplication of arbitrary expressions, not just multiplication of + an expression and a constant. - Moved `::Err` to `Infallible` (#588, resolving #551) - Improved "unexpected token" parse errors when the schema or policy parsers expect an identifier. (#698) @@ -27,7 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [3.1.1] - 2024-03-14 -### Fixed +### Fixed - `ValidationResult` methods `validation_errors` and `validation_warnings`, along with `confusable_string_checker`, now return iterators with static lifetimes instead of diff --git a/cedar-policy/src/tests.rs b/cedar-policy/src/tests.rs index 3505450f0..9d5cf83b2 100644 --- a/cedar-policy/src/tests.rs +++ b/cedar-policy/src/tests.rs @@ -3402,3 +3402,44 @@ mod issue_606 { )); } } + +mod issue_619 { + use crate::{eval_expression, Context, Entities, EvalResult, Policy, Request}; + use cool_asserts::assert_matches; + + /// The first issue reported in issue 619. + /// This policy should parse properly, convert to JSON properly, and convert back from JSON properly. + #[test] + fn issue_619() { + let policy = Policy::parse( + None, + r#"permit(principal, action, resource) when {1 * 2 * true};"#, + ) + .unwrap(); + let json = policy.to_json().unwrap(); + let _ = Policy::from_json(None, json).unwrap(); + } + + /// Another issue from a comment: Ensure the correct error semantics of these expressions + #[test] + fn mult_overflows() { + let eval = |expr: &str| { + eval_expression( + &Request::new(None, None, None, Context::empty(), None).unwrap(), + &Entities::empty(), + &expr.parse().unwrap(), + ) + }; + assert_matches!(eval(&format!("{}*{}*0", 1_i64 << 62, 1_i64 << 62)), Err(e) => { + assert_eq!(&e.to_string(), "integer overflow while attempting to multiply the values `4611686018427387904` and `4611686018427387904`"); + }); + assert_matches!( + eval(&format!("{}*0*{}", 1_i64 << 62, 1_i64 << 62)), + Ok(EvalResult::Long(0)) + ); + assert_matches!( + eval(&format!("0*{}*{}", 1_i64 << 62, 1_i64 << 62)), + Ok(EvalResult::Long(0)) + ); + } +}