From 560417cb0a5d1dbaddc773a2e685f3b34ccf0e43 Mon Sep 17 00:00:00 2001 From: kennethloeffler Date: Fri, 9 Aug 2024 18:24:31 +0100 Subject: [PATCH] Intern property and class names, use UstrMap instead of HashMap Revert "Add property name interning" This reverts commit e82c3dbc8dc0269c399d69dbc2a7a9d0d7ff554b. --- rbx_binary/src/core.rs | 9 +-- rbx_binary/src/deserializer/state.rs | 92 ++++++++++++++-------------- rbx_binary/src/serializer/state.rs | 74 ++++++++++------------ rbx_dom_weak/Cargo.toml | 1 + rbx_dom_weak/src/dom.rs | 18 +++--- rbx_dom_weak/src/instance.rs | 35 +++++------ rbx_dom_weak/src/lib.rs | 2 + rbx_dom_weak/src/viewer.rs | 10 +-- rbx_reflector/src/defaults.rs | 2 +- rbx_util/src/remove_prop.rs | 2 +- rbx_xml/src/deserializer.rs | 33 +++++----- rbx_xml/src/lib.rs | 2 +- rbx_xml/src/serializer.rs | 10 +-- rbx_xml/src/tests/basic.rs | 25 +++++--- 14 files changed, 160 insertions(+), 155 deletions(-) diff --git a/rbx_binary/src/core.rs b/rbx_binary/src/core.rs index 8ea8b836b..94f53146a 100644 --- a/rbx_binary/src/core.rs +++ b/rbx_binary/src/core.rs @@ -3,6 +3,7 @@ use std::{ mem, }; +use rbx_dom_weak::Ustr; use rbx_reflection::{ ClassDescriptor, PropertyDescriptor, PropertyKind, PropertySerialization, ReflectionDatabase, }; @@ -345,10 +346,10 @@ pub struct PropertyDescriptors<'db> { /// class and property name pair. These might be the same descriptor! pub fn find_property_descriptors<'db>( database: &'db ReflectionDatabase<'db>, - class_name: &str, - property_name: &str, + class_name: Ustr, + property_name: Ustr, ) -> Option> { - let mut class_descriptor = database.classes.get(class_name)?; + let mut class_descriptor = database.classes.get(class_name.as_str())?; // We need to find the canonical property descriptor associated with // the property we're working with. @@ -360,7 +361,7 @@ pub fn find_property_descriptors<'db>( loop { // If this class descriptor knows about this property name, we're pretty // much done! - if let Some(property_descriptor) = class_descriptor.properties.get(property_name) { + if let Some(property_descriptor) = class_descriptor.properties.get(property_name.as_str()) { match &property_descriptor.kind { // This property descriptor is the canonical form of this // logical property. That means we've found one of the two diff --git a/rbx_binary/src/deserializer/state.rs b/rbx_binary/src/deserializer/state.rs index 19089bd34..8bb06a74b 100644 --- a/rbx_binary/src/deserializer/state.rs +++ b/rbx_binary/src/deserializer/state.rs @@ -13,7 +13,7 @@ use rbx_dom_weak::{ PhysicalProperties, Ray, Rect, Ref, SecurityCapabilities, SharedString, Tags, UDim, UDim2, UniqueId, Variant, VariantType, Vector2, Vector3, Vector3int16, }, - InstanceBuilder, WeakDom, + InstanceBuilder, Ustr, WeakDom, }; use rbx_reflection::{DataType, PropertyKind, PropertySerialization, ReflectionDatabase}; @@ -68,7 +68,7 @@ struct TypeInfo { type_id: u32, /// The common name for this type like `Folder` or `UserInputService`. - type_name: String, + type_name: Ustr, /// A list of the instances described by this file that are this type. referents: Vec, @@ -92,7 +92,7 @@ struct Instance { /// others (like Font, which has been superceded by FontFace). #[derive(Debug)] struct CanonicalProperty<'db> { - name: &'db str, + name: Ustr, ty: VariantType, migration: Option<&'db PropertySerialization<'db>>, } @@ -100,8 +100,8 @@ struct CanonicalProperty<'db> { fn find_canonical_property<'de>( database: &'de ReflectionDatabase, binary_type: Type, - class_name: &str, - prop_name: &'de str, + class_name: Ustr, + prop_name: Ustr, ) -> Option> { match find_property_descriptors(database, class_name, prop_name) { Some(descriptors) => { @@ -155,7 +155,7 @@ fn find_canonical_property<'de>( ); Some(CanonicalProperty { - name: canonical_name, + name: canonical_name.clone().into(), ty: canonical_type, migration, }) @@ -311,7 +311,7 @@ impl<'db, R: Read> DeserializerState<'db, R> { type_id, TypeInfo { type_id, - type_name, + type_name: type_name.into(), referents, }, ); @@ -398,8 +398,8 @@ This may cause unexpected or broken behavior in your final results if you rely o let property = if let Some(property) = find_canonical_property( self.deserializer.database, binary_type, - &type_info.type_name, - &prop_name, + type_info.type_name, + prop_name.as_str().into(), ) { property } else { @@ -452,7 +452,7 @@ This may cause unexpected or broken behavior in your final results if you rely o let value = Tags::decode(buffer.as_ref()).map_err(|_| { InnerError::InvalidPropData { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name: prop_name.clone(), valid_value: "a list of valid null-delimited UTF-8 strings", actual_value: "invalid UTF-8".to_string(), @@ -499,7 +499,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "String, Content, Tags, Attributes, or BinaryString", actual_type_name: format!("{:?}", invalid_type), @@ -516,7 +516,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "Bool", actual_type_name: format!("{:?}", invalid_type), @@ -548,7 +548,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "Int32", actual_type_name: format!("{:?}", invalid_type), @@ -567,7 +567,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "Float32", actual_type_name: format!("{:?}", invalid_type), @@ -597,7 +597,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "Float64", actual_type_name: format!("{:?}", invalid_type), @@ -624,7 +624,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "UDim", actual_type_name: format!("{:?}", invalid_type), @@ -663,7 +663,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "UDim2", actual_type_name: format!("{:?}", invalid_type), @@ -695,7 +695,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "Ray", actual_type_name: format!("{:?}", invalid_type), @@ -709,7 +709,7 @@ This may cause unexpected or broken behavior in your final results if you rely o let value = chunk.read_u8()?; let faces = Faces::from_bits(value).ok_or_else(|| InnerError::InvalidPropData { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name: prop_name.clone(), valid_value: "less than 63", actual_value: value.to_string(), @@ -720,7 +720,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "Faces", actual_type_name: format!("{:?}", invalid_type), @@ -735,7 +735,7 @@ This may cause unexpected or broken behavior in your final results if you rely o let axes = Axes::from_bits(value).ok_or_else(|| InnerError::InvalidPropData { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name: prop_name.clone(), valid_value: "less than 7", actual_value: value.to_string(), @@ -746,7 +746,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "Axes", actual_type_name: format!("{:?}", invalid_type), @@ -765,7 +765,7 @@ This may cause unexpected or broken behavior in your final results if you rely o .ok() .and_then(BrickColor::from_number) .ok_or_else(|| InnerError::InvalidPropData { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name: prop_name.clone(), valid_value: "a valid BrickColor", actual_value: value.to_string(), @@ -776,7 +776,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "BrickColor", actual_type_name: format!("{:?}", invalid_type), @@ -806,7 +806,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "Color3", actual_type_name: format!("{:?}", invalid_type), @@ -830,7 +830,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "Vector2", actual_type_name: format!("{:?}", invalid_type), @@ -860,7 +860,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "Vector3", actual_type_name: format!("{:?}", invalid_type), @@ -896,7 +896,7 @@ This may cause unexpected or broken behavior in your final results if you rely o rotations.push(basic_rotation); } else { return Err(InnerError::BadRotationId { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, id, }); @@ -926,7 +926,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "CFrame", actual_type_name: format!("{:?}", invalid_type), @@ -945,7 +945,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "Enum", actual_type_name: format!("{:?}", invalid_type), @@ -970,7 +970,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "Ref", actual_type_name: format!("{:?}", invalid_type), @@ -995,7 +995,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "Vector3int16", actual_type_name: format!("{:?}", invalid_type), @@ -1033,7 +1033,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "Font", actual_type_name: format!("{:?}", invalid_type), @@ -1060,7 +1060,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "NumberSequence", actual_type_name: format!("{:?}", invalid_type), @@ -1093,7 +1093,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "ColorSequence", actual_type_name: format!("{:?}", invalid_type), @@ -1113,7 +1113,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "NumberRange", actual_type_name: format!("{:?}", invalid_type), @@ -1146,7 +1146,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "Rect", actual_type_name: format!("{:?}", invalid_type), @@ -1176,7 +1176,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "PhysicalProperties", actual_type_name: format!("{:?}", invalid_type), @@ -1207,7 +1207,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "Color3", actual_type_name: format!("{:?}", invalid_type), @@ -1226,7 +1226,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "Int64", actual_type_name: format!("{:?}", invalid_type), @@ -1242,7 +1242,7 @@ This may cause unexpected or broken behavior in your final results if you rely o let shared_string = self.shared_strings.get(value as usize).ok_or_else(|| { InnerError::InvalidPropData { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name: prop_name.clone(), valid_value: "a valid SharedString", actual_value: format!("{:?}", value), @@ -1256,7 +1256,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "SharedString", actual_type_name: format!("{:?}", invalid_type), @@ -1304,7 +1304,7 @@ This may cause unexpected or broken behavior in your final results if you rely o rotations.push(basic_rotation); } else { return Err(InnerError::BadRotationId { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, id, }); @@ -1352,7 +1352,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "OptionalCFrame", actual_type_name: format!("{:?}", invalid_type), @@ -1382,7 +1382,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "UniqueId", actual_type_name: format!("{:?}", invalid_type), @@ -1407,7 +1407,7 @@ This may cause unexpected or broken behavior in your final results if you rely o } invalid_type => { return Err(InnerError::PropTypeMismatch { - type_name: type_info.type_name.clone(), + type_name: type_info.type_name.to_string(), prop_name, valid_type_names: "SecurityCapabilities", actual_type_name: format!("{:?}", invalid_type), diff --git a/rbx_binary/src/serializer/state.rs b/rbx_binary/src/serializer/state.rs index 504403914..6d27de09b 100644 --- a/rbx_binary/src/serializer/state.rs +++ b/rbx_binary/src/serializer/state.rs @@ -13,7 +13,7 @@ use rbx_dom_weak::{ SecurityCapabilities, SharedString, Tags, UDim, UDim2, UniqueId, Variant, VariantType, Vector2, Vector3, Vector3int16, }, - Instance, WeakDom, + Instance, Ustr, WeakDom, }; use rbx_reflection::{ @@ -89,7 +89,7 @@ struct TypeInfo<'dom, 'db> { /// /// Stored in a sorted map to try to ensure that we write out properties in /// a deterministic order. - properties: BTreeMap, PropInfo<'db>>, + properties: BTreeMap>, /// A reference to the type's class descriptor from rbx_reflection, if this /// is a known class. @@ -98,7 +98,7 @@ struct TypeInfo<'dom, 'db> { /// A set containing the properties that we have seen so far in the file and /// processed. This helps us avoid traversing the reflection database /// multiple times if there are many copies of the same kind of instance. - properties_visited: HashSet<(Cow<'db, str>, VariantType)>, + properties_visited: HashSet<(Ustr, VariantType)>, } /// A property on a specific class that our serializer knows about. @@ -121,14 +121,14 @@ struct PropInfo<'db> { /// The serialized name for this property. This is the name that is actually /// written as part of the PROP chunk and may not line up with the canonical /// name for the property. - serialized_name: Cow<'db, str>, + serialized_name: Ustr, /// A set containing the names of all aliases discovered while preparing to /// serialize this property. Ideally, this set will remain empty (and not /// allocate) in most cases. However, if an instance is missing a property /// from its canonical name, but does have another variant, we can use this /// set to recover and map those values. - aliases: BTreeSet, + aliases: BTreeSet, /// The default value for this property that should be used if any instances /// are missing this property. @@ -159,7 +159,7 @@ struct TypeInfos<'dom, 'db> { /// /// These are stored sorted so that we naturally iterate over them in order /// and improve our chances of being deterministic. - values: BTreeMap>, + values: BTreeMap>, /// The next type ID that should be assigned if a type is discovered and /// added to the serializer. @@ -177,12 +177,12 @@ impl<'dom, 'db> TypeInfos<'dom, 'db> { /// Finds the type info from the given ClassName if it exists, or creates /// one and returns a reference to it if not. - fn get_or_create(&mut self, class: &str) -> &mut TypeInfo<'dom, 'db> { + fn get_or_create(&mut self, class: &Ustr) -> &mut TypeInfo<'dom, 'db> { if !self.values.contains_key(class) { let type_id = self.next_type_id; self.next_type_id += 1; - let class_descriptor = self.database.classes.get(class); + let class_descriptor = self.database.classes.get(class.as_str()); let is_service = if let Some(descriptor) = &class_descriptor { descriptor.tags.contains(&ClassTag::Service) @@ -201,10 +201,10 @@ impl<'dom, 'db> TypeInfos<'dom, 'db> { // We can use a dummy default_value here because instances from // rbx_dom_weak always have a name set. properties.insert( - Cow::Borrowed("Name"), + "Name".into(), PropInfo { prop_type: Type::String, - serialized_name: Cow::Borrowed("Name"), + serialized_name: "Name".into(), aliases: BTreeSet::new(), default_value: Cow::Owned(Variant::String(String::new())), migration: None, @@ -212,7 +212,7 @@ impl<'dom, 'db> TypeInfos<'dom, 'db> { ); self.values.insert( - class.to_owned(), + *class, TypeInfo { type_id, is_service, @@ -335,7 +335,7 @@ impl<'dom, 'db, W: Write> SerializerState<'dom, 'db, W> { // Skip this property+value type pair if we've already seen it. if type_info .properties_visited - .contains(&(Cow::Borrowed(prop_name), prop_value.ty())) + .contains(&(*prop_name, prop_value.ty())) { continue; } @@ -344,7 +344,7 @@ impl<'dom, 'db, W: Write> SerializerState<'dom, 'db, W> { // it. type_info .properties_visited - .insert((Cow::Owned(prop_name.clone()), prop_value.ty())); + .insert((*prop_name, prop_value.ty())); let canonical_name; let serialized_name; @@ -352,7 +352,7 @@ impl<'dom, 'db, W: Write> SerializerState<'dom, 'db, W> { let mut migration = None; let database = self.serializer.database; - match find_property_descriptors(database, &instance.class, prop_name) { + match find_property_descriptors(database, instance.class, *prop_name) { Some(descriptors) => { // For any properties that do not serialize, we can skip // adding them to the set of type_infos. @@ -369,8 +369,8 @@ impl<'dom, 'db, W: Write> SerializerState<'dom, 'db, W> { // serialize let new_descriptors = find_property_descriptors( database, - &instance.class, - &prop_migration.new_property_name, + instance.class, + prop_migration.new_property_name.as_str().into(), ); migration = Some(prop_migration); @@ -378,7 +378,8 @@ impl<'dom, 'db, W: Write> SerializerState<'dom, 'db, W> { match new_descriptors { Some(descriptor) => match descriptor.serialized { Some(serialized) => { - canonical_name = descriptor.canonical.name.clone(); + canonical_name = + descriptor.canonical.name.as_ref().into(); serialized } None => continue, @@ -386,14 +387,14 @@ impl<'dom, 'db, W: Write> SerializerState<'dom, 'db, W> { None => continue, } } else { - canonical_name = descriptors.canonical.name.clone(); + canonical_name = descriptors.canonical.name.as_ref().into(); descriptor } } None => continue, }; - serialized_name = serialized.name.clone(); + serialized_name = serialized.name.as_ref().into(); serialized_ty = match &serialized.data_type { DataType::Value(ty) => *ty, @@ -403,8 +404,8 @@ impl<'dom, 'db, W: Write> SerializerState<'dom, 'db, W> { // rbx_binary is not new enough to handle this kind // of property, whatever it is. return Err(InnerError::UnsupportedPropType { - type_name: instance.class.clone(), - prop_name: prop_name.clone(), + type_name: instance.class.to_string(), + prop_name: prop_name.to_string(), prop_type: format!("{:?}", unknown_ty), }); } @@ -412,21 +413,12 @@ impl<'dom, 'db, W: Write> SerializerState<'dom, 'db, W> { } None => { - canonical_name = Cow::Owned(prop_name.clone()); - serialized_name = Cow::Owned(prop_name.clone()); + canonical_name = *prop_name; + serialized_name = *prop_name; serialized_ty = prop_value.ty(); } } - // In order to prevent cloning canonical_name in a rare branch, - // we conditionally clone here if we'll need canonical_name after - // it's inserted into type_info.properties. - let canonical_name_if_different = if prop_name != &canonical_name { - Some(canonical_name.clone()) - } else { - None - }; - if !type_info.properties.contains_key(&canonical_name) { let default_value = type_info .class_descriptor @@ -440,7 +432,7 @@ impl<'dom, 'db, W: Write> SerializerState<'dom, 'db, W> { // Since we don't know how to generate the default value // for this property, we consider it unsupported. InnerError::UnsupportedPropType { - type_name: instance.class.clone(), + type_name: instance.class.to_string(), prop_name: canonical_name.to_string(), prop_type: format!("{:?}", serialized_ty), } @@ -461,7 +453,7 @@ impl<'dom, 'db, W: Write> SerializerState<'dom, 'db, W> { // binary type value for it. rbx_binary might be out of // date? InnerError::UnsupportedPropType { - type_name: instance.class.clone(), + type_name: instance.class.to_string(), prop_name: serialized_name.to_string(), prop_type: format!("{:?}", serialized_ty), } @@ -482,11 +474,11 @@ impl<'dom, 'db, W: Write> SerializerState<'dom, 'db, W> { // If the property we found on this instance is different than the // canonical name for this property, stash it into the set of known // aliases for this PropInfo. - if let Some(canonical_name) = canonical_name_if_different { + if *prop_name != canonical_name { let prop_info = type_info.properties.get_mut(&canonical_name).unwrap(); if !prop_info.aliases.contains(prop_name) { - prop_info.aliases.insert(prop_name.clone()); + prop_info.aliases.insert(*prop_name); } prop_info.migration = migration; @@ -643,20 +635,20 @@ impl<'dom, 'db, W: Write> SerializerState<'dom, 'db, W> { // We store the Name property in a different field for // convenience, but when serializing to the binary model // format we need to handle it just like other properties. - if prop_name == "Name" { + if *prop_name == "Name" { return Cow::Owned(Variant::String(instance.name.clone())); } // Most properties will be stored on instances using the // property's canonical name, so we'll try that first. - if let Some(property) = instance.properties.get(prop_name.as_ref()) { + if let Some(property) = instance.properties.get(&((*prop_name).into())) { return Cow::Borrowed(property); } // If there were any known aliases for this property // used as part of this file, we can check those next. for alias in &prop_info.aliases { - if let Some(property) = instance.properties.get(alias) { + if let Some(property) = instance.properties.get(&((*alias).into())) { return Cow::Borrowed(property); } } @@ -684,7 +676,7 @@ impl<'dom, 'db, W: Write> SerializerState<'dom, 'db, W> { let type_mismatch = |i: usize, bad_value: &Variant, valid_type_names: &'static str| { Err(InnerError::PropTypeMismatch { - type_name: type_name.clone(), + type_name: type_name.to_string(), prop_name: prop_name.to_string(), valid_type_names, actual_type_name: format!("{:?}", bad_value.ty()), @@ -695,7 +687,7 @@ impl<'dom, 'db, W: Write> SerializerState<'dom, 'db, W> { let invalid_value = |i: usize, bad_value: &Variant| InnerError::InvalidPropValue { instance_full_name: self.full_name_for(type_info.instances[i].referent()), - type_name: type_name.clone(), + type_name: type_name.to_string(), prop_name: prop_name.to_string(), prop_type: format!("{:?}", bad_value.ty()), }; diff --git a/rbx_dom_weak/Cargo.toml b/rbx_dom_weak/Cargo.toml index e59388ed6..64d4cdb15 100644 --- a/rbx_dom_weak/Cargo.toml +++ b/rbx_dom_weak/Cargo.toml @@ -12,6 +12,7 @@ edition = "2018" [dependencies] rbx_types = { version = "1.10.0", path = "../rbx_types", features = ["serde"] } +ustr = { version = "1.1.0", features = ["serde"] } serde = "1.0.137" diff --git a/rbx_dom_weak/src/dom.rs b/rbx_dom_weak/src/dom.rs index e558790bc..14286f516 100644 --- a/rbx_dom_weak/src/dom.rs +++ b/rbx_dom_weak/src/dom.rs @@ -1,6 +1,7 @@ use std::collections::{HashMap, HashSet, VecDeque}; use rbx_types::{Ref, UniqueId, Variant}; +use ustr::ustr; use crate::instance::{Instance, InstanceBuilder}; @@ -69,7 +70,7 @@ impl WeakDom { /// exists. pub fn get_unique_id(&self, referent: Ref) -> Option { let inst = self.instances.get(&referent)?; - match inst.properties.get("UniqueId") { + match inst.properties.get(&ustr("UniqueId")) { Some(Variant::UniqueId(id)) => Some(*id), _ => None, } @@ -350,7 +351,7 @@ impl WeakDom { // Unwrap is safe because we just inserted this referent into the instance map let instance = self.instances.get_mut(&referent).unwrap(); - if let Some(Variant::UniqueId(unique_id)) = instance.properties.get("UniqueId") { + if let Some(Variant::UniqueId(unique_id)) = instance.properties.get(&ustr("UniqueId")) { if self.unique_ids.contains(unique_id) { // We found a collision! We need to replace the UniqueId property with // a new value. @@ -362,7 +363,7 @@ impl WeakDom { self.unique_ids.insert(new_unique_id); instance .properties - .insert("UniqueId".to_string(), Variant::UniqueId(new_unique_id)); + .insert(ustr("UniqueId"), Variant::UniqueId(new_unique_id)); } else { self.unique_ids.insert(*unique_id); }; @@ -375,7 +376,7 @@ impl WeakDom { .remove(&referent) .unwrap_or_else(|| panic!("cannot remove an instance that does not exist")); - if let Some(Variant::UniqueId(unique_id)) = instance.properties.get("UniqueId") { + if let Some(Variant::UniqueId(unique_id)) = instance.properties.get(&ustr("UniqueId")) { self.unique_ids.remove(unique_id); } @@ -698,7 +699,7 @@ mod test { ); let child = dom.get_by_ref(child_ref).unwrap(); - if let Some(Variant::UniqueId(actual_unique_id)) = child.properties.get("UniqueId") { + if let Some(Variant::UniqueId(actual_unique_id)) = child.properties.get(&ustr("UniqueId")) { assert_ne!( unique_id, *actual_unique_id, @@ -725,7 +726,7 @@ mod test { ); let child = dom.get_by_ref(child_ref).unwrap(); - if let Some(Variant::UniqueId(actual_unique_id)) = child.properties.get("UniqueId") { + if let Some(Variant::UniqueId(actual_unique_id)) = child.properties.get(&ustr("UniqueId")) { assert_ne!( unique_id, *actual_unique_id, @@ -747,7 +748,7 @@ mod test { ); let child = dom.get_by_ref(child_ref).unwrap(); - if let Some(Variant::UniqueId(actual_unique_id)) = child.properties.get("UniqueId") { + if let Some(Variant::UniqueId(actual_unique_id)) = child.properties.get(&ustr("UniqueId")) { assert_eq!( unique_id, *actual_unique_id, @@ -778,7 +779,8 @@ mod test { dom.transfer(folder_ref, &mut other_dom, other_root_ref); let folder = other_dom.get_by_ref(folder_ref).unwrap(); - if let Some(Variant::UniqueId(actual_unique_id)) = folder.properties.get("UniqueId") { + if let Some(Variant::UniqueId(actual_unique_id)) = folder.properties.get(&ustr("UniqueId")) + { assert_ne!( unique_id, *actual_unique_id, "WeakDom::transfer caused a UniqueId collision." diff --git a/rbx_dom_weak/src/instance.rs b/rbx_dom_weak/src/instance.rs index 7871c3c85..3a1cfad4f 100644 --- a/rbx_dom_weak/src/instance.rs +++ b/rbx_dom_weak/src/instance.rs @@ -1,6 +1,5 @@ -use std::collections::HashMap; - use rbx_types::{Ref, Variant}; +use ustr::{Ustr, UstrMap}; /** Represents an instance that can be turned into a new @@ -35,23 +34,23 @@ let dom = WeakDom::new(data_model); pub struct InstanceBuilder { pub(crate) referent: Ref, pub(crate) name: String, - pub(crate) class: String, - pub(crate) properties: HashMap, + pub(crate) class: Ustr, + pub(crate) properties: UstrMap, pub(crate) children: Vec, } impl InstanceBuilder { /// Create a new `InstanceBuilder` with the given ClassName. This is also /// used as the instance's Name, unless overwritten later. - pub fn new>(class: S) -> Self { + pub fn new>(class: S) -> Self { let class = class.into(); - let name = class.clone(); + let name = class.to_string(); InstanceBuilder { referent: Ref::new(), name, class, - properties: HashMap::new(), + properties: UstrMap::default(), children: Vec::new(), } } @@ -61,8 +60,8 @@ impl InstanceBuilder { InstanceBuilder { referent: Ref::new(), name: String::new(), - class: String::new(), - properties: HashMap::new(), + class: Ustr::default(), + properties: UstrMap::default(), children: Vec::new(), } } @@ -94,7 +93,7 @@ impl InstanceBuilder { } /// Change the class of the `InstanceBuilder`. - pub fn with_class>(self, class: S) -> Self { + pub fn with_class>(self, class: S) -> Self { Self { class: class.into(), ..self @@ -102,30 +101,30 @@ impl InstanceBuilder { } /// Change the class of the `InstanceBuilder`. - pub fn set_class>(&mut self, class: S) { + pub fn set_class>(&mut self, class: S) { self.class = class.into(); } /// Add a new property to the `InstanceBuilder`. - pub fn with_property, V: Into>(mut self, key: K, value: V) -> Self { + pub fn with_property, V: Into>(mut self, key: K, value: V) -> Self { self.properties.insert(key.into(), value.into()); self } /// Add a new property to the `InstanceBuilder`. - pub fn add_property, V: Into>(&mut self, key: K, value: V) { + pub fn add_property, V: Into>(&mut self, key: K, value: V) { self.properties.insert(key.into(), value.into()); } /// Check if the `InstanceBuilder` already has a property with the given key. - pub fn has_property>(&self, key: K) -> bool { + pub fn has_property>(&self, key: K) -> bool { self.properties.contains_key(&key.into()) } /// Add multiple properties to the `InstanceBuilder` at once. pub fn with_properties(mut self, props: I) -> Self where - K: Into, + K: Into, V: Into, I: IntoIterator, { @@ -138,7 +137,7 @@ impl InstanceBuilder { /// Add multiple properties to the `InstanceBuilder` at once. pub fn add_properties(&mut self, props: I) where - K: Into, + K: Into, V: Into, I: IntoIterator, { @@ -193,10 +192,10 @@ pub struct Instance { pub name: String, /// The instance's class, corresponding to the `ClassName` property. - pub class: String, + pub class: Ustr, /// Any properties stored on the object that are not `Name` or `ClassName`. - pub properties: HashMap, + pub properties: UstrMap, } impl Instance { diff --git a/rbx_dom_weak/src/lib.rs b/rbx_dom_weak/src/lib.rs index af9efe159..55d8c87a4 100644 --- a/rbx_dom_weak/src/lib.rs +++ b/rbx_dom_weak/src/lib.rs @@ -47,6 +47,8 @@ mod viewer; pub use rbx_types as types; +pub use ustr::{Ustr, UstrMap, UstrSet}; + pub use crate::{ dom::WeakDom, instance::{Instance, InstanceBuilder}, diff --git a/rbx_dom_weak/src/viewer.rs b/rbx_dom_weak/src/viewer.rs index 282dbd27b..e802c1840 100644 --- a/rbx_dom_weak/src/viewer.rs +++ b/rbx_dom_weak/src/viewer.rs @@ -8,6 +8,7 @@ use crate::{ WeakDom, }; use serde::{Deserialize, Serialize}; +use ustr::Ustr; /// Contains state for viewing and redacting nondeterministic portions of /// WeakDom objects, making them suitable for usage in snapshot tests. @@ -81,7 +82,6 @@ impl DomViewer { .properties .iter() .map(|(key, value)| { - let key = key.clone(); let new_value = match value { Variant::Ref(referent) => { if referent.is_some() { @@ -111,14 +111,14 @@ impl DomViewer { other => ViewedValue::Other(other.clone()), }; - (key, new_value) + (*key, new_value) }) .collect(); ViewedInstance { referent: self.referent_to_id.get(&referent).unwrap().clone(), name: instance.name.clone(), - class: instance.class.clone(), + class: instance.class, properties, children, } @@ -137,8 +137,8 @@ impl Default for DomViewer { pub struct ViewedInstance { referent: String, name: String, - class: String, - properties: BTreeMap, + class: Ustr, + properties: BTreeMap, children: Vec, } diff --git a/rbx_reflector/src/defaults.rs b/rbx_reflector/src/defaults.rs index fadf0462b..4a514f3ee 100644 --- a/rbx_reflector/src/defaults.rs +++ b/rbx_reflector/src/defaults.rs @@ -59,7 +59,7 @@ fn apply_instance_defaults(database: &mut ReflectionDatabase, instance: &Instanc }; for (property_name, property_value) in &instance.properties { - let property_name = Cow::Owned(property_name.clone()); + let property_name = Cow::Owned(property_name.to_string()); match property_value.ty() { // We skip the Ref type because its default value is not useful. diff --git a/rbx_util/src/remove_prop.rs b/rbx_util/src/remove_prop.rs index ad46cae9c..b5083f08a 100644 --- a/rbx_util/src/remove_prop.rs +++ b/rbx_util/src/remove_prop.rs @@ -48,7 +48,7 @@ impl RemovePropCommand { let inst = dom.get_by_ref_mut(referent).unwrap(); if inst.class == self.class_name { log::trace!("Removed property {}.{}", inst.name, self.prop_name); - inst.properties.remove(&self.prop_name); + inst.properties.remove(&self.prop_name.as_str().into()); } queue.extend_from_slice(inst.children()); } diff --git a/rbx_xml/src/deserializer.rs b/rbx_xml/src/deserializer.rs index 744003a86..79a64c3b2 100644 --- a/rbx_xml/src/deserializer.rs +++ b/rbx_xml/src/deserializer.rs @@ -6,7 +6,7 @@ use std::{ use log::trace; use rbx_dom_weak::{ types::{Ref, SharedString, Variant, VariantType}, - InstanceBuilder, WeakDom, + InstanceBuilder, Ustr, WeakDom, }; use rbx_reflection::{DataType, PropertyKind, PropertySerialization, ReflectionDatabase}; @@ -237,9 +237,10 @@ fn apply_referent_rewrites(state: &mut ParseState) { .get_by_ref_mut(rewrite.id) .expect("rbx_xml bug: had ID in referent rewrite list that didn't end up in the tree"); - instance - .properties - .insert(rewrite.property_name.clone(), Variant::Ref(new_value)); + instance.properties.insert( + rewrite.property_name.as_str().into(), + Variant::Ref(new_value), + ); } } @@ -255,7 +256,7 @@ fn apply_shared_string_rewrites(state: &mut ParseState) { ); instance.properties.insert( - rewrite.property_name.clone(), + rewrite.property_name.as_str().into(), Variant::SharedString(new_value), ); } @@ -453,7 +454,7 @@ fn deserialize_instance( state.referents_to_ids.insert(referent, instance_id); } - let mut properties: HashMap = HashMap::new(); + let mut properties: HashMap = HashMap::new(); loop { match reader.expect_peek()? { @@ -488,7 +489,7 @@ fn deserialize_instance( let instance = state.tree.get_by_ref_mut(instance_id).unwrap(); - instance.name = match properties.remove("Name") { + instance.name = match properties.remove(&"Name".into()) { Some(value) => match value { Variant::String(value) => value, _ => return Err(reader.error(DecodeErrorKind::NameMustBeString(value.ty()))), @@ -497,10 +498,10 @@ fn deserialize_instance( // TODO: Use reflection to get default name instead. This should only // matter for ValueBase instances in files created by tools other than // Roblox Studio. - None => instance.class.clone(), + None => instance.class.to_string(), }; - instance.properties = properties; + instance.properties = properties.into_iter().collect(); Ok(()) } @@ -509,7 +510,7 @@ fn deserialize_properties( reader: &mut XmlEventReader, state: &mut ParseState, instance_id: Ref, - props: &mut HashMap, + props: &mut HashMap, ) -> Result<(), DecodeError> { reader.expect_start_with_name("Properties")?; @@ -615,7 +616,7 @@ fn deserialize_properties( Err(message) => { return Err( reader.error(DecodeErrorKind::UnsupportedPropertyConversion { - class_name: class_name.clone(), + class_name: class_name.to_string(), property_name: descriptor.name.to_string(), expected_type, actual_type: xml_ty, @@ -632,13 +633,13 @@ fn deserialize_properties( let new_property_name = &migration.new_property_name; let old_property_name = &descriptor.name; - if !props.contains_key(new_property_name) { + if !props.contains_key(&new_property_name.into()) { log::trace!( "Attempting to migrate property {old_property_name} to {new_property_name}" ); match migration.perform(&value) { Ok(migrated_value) => { - props.insert(new_property_name.to_string(), migrated_value); + props.insert(new_property_name.into(), migrated_value); log::trace!( "Successfully migrated property {old_property_name} to {new_property_name}" ); @@ -650,7 +651,7 @@ fn deserialize_properties( } } _ => { - props.insert(descriptor.name.to_string(), value); + props.insert(descriptor.name.as_ref().into(), value); } }; } else { @@ -681,11 +682,11 @@ fn deserialize_properties( Some(value) => value, None => continue, }; - props.insert(xml_property_name, value); + props.insert(xml_property_name.into(), value); } DecodePropertyBehavior::ErrorOnUnknown => { return Err(reader.error(DecodeErrorKind::UnknownProperty { - class_name, + class_name: class_name.to_string(), property_name: xml_property_name, })); } diff --git a/rbx_xml/src/lib.rs b/rbx_xml/src/lib.rs index e0bf70481..907602728 100644 --- a/rbx_xml/src/lib.rs +++ b/rbx_xml/src/lib.rs @@ -34,7 +34,7 @@ //! let number_value = model.get_by_ref(number_value_ref).unwrap(); //! //! assert_eq!( -//! number_value.properties.get("Value"), +//! number_value.properties.get(&"Value".into()), //! Some(&Variant::Float64(12345.0)), //! ); //! # Ok::<(), Box>(()) diff --git a/rbx_xml/src/serializer.rs b/rbx_xml/src/serializer.rs index 9c7a77e3b..d4fcf8fb2 100644 --- a/rbx_xml/src/serializer.rs +++ b/rbx_xml/src/serializer.rs @@ -168,7 +168,7 @@ fn serialize_instance<'dom, W: Write>( state: &mut EmitState, tree: &'dom WeakDom, id: Ref, - property_buffer: &mut Vec<(&'dom String, &'dom Variant)>, + property_buffer: &mut Vec<(&'dom str, &'dom Variant)>, ) -> Result<(), NewEncodeError> { let instance = tree.get_by_ref(id).unwrap(); let mapped_id = state.map_id(id); @@ -190,7 +190,7 @@ fn serialize_instance<'dom, W: Write>( // Move references to our properties into property_buffer so we can sort // them and iterate them in order. - property_buffer.extend(&instance.properties); + property_buffer.extend(instance.properties.iter().map(|(k, v)| (k.as_str(), v))); property_buffer.sort_unstable_by_key(|(key, _)| *key); for (property_name, value) in property_buffer.drain(..) { @@ -218,7 +218,7 @@ fn serialize_instance<'dom, W: Write>( Err(message) => { return Err( writer.error(EncodeErrorKind::UnsupportedPropertyConversion { - class_name: instance.class.clone(), + class_name: instance.class.to_string(), property_name: property_name.to_string(), expected_type: data_type, actual_type: value.ty(), @@ -253,8 +253,8 @@ fn serialize_instance<'dom, W: Write>( } EncodePropertyBehavior::ErrorOnUnknown => { return Err(writer.error(EncodeErrorKind::UnknownProperty { - class_name: instance.class.clone(), - property_name: property_name.clone(), + class_name: instance.class.to_string(), + property_name: property_name.to_string(), })); } } diff --git a/rbx_xml/src/tests/basic.rs b/rbx_xml/src/tests/basic.rs index 2fd9391ad..e4540eb38 100644 --- a/rbx_xml/src/tests/basic.rs +++ b/rbx_xml/src/tests/basic.rs @@ -29,7 +29,10 @@ fn with_bool() { assert_eq!(child.name, "BoolValue"); assert_eq!(child.class, "BoolValue"); - assert_eq!(child.properties.get("Value"), Some(&Variant::Bool(true))); + assert_eq!( + child.properties.get(&"Value".into()), + Some(&Variant::Bool(true)) + ); } #[test] @@ -53,7 +56,10 @@ fn read_tags() { tags.push("Hello"); tags.push("World"); - assert_eq!(folder.properties.get("Tags"), Some(&Variant::Tags(tags))); + assert_eq!( + folder.properties.get(&"Tags".into()), + Some(&Variant::Tags(tags)) + ); } #[test] @@ -109,8 +115,8 @@ fn read_attributes() { let dom = crate::from_str_default(document).unwrap(); let folder = dom.get_by_ref(dom.root().children()[0]).unwrap(); - assert_eq!(folder.properties.get("AttributesSerialize"), None); - let folder_attributes = match folder.properties.get("Attributes") { + assert_eq!(folder.properties.get(&"AttributesSerialize".into()), None); + let folder_attributes = match folder.properties.get(&"Attributes".into()) { Some(Variant::Attributes(attrs)) => attrs, Some(other) => panic!( "Attributes property was not Attributes, it was: {:?}", @@ -220,7 +226,8 @@ fn read_material_colors() { let dom = crate::from_str_default(document).unwrap(); let terrain = dom.get_by_ref(dom.root().children()[0]).unwrap(); - if let Some(Variant::MaterialColors(colors)) = terrain.properties.get("MaterialColors") { + if let Some(Variant::MaterialColors(colors)) = terrain.properties.get(&"MaterialColors".into()) + { // There are tests to ensure competency in the actual MaterialColors // implementation, so these are just basic "are you ok" checks. assert_eq!( @@ -238,7 +245,7 @@ fn read_material_colors() { } else { panic!( "MaterialColors was not Some(Variant::MaterialColors(_)) and was instead {:?}", - terrain.properties.get("MaterialColors") + terrain.properties.get(&"MaterialColors".into()) ) } } @@ -276,7 +283,7 @@ fn read_unique_id() { assert_eq!(child.class, "Workspace"); assert_eq!( - child.properties.get("UniqueId"), + child.properties.get(&"UniqueId".into()), Some(&Variant::UniqueId(UniqueId::new( 0x0048_15fc, 0x02e9_c68d, @@ -307,13 +314,13 @@ fn number_widening() { let int_value = tree.get_by_ref(tree.root().children()[0]).unwrap(); assert_eq!(int_value.class, "IntValue"); assert_eq!( - int_value.properties.get("Value"), + int_value.properties.get(&"Value".into()), Some(&Variant::Int64(194)) ); let float_value = tree.get_by_ref(tree.root().children()[1]).unwrap(); assert_eq!(float_value.class, "NumberValue"); assert_eq!( - float_value.properties.get("Value"), + float_value.properties.get(&"Value".into()), Some(&Variant::Float64(1337.0)) ); }