Skip to content

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

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
Daverball opened this issue Jun 13, 2023 · 6 comments · Fixed by #374
Labels
Milestone

Comments

@Daverball
Copy link
Contributor

Daverball commented Jun 13, 2023

Currently Serializer.dumps has an ambiguous return type (str | bytes) due to how the Serializer determines its own return type in __init__ however for some of the derived classes such as URLSafeSerializer we know statically that the return type will be str and not bytes.

If Serializer were generic we could perform this type narrowing when we inherit from Serializer by instead inheriting from Serializer[str]. That way we don't need to type:ignore or assert isinstance(a, str) everywhere we use a URLSafeSerializer and correctly expect to get a str back from dumps.

If you wanted to be a bit more fancy you could even define a generic Protocol for the custom serializer that's passed in __init__ (rather than use Any) and create overloads to return the correct Serializer type, i.e. if dumps on the Serializer that's passed in returns bytes, then it will return Serializer[bytes], the default with None can then return Serializer[str].

i.e. something like this: (it would require a bit more care due to the order of arguments, to properly match every possible case, but the basic idea should come across)

class Serializer(_t.Generic[_t.AnyStr]):

   @_t.overload
    def __init__(
        self: Serializer[str],
        secret_key: _t_secret_key,
        salt: _t_opt_str_bytes = ...,
        serializer: _t.Optional[_SerializerProtocol[str]] = ...,
        serializer_kwargs: _t_opt_kwargs = ...,
        signer: _t.Optional[_t_signer] = ...,
        signer_kwargs: _t_opt_kwargs = ...,
        fallback_signers: _t.Optional[_t_fallbacks] = ...,
    ): ...

   @_t.overload
    def __init__(
        self: Serializer[bytes],
        secret_key: _t_secret_key,
        salt: _t_opt_str_bytes,
        serializer: _SerializerProtocol[bytes],
        serializer_kwargs: _t_opt_kwargs = ...,
        signer: _t.Optional[_t_signer] = ...,
        signer_kwargs: _t_opt_kwargs = ...,
        fallback_signers: _t.Optional[_t_fallbacks] = ...,
    ): ...

   # fallback for when the passed in serializer can't be pattern matched
   @_t.overload
    def __init__(
        self,
        secret_key: _t_secret_key,
        salt: _t_opt_str_bytes,
        serializer: Any,
        serializer_kwargs: _t_opt_kwargs = ...,
        signer: _t.Optional[_t_signer] = ...,
        signer_kwargs: _t_opt_kwargs = ...,
        fallback_signers: _t.Optional[_t_fallbacks] = ...,
    ): ...

Environment:

  • Python version: 3.10
  • ItsDangerous version: 2.1.2
@davidism davidism added this to the 2.2.0 milestone Apr 13, 2024
@davidism
Copy link
Member

Any suggestion on how to handle the fact that Serializer.default_serializer is known to return str? If someone did Serializer().dumps(), we know it returns str since they didn't override default_serializer to something that returns bytes, but the user still has to do s: Serializer[str] = Serializer() to get the type checker to acknowledge that.

@Daverball
Copy link
Contributor Author

Daverball commented Apr 13, 2024

@davidism That's what the suggested overloads are for, the first overload will be matched when only the secret_key is given and since self is annotated as Serializer[str] that's what it will specialize as.

By now PEP-696 is already pretty well supported however, so you can also provide a default type on the TypeVar by importing from typing_extensions instead of typing.

@Daverball
Copy link
Contributor Author

I can take a stab at a PR, if the example on its own isn't illustrative enough.

@davidism
Copy link
Member

Most use cases do not involve providing a serializer argument, so that overload would almost never be helpful. I guess you mean an overload where if serializer is not given then self is Serializer[str]?

@davidism
Copy link
Member

I just opened a PR if you want to review it: #374

@davidism
Copy link
Member

Thanks, the override hint was what I needed, I did what I suggested and now mypy and pyright know the default is Serializer[str] if no serializer argument is given.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants