frozendict should be covariant on V, like Mapping. #93

asford · 2023-12-22T05:40:34Z

Immutable collections, like Sequence, tuple and frozendict are covariant in their contained types.
The frozendict base interface, Mapping[K, V], is covariant in V.

Update the frozendict .pyi stubs to also specify a covariant V for frozendict[K,V].
This enables us to assign collections-of-subtypes to frozendict generics,
which currently raises a type error to value type invariance.

For example:

string_vals: frozendict[str, str] = frozendict(a="foo")
vals: frozendict[str, Hashable] = string_vals # should be allowed

Currently raises a type error:

Expression of type "frozendict[str, str]" cannot be assigned to declared type "frozendict[str, Hashable]"
  "frozendict[str, str]" is incompatible with "frozendict[str, Hashable]"
    Type parameter "V@frozendict" is invariant, but "str" is not the same as "Hashable"

but is a valid assignment.

asford · 2023-12-22T05:42:15Z

@Marco-Sulla this is a minor fix for the stubs added in #70.

Thanks for the great work on this project.

Marco-Sulla · 2023-12-22T19:56:23Z

dict is invariant too.

from collections.abc import Hashable
string_vals: dict[str, str] = dict(a="foo")
vals: dict[str, Hashable] = string_vals

mypy output:

test/typed.py:38: error: Incompatible types in assignment (expression has type "Dict[str, str]", variable has type "Dict[str, Hashable]")  [assignment]
test/typed.py:38: note: "Dict" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
test/typed.py:38: note: Consider using "Mapping" instead, which is covariant in the value type

asford · 2023-12-22T23:08:55Z

dict implements MutableMapping, as it's a mutable container and is therefore invariant.

frozendict is a somewhat special case, as it implements the (covariant) Mapping interface.

By analogy, frozendict is to dict as tuple is to list or frozenset is to set. It's a (essentially covariant) immutable container.

I think you've extended this with non-covariant-ish update/set/ror methods, however this can be appropriately typed using a non-covariant V2 parameter returning a union type, as you've done here, because it does not modify the source frozendict.

Would you be open to reopening with updates to allow covariant behavior on V, and non-covariant extensions? This is actually a pretty useful behavior for frozen dataclasses where we may want to have a concrete, dict-like property.

Marco-Sulla · 2023-12-23T09:18:54Z

dict implements MutableMapping, as it's a mutable container and is therefore invariant.

Mh, you're right. This works:

from collections.abc import Hashable
string_vals: tuple[str] = ("foo", )
vals: tuple[Hashable] = string_vals

For now I simply reopen this, but I check it more deeply later.

Marco-Sulla · 2023-12-23T09:21:02Z

(Personal rant: Python typing is a mess IMHO, and I repeat IMHO. The syntax is ugly, is quite difficult to implement and to debug.)

Marco-Sulla · 2023-12-23T12:37:03Z

Could you also add this to test/typed.py?

from collections.abc import Hashable
string_vals: frozendict[str, str] = frozendict(a="foo")
vals: frozendict[str, Hashable] = string_vals

asford · 2023-12-24T23:07:20Z

(Personal rant: Python typing is a mess IMHO, and I repeat IMHO. The syntax is ugly, is quite difficult to implement and to debug.)

I definitely ...don't disagree.... 😉

Thanks for pointing me to test/typed.py. I took the liberty of:

Updating test/typed.py file to use assert_type, which documents the expected type of a statement and verifies this type via the checker.
Adding typing checks with pyright and mypy (if installed) to mytest.sh

In doing so, I found a few small bugs in __init__.pyi. I've updated these type signatures to clarify the covariance issues we described above as well as tightened up the type declarations for the extended set and setdefault methods.

This fixes a few errors I was seeing in type inference under pylance in vscode.

src/frozendict/__init__.pyi

asford · 2023-12-31T16:57:44Z

Fantastic, thanks for the review.

I'm happy to setup a quick tox-based test config that includes your existing suite and these type tests if you'd like.

It might help if you have other contributors come by and could be run in actions.

Marco-Sulla · 2023-12-31T22:48:05Z

It might help if you have other contributors

I can contribute, but please, let me know about the open points, especially the ones about __init__.pyi

asford · 2024-01-01T03:01:39Z

I'm not sure I understand? There aren't any review comments open right now. The test suite with type assertions is passing on this branch w/ mypy and pyright.

Marco-Sulla · 2024-01-01T09:10:35Z

There aren't any review comments open right now.

Emh. Can you scroll this discussion up? :D

Marco-Sulla · 2024-01-01T09:12:33Z

In particular all the "Why you removed" etc

asford · 2024-01-16T21:25:04Z

In particular all the "Why you removed" etc

I don't see any review comments, maybe they're in draft?

Marco-Sulla · 2023-12-25T10:32:27Z

src/frozendict/__init__.pyi

 K2 = TypeVar("K2")
 V2 = TypeVar("V2")
-SelfT = TypeVar("SelfT", bound=frozendict[K, V])


Why you removed bound?

Marco-Sulla · 2023-12-25T10:33:17Z

src/frozendict/__init__.pyi


 class frozendict(Mapping[K, V]):
    @overload
    def __new__(cls: Type[SelfT]) -> SelfT: ...
    @overload
