From 29d82396aa27ad3c1552fbf4b3ae9cab9c70994a Mon Sep 17 00:00:00 2001 From: kennethloeffler Date: Thu, 28 Mar 2024 13:58:15 -0700 Subject: [PATCH 1/6] Rename first arg of variant_type_from_str from value -> type_name --- rbx_reflector/src/cli/generate.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rbx_reflector/src/cli/generate.rs b/rbx_reflector/src/cli/generate.rs index 82089cd2d..59456f77f 100644 --- a/rbx_reflector/src/cli/generate.rs +++ b/rbx_reflector/src/cli/generate.rs @@ -221,8 +221,8 @@ fn apply_dump(database: &mut ReflectionDatabase, dump: &Dump) -> anyhow::Result< Ok(()) } -fn variant_type_from_str(value: &str) -> anyhow::Result> { - Ok(Some(match value { +fn variant_type_from_str(type_name: &str) -> anyhow::Result> { + Ok(Some(match type_name { "Axes" => VariantType::Axes, "BinaryString" => VariantType::BinaryString, "BrickColor" => VariantType::BrickColor, @@ -272,6 +272,6 @@ fn variant_type_from_str(value: &str) -> anyhow::Result> { // These types are not generally implemented right now. "QDir" | "QFont" | "SystemAddress" | "CSGPropertyData" => return Ok(None), - _ => bail!("Unknown type {}", value), + _ => bail!("unknown type {type_name}"), })) } From 4ec2cb611858c3f8ac5f071b372b5d556704ebf0 Mon Sep 17 00:00:00 2001 From: kennethloeffler Date: Thu, 28 Mar 2024 13:59:59 -0700 Subject: [PATCH 2/6] Remove unimplemented data types variant_type_from_str, don't bail --- rbx_reflector/src/cli/generate.rs | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/rbx_reflector/src/cli/generate.rs b/rbx_reflector/src/cli/generate.rs index 59456f77f..9a210ea7d 100644 --- a/rbx_reflector/src/cli/generate.rs +++ b/rbx_reflector/src/cli/generate.rs @@ -261,17 +261,6 @@ fn variant_type_from_str(type_name: &str) -> anyhow::Result> // ProtectedString is handled as the same as string "ProtectedString" => VariantType::String, - // TweenInfo is not supported by rbx_types yet - "TweenInfo" => return Ok(None), - - // While DateTime is possible to Serialize, the only use it has as a - // DataType is for the TextChatMessage class, which cannot be serialized - // (at least not saved to file as it is locked to nil parent) - "DateTime" => return Ok(None), - - // These types are not generally implemented right now. - "QDir" | "QFont" | "SystemAddress" | "CSGPropertyData" => return Ok(None), - - _ => bail!("unknown type {type_name}"), + _ => return Ok(None), })) } From 7da3c8f7e318e57b8db28babd1d5cbe340b62921 Mon Sep 17 00:00:00 2001 From: kennethloeffler Date: Thu, 28 Mar 2024 15:41:36 -0700 Subject: [PATCH 3/6] Check serialization of unknown data types before bailing apply_dump --- rbx_reflector/src/cli/generate.rs | 46 ++++++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/rbx_reflector/src/cli/generate.rs b/rbx_reflector/src/cli/generate.rs index 9a210ea7d..043909d3a 100644 --- a/rbx_reflector/src/cli/generate.rs +++ b/rbx_reflector/src/cli/generate.rs @@ -174,10 +174,48 @@ fn apply_dump(database: &mut ReflectionDatabase, dump: &Dump) -> anyhow::Result< let value_type = match dump_property.value_type.category { ValueCategory::Enum => DataType::Enum(type_name.clone().into()), ValueCategory::Primitive | ValueCategory::DataType => { - match variant_type_from_str(type_name)? { - Some(variant_type) => DataType::Value(variant_type), - None => { - log::debug!("Skipping property {}.{} because it was of unsupported type '{type_name}'", dump_class.name, dump_property.name); + // variant_type_from_str returns None when passed a + // type name that does not have a corresponding + // VariantType. Exactly what we'd like to do about + // unimplemented data types like this depends on if the + // property serializes or not. + match (variant_type_from_str(type_name)?, &kind) { + // The happy path: the data type has a corresponding + // VariantType. We don't need to care about whether + // the data type is ever serialized, because it + // already has an implementation. + (Some(variant_type), _) => DataType::Value(variant_type), + + // The data type does not have a corresponding + // VariantType, and it serializes. This is a case + // where we should fail. It means that we may need + // to implement the data type. + // + // There is a special exception for QDir and QFont, + // because these types serialize under a few + // different properties, but the properties are not + // normally present in place or model files. They + // 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!( + "Property {}.{} serializes, but its data type ({}) is unimplemented", + dump_class.name, dump_property.name, type_name + ), + + // The data type does not have a corresponding a + // VariantType, and it does not serialize (with QDir + // and QFont as exceptions, noted above). We can + // safely ignore this case because rbx-dom doesn't + // need to know about data types that are never + // serialized. + (None, _) => { + log::debug!( + "Skipping property {}.{} because it is of unimplemented type '{type_name}' and is not serialized", + dump_class.name, dump_property.name + ); continue; } } From 47ec1ea69ff652e15e8329e7695e754815dd20a2 Mon Sep 17 00:00:00 2001 From: kennethloeffler Date: Thu, 28 Mar 2024 15:44:22 -0700 Subject: [PATCH 4/6] Make variant_type_from_str infallible --- rbx_reflector/src/cli/generate.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/rbx_reflector/src/cli/generate.rs b/rbx_reflector/src/cli/generate.rs index 043909d3a..4985e034e 100644 --- a/rbx_reflector/src/cli/generate.rs +++ b/rbx_reflector/src/cli/generate.rs @@ -179,7 +179,7 @@ fn apply_dump(database: &mut ReflectionDatabase, dump: &Dump) -> anyhow::Result< // VariantType. Exactly what we'd like to do about // unimplemented data types like this depends on if the // property serializes or not. - match (variant_type_from_str(type_name)?, &kind) { + match (variant_type_from_str(type_name), &kind) { // The happy path: the data type has a corresponding // VariantType. We don't need to care about whether // the data type is ever serialized, because it @@ -259,8 +259,8 @@ fn apply_dump(database: &mut ReflectionDatabase, dump: &Dump) -> anyhow::Result< Ok(()) } -fn variant_type_from_str(type_name: &str) -> anyhow::Result> { - Ok(Some(match type_name { +fn variant_type_from_str(type_name: &str) -> Option { + Some(match type_name { "Axes" => VariantType::Axes, "BinaryString" => VariantType::BinaryString, "BrickColor" => VariantType::BrickColor, @@ -299,6 +299,6 @@ fn variant_type_from_str(type_name: &str) -> anyhow::Result> // ProtectedString is handled as the same as string "ProtectedString" => VariantType::String, - _ => return Ok(None), - })) + _ => return None, + }) } From 060f4b14f4aaf4dd83abf75de90e963300b3db9a Mon Sep 17 00:00:00 2001 From: kennethloeffler Date: Thu, 28 Mar 2024 15:59:08 -0700 Subject: [PATCH 5/6] Make ingored properties debug output prettier --- rbx_reflector/src/cli/generate.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/rbx_reflector/src/cli/generate.rs b/rbx_reflector/src/cli/generate.rs index 4985e034e..8264936ae 100644 --- a/rbx_reflector/src/cli/generate.rs +++ b/rbx_reflector/src/cli/generate.rs @@ -106,6 +106,8 @@ impl GenerateSubcommand { } fn apply_dump(database: &mut ReflectionDatabase, dump: &Dump) -> anyhow::Result<()> { + let mut ignored_properties = Vec::new(); + for dump_class in &dump.classes { let superclass = if dump_class.superclass == "<<>>" { None @@ -212,10 +214,7 @@ fn apply_dump(database: &mut ReflectionDatabase, dump: &Dump) -> anyhow::Result< // need to know about data types that are never // serialized. (None, _) => { - log::debug!( - "Skipping property {}.{} because it is of unimplemented type '{type_name}' and is not serialized", - dump_class.name, dump_property.name - ); + ignored_properties.push((&dump_class.name, &dump_property.name, type_name)); continue; } } @@ -242,6 +241,12 @@ fn apply_dump(database: &mut ReflectionDatabase, dump: &Dump) -> anyhow::Result< .insert(Cow::Owned(dump_class.name.clone()), class); } + log::debug!("Skipped the following properties because their data types are not implemented, and do not need to serialize:"); + + for (class_name, property_name, type_name) in ignored_properties { + log::debug!("{class_name}.{property_name}: {type_name}"); + } + for dump_enum in &dump.enums { let mut descriptor = EnumDescriptor::new(dump_enum.name.clone()); From c7f9c46697eeaaa02a058de217a792c31485405d Mon Sep 17 00:00:00 2001 From: kennethloeffler Date: Thu, 28 Mar 2024 16:20:35 -0700 Subject: [PATCH 6/6] Silence warnings --- rbx_reflection/src/class_tag.rs | 1 + rbx_reflection/src/property_tag.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/rbx_reflection/src/class_tag.rs b/rbx_reflection/src/class_tag.rs index 82b8bd4f0..a986d45ed 100644 --- a/rbx_reflection/src/class_tag.rs +++ b/rbx_reflection/src/class_tag.rs @@ -18,6 +18,7 @@ pub enum ClassTag { } #[derive(Debug)] +#[allow(dead_code)] pub struct ClassTagFromStrError(String); impl FromStr for ClassTag { diff --git a/rbx_reflection/src/property_tag.rs b/rbx_reflection/src/property_tag.rs index 45ae0fbe6..bc4af4ac2 100644 --- a/rbx_reflection/src/property_tag.rs +++ b/rbx_reflection/src/property_tag.rs @@ -17,6 +17,7 @@ pub enum PropertyTag { } #[derive(Debug)] +#[allow(dead_code)] pub struct PropertyTagFromStrError(String); impl FromStr for PropertyTag {