type `Serializer` as generic #374

davidism · 2024-04-13T20:20:17Z

Serializer now takes one generic parameter, either str or bytes, to define what dumps returns rather than a str | bytes union.

The default_serializer attribute and serializer argument are a generic protocol with the same parameter. The protocol requires an object that has a loads method and a dumps method that returns the appropriate type.

URLSafeSerizlizer is typed to always return str. This makes it more convenient as the user code no longer needs to do extra work to satisfy the type checker.

s = URLSafeSerializer()
data = s.dumps(...)

fixes #347

davidism · 2024-04-13T20:23:52Z

Hmm, mypy somehow knows that Serializer().dumps() returns str, even without an annotation. pyright does not though:

from itsdangerous import Serializer

s = Serializer("secret")
data = s.dumps("test")
reveal_type(data)

Mypy reveals str, pyright reveals Unknown unless you do s: Serializer[str] = Serializer().

davidism · 2024-04-13T20:30:40Z

OK, figured out how to get mypy and pyright to understand that the default is Serializer[str] if no serializer argument is provided. Used an overload that specified the type of self. Now both tools reveal str in the example above.

davidism · 2024-04-13T20:42:06Z

This doesn't quite make things nice for typing users, as now json and pickle are not "valid" as values to serializer since neither mypy or pyright treat modules with functions as fulfilling a protocol. Additionally, pyright is strict about the argument names for protocols. So you're basically forced to write a wrapper to convince the tools that a module or conforms to the protocol, or use an ignore and an explicit type parameter on assignment.

Daverball

Already looks pretty good, it's mostly some subtle variance issues, that weren't quite right.

Daverball · 2024-04-13T20:31:31Z

src/itsdangerous/serializer.py

@@ -10,13 +10,20 @@
 from .signer import _make_keys_list
 from .signer import Signer

+_TAnyStr = t.TypeVar("_TAnyStr", str, bytes, covariant=True)


Suggested change

_TAnyStr = t.TypeVar("_TAnyStr", str, bytes, covariant=True)

I would get rid of this, just use t.AnyStr, this actually needs to be invariant. Alternatively you could replace it with a typing_extensions.TypeVar and set default=str instead of covariant=True, that may lead to more robust results compared to relying on just the overloads on __init__.

I got errors from mypy that it had to be covariant for class Serializer(t.Generic[t.AnyStr])

That was because you weren't using it for the payload argument.

After testing again I'm not getting a covariance error from mypy anymore when using AnyStr, not sure what was going on before.

For Protocol mypy does a variance calculation and gives an error if the variance doesn't match. Since you previously only used _TAnyStr in return type annotations it was calculated as covariant, as soon as you added an argument that uses the same TypeVar the variance calculation changed to invariant.

That's why it's sometimes dangerous to re-use the same TypeVar between a Protocol and a Generic, since the variance may be wrong for the Generic, but mypy does not do a variance calculation for Generic. PEP-695 should make this less of a problem in the future, since type vars will generally be scoped to the generic itself, rather than in global scope and all they use auto-variance by default, so you don't have to think about it.

Daverball · 2024-04-13T20:40:24Z

src/itsdangerous/serializer.py

+class _PDataSerializer(t.Protocol[_TAnyStr]):
+    def loads(self, payload: str | bytes) -> t.Any: ...
+    def dumps(self, obj: t.Any, **kwargs: t.Any) -> _TAnyStr: ...


Suggested change

class _PDataSerializer(t.Protocol[_TAnyStr]):

def loads(self, payload: str | bytes) -> t.Any: ...

def dumps(self, obj: t.Any, **kwargs: t.Any) -> _TAnyStr: ...

class _PDataSerializer(t.Protocol[t.AnyStr]):

def loads(self, payload: t.AnyStr, /) -> t.Any: ...

def dumps(self, obj: object, /) -> t.AnyStr: ...

You are providing an upper bound here, so you don't want to make the argument list too strict.

For loads you can't use str | bytes because parameters are contravariant, so that would require each serializer to accept both str and bytes. Exactly because parameters are contravariant you can satisfy a function that expects str with a supertype of str, such as a union with bytes.

As for dumps, IIRC unless you specifically use *args: Any, **kwargs: Any neither mypy nor pyright will treat this as a gradual signature, that allows serializers with arbitrary extra arguments, it will instead require each serializer function to take arbitrary arguments. This means you will have to ignore a type error when you pass in serializer_kwargs, but that's a hit you will have to take until we have something like a TypeVarMapping to pass just kwargs along, ParamSpec doesn't really work here.

Yep, after making these changes mypy does accept json and pickle as valid.

pyright is still not happy though. It still insists that modules do not satisfy the protocol.

Daverball · 2024-04-13T20:42:40Z

src/itsdangerous/serializer.py

+    def dumps(self, obj: t.Any, **kwargs: t.Any) -> _TAnyStr: ...
+
+
+def is_text_serializer(serializer: _PDataSerializer[t.Any]) -> bool:


You could make this a TypeGuard, although it's probably better to wait for TypeIs support to arrive, since that will yield better results. Although you could make it TypeGuard[_PDataSerializer[str]] for now and add a comment to change it to TypeIs later.

Daverball · 2024-04-13T20:52:42Z

src/itsdangerous/serializer.py

