From c43279e92382cbfe4ab5821cce265742bb3fb4ce Mon Sep 17 00:00:00 2001 From: John Kastner Date: Tue, 31 Dec 2024 16:33:15 +0000 Subject: [PATCH] Use `unwrap_or_clone` instead of `(*foo).clone()` Signed-off-by: John Kastner --- cedar-policy-core/src/ast/policy_set.rs | 2 +- cedar-policy-core/src/est/expr.rs | 204 +++++++++++++----------- cedar-policy-core/src/evaluator.rs | 2 +- 3 files changed, 109 insertions(+), 99 deletions(-) diff --git a/cedar-policy-core/src/ast/policy_set.rs b/cedar-policy-core/src/ast/policy_set.rs index 1508c7922..c5267a9a8 100644 --- a/cedar-policy-core/src/ast/policy_set.rs +++ b/cedar-policy-core/src/ast/policy_set.rs @@ -433,7 +433,7 @@ impl PolicySet { match self.templates.remove(policy_id) { Some(t) => { self.template_to_links_map.remove(policy_id); - Ok((*t).clone()) + Ok(Arc::unwrap_or_clone(t)) } None => panic!("Found in template_to_links_map but not in templates"), } diff --git a/cedar-policy-core/src/est/expr.rs b/cedar-policy-core/src/est/expr.rs index 8e401b44f..0440da513 100644 --- a/cedar-policy-core/src/est/expr.rs +++ b/cedar-policy-core/src/est/expr.rs @@ -673,96 +673,96 @@ impl Expr { ExprNoExt::Var(_) => Ok(self), ExprNoExt::Slot(_) => Ok(self), ExprNoExt::Not { arg } => Ok(Expr::ExprNoExt(ExprNoExt::Not { - arg: Arc::new((*arg).clone().sub_entity_literals(mapping)?), + arg: Arc::new(Arc::unwrap_or_clone(arg).sub_entity_literals(mapping)?), })), ExprNoExt::Neg { arg } => Ok(Expr::ExprNoExt(ExprNoExt::Neg { - arg: Arc::new((*arg).clone().sub_entity_literals(mapping)?), + arg: Arc::new(Arc::unwrap_or_clone(arg).sub_entity_literals(mapping)?), })), ExprNoExt::Eq { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::Eq { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), - right: Arc::new((*right).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), + right: Arc::new(Arc::unwrap_or_clone(right).sub_entity_literals(mapping)?), })), ExprNoExt::NotEq { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::NotEq { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), - right: Arc::new((*right).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), + right: Arc::new(Arc::unwrap_or_clone(right).sub_entity_literals(mapping)?), })), ExprNoExt::In { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::In { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), - right: Arc::new((*right).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), + right: Arc::new(Arc::unwrap_or_clone(right).sub_entity_literals(mapping)?), })), ExprNoExt::Less { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::Less { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), - right: Arc::new((*right).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), + right: Arc::new(Arc::unwrap_or_clone(right).sub_entity_literals(mapping)?), })), ExprNoExt::LessEq { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::LessEq { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), - right: Arc::new((*right).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), + right: Arc::new(Arc::unwrap_or_clone(right).sub_entity_literals(mapping)?), })), ExprNoExt::Greater { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::Greater { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), - right: Arc::new((*right).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), + right: Arc::new(Arc::unwrap_or_clone(right).sub_entity_literals(mapping)?), })), ExprNoExt::GreaterEq { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::GreaterEq { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), - right: Arc::new((*right).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), + right: Arc::new(Arc::unwrap_or_clone(right).sub_entity_literals(mapping)?), })), ExprNoExt::And { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::And { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), - right: Arc::new((*right).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), + right: Arc::new(Arc::unwrap_or_clone(right).sub_entity_literals(mapping)?), })), ExprNoExt::Or { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::Or { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), - right: Arc::new((*right).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), + right: Arc::new(Arc::unwrap_or_clone(right).sub_entity_literals(mapping)?), })), ExprNoExt::Add { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::Add { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), - right: Arc::new((*right).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), + right: Arc::new(Arc::unwrap_or_clone(right).sub_entity_literals(mapping)?), })), ExprNoExt::Sub { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::Sub { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), - right: Arc::new((*right).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), + right: Arc::new(Arc::unwrap_or_clone(right).sub_entity_literals(mapping)?), })), ExprNoExt::Mul { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::Mul { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), - right: Arc::new((*right).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), + right: Arc::new(Arc::unwrap_or_clone(right).sub_entity_literals(mapping)?), })), ExprNoExt::Contains { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::Contains { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), - right: Arc::new((*right).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), + right: Arc::new(Arc::unwrap_or_clone(right).sub_entity_literals(mapping)?), })), ExprNoExt::ContainsAll { left, right } => { Ok(Expr::ExprNoExt(ExprNoExt::ContainsAll { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), - right: Arc::new((*right).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), + right: Arc::new(Arc::unwrap_or_clone(right).sub_entity_literals(mapping)?), })) } ExprNoExt::ContainsAny { left, right } => { Ok(Expr::ExprNoExt(ExprNoExt::ContainsAny { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), - right: Arc::new((*right).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), + right: Arc::new(Arc::unwrap_or_clone(right).sub_entity_literals(mapping)?), })) } ExprNoExt::IsEmpty { arg } => Ok(Expr::ExprNoExt(ExprNoExt::IsEmpty { - arg: Arc::new((*arg).clone().sub_entity_literals(mapping)?), + arg: Arc::new(Arc::unwrap_or_clone(arg).sub_entity_literals(mapping)?), })), ExprNoExt::GetTag { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::GetTag { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), - right: Arc::new((*right).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), + right: Arc::new(Arc::unwrap_or_clone(right).sub_entity_literals(mapping)?), })), ExprNoExt::HasTag { left, right } => Ok(Expr::ExprNoExt(ExprNoExt::HasTag { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), - right: Arc::new((*right).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), + right: Arc::new(Arc::unwrap_or_clone(right).sub_entity_literals(mapping)?), })), ExprNoExt::GetAttr { left, attr } => Ok(Expr::ExprNoExt(ExprNoExt::GetAttr { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), attr, })), ExprNoExt::HasAttr { left, attr } => Ok(Expr::ExprNoExt(ExprNoExt::HasAttr { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), attr, })), ExprNoExt::Like { left, pattern } => Ok(Expr::ExprNoExt(ExprNoExt::Like { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), pattern, })), ExprNoExt::Is { @@ -771,12 +771,14 @@ impl Expr { in_expr, } => match in_expr { Some(in_expr) => Ok(Expr::ExprNoExt(ExprNoExt::Is { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), entity_type, - in_expr: Some(Arc::new((*in_expr).clone().sub_entity_literals(mapping)?)), + in_expr: Some(Arc::new( + Arc::unwrap_or_clone(in_expr).sub_entity_literals(mapping)?, + )), })), None => Ok(Expr::ExprNoExt(ExprNoExt::Is { - left: Arc::new((*left).clone().sub_entity_literals(mapping)?), + left: Arc::new(Arc::unwrap_or_clone(left).sub_entity_literals(mapping)?), entity_type, in_expr: None, })), @@ -786,9 +788,15 @@ impl Expr { then_expr, else_expr, } => Ok(Expr::ExprNoExt(ExprNoExt::If { - cond_expr: Arc::new((*cond_expr).clone().sub_entity_literals(mapping)?), - then_expr: Arc::new((*then_expr).clone().sub_entity_literals(mapping)?), - else_expr: Arc::new((*else_expr).clone().sub_entity_literals(mapping)?), + cond_expr: Arc::new( + Arc::unwrap_or_clone(cond_expr).sub_entity_literals(mapping)?, + ), + then_expr: Arc::new( + Arc::unwrap_or_clone(then_expr).sub_entity_literals(mapping)?, + ), + else_expr: Arc::new( + Arc::unwrap_or_clone(else_expr).sub_entity_literals(mapping)?, + ), })), ExprNoExt::Set(v) => { let mut new_v = vec![]; @@ -833,90 +841,92 @@ impl Expr { Expr::ExprNoExt(ExprNoExt::Var(var)) => Ok(ast::Expr::var(var)), Expr::ExprNoExt(ExprNoExt::Slot(slot)) => Ok(ast::Expr::slot(slot)), Expr::ExprNoExt(ExprNoExt::Not { arg }) => { - Ok(ast::Expr::not((*arg).clone().try_into_ast(id)?)) + Ok(ast::Expr::not(Arc::unwrap_or_clone(arg).try_into_ast(id)?)) } Expr::ExprNoExt(ExprNoExt::Neg { arg }) => { - Ok(ast::Expr::neg((*arg).clone().try_into_ast(id)?)) + Ok(ast::Expr::neg(Arc::unwrap_or_clone(arg).try_into_ast(id)?)) } Expr::ExprNoExt(ExprNoExt::Eq { left, right }) => Ok(ast::Expr::is_eq( - (*left).clone().try_into_ast(id.clone())?, - (*right).clone().try_into_ast(id)?, + Arc::unwrap_or_clone(left).try_into_ast(id.clone())?, + Arc::unwrap_or_clone(right).try_into_ast(id)?, )), Expr::ExprNoExt(ExprNoExt::NotEq { left, right }) => Ok(ast::Expr::noteq( - (*left).clone().try_into_ast(id.clone())?, - (*right).clone().try_into_ast(id)?, + Arc::unwrap_or_clone(left).try_into_ast(id.clone())?, + Arc::unwrap_or_clone(right).try_into_ast(id)?, )), Expr::ExprNoExt(ExprNoExt::In { left, right }) => Ok(ast::Expr::is_in( - (*left).clone().try_into_ast(id.clone())?, - (*right).clone().try_into_ast(id)?, + Arc::unwrap_or_clone(left).try_into_ast(id.clone())?, + Arc::unwrap_or_clone(right).try_into_ast(id)?, )), Expr::ExprNoExt(ExprNoExt::Less { left, right }) => Ok(ast::Expr::less( - (*left).clone().try_into_ast(id.clone())?, - (*right).clone().try_into_ast(id)?, + Arc::unwrap_or_clone(left).try_into_ast(id.clone())?, + Arc::unwrap_or_clone(right).try_into_ast(id)?, )), Expr::ExprNoExt(ExprNoExt::LessEq { left, right }) => Ok(ast::Expr::lesseq( - (*left).clone().try_into_ast(id.clone())?, - (*right).clone().try_into_ast(id)?, + Arc::unwrap_or_clone(left).try_into_ast(id.clone())?, + Arc::unwrap_or_clone(right).try_into_ast(id)?, )), Expr::ExprNoExt(ExprNoExt::Greater { left, right }) => Ok(ast::Expr::greater( - (*left).clone().try_into_ast(id.clone())?, - (*right).clone().try_into_ast(id)?, + Arc::unwrap_or_clone(left).try_into_ast(id.clone())?, + Arc::unwrap_or_clone(right).try_into_ast(id)?, )), Expr::ExprNoExt(ExprNoExt::GreaterEq { left, right }) => Ok(ast::Expr::greatereq( - (*left).clone().try_into_ast(id.clone())?, - (*right).clone().try_into_ast(id)?, + Arc::unwrap_or_clone(left).try_into_ast(id.clone())?, + Arc::unwrap_or_clone(right).try_into_ast(id)?, )), Expr::ExprNoExt(ExprNoExt::And { left, right }) => Ok(ast::Expr::and( - (*left).clone().try_into_ast(id.clone())?, - (*right).clone().try_into_ast(id)?, + Arc::unwrap_or_clone(left).try_into_ast(id.clone())?, + Arc::unwrap_or_clone(right).try_into_ast(id)?, )), Expr::ExprNoExt(ExprNoExt::Or { left, right }) => Ok(ast::Expr::or( - (*left).clone().try_into_ast(id.clone())?, - (*right).clone().try_into_ast(id)?, + Arc::unwrap_or_clone(left).try_into_ast(id.clone())?, + Arc::unwrap_or_clone(right).try_into_ast(id)?, )), Expr::ExprNoExt(ExprNoExt::Add { left, right }) => Ok(ast::Expr::add( - (*left).clone().try_into_ast(id.clone())?, - (*right).clone().try_into_ast(id)?, + Arc::unwrap_or_clone(left).try_into_ast(id.clone())?, + Arc::unwrap_or_clone(right).try_into_ast(id)?, )), Expr::ExprNoExt(ExprNoExt::Sub { left, right }) => Ok(ast::Expr::sub( - (*left).clone().try_into_ast(id.clone())?, - (*right).clone().try_into_ast(id)?, + Arc::unwrap_or_clone(left).try_into_ast(id.clone())?, + Arc::unwrap_or_clone(right).try_into_ast(id)?, )), Expr::ExprNoExt(ExprNoExt::Mul { left, right }) => Ok(ast::Expr::mul( - (*left).clone().try_into_ast(id.clone())?, - (*right).clone().try_into_ast(id)?, + Arc::unwrap_or_clone(left).try_into_ast(id.clone())?, + Arc::unwrap_or_clone(right).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)?, + Arc::unwrap_or_clone(left).try_into_ast(id.clone())?, + Arc::unwrap_or_clone(right).try_into_ast(id)?, )), Expr::ExprNoExt(ExprNoExt::ContainsAll { left, right }) => Ok(ast::Expr::contains_all( - (*left).clone().try_into_ast(id.clone())?, - (*right).clone().try_into_ast(id)?, + Arc::unwrap_or_clone(left).try_into_ast(id.clone())?, + Arc::unwrap_or_clone(right).try_into_ast(id)?, )), Expr::ExprNoExt(ExprNoExt::ContainsAny { left, right }) => Ok(ast::Expr::contains_any( - (*left).clone().try_into_ast(id.clone())?, - (*right).clone().try_into_ast(id)?, + Arc::unwrap_or_clone(left).try_into_ast(id.clone())?, + Arc::unwrap_or_clone(right).try_into_ast(id)?, + )), + Expr::ExprNoExt(ExprNoExt::IsEmpty { arg }) => Ok(ast::Expr::is_empty( + Arc::unwrap_or_clone(arg).try_into_ast(id)?, )), - Expr::ExprNoExt(ExprNoExt::IsEmpty { arg }) => { - Ok(ast::Expr::is_empty((*arg).clone().try_into_ast(id)?)) - } Expr::ExprNoExt(ExprNoExt::GetTag { left, right }) => Ok(ast::Expr::get_tag( - (*left).clone().try_into_ast(id.clone())?, - (*right).clone().try_into_ast(id)?, + Arc::unwrap_or_clone(left).try_into_ast(id.clone())?, + Arc::unwrap_or_clone(right).try_into_ast(id)?, )), Expr::ExprNoExt(ExprNoExt::HasTag { left, right }) => Ok(ast::Expr::has_tag( - (*left).clone().try_into_ast(id.clone())?, - (*right).clone().try_into_ast(id)?, + Arc::unwrap_or_clone(left).try_into_ast(id.clone())?, + Arc::unwrap_or_clone(right).try_into_ast(id)?, + )), + Expr::ExprNoExt(ExprNoExt::GetAttr { left, attr }) => Ok(ast::Expr::get_attr( + Arc::unwrap_or_clone(left).try_into_ast(id)?, + attr, + )), + Expr::ExprNoExt(ExprNoExt::HasAttr { left, attr }) => Ok(ast::Expr::has_attr( + Arc::unwrap_or_clone(left).try_into_ast(id)?, + attr, )), - Expr::ExprNoExt(ExprNoExt::GetAttr { left, attr }) => { - Ok(ast::Expr::get_attr((*left).clone().try_into_ast(id)?, attr)) - } - Expr::ExprNoExt(ExprNoExt::HasAttr { left, attr }) => { - Ok(ast::Expr::has_attr((*left).clone().try_into_ast(id)?, attr)) - } Expr::ExprNoExt(ExprNoExt::Like { left, pattern }) => Ok(ast::Expr::like( - (*left).clone().try_into_ast(id)?, + Arc::unwrap_or_clone(left).try_into_ast(id)?, crate::ast::Pattern::from(pattern.as_slice()), )), Expr::ExprNoExt(ExprNoExt::Is { @@ -926,14 +936,14 @@ impl Expr { }) => ast::EntityType::from_normalized_str(entity_type.as_str()) .map_err(FromJsonError::InvalidEntityType) .and_then(|entity_type_name| { - let left: ast::Expr = (*left).clone().try_into_ast(id.clone())?; + let left: ast::Expr = Arc::unwrap_or_clone(left).try_into_ast(id.clone())?; let is_expr = ast::Expr::is_entity_type(left.clone(), entity_type_name); match in_expr { // The AST doesn't have an `... is ... in ..` node, so // we represent it as a conjunction of `is` and `in`. Some(in_expr) => Ok(ast::Expr::and( is_expr, - ast::Expr::is_in(left, (*in_expr).clone().try_into_ast(id)?), + ast::Expr::is_in(left, Arc::unwrap_or_clone(in_expr).try_into_ast(id)?), )), None => Ok(is_expr), } @@ -943,9 +953,9 @@ impl Expr { then_expr, else_expr, }) => Ok(ast::Expr::ite( - (*cond_expr).clone().try_into_ast(id.clone())?, - (*then_expr).clone().try_into_ast(id.clone())?, - (*else_expr).clone().try_into_ast(id)?, + Arc::unwrap_or_clone(cond_expr).try_into_ast(id.clone())?, + Arc::unwrap_or_clone(then_expr).try_into_ast(id.clone())?, + Arc::unwrap_or_clone(else_expr).try_into_ast(id)?, )), Expr::ExprNoExt(ExprNoExt::Set(elements)) => Ok(ast::Expr::set( elements diff --git a/cedar-policy-core/src/evaluator.rs b/cedar-policy-core/src/evaluator.rs index 864eaca9e..5a434d72c 100644 --- a/cedar-policy-core/src/evaluator.rs +++ b/cedar-policy-core/src/evaluator.rs @@ -720,7 +720,7 @@ impl<'e> Evaluator<'e> { // `rhs` is a list of all the UIDs for which we need to // check if `uid1` is a descendant of let rhs = match arg2.value { - ValueKind::Lit(Literal::EntityUID(uid)) => vec![(*uid).clone()], + ValueKind::Lit(Literal::EntityUID(uid)) => vec![Arc::unwrap_or_clone(uid)], // we assume that iterating the `authoritative` BTreeSet is // approximately the same cost as iterating the `fast` HashSet ValueKind::Set(Set { authoritative, .. }) => authoritative