Skip to content

Commit f4e063b

Browse files
llucaxllucax
authored
Make match statements pass mypy's exhaustive-match check (frequenz-floss#1455)
frequenz-floss#1448 updates `frequenz-repo-config` to 0.19.0, which enables mypy's `exhaustive-match` error code, and CI fails on 11 `match` statements 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.
2 parents 900fa0e + 3ca5a65 commit f4e063b

8 files changed

Lines changed: 217 additions & 63 deletions

File tree

‎RELEASE_NOTES.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,4 +16,4 @@
1616

1717
## Bug Fixes
1818

19-
<!-- Here goes notable bug fixes that are worth a special mention or explanation -->
19+
* `COALESCE()` no longer unsubscribes from the parameter that is producing values when the parameter it was tracking evaluates to nothing at all. This could only happen when a parameter is an expression that has no value yet, like a nested function call, in which case the samples of the producing parameter were counted against the tracked one.

‎src/frequenz/sdk/microgrid/_power_managing/_bounds.py‎

Lines changed: 54 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,40 @@
33

44
"""Utilities for checking and clamping bounds and power values to exclusion bounds."""
55

6+
import enum
7+
from typing import assert_never
8+
69
from frequenz.quantities import Power
710

811
from ...timeseries import Bounds
912

1013

14+
# This used to be a tuple[bool, bool], but mypy can't check that a match over a tuple
15+
# covers every combination (see python/mypy#12364), so callers could leave a case out
16+
# without anyone noticing. It also reads better, as (True, False) gives no hint about
17+
# which of the two bounds it refers to.
18+
@enum.unique
19+
class ExclusionOverlap(enum.Enum):
20+
"""Which bounds of a pair fall inside an exclusion zone."""
21+
22+
NONE = enum.auto()
23+
"""Neither bound is inside the exclusion zone."""
24+
25+
LOWER = enum.auto()
26+
"""Only the lower bound is inside the exclusion zone."""
27+
28+
UPPER = enum.auto()
29+
"""Only the upper bound is inside the exclusion zone."""
30+
31+
BOTH = enum.auto()
32+
"""Both bounds are inside the exclusion zone."""
33+
34+
1135
def check_exclusion_bounds_overlap(
1236
lower_bound: Power,
1337
upper_bound: Power,
1438
exclusion_bounds: Bounds[Power] | None,
15-
) -> tuple[bool, bool]:
39+
) -> ExclusionOverlap:
1640
"""Check if the given bounds overlap with the given exclusion bounds.
1741
1842
Example:
@@ -29,31 +53,30 @@ def check_exclusion_bounds_overlap(
2953
(inside the exclusion zone)
3054
```
3155
32-
Resulting in `(False, True)` because only the upper bound is inside the
33-
exclusion zone.
56+
Resulting in `ExclusionOverlap.UPPER` because only the upper bound is inside
57+
the exclusion zone.
3458
3559
Args:
3660
lower_bound: The lower bound to check.
3761
upper_bound: The upper bound to check.
3862
exclusion_bounds: The exclusion bounds to check against.
3963
4064
Returns:
41-
A tuple containing a boolean indicating if the lower bound is bounded by the
42-
exclusion bounds, and a boolean indicating if the upper bound is bounded by
43-
the exclusion bounds.
65+
Which of the given bounds are inside the exclusion bounds.
4466
"""
4567
if exclusion_bounds is None:
46-
return False, False
47-
48-
bounded_lower = False
49-
bounded_upper = False
68+
return ExclusionOverlap.NONE
5069

51-
if exclusion_bounds.lower < lower_bound < exclusion_bounds.upper:
52-
bounded_lower = True
53-
if exclusion_bounds.lower < upper_bound < exclusion_bounds.upper:
54-
bounded_upper = True
70+
bounded_lower = exclusion_bounds.lower < lower_bound < exclusion_bounds.upper
71+
bounded_upper = exclusion_bounds.lower < upper_bound < exclusion_bounds.upper
5572

