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

Use __new__ instead of __init__ in serializers #315

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Viicos
Copy link
Contributor

@Viicos Viicos commented Jan 8, 2023

Attempt to fix #260. This is starting to be really tricky to handle, so I don't know if this is a viable option. Regarding ModelSerializer, we might want to define the same overload, and only allow specific types for instance with respect to many (e.g. if many=False, only allow _MT, but I'm unsure there's other types that could be allowed as well).

Note: __init__ has been omitted, otherwise mypy will not infer the correct return type from __new__

@Viicos Viicos marked this pull request as draft January 8, 2023 17:28
@intgr
Copy link
Contributor

intgr commented Jan 9, 2023

This has been in the back of my mind also. It would be great if this works.

AFAIK def __new__(...) -> ListSerializer is not allowed: mypy requires __new__ return type to be a subclass of the class being defined. There's a discussion about this somewhere in mypy issues or PRs, I couldn't find it right now.

But I think it's possible to convince mypy developers to change this requirement.

Please add a test case demonstrating whether this works or not.

@intgr
Copy link
Contributor

intgr commented Jan 9, 2023

The CI errors will be fixed in #316.

@Viicos
Copy link
Contributor Author

Viicos commented Jan 9, 2023

AFAIK def __new__(...) -> ListSerializer is not allowed: mypy requires __new__ return type to be a subclass of the class being defined. There's a discussion about this somewhere in mypy issues or PRs, I couldn't find it right now.

Well ListSerializer is a subclass of BaseSerializer, so this should be alright I guess. Will add tests

EDIT: This is not the case for ModelSerializer, so mypy indeed complains about this:

error: Incompatible return type for "__new__" (returns "ListSerializer[_MT]", but must return a subtype of "ModelSerializer[Any]")

So I've added a type: ignore in the meanwhile

@Viicos Viicos marked this pull request as ready for review January 9, 2023 21:43
@Viicos Viicos marked this pull request as draft January 9, 2023 21:44
@Viicos
Copy link
Contributor Author

Viicos commented Jan 10, 2023

Ok this seems to work fine when using BaseSerializer:

IntBaseSerializer = serializers.BaseSerializer[int]
test_serializer = IntBaseSerializer([1, 2], many=True)
reveal_type(test_serializer) # N: Revealed type is "rest_framework.serializers.ListSerializer[builtins.int]"

But when defining a custom serializer:

class TestSerializer(serializers.Serializer[int]):
    pass

test_serializer = TestSerializer(instance=[1, 2], many=True)
reveal_type(test_serializer) # N: Revealed type is "main.TestSerializer"

We get the wrong type, possibly because mypy isn't using the __new__ overload defined in BaseSerializer. But I suspect this might be because ListSerializer isn't a subclass of TestSerializer, so mypy isn't giving the correct type.

I did some tests locally, and it seems that this issue is specific to mypy:

from __future__ import annotations

class A:
    def __new__(cls) -> B:
        return B()

class B(A):
    pass

class C(A):
    pass

a = C()
reveal_type(a) # Revealed type is "C", but got B with pyright (Pylance)

I'll open a ticket @mypy, and I think this could be an opportunity to discuss what you said @intgr about the requirement that the return type must be a subclass of the class being defined

overload definition is subject to change, as it is missing some
arguments.
@intgr
Copy link
Contributor

intgr commented Jan 12, 2023

I found there's already an open mypy issue about this: python/mypy#8330

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Incompatible type with many=True
2 participants