Typing: replace "type" with specific metaclass #2181

lafrech · 2023-09-10T19:47:05Z

Feedback welcome.

deckar01 · 2023-09-11T00:04:34Z

It looks like there are two last anonymous type annotations that could be fixed. f8f4c8f

Changing the return type of dict_class() to type[Mapping] reveals that Mapping doesn't provide some of the dict interface we are using.

The _T TypeVar is hiding some of that incompatibility from the type checker also. serialize doesn't actually provide a generic pass through of the input type. It takes any dict type as input, and regardless of that type it returns an instance of the configured dict_class().

lafrech · 2023-11-21T19:57:53Z

I applied @deckar01's commit and rebased.

Guys, feel free to merge.

sloria

the changes in fields.py look right to me, but i'm less sure about the changes to schema.py. can you double check them?

sloria · 2023-11-21T22:16:07Z

src/marshmallow/schema.py

@@ -497,7 +498,7 @@ def _call_and_store(getter_func, data, *, field_name, error_store, index=None):
            return error.valid_data or missing
        return value

-    def _serialize(self, obj: _T | typing.Iterable[_T], *, many: bool = False):
+    def _serialize(self, obj: dict | typing.Iterable[dict], *, many: bool = False):


this isn't quite right, is it? obj can be any arbitrary object type or an iterable objects.

Per https://github.com/marshmallow-code/marshmallow/pull/2181/files#r1401344892, the issue was that _T was being used incorrectly in _deserialize. Moving it to load() would avoid the need to change this TypeVar.

Per #2181 (comment) obj should be Any.

sloria · 2023-11-21T22:16:37Z

src/marshmallow/schema.py

@@ -510,7 +511,7 @@ def _serialize(self, obj: _T | typing.Iterable[_T], *, many: bool = False):
        if many and obj is not None:
            return [
                self._serialize(d, many=False)
-                for d in typing.cast(typing.Iterable[_T], obj)
+                for d in typing.cast(typing.Iterable[dict], obj)


related to comment above: i think this was already correct

~~Per https://github.com/marshmallow-code/marshmallow/pull/2181/files#r1401346431, yes, this can be reverted.~~

Per #2181 (comment) this should be Any and the cast is not necessary.

sloria · 2023-11-21T22:17:11Z

src/marshmallow/schema.py

@@ -584,7 +585,7 @@ def _deserialize(
        partial=None,
        unknown=RAISE,
        index=None,
-    ) -> _T | list[_T]:
+    ) -> dict | list[dict]:


related to above: _deserialize is expected to return an arbitrary object or list of objects, not dictionaries.

ret_d = self.dict_class() ... return ret_d implies that the return type of _deserialize() is the return type of dict_class() (Mapping). That change reveals that our usage of the return value actually depends on the dict interface. The fact that the type checker doesn't error on key access to a TypeVar which is effectively Any, seems like a mypy bug, but maybe it just needs explicit test coverage with a concrete type.

Enveloping occurs in _do_load(), so the return type of _deserialize() should not be constrained by the TypeVar. Now that I understand the purpose of the TypeVar it looks like it should be moved to load() rather than removed.

When I tested my proposed change mypy errored with "A function returning TypeVar should receive at least one argument containing the same TypeVar". I thought _T was trying to enforce a generic type between two methods, but it can't actually do that. The type vars were only used to cast list items in recursive calls.

Since none of these methods actually pass through an input type, Any should be used and those recursive cast operations are unnecessary.

lafrech · 2024-05-28T18:31:49Z

I tried to apply the changes discussed in comment. I'm not 100% sure I got everything right.

mypy seems to be happy with those changes.

lafrech · 2024-10-01T22:02:18Z

I just fixed _deserialize as per #2181 (comment).

I think I applied all suggested changes.

lafrech · 2024-10-07T20:39:05Z

src/marshmallow/schema.py

@@ -501,7 +502,9 @@ def _call_and_store(getter_func, data, *, field_name, error_store, index=None):
            return error.valid_data or missing
        return value

