Pull in PRs

cedar-policy-core/src/ast/entity.rs

@@ -317,8 +317,17 @@ pub struct Entity {
     ancestors: HashSet<EntityUID>,
 }
 
+impl std::hash::Hash for Entity {
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        self.uid.hash(state);
+    }
+}
+
 impl Entity {
     /// Create a new `Entity` with this UID, attributes, and ancestors
+    ///
+    /// # Errors
+    /// - Will error if any of the [`RestrictedExpr]`s in `attrs` error when evaluated
     pub fn new(
         uid: EntityUID,
         attrs: HashMap<SmolStr, RestrictedExpr>,
@@ -346,6 +355,13 @@ impl Entity {
         })
     }
 
+    /// Create a new `Entity` with this UID, ancestors, and an empty set of attributes
+    ///
+    /// Since there are no attributes, this method does not error, and returns `Self` instead of `Result<Self>`
+    pub fn new_empty_attrs(uid: EntityUID, ancestors: HashSet<EntityUID>) -> Self {
+        Self::new_with_attr_partial_value(uid, HashMap::new(), ancestors)
+    }
+
     /// Create a new `Entity` with this UID, attributes, and ancestors.
     ///
     /// Unlike in `Entity::new()`, in this constructor, attributes are expressed

cedar-policy-core/src/entities.rs

