Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeError when registering ForwardRefs in python 3.10.2 #206

Open
JWCook opened this issue Jan 15, 2022 · 11 comments · May be fixed by #215
Open

TypeError when registering ForwardRefs in python 3.10.2 #206

JWCook opened this issue Jan 15, 2022 · 11 comments · May be fixed by #215

Comments

@JWCook
Copy link
Contributor

JWCook commented Jan 15, 2022

  • cattrs version: 1.10.0
  • Python version: 3.10.2
  • Operating System: Ubuntu 20.04.3

Description

This is related to #201, but probably worth making a separate issue for, since it affects one of the (previously working) workarounds mentioned in that issue. Feel free to close this if you feel this is redundant with #201 or #94.

I have a converter that registers a ForwardRef. In python 3.6.x through 3.10.1, this has worked as intended. As of python 3.10.2, this is now throwing a TypeError:

TypeError: Invalid first argument to `register()`. ForwardRef('MyClass') is not a class.

Strangely, I don't see anything in the python 3.10.2 changelog that would indicate changed behavior with ForwardRef or register().

What I Did

Minimal example:

from attr import define, field
from cattr import Converter  # Note: same behavior with GenConverter
from typing import ForwardRef, List

@define()
class MyClass:
    history: List['MyClass'] = field(factory=list)

converter = Converter()
converter.register_structure_hook(
    ForwardRef('MyClass'), lambda obj, _: converter.structure(obj, MyClass)
)

Traceback:

TypeError                                 Traceback (most recent call last)
Input In [17], in <module>
----> 1 converter.register_structure_hook(
      2     ForwardRef("MyClass"), (lambda obj, _: converter.structure(obj, MyClass))
      3 )

File ~/.virtualenvs/rc-310/lib/python3.10/site-packages/cattr/converters.py:269, in Converter.register_structure_hook(self, cl, func)
    267     self._structure_func.clear_cache()
    268 else:
--> 269     self._structure_func.register_cls_list([(cl, func)])

File ~/.virtualenvs/rc-310/lib/python3.10/site-packages/cattr/dispatch.py:57, in MultiStrategyDispatch.register_cls_list(self, cls_and_handler, direct)
     55         self._direct_dispatch[cls] = handler
     56     else:
---> 57         self._single_dispatch.register(cls, handler)
     58         self.clear_direct()
     59 self.dispatch.cache_clear()

File ~/.pyenv/versions/3.10.2/lib/python3.10/functools.py:856, in singledispatch.<locals>.register(cls, func)
    854 else:
    855     if func is not None:
--> 856         raise TypeError(
    857             f"Invalid first argument to `register()`. "
    858             f"{cls!r} is not a class."
    859         )
    860     ann = getattr(cls, '__annotations__', {})
    861     if not ann:

TypeError: Invalid first argument to `register()`. ForwardRef('MyClass') is not a class.
@JWCook
Copy link
Contributor Author

JWCook commented Jan 15, 2022

Note that one of the other solutions posted in #201 still works in python 3.10.2: #201 (comment)

So the above example would become:

from attr import define, field
from cattr import GenConverter
from typing import ForwardRef, List

@define()
class MyClass:
    history: List['MyClass'] = field(factory=list)

converter = GenConverter()
converter .register_structure_hook_func(
    lambda t: t.__class__ is typing.ForwardRef,
    lambda v, t: converter.structure(v, t.__forward_value__),
)

So this may be a non-issue, but probably still useful for someone else out there googling for the same error.

@aha79
Copy link

aha79 commented Jan 15, 2022

I think the error is due to this commit (a change in functools.py), which was a fix due to bpo-46032.

It seems that the newly added check _is_valid_dispatch_type() returns false and thus we get the TypeError. (I cannot run the example as only Python 10.2 for macos is out).

It is a bit hard to follow the logic, but ForwardRef("MyClass") technically is an instance and not a type (which may explain the behavior).

However, I do not understand is why the solution in your last comment works.