-    def _serialize(self, obj: _T | typing.Iterable[_T], *, many: bool = False):
+    def _serialize(
+        self, obj: typing.Any | typing.Iterable[dict], *, many: bool = False


obj: typing.Any | typing.Iterable[dict]

should have been

obj: typing.Any | typing.Iterable[typing.Any]

which anyway can be simplified as

obj: typing.Any

from mypy perspective at least, for a human | typing.Iterable[typing.Any] indicates a clearer intent about the many case.

I just force-pushed a modified commit with just obj: typing.Any.

lafrech assigned sloria and deckar01 Sep 10, 2023

lafrech force-pushed the typing_type branch from 60ec694 to 7385fd9 Compare September 10, 2023 20:01

deckar01 approved these changes Sep 11, 2023

View reviewed changes

lafrech force-pushed the typing_type branch from 7385fd9 to 160b495 Compare November 21, 2023 19:54

sloria requested changes Nov 21, 2023

View reviewed changes

lafrech force-pushed the typing_type branch from 160b495 to ca16dd1 Compare May 28, 2024 18:29

lafrech and others added 3 commits October 1, 2024 23:45

Typing: replace "type" with specific metaclass

457d9a7

Fix generic schema type hints

c914b2a

Fix _serialize type hints

eeba835

lafrech force-pushed the typing_type branch from ca16dd1 to 7371c3d Compare October 1, 2024 22:00

deckar01 self-requested a review October 3, 2024 16:27

deckar01 approved these changes Oct 3, 2024

View reviewed changes

sloria approved these changes Oct 7, 2024

View reviewed changes

lafrech commented Oct 7, 2024

View reviewed changes

Fix _deserialize type hints

4976d83

lafrech force-pushed the typing_type branch from 7371c3d to 4976d83 Compare October 7, 2024 20:40

lafrech merged commit 0452065 into dev Oct 9, 2024
9 checks passed

lafrech deleted the typing_type branch October 9, 2024 12:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typing: replace "type" with specific metaclass #2181

Typing: replace "type" with specific metaclass #2181

lafrech commented Sep 10, 2023

deckar01 commented Sep 11, 2023 •

edited

Loading

lafrech commented Nov 21, 2023

sloria left a comment

sloria Nov 21, 2023

deckar01 Nov 21, 2023 •

edited

Loading

deckar01 Nov 27, 2023

sloria Nov 21, 2023

deckar01 Nov 21, 2023 •

edited

Loading

deckar01 Nov 27, 2023

sloria Nov 21, 2023

deckar01 Nov 21, 2023 •

edited

Loading

deckar01 Nov 27, 2023

lafrech commented May 28, 2024

lafrech commented Oct 1, 2024

lafrech Oct 7, 2024

lafrech Oct 7, 2024 •

edited

Loading

Typing: replace "type" with specific metaclass #2181

Typing: replace "type" with specific metaclass #2181

Conversation

lafrech commented Sep 10, 2023

deckar01 commented Sep 11, 2023 • edited Loading

lafrech commented Nov 21, 2023

sloria left a comment

Choose a reason for hiding this comment

sloria Nov 21, 2023

Choose a reason for hiding this comment

deckar01 Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

deckar01 Nov 27, 2023

Choose a reason for hiding this comment

sloria Nov 21, 2023

Choose a reason for hiding this comment

deckar01 Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

deckar01 Nov 27, 2023

Choose a reason for hiding this comment

sloria Nov 21, 2023

Choose a reason for hiding this comment

deckar01 Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

deckar01 Nov 27, 2023

Choose a reason for hiding this comment

lafrech commented May 28, 2024

lafrech commented Oct 1, 2024

lafrech Oct 7, 2024

Choose a reason for hiding this comment

lafrech Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

deckar01 commented Sep 11, 2023 •

edited

Loading

deckar01 Nov 21, 2023 •

edited

Loading

deckar01 Nov 21, 2023 •

edited

Loading

deckar01 Nov 21, 2023 •

edited

Loading

lafrech Oct 7, 2024 •

edited

Loading