@@ -71,7 +78,7 @@ class Serializer:
    #: The default serialization module to use to serialize data to a
    #: string internally. The default is :mod:`json`, but can be changed
    #: to any object that provides ``dumps`` and ``loads`` methods.
-    default_serializer: t.Any = json
+    default_serializer: _PDataSerializer[_TAnyStr] = json  # type: ignore[assignment]


Suggested change

default_serializer: _PDataSerializer[_TAnyStr] = json # type: ignore[assignment]

default_serializer: _PDataSerializer[str] = json

This seems more sane to me, if you're worried about subclasses providing a bytes serializer as default, you could change it to _PDataSerializer[t.Any] or a very long-winded t.Union[_PDataSerializer[str], _PDataSerializer[bytes]]. As a class attribute it shouldn't really be generic, that's prone to weird errors.

Daverball · 2024-04-13T20:53:25Z

src/itsdangerous/serializer.py

@@ -135,7 +174,9 @@ def secret_key(self) -> bytes:
        """
        return self.secret_keys[-1]

-    def load_payload(self, payload: bytes, serializer: t.Any | None = None) -> t.Any:
+    def load_payload(
+        self, payload: bytes, serializer: _PDataSerializer[_TAnyStr] | None = None


Suggested change

self, payload: bytes, serializer: _PDataSerializer[_TAnyStr] | None = None

self, payload: bytes, serializer: _PDataSerializer[t.Any] | None = None

The type of the passed in serializer doesn't matter, since it is re-checked.

Daverball · 2024-04-13T20:55:05Z

src/itsdangerous/timed.py

+_TAnyStr = t.TypeVar("_TAnyStr", str, bytes, covariant=True)
+


Suggested change

_TAnyStr = t.TypeVar("_TAnyStr", str, bytes, covariant=True)

Same thing here

Daverball · 2024-04-13T20:55:16Z

src/itsdangerous/timed.py

@@ -166,7 +168,7 @@ def validate(self, signed_value: str | bytes, max_age: int | None = None) -> boo
            return False


-class TimedSerializer(Serializer):
+class TimedSerializer(Serializer[_TAnyStr]):


Suggested change

class TimedSerializer(Serializer[_TAnyStr]):

class TimedSerializer(Serializer[t.AnyStr]):

Daverball · 2024-04-13T20:57:19Z

These variance issues are why you're getting complaints for json and pickle.

davidism · 2024-04-13T21:22:05Z

Continued in #376.

davidism added this to the 2.2.0 milestone Apr 13, 2024

davidism mentioned this pull request Apr 13, 2024

Consider making Serializer generic in t.AnyStr for type checking to avoid overly ambiguous return types #347

Closed

type Serializer as generic

01001c6

davidism force-pushed the generic-serializer branch from dba89a7 to 01001c6 Compare April 13, 2024 20:28

davidism added the typing label Apr 13, 2024

davidism merged commit 385c0eb into main Apr 13, 2024
11 checks passed

davidism deleted the generic-serializer branch April 13, 2024 20:55

Daverball reviewed Apr 13, 2024

View reviewed changes

davidism mentioned this pull request Apr 13, 2024

improve generic typing #376

Closed

github-actions bot locked as resolved and limited conversation to collaborators Apr 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

type `Serializer` as generic #374

type `Serializer` as generic #374

davidism commented Apr 13, 2024 •

edited

davidism commented Apr 13, 2024

davidism commented Apr 13, 2024

davidism commented Apr 13, 2024 •

edited

Daverball left a comment

Daverball Apr 13, 2024

davidism Apr 13, 2024 •

edited

Daverball Apr 13, 2024

davidism Apr 13, 2024

Daverball Apr 13, 2024

Daverball Apr 13, 2024 •

edited

davidism Apr 13, 2024

davidism Apr 13, 2024 •

edited

Daverball Apr 13, 2024

Daverball Apr 13, 2024

Daverball Apr 13, 2024

Daverball Apr 13, 2024

Daverball Apr 13, 2024

Daverball commented Apr 13, 2024

davidism commented Apr 13, 2024

		def dumps(self, obj: t.Any, **kwargs: t.Any) -> _TAnyStr: ...


		def is_text_serializer(serializer: _PDataSerializer[t.Any]) -> bool:

	default_serializer: _PDataSerializer[_TAnyStr] = json # type: ignore[assignment]
	default_serializer: _PDataSerializer[str] = json

	self, payload: bytes, serializer: _PDataSerializer[_TAnyStr] \| None = None
	self, payload: bytes, serializer: _PDataSerializer[t.Any] \| None = None

		_TAnyStr = t.TypeVar("_TAnyStr", str, bytes, covariant=True)

	class TimedSerializer(Serializer[_TAnyStr]):
	class TimedSerializer(Serializer[t.AnyStr]):

type Serializer as generic #374

type Serializer as generic #374

Conversation

davidism commented Apr 13, 2024 • edited

davidism commented Apr 13, 2024

davidism commented Apr 13, 2024

davidism commented Apr 13, 2024 • edited

Daverball left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidism Apr 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daverball Apr 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidism Apr 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daverball commented Apr 13, 2024

davidism commented Apr 13, 2024

type `Serializer` as generic #374

type `Serializer` as generic #374

davidism commented Apr 13, 2024 •

edited

davidism commented Apr 13, 2024 •

edited

davidism Apr 13, 2024 •

edited

Daverball Apr 13, 2024 •

edited

davidism Apr 13, 2024 •

edited