Repository: Tinche/cattrs

Issue:
GenConverter doesn't support inheritance
* cattrs version: 1.1.1
* Python version: 3.7
* Operating System: Mac

### Description

The old converter supports attrs classes which inherit from each other because it doesn't use MRO-based singledispatch per type to dispatch its attrs classes - therefore, when a subclass B is encountered, it is dynamically structured based on the actual type and the attributes available in the dict.

However, the new GenConverter creates and registers a single-dispatched function as soon as it sees a new type. And functools.singledispatch seems to do some MRO stuff and identifies any subclass as "being" its parent class, therefore skipping the code generation and registration process that cattrs would otherwise do for any new subclass.

### What I Did

```
from cattr.converters import GenConverter
import attr


@attr.s(auto_attribs=True)
class A:
    i: int


@attr.s(auto_attribs=True)
class B(A):
    j: int


def test_inheritance():
    converter = GenConverter()

    # succeeds
    assert A(1) == converter.structure(dict(i=1), A)

    # fails
    assert B(1, 2) == converter.structure(dict(i=1, j=2), B)
```

the failure is 
```
E       AssertionError: assert B(i=1, j=2) == A(i=1)
E        +  where B(i=1, j=2) = B(1, 2)
```

essentially, cattrs/singledispatch chooses A and then runs the A structuring code, rather than generating a new method.


How to fix this? I know singledispatch should be fairly well optimized, so it would be un-fun to drop it entirely. But since this is a breaking change from the old converter to the new converter, I feel like we should try to do something.

I wonder if it might be just as well to use a strict type map looking in the multistrategy dispatch before we hit the singledispatch function. Essentially, if there's an exact match, use that before even trying singledispatch. If there isn't, *and* the type is an attrs class, _skip_ singledispatch so that the class is "caught" by the fallback function dispatch. 

