fix: fix name collisions between dataclass fields and guiclass container attributes (#688)

* fix: fix name collisions on guiclass dataclass

* fix test

* lint

* bump minimum version

* undo private name change because magicclass is accessing it

* use first not last

* remove uvlock

Author: tlambert03

pyproject.toml

@@ -35,7 +35,7 @@ classifiers = [
 dynamic = ["version"]
 dependencies = [
     "docstring_parser>=0.7",
-    "psygnal>=0.6.1",
+    "psygnal>=0.8.0",
     "qtpy>=1.7.0",
     "superqt[iconify]>=0.6.1",
     "typing_extensions>=4.6",

src/magicgui/experimental.py

@@ -4,6 +4,6 @@
 removed in future releases. Use at your own risk.
 """
 
-from .schema._guiclass import button, guiclass, is_guiclass
+from .schema._guiclass import GuiBuilder, button, guiclass, is_guiclass
 
-__all__ = ["button", "guiclass", "is_guiclass"]
+__all__ = ["GuiBuilder", "button", "guiclass", "is_guiclass"]

src/magicgui/schema/_guiclass.py

@@ -14,7 +14,7 @@
 from dataclasses import Field, dataclass, field, is_dataclass
 from typing import TYPE_CHECKING, Any, Callable, ClassVar, TypeVar, overload
 
-from psygnal import SignalGroup, SignalInstance, evented
+from psygnal import SignalGroup, SignalInstance, evented, is_evented
 from psygnal import __version__ as psygnal_version
 
 from magicgui.schema._ui_field import build_widget
@@ -149,13 +149,15 @@ def _deco(cls: T) -> T:
                 "https://github.com/pyapp-kit/magicgui/issues."
             )
 
-        setattr(cls, gui_name, GuiBuilder(gui_name, follow_changes=follow_changes))
-
+        builder = GuiBuilder(
+            gui_name,
+            follow_changes=follow_changes,
+            events_namespace=events_namespace,
+        )
         if not is_dataclass(cls):
             cls = dataclass(cls, **dataclass_kwargs)  # type: ignore
-        cls = evented(cls, events_namespace=events_namespace)
-
-        setattr(cls, _GUICLASS_FLAG, True)
+        setattr(cls, gui_name, builder)
+        builder.__set_name__(cls, gui_name)
         return cls
 
     return _deco(cls) if cls is not None else _deco
@@ -195,19 +197,56 @@ def _deco(func: F) -> F:
 
 
 class GuiBuilder:
-    """Descriptor that builds a widget for a dataclass or instance."""
+    """Descriptor that builds a widget for a dataclass or instance.
+
+    Parameters
+    ----------
+    name : str, optional
+        The name of the property that will return a `magicgui` widget.
+        When used as a descriptor, the name of the class-attribute to which this
+        descriptor is applied will be used.
+    follow_changes : bool, optional
+        If `True` (default), changes to the dataclass instance will be reflected in the
+        gui, and changes to the gui will be reflected in the dataclass instance.
+    events_namespace : str
+        If the owner of this descriptor is not already evented when __set_name__ is
+        called, it will be be made evented and the events namespace will be set to this
+        value. By default, this is "events". (see `psygnal.SignalGroupDescriptor`
+        for details.)
+    """
 
-    def __init__(self, name: str = "", follow_changes: bool = True):
+    def __init__(
+        self,
+        name: str = "",
+        follow_changes: bool = True,
+        events_namespace: str = "events",
+    ):
         self._name = name
         self._follow_changes = follow_changes
+        self._owner: type | None = None
+        self._events_namespace = events_namespace
 
     def __set_name__(self, owner: type, name: str) -> None:
         self._name = name
+        self._owner = owner
+        if not is_evented(owner):
+            evented(owner, events_namespace=self._events_namespace)
+        setattr(owner, _GUICLASS_FLAG, True)
+
+    def widget(self) -> ContainerWidget:
+        """Return a widget for the dataclass or instance."""
+        if self._owner is None:
+            raise TypeError(
+                "Cannot call `widget` on an unbound `GuiBuilder` descriptor."
+            )
+        return build_widget(self._owner)
 
     def __get__(
         self, instance: object | None, owner: type
-    ) -> ContainerWidget[BaseValueWidget]:
-        wdg = build_widget(owner if instance is None else instance)
+    ) -> ContainerWidget[BaseValueWidget] | GuiBuilder:
+        if instance is None:
+            return self
+        wdg = build_widget(instance)
 
         # look for @button-decorated methods
         # TODO: move inside build_widget?

src/magicgui/schema/_ui_field.py

@@ -852,4 +852,5 @@ def _get_values(obj: Any) -> dict | None:
 def build_widget(cls_or_instance: Any) -> ContainerWidget[BaseValueWidget]:
     """Build a magicgui widget from a dataclass, attrs, pydantic, or function."""
     values = None if isinstance(cls_or_instance, type) else _get_values(cls_or_instance)
-    return _uifields_to_container(get_ui_fields(cls_or_instance), values=values)
+    fields = get_ui_fields(cls_or_instance)
+    return _uifields_to_container(fields, values=values)

src/magicgui/widgets/bases/_container_widget.py

@@ -2,6 +2,7 @@
 
 import contextlib
 from collections.abc import Iterable, Mapping, MutableSequence, Sequence
+from itertools import chain
 from typing import (
     TYPE_CHECKING,
     Any,
@@ -22,6 +23,7 @@
 from magicgui.widgets.bases._mixins import _OrientationMixin
 
 from ._button_widget import ButtonWidget
+from ._named_list import NamedList
 from ._value_widget import BaseValueWidget
 from ._widget import Widget
 
@@ -80,7 +82,7 @@ class BaseContainerWidget(Widget, _OrientationMixin, Sequence[WidgetVar]):
     _initialized = False
     # this is janky ... it's here to allow connections during __init__ by
     # avoiding a recursion error in __getattr__
-    _list: list[WidgetVar] = []  # noqa: RUF012
+    _list: NamedList[WidgetVar] = NamedList()
 
     def __init__(
         self,
@@ -91,7 +93,7 @@ def __init__(
         labels: bool = True,
         **base_widget_kwargs: Unpack[WidgetKwargs],
     ) -> None:
-        self._list: list[WidgetVar] = []
+        self._list: NamedList[WidgetVar] = NamedList()
         self._labels = labels
         self._layout = layout
         self._scrollable = scrollable
@@ -110,11 +112,14 @@ def __len__(self) -> int:
 
     def __getattr__(self, name: str) -> WidgetVar:
         """Return attribute ``name``.  Will return a widget if present."""
-        for widget in self._list:
-            if name == widget.name:
-                return widget
+        if (wdg := self._list.get_by_name(name)) is not None:
+            return wdg
         return object.__getattribute__(self, name)  # type: ignore
 
+    def get_widget(self, name: str) -> WidgetVar | None:
+        """Return widget with name `name`, or None if one doesn't exist."""
+        return self._list.get_by_name(name)
+
     @overload
     def __getitem__(self, key: int | str) -> WidgetVar: ...
 
@@ -429,17 +434,15 @@ def asdict(self) -> dict[str, Any]:
 
     def update(
         self,
-        mapping: Mapping | Iterable[tuple[str, Any]] | None = None,
+        mapping: Mapping | Iterable[tuple[str, Any]] = (),
         **kwargs: Any,
     ) -> None:
         """Update the parameters in the widget from a mapping, iterable, or kwargs."""
         with self.changed.blocked():
-            if mapping:
-                items = mapping.items() if isinstance(mapping, Mapping) else mapping
-                for key, value in items:
-                    getattr(self, key).value = value
-            for key, value in kwargs.items():
-                getattr(self, key).value = value
+            items = mapping.items() if isinstance(mapping, Mapping) else mapping
+            for key, value in chain(items, kwargs.items()):
+                if isinstance(wdg := self._list.get_by_name(key), BaseValueWidget):
+                    wdg.value = value
         self.changed.emit(self)
 
     @debounce

src/magicgui/widgets/bases/_named_list.py

@@ -0,0 +1,80 @@
+from __future__ import annotations
+
+from typing import TYPE_CHECKING, Generic, TypeVar, overload
+
+if TYPE_CHECKING:
+    from collections.abc import Iterator, MutableSequence
+    from typing import Protocol
+
+    class Named(Protocol):
+        name: str
+
+
+T = TypeVar("T", bound="Named")
+
+
+class NamedList(Generic[T]):
+    """Container that maintains a list of objects with a 'name' attribute,.
+
+    Supports list-like insertion and O(1) lookup by name.
+    """
+
+    def __init__(self) -> None:
+        self._list: list[T] = []
+        self._dict: dict[str, T] = {}
+
+    def insert(self, index: int, item: T) -> None:
+        """Inserts an item at the specified index.
+
+        Raises ValueError if an item with the same name already exists.
+        """
+        self._list.insert(index, item)
+        # NB!
+        # we don't actually assert name uniqueness here, because it ruins
+        # the true list-like quality. So, when retrieving by name, you will
+        # simply get the first item that has been inserted with that name.
+        if item.name not in self._dict:
+            self._dict[item.name] = item
+
+    def append(self, item: T) -> None:
+        """Appends an item at the end of the list."""
+        self.insert(len(self._list), item)
+
+    def get_by_name(self, name: str) -> T | None:
+        """Returns the item with the given name, or None if not found."""
+        return self._dict.get(name)
+
+    def remove_by_name(self, name: str) -> None:
+        """Removes the item with the given name."""
+        item = self._dict.pop(name)
+        self._list.remove(item)
+
+    @overload
+    def __getitem__(self, key: int) -> T: ...
+    @overload
+    def __getitem__(self, key: slice) -> MutableSequence[T]: ...
+    def __getitem__(self, index: int | slice) -> T | MutableSequence[T]:
+        return self._list[index]
+
+    @overload
+    def __delitem__(self, index: slice) -> None: ...
+    @overload
+    def __delitem__(self, index: int) -> None: ...
+    def __delitem__(self, index: int | slice) -> None:
+        if isinstance(index, slice):
+            for item in self._list[index]:
+                self._dict.pop(item.name, None)
+            del self._list[index]
+        else:
+            item = self._list[index]
+            del self._list[index]
+            self._dict.pop(item.name, None)
+
+    def __len__(self) -> int:
+        return len(self._list)
+
+    def __iter__(self) -> Iterator[T]:
+        return iter(self._list)
+
+    def __repr__(self) -> str:
+        return repr(self._list)