From 33ddb4401674bde2da27288525f51b3a9197c021 Mon Sep 17 00:00:00 2001 From: NetterSilver <133535268+Silvertree2010@users.noreply.github.com> Date: Thu, 29 Oct 2026 14:11:48 +0100 Subject: [PATCH] Resolve stored variable ids by exact id, not by variable name Fixes #1513 Co-Authored-By: Claude Opus 5.5 --- crates/doc/src/styles.rs | 8 ++++- crates/doc/src/tests.rs | 36 +++++++++++++++++++ crates/engine/src/cmd/styles.rs | 2 +- crates/engine/src/library_variables_tests.rs | 8 +++-- crates/engine/src/variable_props_tests.rs | 11 +++--- crates/engine/tests/it/main.rs | 2 ++ .../engine/tests/it/variable_binding_ids.rs | 32 +++++++++++++++++ crates/format/src/penpot.rs | 18 +++++----- 8 files changed, 98 insertions(+), 19 deletions(-) create mode 100644 crates/engine/tests/it/variable_binding_ids.rs diff --git a/crates/doc/src/styles.rs b/crates/doc/src/styles.rs index a9cad14a..92ea24cc 100644 --- a/crates/doc/src/styles.rs +++ b/crates/doc/src/styles.rs @@ -268,6 +268,12 @@ impl Variables { self.variables.iter().find(|v| v.id == id || v.name == id) } + /// Look up an already resolved variable id (a stored binding or alias): exact ids only, so a + /// variable named like an id never stands in for it. + pub fn get_by_id(&self, id: &str) -> Option<&Variable> { + self.variables.iter().find(|v| v.id == id) + } + pub fn collection(&self, id: &str) -> Option<&Collection> { self.collections.iter().find(|c| c.id == id || c.name == id) } @@ -277,7 +283,7 @@ impl Variables { pub fn resolve(&self, id: &str, modes: &BTreeMap) -> Option { let mut cur = id.to_string(); for _ in 0..32 { - let var = self.get(&cur)?; + let var = self.get_by_id(&cur)?; let coll = self.collection(&var.collection)?; let mode = modes.get(&coll.id).filter(|m| var.values.contains_key(*m)).or_else(|| coll.default_mode())?; match var.values.get(mode).or_else(|| var.values.values().next())? { diff --git a/crates/doc/src/tests.rs b/crates/doc/src/tests.rs index 70dcd25b..0ed0ef35 100644 --- a/crates/doc/src/tests.rs +++ b/crates/doc/src/tests.rs @@ -310,6 +310,42 @@ fn variables_resolve_modes_and_aliases() { assert_eq!(v.resolve("loop", &std::collections::BTreeMap::new()), None); } +#[test] +fn variables_resolve_stored_ids_never_by_name() { + let mut v = Variables::default(); + v.collections.push(Collection { + id: "c1".into(), + name: "Theme".into(), + modes: vec![Mode { id: "m".into(), name: "Mode 1".into() }], + extends: None, + }); + let var = |id: &str, name: &str, value: VariableValue| Variable { + id: id.into(), + name: name.into(), + collection: "c1".into(), + kind: VariableType::Color, + values: std::collections::BTreeMap::from([("m".to_string(), value)]), + description: String::new(), + scopes: None, + hidden_from_publishing: false, + extends: None, + overrides: Default::default(), + code_syntax: Default::default(), + }; + let red = Color::from_hex("#FF0000").unwrap(); + let blue = Color::from_hex("#0D99FF").unwrap(); + // Named like the next variable's id, and like an id that no longer exists. + v.variables.push(var("VariableID:1", "VariableID:2", VariableValue::Color(red))); + v.variables.push(var("VariableID:2", "surface", VariableValue::Color(blue))); + v.variables.push(var("VariableID:3", "VariableID:9", VariableValue::Color(red))); + v.variables.push(var("VariableID:4", "alias", VariableValue::Alias("VariableID:9".into()))); + let modes = std::collections::BTreeMap::new(); + assert_eq!(v.resolve("VariableID:2", &modes), Some(VariableValue::Color(blue))); + // A deleted id stays unresolved instead of picking up the variable named like it. + assert_eq!(v.resolve("VariableID:9", &modes), None); + assert_eq!(v.resolve("VariableID:4", &modes), None); +} + #[test] fn serde_roundtrip_and_repair() { let mut d = Document::new("x"); diff --git a/crates/engine/src/cmd/styles.rs b/crates/engine/src/cmd/styles.rs index 0f3fdef6..c25fda80 100644 --- a/crates/engine/src/cmd/styles.rs +++ b/crates/engine/src/cmd/styles.rs @@ -784,7 +784,7 @@ pub fn bind(s: &mut Session, p: &Value) -> Result { pub(crate) fn bind_variable(cmd: &str, doc: &mut Document, ids: &[NodeId], field: &str, vid: Option<&str>) -> Result<()> { // An instance's component property takes a variable of its type; other fields the types their // scopes belong to. - if let Some(v) = vid.and_then(|v| doc.variables.get(v)) { + if let Some(v) = vid.and_then(|v| doc.variables.get_by_id(v)) { for i in ids { match field.strip_prefix(crate::components::PROP_BINDING) { Some(name) => super::component::check_prop_binding(doc, *i, name, v.kind).map_err(|e| bad(cmd, e))?, diff --git a/crates/engine/src/library_variables_tests.rs b/crates/engine/src/library_variables_tests.rs index f61bab29..a811806e 100644 --- a/crates/engine/src/library_variables_tests.rs +++ b/crates/engine/src/library_variables_tests.rs @@ -171,7 +171,9 @@ fn library_update_follows(s: &mut Session, lib: &mut Session, local: &str, [inne // Same copies (same ids), new values: the bound layers and the alias follow. assert_eq!(copy_of(s, "primary").id, local); assert_eq!((fill(s, inner), fill(s, outer)), ("222222".into(), "FF0000".into())); - assert!(matches!(s.doc().unwrap().variables.resolve("brand", &modes), Some(VariableValue::Color(c)) if c.to_hex() == "00FFFF")); + assert!( + matches!(s.doc().unwrap().variables.resolve(&s.doc().unwrap().variables.get("brand").unwrap().id, &modes), Some(VariableValue::Color(c)) if c.to_hex() == "00FFFF") + ); assert_eq!(s.doc().unwrap().variables.collections[0].modes[1].name, "Night"); assert_eq!(ex(s, "library.updates", json!({}))["count"], 0); // Undo takes the update back. @@ -234,7 +236,9 @@ fn bind_alias_mode_read_only_and_updates() { ex(&mut s, "variable.create", json!({"collection": "App", "name": "gap", "type": "number", "value": 2})); ex(&mut s, "library.aliasVariable", json!({"library": lid, "alias": "accent", "variable": "brand"})); let modes = Default::default(); - assert!(matches!(s.doc().unwrap().variables.resolve("brand", &modes), Some(VariableValue::Color(c)) if c.to_hex() == "00FF00")); + assert!( + matches!(s.doc().unwrap().variables.resolve(&s.doc().unwrap().variables.get("brand").unwrap().id, &modes), Some(VariableValue::Color(c)) if c.to_hex() == "00FF00") + ); assert!(s.execute("library.aliasVariable", &json!({"library": lid, "alias": "primary", "variable": "gap"})).is_err(), "types differ"); assert!(s.execute("library.aliasVariable", &json!({"library": lid, "alias": "primary", "variable": local})).is_err(), "copies are read-only"); diff --git a/crates/engine/src/variable_props_tests.rs b/crates/engine/src/variable_props_tests.rs index 0d4790d5..2bcb12d5 100644 --- a/crates/engine/src/variable_props_tests.rs +++ b/crates/engine/src/variable_props_tests.rs @@ -373,17 +373,14 @@ fn variable_keys_inside_typed_paint_lists_stay_literal_but_bare_wrappers_error() } #[test] -fn variable_bind_keeps_its_legacy_type_lookup_when_a_name_collides_with_an_id() { +fn variable_bind_type_checks_the_found_variable_when_a_name_collides_with_an_id() { let mut s = Session::new(); run(&mut s, "variable.createCollection", json!({"name": "Theme"})); let number = run(&mut s, "variable.create", json!({"collection": "Theme", "name": "size", "type": "number", "value": 20}))["id"].clone(); let paint = run(&mut s, "variable.create", json!({"collection": "Theme", "name": "surface", "type": "color", "value": "#FF0000"}))["id"].clone(); run(&mut s, "variable.update", json!({"variable": number, "name": paint})); let id = create(&mut s, json!({})); - let before = s.doc().unwrap().clone(); - let depth = s.undo_depth(); - let e = s.execute("variable.bind", &json!({"id": id, "field": "fill", "variable": "surface"})).unwrap_err().to_string(); - assert!(e.contains("`fill` doesn't take a number variable"), "{e}"); - assert_eq!(s.doc().unwrap(), &before); - assert_eq!(s.undo_depth(), depth); + run(&mut s, "variable.bind", json!({"id": id, "field": "fill", "variable": "surface"})); + assert_eq!(s.doc().unwrap().node(id).unwrap().bound.get("fill").map(String::as_str), paint.as_str()); + assert_eq!(color(&s, id, "fill").to_hex(), "FF0000"); } diff --git a/crates/engine/tests/it/main.rs b/crates/engine/tests/it/main.rs index 34a12847..7138d434 100644 --- a/crates/engine/tests/it/main.rs +++ b/crates/engine/tests/it/main.rs @@ -14,3 +14,5 @@ mod scope_reset; mod selection_colors; #[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] mod shape_builder_response; +#[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] +mod variable_binding_ids; diff --git a/crates/engine/tests/it/variable_binding_ids.rs b/crates/engine/tests/it/variable_binding_ids.rs new file mode 100644 index 00000000..cb8b3ab6 --- /dev/null +++ b/crates/engine/tests/it/variable_binding_ids.rs @@ -0,0 +1,32 @@ +use figcraft_engine::Session; +use serde_json::{Value, json}; + +/// Variable A is renamed to variable B's id; returns (session, B's id). +fn renamed_to_other_id(a_type: &str, a_value: Value) -> (Session, String) { + let mut s = Session::new(); + s.execute("variable.createCollection", &json!({"name": "Theme"})).unwrap(); + let a = s.execute("variable.create", &json!({"collection": "Theme", "name": "shadow", "type": a_type, "value": a_value})).unwrap(); + let a = a["id"].as_str().unwrap().to_string(); + let b = s.execute("variable.create", &json!({"collection": "Theme", "name": "surface", "type": "color", "value": "#0D99FF"})).unwrap(); + let b = b["id"].as_str().unwrap().to_string(); + s.execute("variable.update", &json!({"variable": a, "name": b})).unwrap(); + (s, b) +} + +#[test] +fn a_bound_id_wins_over_a_variable_named_like_it() { + let (mut s, b) = renamed_to_other_id("color", json!("#FF0000")); + let r = s.execute("node.create", &json!({"type": "rectangle", "props": {"fill": {"$var": b}}})).unwrap(); + let node = s.execute("node.get", &json!({"id": r["id"]})).unwrap(); + let text = node.to_string(); + assert!(text.contains(&format!("\"boundVariables\":{{\"fill\":\"{b}\"}}")), "{text}"); + assert!(text.contains("#0D99FF"), "{text}"); + assert!(!text.contains("#FF0000"), "{text}"); +} + +#[test] +fn the_type_check_uses_the_bound_id() { + let (mut s, b) = renamed_to_other_id("number", json!(4)); + let r = s.execute("node.create", &json!({"type": "rectangle", "props": {"fill": {"$var": b}}})); + assert!(r.is_ok(), "{r:?}"); +} diff --git a/crates/format/src/penpot.rs b/crates/format/src/penpot.rs index 554c23b1..bc60f809 100644 --- a/crates/format/src/penpot.rs +++ b/crates/format/src/penpot.rs @@ -5955,12 +5955,14 @@ mod tests { assert_eq!(doc.variables.collections.len(), 1); assert_eq!(doc.variables.variables.len(), 5); let values = &doc.variables; - assert!(matches!(values.resolve("Core/color.primary", &BTreeMap::new()), Some(VariableValue::Color(_)))); - assert_eq!(values.resolve("Core/spacing.sm", &BTreeMap::new()), Some(VariableValue::Number(8.0))); - assert!(matches!(values.resolve("Core/copy.cta", &BTreeMap::new()), Some(VariableValue::String(ref s)) if s == "Buy")); - assert_eq!(values.resolve("Core/flags.enabled", &BTreeMap::new()), Some(VariableValue::Boolean(true))); + assert!(matches!(values.resolve(&values.get("Core/color.primary").unwrap().id, &BTreeMap::new()), Some(VariableValue::Color(_)))); + assert_eq!(values.resolve(&values.get("Core/spacing.sm").unwrap().id, &BTreeMap::new()), Some(VariableValue::Number(8.0))); + assert!( + matches!(values.resolve(&values.get("Core/copy.cta").unwrap().id, &BTreeMap::new()), Some(VariableValue::String(ref s)) if s == "Buy") + ); + assert_eq!(values.resolve(&values.get("Core/flags.enabled").unwrap().id, &BTreeMap::new()), Some(VariableValue::Boolean(true))); assert!(warnings.iter().any(|s| s.contains("themes"))); - assert!(matches!(values.resolve("Core/aliases.secondary", &BTreeMap::new()), Some(VariableValue::Color(_)))); + assert!(matches!(values.resolve(&values.get("Core/aliases.secondary").unwrap().id, &BTreeMap::new()), Some(VariableValue::Color(_)))); assert!(!warnings.iter().any(|s| s.contains("unsupported or unresolved"))); let page = doc.node(doc.first_page().unwrap()).unwrap(); assert_eq!( @@ -6118,9 +6120,9 @@ mod tests { assert_eq!(doc.variables.variables.len(), 7); let v = &doc.variables; assert!(matches!(v.get("Core/palette.middle").unwrap().values.values().next(), Some(VariableValue::Alias(_)))); - assert!(matches!(v.resolve("Core/palette.accent", &BTreeMap::new()), Some(VariableValue::Color(_)))); - assert_eq!(v.resolve("Core/space.copy", &BTreeMap::new()), Some(VariableValue::Number(12.0))); - assert_eq!(v.resolve("Core/labelAlias", &BTreeMap::new()), Some(VariableValue::String("Button".into()))); + assert!(matches!(v.resolve(&v.get("Core/palette.accent").unwrap().id, &BTreeMap::new()), Some(VariableValue::Color(_)))); + assert_eq!(v.resolve(&v.get("Core/space.copy").unwrap().id, &BTreeMap::new()), Some(VariableValue::Number(12.0))); + assert_eq!(v.resolve(&v.get("Core/labelAlias").unwrap().id, &BTreeMap::new()), Some(VariableValue::String("Button".into()))); assert!(!warnings.iter().any(|w| w.contains("unsupported or unresolved")), "{warnings:?}"); let saved = crate::save_bytes(&doc, None).unwrap(); let (reloaded, issues) = crate::load_bytes(&saved).unwrap();