Mypy tests 3 #730
Mypy tests 3 #730
Conversation
DMRobertson
force-pushed
the
mypy-tests-3
branch
2 times, most recently
from
a270ea9
to
e0eb2ea
Compare
|
Codecov complains, but AFAICS there's no complaint here: https://app.codecov.io/gh/pyca/pynacl/compare/730?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=checks&utm_campaign=pr%20comments&utm_term=pyca |
DMRobertson
force-pushed
the
mypy-tests-3
branch
from
e0eb2ea
to
7c00fb3
Compare
| @@ -73,13 +73,15 @@ class crypto_secretstream_xchacha20poly1305_state: | |||
|
|
|||
| def __init__(self) -> None: | |||
| """Initialize a clean state object.""" | |||
| self.statebuf = ffi.new( | |||
| # NOTE: the members below aren't `bytearray` objects, but cffi `cdata` objects | |||
| # which own an array of unsigned chars. Is there a better annotation? | |||
There was a problem hiding this comment.
I considered typing.ByteString here, but
- that's immutable, but one of the test writes to it
- the docs suggest that
bytesin an annotation is really shorthand for a ByteString anyway.class typing.ByteString(Sequence[int])
A generic version of collections.abc.ByteString.
This type represents the types bytes, bytearray, and memoryview of byte sequences.
As a shorthand for this type, bytes can be used to annotate arguments of any of the types mentioned above.
bytearray seemed like the least bad option, but would welcome input here!
There was a problem hiding this comment.
In cryptography we've chosen to leave these untyped for now with a hope of typing ffi.new itself in the future. I suppose if we do that then we'd catch these cases here, but I'm not hugely comfortable with picking a "closest approximation" type. e.g. you can't call bytes() on this buffer, which you could on a bytearray.
There was a problem hiding this comment.
That makes sense.
The only place (AFAICS) that the statebuf gets mutated by Python is here, and that's just to copy the data from another ffi unsigned char[].
Perhaps it would be safer to use Sequence[int] here instead of bytearray and type-ignore the part of the test that writes to this buffer. (I stuck a breakpoint in at the snippet linked above, where state_save[0] was the int 68 as opposed to b"\x68".)
There was a problem hiding this comment.
Actually, ByteString seems like a better fit: it inherits from Sequence[int], but additionally expresses that the ints are all going to be byte-sized.
There was a problem hiding this comment.
I've had a go at this in bc42daf.
| # Type safety: mypy spots that you can't decrypt with a public key, but we | ||
| # want to detect this at runtime too. |
There was a problem hiding this comment.
I was pleased to see this caught by mypy---I'd forgotten that we did this in #706!
We're not trying to annotate cffi here (that seems subtle, hard and important to get right). Instead, `ByteString` gives us a `len()` which keeps mypy happy within the project and is slightly more prescriptive than `Sized` or `Sequence[int]`.
|
I'm going to avoid further nitpicking here since we can always find ways to improve in the future. 😁 |
|
For what it's worth, I'm open to nitpicking. Either way, great to see this merged and rounded off! |
Also a draft. Branches off from #728; the first new commit is
8cfeced. I'm just sticking this up to get CI to run on the proposed final batch of annotations to
testsnow.Also, while I'm at it, I wanted to see if we could quantify how much has changed with all this typing. To do so:
TL:DR:
(These numbers from the any-exprs and html reports, respectively).
I think most of that comes down to cffi in
nacl; maybe some bits in tests fromjson.loads().If we just look at the higher level stuff outside of
nacl.bindings, the numbers are rosier: