Compiler: make it an error to access properties from menus from outside

internal/compiler/object_tree.rs

@@ -944,9 +944,12 @@ pub fn pretty_print(
         }
     }
     for (name, expr) in &e.bindings {
-        let expr = expr.borrow();
         indent!();
         write!(f, "{name}: ")?;
+        let Ok(expr) = expr.try_borrow() else {
+            writeln!(f, "<borrowed>")?;
+            continue;
+        };
         expression_tree::pretty_print(f, &expr.expression)?;
         if expr.analysis.as_ref().is_some_and(|a| a.is_const) {
             write!(f, "/*const*/")?;

internal/compiler/passes/lower_menus.rs

@@ -248,8 +248,6 @@ fn process_context_menu(
             .clone()
             .into();
 
-        context_menu_elem.borrow_mut().base_type = components.context_menu_internal.clone();
-
         let mut menu_elem = None;
         context_menu_elem.borrow_mut().children.retain(|x| {
             if x.borrow().base_type == menu_element_type {
@@ -282,9 +280,10 @@ fn process_context_menu(
         }
 
         let children = std::mem::take(&mut menu_elem.borrow_mut().children);
-        let c = lower_menu_items(context_menu_elem, children, components);
+        let c = lower_menu_items(context_menu_elem, children, components, diag);
         let item_tree_root = Expression::ElementReference(Rc::downgrade(&c.root_element));
 
+        context_menu_elem.borrow_mut().base_type = components.context_menu_internal.clone();
         for (name, _) in &components.context_menu_internal.property_list() {
             if let Some(decl) = context_menu_elem.borrow().property_declarations.get(name) {
                 diag.push_error(format!("Cannot re-define internal property '{name}'"), &decl.node);
@@ -341,9 +340,8 @@ fn process_window(
     no_native_menu: bool,
     diag: &mut BuildDiagnostics,
 ) -> bool {
-    let mut window = win.borrow_mut();
     let mut menu_bar = None;
-    window.children.retain(|x| {
+    win.borrow_mut().children.retain(|x| {
         if matches!(&x.borrow().base_type, ElementType::Builtin(b) if b.name == "MenuBar") {
             if menu_bar.is_some() {
                 diag.push_error("Only one MenuBar is allowed in a Window".into(), &*x.borrow());
@@ -370,7 +368,7 @@ fn process_window(
 
     // Lower MenuItem's into a tree root
     let children = std::mem::take(&mut menu_bar.borrow_mut().children);
-    let c = lower_menu_items(&menu_bar, children, components);
+    let c = lower_menu_items(&menu_bar, children, components, diag);
     let item_tree_root = Expression::ElementReference(Rc::downgrade(&c.root_element));
 
     if !no_native_menu {
@@ -393,6 +391,7 @@ fn process_window(
         };
     }
 
+    let mut window = win.borrow_mut();
     let menubar_impl = Element {
         id: format_smolstr!("{}-menulayout", window.id),
         base_type: components.menubar_impl.clone(),
@@ -517,6 +516,7 @@ fn lower_menu_items(
     parent: &ElementRc,
     children: Vec<ElementRc>,
     components: &UsefulMenuComponents,
+    diag: &mut BuildDiagnostics,
 ) -> Rc<Component> {
     let component = Rc::new_cyclic(|component_weak| {
         let root_element = Rc::new(RefCell::new(Element {
@@ -556,13 +556,17 @@ fn lower_menu_items(
             ..Default::default()
         }
     });
-    parent
-        .borrow()
-        .enclosing_component
-        .upgrade()
-        .unwrap()
-        .menu_item_tree
-        .borrow_mut()
-        .push(component.clone());
+    let enclosing = parent.borrow().enclosing_component.upgrade().unwrap();
+
+    super::lower_popups::check_no_reference_to_popup(
+        parent,
+        &enclosing,
+        &Rc::downgrade(&component),
+        &NamedReference::new(parent, SmolStr::new_static("x")),
+        diag,
+    );
+
+    enclosing.menu_item_tree.borrow_mut().push(component.clone());
+
     component
 }

internal/compiler/passes/lower_popups.rs

@@ -112,10 +112,6 @@ fn lower_popup_window(
         parent_cip.insertion_index -= 1;
     }
 
-    if matches!(popup_window_element.borrow().base_type, ElementType::Builtin(_)) {
-        popup_window_element.borrow_mut().base_type = window_type.clone();
-    }
-
     let map_close_on_click_value = |b: &BindingExpression| {
         let Expression::BoolLiteral(v) = super::ignore_debug_hooks(&b.expression) else {
             assert!(diag.has_errors());
@@ -213,28 +209,11 @@ fn lower_popup_window(
         popup_mut.geometry_props.as_mut().unwrap().y = dummy2;
     }
 
-    // Throw error when accessing the popup from outside
-    // FIXME:
-    // - the span is the span of the PopupWindow, that's wrong, we should have the span of the reference
-    // - There are other object reference than in the NamedReference
-    // - Maybe this should actually be allowed
-    visit_all_named_references(&parent_component, &mut |nr| {
-        let element = &nr.element();
-        if check_element(element, &weak, diag, popup_window_element) {
-            // just set it to whatever is a valid NamedReference, otherwise we'll panic later
-            *nr = coord_x.clone();
-        }
-    });
-    visit_all_expressions(&parent_component, |exp, _| {
-        exp.visit_recursive_mut(&mut |exp| {
-            if let Expression::ElementReference(element) = exp {
-                let elem = element.upgrade().unwrap();
-                if !Rc::ptr_eq(&elem, popup_window_element) {
-                    check_element(&elem, &weak, diag, popup_window_element);
-                }
-            }
-        });
-    });
+    check_no_reference_to_popup(popup_window_element, &parent_component, &weak, &coord_x, diag);
+
+    if matches!(popup_window_element.borrow().base_type, ElementType::Builtin(_)) {
+        popup_window_element.borrow_mut().base_type = window_type.clone();
+    }
 
     super::focus_handling::call_focus_on_init(&popup_comp);
 
@@ -251,15 +230,63 @@ fn report_const_error(prop: &str, span: &Option<SourceLocation>, diag: &mut Buil
     diag.push_error(format!("The {prop} property only supports constants at the moment"), span);
 }
 
+/// Throw error when accessing the popup from outside
+// FIXME:
+// - the span is the span of the PopupWindow, that's wrong, we should have the span of the reference
+// - There are other object reference than in the NamedReference
+// - Maybe this should actually be allowed
+pub fn check_no_reference_to_popup(
+    popup_window_element: &ElementRc,
+    parent_component: &Rc<Component>,
+    new_weak: &Weak<Component>,
+    random_valid_ref: &NamedReference,
+    diag: &mut BuildDiagnostics,
+) {
+    visit_all_named_references(parent_component, &mut |nr| {
+        let element = &nr.element();
+        if check_element(element, new_weak, diag, popup_window_element, nr.name()) {
+            // just set it to whatever is a valid NamedReference, otherwise we'll panic later
+            *nr = random_valid_ref.clone();
+        }
+    });
+    visit_all_expressions(parent_component, |exp, _| {
+        exp.visit_recursive_mut(&mut |exp| {
+            if let Expression::ElementReference(element) = exp {
+                let elem = element.upgrade().unwrap();
+                if !Rc::ptr_eq(&elem, popup_window_element) {
+                    check_element(&elem, new_weak, diag, popup_window_element, &"");
+                }
+            }
+        });
+    });
+}
+
 fn check_element(
     element: &ElementRc,
     popup_comp: &Weak<Component>,
     diag: &mut BuildDiagnostics,
     popup_window_element: &ElementRc,
+    prop_name: &str,
 ) -> bool {
     if Weak::ptr_eq(&element.borrow().enclosing_component, popup_comp) {
+        let element_name = popup_window_element
+            .borrow()
+            .builtin_type()
+            .map(|t| t.name.clone())
+            .unwrap_or_else(|| SmolStr::new_static("PopupWindow"));
+        let id = element.borrow().id.clone();
+        let what = if prop_name.is_empty() {
+            if id.is_empty() { "something".into() } else { format!("element '{id}'") }
+        } else {
+            if id.is_empty() {
+                format!("property or callback '{prop_name}'")
+            } else {
+                format!("property or callback '{id}.{prop_name}'")
+            }
+        };
+
         diag.push_error(
-            "Cannot access the inside of a PopupWindow from enclosing component".into(),
+            format!("Cannot access {what} inside of a {element_name} from enclosing component"),
             &*popup_window_element.borrow(),
         );
         true

internal/compiler/tests/syntax/basic/popup.slint

@@ -9,7 +9,7 @@ component Foo {
 
 component Issue5852 {
     p := PopupWindow {
-//       >          <error{Cannot access the inside of a PopupWindow from enclosing component}
+//       >          <error{Cannot access element 'input' inside of a PopupWindow from enclosing component}
         input := TextInput {}
     }
     TouchArea {
@@ -29,7 +29,7 @@ export X := PopupWindow {
 //  >        <error{Access to property 'inner.opacity' which is inlined into a PopupWindow via @children is forbidden}
 
         popup := PopupWindow {  // FIXME, the error should be located on property access (#4438)
-//               >          <error{Cannot access the inside of a PopupWindow from enclosing component}
+//               >          <error{Cannot access property or callback 'r.background' inside of a PopupWindow from enclosing component}
 
             r := Rectangle {
             }

internal/compiler/tests/syntax/elements/contextmenu3.slint

@@ -0,0 +1,18 @@
+// Copyright © SixtyFPS GmbH <[email protected]>
+// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-2.0 OR LicenseRef-Slint-Software-3.0
+
+export component A  {
+    cb := ContextMenuArea {
+//        >              <error{Cannot access property or callback 'inner-menu.title' inside of a ContextMenuArea from enclosing component}
+        Menu {
+            inner-menu := Menu { }
+        }
+    }
+
+    if true : TouchArea {
+        clicked => {
+            inner-menu.title = "Hi";
+        }
+    }
+}
+

internal/compiler/tests/syntax/elements/menubar4.slint

@@ -0,0 +1,15 @@
+// Copyright © SixtyFPS GmbH <[email protected]>
+// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-2.0 OR LicenseRef-Slint-Software-3.0
+
+export component B inherits Window {
+    callback activate <=> item.activated;
+    MenuBar {
+//  >      <error{Cannot access property or callback 'item.activated' inside of a MenuBar from enclosing component}
+        Menu {
+            title: "hello";
+            item := MenuItem {
+                title: "world";
+            }
+        }
+    }
+}