Fix global changed handler inlining

internal/compiler/passes/remove_aliases.rs

@@ -12,31 +12,55 @@ use std::collections::{HashMap, HashSet, btree_map::Entry};
 use std::rc::Rc;
 
 // The property in the key is to be removed, and replaced by the property in the value
-type Mapping = HashMap<NamedReference, NamedReference>;
+type Mapping = HashMap<NamedReference, (NamedReference, PropertySet)>;
+type PropertySet = Rc<RefCell<HashSet<NamedReference>>>;
 
 #[derive(Default, Debug)]
 struct PropertySets {
     map: HashMap<NamedReference, Rc<RefCell<HashSet<NamedReference>>>>,
-    all_sets: Vec<Rc<RefCell<HashSet<NamedReference>>>>,
+    all_sets: Vec<PropertySet>,
 }
 
 impl PropertySets {
     fn add_link(&mut self, p1: NamedReference, p2: NamedReference) {
-        let (e1, e2) = (p1.element(), p2.element());
-        let same_component = std::rc::Weak::ptr_eq(
-            &e1.borrow().enclosing_component,
-            &e2.borrow().enclosing_component,
+        let mut members = vec![p1.clone(), p2.clone()];
+        for p in [&p1, &p2] {
+            if let Some(s) = self.map.get(p) {
+                members.extend(s.borrow().iter().cloned());
+            }
+        }
+        let initial_component = members[0].element().borrow().enclosing_component.clone();
+        let link_is_same_component = std::rc::Weak::ptr_eq(
+            &p1.element().borrow().enclosing_component,
+            &p2.element().borrow().enclosing_component,
         );
-        if !same_component {
-            let can_merge_across_components =
-                (e1.borrow().enclosing_component.upgrade().unwrap().is_global()
-                    && !e2.borrow().change_callbacks.contains_key(p2.name()))
-                    || (e2.borrow().enclosing_component.upgrade().unwrap().is_global()
-                        && !e1.borrow().change_callbacks.contains_key(p1.name()));
+        let contains_changed_handlers = members.iter().any(|property| {
+            property.element().borrow().change_callbacks.contains_key(property.name())
+        });
+        let set_is_same_component = members.iter().all(|property| {
+            std::rc::Weak::ptr_eq(
+                &initial_component,
+                &property.element().borrow().enclosing_component,
+            )
+        });
+        let link_involves_global = [&p1, &p2].iter().any(|property| {
+            property.element().borrow().enclosing_component.upgrade().unwrap().is_global()
+        });
+
+        if !link_is_same_component {
+            // We can only add a new link across components if the link involves a global and none
+            // of the involved bindings (including earlier aliases) have a changed handler.
+            let can_merge_across_components = !contains_changed_handlers && link_involves_global;
             if !can_merge_across_components {
-                // We can only merge aliases if they are in the same Component. (unless one of them is global if the other one don't have change event)
-                // TODO: actually we could still merge two alias in a component pointing to the same
-                // property in a parent component
+                return;
+            }
+        } else {
+            // the new link is within the same component,
+            // but the previously processed aliases may already contain another component (which must be a
+            // global due to the `if` path.
+            // If that is the case, only alias if no changed handlers are involved.
+            let can_merge_within_component = !contains_changed_handlers || set_is_same_component;
+            if !can_merge_within_component {
                 return;
             }
         }
@@ -105,8 +129,8 @@ pub fn remove_aliases(doc: &Document, diag: &mut BuildDiagnostics) {
 
     // For each set, find a "master" property. Only reference to this master property will be kept,
     // and only the master property will keep its binding
-    for set in property_sets.all_sets {
-        let set = set.borrow();
+    for set_rc in property_sets.all_sets {
+        let set = set_rc.borrow();
 
         // Globals are singletons, so a callback aliased across globals must have at most
         // one implementation. More than one handler in the set is an ambiguous conflict.
@@ -150,7 +174,7 @@ pub fn remove_aliases(doc: &Document, diag: &mut BuildDiagnostics) {
             }
             for x in set.iter() {
                 if *x != best {
-                    aliases_to_remove.insert(x.clone(), best.clone());
+                    aliases_to_remove.insert(x.clone(), (best.clone(), Rc::clone(&set_rc)));
                 }
             }
         }
@@ -159,14 +183,14 @@ pub fn remove_aliases(doc: &Document, diag: &mut BuildDiagnostics) {
     doc.visit_all_used_components(|component| {
         // Do the replacements
         visit_all_named_references(component, &mut |nr: &mut NamedReference| {
-            if let Some(new) = aliases_to_remove.get(nr) {
+            if let Some((new, _set)) = aliases_to_remove.get(nr) {
                 *nr = new.clone();
             }
         })
     });
 
     // Remove the properties
-    for (remove, to) in aliases_to_remove {
+    for (remove, (to, set)) in aliases_to_remove {
         let elem = remove.element();
         let to_elem = to.element();
 
@@ -186,6 +210,52 @@ pub fn remove_aliases(doc: &Document, diag: &mut BuildDiagnostics) {
 
         remove_from_binding_expression(&mut old_binding, &to);
 
+        // When the master `to` is a global, re-home two-way bindings whose target is *not* in this
+        // set onto the surviving target instead of merging them onto `to`.
+        //
+        // A two-way binding to a property outside this set only survives here because `add_link`
+        // *rejected* merging it into the set (e.g. it would have pulled a `changed` handler across
+        // into a global). The two endpoints are still meant to be linked at runtime, but the rejected
+        // target was never folded away, so we must not let `old_binding` carry that reference onto `to`
+        // via the `merge_with` below: that would leave the global singleton holding a reference to an
+        // instance element it cannot resolve ("accessing deleted parent" at runtime).
+        //
+        // Instead, re-express the link from the surviving target's side as `target <=> to`. This is
+        // only sound because `to` is a global: at runtime an element-to-global reference is resolved by
+        // a direct global lookup (`enclosing_component_instance_for_element`), so `target` can always
+        // resolve `to` regardless of where `target` lives in the tree.
+        //
+        // The reverse is *not* true, which is why this is gated on `to` being a global: when `to` is an
+        // instance, references to it are resolved by walking *up* the parent chain from the hosting
+        // element (`enclosing_component_for_element`). The normal `merge_with` keeps the binding on `to`
+        // and references `target` -- the same direction the original `remove <=> target` binding used,
+        // so it is known to resolve. Flipping it to host on `target` and reference the instance `to`
+        // would require `target`'s context to reach `to`, which is not guaranteed (e.g. `to` lives in a
+        // nested sub-component) and panics with the same "accessing deleted parent" -- as observed when
+        // this guard is dropped (e.g. the `todo` demo).
+        if to_elem.borrow().enclosing_component.upgrade().unwrap().is_global() {
+            old_binding.two_way_bindings.retain(|twb| {
+                let TwoWayBinding::Property { property, field_access } = twb else { return true };
+                if !field_access.is_empty() || set.borrow().contains(property) {
+                    // Field-access bindings aren't aliased (see above), and a target that *is* in
+                    // the set was folded into `to` already, so the normal merge below handles it.
+                    return true;
+                }
+                let target_elem = property.element();
+                let mut target_elem = target_elem.borrow_mut();
+                let entry = target_elem
+                    .bindings
+                    .entry(property.name().clone())
+                    .or_insert_with(|| BindingExpression::new_two_way(to.clone().into()).into());
+                let mut b = entry.borrow_mut();
+                if !b.two_way_bindings.iter().any(|x| x.property() == Some(&to)) {
+                    b.two_way_bindings.push(to.clone().into());
+                }
+                // drop from old_binding so the merge below won't carry it onto the global
+                false
+            });
+        }
+
         let same_component = std::rc::Weak::ptr_eq(
             &elem.borrow().enclosing_component,
             &to_elem.borrow().enclosing_component,

tests/cases/bindings/global_two_way_binding_changed_handler_inlining.slint

@@ -0,0 +1,48 @@
+// Copyright © SixtyFPS GmbH <info@slint.dev>
+// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-2.0 OR LicenseRef-Slint-Software-3.0
+
+import { ComboBox } from "std-widgets.slint";
+
+global TestGlobal {
+    in-out property <int> current-selection: 2;
+}
+
+// Originally, we reproduced the test with a combobox,
+// a combo-box was causing the regression, as it contains a changed handler on the current index.
+// Create a basic reproducer here so that internal changes to the combobox don't make the test invalid.
+export component TestCombobox {
+    in-out property <int> current-index;
+
+    in-out property <[string]> model;
+
+    changed current-index => {
+        debug(model[current-index]);
+    }
+}
+
+export component TestView {
+
+    in-out property <int> current-selection <=> combo-box.current-index;
+
+    // The ComboBox current-index has a changed handler which
+    // tries to access `root`.
+    // If the current-index is inlined into the TestGlobal, the changed callback
+    // will fail in the interpreter, as it cannot access `root`.
+    combo-box := TestCombobox {
+        model: ["1", "2", "3"];
+    }
+}
+
+export component Test {
+    view := TestView {
+        current-selection <=> TestGlobal.current-selection;
+    }
+
+    init => {
+        TestGlobal.current-selection = 1;
+    }
+
+    out property <bool> test: view.current-selection == 1;
+}
+
+