Make match statements pass mypy's exhaustive-match check - #1455
Merged
Conversation
llucax
requested review from
florian-wagner-frequenz
and removed request for
a team
September 8, 2026 09:14
github-actions
Bot
added
part:docs
llucax
force-pushed
the
fix-exhaustive-match
branch
from
September 8, 2026 09:18
fef848d to
0207097
Compare
github-actions
Bot
added
the
part:tooling
Contributor
Author
|
Adding a new commit to enable relative cross refs in mkdocs as a quick fix. It seems for some reason properdocs is being used, not sure if that is related to parsing docs from a dependency or not. |
Contributor
Author
|
Dammit! This didn't help because the quantities docs relative references are really incorrect. I have no idea why the CI didn't fail in the quantities repo. See: |
llucax
force-pushed
the
fix-exhaustive-match
branch
from
September 8, 2026 10:15
7c01e49 to
0207097
Compare
Contributor
Author
|
Removing the last commit, it is not helping, it seems to be a bug in mkdocstrings relative-cross referencing 😬 |
The function returned a `tuple[bool, bool]`, which callers matched on with one `case` per combination. mypy can't narrow tuple items across `case` clauses (python/mypy#12364), so it can't tell whether such a `match` covers every combination, and a missing case would go unnoticed. This is about to matter, as the next `frequenz-repo-config` enables mypy's `exhaustive-match` error code, which reports these two `match` statements. Return a four-valued enum instead, which mypy does check exhaustively, and which also reads better: `(True, False)` gave no hint about which of the two bounds it referred to. The two call sites in `_matryoshka.py` only cared about one combination, so they become plain `if` statements. Both matches also get an `assert_never()` case, to fail early if an unexpected value ever reaches them: type hints are not enforced at runtime. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
`COALESCE()` keeps track of which parameter it is getting samples from, and switches to another one when the tracked parameter stops producing values, so that it can unsubscribe from the parameters after it once the new one is stable. It only recognized a `Sample` without a value as "stopped producing", not a parameter evaluating to `None`. A parameter evaluates to `None`, rather than to an empty `Sample`, when it is an expression with no value at all yet, like a nested function call. In that case the samples of the parameter actually producing values were counted against the tracked one, and once the count reached the required number of stable samples, `COALESCE()` unsubscribed from every parameter after the tracked one, dropping the only producer. Treat both cases the same, and add a test covering it. The check was written as a `match` over `used_param > 0 and args[...]`, which made the missing case easy to miss. Spell the value out first and match over it, so all the cases are visible. The match also gets an `assert_never()` case, to fail early if an unexpected value ever reaches it. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
The `match` over the results only had cases for `PartialFailure` and `Success`, so `Error` and `OutOfBounds` fell through it silently. That is the intended behaviour, as no power was set at all in either case, so there is nothing to correct, and only a successful request should clear the partial failure state. Document it, so it reads as a decision rather than an oversight. There is no behaviour change. The final `assert_never()` case makes it fail early if an unexpected result ever reaches it: type hints are not enforced at runtime. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
The `match` covered all four combinations of a proposal's optional lower and upper bounds, but mypy can't narrow tuple items across `case` clauses (see python/mypy#12364), so it can't prove such a `match` exhaustive no matter how many cases are written. Nested `if`s over the two bounds cover the same four combinations, mypy does check those, and the shared code between the cases no longer needs repeating. There is no behaviour change. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
The `match` in each binary operator covers all four combinations of its two operands, plus the ones where an operand is `None`, and was followed by an unreachable `return None`. mypy can't narrow tuple items across `case` clauses (see python/mypy#12364), so it can't prove the `match` exhaustive no matter how many cases are written. Turn that trailing `return None` into the explicit last case, and raise instead of returning, so an operand of an unexpected type fails early rather than silently evaluating the whole formula to nothing: type hints are not enforced at runtime. `assert_never()` can't be used here, as mypy doesn't narrow the subject to `Never`. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
llucax
added a commit
to llucax/frequenz-sdk-python
that referenced
this pull request
Sep 8, 2026
…ss#1456) CI started failing on `v1.x.x` yesterday with three unrelated-looking config test failures. They come from the `frequenz-quantities` 1.0.2 release, which raised its `marshmallow` floor to 4 (frequenz-floss/frequenz-quantities-python#107) and thereby dragged `marshmallow` 4 into our environment for the first time. Marshmallow 4 dropped support for validators that return `False` instead of raising, which is what the config test dataclasses were doing, so the validation silently stopped happening. The fix is test-only, but there is a release notes entry because the same silent acceptance hits anyone who wrote boolean validators for their own config dataclasses. Heads-up for whoever reviews: the docs job on this PR will stay red, for a second and unrelated problem from the same `frequenz-quantities` 1.0.2 release. That one is a docstring in `frequenz-quantities` using a relative mkdocstrings cross-reference, which misresolves whenever it is rendered under a path other than the one it was written in, as happens here with `BaseConfigSchema` inheriting `QuantitySchema.TYPE_MAPPING`. It cannot be fixed on this side, only worked around by dropping documentation, and frequenz-floss/frequenz-quantities-python#170 already fixes it upstream. frequenz-floss#1455 is blocked on the same thing.
llucax
force-pushed
the
fix-exhaustive-match
branch
from
September 8, 2026 12:43
0207097 to
3ca5a65
Compare
llucax
enabled auto-merge
September 8, 2026 12:58
Contributor
Author
|
Green now. |
Contributor
Author
|
@shsms some changes affect formulas including a bug fix, so it would be good to have your review here. |
Contributor
|
All commits make sense and the first one is particularly nice because it simplifies a public interface. Still looking into the second commit. |
shsms
approved these changes
Sep 8, 2026
| args[self.used_param - 1] if self.used_param > 0 else None | ||
| ) | ||
| match used_arg: | ||
| case None | Sample(value=None): |
Contributor
There was a problem hiding this comment.
I don't think there are any cases that would produce a plain None. So this is improving type coverage, not fixing a bug. I think an alternate solution is to drop the None from the types completely, and that would make some of the other nodes simpler as well.
But happy to accept it as it is, because it is only a minor cost.
Contributor
There was a problem hiding this comment.
And the unit test is also just making up some random stuff, because it cannot produce a case to test this by going through the parser.
Contributor
Author
There was a problem hiding this comment.
I might not have looked at the tests 😬
github-project-automation
Bot
moved this from To do
to Review approved
in Python SDK Roadmap
github-project-automation
Bot
moved this from Review approved
to Done
in Python SDK Roadmap
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
2 participants
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.
#1448 updates
frequenz-repo-configto 0.19.0, which enables mypy'sexhaustive-matcherror code, and CI fails on 11matchstatements that mypy can't prove to be exhaustive. These are the code changes needed to make it pass.Most of the reported statements match over a tuple, and mypy can't prove any of those exhaustive: it narrows tuple items inside a matching case, but can't subtract combinations across cases (python/mypy#12364), so writing the missing case does not help. Those either stop matching over a tuple, or get an explicit last case. The rest were genuinely missing cases, which are now spelled out.
One bug came out of it, in
COALESCE(), and it has its own commit and release notes entry. Everything else is behaviour preserving, except that the formula operators now raise instead of silently evaluating to nothing when an operand has an unexpected type, which can only happen if something violates the type hints at runtime.