From bc8ab6b23e00822a784d23a8f2887c18ffdc7865 Mon Sep 17 00:00:00 2001 From: Kenneth Loeffler Date: Fri, 1 Nov 2024 15:47:20 +0000 Subject: [PATCH 1/2] Don't bail for MaterialColors and Attributes parse failures (#471) --- rbx_binary/src/deserializer/error.rs | 7 ---- rbx_binary/src/deserializer/state.rs | 36 ++++++++++++----- rbx_dom_lua/src/Error.lua | 1 + rbx_dom_lua/src/customProperties.lua | 11 ++++++ rbx_xml/src/conversion.rs | 59 ++++++++++++++++++++-------- rbx_xml/src/deserializer.rs | 2 +- rbx_xml/src/serializer.rs | 2 +- 7 files changed, 83 insertions(+), 35 deletions(-) diff --git a/rbx_binary/src/deserializer/error.rs b/rbx_binary/src/deserializer/error.rs index 0935aa420..d0b6191c4 100644 --- a/rbx_binary/src/deserializer/error.rs +++ b/rbx_binary/src/deserializer/error.rs @@ -79,11 +79,4 @@ pub(crate) enum InnerError { expected_type_id: u8, actual_type_id: u8, }, - - #[error("Failed to deserialize {class_name}.{prop_name} because {source}")] - BadPropertyValue { - source: rbx_dom_weak::types::Error, - prop_name: String, - class_name: String, - }, } diff --git a/rbx_binary/src/deserializer/state.rs b/rbx_binary/src/deserializer/state.rs index f47796c3d..43722abe5 100644 --- a/rbx_binary/src/deserializer/state.rs +++ b/rbx_binary/src/deserializer/state.rs @@ -481,11 +481,19 @@ This may cause unexpected or broken behavior in your final results if you rely o add_property(instance, &property, value.into()); } Err(err) => { - return Err(InnerError::BadPropertyValue { - source: err, - class_name: type_info.type_name.to_string(), - prop_name, - }) + log::warn!( + "Failed to parse Attributes on {} because {:?}; falling back to BinaryString. + +rbx-dom may require changes to fully support this property. Please open an issue at https://github.com/rojo-rbx/rbx-dom/issues and show this warning.", + type_info.type_name, + err + ); + + add_property( + instance, + &property, + BinaryString::from(buffer).into(), + ); } } } @@ -497,11 +505,19 @@ This may cause unexpected or broken behavior in your final results if you rely o match MaterialColors::decode(&buffer) { Ok(value) => add_property(instance, &property, value.into()), Err(err) => { - return Err(InnerError::BadPropertyValue { - source: err, - class_name: type_info.type_name.to_string(), - prop_name, - }) + log::warn!( + "Failed to parse MaterialColors on {} because {:?}; falling back to BinaryString. + +rbx-dom may require changes to fully support this property. Please open an issue at https://github.com/rojo-rbx/rbx-dom/issues and show this warning.", + type_info.type_name, + err + ); + + add_property( + instance, + &property, + BinaryString::from(buffer).into(), + ); } } } diff --git a/rbx_dom_lua/src/Error.lua b/rbx_dom_lua/src/Error.lua index 5b8f582c2..909c600e4 100644 --- a/rbx_dom_lua/src/Error.lua +++ b/rbx_dom_lua/src/Error.lua @@ -5,6 +5,7 @@ Error.Kind = { UnknownProperty = "UnknownProperty", PropertyNotReadable = "PropertyNotReadable", PropertyNotWritable = "PropertyNotWritable", + CannotParseBinaryString = "CannotParseBinaryString", Roblox = "Roblox", } diff --git a/rbx_dom_lua/src/customProperties.lua b/rbx_dom_lua/src/customProperties.lua index d8c12d5a1..dc41a2b88 100644 --- a/rbx_dom_lua/src/customProperties.lua +++ b/rbx_dom_lua/src/customProperties.lua @@ -1,6 +1,8 @@ local CollectionService = game:GetService("CollectionService") local ScriptEditorService = game:GetService("ScriptEditorService") +local Error = require(script.Parent.Error) + --- A list of `Enum.Material` values that are used for Terrain.MaterialColors local TERRAIN_MATERIAL_COLORS = { Enum.Material.Grass, @@ -51,6 +53,10 @@ return { return true, instance:GetAttributes() end, write = function(instance, _, value) + if typeof(value) ~= "table" then + return false, Error.new(Error.Kind.CannotParseBinaryString) + end + local existing = instance:GetAttributes() local didAllWritesSucceed = true @@ -160,9 +166,14 @@ return { return true, colors end, write = function(instance: Terrain, _, value: { [Enum.Material]: Color3 }) + if typeof(value) ~= "table" then + return false, Error.new(Error.Kind.CannotParseBinaryString) + end + for material, color in value do instance:SetMaterialColor(material, color) end + return true end, }, diff --git a/rbx_xml/src/conversion.rs b/rbx_xml/src/conversion.rs index a878d578a..d84fc049a 100644 --- a/rbx_xml/src/conversion.rs +++ b/rbx_xml/src/conversion.rs @@ -4,20 +4,27 @@ use std::borrow::{Borrow, Cow}; use std::convert::TryInto; -use rbx_dom_weak::types::{ - Attributes, BrickColor, Color3uint8, MaterialColors, Tags, Variant, VariantType, +use rbx_dom_weak::{ + types::{Attributes, BrickColor, Color3uint8, MaterialColors, Tags, Variant, VariantType}, + Ustr, }; pub trait ConvertVariant: Clone + Sized { - fn try_convert(self, target_type: VariantType) -> Result { - Self::try_convert_cow(Cow::Owned(self), target_type).map(|value| value.into_owned()) + fn try_convert(self, class_name: Ustr, target_type: VariantType) -> Result { + Self::try_convert_cow(class_name, Cow::Owned(self), target_type) + .map(|value| value.into_owned()) } - fn try_convert_ref(&self, target_type: VariantType) -> Result, String> { - Self::try_convert_cow(Cow::Borrowed(self), target_type) + fn try_convert_ref( + &self, + class_name: Ustr, + target_type: VariantType, + ) -> Result, String> { + Self::try_convert_cow(class_name, Cow::Borrowed(self), target_type) } fn try_convert_cow( + class_name: Ustr, value: Cow<'_, Self>, target_type: VariantType, ) -> Result, String>; @@ -25,6 +32,7 @@ pub trait ConvertVariant: Clone + Sized { impl ConvertVariant for Variant { fn try_convert_cow( + class_name: Ustr, value: Cow<'_, Self>, target_type: VariantType, ) -> Result, String> { @@ -57,18 +65,37 @@ impl ConvertVariant for Variant { )), (Variant::BinaryString(value), VariantType::Attributes) => { let bytes: &[u8] = value.as_ref(); + match Attributes::from_reader(bytes) { + Ok(attributes) => Ok(Cow::Owned(attributes.into())), + Err(err) => { + log::warn!( + "Failed to parse Attributes on {} because {:?}; falling back to BinaryString. - Ok(Cow::Owned( - Attributes::from_reader(bytes) - .map_err(|_| "Unknown or invalid Attributes")? - .into(), - )) +rbx-dom may require changes to fully support this property. Please open an issue at https://github.com/rojo-rbx/rbx-dom/issues and show this warning.", + class_name, + err + ); + + Ok(Cow::Owned(value.clone().into())) + } + } + } + (Variant::BinaryString(value), VariantType::MaterialColors) => { + match MaterialColors::decode(value.as_ref()) { + Ok(material_colors) => Ok(Cow::Owned(material_colors.into())), + Err(err) => { + log::warn!( + "Failed to parse MaterialColors on {} because {:?}; falling back to BinaryString. + +rbx-dom may require changes to fully support this property. Please open an issue at https://github.com/rojo-rbx/rbx-dom/issues and show this warning.", + class_name, + err + ); + + Ok(Cow::Owned(value.clone().into())) + } + } } - (Variant::BinaryString(value), VariantType::MaterialColors) => Ok(Cow::Owned( - MaterialColors::decode(value.as_ref()) - .map_err(|_| "invalid MaterialColors value")? - .into(), - )), (_, _) => Ok(value), } } diff --git a/rbx_xml/src/deserializer.rs b/rbx_xml/src/deserializer.rs index 7eb52fbaf..bd07b520d 100644 --- a/rbx_xml/src/deserializer.rs +++ b/rbx_xml/src/deserializer.rs @@ -613,7 +613,7 @@ fn deserialize_properties( }; log::trace!("property's read type: {xml_ty:?}, canonical type: {expected_type:?}"); - let value = match value.try_convert(expected_type) { + let value = match value.try_convert(class_name, expected_type) { Ok(value) => value, // The property descriptor disagreed, and there was no diff --git a/rbx_xml/src/serializer.rs b/rbx_xml/src/serializer.rs index 652a5f5f2..7708578be 100644 --- a/rbx_xml/src/serializer.rs +++ b/rbx_xml/src/serializer.rs @@ -210,7 +210,7 @@ fn serialize_instance<'dom, W: Write>( let mut serialized_name = serialized_descriptor.name.as_ref(); - let mut converted_value = match value.try_convert_ref(data_type) { + let mut converted_value = match value.try_convert_ref(instance.class, data_type) { Ok(value) => value, Err(message) => { return Err( From 8ecd2faa77787189e35817bdc4bd984e636f590a Mon Sep 17 00:00:00 2001 From: Kenneth Loeffler Date: Fri, 1 Nov 2024 15:47:43 +0000 Subject: [PATCH 2/2] Don't bail on attempt to reflect unknown serialized data types (#472) --- rbx_reflector/src/cli/generate.rs | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/rbx_reflector/src/cli/generate.rs b/rbx_reflector/src/cli/generate.rs index 8cefc0f35..fa8ca7bd6 100644 --- a/rbx_reflector/src/cli/generate.rs +++ b/rbx_reflector/src/cli/generate.rs @@ -227,12 +227,18 @@ fn apply_dump(database: &mut ReflectionDatabase, dump: &Dump) -> anyhow::Result< // are usually only present in Roblox Studio // settings files. They are not used otherwise and // can safely be ignored. - (None, PropertyKind::Canonical { - serialization: PropertySerialization::Serializes - }) if type_name != "QDir" && type_name != "QFont" => bail!( + ( + None, + PropertyKind::Canonical { + serialization: PropertySerialization::Serializes, + }, + ) if type_name != "QDir" && type_name != "QFont" => { + log::warn!( "Property {}.{} serializes, but its data type ({}) is unimplemented", dump_class.name, dump_property.name, type_name - ), + ); + continue; + } // The data type does not have a corresponding a // VariantType, and it does not serialize (with QDir @@ -241,7 +247,11 @@ fn apply_dump(database: &mut ReflectionDatabase, dump: &Dump) -> anyhow::Result< // need to know about data types that are never // serialized. (None, _) => { - ignored_properties.push((&dump_class.name, &dump_property.name, type_name)); + ignored_properties.push(( + &dump_class.name, + &dump_property.name, + type_name, + )); continue; } }