@@ -162,6 +162,13 @@ impl Entities {
     ///
     /// If you pass `TCComputation::AssumeAlreadyComputed`, then the caller is
     /// responsible for ensuring that TC and DAG hold before calling this method.
+    ///
+    /// # Errors
+    /// - [`EntitiesError::Duplicate`] if there are any duplicate entities in `entities`
+    /// - [`EntitiesError::TransitiveClosureError`] if `tc_computation ==
+    ///   TCComputation::EnforceAlreadyComputed` and the entities are not transitivly closed
+    /// - [`EntitiesError::InvalidEntity`] if `schema` is not none and any entities do not conform
+    ///   to the schema
     pub fn from_entities(
         entities: impl IntoIterator<Item = Entity>,
         schema: Option<&impl Schema>,
@@ -817,7 +824,7 @@ mod json_parsing_tests {
         parser.from_json_value(json).expect("JSON is correct")
     }
 
-    /// Ensure the initial conditions of the entiites still hold
+    /// Ensure the initial conditions of the entities still hold
     fn simple_entities_still_sane(e: &Entities) {
         let bob = r#"Test::"bob""#.parse().unwrap();
         let alice = e.entity(&r#"Test::"alice""#.parse().unwrap()).unwrap();
@@ -2401,7 +2408,7 @@ mod schema_based_parsing_tests {
                 &entitiesjson,
                 &miette::Report::new(e),
                 &ExpectedErrorMessageBuilder::error("error during entity deserialization")
-                    .source(r#"in attribute `hr_contacts` on `Employee::"12UA45"`, type mismatch: value was expected to have type (set of `HR`), but actually has type record with attributes: {"id" => (optional) string, "type" => (optional) string}: `{"id": "aaaaa", "type": "HR"}`"#)
+                    .source(r#"in attribute `hr_contacts` on `Employee::"12UA45"`, type mismatch: value was expected to have type [`HR`], but actually has type { "id" => (optional) string, "type" => (optional) string }: `{"id": "aaaaa", "type": "HR"}`"#)
                     .build()
             );
         });
@@ -2589,7 +2596,7 @@ mod schema_based_parsing_tests {
                 &entitiesjson,
                 &miette::Report::new(e),
                 &ExpectedErrorMessageBuilder::error_starts_with("entity does not conform to the schema")
-                    .source(r#"in attribute `json_blob` on `Employee::"12UA45"`, type mismatch: value was expected to have type record with attributes: "#)
+                    .source(r#"in attribute `json_blob` on `Employee::"12UA45"`, type mismatch: value was expected to have type {"#)
                     .build()
             );
         });

cedar-policy-core/src/entities/conformance.rs

@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 
+use std::collections::BTreeMap;
+
 use super::{
     json::err::TypeMismatchError, schematype_of_restricted_expr, EntityTypeDescription,
     GetSchemaTypeError, HeterogeneousSetError, Schema, SchemaType,
@@ -24,6 +26,7 @@ use crate::ast::{
 use crate::extensions::{ExtensionFunctionLookupError, Extensions};
 use either::Either;
 use miette::Diagnostic;
+use smol_str::SmolStr;
 use thiserror::Error;
 pub mod err;
 
@@ -176,6 +179,60 @@ pub fn typecheck_value_against_schematype(
     }
 }
 
+/// Check whether the given `RestrictedExpr` is a valid instance of `SchemaType`
+pub fn does_restricted_expr_implement_schematype(
+    expr: BorrowedRestrictedExpr<'_>,
+    expected_ty: &SchemaType,
+) -> bool {
+    use SchemaType::*;
+
+    match expected_ty {
+        Bool => expr.as_bool().is_some(),
+        Long => expr.as_long().is_some(),
+        String => expr.as_string().is_some(),
+        EmptySet => expr.as_set_elements().is_some_and(|e| e.count() == 0),
+        Set { .. } if expr.as_set_elements().is_some_and(|e| e.count() == 0) => true,
+        Set { element_ty: elty } => match expr.as_set_elements() {
+            Some(mut els) => els.all(|e| does_restricted_expr_implement_schematype(e, elty)),
+            None => false,
+        },
+        Record { attrs, open_attrs } => match expr.as_record_pairs() {
+            Some(pairs) => {
+                let pairs_map: BTreeMap<&SmolStr, BorrowedRestrictedExpr<'_>> = pairs.collect();
+                let all_req_schema_attrs_in_record = attrs.iter().all(|(k, v)| {
+                    !v.required
+                        || match pairs_map.get(k) {
+                            Some(inner_e) => {
+                                does_restricted_expr_implement_schematype(*inner_e, &v.attr_type)
+                            }
+                            None => false,
+                        }
+                });
+                let all_rec_attrs_match_schema =
+                    pairs_map.iter().all(|(k, inner_e)| match attrs.get(*k) {
+                        Some(sch_ty) => {
+                            does_restricted_expr_implement_schematype(*inner_e, &sch_ty.attr_type)
+                        }
+                        None => *open_attrs,
+                    });
+                all_rec_attrs_match_schema && all_req_schema_attrs_in_record
+            }
+            None => false,
+        },
+        Extension { name } => match expr.as_extn_fn_call() {
+            Some((actual_name, _)) => match name.0.id.as_ref() {
+                "ipaddr" => actual_name.0.id.as_ref() == "ip",
+                _ => name == actual_name,
+            },
+            None => false,
+        },
+        Entity { ty } => match expr.as_euid() {
+            Some(actual_euid) => actual_euid.entity_type() == ty,
+            None => false,
+        },
+    }
+}
+
 /// Check whether the given `RestrictedExpr` typechecks with the given `SchemaType`.
 /// If the typecheck passes, return `Ok(())`.
 /// If the typecheck fails, return an appropriate `Err`.
@@ -184,23 +241,15 @@ pub fn typecheck_restricted_expr_against_schematype(
     expected_ty: &SchemaType,
     extensions: &Extensions<'_>,
 ) -> Result<(), TypecheckError> {
-    // TODO(#440): instead of computing the `SchemaType` of `expr` and then
-    // checking whether the schematypes are "consistent", wouldn't it be less
-    // confusing, more efficient, and maybe even more precise to just typecheck
-    // directly?
+    if does_restricted_expr_implement_schematype(expr, expected_ty) {
+        return Ok(());
+    }
     match schematype_of_restricted_expr(expr, extensions) {
-        Ok(actual_ty) => {
-            if actual_ty.is_consistent_with(expected_ty) {
-                // typecheck passes
-                Ok(())
-            } else {
-                Err(TypecheckError::TypeMismatch(TypeMismatchError {
-                    expected: Box::new(expected_ty.clone()),
-                    actual_ty: Some(Box::new(actual_ty)),
-                    actual_val: Either::Right(Box::new(expr.to_owned())),
-                }))
-            }
-        }
+        Ok(actual_ty) => Err(TypecheckError::TypeMismatch(TypeMismatchError {
+            expected: Box::new(expected_ty.clone()),
+            actual_ty: Some(Box::new(actual_ty)),
+            actual_val: Either::Right(Box::new(expr.to_owned())),
+        })),
         Err(GetSchemaTypeError::UnknownInsufficientTypeInfo { .. }) => {
             // in this case we just don't have the information to know whether
             // the attribute value (an unknown) matches the expected type.

cedar-policy-core/src/entities/json/schema_types.rs

@@ -69,9 +69,9 @@ pub enum SchemaType {
 #[derive(Debug, Hash, PartialEq, Eq, Clone)]
 pub struct AttributeType {
     /// Type of the attribute
-    attr_type: SchemaType,
+    pub(crate) attr_type: SchemaType,
     /// Is the attribute required
-    required: bool,
+    pub(crate) required: bool,
 }
 
 impl SchemaType {
@@ -220,38 +220,31 @@ impl std::fmt::Display for SchemaType {
             Self::Bool => write!(f, "bool"),
             Self::Long => write!(f, "long"),
             Self::String => write!(f, "string"),
-            Self::Set { element_ty } => write!(f, "(set of {})", &element_ty),
-            Self::EmptySet => write!(f, "empty-set"),
+            Self::Set { element_ty } => write!(f, "[{element_ty}]"),
+            Self::EmptySet => write!(f, "[]"),
             Self::Record { attrs, open_attrs } => {
-                if attrs.is_empty() && *open_attrs {
-                    write!(f, "any record")
-                } else if attrs.is_empty() {
-                    write!(f, "empty record")
-                } else {
-                    if *open_attrs {
-                        write!(f, "record with at least attributes: {{")?;
-                    } else {
-                        write!(f, "record with attributes: {{")?;
+                write!(f, "{{ ")?;
+                // sorting attributes ensures that there is a single, deterministic
+                // Display output for each `SchemaType`, which is important for
+                // tests that check equality of error messages
+                for (i, (k, v)) in attrs
+                    .iter()
+                    .sorted_unstable_by_key(|(k, _)| SmolStr::clone(k))
+                    .enumerate()
+                {
+                    write!(f, "{k:?} => {v}")?;
+                    if i < (attrs.len() - 1) {
+                        write!(f, ", ")?;
                     }
-                    // sorting attributes ensures that there is a single, deterministic
-                    // Display output for each `SchemaType`, which is important for
-                    // tests that check equality of error messages
-                    for (i, (k, v)) in attrs
-                        .iter()
-                        .sorted_unstable_by_key(|(k, _)| SmolStr::clone(k))
-                        .enumerate()
-                    {
-                        write!(f, "{k:?} => {v}")?;
-                        if i < (attrs.len() - 1) {
-                            write!(f, ", ")?;
-                        }
-                    }
-                    write!(f, "}}")?;
-                    Ok(())
                 }
+                if *open_attrs {
+                    write!(f, ", ...")?;
+                }
+                write!(f, " }}")?;
+                Ok(())
             }
             Self::Entity { ty } => write!(f, "`{ty}`"),
-            Self::Extension { name } => write!(f, "{}", name),
+            Self::Extension { name } => write!(f, "{name}"),
         }
     }
 }
@@ -361,11 +354,8 @@ impl Diagnostic for NontrivialResidualError {
 /// required or optional.
 /// This function, when given a record that has keys A, B, and C, will return a
 /// `SchemaType` where A, B, and C are all marked as optional attributes, but no
-/// other attributes are possible.
-/// That is, this assumes that all existing attributes are optional, but that no
-/// other optional attributes are possible.
-/// Compared to marking A, B, and C as required, this allows the returned
-/// `SchemaType` to `is_consistent_with()` more types.
+/// other attributes are possible. This maximized flexibility while avoiding
+/// heterogeneous sets.
 ///
 /// This function may return `GetSchemaTypeError`, but should never return
 /// `NontrivialResidual`, because `RestrictedExpr`s can't contain nontrivial
@@ -389,9 +379,8 @@ pub fn schematype_of_restricted_expr(
                         BorrowedRestrictedExpr::new_unchecked(v), // assuming the invariant holds for the record as a whole, it will also hold for each attribute value
                         extensions,
                     )?;
-                    // we can't know if the attribute is required or optional,
-                    // but marking it optional is more flexible -- allows the
-                    // attribute type to `is_consistent_with()` more types
+                    // We can't know if the attribute is required or optional.
+                    // Keep as optional to minimize heterogeneous sets.
                     Ok((k.clone(), AttributeType::optional(attr_type)))
                 }).collect::<Result<BTreeMap<_,_>, GetSchemaTypeError>>()?,
                 open_attrs: false,

cedar-policy-validator/src/cedar_schema/test.rs

@@ -2337,7 +2337,7 @@ mod ea_maps {
     #[test]
     fn entity_attribute() {
         let src = "entity E { tags: { ?: Set<String> } };";
-        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, &Extensions::all_available())), Ok((frag, warnings)) => {
+        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Ok((frag, warnings)) => {
             assert!(warnings.is_empty());
             let entity_type = frag.0.get(&None).unwrap().entity_types.get(&"E".parse().unwrap()).unwrap();
             assert_matches!(&entity_type.shape, json_schema::EntityAttributes::EntityAttributes(json_schema::EntityAttributesInternal { attrs, .. }) => {
@@ -2356,7 +2356,7 @@ mod ea_maps {
     #[test]
     fn record_attribute_inside_entity_attribute() {
         let src = "entity E { tags: { foo: { ?: Set<String> } } };";
-        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, &Extensions::all_available())), Err(e) => {
+        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Err(e) => {
             expect_err(
                 src,
                 &miette::Report::new(e),
@@ -2370,7 +2370,7 @@ mod ea_maps {
     #[test]
     fn context_attribute() {
         let src = "entity E; action read appliesTo { principal: [E], resource: [E], context: { operationDetails: { ?: String } } };";
-        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, &Extensions::all_available())), Err(e) => {
+        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Err(e) => {
             expect_err(
                 src,
                 &miette::Report::new(e),
@@ -2384,7 +2384,7 @@ mod ea_maps {
     #[test]
     fn toplevel_entity() {
         let src = "entity E { ?: Set<String> };";
-        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, &Extensions::all_available())), Err(e) => {
+        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Err(e) => {
             expect_err(
                 src,
                 &miette::Report::new(e),
@@ -2399,7 +2399,7 @@ mod ea_maps {
     #[test]
     fn toplevel_context() {
         let src = "entity E; action read appliesTo { principal: [E], resource: [E], context: { ?: String } };";
-        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, &Extensions::all_available())), Err(e) => {
+        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Err(e) => {
             expect_err(
                 src,
                 &miette::Report::new(e),
@@ -2413,7 +2413,7 @@ mod ea_maps {
     #[test]
     fn common_type() {
         let src = "type blah = { ?: String }; entity User { blah: blah };";
-        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, &Extensions::all_available())), Err(e) => {
+        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Err(e) => {
             expect_err(
                 src,
                 &miette::Report::new(e),
@@ -2427,7 +2427,7 @@ mod ea_maps {
     #[test]
     fn value_type_is_common_type() {
         let src = "type blah = { foo: String }; entity User { blah: { ?: blah } };";
-        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, &Extensions::all_available())), Ok((frag, warnings)) => {
+        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Ok((frag, warnings)) => {
             assert!(warnings.is_empty());
             let user = frag.0.get(&None).unwrap().entity_types.get(&"User".parse().unwrap()).unwrap();
             assert_matches!(&user.shape, json_schema::EntityAttributes::EntityAttributes(json_schema::EntityAttributesInternal { attrs, .. }) => {
@@ -2444,7 +2444,7 @@ mod ea_maps {
     #[test]
     fn nested_ea_map() {
         let src = "entity E { tags: { ?: { ?: String } } };";
-        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, &Extensions::all_available())), Err(e) => {
+        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Err(e) => {
             expect_err(
                 src,
                 &miette::Report::new(e),
@@ -2458,7 +2458,7 @@ mod ea_maps {
     #[test]
     fn ea_map_and_attribute() {
         let src = "entity E { tags: { foo: Long, ?: String } };";
-        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, &Extensions::all_available())), Err(e) => {
+        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Err(e) => {
             expect_err(
                 src,
                 &miette::Report::new(e),
@@ -2472,7 +2472,7 @@ mod ea_maps {
     #[test]
     fn two_ea_maps() {
         let src = "entity E { tags: { ?: Long, ?: String } };";
-        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, &Extensions::all_available())), Err(e) => {
+        assert_matches!(collect_warnings(json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available())), Err(e) => {
             expect_err(
                 src,
                 &miette::Report::new(e),

cedar-policy-validator/src/coreschema.rs

@@ -501,7 +501,7 @@ mod test {
                 }
             }
         }});
-        ValidatorSchema::from_json_value(src, &Extensions::all_available())
+        ValidatorSchema::from_json_value(src, Extensions::all_available())
             .expect("failed to create ValidatorSchema")
     }
 

cedar-policy-validator/src/json_schema.rs

@@ -1014,7 +1014,7 @@ impl ActionEntityUID<ConditionalName> {
                 }
             }
         }
-        Err(ActionNotDefinedError(nonempty!(self)).into())
+        Err(ActionNotDefinedError(nonempty!(self)))
     }
 
     /// Get the possible fully-qualified [`ActionEntityUID<InternalName>`]s
@@ -2564,7 +2564,7 @@ mod test {
                 "actions": {}
             }
         });
-        let schema = ValidatorSchema::from_json_value(src.clone(), &Extensions::all_available());
+        let schema = ValidatorSchema::from_json_value(src.clone(), Extensions::all_available());
         assert_matches!(schema, Err(e) => {
             expect_err(
                 &src,