gh-104050: Annotate Argument Clinic parameter permutation helpers by erlend-aasland · Pull Request #106431 · python/cpython

erlend-aasland

Issue: Add type annotations to clinic.py #104050

<silly rant> Sometimes I regret starting with this typing business. My hair got a lot more gray from working on this PR 😵‍💫 Alex, can you please help? @AlexWaygood </silly rant>

Sometimes I regret starting with this typing business.

:( I'm sorry to hear that. There's definitely a lot of concepts to grapple with, I'll fully concede.

We have uncovered at least two real bugs along the way, though!

:( I'm sorry to hear that.

No worries, I'm having fun (slowly learning)! And I think this whole project will end up increasing the readability and maintainability of clinic.py. I'm just frustrated when I can't figure out new stuff, nothing else 😄

Also, once again, thanks for helping reviewing and explaining!

So, here's what mypy complains about on my computer:

Tools/clinic/clinic.py:536: error: Incompatible types in assignment (expression has type "list[Iterable[Parameter] | Parameter]", variable has type "list[Iterable[Parameter]]")  [assignment]
Tools/clinic/clinic.py:553: error: Argument 1 to "extend" of "list" has incompatible type "Iterable[Parameter]"; expected "Iterable[Iterable[Parameter]]"  [arg-type]
Tools/clinic/clinic.py:584: error: Argument 1 to "append" of "list" has incompatible type "tuple[Iterable[Parameter], ...]"; expected "Iterable[Parameter]"  [arg-type]
Tools/clinic/clinic.py:586: error: Argument "key" to "sort" of "list" has incompatible type "Callable[[Sized], int]"; expected "Callable[[Iterable[Parameter]], SupportsDunderLT[Any] | SupportsDunderGT[Any]]"  [arg-type]

Is mypy fooled by the list(group) thing? Also, I'm puzzled by the key param complaint.

Humm, I wrote you a small essay above there, but I'm still working on getting mypy to actually pass with my suggestions... I'll get back to you :)

Okay! Here's a diff that makes mypy happy, and I think it has the added virtue of being correct. How does it look to you?

Details

--- a/Tools/clinic/clinic.py
+++ b/Tools/clinic/clinic.py
@@ -28,7 +28,7 @@
 import textwrap
 import traceback

-from collections.abc import Callable
+from collections.abc import Callable, Iterable, Iterator, Sequence
 from types import FunctionType, NoneType
 from typing import (
     Any,
@@ -516,7 +516,11 @@ class PythonLanguage(Language):
     checksum_line = "#/*[{dsl_name} end generated code: {arguments}]*/"


-def permute_left_option_groups(l):
+ParamGroupSequence = Sequence[Iterable["Parameter"]]
+ParamGroupTuple = tuple["Parameter", ...]
+
+
+def permute_left_option_groups(l: ParamGroupSequence) -> Iterator[ParamGroupTuple]:
     """
     Given [(1,), (2,), (3,)], should yield:
        ()
@@ -525,13 +529,13 @@ def permute_left_option_groups(l):
        (1, 2, 3)
     """
     yield tuple()
-    accumulator = []
+    accumulator: list[Parameter] = []
     for group in reversed(l):
         accumulator = list(group) + accumulator
         yield tuple(accumulator)


-def permute_right_option_groups(l):
+def permute_right_option_groups(l: ParamGroupSequence) -> Iterator[ParamGroupTuple]:
     """
     Given [(1,), (2,), (3,)], should yield:
       ()
@@ -540,13 +544,17 @@ def permute_right_option_groups(l):
       (1, 2, 3)
     """
     yield tuple()
-    accumulator = []
+    accumulator: list[Parameter] = []
     for group in l:
         accumulator.extend(group)
         yield tuple(accumulator)


-def permute_optional_groups(left, required, right):
+def permute_optional_groups(
+    left: ParamGroupSequence,
+    required: Iterable[Parameter],
+    right: ParamGroupSequence
+) -> tuple[ParamGroupTuple, ...]:
     """
     Generator function that computes the set of acceptable
     argument lists for the provided iterables of
@@ -561,7 +569,7 @@ def permute_optional_groups(left, required, right):
         if left:
             raise ValueError("required is empty but left is not")

-    accumulator = []
+    accumulator: list[ParamGroupTuple] = []
     counts = set()
     for r in permute_right_option_groups(right):
         for l in permute_left_option_groups(left):

Okay! Here's a diff that makes mypy happy, and I think it has the added virtue of being correct. How does it look to you?

Argh, the names of those type aliases are quite bad actually. (Again, this shows the peril of type aliases!)

Maybe this instead:

--- a/Tools/clinic/clinic.py
+++ b/Tools/clinic/clinic.py
@@ -28,7 +28,7 @@
 import textwrap
 import traceback

-from collections.abc import Callable
+from collections.abc import Callable, Iterable, Iterator, Sequence
 from types import FunctionType, NoneType
 from typing import (
     Any,
@@ -516,7 +516,11 @@ class PythonLanguage(Language):
     checksum_line = "#/*[{dsl_name} end generated code: {arguments}]*/"


-def permute_left_option_groups(l):
+SequenceOfParameterGroups = Sequence[Iterable["Parameter"]]
+ParameterTuple = tuple["Parameter", ...]
+
+
+def permute_left_option_groups(l: SequenceOfParameterGroups) -> Iterator[ParameterTuple]:
     """
     Given [(1,), (2,), (3,)], should yield:
        ()
@@ -525,13 +529,13 @@ def permute_left_option_groups(l):
        (1, 2, 3)
     """
     yield tuple()
-    accumulator = []
+    accumulator: list[Parameter] = []
     for group in reversed(l):
         accumulator = list(group) + accumulator
         yield tuple(accumulator)


-def permute_right_option_groups(l):
+def permute_right_option_groups(l: SequenceOfParameterGroups) -> Iterator[ParameterTuple]:
     """
     Given [(1,), (2,), (3,)], should yield:
       ()
@@ -540,13 +544,17 @@ def permute_right_option_groups(l):
       (1, 2, 3)
     """
     yield tuple()
-    accumulator = []
+    accumulator: list[Parameter] = []
     for group in l:
         accumulator.extend(group)
         yield tuple(accumulator)


-def permute_optional_groups(left, required, right):
+def permute_optional_groups(
+    left: SequenceOfParameterGroups,
+    required: Iterable[Parameter],
+    right: SequenceOfParameterGroups
+) -> tuple[ParameterTuple, ...]:
     """
     Generator function that computes the set of acceptable
     argument lists for the provided iterables of
@@ -561,7 +569,7 @@ def permute_optional_groups(left, required, right):
         if left:
             raise ValueError("required is empty but left is not")

-    accumulator = []
+    accumulator: list[ParameterTuple] = []
     counts = set()
     for r in permute_right_option_groups(right):
         for l in permute_left_option_groups(left):

- Use a single alias: ParamTuple
- Ditch generators, use Iterator
- Use Sequence iso. layers of Iterable/Reversible/etc.

I did a variant with only one (IMO slightly better named) alias ;) Thank you so much! I'm never found out what the key thing was, but it disappeared, so I'm happy 🎉 🪄

This comment was marked as outdated.

I did a variant with only one (IMO slightly better named) alias ;) Thank you so much! I'm never found out what the key thing was, but it disappeared, so I'm happy 🎉 🪄

The key thing was a side-effect of one of the typing errors. In your first version, mypy was incorrectly inferring (due to some errors in your type annotations) that a list in one of your functions was of type list[Parameter | tuple[Parameter, ...]]. The function then called .sort() on that list, and mypy (correctly!) was pointing out that you'd get an exception at runtime if you tried to call .sort() on a list that had both Parameter and tuple elements in it (Parameter and tuple can't be compared with <).

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we're there ;)

This was a very tricky one!

carljm added a commit to carljm/cpython that referenced this pull request

Jul 5, 2023