Annotations for nacl.bindings.crypto_box

[DMRobertson]

This introduced a few errors in nacl.public which I wasn't sure how to
suppress. AFAICS both will raise a TypeError at run-time, so I
suppressed mypy's errors. I wasn't completely satisfied with this; I
very much welcome input here! (cc @Dreamsorcerer)

Box._shared_key

The first error mypy reported was

src/nacl/public.py:193: error: Incompatible types in assignment (expression has type "None", variable has type "bytes")  [assignment]

referring to self._shared_key. I resolved this by marking
_shared_key: Optional[bytes] to cover both cases in the constructor.
But doing this introduced two new errors:

src/nacl/public.py:236: error: Argument 3 to "crypto_box_afternm" has incompatible type "Optional[bytes]"; expected "bytes"  [arg-type]
src/nacl/public.py:278: error: Argument 3 to "crypto_box_open_afternm" has incompatible type "Optional[bytes]"; expected "bytes"  [arg-type]

Which correspond to the annotations I added to crypto_box_afternm and
crypto_box_open_afternm's k argument. Both docstrings say that k
should be a bytes instance, and go on to inspect len(k). Therefore,
calling this with k=None will raise a TypeError.

SealedBox._private_key

The second of the original two errors was

src/nacl/public.py:382: error: Argument 3 to "crypto_box_seal_open" has incompatible type "Optional[bytes]"; expected "bytes"  [arg-type

because self._private_key may be None. In this instance there is an
explicit ensure check in crypto_box_seal_open which will raise a
TypeError.

I also adjusted the SealedBox docstring to match its __init__ method,
which appears to be the convention (judging by the other classes in this
file).

[DMRobertson]

I also noticed that bytes(Box(None, None)) is None, which surprised me and might cause further typechecking pain down the line. (See https://github.com/DMRobertson/pynacl/blob/8a0cf857af989cd4b25019542da5cfafe945419f/src/nacl/public.py#L180-L196)

I think the intention is that one either creates a box by providing both keys to __init__; or else one uses decode. If everyone plays by those rules, then _shared_key is never None. But I'm not quite sure how to convince mypy of this!

[Dreamsorcerer]

Box._shared_key

Am I right in thinking that None is only supported for the classmethod at line 201 (looks like you have the same conclusion in your last comment)? Which then sets the shared_key separately. So, the best solution would be to remove the support for None and refactor that classmethod. I can't see any other possible use for it, it just looks like a hack to make that classmethod work.

Not sure I have a good suggestion on how to make that work correctly. Maybe adding support for a shared_key argument to __init__()?.
But, at worst, we could add annotations to __init__() (without None), remove the else (knowing that we will set it in the classmethod), and add a type: ignore in the classmethod (where the actual hack is happening).

[Dreamsorcerer]

Looks like this is a legitimate error and we could be more explicit about it by checking at the start of the function:

if self._private_key is None:
    raise TypeError("Must be initialised with a PrivateKey.")

Or similar... I don't think allowing the lower TypeError to propogate is very clear what has been done wrong.

[Dreamsorcerer]

If we wanted to provide much better type safety though, then this class could be refactored into 2 clases, or a Generic class, with this method only existing on the PrivateKey version.

e.g. Maybe something along these lines (totally untested):

_Key = TypeVar("_Key", PrivateKey, PublicKey)
class SealedBox(Generic[_Key]):
    def __init__(self, recipient_key: _Key): ...
    def encrypt(self: SealedBox[PrivateKey], ...): ...

[reaperhulk]

Let's separate initial type hinting from changing PyNaCl's API to work better in a type hinted world. The latter is desirable and we should do it where appropriate, but for the moment we're looking for full type hinting of existing surface area (with whatever drawbacks that surface area has).

I'm okay with either the type check @Dreamsorcerer proposes (although you'll need to add a test for that case) or a simple assert self._private_key is not None. The latter isn't actually safe since you can turn off runtime assertions, but that's no worse than the current API and teaches mypy what the expected type is at that point.

[reaperhulk]

It would also be possible to change the classmethod to use cls.__new__(cls) and remove the allowance for None in __init__. The risk of that approach is that if other logic is added to __init__ in the future then the classmethod won't get that for free, but I'm okay with that if we want to do it.

[DMRobertson]

Let's separate initial type hinting from changing PyNaCl's API to work better in a type hinted world.

Agreed: I'm trying to avoid touching the logic and machinery where possible.

I'm okay with either the type check @Dreamsorcerer proposes (although you'll need to add a test for that case)

I think test_sealed_box_public_key_cannot_decrypt covers this already---do you agree?

I'd still like to add a more helpful exception message while I'm here. Library style seem to be to use ensure(...), but I don't think mypy will understand the guarantees it enforces without help from a plugin.

[DMRobertson]

I also like the use of Generic, but maybe that's best saved for when we come to tackle public.py proper.

[DMRobertson]

I couldn't resist. It does spot the following error:

SealedBox(PublicKey(b"publickey")).decrypt(b"ciphertext")

src/nacl/public.py:404: error: Invalid self argument "SealedBox[PublicKey]" to attribute function "decrypt" with type "Callable[[SealedBox[PrivateKey], bytes, Type[_Encoder]], Any]"  [misc]

[Dreamsorcerer]

Library style seem to be to use ensure(...), but I don't think mypy will understand the guarantees it enforces without help from a plugin.

Yep, it won't work with mypy. It also seems less readable and longer code than just doing it the standard way, so I'd suggest ditching that function personally.

[DMRobertson]

This should have been added in the following commit; apologies for the poor hygiene here.