gh-107704: Argument Clinic: add support for deprecating keyword use of parameters by serhiy-storchaka · Pull Request #107984 · python/cpython · GitHub

It is now possible to deprecate passing keyword arguments for keyword-or-positional parameters with Argument Clinic, using the new '/ [from X.Y]' syntax.
(To be read as "positional-only from Python version X.Y")

Issue: Argument Clinic: add support for deprecating keyword use of parameters #107704

… use of parameters It is now possible to deprecate passing keyword arguments for keyword-or-positional parameters with Argument Clinic, using the new '/ [from X.Y]' syntax. (To be read as "positional-only from Python version X.Y")

Do you want an initial review of the draft, or would you like to wait until it is marked ready for review?

…utput.

I kept it in a draft state for the first run of tests and the final stage of self-review.

It is now ready for review.

I think it would be beneficial to try and reduce some of the code duplication, either in this PR, or in a following-up refactor. Perhaps the latter is preferable.

Regarding _Py_ID:

Unfortunately it is the simplest general way to check that the argument was passed by keyword. It is not the fastest. For inlined parsing code we can use more efficient but more complex code, we can even build it in _PyArg_UnpackKeywords(). But if PyArg_ParseTupleAndKeywords() or similar function is used, the only other way is to use deprecated and inefficient PyMapping_HasKeyString() or introduce a new private inefficient API.

cc. @ericsnowcurrently

What's the concern with using _Py_ID() here?

Currently, there is no concern; I just wanted to point out that even though Argument Clinic does not currently care about the Limited API, we may want to add such functionality in the future; IMO, it should be possible for Argument Clinic to generate code that does not depend on internal APIs, for example for stdlib extension modules such as sqlite3.

Many core devs seem to be in favour of not using internal APIs in stdlib extension modules, but I'm not sure if there is a consensus.

Thank you for your review @erlend-aasland. I tried to address them. Seems the only unresolved issue is with _Py_ID().

Thanks for doing this, @serhiy-storchaka. @AlexWaygood, do you want to have a look?

Some small suggestions for the docs:

Code looks good, just a few very minor nits:

Thank you for your review @AlexWaygood. Do these docstrings work?

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

LGTM. Thanks @serhiy-storchaka!

…ional args.

Post-review changes:

I have found how to make keyword argument check more efficient in case of inlined parsing code. It now has almost zero overhead.
Since the tests with the new generated code no longer covered all cases in the generator, I added new tests, and they exposed bugs in a corner case in constructors (including * [from ...]). Fixed.

