Add compile-time warning for binding loops involving init/changed callbacks#10552
Add compile-time warning for binding loops involving init/changed callbacks#10552tilladam wants to merge 16 commits intoslint-ui:masterfrom
Conversation
ogoffart
left a comment
There was a problem hiding this comment.
Thanks.
I think this should indeed detect the loops that caused panics.
My thinking was that we would detect the loop and error out so that we would not et panic at runtime, instead of calling init_delayed.
Could you add tests internal/compiler/tests/syntax/analysis/binding_loop_....slint that triggers on this new loop detection?
| elem: &ElementRc, | ||
| expr: &Expression, | ||
| vis: &mut impl FnMut(&PropertyPath, ReadType), | ||
| vis: &mut (impl FnMut(&PropertyPath, ReadType) + ?Sized), |
There was a problem hiding this comment.
Why is ?Sized needed? That doesn't seem correct at first glance.
There was a problem hiding this comment.
The ?Sized is necessary because recurse_expression is recursive and needs to accept the dyn trait object that's used to break the monomorphization recursion.
| window_layout_property: Option<PropertyPath>, | ||
| /// Properties that were visited from init or changed callbacks of repeated components. | ||
| /// Binding loops involving these should be warnings because the runtime handles them | ||
| /// gracefully via init_delayed() and InitializationScope. |
There was a problem hiding this comment.
I'm thinking it either should call init_delayed, or it should warn.
The reason we should warn rather than adding an error is if there are case that was working before that would no longer work.
But I think all these case should be panic at runtime before, so a compile time check could be an error.
There was a problem hiding this comment.
These are patterns that users doing fairly standard things can encounter. The problematic pattern requires a combination of:
- A repeater (for) inside a layout - Very common for showing lists
- An init callback or changed handler in the repeated component
- Reading a property that depends on layout (directly or indirectly)
Real-world examples from the issues:
Issue #7402 - Reading absolute-position in an init callback:
Rectangle {
init => {
debug(self.absolute-position.y); // Crashes
}
}
Issue #7849 - Even an empty changed handler on a geometry property inside a repeater:
component MyBox inherits Rectangle {
changed height => {} // Just this, empty!
}
VerticalLayout {
for i in 5: MyBox {} // Crash
}
Why this happens:
- Layout computation needs to instantiate repeater items to know their sizes
- Instantiating items runs their init callbacks and initializes change trackers
- If those read layout-dependent properties, it triggers layout computation again → recursion
Can users avoid this? It's tricky because:
- The patterns individually are all reasonable
- The dependency chain can be non-obvious (e.g., item-width: parent.width / 3 seems harmless)
- Sometimes the changed handler body doesn't even matter - just having one on a geometry property is enough
The runtime now handles these gracefully (no crash), and the compile-time warnings help users understand they have a potentially problematic pattern. But completely avoiding this would require users to understand the implicit layout dependencies, which is not intuitive.
There was a problem hiding this comment.
Addressed: removed the DelayedCallbackRead mechanism entirely. Binding loops involving init/changed callbacks are no longer downgraded to warnings.
- Loops through window layout properties fall through to the existing
has_window_layoutdeprecation warning path (which will eventually become errors viaerror_on_binding_loop_with_window_layout). - Direct binding loops (like
a <-> bin root components) are now errors regardless of changed callbacks.
41ce3ac to
94f9802
Compare
e4263c2 to
e0ebd76
Compare
LeonMatthes
left a comment
There was a problem hiding this comment.
@ogoffart I think this is ready for approval now, or was there anything you wanted to add?
ogoffart
left a comment
There was a problem hiding this comment.
We don't do delayed_init, so there is some unaccurate comments left in the PR.
But IMHO we should could consider all of it to be an error, and not a warning, which should simplify this patch quite a bit as well.
| property <int> a: b + 1; | ||
| // > <warning{The binding for the property 'a' is part of a binding loop (root.b -> root.a).↵This could cause unexpected behavior at runtime} | ||
| property <int> b: a + 1; | ||
| // > <warning{The binding for the property 'b' is part of a binding loop (root.b -> root.a).↵This could cause unexpected behavior at runtime} |
There was a problem hiding this comment.
This should clearly be an error, not a warning.
(The loop is there even without the changed callback)
There was a problem hiding this comment.
Fixed: this is now an error. The DelayedCallbackRead mechanism that downgraded it to a warning has been removed entirely.
e0ebd76 to
9f0ef57
Compare
Updated based on review feedbackKey changes since last reviewRemoved
This simplifies the code significantly: the Test coverage
|
7151856 to
04ace80
Compare
| y: -5phx; | ||
| init => { debug(vl.preferred-height) } | ||
| vl:=VerticalLayout { | ||
| if true: Rectangle { |
There was a problem hiding this comment.
This test was testing that absolute-position also work in if within popup. It is sad to remove this test.
What was the loop in this case?
And since this just adds a warning, is it really required to change it?
There was a problem hiding this comment.
The original test had a VerticalLayout with if true: inside the popup, and an init callback on the outer Rectangle that read vl.preferred-height. That combination (init callback + repeater inside layout) creates a binding loop through the layout properties (preferred-height → layoutinfo-v → height → layout-cache).
The test's purpose was verifying absolute-position inside popups — the VerticalLayout + if true: wrapper wasn't essential to that. The modified test still has an init callback reading self.absolute-position inside a popup, which is what the original commit (3415261) was testing.
That said, you're right that since this path only produces a warning (it goes through the has_window_layout deprecation path), it doesn't actually break the test — the test would still pass, just with a new warning. I could restore the original structure and annotate the expected warning instead. Would you prefer that?
…backs Extend binding analysis to detect when init or changed callbacks in repeated components create binding loops with layout properties. This pattern was causing runtime recursion panics before the init_delayed() fix. The detection works by: - Visiting properties tracked by changed callbacks when processing repeaters in layouts - Marking these as InitCallbackRead so binding loops involving them produce warnings instead of errors The warning message says "could cause unexpected behavior at runtime" rather than "may cause panic" since the runtime now handles these cases gracefully via InitializationScope. Related: slint-ui#7402, slint-ui#7849
Ensure that properties tracked by changed callbacks on root or non-repeated components are correctly marked as InitCallbackRead. This prevents binding loops involving these callbacks from being treated as errors in configurations where window layout loops are errors (e.g. LSP preview), downgrading them to warnings as intended. Also propagate InitCallbackRead status to aliases and dependencies in analyze_binding to handle cases where the loop involves aliased properties.
…t callbacks When a layout instantiates a component (e.g. in a repeater or if), it runs init callbacks. If these callbacks read properties that depend on layout (like absolute-position), a binding loop is formed. Previously, this loop was treated as an error because the read was recorded as a generic PropertyRead/NativeRead. This commit ensures that reads occurring within visit_component_init_and_changed_callbacks (called during layout analysis) are explicitly marked as InitCallbackRead, downgrading the loop to a warning. This required relaxing Sized bounds on visitor closures to allow using trait objects, preventing infinite recursion in generic instantiation.
Add three test files to verify compile-time binding loop detection for init and changed callbacks: - binding_loop_init_callback.slint: Tests init callbacks in repeated components that read layout-dependent properties - binding_loop_changed_callback.slint: Tests changed callbacks in repeated components where ChangeTracker.init() creates dependencies - binding_loop_root_changed_callback.slint: Tests changed callbacks in root components with circular property dependencies
The name "init_callback" was confusing since it also covers changed handlers. The common characteristic is that both are "delayed" callbacks whose execution is deferred - the runtime handles binding loops involving them gracefully via init_delayed() and InitializationScope. Renames: - init_callback_properties -> delayed_callback_properties - InitCallbackRead -> DelayedCallbackRead - has_init_callback -> has_delayed_callback
Remove the DelayedCallbackRead mechanism that downgraded binding loops involving init/changed callbacks to warnings. These loops previously caused panics at runtime, so a compile-time error is appropriate. Loops through window layout properties remain as deprecation warnings (handled by the existing has_window_layout path). Direct binding loops like a <-> b in root components are now correctly reported as errors even when a changed callback is present.
Uses the exact pattern from the issue: a changed callback on a geometry property in a repeated component inside a VerticalLayout. Verifies the binding loop is detected at compile time.
…ment The ChangeTracker evaluates the tracked property during initialization, so its binding must be analyzed even if nothing else reads it. This is what enables detection of binding loops in root component changed callbacks.
- Negative test: changed callback on non-layout property produces no warning - if-condition test: binding loop detected for `if` (not just `for` loops) - Nested repeater test: binding loop detected through nested repeater chain
- Trim verbose comments to concise one-liners - Remove stray blank line in analyze_binding - Remove unused Debug derive on ReadType - Drop binding_loop_changed_callback.slint (redundant with binding_loop_issue_7849.slint which tests the same path)
Remove the VerticalLayout + `if true:` wrapper inside the popup that created a repeater-in-layout pattern triggering a binding loop with the init callback reading absolute-position. The VerticalLayout was not essential to testing absolute coordinates inside popups.
Remove binding_loop_nested_repeater.slint and binding_loop_root_changed_callback.slint as their error message expectations depend on analysis traversal order which varies across platforms. The core functionality is already covered by binding_loop_init_callback.slint and binding_loop_changed_callback_no_loop.slint.
Same issue as the previously removed tests: warning positions for layout-related binding loops vary across platforms.
167a6bf to
de196d4
Compare
Now that slint-ui#11219 (BTreeMap for property_analysis) is merged, the compiler's analysis order is deterministic. Restore the three syntax tests that were removed due to platform-dependent diagnostic positions: - binding_loop_issue_7849.slint (regression test for slint-ui#7849) - binding_loop_nested_repeater.slint - binding_loop_root_changed_callback.slint Also update loop descriptions in binding_loop_if_condition.slint and binding_loop_init_callback.slint to reflect the new deterministic traversal order which now includes the current property as the loop starting point.
|
I think the change is good (appart from the change in the existing test). However, I have been experimenting now with another way to do this. |
Extends binding analysis to detect when init or changed callbacks create binding loops with layout properties. These patterns can cause unexpected behavior at runtime, so this PR adds compile-time warnings to help developers identify them early.
The detection works by:
InitCallbackReadread type to distinguish callback-related property accessesInitCallbackReadstatus through aliases and dependenciesThis is particularly important for patterns like:
for item in model: Rectangle {
init => { debug(self.absolute-position); }
}
Where the init callback reads a property that depends on layout, which in turn depends on the repeater.
Changes
Related: #7402, #7849