From 6552ea5ece0ec47df2f45802ce49e550b8291839 Mon Sep 17 00:00:00 2001 From: Max Date: Fri, 8 Jul 2022 15:27:38 -0500 Subject: [PATCH 01/13] Support implicit values for primitive attributes This commit adds support for strings, numbers, and booleans to be implicitly typed in attribute maps, reducing the redundancy of needing to specify their types. I also quietly adjusted one of the tests to use a more stable class/property pair. Since SourceAssetId is locked to Roblox, it could potentially disappear at any time. --- .../end_to_end__tests__build__attributes.snap | 3 +- .../attributes/default.project.json | 6 +- src/resolution.rs | 86 +++++++++++++++---- src/snapshot_middleware/json_model.rs | 2 +- src/snapshot_middleware/meta_file.rs | 4 +- src/snapshot_middleware/project.rs | 2 +- 6 files changed, 75 insertions(+), 28 deletions(-) diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__attributes.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__attributes.snap index d78f81016..8df1ae857 100644 --- a/rojo-test/build-test-snapshots/end_to_end__tests__build__attributes.snap +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__attributes.snap @@ -1,6 +1,5 @@ --- source: tests/tests/build.rs -assertion_line: 99 expression: contents --- @@ -17,7 +16,7 @@ expression: contents ImplicitAttributes - AgAAAAMAAABIZXkCBwAAAEdyYW5kbWEGAAAAVmVjdG9yEQAAgEAAAKBAAADAQA== + BAAAAAQAAABCb29sAwEDAAAASGV5AgcAAABHcmFuZG1hBAAAAFRlc3QGAAAAAAAA4D8GAAAAVmVjdG9yEQAAgEAAAKBAAADAQA== diff --git a/rojo-test/build-tests/attributes/default.project.json b/rojo-test/build-tests/attributes/default.project.json index 44be8f16a..444b30c98 100644 --- a/rojo-test/build-tests/attributes/default.project.json +++ b/rojo-test/build-tests/attributes/default.project.json @@ -23,9 +23,9 @@ "$className": "Folder", "$properties": { "Attributes": { - "Hey": { - "String": "Grandma" - }, + "Hey": "Grandma", + "Test": 0.5, + "Bool": true, "Vector": { "Vector3": [4, 5, 6] } diff --git a/src/resolution.rs b/src/resolution.rs index 547e17299..700f8bc56 100644 --- a/src/resolution.rs +++ b/src/resolution.rs @@ -1,4 +1,5 @@ use std::borrow::Borrow; +use std::collections::HashMap; use anyhow::format_err; use rbx_dom_weak::types::{ @@ -22,10 +23,17 @@ pub enum UnresolvedValue { } impl UnresolvedValue { - pub fn resolve(self, class_name: &str, prop_name: &str) -> anyhow::Result { + pub fn resolve_explicit(self, class_name: &str, prop_name: &str) -> anyhow::Result { match self { UnresolvedValue::FullyQualified(full) => Ok(full), - UnresolvedValue::Ambiguous(partial) => partial.resolve(class_name, prop_name), + UnresolvedValue::Ambiguous(partial) => partial.resolve_explicit(class_name, prop_name), + } + } + + pub fn resolve_implicit(self, attribute_name: &str) -> anyhow::Result { + match self { + UnresolvedValue::FullyQualified(full) => Ok(full), + UnresolvedValue::Ambiguous(partial) => partial.resolve_implicit(attribute_name), } } } @@ -42,10 +50,11 @@ pub enum AmbiguousValue { Array4([f64; 4]), Array12([f64; 12]), Attributes(Attributes), + UnresolvedValueMap(HashMap), } impl AmbiguousValue { - pub fn resolve(self, class_name: &str, prop_name: &str) -> anyhow::Result { + pub fn resolve_explicit(self, class_name: &str, prop_name: &str) -> anyhow::Result { let property = find_descriptor(class_name, prop_name) .ok_or_else(|| format_err!("Unknown property {}.{}", class_name, prop_name))?; @@ -129,8 +138,17 @@ impl AmbiguousValue { Ok(CFrame::new(pos, orientation).into()) } + + (VariantType::Attributes, AmbiguousValue::UnresolvedValueMap(value)) => { + let mut resolved = Attributes::new(); - (VariantType::Attributes, AmbiguousValue::Attributes(value)) => Ok(value.into()), + for (name, unresolved) in value { + let value = unresolved.resolve_implicit(&name); + resolved.insert(name.into(), value.unwrap()); + } + + Ok(resolved.into()) + } (_, unresolved) => Err(format_err!( "Wrong type of value for property {}.{}. Expected {:?}, got {}", @@ -148,6 +166,19 @@ impl AmbiguousValue { } } + pub fn resolve_implicit(self, attribute_name: &str) -> anyhow::Result { + match self { + AmbiguousValue::Bool(value) => Ok(value.into()), + AmbiguousValue::Number(value) => Ok(value.into()), + AmbiguousValue::String(value) => Ok(value.into()), + + _ => Err(format_err!( + "Cannot disambiguate implicit value of attribute {}", + attribute_name, + )) + } + } + fn describe(&self) -> &'static str { match self { AmbiguousValue::Bool(_) => "a bool", @@ -159,6 +190,7 @@ impl AmbiguousValue { AmbiguousValue::Array4(_) => "an array of four numbers", AmbiguousValue::Array12(_) => "an array of twelve numbers", AmbiguousValue::Attributes(_) => "an object containing attributes", + AmbiguousValue::UnresolvedValueMap(_) => "a map of strings to unresolved values", } } } @@ -213,30 +245,39 @@ fn nonexhaustive_list(values: &[&str]) -> String { mod test { use super::*; - fn resolve(class: &str, prop: &str, json_value: &str) -> Variant { + fn resolve_explicit(class: &str, prop: &str, json_value: &str) -> Variant { + let unresolved: UnresolvedValue = serde_json::from_str(json_value).unwrap(); + unresolved.resolve_explicit(class, prop).unwrap() + } + + fn resolve_implicit(name: &str, json_value: &str) -> Variant { let unresolved: UnresolvedValue = serde_json::from_str(json_value).unwrap(); - unresolved.resolve(class, prop).unwrap() + unresolved.resolve_implicit(name).unwrap() } #[test] fn bools() { - assert_eq!(resolve("BoolValue", "Value", "false"), Variant::Bool(false)); + assert_eq!(resolve_explicit("BoolValue", "Value", "false"), Variant::Bool(false)); // Script.Disabled is inherited from BaseScript - assert_eq!(resolve("Script", "Disabled", "true"), Variant::Bool(true)); + assert_eq!(resolve_explicit("Script", "Disabled", "true"), Variant::Bool(true)); + + // Check implicit attribute values + assert_eq!(resolve_implicit("TestBool", "false"), Variant::Bool(false)); + assert_eq!(resolve_implicit("TestBool", "true"), Variant::Bool(true)); } #[test] fn strings() { // String literals can stay as strings assert_eq!( - resolve("StringValue", "Value", "\"Hello!\""), + resolve_explicit("StringValue", "Value", "\"Hello!\""), Variant::String("Hello!".into()), ); // String literals can also turn into Content assert_eq!( - resolve("Sky", "MoonTextureId", "\"rbxassetid://12345\""), + resolve_explicit("Sky", "MoonTextureId", "\"rbxassetid://12345\""), Variant::Content("rbxassetid://12345".into()), ); @@ -244,36 +285,43 @@ mod test { // don't support any shorthands for BinaryString. // // assert_eq!( - // resolve("Folder", "Tags", "\"a\\u0000b\\u0000c\""), + // resolve_explicit("Folder", "Tags", "\"a\\u0000b\\u0000c\""), // Variant::BinaryString(b"a\0b\0c".to_vec().into()), // ); + + // Check implicit attribute value + assert_eq!( + resolve_implicit("TestString", "\"Hello world!\""), + Variant::String("Hello world!".into()), + ); } #[test] fn numbers() { assert_eq!( - resolve("Part", "CollisionGroupId", "123"), + resolve_explicit("Part", "CollisionGroupId", "123"), Variant::Int32(123), ); assert_eq!( - resolve("Folder", "SourceAssetId", "532413"), + resolve_explicit("IntValue", "Value", "532413"), Variant::Int64(532413), ); - assert_eq!(resolve("Part", "Transparency", "1"), Variant::Float32(1.0)); - assert_eq!(resolve("NumberValue", "Value", "1"), Variant::Float64(1.0)); + assert_eq!(resolve_explicit("Part", "Transparency", "1"), Variant::Float32(1.0)); + assert_eq!(resolve_explicit("NumberValue", "Value", "1"), Variant::Float64(1.0)); + assert_eq!(resolve_implicit("TestNumber", "1"), Variant::Float64(1.0)); } #[test] fn vectors() { assert_eq!( - resolve("ParticleEmitter", "SpreadAngle", "[1, 2]"), + resolve_explicit("ParticleEmitter", "SpreadAngle", "[1, 2]"), Variant::Vector2(Vector2::new(1.0, 2.0)), ); assert_eq!( - resolve("Part", "Position", "[4, 5, 6]"), + resolve_explicit("Part", "Position", "[4, 5, 6]"), Variant::Vector3(Vector3::new(4.0, 5.0, 6.0)), ); } @@ -281,7 +329,7 @@ mod test { #[test] fn colors() { assert_eq!( - resolve("Part", "Color", "[1, 1, 1]"), + resolve_explicit("Part", "Color", "[1, 1, 1]"), Variant::Color3(Color3::new(1.0, 1.0, 1.0)), ); @@ -292,7 +340,7 @@ mod test { #[test] fn enums() { assert_eq!( - resolve("Lighting", "Technology", "\"Voxel\""), + resolve_explicit("Lighting", "Technology", "\"Voxel\""), Variant::Enum(Enum::from_u32(1)), ); } diff --git a/src/snapshot_middleware/json_model.rs b/src/snapshot_middleware/json_model.rs index c29388c12..4f8e70dcc 100644 --- a/src/snapshot_middleware/json_model.rs +++ b/src/snapshot_middleware/json_model.rs @@ -92,7 +92,7 @@ impl JsonModel { let mut properties = HashMap::with_capacity(self.properties.len()); for (key, unresolved) in self.properties { - let value = unresolved.resolve(&class_name, &key)?; + let value = unresolved.resolve_explicit(&class_name, &key)?; properties.insert(key, value); } diff --git a/src/snapshot_middleware/meta_file.rs b/src/snapshot_middleware/meta_file.rs index 2715dbbe5..5c891628f 100644 --- a/src/snapshot_middleware/meta_file.rs +++ b/src/snapshot_middleware/meta_file.rs @@ -46,7 +46,7 @@ impl AdjacentMetadata { for (key, unresolved) in self.properties.drain() { let value = unresolved - .resolve(&snapshot.class_name, &key) + .resolve_explicit(&snapshot.class_name, &key) .with_context(|| format!("error applying meta file {}", path.display()))?; snapshot.properties.insert(key, value); @@ -133,7 +133,7 @@ impl DirectoryMetadata { for (key, unresolved) in self.properties.drain() { let value = unresolved - .resolve(&snapshot.class_name, &key) + .resolve_explicit(&snapshot.class_name, &key) .with_context(|| format!("error applying meta file {}", path.display()))?; snapshot.properties.insert(key, value); diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index 5650c99a1..fde0b8437 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -206,7 +206,7 @@ pub fn snapshot_project_node( for (key, unresolved) in &node.properties { let value = unresolved .clone() - .resolve(&class_name, key) + .resolve_explicit(&class_name, key) .with_context(|| { format!( "Unresolvable property in project at path {}", From 27b656be3b34f7ae00044ed41442f7d2970f3d63 Mon Sep 17 00:00:00 2001 From: Max Date: Fri, 8 Jul 2022 15:34:00 -0500 Subject: [PATCH 02/13] Apply formatting. --- src/resolution.rs | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/src/resolution.rs b/src/resolution.rs index 700f8bc56..2f03f222d 100644 --- a/src/resolution.rs +++ b/src/resolution.rs @@ -138,7 +138,7 @@ impl AmbiguousValue { Ok(CFrame::new(pos, orientation).into()) } - + (VariantType::Attributes, AmbiguousValue::UnresolvedValueMap(value)) => { let mut resolved = Attributes::new(); @@ -166,7 +166,7 @@ impl AmbiguousValue { } } - pub fn resolve_implicit(self, attribute_name: &str) -> anyhow::Result { + pub fn resolve_implicit(self, attribute_name: &str) -> anyhow::Result { match self { AmbiguousValue::Bool(value) => Ok(value.into()), AmbiguousValue::Number(value) => Ok(value.into()), @@ -175,7 +175,7 @@ impl AmbiguousValue { _ => Err(format_err!( "Cannot disambiguate implicit value of attribute {}", attribute_name, - )) + )), } } @@ -257,10 +257,16 @@ mod test { #[test] fn bools() { - assert_eq!(resolve_explicit("BoolValue", "Value", "false"), Variant::Bool(false)); + assert_eq!( + resolve_explicit("BoolValue", "Value", "false"), + Variant::Bool(false) + ); // Script.Disabled is inherited from BaseScript - assert_eq!(resolve_explicit("Script", "Disabled", "true"), Variant::Bool(true)); + assert_eq!( + resolve_explicit("Script", "Disabled", "true"), + Variant::Bool(true) + ); // Check implicit attribute values assert_eq!(resolve_implicit("TestBool", "false"), Variant::Bool(false)); @@ -308,8 +314,14 @@ mod test { Variant::Int64(532413), ); - assert_eq!(resolve_explicit("Part", "Transparency", "1"), Variant::Float32(1.0)); - assert_eq!(resolve_explicit("NumberValue", "Value", "1"), Variant::Float64(1.0)); + assert_eq!( + resolve_explicit("Part", "Transparency", "1"), + Variant::Float32(1.0) + ); + assert_eq!( + resolve_explicit("NumberValue", "Value", "1"), + Variant::Float64(1.0) + ); assert_eq!(resolve_implicit("TestNumber", "1"), Variant::Float64(1.0)); } From 49759a9fed2b931f80c89333b40e2a3fb8896a89 Mon Sep 17 00:00:00 2001 From: Max Date: Fri, 8 Jul 2022 16:50:37 -0500 Subject: [PATCH 03/13] Address feedback --- src/resolution.rs | 85 ++++++++++++--------------- src/snapshot_middleware/json_model.rs | 2 +- src/snapshot_middleware/meta_file.rs | 4 +- src/snapshot_middleware/project.rs | 2 +- 4 files changed, 41 insertions(+), 52 deletions(-) diff --git a/src/resolution.rs b/src/resolution.rs index 2f03f222d..5d5b56a91 100644 --- a/src/resolution.rs +++ b/src/resolution.rs @@ -1,7 +1,7 @@ use std::borrow::Borrow; use std::collections::HashMap; -use anyhow::format_err; +use anyhow::{bail, format_err, Context}; use rbx_dom_weak::types::{ Attributes, CFrame, Color3, Content, Enum, Matrix3, Tags, Variant, VariantType, Vector2, Vector3, @@ -23,17 +23,17 @@ pub enum UnresolvedValue { } impl UnresolvedValue { - pub fn resolve_explicit(self, class_name: &str, prop_name: &str) -> anyhow::Result { + pub fn resolve(self, class_name: &str, prop_name: &str) -> anyhow::Result { match self { UnresolvedValue::FullyQualified(full) => Ok(full), - UnresolvedValue::Ambiguous(partial) => partial.resolve_explicit(class_name, prop_name), + UnresolvedValue::Ambiguous(partial) => partial.resolve(class_name, prop_name), } } - pub fn resolve_implicit(self, attribute_name: &str) -> anyhow::Result { + pub fn resolve_unambiguous(self) -> anyhow::Result { match self { UnresolvedValue::FullyQualified(full) => Ok(full), - UnresolvedValue::Ambiguous(partial) => partial.resolve_implicit(attribute_name), + UnresolvedValue::Ambiguous(partial) => partial.resolve_unambiguous(), } } } @@ -54,7 +54,7 @@ pub enum AmbiguousValue { } impl AmbiguousValue { - pub fn resolve_explicit(self, class_name: &str, prop_name: &str) -> anyhow::Result { + pub fn resolve(self, class_name: &str, prop_name: &str) -> anyhow::Result { let property = find_descriptor(class_name, prop_name) .ok_or_else(|| format_err!("Unknown property {}.{}", class_name, prop_name))?; @@ -143,8 +143,11 @@ impl AmbiguousValue { let mut resolved = Attributes::new(); for (name, unresolved) in value { - let value = unresolved.resolve_implicit(&name); - resolved.insert(name.into(), value.unwrap()); + let value = unresolved.resolve_unambiguous().with_context(|| { + format!("Could not resolve value type of attribute '{name}'!") + })?; + + resolved.insert(name.into(), value); } Ok(resolved.into()) @@ -166,16 +169,13 @@ impl AmbiguousValue { } } - pub fn resolve_implicit(self, attribute_name: &str) -> anyhow::Result { + pub fn resolve_unambiguous(self) -> anyhow::Result { match self { AmbiguousValue::Bool(value) => Ok(value.into()), AmbiguousValue::Number(value) => Ok(value.into()), AmbiguousValue::String(value) => Ok(value.into()), - _ => Err(format_err!( - "Cannot disambiguate implicit value of attribute {}", - attribute_name, - )), + _ => bail!("Cannot unambiguously resolve type!"), } } @@ -245,45 +245,39 @@ fn nonexhaustive_list(values: &[&str]) -> String { mod test { use super::*; - fn resolve_explicit(class: &str, prop: &str, json_value: &str) -> Variant { + fn resolve(class: &str, prop: &str, json_value: &str) -> Variant { let unresolved: UnresolvedValue = serde_json::from_str(json_value).unwrap(); - unresolved.resolve_explicit(class, prop).unwrap() + unresolved.resolve(class, prop).unwrap() } - fn resolve_implicit(name: &str, json_value: &str) -> Variant { + fn resolve_unambiguous(json_value: &str) -> Variant { let unresolved: UnresolvedValue = serde_json::from_str(json_value).unwrap(); - unresolved.resolve_implicit(name).unwrap() + unresolved.resolve_unambiguous().unwrap() } #[test] fn bools() { - assert_eq!( - resolve_explicit("BoolValue", "Value", "false"), - Variant::Bool(false) - ); + assert_eq!(resolve("BoolValue", "Value", "false"), Variant::Bool(false)); // Script.Disabled is inherited from BaseScript - assert_eq!( - resolve_explicit("Script", "Disabled", "true"), - Variant::Bool(true) - ); + assert_eq!(resolve("Script", "Disabled", "true"), Variant::Bool(true)); - // Check implicit attribute values - assert_eq!(resolve_implicit("TestBool", "false"), Variant::Bool(false)); - assert_eq!(resolve_implicit("TestBool", "true"), Variant::Bool(true)); + // Check unambiguous boolean values + assert_eq!(resolve_unambiguous("false"), Variant::Bool(false)); + assert_eq!(resolve_unambiguous("true"), Variant::Bool(true)); } #[test] fn strings() { // String literals can stay as strings assert_eq!( - resolve_explicit("StringValue", "Value", "\"Hello!\""), + resolve("StringValue", "Value", "\"Hello!\""), Variant::String("Hello!".into()), ); // String literals can also turn into Content assert_eq!( - resolve_explicit("Sky", "MoonTextureId", "\"rbxassetid://12345\""), + resolve("Sky", "MoonTextureId", "\"rbxassetid://12345\""), Variant::Content("rbxassetid://12345".into()), ); @@ -291,13 +285,13 @@ mod test { // don't support any shorthands for BinaryString. // // assert_eq!( - // resolve_explicit("Folder", "Tags", "\"a\\u0000b\\u0000c\""), + // resolve("Folder", "Tags", "\"a\\u0000b\\u0000c\""), // Variant::BinaryString(b"a\0b\0c".to_vec().into()), // ); - // Check implicit attribute value + // Check unambiguous string value assert_eq!( - resolve_implicit("TestString", "\"Hello world!\""), + resolve_unambiguous("\"Hello world!\""), Variant::String("Hello world!".into()), ); } @@ -305,35 +299,30 @@ mod test { #[test] fn numbers() { assert_eq!( - resolve_explicit("Part", "CollisionGroupId", "123"), + resolve("Part", "CollisionGroupId", "123"), Variant::Int32(123), ); assert_eq!( - resolve_explicit("IntValue", "Value", "532413"), + resolve("IntValue", "Value", "532413"), Variant::Int64(532413), ); - assert_eq!( - resolve_explicit("Part", "Transparency", "1"), - Variant::Float32(1.0) - ); - assert_eq!( - resolve_explicit("NumberValue", "Value", "1"), - Variant::Float64(1.0) - ); - assert_eq!(resolve_implicit("TestNumber", "1"), Variant::Float64(1.0)); + assert_eq!(resolve("Part", "Transparency", "1"), Variant::Float32(1.0)); + assert_eq!(resolve("NumberValue", "Value", "1"), Variant::Float64(1.0)); + + assert_eq!(resolve_unambiguous("1.337"), Variant::Float64(1.337)); } #[test] fn vectors() { assert_eq!( - resolve_explicit("ParticleEmitter", "SpreadAngle", "[1, 2]"), + resolve("ParticleEmitter", "SpreadAngle", "[1, 2]"), Variant::Vector2(Vector2::new(1.0, 2.0)), ); assert_eq!( - resolve_explicit("Part", "Position", "[4, 5, 6]"), + resolve("Part", "Position", "[4, 5, 6]"), Variant::Vector3(Vector3::new(4.0, 5.0, 6.0)), ); } @@ -341,7 +330,7 @@ mod test { #[test] fn colors() { assert_eq!( - resolve_explicit("Part", "Color", "[1, 1, 1]"), + resolve("Part", "Color", "[1, 1, 1]"), Variant::Color3(Color3::new(1.0, 1.0, 1.0)), ); @@ -352,7 +341,7 @@ mod test { #[test] fn enums() { assert_eq!( - resolve_explicit("Lighting", "Technology", "\"Voxel\""), + resolve("Lighting", "Technology", "\"Voxel\""), Variant::Enum(Enum::from_u32(1)), ); } diff --git a/src/snapshot_middleware/json_model.rs b/src/snapshot_middleware/json_model.rs index 4f8e70dcc..c29388c12 100644 --- a/src/snapshot_middleware/json_model.rs +++ b/src/snapshot_middleware/json_model.rs @@ -92,7 +92,7 @@ impl JsonModel { let mut properties = HashMap::with_capacity(self.properties.len()); for (key, unresolved) in self.properties { - let value = unresolved.resolve_explicit(&class_name, &key)?; + let value = unresolved.resolve(&class_name, &key)?; properties.insert(key, value); } diff --git a/src/snapshot_middleware/meta_file.rs b/src/snapshot_middleware/meta_file.rs index 5c891628f..2715dbbe5 100644 --- a/src/snapshot_middleware/meta_file.rs +++ b/src/snapshot_middleware/meta_file.rs @@ -46,7 +46,7 @@ impl AdjacentMetadata { for (key, unresolved) in self.properties.drain() { let value = unresolved - .resolve_explicit(&snapshot.class_name, &key) + .resolve(&snapshot.class_name, &key) .with_context(|| format!("error applying meta file {}", path.display()))?; snapshot.properties.insert(key, value); @@ -133,7 +133,7 @@ impl DirectoryMetadata { for (key, unresolved) in self.properties.drain() { let value = unresolved - .resolve_explicit(&snapshot.class_name, &key) + .resolve(&snapshot.class_name, &key) .with_context(|| format!("error applying meta file {}", path.display()))?; snapshot.properties.insert(key, value); diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index fde0b8437..5650c99a1 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -206,7 +206,7 @@ pub fn snapshot_project_node( for (key, unresolved) in &node.properties { let value = unresolved .clone() - .resolve_explicit(&class_name, key) + .resolve(&class_name, key) .with_context(|| { format!( "Unresolvable property in project at path {}", From 2014689ae1061b53c21c721e10c07f08f4a48e78 Mon Sep 17 00:00:00 2001 From: Max Date: Fri, 8 Jul 2022 16:56:55 -0500 Subject: [PATCH 04/13] Backwards compatible format usage. --- src/resolution.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/resolution.rs b/src/resolution.rs index 5d5b56a91..a8167bcf6 100644 --- a/src/resolution.rs +++ b/src/resolution.rs @@ -144,7 +144,7 @@ impl AmbiguousValue { for (name, unresolved) in value { let value = unresolved.resolve_unambiguous().with_context(|| { - format!("Could not resolve value type of attribute '{name}'!") + format!("Could not resolve value type of attribute '{}'!", name) })?; resolved.insert(name.into(), value); From 6fe43a99db3cf55af97e37949bc1e961fd225650 Mon Sep 17 00:00:00 2001 From: Max Date: Mon, 11 Jul 2022 20:54:21 -0500 Subject: [PATCH 05/13] Axe UnresolvedValueMap in favor of $attributes Attributes can be defined directly on instances, with support for unambiguous types. --- src/project.rs | 7 +++++++ src/resolution.rs | 19 ++----------------- src/snapshot_middleware/json_model.rs | 15 +++++++++++++++ src/snapshot_middleware/meta_file.rs | 17 +++++++++++++++++ src/snapshot_middleware/project.rs | 18 ++++++++++++++++++ 5 files changed, 59 insertions(+), 17 deletions(-) diff --git a/src/project.rs b/src/project.rs index 53d52808f..330cbd23a 100644 --- a/src/project.rs +++ b/src/project.rs @@ -229,6 +229,13 @@ pub struct ProjectNode { )] pub properties: HashMap, + #[serde( + rename = "$attributes", + default, + skip_serializing_if = "HashMap::is_empty" + )] + pub attributes: HashMap, + /// Defines the behavior when Rojo encounters unknown instances in Roblox /// Studio during live sync. `$ignoreUnknownInstances` should be considered /// a large hammer and used with care. diff --git a/src/resolution.rs b/src/resolution.rs index a8167bcf6..62e1d7eeb 100644 --- a/src/resolution.rs +++ b/src/resolution.rs @@ -1,7 +1,6 @@ use std::borrow::Borrow; -use std::collections::HashMap; -use anyhow::{bail, format_err, Context}; +use anyhow::{bail, format_err}; use rbx_dom_weak::types::{ Attributes, CFrame, Color3, Content, Enum, Matrix3, Tags, Variant, VariantType, Vector2, Vector3, @@ -50,7 +49,6 @@ pub enum AmbiguousValue { Array4([f64; 4]), Array12([f64; 12]), Attributes(Attributes), - UnresolvedValueMap(HashMap), } impl AmbiguousValue { @@ -139,19 +137,7 @@ impl AmbiguousValue { Ok(CFrame::new(pos, orientation).into()) } - (VariantType::Attributes, AmbiguousValue::UnresolvedValueMap(value)) => { - let mut resolved = Attributes::new(); - - for (name, unresolved) in value { - let value = unresolved.resolve_unambiguous().with_context(|| { - format!("Could not resolve value type of attribute '{}'!", name) - })?; - - resolved.insert(name.into(), value); - } - - Ok(resolved.into()) - } + (VariantType::Attributes, AmbiguousValue::Attributes(value)) => Ok(value.into()), (_, unresolved) => Err(format_err!( "Wrong type of value for property {}.{}. Expected {:?}, got {}", @@ -190,7 +176,6 @@ impl AmbiguousValue { AmbiguousValue::Array4(_) => "an array of four numbers", AmbiguousValue::Array12(_) => "an array of twelve numbers", AmbiguousValue::Attributes(_) => "an object containing attributes", - AmbiguousValue::UnresolvedValueMap(_) => "a map of strings to unresolved values", } } } diff --git a/src/snapshot_middleware/json_model.rs b/src/snapshot_middleware/json_model.rs index c29388c12..787981366 100644 --- a/src/snapshot_middleware/json_model.rs +++ b/src/snapshot_middleware/json_model.rs @@ -2,6 +2,7 @@ use std::{borrow::Cow, collections::HashMap, path::Path, str}; use anyhow::Context; use memofs::Vfs; +use rbx_dom_weak::types::Attributes; use serde::Deserialize; use crate::{ @@ -78,6 +79,9 @@ struct JsonModel { skip_serializing_if = "HashMap::is_empty" )] properties: HashMap, + + #[serde(default = "HashMap::new", skip_serializing_if = "HashMap::is_empty")] + attributes: HashMap, } impl JsonModel { @@ -96,6 +100,17 @@ impl JsonModel { properties.insert(key, value); } + if !self.attributes.is_empty() { + let mut attributes = Attributes::new(); + + for (key, unresolved) in self.attributes { + let value = unresolved.resolve_unambiguous()?; + attributes.insert(key, value); + } + + properties.insert("Attributes".to_string(), attributes.into()); + } + Ok(InstanceSnapshot { snapshot_id: None, metadata: Default::default(), diff --git a/src/snapshot_middleware/meta_file.rs b/src/snapshot_middleware/meta_file.rs index 2715dbbe5..50db9b882 100644 --- a/src/snapshot_middleware/meta_file.rs +++ b/src/snapshot_middleware/meta_file.rs @@ -1,6 +1,7 @@ use std::{borrow::Cow, collections::HashMap, path::PathBuf}; use anyhow::{format_err, Context}; +use rbx_dom_weak::types::Attributes; use serde::{Deserialize, Serialize}; use crate::{resolution::UnresolvedValue, snapshot::InstanceSnapshot}; @@ -78,6 +79,9 @@ pub struct DirectoryMetadata { #[serde(default, skip_serializing_if = "HashMap::is_empty")] pub properties: HashMap, + #[serde(default, skip_serializing_if = "HashMap::is_empty")] + pub attributes: HashMap, + #[serde(skip_serializing_if = "Option::is_none")] pub class_name: Option, @@ -139,6 +143,19 @@ impl DirectoryMetadata { snapshot.properties.insert(key, value); } + if !self.attributes.is_empty() { + let mut attributes = Attributes::new(); + + for (key, unresolved) in self.attributes.drain() { + let value = unresolved.resolve_unambiguous()?; + attributes.insert(key, value); + } + + snapshot + .properties + .insert("Attributes".to_string(), attributes.into()); + } + Ok(()) } } diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index 5650c99a1..677108ba8 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -2,6 +2,7 @@ use std::{borrow::Cow, collections::HashMap, path::Path}; use anyhow::{bail, Context}; use memofs::Vfs; +use rbx_dom_weak::types::Attributes; use rbx_reflection::ClassTag; use crate::{ @@ -231,6 +232,23 @@ pub fn snapshot_project_node( properties.insert(key.clone(), value); } + if !node.attributes.is_empty() { + let mut attributes = Attributes::new(); + + for (key, unresolved) in &node.attributes { + let value = unresolved.clone().resolve_unambiguous().with_context(|| { + format!( + "Unresolvable attribute in project at path {}", + project_path.display() + ) + })?; + + attributes.insert(key.clone(), value); + } + + properties.insert("Attributes".to_string(), attributes.into()); + } + // If the user specified $ignoreUnknownInstances, overwrite the existing // value. // From 4f97d42348fa5ea41cc087e6ac427485f4bd03b0 Mon Sep 17 00:00:00 2001 From: Max Date: Mon, 11 Jul 2022 20:59:16 -0500 Subject: [PATCH 06/13] Adjust test. --- .../attributes/default.project.json | 26 ++++++++----------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/rojo-test/build-tests/attributes/default.project.json b/rojo-test/build-tests/attributes/default.project.json index 444b30c98..d88818d65 100644 --- a/rojo-test/build-tests/attributes/default.project.json +++ b/rojo-test/build-tests/attributes/default.project.json @@ -7,13 +7,11 @@ "$className": "Folder", "$properties": { "Attributes": { - "Attributes": { - "Hello": { - "String": "World" - }, - "Vector": { - "Vector3": [1, 2, 3] - } + "Hello": { + "String": "World" + }, + "Vector": { + "Vector3": [1, 2, 3] } } } @@ -21,14 +19,12 @@ "ImplicitAttributes": { "$className": "Folder", - "$properties": { - "Attributes": { - "Hey": "Grandma", - "Test": 0.5, - "Bool": true, - "Vector": { - "Vector3": [4, 5, 6] - } + "$attributes": { + "Hey": "Grandma", + "Test": 0.5, + "Bool": true, + "Vector": { + "Vector3": [4, 5, 6] } } } From 8da31ccf6db1a1e00f74431f2d07281ec2863220 Mon Sep 17 00:00:00 2001 From: Max Date: Tue, 12 Jul 2022 13:10:35 -0500 Subject: [PATCH 07/13] to_string() -> into() --- src/snapshot_middleware/json_model.rs | 2 +- src/snapshot_middleware/meta_file.rs | 2 +- src/snapshot_middleware/project.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/snapshot_middleware/json_model.rs b/src/snapshot_middleware/json_model.rs index 787981366..0f1388e4f 100644 --- a/src/snapshot_middleware/json_model.rs +++ b/src/snapshot_middleware/json_model.rs @@ -108,7 +108,7 @@ impl JsonModel { attributes.insert(key, value); } - properties.insert("Attributes".to_string(), attributes.into()); + properties.insert("Attributes".into(), attributes.into()); } Ok(InstanceSnapshot { diff --git a/src/snapshot_middleware/meta_file.rs b/src/snapshot_middleware/meta_file.rs index 50db9b882..abf53fbab 100644 --- a/src/snapshot_middleware/meta_file.rs +++ b/src/snapshot_middleware/meta_file.rs @@ -153,7 +153,7 @@ impl DirectoryMetadata { snapshot .properties - .insert("Attributes".to_string(), attributes.into()); + .insert("Attributes".into(), attributes.into()); } Ok(()) diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index 677108ba8..f8fa7920e 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -246,7 +246,7 @@ pub fn snapshot_project_node( attributes.insert(key.clone(), value); } - properties.insert("Attributes".to_string(), attributes.into()); + properties.insert("Attributes".into(), attributes.into()); } // If the user specified $ignoreUnknownInstances, overwrite the existing From baeba5ce5f9dd498b30dfa385a48ab713be44540 Mon Sep 17 00:00:00 2001 From: Max Date: Tue, 12 Jul 2022 14:33:22 -0500 Subject: [PATCH 08/13] Made attribute test more concise. --- .../end_to_end__tests__build__attributes.snap | 18 ++- .../attributes/default.project.json | 108 +++++++++++++++--- 2 files changed, 108 insertions(+), 18 deletions(-) diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__attributes.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__attributes.snap index 8df1ae857..9cdcc28a9 100644 --- a/rojo-test/build-test-snapshots/end_to_end__tests__build__attributes.snap +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__attributes.snap @@ -9,14 +9,26 @@ expression: contents - Explicit - AgAAAAUAAABIZWxsbwIFAAAAV29ybGQGAAAAVmVjdG9yEQAAgD8AAABAAABAQA== + ExplicitAttributes + DQAAAAQAAABCb29sAwEKAAAAQnJpY2tDb2xvcg4BAAAABgAAAENvbG9yMw8AAAAAAAAAAAAAAAANAAAAQ29sb3JTZXF1ZW5jZRkCAAAAAAAAAAAAAAAAAIA/AACAPwAAgD8AAAAAAACAPwAAgD8AAIA/AACAPwcAAABGbG9hdDMyBQAAAAAHAAAARmxvYXQ2NAYAAAAAAAAAAAsAAABOdW1iZXJSYW5nZRsAAAAAAAAAAA4AAABOdW1iZXJTZXF1ZW5jZRcCAAAAAAAAAAAAAAAAAAAAAAAAAAAAgD8AAAAABAAAAFJlY3QcAAAAAAAAAAAAAAAAAAAAAAQAAABVRGltCQAAAAAAAAAABQAAAFVEaW0yCgAAAAAAAAAAAAAAAAAAAAAHAAAAVmVjdG9yMhAAAAAAAAAAAAcAAABWZWN0b3IzEQAAAAAAAAAAAAAAAA== ImplicitAttributes - BAAAAAQAAABCb29sAwEDAAAASGV5AgcAAABHcmFuZG1hBAAAAFRlc3QGAAAAAAAA4D8GAAAAVmVjdG9yEQAAgEAAAKBAAADAQA== + AwAAAAQAAABCb29sAwEGAAAATnVtYmVyBgAAAAAAAOA/BgAAAFN0cmluZwIEAAAAVGVzdA== + + + + + LegacyExplicit + AgAAAAUAAABIZWxsbwIFAAAAV29ybGQGAAAAVmVjdG9yEQAAgD8AAABAAABAQA== + + + + + LegacyImplicit + AgAAAAMAAABIZXkCBwAAAEdyYW5kbWEGAAAAVmVjdG9yEQAAgEAAAKBAAADAQA== diff --git a/rojo-test/build-tests/attributes/default.project.json b/rojo-test/build-tests/attributes/default.project.json index d88818d65..edfe1c2cc 100644 --- a/rojo-test/build-tests/attributes/default.project.json +++ b/rojo-test/build-tests/attributes/default.project.json @@ -2,29 +2,107 @@ "name": "attributes", "tree": { "$className": "Folder", - - "Explicit": { + "ImplicitAttributes": { + "$className": "Folder", + "$attributes": { + "Bool": true, + "Number": 0.5, + "String": "Test" + } + }, + "ExplicitAttributes": { + "$className": "Folder", + "$attributes": { + "Bool": { + "Bool": true + }, + "Float32": { + "Float32": 0 + }, + "Float64": { + "Float64": 0 + }, + "UDim": { + "UDim": [0, 0] + }, + "UDim2": { + "UDim2": [[0, 0], [0, 0]] + }, + "BrickColor": { + "BrickColor": 1 + }, + "Color3": { + "Color3": [0, 0, 0] + }, + "Vector2": { + "Vector2": [0, 0] + }, + "Vector3": { + "Vector3": [0, 0, 0] + }, + "NumberSequence": { + "NumberSequence": { + "keypoints": [ + { + "time": 0, + "value": 0, + "envelope": 0 + }, + { + "time": 1, + "value": 0, + "envelope": 0 + } + ] + } + }, + "ColorSequence": { + "ColorSequence": { + "keypoints": [ + { + "time": 0, + "color": [1, 1, 1] + }, + { + "time": 1, + "color": [1, 1, 1] + } + ] + } + }, + "NumberRange": { + "NumberRange": [0, 0] + }, + "Rect": { + "Rect": [[0, 0], [0, 0]] + } + } + }, + "LegacyExplicit": { "$className": "Folder", "$properties": { "Attributes": { - "Hello": { - "String": "World" - }, - "Vector": { - "Vector3": [1, 2, 3] + "Attributes": { + "Hello": { + "String": "World" + }, + "Vector": { + "Vector3": [1, 2, 3] + } } } } }, - - "ImplicitAttributes": { + "LegacyImplicit": { "$className": "Folder", - "$attributes": { - "Hey": "Grandma", - "Test": 0.5, - "Bool": true, - "Vector": { - "Vector3": [4, 5, 6] + "$properties": { + "Attributes": { + "Hey": { + "String": "Grandma" + }, + "Vector": { + "Vector3": [4, 5, 6] + } } } } From 5bc946970f870199c0e59d3e993bea6813e11697 Mon Sep 17 00:00:00 2001 From: Max Date: Tue, 12 Jul 2022 14:35:51 -0500 Subject: [PATCH 09/13] small cleanup --- .../end_to_end__tests__build__attributes.snap | 4 ++-- rojo-test/build-tests/attributes/default.project.json | 8 ++++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__attributes.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__attributes.snap index 9cdcc28a9..04296e4e5 100644 --- a/rojo-test/build-test-snapshots/end_to_end__tests__build__attributes.snap +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__attributes.snap @@ -9,13 +9,13 @@ expression: contents - ExplicitAttributes + Explicit DQAAAAQAAABCb29sAwEKAAAAQnJpY2tDb2xvcg4BAAAABgAAAENvbG9yMw8AAAAAAAAAAAAAAAANAAAAQ29sb3JTZXF1ZW5jZRkCAAAAAAAAAAAAAAAAAIA/AACAPwAAgD8AAAAAAACAPwAAgD8AAIA/AACAPwcAAABGbG9hdDMyBQAAAAAHAAAARmxvYXQ2NAYAAAAAAAAAAAsAAABOdW1iZXJSYW5nZRsAAAAAAAAAAA4AAABOdW1iZXJTZXF1ZW5jZRcCAAAAAAAAAAAAAAAAAAAAAAAAAAAAgD8AAAAABAAAAFJlY3QcAAAAAAAAAAAAAAAAAAAAAAQAAABVRGltCQAAAAAAAAAABQAAAFVEaW0yCgAAAAAAAAAAAAAAAAAAAAAHAAAAVmVjdG9yMhAAAAAAAAAAAAcAAABWZWN0b3IzEQAAAAAAAAAAAAAAAA== - ImplicitAttributes + Implicit AwAAAAQAAABCb29sAwEGAAAATnVtYmVyBgAAAAAAAOA/BgAAAFN0cmluZwIEAAAAVGVzdA== diff --git a/rojo-test/build-tests/attributes/default.project.json b/rojo-test/build-tests/attributes/default.project.json index edfe1c2cc..b9bb2d952 100644 --- a/rojo-test/build-tests/attributes/default.project.json +++ b/rojo-test/build-tests/attributes/default.project.json @@ -2,7 +2,8 @@ "name": "attributes", "tree": { "$className": "Folder", - "ImplicitAttributes": { + + "Implicit": { "$className": "Folder", "$attributes": { "Bool": true, @@ -10,7 +11,8 @@ "String": "Test" } }, - "ExplicitAttributes": { + + "Explicit": { "$className": "Folder", "$attributes": { "Bool": { @@ -78,6 +80,7 @@ } } }, + "LegacyExplicit": { "$className": "Folder", "$properties": { @@ -93,6 +96,7 @@ } } }, + "LegacyImplicit": { "$className": "Folder", "$properties": { From 07fd85a4b1a096ff8c666ece5999196b4872123b Mon Sep 17 00:00:00 2001 From: Lucien Greathouse Date: Wed, 3 Aug 2022 19:28:43 -0400 Subject: [PATCH 10/13] Update src/resolution.rs --- src/resolution.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/resolution.rs b/src/resolution.rs index 62e1d7eeb..8b9930a43 100644 --- a/src/resolution.rs +++ b/src/resolution.rs @@ -161,7 +161,7 @@ impl AmbiguousValue { AmbiguousValue::Number(value) => Ok(value.into()), AmbiguousValue::String(value) => Ok(value.into()), - _ => bail!("Cannot unambiguously resolve type!"), + other => bail!("Cannot unambiguously resolve the value {other:?}"), } } From 09b0a346eec48bc4f45747f12342549356adab2d Mon Sep 17 00:00:00 2001 From: Lucien Greathouse Date: Wed, 3 Aug 2022 19:28:47 -0400 Subject: [PATCH 11/13] Update src/resolution.rs --- src/resolution.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/resolution.rs b/src/resolution.rs index 8b9930a43..5ad827395 100644 --- a/src/resolution.rs +++ b/src/resolution.rs @@ -247,7 +247,6 @@ mod test { // Script.Disabled is inherited from BaseScript assert_eq!(resolve("Script", "Disabled", "true"), Variant::Bool(true)); - // Check unambiguous boolean values assert_eq!(resolve_unambiguous("false"), Variant::Bool(false)); assert_eq!(resolve_unambiguous("true"), Variant::Bool(true)); } From 039726265b125774947ea7a38fb522f9e436ff38 Mon Sep 17 00:00:00 2001 From: Lucien Greathouse Date: Wed, 3 Aug 2022 19:28:50 -0400 Subject: [PATCH 12/13] Update src/resolution.rs --- src/resolution.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/resolution.rs b/src/resolution.rs index 5ad827395..7eab944c9 100644 --- a/src/resolution.rs +++ b/src/resolution.rs @@ -273,7 +273,6 @@ mod test { // Variant::BinaryString(b"a\0b\0c".to_vec().into()), // ); - // Check unambiguous string value assert_eq!( resolve_unambiguous("\"Hello world!\""), Variant::String("Hello world!".into()), From 26fc1a7ba65abb47a3f92d1ba72318417afd1588 Mon Sep 17 00:00:00 2001 From: Lucien Greathouse Date: Wed, 3 Aug 2022 19:28:54 -0400 Subject: [PATCH 13/13] Update src/resolution.rs --- src/resolution.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/resolution.rs b/src/resolution.rs index 7eab944c9..568891454 100644 --- a/src/resolution.rs +++ b/src/resolution.rs @@ -294,7 +294,7 @@ mod test { assert_eq!(resolve("Part", "Transparency", "1"), Variant::Float32(1.0)); assert_eq!(resolve("NumberValue", "Value", "1"), Variant::Float64(1.0)); - assert_eq!(resolve_unambiguous("1.337"), Variant::Float64(1.337)); + assert_eq!(resolve_unambiguous("12.5"), Variant::Float64(12.5)); } #[test]