Fix remaining ty static type checker warnings and errors. - #139
Fix remaining ty static type checker warnings and errors.#139roberthdevries wants to merge 1 commit into
Conversation
This will also allow to run ty check in the pipeline static checks step.
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 6 total — 5 posted, 1 skipped
4 finding(s) posted as inline comments (see file-level comments below)
1 finding(s) not tied to a diff line (full detail below)
Posted findings
- [High] EccPrivate.encode_key() accepts with_curve but silently ignores it —
wolfcrypt/ciphers.py:1396-1409 - [Medium] _Hmac is no longer abstract after removing the _type abstractmethod —
wolfcrypt/hashes.py:447-503 - [Medium] New uv run ty check CI gate may never fail the build —
.github/workflows/python-app.yml:35-38 - [Low] _type should be annotated ClassVar[int], not int —
wolfcrypt/hashes.py:460
Findings not tied to a diff line
New ty suppressions use a non-standard ignore [code] spacing
File: tests/test_mldsa.py:235,252; tests/test_mlkem.py:624,633
Function: test_sign_with_seed_bad_type / test_make_key_from_seed_bad_type / test_make_key_with_random_bad_random_type / test_encapsulate_with_random_bad_random_type
Severity: Medium
All four suppressions added by this PR are written as # ty: ignore [invalid-argument-type] with a space between ignore and [. Every one of the roughly fifty pre-existing suppressions in this repository uses the tight form # ty: ignore[code] -- including tests/test_mldsa.py:206 (_ = mldsa_priv.sign_with_seed(message, "") # ty: ignore[invalid-argument-type]), which sits twenty-nine lines above one of the new ones in the same file, and every suppression this same PR adds to wolfcrypt/ciphers.py and wolfcrypt/hkdf.py.
ty's suppression grammar expects the code list to follow ignore directly; a bare # ty: ignore (no bracket immediately after) is the blanket form that suppresses every diagnostic on the line. With the space, ty will most likely either (a) parse these as blanket suppressions and treat [invalid-argument-type] as free-form trailing comment text, or (b) flag them under invalid-ignore-comment. Outcome (a) is the dangerous one: these lines would silently swallow any future unrelated diagnostic rather than just the intended invalid-argument-type, and the narrowing the author clearly intended would be lost without any signal. Outcome (b) produces noise that the new CI gate is supposed to be catching.
Recommendation: Delete the space before [ in all four new suppressions so they match the repository-wide convention and are unambiguously parsed as scoped (not blanket) suppressions.
Referenced code: tests/test_mldsa.py:235,252; tests/test_mlkem.py:624,633 (11 lines)
Skipped findings
- [Medium]
No test covers the newly added with_curve parameter
Review generated by Skoll
| @@ -1394,7 +1394,7 @@ def decode_key_raw(self, qx: BytesOrStr, qy: BytesOrStr, d: BytesOrStr, curve_id | |||
| raise WolfCryptApiError("Key decode error", ret) | |||
|
|
|||
| @override | |||
There was a problem hiding this comment.
🔴 [High] EccPrivate.encode_key() accepts with_curve but silently ignores it
To silence ty's invalid-method-override, the signature of EccPrivate.encode_key was widened from (self) -> bytes to (self, with_curve: bool = True) -> bytes, but the body was not touched. It still calls _lib.wc_EccKeyToDer(self.native_object, key, len(key)). Per the CFFI cdef in scripts/build_ffi.py:1112 (int wc_EccKeyToDer(ecc_key*, byte* output, word32 inLen);), that wolfSSL API has no curve flag at all -- only wc_EccPublicKeyToDer (used by the base EccPublic.encode_key, ciphers.py:1213) takes with_curve. The new parameter is therefore dead: it is accepted, type-checked, documented by its presence in the signature, and then dropped on the floor.
This is a behavioral regression, not just dead code. Before this PR, ecc_priv.encode_key(with_curve=False) raised TypeError: encode_key() takes 1 positional argument but 2 were given -- a loud, immediate failure. After this PR the same call silently returns curve-bearing DER, i.e. exactly the opposite of what the caller asked for. This is the LSP violation ty was warning about, converted from a noisy runtime error into a silent wrong answer.
It is also inconsistent with the rest of this same diff. The four other override mismatches touched here (EccPrivate.decode_key_raw at line 1375, EccPrivate.encode_key_raw at line 1412, Ed25519Private.encode_key at line 1683, Ed448Private.encode_key at line 1892) were all resolved with # ty: ignore[invalid-method-override], deliberately preserving the runtime signature. Only this one method had its signature altered, and it is the one case where doing so…
Fix: Revert the signature to def encode_key(self) -> bytes: and suppress with # ty: ignore[invalid-method-override], matching the four sibling methods fixed the same way in this diff. If the widened signature is intentionally kept for LSP substitutability, the parameter must not be a silent no-op: raise ValueError("with_curve=False is not supported for ECC private keys") when with_curve is falsy, and document the parameter in the docstring.
| def _type(self) -> int: ... | ||
|
|
||
| def _hmac_init(self, hmac: int, key: bytes) -> int: | ||
| ret = _lib.wc_HmacInit(self._native_object, _ffi.NULL, -2) |
There was a problem hiding this comment.
🟠 [Medium] _Hmac is no longer abstract after removing the _type abstractmethod
The diff deletes the @property @abstractmethod def _type(self) -> int: ... declaration and replaces it with a bare class-body annotation _type: int (line 460). A bare annotation creates no attribute and, critically, is not an abstract member -- so it removes the only thing that was keeping _Hmac abstract.
Walking the ABC bookkeeping: _Hash declares _init, _update, _final, _native_size, _native_type, digest_size, and new as abstract. _Hmac supplies concrete overrides or class attributes for every one of them (digest_size = None, _native_type = "Hmac *", _native_size = _ffi.sizeof("Hmac"), _init, _update, _final, new). With _type gone from __abstractmethods__, that set is now empty and _Hmac is instantiable.
Concretely: _Hmac(b"key") previously raised TypeError: Can't instantiate abstract class _Hmac with abstract method _type before any C resource was touched. It now proceeds into __init__, allocates the native object via _ffi.new(self._native_type), and only then dies at self._hmac_init(self._type, key) (line 479) with AttributeError: '_Hmac' object has no attribute '_type'. The same loss of enforcement applies to any future HMAC variant added to this file: forgetting _type = _TYPE_... used to be a class-instantiation-time error and is now a late AttributeError from inside __init__. _Hmac is a documented extension point (docs/mac.rst:25 has .. autoclass:: _Hmac), so this matters beyond internal use.
Fix: Restore an equivalent guard so a missing _type still fails early and legibly. Either add an __init_subclass__ check as shown, or keep _Hmac explicitly abstract (e.g. leave a trivial abstract member) so _Hmac itself cannot be constructed. At minimum, guard __init__ with an explicit check that produces a clear message instead of a raw AttributeError.
| @@ -33,7 +33,9 @@ jobs: | |||
| - name: Install the project | |||
| run: uv sync --dev | |||
| - name: Perform static checks | |||
There was a problem hiding this comment.
🟠 [Medium] New uv run ty check CI gate may never fail the build
The PR's stated purpose is to "allow to run ty check in the pipeline static checks step," but the step as written may be a no-op gate. pyproject.toml contains [tool.ty.rules] with all = "warn", which downgrades every ty rule from its default severity to warning level. ty check exits non-zero for error-level diagnostics; warning-level diagnostics require the --error-on-warning flag to affect the exit code. If that is the case here, the step will print diagnostics to the CI log and still exit 0, so the type regressions this PR just spent effort eliminating could silently reappear without turning CI red.
The multi-command run: | block itself is fine -- GitHub Actions uses bash -e {0} on Linux, so a failing uv run ruff check will still abort before ty check.
Fix: Verify the gate actually blocks: temporarily introduce a deliberate type error on the branch and confirm the job goes red. If it does not, either add --error-on-warning to the CI invocation or promote the rules that should be blocking from "warn" to "error" in [tool.ty.rules].
| digest_size = None | ||
| _native_type = "Hmac *" | ||
| _native_size = _ffi.sizeof("Hmac") | ||
| _type: int |
There was a problem hiding this comment.
🔵 [Low] _type should be annotated ClassVar[int], not int
_type: int declares an instance variable, but every concrete subclass assigns it at class level -- HmacSha._type = _TYPE_SHA (line 533), HmacSha256._type = _TYPE_SHA256 (line 545), HmacSha384 (line 557), HmacSha512 (line 569) -- and wolfcrypt/hkdf.py reads it straight off the class object, never an instance: hash_cls._type at hkdf.py:70, hkdf.py:106, and hkdf.py:137, where hash_cls: type[_Hmac]. ClassVar[int] states the actual contract (a class-level constant selecting the wolfCrypt HMAC type id), makes class-object access unambiguous for checkers, and prevents a subclass from accidentally shadowing it per-instance. Worth doing since mypy is also in the dev dependency group and may treat class-object access to a plain instance annotation less permissively than ty.
Fix: Change the annotation to _type: ClassVar[int] and add ClassVar to the typing import at wolfcrypt/hashes.py:26.
This will also allow to run ty check in the pipeline static checks step.