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

Change astuple and asdict factories #8519

Closed
wants to merge 11 commits into from
Closed

Conversation

sobolevn
Copy link
Member

There's a problem with tuple_factory and dict_factory in dataclasses.pyi
Why?
Because the given callback type does not really match tuple / dict constructors.

Example:

from dataclasses import dataclass, astuple, asdict

@dataclass
class D:
    x: int

astuple(D(1), tuple_factory=tuple)
asdict(D(2), dict_factory=dict)  # Argument "dict_factory" to "asdict" has incompatible type "Type[Dict[Any, Any]]"; expected "Callable[[List[Tuple[str, Any]]], Dict[_KT, _VT]]"

Source: https://github.com/python/cpython/blob/main/Lib/dataclasses.py#L1345-L1349

Closes #8518

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member Author

I've added some simple test cases for this. Because in my opinion this annotation is very complex.

This should be something like:

def asdict(obj: Any, *, dict_factory: Callable[[Iterable[Tuple[str, Any]]], _T[str, Any]]) -> _T[str, Any]: ...
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

Here's the homeassistant code: https://github.com/home-assistant/core/blob/982d197ff3c493024846f38d8688985b6acd3707/homeassistant/components/repairs/websocket_api.py#L66-L78. The primer error is due to parameter types in a Callable being contravariant, so Callable[[list[tuple[Any, Any]]], Any] is not seen as a valid subtype of Callable[[Iterable[tuple[Any, Any]]], Any]. I think they'll just have to update their annotation from list[tuple[Any, Any]] to Iterable[tuple[Any, Any]], if we merge this.

@sobolevn
Copy link
Member Author

I am not sure that list -> Iterable is the proper fix.
Because dict is still not allowed, see failing tests 😒

@github-actions

This comment has been minimized.

@Akuli
Copy link
Collaborator

Akuli commented Aug 10, 2022

Minimal reproducer:

from typing import *
T = TypeVar("T")

def f(dict_factory: Callable[[list[tuple[str, Any]]], T]) -> T:
    ...

f(dict)

Error: Argument 1 to "f" has incompatible type "Type[Dict[Any, Any]]"; expected "Callable[[list[Tuple[str, Any]]], Dict[_KT, _VT]]"

Mypy correctly infers T to be the return type of dict, viewed as a callable, which is dict[_KT, _VT]. But it doesn't realize that _KT can be solved to a string. Changing tuple[str, Any] to tuple[Any, Any] helps, and would be a better fix than changing list:

  • The user doesn't have to be prepared to handle arbitrary iterables. At runtime it is always a list.
  • It seems like if the user types their function with tuple[str, Any] instead of tuple[Any, Any], it would still work even though list is invariant.

We could also try tuple[str | Any, Any].

@github-actions

This comment has been minimized.



assert_type(dataclasses.astuple(D()), Tuple[Any, ...])
assert_type(dataclasses.astuple(D(), tuple_factory=tuple), Tuple[Any, ...])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Eric fixed this recently (Any and Unknown being considered different in assert_type()). Let's upgrade pyright to see if it helps.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@AlexWaygood
Copy link
Member

Closing for now due to lack of activity and to keep the number of open PRs manageable. Happy to reopen if you're still interested in working on this! :)

@sobolevn
Copy link
Member Author

I have 0 ideas on how to fix it.

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