Skip to content

Commit

Permalink
Revert "compiler: inline components that are used only once" (#7697)
Browse files Browse the repository at this point in the history
This reverts commit bf716ff.
(And also revert the test part of 259756c)

This exposed the bug #7693 and more issues mentined in
#7693 (comment)

Fixes #7693
CC #7680
  • Loading branch information
ogoffart authored Feb 21, 2025
1 parent 34b115b commit 1a2aff8
Show file tree
Hide file tree
Showing 6 changed files with 9 additions and 46 deletions.
42 changes: 3 additions & 39 deletions internal/compiler/passes/inlining.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,14 @@ pub fn inline(doc: &Document, inline_selection: InlineSelection, diag: &mut Buil
fn inline_components_recursively(
component: &Rc<Component>,
roots: &HashSet<ByAddress<Rc<Component>>>,
used_once: &HashSet<ByAddress<Rc<Component>>>,
inline_selection: InlineSelection,
diag: &mut BuildDiagnostics,
) {
recurse_elem_no_borrow(&component.root_element, &(), &mut |elem, _| {
let base = elem.borrow().base_type.clone();
if let ElementType::Component(c) = base {
// First, make sure that the component itself is properly inlined
inline_components_recursively(&c, roots, used_once, inline_selection, diag);
inline_components_recursively(&c, roots, inline_selection, diag);

if c.parent_element.upgrade().is_some() {
// We should not inline a repeated element
Expand All @@ -49,27 +48,24 @@ pub fn inline(doc: &Document, inline_selection: InlineSelection, diag: &mut Buil
|| component.parent_element.upgrade().is_none() && Rc::ptr_eq(elem, &component.root_element)
// We always inline other roots as a component can't be both a sub component and a root
|| roots.contains(&ByAddress(c.clone()))
|| used_once.contains(&ByAddress(c.clone()))
}
} {
inline_element(elem, &c, component, diag);
}
}
});
component.popup_windows.borrow().iter().for_each(|p| {
inline_components_recursively(&p.component, roots, used_once, inline_selection, diag)
inline_components_recursively(&p.component, roots, inline_selection, diag)
})
}
let mut roots = HashSet::new();
let mut used_once = HashSet::new();
if inline_selection == InlineSelection::InlineOnlyRequiredComponents {
for component in doc.exported_roots().chain(doc.popup_menu_impl.iter().cloned()) {
roots.insert(ByAddress(component.clone()));
}
used_once = collect_subcomponents_used_once(doc);
}
for component in doc.exported_roots().chain(doc.popup_menu_impl.iter().cloned()) {
inline_components_recursively(&component, &roots, &used_once, inline_selection, diag);
inline_components_recursively(&component, &roots, inline_selection, diag);
let mut init_code = component.init_code.borrow_mut();
let inlined_init_code = core::mem::take(&mut init_code.inlined_init_code);
init_code.constructor_code.splice(0..0, inlined_init_code.into_values());
Expand Down Expand Up @@ -655,35 +651,3 @@ fn element_require_inlining(elem: &ElementRc) -> bool {

false
}

fn collect_subcomponents_used_once(doc: &Document) -> HashSet<ByAddress<Rc<Component>>> {
fn recursive(component: &Rc<Component>, hash: &mut HashMap<ByAddress<Rc<Component>>, bool>) {
match hash.entry(ByAddress(component.clone())) {
std::collections::hash_map::Entry::Occupied(mut o) => {
if !*o.get() {
// We have already set this element (and its children to "set multiple times"
return;
}
// Element used multiple times, visit all children to set them to multiple times too.
*o.get_mut() = false;
}
std::collections::hash_map::Entry::Vacant(v) => {
v.insert(true);
}
}

recurse_elem(&component.root_element, &(), &mut |elem: &ElementRc, &()| {
let ElementType::Component(base_comp) = &elem.borrow().base_type else { return };
recursive(base_comp, hash);
});
for popup in component.popup_windows.borrow().iter() {
recursive(&popup.component, hash);
}
}
// Stores true for elements that are used once, false for elements that are used multiple times;
let mut result = HashMap::new();
for component in doc.exported_roots().chain(doc.popup_menu_impl.iter().cloned()) {
recursive(&component, &mut result);
}
result.into_iter().filter(|(_, v)| *v).map(|(k, _)| k).collect()
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-2.0 OR LicenseRef-Slint-Software-3.0

TC := Rectangle {
// ^error{The binding for the property 'layoutinfo-h' is part of a binding loop}
outer := VerticalLayout {
// ^error{The binding for the property 'width' is part of a binding loop}
// ^^error{The binding for the property 'layoutinfo-h' is part of a binding loop}
Expand Down Expand Up @@ -45,7 +46,6 @@ export Test := Rectangle {

tc := TC {
// ^error{The binding for the property 'preferred-width' is part of a binding loop}
// ^^error{The binding for the property 'layoutinfo-h' is part of a binding loop}
width: preferred-width;
// ^error{The binding for the property 'width' is part of a binding loop}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-2.0 OR LicenseRef-Slint-Software-3.0

Compo := Rectangle {
// ^error{The binding for the property 'layoutinfo-h' is part of a binding loop}

property <string> the_text;

Expand All @@ -24,7 +25,6 @@ Compo := Rectangle {
export Foo := Rectangle {
Compo {
// ^error{The binding for the property 'preferred-width' is part of a binding loop}
// ^^error{The binding for the property 'layoutinfo-h' is part of a binding loop}
the_text: self.preferred-width / 1px;
// ^error{The binding for the property 'the-text' is part of a binding loop}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ component Foo {
}

component Bar {
// ^error{The binding for the property 'layoutinfo-h' is part of a binding loop}
HorizontalLayout {
// ^error{The binding for the property 'layout-cache' is part of a binding loop}
// ^^error{The binding for the property 'width' is part of a binding loop}
Expand All @@ -39,6 +40,4 @@ export component Apps inherits Window {
Bar {}
// ^error{The binding for the property 'preferred-width' is part of a binding loop}
// ^^error{The binding for the property 'width' is part of a binding loop}
// ^^^error{The binding for the property 'layoutinfo-h' is part of a binding loop}
// ^^^^error{The binding for the property 'min-width' is part of a binding loop}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ component Wrapper {
}

component WrapperInherited inherits Rectangle {
// ^error{The binding for the property 'layoutinfo-h' is part of a binding loop}
VerticalLayout {
// ^error{The binding for the property 'layoutinfo-h' is part of a binding loop}
if root.width > 200px: Rectangle { }
Expand All @@ -29,9 +30,8 @@ export component Test inherits Window {
// ^^error{The binding for the property 'width' is part of a binding loop}
// ^^^error{The binding for the property 'layout-cache' is part of a binding loop}
WrapperInherited { }
// ^error{The binding for the property 'layoutinfo-h' is part of a binding loop}
Wrapper { }
// ^error{The binding for the property 'layoutinfo-h' is part of a binding loop}
}
}
}
}
2 changes: 1 addition & 1 deletion internal/compiler/tests/syntax/basic/dialog.slint
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ MyDiag1 := Dialog {
}

MyDiag2 := Dialog {
// ^error{A Dialog must have a single child element that is not StandardButton}
StandardButton { kind: reset; }
StandardButton {
kind: cancel;
Expand Down Expand Up @@ -54,7 +55,6 @@ MyDialog4 := Dialog {
export Test := Rectangle {
MyDiag1 {}
MyDiag2 {}
// ^error{A Dialog must have a single child element that is not StandardButton}
MyDiag3 {}
MyDialog4 {}
}

0 comments on commit 1a2aff8

Please sign in to comment.