fix: resolve all pyrefly type errors and gate them in ci - #25
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
23 errors to 0, fixed at the root in each case rather than suppressed. No
type: ignoreanywhere.Ten errors were one real API flaw.
non_dominated_sort,crowding_distanceandpareto_selectdeclaredobjectives: dict[int, Objectives], anddictis invariant in its value type, so a caller holding adict[int, tuple[float, float]]could not pass it even though that tuple is a perfectly goodtuple[float, ...]. All three only ever read the mapping, so they now takeMapping, which is covariant. That is a genuine widening of the API, not a workaround.Seven were the analysis scripts being invisible. The round-trip tests import
plot_runandclean_runsdirectly to check they still read what storage writes, butscripts/is not an installed package. Addedsearch-path = ["src", "scripts"]to the pyrefly config.Two were missing assertions in tests, and the checker was right.
validatereturnstuple[bool, str | None]andapply_search_replacereturnsstr | None, and both tests went straight to anincheck against the possibly-None value. Now they assert non-None first, which is better test hygiene regardless of the type checker.Two came from
worker_loopover-declaring what it needs. It was annotatedmp.Queue, but it only callsgeton one queue andputon the other; production passes a multiprocessing queue and the tests pass aqueue.Queue, and those share no base class. Replaced with aMessageQueueprotocol describing exactly that surface. The methods are positional-only because the two queue classes name the argument differently (itemversusobj), which otherwise fails protocol matching.multiprocessingis no longer imported there.Two were false positives introduced by the SDK upgrade in #24.
chat.completions.createandmessages.createare overloaded on thestreamliteral to return either a response or aStream, and building the arguments as a dict hides the literal from the checker, so it picks the streaming branch. Neither call ever enables streaming. These are the only two spots I could not fix structurally without unpicking the conditional-kwargs pattern in both providers, so they arecasts with a comment explaining why.castis a no-op at runtime.Also added pyrefly to CI. It was configured in
pyproject.tomlbut nothing ran it, which is exactly how it drifted to 21 errors unnoticed. Without the gate this PR just resets a counter that will climb again.Verified beyond the checker, since two changes touch runtime paths: a real multiprocess spawn run with three workers completes and writes output (score 0.0039), confirming the protocol swap did not disturb the pickled worker entry point, and all three providers still construct. 472 tests pass, ruff clean, and the full CI gate passes locally.
The 2 remaining warnings pyrefly does not print by default are unaffected; only errors were in scope.