Merge pull request #1542 from Silvertree2010/fix/1513-bug-report-figcraft-a-variable-n

Resolve stored variable ids by exact id, not by variable name

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<CollectionId, ModeId>) -> Option<VariableValue> {
         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())? {

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");

crates/engine/src/cmd/styles.rs

@@ -784,7 +784,7 @@ pub fn bind(s: &mut Session, p: &Value) -> Result<Value> {
 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))?,

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");
 

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");
 }

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;

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:?}");
+}

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();