Skip to content

Commit

Permalink
Cherry-pick multiplication change (RFC 57) to 2.4.x (#753)
Browse files Browse the repository at this point in the history
Signed-off-by: Craig Disselkoen <[email protected]>
Signed-off-by: John Kastner <[email protected]>
Co-authored-by: Craig Disselkoen <[email protected]>
  • Loading branch information
john-h-kastner-aws and cdisselkoen authored Mar 29, 2024
1 parent ac741c5 commit 38ce559
Show file tree
Hide file tree
Showing 16 changed files with 214 additions and 275 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"decision": "Deny",
"reasons": [],
"errors": [
"error occurred while evaluating policy `policy0`: integer overflow while attempting to multiply -1537158028109086738 by 60138"
"error occurred while evaluating policy `policy0`: integer overflow while attempting to multiply the values -1537158028109086738 and 60138"
]
},
{
Expand All @@ -25,7 +25,7 @@
"decision": "Deny",
"reasons": [],
"errors": [
"error occurred while evaluating policy `policy0`: integer overflow while attempting to multiply -1537158028109086738 by 60138"
"error occurred while evaluating policy `policy0`: integer overflow while attempting to multiply the values -1537158028109086738 and 60138"
]
},
{
Expand All @@ -37,7 +37,7 @@
"decision": "Deny",
"reasons": [],
"errors": [
"error occurred while evaluating policy `policy0`: integer overflow while attempting to multiply -1537158028109086738 by 60138"
"error occurred while evaluating policy `policy0`: integer overflow while attempting to multiply the values -1537158028109086738 and 60138"
]
},
{
Expand All @@ -49,7 +49,7 @@
"decision": "Deny",
"reasons": [],
"errors": [
"error occurred while evaluating policy `policy0`: integer overflow while attempting to multiply -1537158028109086738 by 60138"
"error occurred while evaluating policy `policy0`: integer overflow while attempting to multiply the values -1537158028109086738 and 60138"
]
},
{
Expand All @@ -61,7 +61,7 @@
"decision": "Deny",
"reasons": [],
"errors": [
"error occurred while evaluating policy `policy0`: integer overflow while attempting to multiply -1537158028109086738 by 60138"
"error occurred while evaluating policy `policy0`: integer overflow while attempting to multiply the values -1537158028109086738 and 60138"
]
},
{
Expand All @@ -73,7 +73,7 @@
"decision": "Deny",
"reasons": [],
"errors": [
"error occurred while evaluating policy `policy0`: integer overflow while attempting to multiply -1537158028109086738 by 60138"
"error occurred while evaluating policy `policy0`: integer overflow while attempting to multiply the values -1537158028109086738 and 60138"
]
},
{
Expand All @@ -85,7 +85,7 @@
"decision": "Deny",
"reasons": [],
"errors": [
"error occurred while evaluating policy `policy0`: integer overflow while attempting to multiply -1537158028109086738 by 60138"
"error occurred while evaluating policy `policy0`: integer overflow while attempting to multiply the values -1537158028109086738 and 60138"
]
},
{
Expand All @@ -97,8 +97,8 @@
"decision": "Deny",
"reasons": [],
"errors": [
"error occurred while evaluating policy `policy0`: integer overflow while attempting to multiply -1537158028109086738 by 60138"
"error occurred while evaluating policy `policy0`: integer overflow while attempting to multiply the values -1537158028109086738 and 60138"
]
}
]
}
}
56 changes: 17 additions & 39 deletions cedar-policy-core/src/ast/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,17 +101,6 @@ pub enum ExprKind<T = ()> {
/// Second arg
arg2: Arc<Expr<T>>,
},
/// 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<Expr<T>>,
/// second argument, which must be an integer constant
constant: i64,
},
/// Application of an extension function to n arguments
/// INVARIANT (MethodStyleArgs):
/// if op.style is MethodStyle then args _cannot_ be empty.
Expand Down Expand Up @@ -382,9 +371,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: i64) -> 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.
Expand Down Expand Up @@ -566,9 +555,6 @@ impl Expr {
.collect::<Result<Vec<_>, _>>()?;
Ok(Expr::record(pairs))
}
ExprKind::MulByConst { arg, constant } => {
Ok(Expr::mul(arg.substitute(definitions)?, *constant))
}
}
}
}
Expand Down Expand Up @@ -652,6 +638,12 @@ impl std::fmt::Display for Expr {
maybe_with_parens(arg1),
maybe_with_parens(arg2),
),
BinaryOp::Mul => write!(
f,
"{} * {}",
maybe_with_parens(arg1),
maybe_with_parens(arg2),
),
BinaryOp::In => write!(
f,
"{} in {}",
Expand All @@ -668,9 +660,6 @@ impl std::fmt::Display for Expr {
write!(f, "{}.containsAny({})", maybe_with_parens(arg1), &arg2)
}
},
ExprKind::MulByConst { arg, constant } => {
write!(f, "{} * {}", maybe_with_parens(arg), constant)
}
ExprKind::ExtensionFunctionApp { fn_name, args } => {
// search for the name and callstyle
let style = Extensions::all_available().all_funcs().find_map(|f| {
Expand Down Expand Up @@ -756,7 +745,6 @@ fn maybe_with_parens(expr: &Expr) -> String {
format!("({})", expr)
}
ExprKind::BinaryApp { .. } => format!("({})", expr),
ExprKind::MulByConst { .. } => format!("({})", expr),
ExprKind::ExtensionFunctionApp { .. } => format!("({})", expr),
ExprKind::GetAttr { .. } => format!("({})", expr),
ExprKind::HasAttr { .. } => format!("({})", expr),
Expand Down Expand Up @@ -994,11 +982,12 @@ impl<T> ExprBuilder<T> {
})
}

/// Create a 'mul' expression. First argument must evaluate to Long type.
pub fn mul(self, e: Expr<T>, c: i64) -> Expr<T> {
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<T>, e2: Expr<T>) -> Expr<T> {
self.with_expr_kind(ExprKind::BinaryApp {
op: BinaryOp::Mul,
arg1: Arc::new(e1),
arg2: Arc::new(e2),
})
}

Expand Down Expand Up @@ -1245,13 +1234,6 @@ impl<T> Expr<T> {
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 {
Expand Down Expand Up @@ -1337,10 +1319,6 @@ impl<T> Expr<T> {
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());
Expand Down Expand Up @@ -1682,8 +1660,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()),
Expand Down
3 changes: 0 additions & 3 deletions cedar-policy-core/src/ast/expr_iterator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
6 changes: 6 additions & 0 deletions cedar-policy-core/src/ast/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down Expand Up @@ -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"),
Expand Down
4 changes: 4 additions & 0 deletions cedar-policy-core/src/ast/policy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,17 @@ use thiserror::Error;

/// 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")]
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<SlotId>,
}

Expand Down
7 changes: 1 addition & 6 deletions cedar-policy-core/src/ast/restricted_expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,13 +205,8 @@ fn is_restricted(expr: &Expr) -> Result<(), RestrictedExpressionError> {
feature: op.to_string(),
})
}
ExprKind::MulByConst { .. } => {
Err(RestrictedExpressionError::InvalidRestrictedExpression {
feature: "multiplication".into(),
})
}
ExprKind::GetAttr { .. } => Err(RestrictedExpressionError::InvalidRestrictedExpression {
feature: "get-attribute".into(),
feature: "attribute accesses".into(),
}),
ExprKind::HasAttr { .. } => Err(RestrictedExpressionError::InvalidRestrictedExpression {
feature: "'has'".into(),
Expand Down
1 change: 0 additions & 1 deletion cedar-policy-core/src/ast/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ impl TryFrom<Expr> for Value {
ExprKind::Or { .. } => Err(NotValue::NotValue),
ExprKind::UnaryApp { .. } => Err(NotValue::NotValue),
ExprKind::BinaryApp { .. } => Err(NotValue::NotValue),
ExprKind::MulByConst { .. } => Err(NotValue::NotValue),
ExprKind::ExtensionFunctionApp { .. } => Err(NotValue::NotValue),
ExprKind::GetAttr { .. } => Err(NotValue::NotValue),
ExprKind::HasAttr { .. } => Err(NotValue::NotValue),
Expand Down
4 changes: 2 additions & 2 deletions cedar-policy-core/src/est/err.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

use crate::ast::{self, SlotId};
use crate::ast;
use crate::entities::JsonDeserializationError;
use crate::parser::unescape;
use smol_str::SmolStr;
Expand All @@ -40,7 +40,7 @@ pub enum EstToAstError {
#[error("found template slot {slot} in a `{clausetype}` clause")]
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,
},
Expand Down
29 changes: 5 additions & 24 deletions cedar-policy-core/src/est/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -522,26 +522,10 @@ impl TryFrom<Expr> for ast::Expr {
(*left).clone().try_into()?,
(*right).clone().try_into()?,
)),
Expr::ExprNoExt(ExprNoExt::Mul { left, right }) => {
let left: ast::Expr = (*left).clone().try_into()?;
let right: ast::Expr = (*right).clone().try_into()?;
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(EstToAstError::MultiplicationByNonConstant {
arg1: left,
arg2: right,
})?,
}
}
Expr::ExprNoExt(ExprNoExt::Mul { left, right }) => Ok(ast::Expr::mul(
(*left).clone().try_into()?,
(*right).clone().try_into()?,
)),
Expr::ExprNoExt(ExprNoExt::Contains { left, right }) => Ok(ast::Expr::contains(
(*left).clone().try_into()?,
(*right).clone().try_into()?,
Expand Down Expand Up @@ -658,15 +642,12 @@ impl From<ast::Expr> 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(
unwrap_or_clone(arg).into(),
Expr::lit(JSONValue::Long(constant)),
),
ast::ExprKind::ExtensionFunctionApp { fn_name, args } => {
let args = unwrap_or_clone(args).into_iter().map(Into::into).collect();
Expr::ext_call(fn_name.to_string().into(), args)
Expand Down
Loading

0 comments on commit 38ce559

Please sign in to comment.