…eyword use of parameters (python/cpython#107984) It is now possible to deprecate passing keyword arguments for keyword-or-positional parameters with Argument Clinic, using the new '/ [from X.Y]' syntax. (To be read as "positional-only from Python version X.Y") Co-authored-by: Erlend E. Aasland <erlend@python.org> Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

serhiy-storchaka requested review from AlexWaygood and erlend-aasland August 15, 2023 17:33

serhiy-storchaka force-pushed the clinic-deprecated-keyword branch from 2b3c1e2 to 059cd6a Compare August 15, 2023 17:37

bedevere-bot mentioned this pull request Aug 15, 2023

Argument Clinic: add support for deprecating keyword use of parameters #107704

Closed

serhiy-storchaka added 2 commits August 15, 2023 21:22

Silence check-c-globals.py.

b4b9f3c

Update the documentation.

d5e0064

Make constructor's arguments optional, it produces more interesting o…

b1300c7

…utput.

serhiy-storchaka marked this pull request as ready for review August 15, 2023 19:38

serhiy-storchaka requested a review from berkerpeksag as a code owner August 15, 2023 19:38

bedevere-bot added the awaiting core review label Aug 15, 2023

erlend-aasland reviewed Aug 16, 2023

View reviewed changes

Address review comments.

ad36c15

serhiy-storchaka commented Aug 16, 2023

View reviewed changes

erlend-aasland reviewed Aug 16, 2023

View reviewed changes

Comment thread Tools/clinic/clinic.py Outdated

serhiy-storchaka added 2 commits August 17, 2023 13:31

Add a test for * [from ...] with not inlined parsing code

e271834

Consistently use simple function name in error messages.

e603de7

erlend-aasland approved these changes Aug 18, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Aug 18, 2023

AlexWaygood reviewed Aug 18, 2023

View reviewed changes

Comment thread Doc/howto/clinic.rst Outdated

Comment thread Doc/howto/clinic.rst Outdated

Comment thread Doc/howto/clinic.rst Outdated

AlexWaygood reviewed Aug 18, 2023

View reviewed changes

Comment thread Tools/clinic/clinic.py

Comment thread Tools/clinic/clinic.py Outdated

Comment thread Tools/clinic/clinic.py Outdated

serhiy-storchaka commented Aug 18, 2023

View reviewed changes

Comment thread Tools/clinic/clinic.py

Comment thread Tools/clinic/clinic.py Outdated

Comment thread Tools/clinic/clinic.py Outdated

serhiy-storchaka and others added 3 commits August 18, 2023 16:05

Apply suggestions from code review

3b6f6c4

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

Merge branch 'main' into clinic-deprecated-keyword

94d9b34

Rename _testclinic_depr_star.c.h -> _testclinic_depr.c.h

158c5e3

AlexWaygood approved these changes Aug 18, 2023

View reviewed changes

More efficient checks in case of inlined parsing code.

5c92d36

serhiy-storchaka added 3 commits August 18, 2023 22:20

Fix non-inlined parsing in __init__ and __new__.

6607137

Fix a case of: inlined parsing + no keyword args + omit some some opt…

9fee851

…ional args.

Update Tools/c-analyzer/cpython/globals-to-fix.tsv.

38c9560

Merge branch 'main' into clinic-deprecated-keyword

71ea31c

serhiy-storchaka merged commit 2f31143 into python:main Aug 19, 2023

bedevere-bot removed the awaiting merge label Aug 19, 2023

serhiy-storchaka deleted the clinic-deprecated-keyword branch August 19, 2023 07:13

erlend-aasland mentioned this pull request Apr 2, 2024

Speed up s.startswith() faster-cpython/ideas#671

Open

+                              if func.kind.new_or_init:
+                                  conditions.append(f"nargs < {i+1} && kwargs && PyDict_Contains(kwargs, &_Py_ID({p.name}))")
+                                  containscheck = "PyDict_Contains"
+                              else:
+                                  conditions.append(f"nargs < {i+1} && kwnames && PySequence_Contains(kwnames, &_Py_ID({p.name}))")
+                                  containscheck = "PySequence_Contains"

gh-107704: Argument Clinic: add support for deprecating keyword use of parameters by serhiy-storchaka · Pull Request #107984 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-107704: Argument Clinic: add support for deprecating keyword use of parameters - #107984

gh-107704: Argument Clinic: add support for deprecating keyword use of parameters#107984serhiy-storchaka merged 15 commits intopython:mainpython/cpython:mainfrom serhiy-storchaka:clinic-deprecated-keywordserhiy-storchaka/cpython:clinic-deprecated-keywordCopy head branch name to clipboard

Conversation

serhiy-storchaka commented Aug 15, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented Aug 15, 2023

Uh oh!

serhiy-storchaka commented Aug 15, 2023

Uh oh!

erlend-aasland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erlend-aasland Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

erlend-aasland Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

erlend-aasland Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erlend-aasland left a comment

Choose a reason for hiding this comment

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Aug 19, 2023

Uh oh!

gh-107704: Argument Clinic: add support for deprecating keyword use of parameters#107984
serhiy-storchaka merged 15 commits into
python:mainfrom
serhiy-storchaka:clinic-deprecated-keyword

serhiy-storchaka commented Aug 15, 2023 •
edited by bedevere-bot
Loading