Honor converter hooks in make_dict_unstructure_fn

HISTORY.md

@@ -13,6 +13,8 @@ Our backwards-compatibility policy can be found [here](https://github.com/python
 
 ## NEXT (UNRELEASED)
 
+- Fix {func}`make_dict_unstructure_fn <cattrs.gen.make_dict_unstructure_fn>` ignoring hooks registered on the converter (like the methods picked up by the {func}`use_class_methods <cattrs.strategies.use_class_methods>` strategy) when no customizations are requested; hook factories built on top of it now compose with those hooks instead of silently dropping them.
+  ([#566](https://github.com/python-attrs/cattrs/issues/566))
 - Fix heterogeneous tuples and `NamedTuple`s with a member type containing a quote in its `repr`, like `tuple[Literal["a"], int]`, crashing structuring code generation with `SyntaxError`; the index note is now embedded with `repr`.
   ([#777](https://github.com/python-attrs/cattrs/pull/777))
 - Fix {func}`transform_error <cattrs.transform_error>` listing the extra keys of a `ForbiddenExtraKeysError` in set iteration order, which made the message differ between runs; the keys are now sorted, like the error's own `__str__` already sorts them.

src/cattrs/dispatch.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 from functools import lru_cache, singledispatch
+from threading import local
 from typing import TYPE_CHECKING, Any, Callable, Generic, Literal, TypeVar
 
 from attrs import Factory, define
@@ -27,6 +28,40 @@ class _DispatchNotFound:
     """A dummy object to help signify a dispatch not found."""
 
 
+_factory_state = local()
+"""Thread-local state for tracking in-progress hook factory invocations."""
+
+
+def _in_progress_factories() -> set:
+    """The set of hook factories currently being evaluated, as `(id, type)` pairs.
+
+    Used to detect re-entrant factory invocations for the same type; purely
+    observational, it does not change dispatching itself.
+    """
+    try:
+        return _factory_state.in_progress
+    except AttributeError:
+        res = set()
+        _factory_state.in_progress = res
+        return res
+
+
+def _factory_key(handler: Callable, typ: Any) -> tuple | None:
+    """A hashable key identifying a factory invocation, if possible."""
+    key = (id(handler), typ)
+    try:
+        hash(key)
+    except TypeError:
+        return None
+    return key
+
+
+def _is_factory_in_progress(handler: Callable, typ: Any) -> bool:
+    """Is `handler` currently being evaluated for `typ` on this thread?"""
+    key = _factory_key(handler, typ)
+    return key is not None and key in _in_progress_factories()
+
+
 @define
 class FunctionDispatch:
     """
@@ -71,9 +106,17 @@ def dispatch(self, typ: Any) -> Callable[..., Any] | None:
                 continue
             if ch:
                 if is_generator:
-                    if takes_converter:
-                        return handler(typ, self._converter)
-                    return handler(typ)
+                    key = _factory_key(handler, typ)
+                    in_progress = _in_progress_factories() if key is not None else None
+                    if in_progress is not None:
+                        in_progress.add(key)
+                    try:
+                        if takes_converter:
+                            return handler(typ, self._converter)
+                        return handler(typ)
+                    finally:
+                        if in_progress is not None:
+                            in_progress.discard(key)
 
                 return handler
         return None

src/cattrs/gen/__init__.py

@@ -19,7 +19,7 @@
     is_generic,
 )
 from .._generics import deep_copy_with
-from ..dispatch import UnstructureHook
+from ..dispatch import UnstructureHook, _DispatchNotFound, _is_factory_in_progress
 from ..errors import (
     AttributeValidationNote,
     ClassValidationError,
@@ -251,6 +251,75 @@ def make_dict_unstructure_fn_from_attrs(
     return res
 
 
+def _converter_unstructure_hook(
+    converter: BaseConverter, cl: type[T]
+) -> Callable[[T], Any] | None:
+    """Get the converter's own unstructure hook for `cl`, if it has a custom one.
+
+    "Custom" here means anything except the converter's default attrs
+    unstructuring. Hook factories which are already being evaluated for `cl`
+    (directly causing this call, for example a hook factory delegating to
+    `make_dict_unstructure_fn`) are skipped, giving lower-precedence factories
+    a chance; factories raising `RecursionError` are skipped as well.
+
+    Used by `make_dict_unstructure_fn` so it composes with hooks registered on
+    the converter (for example methods picked up by
+    `cattrs.strategies.use_class_methods`, or hooks registered directly for
+    `cl`) instead of silently ignoring them.
+    """
+    unstructure_func = converter._unstructure_func
+
+    def is_default_attrs_hook(hook: Any) -> bool:
+        # `BaseConverter._unstructure_attrs` is the default attrs unstructuring;
+        # bound methods compare by `__self__` and `__func__`.
+        return hook == converter._unstructure_attrs
+
+    # Hooks registered for (super)classes take precedence, mirroring dispatch.
+    try:
+        hook = unstructure_func._single_dispatch.dispatch(cl)
+    except Exception:  # noqa: S110
+        pass
+    else:
+        if hook is not _DispatchNotFound and not is_default_attrs_hook(hook):
+            return hook
+
+    hook = unstructure_func._direct_dispatch.get(cl)
+    if hook is not None and not is_default_attrs_hook(hook):
+        return hook
+
+    for (
+        predicate,
+        handler,
+        is_generator,
+        takes_converter,
+    ) in unstructure_func._function_dispatch._handler_pairs:
+        try:
+            matches = predicate(cl)
+        except Exception:  # noqa: S112
+            continue
+        if not matches:
+            continue
+        if not is_generator:
+            if not is_default_attrs_hook(handler):
+                return handler
+        elif _is_factory_in_progress(handler, cl):
+            # The factory is already being evaluated for `cl` further up the
+            # stack; calling it again would just hit its own recursion guard
+            # (or recurse), so give lower-precedence factories a chance.
+            continue
+        else:
+            try:
+                hook = handler(cl, converter) if takes_converter else handler(cl)
+            except RecursionError:
+                # The factory recurses back into `make_dict_unstructure_fn`;
+                # skip it so factories with a lower precedence get a chance.
+                continue
+            if not is_default_attrs_hook(hook):
+                return hook
+
+    return None
+
+
 def make_dict_unstructure_fn(
     cl: type[T],
     converter: BaseConverter,
@@ -287,7 +356,8 @@ def make_dict_unstructure_fn(
     attrs = adapted_fields(origin or cl)  # type: ignore
 
     mapping = {}
-    if _cattrs_use_alias == "from_converter":
+    use_alias_from_converter = _cattrs_use_alias == "from_converter"
+    if use_alias_from_converter:
         # BaseConverter doesn't have it so we're careful.
         _cattrs_use_alias = getattr(converter, "use_alias", False)
     if is_generic(cl):
@@ -309,6 +379,21 @@ def make_dict_unstructure_fn(
     working_set.add(cl)
 
     try:
+        if (
+            not kwargs
+            and not _cattrs_omit_if_default
+            and not _cattrs_include_init_false
+            and use_alias_from_converter
+        ):
+            # No customizations were requested, so the converter may have a
+            # better hook for `cl` than the one we would generate (for example
+            # a method picked up by `cattrs.strategies.use_class_methods`, or a
+            # hook registered directly for `cl`); prefer it so hook factories
+            # built on top of this function compose with those customizations
+            # instead of silently ignoring them.
+            custom_hook = _converter_unstructure_hook(converter, cl)
+            if custom_hook is not None:
+                return custom_hook
         return make_dict_unstructure_fn_from_attrs(
             attrs,
             cl,

tests/strategies/test_class_methods.py

@@ -2,11 +2,12 @@
 from typing import Union
 
 import pytest
-from attrs import define
+from attrs import define, has
 from hypothesis import given
 from hypothesis.strategies import integers
 
 from cattrs import BaseConverter
+from cattrs.gen import make_dict_unstructure_fn
 from cattrs.strategies import use_class_methods
 
 
@@ -120,3 +121,41 @@ def _unstructure(cls):
         converter.structure({"a": 1}, Bad)
     with pytest.raises(TypeError):
         converter.unstructure(Bad(1))
+
+
+def test_make_dict_unstructure_fn_honors_class_methods(converter: BaseConverter):
+    """`make_dict_unstructure_fn` should honor the converter's own hooks.
+
+    A hook factory delegating to `make_dict_unstructure_fn` should compose with
+    the methods picked up by `use_class_methods` instead of ignoring them.
+
+    See https://github.com/python-attrs/cattrs/issues/566.
+    """
+
+    @define
+    class A:
+        a: int
+
+        def _unstructure(self):
+            return {"a": str(self.a)}
+
+    use_class_methods(converter, None, "_unstructure")
+
+    # The metamethod is picked up directly.
+    assert make_dict_unstructure_fn(A, converter)(A(1)) == {"a": "1"}
+
+    def tag_attrs_hook_factory(cl):
+        base_hook = make_dict_unstructure_fn(cl, converter)
+
+        def hook(inst):
+            unstructured = base_hook(inst)
+            unstructured["_type"] = type(inst).__name__
+            return unstructured
+
+        return hook
+
+    # The tag factory takes precedence (registered last), but the metamethod
+    # should still be honored through `make_dict_unstructure_fn`.
+    converter.register_unstructure_hook_factory(has, tag_attrs_hook_factory)
+
+    assert converter.unstructure(A(1)) == {"a": "1", "_type": "A"}