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 a TypeGuard for dataclasses.is_dataclass(); refine asdict(), astuple(), fields(), replace() #9362

Merged
merged 15 commits into from
Jan 28, 2023

Conversation

AlexWaygood
Copy link
Member

Fixes #9345. Refs python/mypy#14215

stdlib/dataclasses.pyi Outdated Show resolved Hide resolved
@AlexWaygood AlexWaygood changed the title Use a TypeGuard for dataclasses.is_dataclass(); refine as_dict(), as_tuple() Use a TypeGuard for dataclasses.is_dataclass(); refine as_dict(), as_tuple(), fields(), replace() Dec 14, 2022
@github-actions

This comment has been minimized.

stdlib/dataclasses.pyi Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Dec 14, 2022

The pyright tests are failing due to microsoft/pyright#4339.

Links to the mypy_primer hits:

@AlexWaygood AlexWaygood marked this pull request as ready for review December 14, 2022 20:50
@github-actions

This comment has been minimized.

@AlexWaygood AlexWaygood changed the title Use a TypeGuard for dataclasses.is_dataclass(); refine as_dict(), as_tuple(), fields(), replace() Use a TypeGuard for dataclasses.is_dataclass(); refine asdict(), astuple(), fields(), replace() Dec 15, 2022
@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

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

porcupine (https://github.com/Akuli/porcupine)
+ porcupine/utils.py:408: error: No overload variant of "asdict" matches argument type "EventDataclass"  [call-overload]
+ porcupine/utils.py:408: note: Possible overload variants:
+ porcupine/utils.py:408: note:     def asdict(obj: _DataclassInstance) -> Dict[str, Any]
+ porcupine/utils.py:408: note:     def [_T] asdict(obj: _DataclassInstance, *, dict_factory: Callable[[List[Tuple[str, Any]]], _T]) -> _T

hydra-zen (https://github.com/mit-ll-responsible-ai/hydra-zen)
- src/hydra_zen/structured_configs/_implementations.py:1666: error: Incompatible types in assignment (expression has type "Union[Type[Any], Callable[P, R], Callable[..., Any]]", variable has type overloaded function)  [assignment]
+ src/hydra_zen/structured_configs/_implementations.py:1666: error: Incompatible types in assignment (expression has type "Union[Type[_DataclassInstance], Type[Any], Callable[P, R], Callable[..., Any], DataClass_]", variable has type overloaded function)  [assignment]

xarray-dataclasses (https://github.com/astropenguin/xarray-dataclasses)
+ xarray_dataclasses/v2/specs.py:115: error: Argument 1 to "fields" has incompatible type "Type[DataClass[P]]"; expected "Union[_DataclassInstance, Type[_DataclassInstance]]"  [arg-type]
+ xarray_dataclasses/v2/specs.py:115: note: Only class variables allowed for class object access on protocols, __dataclass_fields__ is an instance variable of "DataClass"
+ xarray_dataclasses/v2/specs.py:115: note: ClassVar protocol member _DataclassInstance.__dataclass_fields__ can never be matched by a class object
+ xarray_dataclasses/v2/specs.py:158: error: Argument 1 to "fields" has incompatible type "Type[DataClass[P]]"; expected "Union[_DataclassInstance, Type[_DataclassInstance]]"  [arg-type]
+ xarray_dataclasses/v2/specs.py:158: note: Only class variables allowed for class object access on protocols, __dataclass_fields__ is an instance variable of "DataClass"
+ xarray_dataclasses/v2/specs.py:158: note: ClassVar protocol member _DataclassInstance.__dataclass_fields__ can never be matched by a class object

core (https://github.com/home-assistant/core)
+ homeassistant/components/p1_monitor/diagnostics.py:45: error: Argument 1 to "asdict" has incompatible type "Optional[Any]"; expected "_DataclassInstance"  [arg-type]
+ homeassistant/components/zwave_js/discovery.py:78: error: No overload variant of "asdict" matches argument type "DataclassMustHaveAtLeastOne"  [call-overload]
+ homeassistant/components/zwave_js/discovery.py:78: note: Possible overload variants:
+ homeassistant/components/zwave_js/discovery.py:78: note:     def asdict(obj: _DataclassInstance) -> Dict[str, Any]
+ homeassistant/components/zwave_js/discovery.py:78: note:     def [_T] asdict(obj: _DataclassInstance, *, dict_factory: Callable[[List[Tuple[str, Any]]], _T]) -> _T

@AlexWaygood
Copy link
Member Author

@stroxler / @pradeep90: from @thomkeh's experiments in #9345, it looks like pyre doesn't currently synthesize a __dataclass_fields__ ClassVar for dataclass classes in the same way that mypy and pyright do. That means that if this PR were to be merged, it might cause false positives for pyre users. Would you be okay with it being merged, or would you rather we held off for now?

Same question for @rchen152 -- not sure if pytype synthesizes this attribute for dataclasses or not? Would merging this cause issues for pytype?

@rchen152
Copy link
Collaborator

Yep, pytype synthesizes the __dataclass_fields__ attribute! Somewhat amusingly, the entire reason we added it was to get this pytype_extensions.Dataclass protocol working: https://github.com/google/pytype/blob/8d0970083128ffc7f6042b08fad076f8c2ee01a6/pytype_extensions/__init__.py#L67

@tmke8
Copy link
Contributor

tmke8 commented Dec 19, 2022

Should typing_extensions also get a Dataclass protocol then? 🤔

@AlexWaygood
Copy link
Member Author

Yep, pytype synthesizes the dataclass_fields attribute!

Brilliant, thanks @rchen152!

Should typing_extensions also get a Dataclass protocol then? 🤔

Maybe. But it's probably better to add it to typeshed first and wait a while (a few months, at least) to see if it's stable before adding it to typing_extensions.

@pradeep90
Copy link
Contributor

@stroxler / @pradeep90: from @thomkeh's experiments in #9345, it looks like pyre doesn't currently synthesize a __dataclass_fields__ ClassVar for dataclass classes in the same way that mypy and pyright do. That means that if this PR were to be merged, it might cause false positives for pyre users. Would you be okay with it being merged, or would you rather we held off for now?

Yes, feel free to merge this change.

Pyre doesn't currently synthesize a __dataclass_fields__ ClassVar for dataclasses. I'll fix it on the Pyre side. We vendor a typeshed version manually anyway, so we will ensure the fix has landed before we upgrade next.

@cdce8p
Copy link
Contributor

cdce8p commented Feb 4, 2023

I've been working on updating Home Assistant for these changes. Some notes

There are a couple of places where I needed to use the dataclass protocol itself. One example are dataclass mixin classes. From that I think it would make sense to either include it in typing_extensions (like others suggested already) or in _typeshed.

At least for mypy, pydantic dataclasses don't seem to work, yet. Haven't had the time to look into it.

import dataclasses
from pydantic.dataclasses import dataclass

@dataclass
class Info:
    var: str

def func(info: Info) -> None:
    dataclasses.asdict(info)
t:9: error: No overload variant of "asdict" matches argument type "Info"  [call-overload]
t:9: note: Possible overload variants:
t:9: note:     def asdict(obj: _DataclassInstance) -> Dict[str, Any]
t:9: note:     def [_T] asdict(obj: _DataclassInstance, *, dict_factory: Callable[[List[Tuple[str, Any]]], _T]) -> _T

Tested with pydantic==1.10.4. It's quite common that an upstream library uses pydantic dataclasses (-> Info), whereas the caller uses dataclasses (-> func).

@tmke8
Copy link
Contributor

tmke8 commented Feb 4, 2023

As long as pydantic's dataclass has the __dataclass_fields__ attribute, it should work, right?

EDIT: oh wait, nevermind. The type checker needs to know that it's going to be synthesized. So maybe typing.dataclass_transform needs to have the option to synthesize the attribute.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Feb 4, 2023

I think pydantic has a custom mypy plugin, so it's probably an issue with the plugin not synthesising the attribute (haven't dug in to confirm my suspicion).

@AlexWaygood
Copy link
Member Author

Anyway, thanks (as ever) for the very early testing here @cdce8p! Given how new/experimental the protocol is, and how it doesn't work particularly well with pyright due to microsoft/pyright#4339, I think it definitely isn't a suitable candidate to be added to typing_extensions just yet. But adding it to _typeshed is something we can easily do now, so I'll try to get on that (or alternatively, I'll happily accept a PR if you get there first ;)

@AlexWaygood
Copy link
Member Author

(To be clear, this PR shouldn't cause any regressions for pyright users, the pyright issue just means that this PR doesn't provide quite as much of a type-safety improvement for pyright users as it does for mypy users.)

@cdce8p
Copy link
Contributor

cdce8p commented Feb 4, 2023

Small update regarding pydantic. Enabling it's mypy plugin does resolve the issue

[mypy]
plugins = pydantic.mypy

https://docs.pydantic.dev/mypy_plugin/

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

Successfully merging this pull request may close these issues.

is_dataclass should be a type guard and asdict should only work on objects with __dataclass_fields__
6 participants