Add defaultdict.__or__; improve ChainMap.__or__ and UserDict.__or__

[eltoder]

No description provided.

[AlexWaygood]

Thanks! Looks pretty good, but a few nitpicks:

[AlexWaygood]

If you have a subclass of defaultdict, then __or__ will actually return an instance of the subclass, even if __or__ has not been overridden on the subclass (note this differs from how dict.__or__ behaves):

>>> from collections import defaultdict
>>> classFoo(defaultdict): ...
...
>>> type(Foo(int) | defaultdict(int))
<class '__main__.Foo'>

We can't express this fully without higher-kinded typevars (python/typing#548), but we can give a more specific return type in the specific case where a mapping that has the same type parameters is being __or__'d.

Also: the second parameter should be named value, not other:

>>> from collections import defaultdict
>>> help(defaultdict.__or__)
Help on wrapper_descriptor:
__or__(self, value, /)
Return self|value.

And it should be positional-only:

>>> from collections import defaultdict
>>> x = defaultdict(int)
>>> x.__or__(value={})
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: wrapper __or__() takes no keyword arguments

Putting all this together, I think the signature should be:

Suggested change

	def__or__(self, other: Mapping[_T1, _T2]) ->defaultdict[_KT|_T1, _VT|_T2]: ...
	def__ror__(self, other: Mapping[_T1, _T2]) ->defaultdict[_KT|_T1, _VT|_T2]: ...
	@overload
	def__or__(self, __value: Mapping[_KT, _VT]) ->Self: ...
	@overload
	def__or__(self, __value: Mapping[_T1, _T2]) ->defaultdict[_KT|_T1, _VT|_T2]: ...
	@overload
	def__or__(self, __value: Mapping[_KT, _VT]) ->Self: ..
	@overload
	def__ror__(self, __value: Mapping[_T1, _T2]) ->defaultdict[_KT|_T1, _VT|_T2]: ...

[JelleZijlstra]

Probably the type of the other parameter should be _typeshed.SupportsKeysAndGetItem, not Mapping, right?

[AlexWaygood]

Probably the type of the other parameter should be _typeshed.SupportsKeysAndGetItem, not Mapping, right?

Doesn't look like it?

>>> from collections import defaultdict
>>> classFoo:
... def__init__(self):
... self.data = {}
... defkeys(self):
... returnself.data.keys()
... def__getitem__(self, key):
... returnself.data[key]
...
>>> defaultdict(int) | Foo()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for |: 'collections.defaultdict' and 'Foo'

[eltoder]

@JelleZijlstra@AlexWaygood defaultdict actually only supports subclasses of dict: https://github.com/python/cpython/blob/388b5daa523b828dc0f7e2a1a6886bebc20833ba/Modules/_collectionsmodule.c#L2136
However, we cannot declare it here as dict, because dict is declared to accept Mapping in typeshed, so this causes a violation of Liskov substitution. This also seems wrong. The code only accept dicts: https://github.com/python/cpython/blob/388b5daa523b828dc0f7e2a1a6886bebc20833ba/Objects/dictobject.c#L3606
Quick demo:

>>>fromcollections.abcimportMapping>>>classD(Mapping):
def__getitem__(self, key):
raiseKeyErrordef__len__(self):
return0def__iter__(self):
returniter(())
>>> {"a": 1} |D()
Traceback (mostrecentcalllast):
File"<stdin>", line1, in<module>TypeError: unsupportedoperandtype(s) for|: 'dict'and'D'

[AlexWaygood]

@JelleZijlstra@AlexWaygood defaultdict actually only supports subclasses of dict:

Yes we had just come to the same conclusion :)

However, we cannot declare it here as dict, because dict is declared to accept Mapping in typeshed, so this causes a violation of Liskov substitution. This also seems wrong. The code only accept dicts: https://github.com/python/cpython/blob/388b5daa523b828dc0f7e2a1a6886bebc20833ba/Objects/dictobject.c#L3606

Good catch! Want to update dict.__(r)or__ in this PR as well?

[Akuli]

I don't know why this works, but it works. Mypy (with this PR) and Python somehow decide that defaultdict(int, {"foo": 1}) | {"bar": 2} and {"bar": 2} | defaultdict(int, {"foo": 1}) are defaultdicts, even though dict and defaultdict both define __or__ and __ror__.

[eltoder]

@Akuli defaultdict is a subclass of dict, so python prefers its method over dict's.

[AlexWaygood]

I don't know why this works, but it works. Mypy (with this PR) and Python somehow decide that defaultdict(int, {"foo": 1}) | {"bar": 2} and {"bar": 2} | defaultdict(int, {"foo": 1}) are defaultdicts, even though dict and defaultdict both define __or__ and __ror__.

When evaluating the expression x | y, Python will use type(y).__ror__ instead of type(x).__or__ iff both type(x).__or__ and type(y).__ror__ exist and type(y) is a subclass of type(x). Mypy's presumably aware of this and figures out the type accordingly :)

In all other cases, Python will use type(x).__or__ to evaluate x | y if type(x).__or__ and type(y).__ror__ are both defined.

[AlexWaygood]

Here's a full description of the nightmarish complexity Python goes through to evaluate x | y: https://snarky.ca/unravelling-binary-arithmetic-operations-in-python/

[Akuli]

Thanks. I suspected it might have something to do with subclassing, but was somehow too lazy to look it up :)

[eltoder]

@AlexWaygood It looks like the correct type for dict.__or__ has an unfortunate side-effect: now there's no nice way to type a function that does dict | os.environ. This works because _Environ defines __ror__, but the type is not public and one wouldn't want to put it in the function signature. IOW:

deffn1(lhs: dict[str, str], rhs: Mapping[str, str]) ->None:
lhs|rhs# does not type check nowdeffn2(lhs: dict[str, str], rhs: dict[str, str]) ->None:
lhs|rhs# type checks, but cannot be called with rhs=os.environ

Also, I got an error from mypy:

stdlib/collections/__init__.pyi:408: error: Signatures of "__ror__" of "defaultdict" and "__or__" of "dict[_T1, _T2]" are unsafely overlapping [misc]

which did not make sense to me, so I added ignore.

I'm thinking to revert the change to dict. WDYT?

[eltoder]

Maybe a better change would be to propose a change in cpython to use PyMapping_Check instead of PyDict_Check in dict and defaultdict. This is what almost all Python implemented __or__ methods do. This is inconsistent with set, though.

[JelleZijlstra]

Maybe a better change would be to propose a change in cpython to use PyMapping_Check instead of PyDict_Check in dict and defaultdict. This is what almost all Python implemented __or__ methods do. This is inconsistent with set, though.

That thought crossed my mind too, but it's not clear if it's right, and in any case we'd only make this change in 3.13+, so it doesn't really solve the problem of what to do in typeshed right now.

[AlexWaygood]

I'll try something by pushing to your PR branch and see if it fixes some of the primer errors

[JelleZijlstra]

As for environ, we do have stubs for the os._Environ type, and they define an __or__ that accepts Mapping. I haven't checked whether that's exactly correct.

I'd be OK with just accepting Mapping for now in defaultdict.__or__ and fixing the type in a separate PR.

[AlexWaygood]

Well, my idea didn't help anything, and it's not surprising that it didn't.

The issue isn't anything to do with our stubs for os._Environ.__(r)or__. The issue is that mypy has no way of knowing that the object being |'d is an instance of os._Environ, as they're just using Mapping as the type hint: https://github.com/systemd/mkosi/blob/84ba040dc048971f675c47f45b9b154e087e342e/mkosi/run.py#L227-L246C11

Arguably this is a true positive, as __or__ and __ror__ aren't included in the Mapping interface, so you can't guarantee that an arbitrary mapping can be |'d with a dictionary or an instance of os._Environ. But perhaps it makes it more annoying for users, for little gain. So maybe we should just stick with Mapping for dict.__(r)or__ and defaultdict.__(r)or__ for now.

[eltoder]

@AlexWaygood it is a true positive, but it is annoying to properly type a function like run. env: Mapping[str, PathString] seems logical, but actually needs to be "a type that can be or'ed to a dict", which is "anything that accepts dict in __ror__". I think we can do this with a protocol and stick it into typing_extensions or something, but it's a lot of work. A more conservative type of dict will disallow os.environ. A quick fix for that is dict | os._Environ, but that seems ugly.

[AlexWaygood]

Yeah, just go with Mapping for dict.__or__ and defaultdict.__or__ for now :)

[eltoder]

Done.

[AlexWaygood]

LGTM. Thanks!

[AlexWaygood]

By the way, a workflow nit for future typeshed PRs: we generally prefer to avoid force pushes at typeshed. They don't work so well with GitHub's UI, making it hard to see what changed in between commits and thereby making it harder to do incremental reviews. We squash everything into a single commit before merging, anyway, so there's no need to worry about a messy commit history in a PR :)

[eltoder]

Will note for future PRs.

[github-actions]

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