-    def __new__(cls: Type[SelfT], **kwargs: V) -> frozendict[str, V]: ...


Why you removed many Type[SelfT]

Marco-Sulla · 2023-12-25T10:34:26Z

src/frozendict/__init__.pyi

-    def key(self: SelfT, index: int) -> K: ...
-    def value(self: SelfT, index: int) -> V: ...
-    def item(self: SelfT, index: int) -> Tuple[K, V]: ...
-    @overload


Why you removed the overload?

Marco-Sulla · 2023-12-25T10:38:41Z

test/typed.py

+from collections.abc import Hashable
+from typing import ItemsView, Iterator, KeysView, Mapping, ValuesView
+
+from typing_extensions import assert_type


Hey, that's cool! Can this file be removed and this code added to pytest suite?

Marco-Sulla · 2023-12-25T10:40:06Z

mytest.sh

+    popd
+fi
+
+if command -v pyright &> /dev/null


What the d@&^ is pyright? :D

Marco-Sulla · 2023-12-25T10:41:24Z

mytest.sh

@@ -1,3 +1,15 @@
 rm test/core.*

 ./test/debug.py && pytest
+
+if command -v mypy &> /dev/null


I think this code is obsolete now that you changed typed.py. In particular if the code of typed.py will be moved in pytest

Marco-Sulla · 2024-01-17T18:19:46Z

Mh maybe I forgot to submit review...

Marco-Sulla · 2024-03-31T21:40:30Z

Superseeded by ebbc70b and subsequent commits.

frozendict should be covariant on V, like Mapping

b781f8b

Marco-Sulla closed this Dec 22, 2023

Marco-Sulla added the Status: Not a bug This doesn't seem right label Dec 22, 2023

Marco-Sulla reopened this Dec 23, 2023

Marco-Sulla added Type: Defect Something works, but can work better Priority: Low Not a big problem... Effort: Low easy task Needs: Test Hey, it compiles! Ship it! and removed Status: Not a bug This doesn't seem right labels Dec 23, 2023

asford added 2 commits December 24, 2023 22:56

Fix minor errors in __init__.pyi, add assertions to test/typed.py

a6ec52d

Add typing tests to mytest.sh

6fac176

asford and others added 2 commits December 24, 2023 23:08

Tidy unused import in __init__.pyi

37069fe

I usually run mytest with frozendict installed in a venv

ffd2b42

Marco-Sulla reviewed Dec 25, 2023

View reviewed changes

src/frozendict/__init__.pyi Outdated Show resolved Hide resolved

I prefer this style :)

bb7455f

Merge branch 'master' into master

2a16ab5

Marco-Sulla requested changes Jan 17, 2024

View reviewed changes

Marco-Sulla closed this Mar 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

frozendict should be covariant on V, like Mapping. #93

frozendict should be covariant on V, like Mapping. #93

asford commented Dec 22, 2023

asford commented Dec 22, 2023

Marco-Sulla commented Dec 22, 2023 •

edited

Loading

asford commented Dec 22, 2023

Marco-Sulla commented Dec 23, 2023

Marco-Sulla commented Dec 23, 2023

Marco-Sulla commented Dec 23, 2023

asford commented Dec 24, 2023

asford commented Dec 31, 2023

Marco-Sulla commented Dec 31, 2023

asford commented Jan 1, 2024

Marco-Sulla commented Jan 1, 2024

Marco-Sulla commented Jan 1, 2024

asford commented Jan 16, 2024

Marco-Sulla Dec 25, 2023

Marco-Sulla Dec 25, 2023

Marco-Sulla Dec 25, 2023

Marco-Sulla Dec 25, 2023

Marco-Sulla Dec 25, 2023

Marco-Sulla Dec 25, 2023

Marco-Sulla commented Jan 17, 2024

Marco-Sulla commented Mar 31, 2024

frozendict should be covariant on V, like Mapping. #93

frozendict should be covariant on V, like Mapping. #93

Conversation

asford commented Dec 22, 2023

asford commented Dec 22, 2023

Marco-Sulla commented Dec 22, 2023 • edited Loading

asford commented Dec 22, 2023

Marco-Sulla commented Dec 23, 2023

Marco-Sulla commented Dec 23, 2023

Marco-Sulla commented Dec 23, 2023

asford commented Dec 24, 2023

asford commented Dec 31, 2023

Marco-Sulla commented Dec 31, 2023

asford commented Jan 1, 2024

Marco-Sulla commented Jan 1, 2024

Marco-Sulla commented Jan 1, 2024

asford commented Jan 16, 2024

Marco-Sulla Dec 25, 2023

Choose a reason for hiding this comment

Marco-Sulla Dec 25, 2023

Choose a reason for hiding this comment

Marco-Sulla Dec 25, 2023

Choose a reason for hiding this comment

Marco-Sulla Dec 25, 2023

Choose a reason for hiding this comment

Marco-Sulla Dec 25, 2023

Choose a reason for hiding this comment

Marco-Sulla Dec 25, 2023

Choose a reason for hiding this comment

Marco-Sulla commented Jan 17, 2024

Marco-Sulla commented Mar 31, 2024

Marco-Sulla commented Dec 22, 2023 •

edited

Loading