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 |= and | operators support for TypedDict #16249

Merged
merged 15 commits into from
Oct 23, 2023
Merged

Add |= and | operators support for TypedDict #16249

merged 15 commits into from
Oct 23, 2023

Conversation

sobolevn
Copy link
Member

Please, note that there are several problems with __ror__ definitions.

  1. dict.__ror__ does not define support for Mapping? types. For example:
>>> import types
>>> {'a': 1} | types.MappingProxyType({'b': 2})
{'a': 1, 'b': 2}
>>> 
  1. TypedDict.__ror__ also does not define this support

So, I would like to defer this feature for the future, we need some discussion to happen.
However, this PR does fully solve the problem OP had.

Closes #16244

@sobolevn
Copy link
Member Author

sobolevn commented Oct 11, 2023

Hm, looks like {'a': 1} | types.MappingProxyType({'b': 2}) is allowed, because types.MappingProxyType has correct __or__ and __ror__ definitions.

Refs python/typeshed#10678

@sobolevn
Copy link
Member Author

However:

from typing import TypedDict, Any, TypeVar, Mapping

class Foo(TypedDict):
    key: int

foo: Foo = {
    "key": 1
}

reveal_type({'key': 1} | foo)

Causes:

ex.py:10: error: No overload variant of "__or__" of "dict" matches argument type "Foo"  [operator]
ex.py:10: note: Possible overload variants:
ex.py:10: note:     def __or__(self, dict[str, int], /) -> dict[str, int]
ex.py:10: note:     def [_T1, _T2] __or__(self, dict[_T1, _T2], /) -> dict[str | _T1, int | _T2]

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

test-data/unit/fixtures/typing-typeddict.pyi Outdated Show resolved Hide resolved
test-data/unit/fixtures/typing-typeddict.pyi Outdated Show resolved Hide resolved
mypy/checker.py Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

I'm not sure what's being fixed here, exactly; see my inline comments below

Comment on lines 3241 to 3270
[case testTypedDictWith__or__method]
from mypy_extensions import TypedDict

class Foo(TypedDict):
key: int

foo1: Foo = {'key': 1}
foo2: Foo = {'key': 2}

reveal_type(foo1 | foo2) # N: Revealed type is "TypedDict('__main__.Foo', {'key': builtins.int})"
reveal_type(foo1 | {'key': 1}) # N: Revealed type is "TypedDict('__main__.Foo', {'key': builtins.int})"
reveal_type(foo1 | {'key': 'a'}) # N: Revealed type is "typing.Mapping[builtins.str, builtins.object]"
reveal_type(foo1 | {}) # N: Revealed type is "typing.Mapping[builtins.str, builtins.object]"
[builtins fixtures/dict.pyi]
[typing fixtures/typing-typeddict.pyi]

[case testTypedDictWith__or__method_error]
from mypy_extensions import TypedDict

class Foo(TypedDict):
key: int

foo: Foo = {'key': 1}

foo | 1
[out]
main:8: error: No overload variant of "__or__" of "TypedDict" matches argument type "int"
main:8: note: Possible overload variants:
main:8: note: def __or__(self, Foo, /) -> Foo
main:8: note: def __or__(self, Mapping[str, object], /) -> Mapping[str, object]
Copy link
Member

Choose a reason for hiding this comment

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

These tests both pass with the master branch checked out

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what's being fixed here

Yes, this is just the extra test that was missing. Please, see #16244 for the bug the user had.

test-data/unit/check-typeddict.test Outdated Show resolved Hide resolved
@sobolevn
Copy link
Member Author

Done, we now support {} | typed_dict and tests are now more realistic.

@github-actions

This comment has been minimized.

Comment on lines 81 to 87
# TODO: re-enable after `__ror__` definition is fixed
# https://github.com/python/typeshed/issues/10678
# @overload
# def __ror__(self, __value: Self) -> Self: ...
# @overload
# def __ror__(self, __value: dict[str, Any]) -> dict[str, object]: ...
# supposedly incompatible definitions of __or__ and __ior__
Copy link
Member

@AlexWaygood AlexWaygood Oct 13, 2023

Choose a reason for hiding this comment

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

I don't understand this comment: see what I wrote in python/typeshed#10678 (comment). What do you think is broken in _TypedDict.__(r)or__?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's my understanding of this problem:

  1. dict.__or__ does not allow TypedDict, because it only allows dict (and TypedDict does not match it):
