oxan / djangorestframework-dataclasses

Dataclasses serializer for Django REST framework
BSD 3-Clause "New" or "Revised" License
429 stars 28 forks source link

TYPE_CHECKING imports result in error on type evaluation #57

Closed Syntaf closed 1 year ago

Syntaf commented 2 years ago

I'm not sure if there's actually a solution here - but I came across this situation today where my usage of typing-only imports causes the dataclass type evaluator to fail.

I'd be open to submitting a PR for a patch (if possible) or a PR to provide better error messaging if this is a wont-fix.

Minimal example:

# app/foo/domains.py

from dataclasses import dataclass

@dataclass
class Foo:
    a: str
# app/bar/domains.py

from __future__ import annotations
from typing import TYPE_CHECKING
from dataclasses import dataclass

if TYPE_CHECKING:
    from app.foo.domains import Foo

@dataclass
class Bar:
    b: str
    foo: Foo
# app/bar/serializers.py

from rest_framework_dataclasses.serializers import DataclassSerializer
from app.bar.domains import Bar

class FooSerializer(DataclassSerializer):
    class Meta:
        dataclass = Foo

class BarSerializer(DataclassSerializer):
    foo = FooSerializer()

    class Meta:
        dataclass = Bar

Error (when calling to_representation on BarSerializer:

  ...
  File ".pyenv/versions/3.8.13/lib/python3.8/typing.py", line 1232, in get_type_hints
    value = _eval_type(value, base_globals, localns)
  File ".pyenv/versions/3.8.13/lib/python3.8/typing.py", line 270, in _eval_type
    return t._evaluate(globalns, localns)
  File ".pyenv/versions/3.8.13/lib/python3.8/typing.py", line 518, in _evaluate
    eval(self.__forward_code__, globalns, localns),
  File "<string>", line 1, in <module>
NameError: name 'Foo' is not defined
oxan commented 2 years ago

I don't think there's a way around this; the serializer needs to resolve the actual type the field refers to, and for that at least the name needs to be defined. Why do you put the import under if TYPE_CHECKING?

Syntaf commented 2 years ago

if TYPE_CHECKING can be a useful trick to reduce the dependency graph of a module in a production environment while still having detailed typings inside your IDE

I figured there might not be a workaround to this as we have to fetch the type from somewhere. Would you be interested in a PR that attaches a hint onto NameError exceptions caught while searching for the type? I could see an exception like:

**  ...
  File ".pyenv/versions/3.8.13/lib/python3.8/typing.py", line 1232, in get_type_hints
    value = _eval_type(value, base_globals, localns)
  File ".pyenv/versions/3.8.13/lib/python3.8/typing.py", line 270, in _eval_type
    return t._evaluate(globalns, localns)
  File ".pyenv/versions/3.8.13/lib/python3.8/typing.py", line 518, in _evaluate
    eval(self.__forward_code__, globalns, localns),
  File "<string>", line 1, in <module>
NameError: name 'Foo' is not defined - Ensure your types are defined outside of a TYPE_CHECKING context
oxan commented 2 years ago

I don't think it makes sense to single out TYPE_CHECKING in the error message, there are plenty of unrelated reasons you could get that error (e.g. a typo). I wouldn't be opposed to a more generic message though, something like "could not resolve type hint $name, please ensure all referenced types are available in scope.". Ideally we'd also add the name of the field that causes the error, but I don't immediately know if that can easily be done.

We could also add a few sentences about TYPE_CHECKING to the README (a FAQ section might be a good idea in general).

adonig commented 1 year ago

I run into this issue because I split my one huge data class into multiple ones:

mymodel/base.py mymodel/foo.py mymodel/bar.py ...

# in base.py
from . import foo
from . import bar

@dataclass
class BaseModel(
    foo.FooMixin,
    bar.BarMixin,
    # more mixins here
):
   base_field: int | None = None
   # more fields here

The foo and bar submodules use stuff from the base submodule for typing, so I import it like that to prevent a circular import:

# in foo.py
if TYPE_CHECKING:
    from .base import X

I get that the serializer needs to know the types of the fields, so there's probably no other way than to either find another way to prevent the circular imports or just put everything in one big file (what a giant mess).

If I see it correctly, you use typing.get_type_hints(tp) to get the type hints.

I found something interesting in this bug report:

    def _get_type_hints(func, **kwargs):
        try:
            return typing.get_type_hints(func, **kwargs)
        except Exception:
            # First, try to use the get_type_hints to resolve
            # annotations. But for keeping the behavior intact
            # if there was a problem with that (like the namespace
            # can't resolve some annotation) continue to use
            # string annotations
            return func.__annotations__

Is there maybe a way to get it working with that helper function?

oxan commented 1 year ago

The problem isn't getting the type hints, it's assigning meaning to them. In your example, the _get_type_hints() function will just return the string X. How can the serializer know which class from which module this is, if it hasn't been imported?

ippeiukai commented 1 year ago

Any chance passing forward referenced types to typing.get_type_hints function as globalns would help?

class BarSerializer(DataclassSerializer):
  …

  class Meta:
    dataclass = Bar
    forward_references = dict(Foo=Foo)

credit: https://github.com/konradhalas/dacite/blob/master/README.md#forward-references (I just recognised the pattern.)

oxan commented 1 year ago

I guess that'd work, but I find assigning meaning to a type hint in a different location pretty ugly (and hard to maintain). If someone has a need for this, I'm happy to consider a PR though.

oxan commented 1 year ago

I don't think there's anything actionable for me here. Feel free to reopen or create a new issue if someone has an idea about how this could be resolved.