Merge branch 'main' into unknowns

cedar-policy · Jan 4, 2025 · 3ac8ad6 · 3ac8ad6
2 parents a63ed5b + 59adc6d
commit 3ac8ad6
Show file tree

Hide file tree

Showing 46 changed files with 1,496 additions and 1,048 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -43,6 +43,7 @@ rust-2018-idioms = "deny"
 # means we'll see it in local runs.
 [workspace.lints.clippy]
 nursery = { level = "warn", priority = -1 }
+needless_pass_by_value = "warn"
 
 # These lints may be worth enforcing, but cause a lot of noise at the moment.
 use_self = "allow"

diff --git a/cedar-policy-cli/Cargo.toml b/cedar-policy-cli/Cargo.toml
@@ -37,8 +37,8 @@ protobufs = ["dep:prost", "dep:prost-build", "cedar-policy/protobufs", "cedar-po
 [dev-dependencies]
 assert_cmd = "2.0"
 tempfile = "3"
-glob = "0.3.1"
-predicates = "3.1.0"
+glob = "0.3.2"
+predicates = "3.1.3"
 rstest = "0.23.0"
 
 # We override the name of the binary for src/main.rs, which otherwise would be

diff --git a/cedar-policy-cli/src/lib.rs b/cedar-policy-cli/src/lib.rs
@@ -947,7 +947,7 @@ fn translate_policy_inner(args: &TranslatePolicyArgs) -> Result<String> {
     let translate = match args.direction {
         PolicyTranslationDirection::CedarToJson => translate_policy_to_json,
     };
-    read_from_file_or_stdin(args.input_file.clone(), "policy").and_then(translate)
+    read_from_file_or_stdin(args.input_file.as_ref(), "policy").and_then(translate)
 }
 
 pub fn translate_policy(args: &TranslatePolicyArgs) -> CedarExitCode {
@@ -984,7 +984,7 @@ fn translate_schema_inner(args: &TranslateSchemaArgs) -> Result<String> {
         SchemaTranslationDirection::JsonToCedar => translate_schema_to_cedar,
         SchemaTranslationDirection::CedarToJson => translate_schema_to_json,
     };
-    read_from_file_or_stdin(args.input_file.clone(), "schema").and_then(translate)
+    read_from_file_or_stdin(args.input_file.as_ref(), "schema").and_then(translate)
 }
 
 pub fn translate_schema(args: &TranslateSchemaArgs) -> CedarExitCode {
@@ -1377,7 +1377,7 @@ fn load_entities(entities_filename: impl AsRef<Path>, schema: Option<&Schema>) -
 /// This will rename template-linked policies to the id of their template, which may
 /// cause id conflicts, so only call this function before instancing
 /// templates into the policy set.
-fn rename_from_id_annotation(ps: PolicySet) -> Result<PolicySet> {
+fn rename_from_id_annotation(ps: &PolicySet) -> Result<PolicySet> {
     let mut new_ps = PolicySet::new();
     let t_iter = ps.templates().map(|t| match t.annotation("id") {
         None => Ok(t.clone()),
@@ -1443,7 +1443,7 @@ fn read_cedar_policy_set(
             Report::new(err).with_source_code(NamedSource::new(name, ps_str))
         })
         .wrap_err_with(|| format!("failed to parse {context}"))?;
-    rename_from_id_annotation(ps)
+    rename_from_id_annotation(&ps)
 }
 
 /// Read a policy set, static policy or policy template, in Cedar JSON (EST) syntax, from the file given

diff --git a/cedar-policy-cli/tests/sample.rs b/cedar-policy-cli/tests/sample.rs
@@ -106,7 +106,7 @@ fn run_link_test(
     links_file: impl Into<String>,
     template_id: impl Into<String>,
     linked_id: impl Into<String>,
-    env: HashMap<SlotId, String>,
+    env: impl IntoIterator<Item = (SlotId, String)>,
     expected: CedarExitCode,
 ) {
     let cmd = LinkArgs {
@@ -117,7 +117,9 @@ fn run_link_test(
         },
         template_id: template_id.into(),
         new_id: linked_id.into(),
-        arguments: Arguments { data: env },
+        arguments: Arguments {
+            data: HashMap::from_iter(env),
+        },
     };
     let output = link(&cmd);
     assert_eq!(output, expected);
@@ -792,9 +794,7 @@ fn test_link_samples() {
         &linked_file_name,
         "AccessVacation",
         "AliceAccess",
-        [(SlotId::principal(), "User::\"alice\"".to_string())]
-            .into_iter()
-            .collect(),
+        [(SlotId::principal(), "User::\"alice\"".to_string())],
         CedarExitCode::Failure,
     );
 
@@ -803,9 +803,7 @@ fn test_link_samples() {
         &linked_file_name,
         "AccessVacation",
         "AliceAccess",
-        [(SlotId::principal(), "invalid".to_string())]
-            .into_iter()
-            .collect(),
+        [(SlotId::principal(), "invalid".to_string())],
         CedarExitCode::Failure,
     );
 
@@ -814,9 +812,7 @@ fn test_link_samples() {
         &linked_file_name,
         "AccessVacation",
         "AliceAccess",
-        [(SlotId::principal(), "User::\"alice\"".to_string())]
-            .into_iter()
-            .collect(),
+        [(SlotId::principal(), "User::\"alice\"".to_string())],
         CedarExitCode::Success,
     );
 
@@ -845,9 +841,7 @@ fn test_link_samples() {
         &linked_file_name,
         "AccessVacation",
         "BobAccess",
-        [(SlotId::principal(), "User::\"bob\"".to_string())]
-            .into_iter()
-            .collect(),
+        [(SlotId::principal(), "User::\"bob\"".to_string())],
         CedarExitCode::Success,
     );
 

diff --git a/cedar-policy-core/Cargo.toml b/cedar-policy-core/Cargo.toml
@@ -14,7 +14,7 @@ repository.workspace = true
 
 [dependencies]
 serde = { version = "1.0", features = ["derive", "rc"] }
-serde_with = { version = "3.0", features = ["json"] }
+serde_with = { version = "3.12", features = ["json"] }
 serde_json = "1.0"
 lalrpop-util = { version = "0.22.0", features = ["lexer"] }
 lazy_static = "1.4"

diff --git a/cedar-policy-core/src/ast/extension.rs b/cedar-policy-core/src/ast/extension.rs
@@ -124,9 +124,22 @@ pub enum CallStyle {
 
 // Note: we could use currying to make this a little nicer
 
-/// Trait object that implements the extension function call.
-pub type ExtensionFunctionObject =
-    Box<dyn Fn(&[Value]) -> evaluator::Result<ExtensionOutputValue> + Sync + Send + 'static>;
+macro_rules! extension_function_object {
+    ( $( $tys:ty ), * ) => {
+        Box<dyn Fn($($tys,)*) -> evaluator::Result<ExtensionOutputValue> + Sync + Send + 'static>
+    }
+}
+
+/// Trait object that implements the extension function call accepting any number of arguments.
+pub type ExtensionFunctionObject = extension_function_object!(&[Value]);
+/// Trait object that implements the extension function call accepting exactly 0 arguments
+pub type NullaryExtensionFunctionObject = extension_function_object!();
+/// Trait object that implements the extension function call accepting exactly 1 arguments
+pub type UnaryExtensionFunctionObject = extension_function_object!(&Value);
+/// Trait object that implements the extension function call accepting exactly 2 arguments
+pub type BinaryExtensionFunctionObject = extension_function_object!(&Value, &Value);
+/// Trait object that implements the extension function call accepting exactly 3 arguments
+pub type TernaryExtensionFunctionObject = extension_function_object!(&Value, &Value, &Value);
 
 /// Extension function. These can be called by the given `name` in Ceder
 /// expressions.
@@ -172,7 +185,7 @@ impl ExtensionFunction {
     pub fn nullary(
         name: Name,
         style: CallStyle,
-        func: Box<dyn Fn() -> evaluator::Result<ExtensionOutputValue> + Sync + Send + 'static>,
+        func: NullaryExtensionFunctionObject,
         return_type: SchemaType,
     ) -> Self {
         Self::new(
@@ -200,14 +213,14 @@ impl ExtensionFunction {
     pub fn partial_eval_unknown(
         name: Name,
         style: CallStyle,
-        func: Box<dyn Fn(Value) -> evaluator::Result<ExtensionOutputValue> + Sync + Send + 'static>,
+        func: UnaryExtensionFunctionObject,
         arg_type: SchemaType,
     ) -> Self {
         Self::new(
             name.clone(),
             style,
             Box::new(move |args: &[Value]| match args.first() {
-                Some(arg) => func(arg.clone()),
+                Some(arg) => func(arg),
                 None => Err(evaluator::EvaluationError::wrong_num_arguments(
                     name.clone(),
                     1,
@@ -221,18 +234,19 @@ impl ExtensionFunction {
     }
 
     /// Create a new `ExtensionFunction` taking one argument
+    #[allow(clippy::type_complexity)]
     pub fn unary(
         name: Name,
         style: CallStyle,
-        func: Box<dyn Fn(Value) -> evaluator::Result<ExtensionOutputValue> + Sync + Send + 'static>,
+        func: UnaryExtensionFunctionObject,
         return_type: SchemaType,
         arg_type: SchemaType,
     ) -> Self {
         Self::new(
             name.clone(),
             style,
             Box::new(move |args: &[Value]| match &args {
-                &[arg] => func(arg.clone()),
+                &[arg] => func(arg),
                 _ => Err(evaluator::EvaluationError::wrong_num_arguments(
                     name.clone(),
                     1,
@@ -246,20 +260,19 @@ impl ExtensionFunction {
     }
 
     /// Create a new `ExtensionFunction` taking two arguments
+    #[allow(clippy::type_complexity)]
     pub fn binary(
         name: Name,
         style: CallStyle,
-        func: Box<
-            dyn Fn(Value, Value) -> evaluator::Result<ExtensionOutputValue> + Sync + Send + 'static,
-        >,
+        func: BinaryExtensionFunctionObject,
         return_type: SchemaType,
         arg_types: (SchemaType, SchemaType),
     ) -> Self {
         Self::new(
             name.clone(),
             style,
             Box::new(move |args: &[Value]| match &args {
-                &[first, second] => func(first.clone(), second.clone()),
+                &[first, second] => func(first, second),
                 _ => Err(evaluator::EvaluationError::wrong_num_arguments(
                     name.clone(),
                     2,
@@ -273,23 +286,19 @@ impl ExtensionFunction {
     }
 
     /// Create a new `ExtensionFunction` taking three arguments
+    #[allow(clippy::type_complexity)]
     pub fn ternary(
         name: Name,
         style: CallStyle,
-        func: Box<
-            dyn Fn(Value, Value, Value) -> evaluator::Result<ExtensionOutputValue>
-                + Sync
-                + Send
-                + 'static,
-        >,
+        func: TernaryExtensionFunctionObject,
         return_type: SchemaType,
         arg_types: (SchemaType, SchemaType, SchemaType),
     ) -> Self {
         Self::new(
             name.clone(),
             style,
             Box::new(move |args: &[Value]| match &args {
-                &[first, second, third] => func(first.clone(), second.clone(), third.clone()),
+                &[first, second, third] => func(first, second, third),
                 _ => Err(evaluator::EvaluationError::wrong_num_arguments(
                     name.clone(),
                     3,

diff --git 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"),
         }
@@ -630,11 +630,10 @@ mod test {
         .expect("Failed to parse");
         pset.add_template(template).expect("Add failed");
 
-        let env: HashMap<SlotId, EntityUID> = [(
+        let env: HashMap<SlotId, EntityUID> = std::iter::once((
             SlotId::principal(),
             r#"Test::"test""#.parse().expect("Failed to parse"),
-        )]
-        .into_iter()
+        ))
         .collect();
 
         let r = pset.link(PolicyID::from_string("t"), PolicyID::from_string("id"), env);
@@ -669,11 +668,10 @@ mod test {
             )
             .expect("Failed to parse"),
         );
-        let env1: HashMap<SlotId, EntityUID> = [(
+        let env1: HashMap<SlotId, EntityUID> = std::iter::once((
             SlotId::principal(),
             r#"Test::"test1""#.parse().expect("Failed to parse"),
-        )]
-        .into_iter()
+        ))
         .collect();
 
         let p1 = Template::link(Arc::clone(&template), PolicyID::from_string("link"), env1)
@@ -686,11 +684,10 @@ mod test {
             "Adding link should implicitly add the template"
         );
 
-        let env2: HashMap<SlotId, EntityUID> = [(
+        let env2: HashMap<SlotId, EntityUID> = std::iter::once((
             SlotId::principal(),
             r#"Test::"test2""#.parse().expect("Failed to parse"),
-        )]
-        .into_iter()
+        ))
         .collect();
 
         let p2 = Template::link(
@@ -717,11 +714,10 @@ mod test {
             )
             .expect("Failed to parse"),
         );
-        let env3: HashMap<SlotId, EntityUID> = [(
+        let env3: HashMap<SlotId, EntityUID> = std::iter::once((
             SlotId::resource(),
             r#"Test::"test3""#.parse().expect("Failed to parse"),
-        )]
-        .into_iter()
+        ))
         .collect();
 
         let p4 = Template::link(
@@ -781,9 +777,7 @@ mod test {
         set.link(
             PolicyID::from_string("template"),
             PolicyID::from_string("id"),
-            [(SlotId::principal(), EntityUID::with_eid("eid"))]
-                .into_iter()
-                .collect(),
+            std::iter::once((SlotId::principal(), EntityUID::with_eid("eid"))).collect(),
         )
         .expect("Linking failed!");
         assert_eq!(set.static_policies().count(), 1);