Also what the change to singledispatch means for cattrs. I have the feeling that this will bite us again.

@JWCook
Copy link
Contributor Author

JWCook commented Jan 15, 2022

Good to know, thanks for tracking that down. It looks like this may be an unintended side effect of that bugfix rather than an intentional change, then.

It is a bit hard to follow the logic, but ForwardRef("MyClass") technically is an instance and not a type (which may explain the behavior).

True, but an instance of ForwardRef also looks like a class:

>>> from typing import ForwardRef
>>> type(ForwardRef("MyClass"))
<class 'typing.ForwardRef'>

However, I do not understand is why the solution in your last comment works.

Tinche would be able to give a more thorough explanation, but I believe it works because Converter.register_structure_hook_func() takes a different path than register_structure_hook(), via FunctionDispatch.dispatch():

@attr.s(slots=True)
class FunctionDispatch:
"""
FunctionDispatch is similar to functools.singledispatch, but
instead dispatches based on functions that take the type of the
first argument in the method, and return True or False.
objects that help determine dispatch should be instantiated objects.
"""
_handler_pairs: list = attr.ib(factory=list)
def register(
self, can_handle: Callable[[Any], bool], func, is_generator=False
):
self._handler_pairs.insert(0, (can_handle, func, is_generator))
def dispatch(self, typ):
"""
returns the appropriate handler, for the object passed.
"""
for can_handle, handler, is_generator in self._handler_pairs:
# can handle could raise an exception here
# such as issubclass being called on an instance.
# it's easier to just ignore that case.
try:
ch = can_handle(typ)
except Exception:
continue
if ch:
if is_generator:
return handler(typ)
else:
return handler
raise StructureHandlerNotFoundError(
f"unable to find handler for {typ}", type_=typ
)

We're using the function we provide (in this case testing t.__class__ is typing.ForwardRef) instead of the checks in functools.singledispatch.register(), so _is_valid_dispatch_type() is never called on a ForwardRef instance.

@Tinche
Copy link
Member

Tinche commented Jan 16, 2022

Yeah, as you folks found out, singledispatch is very inadequate for a bunch of use cases using more abstract types (i.e. not actual classes). That's why after checking singledispatch cattrs has a list of predicates that it'll check in order, and that's what you're enabling with register_structure_hook_func. I'm actually surprised ForwardRefs worked before with singledispatch.

So the predicate approach would be preferred, yeah.

@ermshiperete
Copy link

Same bug occurs with Python 3.9.10 (on Debian/Ubuntu).