ex.py:7: error: No overload variant of "__or__" of "dict" matches argument type "Foo"  [operator]
ex.py:7: note: Possible overload variants:
ex.py:7: note:     def __or__(self, dict[str, int], /) -> dict[str, int]
ex.py:7: note:     def [_T1, _T2] __or__(self, dict[_T1, _T2], /) -> dict[str | _T1, int | _T2]
ex.py:7: note: Revealed type is "Any"
ex.py:8: error: No overload variant of "__or__" of "dict" matches argument type "Foo"  [operator]
ex.py:8: note: Possible overload variants:
ex.py:8: note:     def __or__(self, dict[str, str], /) -> dict[str, str]
ex.py:8: note:     def [_T1, _T2] __or__(self, dict[_T1, _T2], /) -> dict[str | _T1, str | _T2]
ex.py:8: note: Revealed type is "Any"
  1. mypy does the reverse checking for every __or__ operator, so even for foo | {'key': 1} it raises (without this PR):
ex.py:7: error: No overload variant of "__ror__" of "dict" matches argument type "Foo"  [operator]
ex.py:7: note: Possible overload variants:
ex.py:7: note:     def __ror__(self, dict[str, int], /) -> dict[str, int]
ex.py:7: note:     def [_T1, _T2] __ror__(self, dict[_T1, _T2], /) -> dict[str | _T1, int | _T2]
ex.py:7: note: Revealed type is "Any"

So, my problem definition is: dict.__or__ is not compatible with TypedDict and dict.__ror__ is also not compatible with TypedDict

Copy link
Member

Choose a reason for hiding this comment

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

Here is the snippet the OP was complaining about in #16244:

from typing import TypedDict

class Foo(TypedDict):
    key: int
    
foo: Foo = {"key": 1}
foo |= {"key": 3}

mypy reports:

error: Incompatible types in assignment (expression has type "dict[str, object]", variable has type "Foo")  [assignment]

https://mypy-play.net/?mypy=latest&python=3.11&gist=9927e1732f6fce4595d29a19493ae603

Note: mypy does not report that |= cannot be used between foo and {"key: 3}. It understands that the |= operator can be used between the two objects. However, it is inferring that the type of the foo variable after the |-ing is dict[str, object] rather than Foo, which would be an incompatible reassignment of the foo variable, since the foo variable has been declared to always have type Foo.

That indicates that mypy is using the second overload of _TypedDict.__or__ here to infer the type of foo after the |= operation, rather than _TypedDict.__ior__ or the first overload of _TypedDict.__or__. If it used the _TypedDict.__ior__ annotations, or the first overload of _TypedDict.__or__, it would be able to correctly infer that the type of foo after the |= operation is still Foo, which is a reassignment that's compatible with the declared type of the foo variable:

@overload
def __or__(self, __value: typing_extensions.Self) -> typing_extensions.Self: ...
@overload
def __or__(self, __value: dict[str, Any]) -> dict[str, object]: ...
@overload
def __ror__(self, __value: typing_extensions.Self) -> typing_extensions.Self: ...
@overload
def __ror__(self, __value: dict[str, Any]) -> dict[str, object]: ...
# supposedly incompatible definitions of __or__ and __ior__
def __ior__(self, __value: typing_extensions.Self) -> typing_extensions.Self: ... # type: ignore[misc]

I think the issue is that in this snippet, mypy is not able currently to infer using type context (bidirectional type inference) that the object on the right-hand-side of the |= operator can be considered to be an instance of Foo. If it did so, then it would select _TypedDict.__ior__ or the first overload of _TypedDict.__or__ to infer the type of foo after the |= operation, and would correctly understand that the type of the foo variable does not change as a result of the operation.

mypy/checkexpr.py Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

I didn't look at typeshed, and TBH I don't remember the discussion in 3.9, but FWIW I would expect dict.__or__() and dict.__ior__() to accept Mapping rather than just a dict (note TypedDict is a subtype of Mapping[str, object]).

But, more importantly, this solution is missing something that I would expect: mypy has a plugin hook for TypedDict.update() that provides a better signature for the method (and btw it gives a better type context for the argument, allowing e.g. dict literals with missing keys if possible). Did someone try to copy that plugin to TypedDict.__ior__() and/or TypedDict.__or__()?

@AlexWaygood
Copy link
Member

FWIW I would expect dict.__or__() and dict.__ior__() to accept Mapping rather than just a dict

@ilevkivskyi, that would be incorrect, and would lead to false negatives, because it does have to be an actual dict at runtime. See python/typeshed#10678

@sobolevn
Copy link
Member Author

sobolevn commented Oct 13, 2023

Did someone try to copy that plugin to TypedDict.ior() and/or TypedDict.or()?

Yes, I started exactly with this, but it didn't work. Because it is not only about TypedDict, but also about dict (TypedDict.__or__ calls dict.__ror__).

@ilevkivskyi
Copy link
Member

@AlexWaygood I am curious about what would be the motivation for that error at runtime? Also, can we allow Mapping at least for __ior__() or is it unsafe for some reasons as well?

Anyway, my second question about this PR still stands: if we are fixing this it definitely makes sense to have full parity between TypedDict.__ior__() and TypedDict.update().

Finally, it seems to me the original issue can be fixed by simply returning Self from TypedDict.__ior__(), is this true? If yes, maybe we can first focus on this part? Because in Python .__rX__() vs .__X__() are tricky (and not well compatible with static typing in general).

@AlexWaygood
Copy link
Member

it seems to me the original issue can be fixed by simply returning Self from TypedDict.__ior__()

It already does!

def __ior__(self, __value: typing_extensions.Self) -> typing_extensions.Self: ... # type: ignore[misc]

@ilevkivskyi
Copy link
Member

@sobolevn

Yes, I started exactly with this, but it didn't work.

Right, it is not about fixing the example in the issue (because it is oversimplified). I mean for something like this:

class TD(TypedDict):  # note both keys are required.
    x: int
    y: int

td: TD = {"x": 1, "y": 1}
td |= {"y": 2}

@AlexWaygood
Copy link
Member

Also, can we allow Mapping at least for __ior__()

We already do!

# dict.__ior__ should be kept roughly in line with MutableMapping.update()
@overload # type: ignore[misc]
def __ior__(self, __value: SupportsKeysAndGetItem[_KT, _VT]) -> Self: ...
@overload
def __ior__(self, __value: Iterable[tuple[_KT, _VT]]) -> Self: ...

@ilevkivskyi
Copy link
Member

@AlexWaygood

It already does!

OK, then I think I understand what is the issue. There is this rule about whether to use .__X__() or .__rX__() depending whether one type is a subtype of other. I guess this rule conflicts with the fact that at runtime TypedDict is just dict while from static point of view it is a Mapping. I guess we can then skip the reverse override specifically for TypedDicts.

I will take a more detailed look later.

@sobolevn
Copy link
Member Author

I guess we can then skip the reverse override specifically for TypedDicts.

This is what I did :)

