Add type annotations to `nacl`

DMRobertson commented 2 years ago

Fixes #660.

I went on a bit of an annotating spree and I think I managed to get all of the functions in nacl annotated. For the most part it was a case of propagating the existing type hints in the docstrings and reformatting them.

It's worth highlighting that I'm not a cryptography expert! All eyes on the changes are appreciated.

Methodology:

configure mypy to ignore nacl._sodium since that's the C extension(?)
run mypy --disallow-untyped-defs src/nacl to spot untyped functions
annotate them
rerun mypy and continue.

One thing I'm not sure about: to annotate the encoder arguments I made use of a typing.Protocol. That was added in Python 3.8; I pulled in typing_extensions so that it's available in Python 3.6 and 3.7. But having done that, I wasn't sure if adding a dependency was a good idea. Perhaps a try-except along these lines might be better?

try:
    from typing import Protocol
except ImportError:
    class Protocol: pass

I also ran mypy --strict out of curiosity.

It was mostly unhappy that I was returning expressions of the form ffi.buffer(foo, bar)[:]. From mypy's perspective these have type Any, but I was returning them from functions which had been marked as returning bool.
It was also unhappy about parts of Box.decode, but I don't think there was anything obviously wrong there---there's just some trickery in passing None to Box.__init__.
Lastly mypy spotted that SealedBox.decrypt could have self._private_key = None, which would contradict the type of crypto_box_seal_open's sk argument. This happens if the SealedBox is created with a PublicKey, not a PrivateKey. That probably makes sense: shouldn't be able to decrypt without a PrivateKey!

DMRobertson commented 2 years ago

I'm not sure what to make of the codecov failures. It notes that the line I've added isn't run in tests. Maybe replacing ... with pass will remedy that?

DMRobertson commented 2 years ago

I'm not sure what to make of the codecov failures. It notes that the line I've added isn't run in tests. Maybe replacing ... with pass will remedy that?

Or alternatively I could start annotating the tests so they pull in Encoder and SupportsBytes---but not sure that's actually useful.

DMRobertson commented 2 years ago

@Dreamsorcerer you mentioned in #660 that you'd be interested in annotations. Would you be able to have a look over these changes?

reaperhulk commented 2 years ago

Thanks for doing this work! There's a bunch here, but I think to make sure we don't get stalled and get this all reviewed quickly we need to add a mypy job (with whatever set of flags/config we need to have it passing), merge that, then incrementally add chunks of this PR so that it's more easily reviewable.

I'd suggest looking at pyca/cryptography to copy the setup we have there for CI and then set up a decent base config (e.g. https://github.com/pyca/cryptography/blob/main/pyproject.toml#L24, but with the right set of changes to work with minimal/no typing at first).

Dreamsorcerer commented 2 years ago

Looks good overall, just a couple of minor points and the lack of CI.

DMRobertson commented 2 years ago

Thank you both for your feedback.

to make sure we don't get stalled and get this all reviewed quickly we need to add a mypy job (with whatever set of flags/config we need to have it passing), merge that, then incrementally add chunks of this PR so that it's more easily reviewable.

I've attempted this at https://github.com/pyca/pynacl/pull/693. Its CI is sad---but I don't think in a way that I've caused? But let's discuss it there.

pyca / pynacl

Add type annotations to `nacl` #692