56-
return bounded_lower, bounded_upper
73+
if bounded_lower and bounded_upper:
74+
return ExclusionOverlap.BOTH
75+
if bounded_lower:
76+
return ExclusionOverlap.LOWER
77+
if bounded_upper:
78+
return ExclusionOverlap.UPPER
79+
return ExclusionOverlap.NONE
5780

5881

5982
def adjust_exclusion_bounds(
@@ -80,13 +103,16 @@ def adjust_exclusion_bounds(
80103
# And if the given bounds overlap with the exclusion bounds on one side, then clamp
81104
# the given bounds on that side.
82105
match check_exclusion_bounds_overlap(lower_bound, upper_bound, exclusion_bounds):
83-
case (True, True):
106+
case ExclusionOverlap.BOTH:
84107
return Power.zero(), Power.zero()
85-
case (False, True):
108+
case ExclusionOverlap.UPPER:
86109
return lower_bound, exclusion_bounds.lower
87-
case (True, False):
110+
case ExclusionOverlap.LOWER:
88111
return exclusion_bounds.upper, upper_bound
89-
return lower_bound, upper_bound
112+
case ExclusionOverlap.NONE:
113+
return lower_bound, upper_bound
114+
case unexpected:
115+
assert_never(unexpected)
90116

91117

92118
# Just 20 lines of code in this function, but unfortunately 8 of those are return
@@ -123,14 +149,20 @@ def clamp_to_bounds( # pylint: disable=too-many-return-statements
123149
match check_exclusion_bounds_overlap(
124150
lower_bound, upper_bound, exclusion_bounds
125151
):
126-
case (True, True):
152+
case ExclusionOverlap.BOTH:
127153
return None, None
128-
case (True, False):
154+
case ExclusionOverlap.LOWER:
129155
if value < exclusion_bounds.upper:
130156
return None, exclusion_bounds.upper
131-
case (False, True):
157+
case ExclusionOverlap.UPPER:
132158
if value > exclusion_bounds.lower:
133159
return exclusion_bounds.lower, None
160+
case ExclusionOverlap.NONE:
161+
# The bounds don't overlap the exclusion zone, so the value only needs
162+
# the generic clamping done below.
163+
pass
164+
case unexpected:
165+
assert_never(unexpected)
134166

135167
# If the given value is outside the given bounds, clamp it to the closest bound.
136168
if value < lower_bound:

‎src/frequenz/sdk/microgrid/_power_managing/_matryoshka.py‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -111,11 +111,13 @@ def _calc_target_power(
111111
# If the bounds from the current proposal are fully within the exclusion
112112
# bounds, then don't use them to narrow the bounds further. This allows
113113
# subsequent proposals to not be blocked by the current proposal.
114-
match _bounds.check_exclusion_bounds_overlap(
115-
proposal_lower, proposal_upper, exclusion_bounds
114+
if (
115+
_bounds.check_exclusion_bounds_overlap(
116+
proposal_lower, proposal_upper, exclusion_bounds
117+
)
118+
is _bounds.ExclusionOverlap.BOTH
116119
):
117-
case (True, True):
118-
continue
120+
continue
119121
lower_bound = max(lower_bound, proposal_lower)
120122
upper_bound = min(upper_bound, proposal_upper)
121123
lower_bound, upper_bound = _bounds.adjust_exclusion_bounds(
@@ -275,11 +277,13 @@ def get_status(
275277
break
276278
proposal_lower = next_proposal.bounds.lower or lower_bound
277279
proposal_upper = next_proposal.bounds.upper or upper_bound
278-
match _bounds.check_exclusion_bounds_overlap(
279-
proposal_lower, proposal_upper, exclusion_bounds
280+
if (
281+
_bounds.check_exclusion_bounds_overlap(
282+
proposal_lower, proposal_upper, exclusion_bounds
283+
)
284+
is _bounds.ExclusionOverlap.BOTH
280285
):
281-
case (True, True):
282-
continue
286+
continue
283287
calc_lower_bound = max(lower_bound, proposal_lower)
284288
calc_upper_bound = min(upper_bound, proposal_upper)
285289
if calc_lower_bound <= calc_upper_bound:

‎src/frequenz/sdk/microgrid/_power_managing/_power_managing_actor.py‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,7 @@ async def _send_updated_target_power(
197197
)
198198
)
199199

200-
@override
200+
@override # pylint: disable-next=too-many-branches
201201
async def _run(self) -> None:
202202
"""Run the power managing actor."""
203203
last_result_partial_failure = False
@@ -261,6 +261,14 @@ async def _run(self) -> None:
261261
)
262262
case _power_distributing.Success():
263263
last_result_partial_failure = False
264+
case (
265+
_power_distributing.Error() | _power_distributing.OutOfBounds()
266+
):
267+
# No power was set at all, so there is nothing to correct here.
268+
# Only a successful request clears the partial failure state.
269+
pass
270+
case unexpected:
271+
assert_never(unexpected)
264272
await self._send_reports(frozenset(result.request.component_ids))
265273

266274
elif selected_from(selected, drop_old_proposals_timer):

‎src/frequenz/sdk/microgrid/_power_managing/_shifting_matryoshka.py‎

Lines changed: 16 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -122,25 +122,24 @@ def _calc_targets( # pylint: disable=too-many-branches,too-many-statements
122122
if upper_bound < lower_bound:
123123
break
124124

125-
match (next_proposal.bounds.lower, next_proposal.bounds.upper):
126-
case (None, None):
125+
# Bounds the proposal leaves open are taken from the currently available
126+
# bounds, unless the bound the proposal does set is already past them, in
127+
# which case the proposal collapses to that single value.
128+
proposal_lower = next_proposal.bounds.lower
129+
proposal_upper = next_proposal.bounds.upper
130+
if proposal_lower is None:
131+
if proposal_upper is None:
127132
proposal_lower = lower_bound
128133
proposal_upper = upper_bound
129-
case (Power(), None):
130-
proposal_lower = next_proposal.bounds.lower
131-
if proposal_lower > upper_bound:
132-
proposal_upper = proposal_lower
133-
else:
134-
proposal_upper = upper_bound
135-
case (None, Power()):
136-
proposal_upper = next_proposal.bounds.upper
137-
if proposal_upper < lower_bound:
138-
proposal_lower = proposal_upper
139-
else:
140-
proposal_lower = lower_bound
141-
case (Power(), Power()):
142-
proposal_lower = next_proposal.bounds.lower
143-
proposal_upper = next_proposal.bounds.upper
134+
elif proposal_upper < lower_bound:
135+
proposal_lower = proposal_upper
136+
else:
137+
proposal_lower = lower_bound
138+
elif proposal_upper is None:
139+
if proposal_lower > upper_bound:
140+
proposal_upper = proposal_lower
141+
else:
142+
proposal_upper = upper_bound
144143

145144
proposal_power = next_proposal.preferred_power
146145

‎src/frequenz/sdk/timeseries/formulas/_ast.py‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,11 @@ async def evaluate(self) -> Sample[QuantityT] | QuantityT | None:
177177
)
178178
case (None, _) | (_, None):
179179
return None
180-
return None
180+
case unexpected:
181+
# The cases above are exhaustive, but mypy can't narrow tuple patterns
182+
# (see python/mypy#12364), so it neither sees that nor accepts
183+
# assert_never() here.
184+
raise AssertionError(f"Unexpected operands: {unexpected!r}")
181185

182186
@override
183187
def format(self, wrap: bool = False) -> str:
@@ -254,7 +258,11 @@ async def evaluate(self) -> Sample[QuantityT] | QuantityT | None:
254258
)
255259
case (None, _) | (_, None):
256260
return None
257-
return None
261+
case unexpected:
262+
# The cases above are exhaustive, but mypy can't narrow tuple patterns
263+
# (see python/mypy#12364), so it neither sees that nor accepts
264+
# assert_never() here.
265+
raise AssertionError(f"Unexpected operands: {unexpected!r}")
258266

259267
@override
260268
def format(self, wrap: bool = False) -> str:
@@ -331,7 +339,11 @@ async def evaluate(self) -> Sample[QuantityT] | QuantityT | None:
331339
)
332340
case (None, _) | (_, None):
333341
return None
334-
return None
342+
case unexpected:
343+
# The cases above are exhaustive, but mypy can't narrow tuple patterns
344+
# (see python/mypy#12364), so it neither sees that nor accepts
345+
# assert_never() here.
346+
raise AssertionError(f"Unexpected operands: {unexpected!r}")
335347

336348
@override
337349
def format(self, wrap: bool = False) -> str:
@@ -406,7 +418,11 @@ async def evaluate(self) -> Sample[QuantityT] | QuantityT | None:
406418
)
407419
case (None, _) | (_, None):
408420
return None
409-
return None
421+
case unexpected:
422+
# The cases above are exhaustive, but mypy can't narrow tuple patterns
423+
# (see python/mypy#12364), so it neither sees that nor accepts
424+
# assert_never() here.
425+
raise AssertionError(f"Unexpected operands: {unexpected!r}")
410426

411427
@override
412428
def format(self, wrap: bool = False) -> str:

‎src/frequenz/sdk/timeseries/formulas/_functions.py‎

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
import logging
1111
from dataclasses import dataclass, field
1212
from datetime import datetime
13-
from typing import Generic
13+
from typing import Generic, assert_never
1414

1515
from frequenz.quantities import Quantity
1616
from typing_extensions import override
@@ -117,7 +117,7 @@ def name(self) -> str:
117117
"""Return the name of the function."""
118118
return "COALESCE"
119119

120-
@override
120+
@override # pylint: disable-next=too-many-branches
121121
async def __call__(self) -> Sample[QuantityT] | QuantityT | None:
122122
"""Return the first non-None argument."""
123123
ts: datetime | None = None
@@ -132,16 +132,29 @@ async def __call__(self) -> Sample[QuantityT] | QuantityT | None:
132132
match arg:
133133
case Sample(timestamp, value):
134134
if value is not None:
135-
# Keep track of which parameter we are getting samples from.
136-
# this slightly convoluted check ensures that we unsubscribe
137-
# from the last parameter if any earlier one produces at least
138-
# REQUIRED_CONSECUTIVE_STABLE_SAMPLES samples, regardless of
139-
# intermittent non-None values received from other params.
140-
match self.used_param > 0 and args[self.used_param - 1]:
141-
case False | Sample(value=None):
135+
# Keeps track of which parameter we are getting samples
136+
# from. This slightly convoluted check ensures that we
137+
# unsubscribe from the last parameter if any earlier
138+
# one produces at least
139+
# `REQUIRED_CONSECUTIVE_STABLE_SAMPLES` samples,
140+
# regardless of intermittent non-None values received
141+
# from other params.
142+
used_arg = (
143+
args[self.used_param - 1] if self.used_param > 0 else None
144+
)
145+
match used_arg:
146+
case None | Sample(value=None):
147+
# We are not tracking a parameter yet, or the
148+
# one we track stopped producing values, so
149+
# track this one instead.
142150
self.used_param = param
143151
self.num_samples = 0
144-
152+
case Sample() | Quantity():
153+
# The tracked parameter is still producing
154+
# values, so keep counting samples for it.
155+
pass
156+
case unexpected:
157+
assert_never(unexpected)
145158
self.num_samples += 1
146159

147160
if (

0 commit comments

Comments
 (0)