If you're willing to accept this approach, I found a patch that works nicely. I'll probably put up a PR in the next couple of days.






Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/Tinche/cattrs/pull/117?src=pr&el=h1) Report
> Merging [#117](https://codecov.io/gh/Tinche/cattrs/pull/117?src=pr&el=desc) (a25ca9a) into [master](https://codecov.io/gh/Tinche/cattrs/commit/61c944584b3a5472b5733beac79134c77580274d?el=desc) (61c9445) will **decrease** coverage by `4.11%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/Tinche/cattrs/pull/117/graphs/tree.svg?width=650&height=150&src=pr&token=81C7gjO09w)](https://codecov.io/gh/Tinche/cattrs/pull/117?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
- Coverage   98.23%   94.11%   -4.12%     
==========================================
  Files           7        7              
  Lines         453      459       +6     
==========================================
- Hits          445      432      -13     
- Misses          8       27      +19     
```


| [Impacted Files](https://codecov.io/gh/Tinche/cattrs/pull/117?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/cattr/converters.py](https://codecov.io/gh/Tinche/cattrs/pull/117/diff?src=pr&el=tree#diff-c3JjL2NhdHRyL2NvbnZlcnRlcnMucHk=) | `99.01% <100.00%> (ø)` | |
| [src/cattr/multistrategy\_dispatch.py](https://codecov.io/gh/Tinche/cattrs/pull/117/diff?src=pr&el=tree#diff-c3JjL2NhdHRyL211bHRpc3RyYXRlZ3lfZGlzcGF0Y2gucHk=) | `100.00% <100.00%> (ø)` | |
| [src/cattr/\_compat.py](https://codecov.io/gh/Tinche/cattrs/pull/117/diff?src=pr&el=tree#diff-c3JjL2NhdHRyL19jb21wYXQucHk=) | `62.74% <0.00%> (-37.26%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Tinche/cattrs/pull/117?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/Tinche/cattrs/pull/117?src=pr&el=footer). Last update [61c9445...285b608](https://codecov.io/gh/Tinche/cattrs/pull/117?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

petergaultney: i put the entry in 1.2.0 but if you decide to do a bugfix release first, let me know and I can change it to a 1.1.3.

Patch:
diff --git a/HISTORY.rst b/HISTORY.rst
index 6f59b9b..9883525 100644
--- a/HISTORY.rst
+++ b/HISTORY.rst
@@ -7,6 +7,8 @@ History
 * ``converter.unstructure`` now supports an optional parameter, `unstructure_as`, which can be used to unstructure something as a different type. Useful for unions.
 * Improve support for union un/structuring hooks. Flesh out docs for advanced union handling.
   (`#115 <https://github.com/Tinche/cattrs/pull/115>`_)
+* Fix `GenConverter` behavior with inheritance hierarchies of `attrs` classes.
+  (`#117 <https://github.com/Tinche/cattrs/pull/117>`_) (`#116 <https://github.com/Tinche/cattrs/issues/116>`_)
 
 1.1.2 (2020-11-29)
 ------------------
diff --git a/src/cattr/converters.py b/src/cattr/converters.py
index 85cd1d7..d393639 100644
--- a/src/cattr/converters.py
+++ b/src/cattr/converters.py
@@ -508,7 +508,9 @@ class GenConverter(Converter):
             omit_if_default=self.omit_if_default,
             **attrib_overrides
         )
-        self.register_unstructure_hook(obj.__class__, h)
+        self._unstructure_func.register_cls_list(
+            [(obj.__class__, h)], no_singledispatch=True
+        )
         return h(obj)
 
     def structure_attrs_fromdict(
@@ -524,5 +526,8 @@ class GenConverter(Converter):
             if a.type in self.type_overrides
         }
         h = make_dict_structure_fn(cl, self, **attrib_overrides)
-        self.register_structure_hook(cl, h)
+        self._structure_func.register_cls_list(
+            [(cl, h)], no_singledispatch=True
+        )
+        # only direct dispatch so that subclasses get separately generated
         return h(obj, cl)
diff --git a/src/cattr/multistrategy_dispatch.py b/src/cattr/multistrategy_dispatch.py
index c326663..d211f3b 100644
--- a/src/cattr/multistrategy_dispatch.py
+++ b/src/cattr/multistrategy_dispatch.py
@@ -15,16 +15,24 @@ class _DispatchNotFound(object):
 class MultiStrategyDispatch(object):
     """
     MultiStrategyDispatch uses a
-    combination of FunctionDispatch and singledispatch.
+    combination of exact-match dispatch, singledispatch, and FunctionDispatch.
 
-    singledispatch is attempted first. If nothing is
-    registered for singledispatch, or an exception occurs,
+    Exact match dispatch is attempted first, based on a direct
+    lookup of the exact class type, if the hook was registered to avoid singledispatch.
+    singledispatch is attempted next - it will handle subclasses of base classes using MRO
+    If nothing is registered for singledispatch, or an exception occurs,
     the FunctionDispatch instance is then used.
     """
 
-    __slots__ = ("_function_dispatch", "_single_dispatch", "dispatch")
+    __slots__ = (
+        "_direct_dispatch",
+        "_function_dispatch",
+        "_single_dispatch",
+        "dispatch",
+    )
 
     def __init__(self, fallback_func):
+        self._direct_dispatch = {}
         self._function_dispatch = FunctionDispatch()
         self._function_dispatch.register(lambda _: True, fallback_func)
         self._single_dispatch = singledispatch(_DispatchNotFound)
@@ -32,6 +40,9 @@ class MultiStrategyDispatch(object):
 
     def _dispatch(self, cl):
         try:
+            direct_dispatch = self._direct_dispatch.get(cl)
+            if direct_dispatch is not None:
+                return direct_dispatch
             dispatch = self._single_dispatch.dispatch(cl)
             if dispatch is not _DispatchNotFound:
                 return dispatch
@@ -39,10 +50,15 @@ class MultiStrategyDispatch(object):
             pass
         return self._function_dispatch.dispatch(cl)
 
-    def register_cls_list(self, cls_and_handler):
-        """ register a class to singledispatch """
+    def register_cls_list(
+        self, cls_and_handler, no_singledispatch: bool = False
+    ):
+        """ register a class to direct or singledispatch """
         for cls, handler in cls_and_handler:
-            self._single_dispatch.register(cls, handler)
+            if no_singledispatch:
+                self._direct_dispatch[cls] = handler
+            else:
+                self._single_dispatch.register(cls, handler)
         self.dispatch.cache_clear()
 
     def register_func_list(self, func_and_handler):


Test patch:
diff --git a/tests/test_genconverter_inheritance.py b/tests/test_genconverter_inheritance.py
new file mode 100644
index 0000000..b38d369
--- /dev/null
+++ b/tests/test_genconverter_inheritance.py
@@ -0,0 +1,20 @@
+from cattr.converters import GenConverter
+import attr
+
+
+def test_inheritance():
+    @attr.s(auto_attribs=True)
+    class A:
+        i: int
+
+    @attr.s(auto_attribs=True)
+    class B(A):
+        j: int
+
+    converter = GenConverter()
+
+    # succeeds
+    assert A(1) == converter.structure(dict(i=1), A)
+
+    # fails
+    assert B(1, 2) == converter.structure(dict(i=1, j=2), B)