Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-104050: Annotate Argument Clinic parameter permutation helpers#106431

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commentedJul 4, 2023
edited by bedevere-bot
Loading

@erlend-aasland
Copy link
ContributorAuthor

erlend-aasland commentedJul 4, 2023
edited
Loading

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

@AlexWaygood
Copy link
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.

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

@erlend-aasland
Copy link
ContributorAuthor

erlend-aasland commentedJul 4, 2023
edited
Loading

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

@erlend-aasland
Copy link
ContributorAuthor

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

AlexWaygood reacted with heart emoji

@erlend-aasland
Copy link
ContributorAuthor

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 thelist(group) thing? Also, I'm puzzled by thekey param complaint.

@AlexWaygood
Copy link
Member

AlexWaygood commentedJul 4, 2023
edited
Loading

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

erlend-aasland reacted with heart emoji

@AlexWaygood
Copy link
Member

AlexWaygood commentedJul 4, 2023
edited
Loading

Okay! Here's a diff that makes mypy happy, and Ithink 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

@AlexWaygood
Copy link
Member

Okay! Here's a diff that makes mypy happy, and Ithink 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.
@erlend-aasland
Copy link
ContributorAuthor

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

AlexWaygood reacted with heart emoji

@erlend-aaslanderlend-aasland marked this pull request as ready for reviewJuly 4, 2023 20:27
@erlend-aasland

This comment was marked as outdated.

@AlexWaygood
Copy link
Member

AlexWaygood commentedJul 4, 2023
edited
Loading

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

Thekey 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 typelist[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 bothParameter andtuple elements in it (Parameter andtuple can't be compared with<).

erlend-aasland reacted with eyes emoji

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

@AlexWaygoodAlexWaygood left a 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!

erlend-aasland reacted with hooray emoji
@erlend-aaslanderlend-aaslandenabled auto-merge (squash)July 4, 2023 21:52
@erlend-aaslanderlend-aasland merged commit2208751 intopython:mainJul 4, 2023
@erlend-aaslanderlend-aasland deleted the clinic/annotate-permutators branchJuly 4, 2023 22:44
carljm added a commit to carljm/cpython that referenced this pull requestJul 5, 2023
* 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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@AlexWaygoodAlexWaygoodAlexWaygood approved these changes

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@erlend-aasland@AlexWaygood@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp