djangorestframework-dataclasses icon indicating copy to clipboard operation
djangorestframework-dataclasses copied to clipboard

TYPE_CHECKING imports result in error on type evaluation

Open Syntaf opened this issue 3 years ago • 3 comments

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

Syntaf avatar Jun 15 '22 17:06 Syntaf

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?

oxan avatar Jun 22 '22 20:06 oxan

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

Syntaf avatar Jun 25 '22 17:06 Syntaf

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).

oxan avatar Jun 25 '22 23:06 oxan

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?

adonig avatar Dec 07 '22 12:12 adonig

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?

oxan avatar Dec 07 '22 15:12 oxan

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.)

ippeiukai avatar Dec 07 '22 15:12 ippeiukai

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 avatar Dec 07 '22 16:12 oxan

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.

oxan avatar Aug 21 '23 19:08 oxan