ermshiperete added a commit to keymanapp/keyman that referenced this issue Jan 18, 2022
This works around a Python bug introduced in Python 3.10.2 and
3.9.10 (python-attrs/cattrs#206).

This change should be reverted once that bug is fixed and new
Python packages available in Debian/Ubuntu.

Fixes #6119.
ermshiperete added a commit to keymanapp/keyman that referenced this issue Jan 18, 2022
This works around a Python bug introduced in Python 3.10.2 and
3.9.10 (python-attrs/cattrs#206).

This change should be reverted once that bug is fixed and new
Python packages are available in Debian/Ubuntu.

Fixes #6119.
ermshiperete added a commit to keymanapp/keyman that referenced this issue Jan 18, 2022
This works around a Python bug introduced in Python 3.10.2 and
3.9.10 (python-attrs/cattrs#206).

This change should be reverted once that bug is fixed and new
Python packages are available in Debian/Ubuntu.

Fixes #6119.

(cherry picked from commit 412b6c6)
ermshiperete added a commit to keymanapp/keyman that referenced this issue Jan 18, 2022
This works around a Python bug introduced in Python 3.10.2 and
3.9.10 (python-attrs/cattrs#206).

This change should be reverted once that bug is fixed and new
Python packages are available in Debian/Ubuntu.

Fixes #6119.
ermshiperete added a commit to keymanapp/keyman that referenced this issue Jan 18, 2022
This works around a Python bug introduced in Python 3.10.2 and
3.9.10 (python-attrs/cattrs#206).

This change should be reverted once that bug is fixed and new
Python packages are available in Debian/Ubuntu.

Fixes #6119.

(cherry picked from commit 10e1697)
@ermshiperete
Copy link

See also the bug in requests-cache: requests-cache/requests-cache#501

@alekseiloginov
Copy link

alekseiloginov commented Jan 26, 2022

Hi, any timeline to fix this issue for python 3.9.10? Thanks

@ermshiperete
Copy link

Hi, any timeline to fix this issue for python 3.10.2? Thanks

This is fixed in requests-cache 0.9.1

@alekseiloginov
Copy link

Thanks, but I'm curious more about cattrs, since I'm getting it from cattrs 1.10.0:

  File "/opt/brew/lib/python3.9/site-packages/cattr/converters.py", line 269, in register_structure_hook
    self._structure_func.register_cls_list([(cl, func)])
  File "/opt/brew/lib/python3.9/site-packages/cattr/dispatch.py", line 57, in register_cls_list
    self._single_dispatch.register(cls, handler)
  File "/opt/brew/Cellar/[email protected]/3.9.10/Frameworks/Python.framework/Versions/3.9/lib/python3.9/functools.py", line 855, in register
    raise TypeError(
TypeError: Invalid first argument to `register()`. ForwardRef('AccessToken') is not a class.

@Tinche
Copy link
Member

Tinche commented Jan 27, 2022

@alekseiloginov cattrs has no "official" support for ForwardRefs yet (or has ever had it). The register_structure_hook approach worked while singledispatch supported it, but since https://bugs.python.org/issue46032 was released it no longer supports it.

We might build ForwardRef support into cattrs in a special way (there's some interest) in the future, but until then simply switch from using register_structure_hook to using register_structure_hook_func (see #206 (comment)). This isn't a hack, this is how cattrs supports all hooks that cannot work via singledispatch (there are many).

@aha79 aha79 linked a pull request Jan 27, 2022 that will close this issue
mbaluch pushed a commit to RedHatProductSecurity/osidb that referenced this issue Jan 31, 2022
Due to some changes very high up the stack in CPython 3.9.10 [1],
requests-cache does no longer work properly [2] and will fail due to
its dependency on cattrs [3] which uses functools.singledispatch
which is what changed in the aforementioned CPython update.

The requests-cache devs have implemented a workaround in version
0.9.1 which is the only thing that they can do since cattrs does
not explicitly support their usage (and are not willing to
implement a workaround at their level).

This commit upgrades to said version.

[1] https://bugs.python.org/issue46032
[2] requests-cache/requests-cache#504
[3] python-attrs/cattrs#206
@ex-nerd
Copy link

ex-nerd commented Sep 2, 2022

Note that one of the other solutions posted in #201 still works in python 3.10.2: #201 (comment)

I wasn't able to get my slightly different situation to work with python==3.10.4, cattrs==22.1.0 (MacOS, pyenv). It runs, but fails to structure things properly (just leaves the nested property as a regular dict instead of my class). However, one of the other solutions from #201 (comment) works great: just put the whole thing in quotes.

from attr import define, field

@define()
class MyClass:
    history: "Optional[List[MyClass]]" = field(default=None)

lewisjared added a commit to climate-resource/spaemis that referenced this issue Feb 13, 2023
lewisjared added a commit to climate-resource/spaemis that referenced this issue Feb 13, 2023
* feat: Add generate command for generating a scaler CSV file
* feat: Read scaler configuration from a CSV
* feat: Add configurations for ssp119, ssp126 and ssp245
* fix: Update register hook

See python-attrs/cattrs#206 (comment)
lewisjared added a commit to climate-resource/spaemis that referenced this issue Mar 25, 2023
* feat: Add generate command for generating a scaler CSV file
* feat: Read scaler configuration from a CSV
* feat: Add configurations for ssp119, ssp126 and ssp245
* fix: Update register hook

See python-attrs/cattrs#206 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants