From dddfcf572d06425dc875579edc0c951902f9f8d5 Mon Sep 17 00:00:00 2001
From: Olivier Goffart <olivier.goffart@slint-ui.com>
Date: Mon, 6 Feb 2023 17:46:27 +0100
Subject: [PATCH] Fix const detection with two ways binding

The const detection for two way binding was not detecting change if one
of the property was set to a const value in a component using it.

This would cause the compiler to generate call set_content on one of the
property in a two way bindings, and later, the "const sentinel" be
present in the dependency list, causing crash.

To avoid segfault for similar bug in the future, added added an assert!
in the property system to detect that.

Fixes #2185
---
 internal/compiler/passes/binding_analysis.rs  |  2 +-
 internal/core/properties.rs                   | 25 ++++++--
 ...e_sub_property_indirection_issue2185.slint | 59 +++++++++++++++++++
 3 files changed, 81 insertions(+), 5 deletions(-)
 create mode 100644 tests/cases/bindings/change_sub_property_indirection_issue2185.slint

diff --git a/internal/compiler/passes/binding_analysis.rs b/internal/compiler/passes/binding_analysis.rs
index 2cedf70fb8c..c163c705c09 100644
--- a/internal/compiler/passes/binding_analysis.rs
+++ b/internal/compiler/passes/binding_analysis.rs
@@ -524,8 +524,8 @@ fn propagate_is_set_on_aliases(component: &Rc<Component>, reverse_aliases: &mut
     }
 
     fn mark_alias(alias: &NamedReference) {
+        alias.mark_as_set();
         if !alias.is_externally_modified() {
-            alias.mark_as_set();
             if let Some(bind) = alias.element().borrow().bindings.get(alias.name()) {
                 propagate_alias(&bind.borrow())
             }
diff --git a/internal/core/properties.rs b/internal/core/properties.rs
index b6cffefd65d..db85667f79e 100644
--- a/internal/core/properties.rs
+++ b/internal/core/properties.rs
@@ -493,8 +493,8 @@ impl PropertyHandle {
                     (*binding).dependencies.set(0);
                 } else {
                     DependencyListHead::mem_move(
-                        (&mut (*binding).dependencies) as *mut _ as *mut _,
-                        self.handle.as_ptr() as *mut _,
+                        (*binding).dependencies.as_ptr() as *mut DependencyListHead,
+                        self.handle.as_ptr() as *mut DependencyListHead,
                     );
                 }
                 ((*binding).vtable.drop)(binding);
@@ -540,8 +540,8 @@ impl PropertyHandle {
                 (*binding).dependencies.set(const_sentinel);
             } else {
                 DependencyListHead::mem_move(
-                    self.handle.as_ptr() as *mut _,
-                    (&mut (*binding).dependencies) as *mut _ as *mut _,
+                    self.handle.as_ptr() as *mut DependencyListHead,
+                    (*binding).dependencies.as_ptr() as *mut DependencyListHead,
                 );
             }
         }
@@ -657,10 +657,23 @@ impl Drop for PropertyHandle {
 
 /// Safety: the dependency list must be valid and consistent
 unsafe fn mark_dependencies_dirty(dependencies: *mut DependencyListHead) {
+    debug_assert!(!core::ptr::eq(
+        *(dependencies as *mut *const u32),
+        (&CONSTANT_PROPERTY_SENTINEL) as *const u32,
+    ));
     DependencyListHead::for_each(&*dependencies, |binding| {
         let binding: &BindingHolder = &**binding;
         let was_dirty = binding.dirty.replace(true);
         (binding.vtable.mark_dirty)(binding as *const BindingHolder, was_dirty);
+
+        assert!(
+            !core::ptr::eq(
+                binding.dependencies.as_ptr() as *const u32,
+                (&CONSTANT_PROPERTY_SENTINEL) as *const u32,
+            ),
+            "Const property marked as dirty"
+        );
+
         mark_dependencies_dirty(binding.dependencies.as_ptr() as *mut DependencyListHead)
     });
 }
@@ -1247,6 +1260,10 @@ impl<DirtyHandler: PropertyDirtyHandler> PropertyTracker<DirtyHandler> {
         if CURRENT_BINDING.is_set() {
             CURRENT_BINDING.with(|cur_binding| {
                 if let Some(cur_binding) = cur_binding {
+                    debug_assert!(!core::ptr::eq(
+                        self.holder.dependencies.get() as *const u32,
+                        (&CONSTANT_PROPERTY_SENTINEL) as *const u32,
+                    ));
                     cur_binding.register_self_as_dependency(
                         self.holder.dependencies.as_ptr() as *mut DependencyListHead,
                         #[cfg(slint_debug_property)]
diff --git a/tests/cases/bindings/change_sub_property_indirection_issue2185.slint b/tests/cases/bindings/change_sub_property_indirection_issue2185.slint
new file mode 100644
index 00000000000..1427935c414
--- /dev/null
+++ b/tests/cases/bindings/change_sub_property_indirection_issue2185.slint
@@ -0,0 +1,59 @@
+// Copyright © SixtyFPS GmbH <info@slint-ui.com>
+// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-commercial
+
+component Buggy {
+    in-out property<string> magic: "Nope";
+    property<bool> condition: false;
+
+    public function change_condition() {
+        condition = !condition;
+    }
+
+    HorizontalLayout {
+        if root.condition: TouchArea {
+            property <string> innermagic <=> root.magic;
+            clicked => { self.innermagic = "Hello"; }
+            horizontal-stretch: 1;
+        }
+
+        Rectangle {
+            background: red;
+            TouchArea {
+                clicked => { change-condition() }
+            }
+        }
+    }
+}
+
+
+component TestCase {
+    width: 300px;
+    height: 100px;
+
+    public function change_condition() {
+        b.change-condition();
+    }
+
+    b := Buggy {
+        magic: "Hi";
+    }
+
+    out property <string> magic: b.magic;
+}
+
+
+/*
+
+```rust
+let instance = TestCase::new();
+assert_eq!(instance.get_magic(), slint::SharedString::from("Hi"));
+instance.invoke_change_condition();
+instance.invoke_change_condition();
+instance.invoke_change_condition();
+assert_eq!(instance.get_magic(), slint::SharedString::from("Hi"));
+slint_testing::send_mouse_click(&instance, 10., 10.);
+assert_eq!(instance.get_magic(), slint::SharedString::from("Hello"));
+```
+
+
+*/