@github-actions

This comment has been minimized.

# 2. Switch `dict.__or__` to `TypedDict.__or__` (the same from typing's perspective)
# 3. Do not allow `dict.__ror__` to be executed, since this is a special case
# This can later be removed if `typeshed` can do this without special casing.
# https://github.com/python/mypy/pull/16249
Copy link
Member

Choose a reason for hiding this comment

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

So do you have some specific signature(s) in mind? If not, this comment should not be here. (FWIW from what @AlexWaygood explained, it looks like the signatures are already as good as they can reasonable be.)

@ilevkivskyi
Copy link
Member

This is what I did

I looked a bit more at the implementation, and I don't like couple things:
First, what if there is a dict that is not a dict literal? Does this mean that we will now give an error? Like in

class Foo(TypedDict):
    x: int
    y: int

foo: Foo
x: dict[str, int]
y = foo | x  # this should be allowed

and in similar other combinations. Even if this works for some reason, please add test cases for various scenarios where dict is not a literal.
Second, I don't like constructing new synthetic expressions (unless really necessary). Can we instead have a three-valued flag use_reverse that would take values ALWAYS, NEVER, and DEFAULT? The first two will then be used for <dict literal> | TypedDict and TypedDict | <dict literal> IIUC.

@sobolevn
Copy link
Member Author

Done! I've added test cases with dict[...] type. Also added ALWAYS/NEVER/DEFAULT argument as you suggested.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

A small batch of remaining comments, feel free to merge after addressing them.

mypy/checkexpr.py Outdated Show resolved Hide resolved
mypy/checkexpr.py Outdated Show resolved Hide resolved
mypy/checkexpr.py Outdated Show resolved Hide resolved
mypy/checkexpr.py Outdated Show resolved Hide resolved
test-data/unit/check-typeddict.test Show resolved Hide resolved
@sobolevn
Copy link
Member Author

@ilevkivskyi I've added a plugin support, please review again :)

@github-actions
Copy link
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@sobolevn sobolevn merged commit 8236c93 into master Oct 23, 2023
18 checks passed
@sobolevn sobolevn deleted the issue-16244 branch October 23, 2023 12:52
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.

TypedDict errors on use of merge operator |= even if keys are correct
4 participants