Fix implicit BTreeMap clones on Attributes::into_iter

cedar-language-server/src/policy/types/cedar.rs

@@ -223,7 +223,7 @@ impl From<Type> for CedarTypeKind {
             }),
             Type::Record { attrs, .. } => {
                 let m = attrs
-                    .into_iter()
+                    .iter()
                     .map(|kv_pair| (kv_pair.0.clone(), Attribute::from(kv_pair)))
                     .collect::<BTreeMap<_, _>>();
                 let record = Record { attrs: m.into() };

cedar-policy-core/src/validator/schema.rs

@@ -42,7 +42,7 @@ use crate::validator::{
     cedar_schema::SchemaWarning,
     json_schema,
     partition_nonempty::PartitionNonEmpty,
-    types::{Attributes, EntityKind, OpenTag, RequestEnv, Type, TypeIterator, UnlinkedRequestEnv},
+    types::{EntityKind, RequestEnv, Type, TypeIterator, UnlinkedRequestEnv},
     ValidationMode,
 };
 
@@ -770,17 +770,20 @@ impl ValidatorSchema {
                         parents: _,
                         tags,
                     } => {
-                        let (attributes, open_attributes) = {
-                            let attrs_ty = try_jsonschema_type_into_validator_type(
-                                attributes.0,
-                                extensions,
-                                &common_types,
-                            )?;
-                            Self::record_attributes_or_none(attrs_ty).ok_or_else(|| {
-                                ContextOrShapeNotRecordError {
-                                    ctx_or_shape: ContextOrShape::EntityTypeShape(name.clone()),
-                                }
-                            })?
+                        let attrs_ty = try_jsonschema_type_into_validator_type(
+                            attributes.0,
+                            extensions,
+                            &common_types,
+                        )?;
+                        let Type::Record {
+                            attrs,
+                            open_attributes,
+                        } = attrs_ty.ty
+                        else {
+                            return Err(ContextOrShapeNotRecordError {
+                                ctx_or_shape: ContextOrShape::EntityTypeShape(name),
+                            }
+                            .into());
                         };
                         let tags = tags
                             .map(|tags| {
@@ -797,7 +800,7 @@ impl ValidatorSchema {
                             ValidatorEntityType::new_standard(
                                 name.clone(),
                                 descendants,
-                                attributes,
+                                attrs,
                                 open_attributes,
                                 tags.map(|t| t.ty),
                                 name.loc().cloned(),
@@ -821,25 +824,25 @@ impl ValidatorSchema {
             .into_iter()
             .map(|(name, action)| -> Result<_> {
                 let descendants = action_children.remove(&name).unwrap_or_default();
-                let (context, open_context_attributes) = {
-                    let context_ty = try_jsonschema_type_into_validator_type(
-                        action.context,
-                        extensions,
-                        &common_types,
-                    )?;
-                    Self::record_attributes_or_none(context_ty).ok_or_else(|| {
-                        ContextOrShapeNotRecordError {
-                            ctx_or_shape: ContextOrShape::ActionContext(name.clone()),
-                        }
-                    })?
+                let context = try_jsonschema_type_into_validator_type(
+                    action.context,
+                    extensions,
+                    &common_types,
+                )?
+                .ty;
+                let Type::Record { .. } = context else {
+                    return Err(ContextOrShapeNotRecordError {
+                        ctx_or_shape: ContextOrShape::ActionContext(name),
+                    }
+                    .into());
                 };
                 Ok((
                     name.clone(),
                     ValidatorActionId {
                         name,
                         applies_to: action.applies_to,
                         descendants,
-                        context: Type::record_with_attributes(context, open_context_attributes),
+                        context,
                         loc: action.loc,
                     },
                 ))
@@ -956,18 +959,6 @@ impl ValidatorSchema {
         Ok(())
     }
 
-    fn record_attributes_or_none(ty: LocatedType) -> Option<(Attributes, OpenTag)> {
-        if let Type::Record {
-            attrs,
-            open_attributes,
-        } = ty.ty
-        {
-            Some((attrs, open_attributes))
-        } else {
-            None
-        }
-    }
-
     /// Check that all entity types appearing inside a type are in the set of
     /// declared entity types, adding any undeclared entity types to the
     /// `undeclared_types` set.
@@ -1748,8 +1739,11 @@ pub(crate) mod test {
         str::FromStr,
     };
 
-    use crate::validator::json_schema;
     use crate::validator::types::{AttributeType, Type};
+    use crate::validator::{
+        json_schema,
+        types::{Attributes, OpenTag},
+    };
 
     use crate::test_utils::{expect_err, ExpectedErrorMessageBuilder};
     use cool_asserts::assert_matches;

cedar-policy-core/src/validator/schema/namespace_def.rs

@@ -916,15 +916,16 @@ pub(crate) fn try_record_type_into_validator_type(
         Err(UnsupportedFeatureError(UnsupportedFeature::OpenRecordsAndEntities).into())
     } else {
         let attrs = parse_record_attributes(rty.attributes, extensions, common_type_defs)?;
+        let open_attributes = if rty.additional_attributes {
+            OpenTag::OpenAttributes
+        } else {
+            OpenTag::ClosedAttributes
+        };
         Ok(LocatedType::new_with_loc(
-            Type::record_with_attributes(
+            Type::Record {
                 attrs,
-                if rty.additional_attributes {
-                    OpenTag::OpenAttributes
-                } else {
-                    OpenTag::ClosedAttributes
-                },
-            ),
+                open_attributes,
+            },
             loc,
         ))
     }

cedar-policy-core/src/validator/typecheck.rs

@@ -784,7 +784,6 @@ impl<'a> SingleEnvTypechecker<'a> {
 
                 actual.then_typecheck(|typ_expr_actual, _| match typ_expr_actual.data() {
                     Some(typ_actual) => {
-                        let all_attrs = typ_actual.all_attributes(self.schema);
                         let attr_ty = Type::lookup_attribute_type(self.schema, typ_actual, attr);
                         let annot_expr = ExprBuilder::with_data(
                             attr_ty
@@ -836,8 +835,9 @@ impl<'a> SingleEnvTypechecker<'a> {
                                 )
                             }
                             None => {
+                                let all_attrs = typ_actual.all_attributes(self.schema);
                                 let borrowed =
-                                    all_attrs.iter().map(|s| s.as_str()).collect::<Vec<_>>();
+                                    all_attrs.keys().map(|s| s.as_str()).collect::<Vec<_>>();
                                 let suggestion = fuzzy_search(attr, &borrowed);
                                 type_errors.push(ValidationError::unsafe_attribute_access(
                                     e.source_loc().cloned(),

cedar-policy-core/src/validator/types.rs

@@ -418,11 +418,11 @@ impl Type {
     /// Get all statically known attributes of an entity or record type.
     /// Returns an empty vector if there are no declared attributes or the type
     /// is not an entity or record type.
-    pub fn all_attributes(&self, schema: &ValidatorSchema) -> Vec<SmolStr> {
+    pub fn all_attributes(&self, schema: &ValidatorSchema) -> Attributes {
         match self {
             Type::Entity(e) => e.all_known_attrs(schema),
-            Type::Record { attrs, .. } => attrs.attrs.keys().cloned().collect(),
-            _ => vec![],
+            Type::Record { attrs, .. } => attrs.clone(),
+            _ => Attributes::with_attributes(None),
         }
     }
 
@@ -764,11 +764,11 @@ impl TryFrom<Type> for CoreSchemaType {
             } => Ok(CoreSchemaType::Record {
                 attrs: {
                     attrs
-                        .into_iter()
+                        .iter()
                         .map(|(k, v)| {
                             let schema_type = v.attr_type.as_ref().clone().try_into()?;
                             Ok((
-                                k,
+                                k.clone(),
                                 match v.is_required {
                                     true => CoreAttributeType::required(schema_type),
                                     false => CoreAttributeType::optional(schema_type),
@@ -876,12 +876,10 @@ impl EntityLUB {
             clippy::expect_used,
             reason = "Invariant on `lub_elements` guarantees the set is non-empty"
         )]
-        let arbitrary_first = Attributes::with_attributes(
-            lub_element_attributes
-                .next()
-                .expect("Invariant violated: EntityLUB set must be non-empty."),
-        );
-        lub_element_attributes.fold(arbitrary_first, |acc, elem| {
+        let arbitrary_first = lub_element_attributes
+            .next()
+            .expect("Invariant violated: EntityLUB set must be non-empty.");
+        lub_element_attributes.fold(arbitrary_first, move |acc, elem| {
             // Use the permissive version of least upper bound here for two
             // reasons. First, when in permissive mode, the attributes least
             // upper bound can never fail. We could call the main lub function
@@ -890,7 +888,7 @@ impl EntityLUB {
             // element, so that LUB can never fail, and the strict
             // attributes lub is the same as permissive if there is only one
             // attribute.
-            Attributes::permissive_least_upper_bound(&acc, &Attributes::with_attributes(elem))
+            Attributes::permissive_least_upper_bound(&acc, &elem)
         })
     }
 
@@ -1044,16 +1042,6 @@ impl Attributes {
     }
 }
 
-impl IntoIterator for Attributes {
-    type Item = (SmolStr, AttributeType);
-
-    type IntoIter = <BTreeMap<SmolStr, AttributeType> as IntoIterator>::IntoIter;
-
-    fn into_iter(self) -> Self::IntoIter {
-        self.attrs.as_ref().clone().into_iter()
-    }
-}
-
 /// Used to tag record types to indicate if their attributes record is open or
 /// closed.
 #[derive(Hash, Ord, PartialOrd, Eq, PartialEq, Debug, Copy, Clone, Default)]
@@ -1144,24 +1132,18 @@ impl EntityKind {
         }
     }
 
-    /// Get all the attribute names _known to exist_ for this entity.
+    /// Get all the attributes _known to exist_ for this entity.
     ///
-    /// For `AnyEntity`, this will return an empty vec, as there are no
+    /// For `AnyEntity`, this will be empty, as there are no
     /// attribute names we _know_ must exist (even though `AnyEntity` types may
     /// clearly have attributes).
     /// For LUB types, this will return only the attribute names known to exist
     /// in the LUB.
-    pub fn all_known_attrs(&self, schema: &ValidatorSchema) -> Vec<SmolStr> {
+    pub fn all_known_attrs(&self, schema: &ValidatorSchema) -> Attributes {
         // Wish the clone here could be avoided, but `get_attribute_types` returns an owned `Attributes`.
         match self {
-            EntityKind::AnyEntity => vec![],
-            EntityKind::Entity(lub) => lub
-                .get_attribute_types(schema)
-                .attrs
-                .as_ref()
-                .keys()
-                .cloned()
-                .collect(),
+            EntityKind::AnyEntity => Attributes::with_attributes(None),
+            EntityKind::Entity(lub) => lub.get_attribute_types(schema),
         }
     }
 

cedar-policy-symcc/src/symcc/env.rs

@@ -615,7 +615,10 @@ impl<'a> Environment<'a> {
 
     /// Returns the type of the context.
     pub fn context_type(&self) -> Type {
-        Type::record_with_attributes(self.req_ty.context.clone(), OpenTag::ClosedAttributes)
+        Type::Record {
+            attrs: self.req_ty.context.clone(),
+            open_attributes: OpenTag::ClosedAttributes,
+        }
     }
 }