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

Incompatible type with many=True #260

Open
christianbundy opened this issue Sep 27, 2022 · 9 comments · May be fixed by #315
Open

Incompatible type with many=True #260

christianbundy opened this issue Sep 27, 2022 · 9 comments · May be fixed by #315
Labels
bug Something isn't working

Comments

@christianbundy
Copy link
Contributor

Bug report

What's wrong

class MySerializer(serializers.Serializer[int]): ...

MySerializer([1,2,3], many=True)
error: Argument 1 to "MySerializer" has incompatible type "List[int]"; expected "Optional[int]"  [arg-type]

How is that should be

Success: no issues found in 1 source file

System information

  • OS: Linux f16759b1b324 5.10.104-linuxkit #1 SMP PREEMPT Thu Mar 17 17:05:54 UTC 2022 aarch64 GNU/Linux
  • python version: 3.10.6
  • django version: 4.0.7
  • mypy version: 0.961
  • django-stubs version: 1.12.0

Also, it isn't asked for by the template, but:

  • djangorestframework version: 3.13.1
  • djangorestframework-stubs version: 1.7.0
@christianbundy christianbundy added the bug Something isn't working label Sep 27, 2022
@mdantonio
Copy link

We are observing exactly the same error, in our case we have:

  • python 3.9.13
  • django 4.1.1
  • mypy 0.961
  • django-stubs version: 1.12.0
  • djangorestframework version: 3.13.1
  • djangorestframework-stubs version: 1.7.0

I see that BaseSerializer.instance is annotated as:

    instance: Optional[_IN]

That explains why the list is not accepted, even if it is a correct DRF syntax when passing many=True

@sshishov
Copy link
Contributor

I am having the issue with many=True as well, but actually the problem is the type of the instance when we are calling save method (instances = serializer.save()) when serializer was initialized with many=True.

The instance type is returned Instance instead of list[Instance] or Sequence or Iterable...

@Viicos
Copy link
Contributor

Viicos commented Jan 8, 2023

This looks tricky to fix. When many=True is used, the many_init classmethod is used to return a ListSerializer instance. So first of all this method should return ListSerializer instead of BaseSerializer. Assuming people specifying a custom class are inheriting from ListSerializer, this should be ok.

def many_init(cls, *args: Any, **kwargs: Any) -> BaseSerializer: ...

Then an overload should be used here:

def __new__(cls: type[Self], *args: Any, **kwargs: Any) -> Self: ...

To return something ListSerializer instead of Self if many=True.

Finally, an overload similar to the previous one should be used in the __init__ method:

@overload
def __init__(
    self,
    instance: List[_IN] | None = ...,
    data: Any = ...,
    partial: Literal[True] = ...,
    many: bool = ...,
    ...

But type checkers would probably not be able to infer the returned instance based on the many argument anyway.

@Viicos Viicos linked a pull request Jan 8, 2023 that will close this issue
@christianbundy
Copy link
Contributor Author

But type checkers would probably not be able to infer the returned instance based on the many argument anyway.

I was imagining that we might be able to use an overload here, discriminated on whether many: Literal[True] or many: Literal[False] -- do you think that would work, or are there gotchas?

@Viicos
Copy link
Contributor

Viicos commented Jan 8, 2023

But type checkers would probably not be able to infer the returned instance based on the many argument anyway.

I was imagining that we might be able to use an overload here, discriminated on whether many: Literal[True] or many: Literal[False] -- do you think that would work, or are there gotchas?

Made a first draft in the linked PR, I'd be happy to have your insight on it as I'm unsure this would work :)

@sshishov
Copy link
Contributor

I assume this is the related issue on the main repo: encode/django-rest-framework#8926

@flaeppe
Copy link
Member

flaeppe commented Jun 28, 2023

Would it be possible to @overload on __new__? To capture returning a ListSerializer? As mypy should consider __new__s return type nowadays

@Viicos
Copy link
Contributor

Viicos commented Jun 28, 2023

Would it be possible to @overload on __new__? To capture returning a ListSerializer? As mypy should consider __new__s return type nowadays

You can check the pending PR #315, mypy doesn't allow all __new__s return type unfortunately

@flaeppe
Copy link
Member

flaeppe commented Jun 28, 2023

Right, I suppose the simplest way to get types right is to just call .many_init(...) directly instead of passing many=True via __new__

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

5 participants