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

Add error for asdict, astuple, fields, and replace in dataclasses - Fixes #14215 #14263

Closed

Conversation

tdscheper
Copy link

Fixes #14215

This change ensures Mypy detects an error when any of the asdict, astuple, fields, or replace methods from the dataclasses library is called on an object that is not a dataclass.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2022

Diff from mypy_primer, showing the effect of this PR on open source code:

core (https://github.com/home-assistant/core)
+ homeassistant/components/zwave_js/discovery.py:78: error: asdict() should be called on dataclass instances  [misc]

porcupine (https://github.com/Akuli/porcupine)
+ porcupine/utils.py:408: error: asdict() should be called on dataclass instances  [misc]

@tmke8
Copy link
Contributor

tmke8 commented Dec 8, 2022

Will something like

from dataclasses import asdict, is_dataclass

def f(x: object) -> dict:
    if is_dataclass(x):
        return asdict(x)
    return {}

work?

@tdscheper
Copy link
Author

tdscheper commented Dec 8, 2022

@thomkeh Just checked, no it does not. I'm thinking the solution to that would involve adding 'dataclass' to the object's metadata if is_dataclass returns True, but being that this is my first time contributing to Mypy, I'm not sure if that is something that should be done. I'll keep looking into it and can submit another PR some time soon, if that works

@JelleZijlstra
Copy link
Member

The principled solution would be to make is_dataclass work similar to a TypeGuard and narrow down the type. We probably should do that first before restricting the types that can be passed to asdict() and friends.

@AlexWaygood
Copy link
Member

AlexWaygood commented Dec 8, 2022

This could potentially be fixed in typeshed rather than mypy, which might be preferable:

from typing import Any, ClassVar, Protocol
from typing_extensions import TypeGuard

class _Dataclassy(Protocol):
   __dataclass_fields__: ClassVar[dict[str, Any]]

def asdict(obj: _Dataclassy) -> dict[str, Any]: ...
def is_dataclass(obj: object) -> TypeGuard[_Dataclassy]: ...

I believe both pyright and mypy already infer that dataclasses automatically have a __dataclass_fields__ ClassVar, so they should be able to infer that any dataclass instance will satisfy the _Dataclassy interface. No idea about whether pytype/pyre will be able to do the same inference, though.

@tmke8
Copy link
Contributor

tmke8 commented Dec 8, 2022

from typing import Any, ClassVar, Protocol
from typing_extensions import TypeGuard

class _Dataclassy(Protocol):
   __dataclass_fields__: ClassVar[dict[str, Any]]

def asdict(obj: _Dataclassy) -> dict[str, Any]: ...
def is_dataclass(obj: object) -> TypeGuard[_Dataclassy]: ...

I can confirm that this works in mypy at least. Though the error messages leak the name of the dummy protocol:

error: Argument 1 to "asdict" has incompatible type "B"; expected "_Dataclassy"  [arg-type]

which might confuse people.

EDIT: works in pyright too, but doesn't work in pyre

@AlexWaygood
Copy link
Member

Though the error messages leak the name of the dummy protocol:

error: Argument 1 to "asdict" has incompatible type "B"; expected "_Dataclassy"  [arg-type]

which might confuse people.

We can probably name it something more boring and less punny like _DataclassInstance ☹️

@AlexWaygood
Copy link
Member

Thanks for the PR! Unfortunately I just merged python/typeshed#9362, which makes this redundant.

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

Successfully merging this pull request may close these issues.

(🎁) Validate arguments to dataclasses utility functions replace and fields
4 participants