GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-104050: Annotate Argument Clinic parameter permutation helpers by erlend-aasland · Pull Request #106431 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Jul 4, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  • Issue: Add type annotations to clinic.py #104050

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@erlend-aasland
pythongh-104050: Annotate Argument Clinic parameter permutation helpers
81adeb5
@erlend-aasland
erlend-aasland requested a review from AlexWaygood July 4, 2023 19:15
@bedevere-bot bedevere-bot mentioned this pull request Jul 4, 2023
Add type annotations to clinic.py #104050
Closed
AlexWaygood
AlexWaygood reviewed Jul 4, 2023
View reviewed changes
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Jul 4, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

<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>

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
Address review: import Generator from collections.abc
3667a17
@AlexWaygood

AlexWaygood commented Jul 4, 2023

Copy link
Copy Markdown
Member

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Jul 4, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

:( 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 😄

AlexWaygood reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Jul 4, 2023

Copy link
Copy Markdown
Contributor Author

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

AlexWaygood reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Jul 4, 2023

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood
AlexWaygood reviewed Jul 4, 2023
View reviewed changes
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood

AlexWaygood commented Jul 4, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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 :)

erlend-aasland reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood

AlexWaygood commented Jul 4, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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):
erlend-aasland reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood

AlexWaygood commented Jul 4, 2023

Copy link
Copy Markdown
Member

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):
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood AlexWaygood added the skip news label Jul 4, 2023
@erlend-aasland
Address review:
c6241b0
- Use a single alias: ParamTuple
- Ditch generators, use Iterator
- Use Sequence iso. layers of Iterable/Reversible/etc.
@erlend-aasland

erlend-aasland commented Jul 4, 2023

Copy link
Copy Markdown
Contributor Author

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 🎉 🪄

AlexWaygood reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
erlend-aasland marked this pull request as ready for review July 4, 2023 20:27
@bedevere-bot bedevere-bot added the awaiting core review label Jul 4, 2023
@erlend-aasland

This comment was marked as outdated.

Sign in to view
AlexWaygood
AlexWaygood reviewed Jul 4, 2023
View reviewed changes
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood

AlexWaygood commented Jul 4, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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 <).

erlend-aasland reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland added 3 commits July 4, 2023 23:18
@erlend-aasland
Pull in main
f05fc6e
@erlend-aasland
Address review: make it less specific
d854fbd
@erlend-aasland
Add ParamGroup alias
8d1821c
AlexWaygood
AlexWaygood reviewed Jul 4, 2023
View reviewed changes
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland @AlexWaygood
Update Tools/clinic/clinic.py
40cc1dc
Co-authored-by: Alex Waygood <[email protected]>
AlexWaygood
AlexWaygood approved these changes Jul 4, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Jul 4, 2023
@erlend-aasland
erlend-aasland enabled auto-merge (squash) July 4, 2023 21:52
@erlend-aasland
erlend-aasland merged commit 2208751 into python:main Jul 4, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Jul 4, 2023
@erlend-aasland
erlend-aasland deleted the clinic/annotate-permutators branch July 4, 2023 22:44
carljm added a commit to carljm/cpython that referenced this pull request Jul 5, 2023
@carljm
Merge branch 'main' into modulelocals
4c59e51
* main: (39 commits)
  pythongh-102542 Remove unused bytes object and bytes slicing (python#106433)
  Clarify state of CancelledError in doc (python#106453)
  pythongh-64595: Fix regression in file write logic in Argument Clinic (python#106449)
  pythongh-104683: Rename Lib/test/clinic.test as Lib/test/clinic.test.c (python#106443)
  tp_flags docs: fix indentation (python#106420)
  pythongh-104050: Partially annotate Argument Clinic CLanguage class (python#106437)
  pythongh-106368: Add tests for formatting helpers in Argument Clinic (python#106415)
  pythongh-104050: Annotate Argument Clinic parameter permutation helpers (python#106431)
  pythongh-104050: Annotate toplevel functions in clinic.py (python#106435)
  pythongh-106320: Fix specialize.c compilation by including pycore_pylifecycle.h (python#106434)
  Add some codeowners for `Tools/clinic/` (python#106430)
  pythongh-106217: Truncate the issue body size of `new-bugs-announce-notifier` (python#106423)
  pythongh-61215: Rename `wait_until_any_call` to `wait_until_any_call_with` (python#106414)
  pythongh-106162: array: suppress warning in test_array (python#106404)
  pythongh-106320: Remove _PyInterpreterState_HasFeature() (python#106425)
  pythonGH-106360: Support very basic superblock introspection (python#106422)
  pythongh-106406: Fix _Py_IsInterpreterFinalizing() in _winapi.c (python#106408)
  pythongh-106396: Special-case empty format spec to gen empty JoinedStr node (python#106401)
  pythongh-106368: Add tests for permutation helpers in Argument Clinic (python#106407)
  pythonGH-106008: Fix refleak when peepholing `None` comparisons (python#106367)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlexWaygood AlexWaygood AlexWaygood approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@erlend-aasland @AlexWaygood @bedevere-bot
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.