Merge pull request #1138 from davep/no-self-own

Raise an error if a widget tries to be its own parent

Author: davep

src/textual/widget.py

@@ -147,6 +147,14 @@ class RenderCache(NamedTuple):
     lines: Lines
 
 
+class WidgetError(Exception):
+    """Base widget error."""
+
+
+class MountError(WidgetError):
+    """Error raised when there was a problem with the mount request."""
+
+
 @rich.repr.auto
 class Widget(DOMNode):
     """
@@ -233,6 +241,10 @@ def __init__(
             id=id,
             classes=self.DEFAULT_CLASSES if classes is None else classes,
         )
+
+        if self in children:
+            raise WidgetError("A widget can't be its own parent")
+
         self._add_children(*children)
 
     virtual_size = Reactive(Size(0, 0), layout=True)
@@ -374,9 +386,6 @@ def _get_virtual_dom(self) -> Iterable[Widget]:
         if self._scrollbar_corner is not None:
             yield self._scrollbar_corner
 
-    class MountError(Exception):
-        """Error raised when there was a problem with the mount request."""
-
     def _find_mount_point(self, spot: int | str | "Widget") -> tuple["Widget", int]:
         """Attempt to locate the point where the caller wants to mount something.
 
@@ -387,7 +396,7 @@ def _find_mount_point(self, spot: int | str | "Widget") -> tuple["Widget", int]:
             tuple[Widget, int]: The parent and the location in its child list.
 
         Raises:
-            Widget.MountError: If there was an error finding where to mount a widget.
+            MountError: If there was an error finding where to mount a widget.
 
         The rules of this method are:
 
@@ -414,7 +423,7 @@ def _find_mount_point(self, spot: int | str | "Widget") -> tuple["Widget", int]:
         # have a parent? There's no way we can use it as a sibling to make
         # mounting decisions if it doesn't have a parent.
         if spot.parent is None:
-            raise self.MountError(
+            raise MountError(
                 f"Unable to find relative location of {spot!r} because it has no parent"
             )
 
@@ -424,7 +433,7 @@ def _find_mount_point(self, spot: int | str | "Widget") -> tuple["Widget", int]:
         try:
             return spot.parent, spot.parent.children.index(spot)
         except ValueError:
-            raise self.MountError(f"{spot!r} is not a child of {self!r}") from None
+            raise MountError(f"{spot!r} is not a child of {self!r}") from None
 
     def mount(
         self,
@@ -452,7 +461,7 @@ def mount(
 
         # Saying you want to mount before *and* after something is an error.
         if before is not None and after is not None:
-            raise self.MountError(
+            raise MountError(
                 "Only one of `before` or `after` can be handled -- not both"
             )
 

tests/test_widget_mount_point.py

@@ -1,6 +1,6 @@
 import pytest
 
-from textual.widget import Widget
+from textual.widget import Widget, MountError
 
 
 class Content(Widget):
@@ -36,5 +36,5 @@ def test_find_dom_spot():
     # Finally, let's be sure that we get an error if, for some odd reason,
     # we go looking for a widget that isn't actually part of the DOM we're
     # looking in.
-    with pytest.raises(Widget.MountError):
+    with pytest.raises(MountError):
         _ = screen._find_mount_point(Widget())

tests/test_widget_mounting.py

@@ -1,15 +1,25 @@
 import pytest
 
 from textual.app import App
+from textual.widget import Widget, WidgetError, MountError
 from textual.widgets import Static
 from textual.css.query import TooManyMatches
 
+class SelfOwn(Widget):
+    """Test a widget that tries to own itself."""
+    def __init__(self) -> None:
+        super().__init__(self)
+
 async def test_mount_via_app() -> None:
     """Perform mount tests via the app."""
 
     # Make a background set of widgets.
     widgets = [Static(id=f"starter-{n}") for n in range( 10 )]
 
+    async with App().run_test() as pilot:
+        with pytest.raises(WidgetError):
+            await pilot.app.mount(SelfOwn())
+
     async with App().run_test() as pilot:
         # Mount the first one and make sure it's there.
         await pilot.app.mount(widgets[0])
@@ -83,16 +93,16 @@ async def test_mount_via_app() -> None:
     async with App().run_test() as pilot:
         # Make sure we get told off for trying to before and after.
         await pilot.app.mount_all(widgets)
-        with pytest.raises(Static.MountError):
+        with pytest.raises(MountError):
             await pilot.app.mount(Static(), before=2, after=2)
 
     async with App().run_test() as pilot:
         # Make sure we get told off trying to mount relative to something
         # that isn't actually in the DOM.
         await pilot.app.mount_all(widgets)
-        with pytest.raises(Static.MountError):
+        with pytest.raises(MountError):
             await pilot.app.mount(Static(), before=Static())
-        with pytest.raises(Static.MountError):
+        with pytest.raises(MountError):
             await pilot.app.mount(Static(), after=Static())
 
     async with App().run_test() as pilot: