Skip to content

Dictionary accepting Mapping in a merge operation #11264

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

Closed
jzazo opened this issue Jan 9, 2024 · 5 comments
Closed

Dictionary accepting Mapping in a merge operation #11264

jzazo opened this issue Jan 9, 2024 · 5 comments

Comments

@jzazo
Copy link
Contributor

jzazo commented Jan 9, 2024

Hi. I would like to propose that a dictionary accepts a mapping to the or operation. Something like:

class dict(MutableMapping[_KT, _VT]):
    ...
        @overload
        def __or__(self, __value: Mapping[_KT, _VT]) -> dict[_KT, _VT]: ...
        @overload
        def __or__(self, __value: Mapping[_T1, _T2]) -> dict[_KT | _T1, _VT | _T2]: ...

It would be useful in scenarios like the following:

from collections.abc import Mapping
from typing import cast

a = cast(Mapping[str, int], {"a": 1})
{"b": 2} | a

At this point, pyright complains with # error: Operator "|" not supported for types "dict[str, int]" and "Mapping[str, int]".
I referenced this problem in this issue. Thanks.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 9, 2024

That would be incorrect, however, because not all mappings can be merged with a dict using the | operator: it's not part of the collections.abc.Mapping interface. We used to define dict.__or__ in the way you propose above, but we changed it after a user complained that their type checker failed to spot a bug in their code. See #10678 and #10679.

@jzazo
Copy link
Contributor Author

jzazo commented Jan 9, 2024

I don't understand what you mean with it's not part of the Mapping interface. dict | Mapping -> dict and this can be guaranteed regardless of the concrete mapping instance. And Mapping | dict -> ? because the concrete implementation of Mapping will take precedence over the dict.__ror__ method. Am I wrong?

I was proposing a change on the __or__ method, not on the __ror__ method. Thanks and let me know if I overlooked something.

@srittau
Copy link
Collaborator

srittau commented Jan 10, 2024

As pointed out by Alex in #10678, dict.__or__ explicitly checks whether the other object is a dict:

https://github.com/python/cpython/blob/5d384b0468b35b393f8ae2d3149d13ff607c9501/Objects/dictobject.c#L3713C1-L3728

static PyObject *
dict_or(PyObject *self, PyObject *other)
{
    if (!PyDict_Check(self) || !PyDict_Check(other)) {
        Py_RETURN_NOTIMPLEMENTED;
    }
    /* ... */
}

The following code fails at runtime:

from collections.abc import Mapping

class MyMapping(Mapping):
    def __getitem__(self, i):
        return 42
    def __iter__(self):
        return iter([42])
    def __len__(self):
        return 1

{} | MyMapping()
Traceback (most recent call last):
  File "foo.py", line 11, in <module>
    {} | MyMapping()
    ~~~^~~~~~~~~~~~~
TypeError: unsupported operand type(s) for |: 'dict' and 'MyMapping'

@srittau srittau closed this as not planned Won't fix, can't repro, duplicate, stale Jan 10, 2024
@jzazo
Copy link
Contributor Author

jzazo commented Jan 10, 2024

Thanks @srittau for explaining.

@JelleZijlstra
Copy link
Member

And just in case you were looking for a workaround {"b": 2, **a} works on any Mapping and should be accepted by type checkers.

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

No branches or pull requests

4 participants