python
gh-64490: Fix bugs in argument clinic varargs processing
Fix 3 bugs introduced in #18609.
Bug reason lists in each commit's commit message.
https://bugs.python.org/issue20291
- Issue: gh-64490
Most changes to Python require a NEWS entry.
Please add it using the blurb_it web app or the blurb command-line tool.
NEWS added :)
Thanks; that's some entry! :) Could you narrow it down to a couple of concise sentences that describe the actual changes only?
Also, could you please add tests for each case?
Do you mean put the test cases into test_linic.py?
To see if the OOB bug is fixed in commit https://github.com/python/cpython/pull/32092/commits/620396279cf94e09bd7cd6e925e6e80471130d6f , I should write some code snippet calling function _PyArg_UnpackKeywordsWithVararg, compile it and ensure that it does not crash.
I'm not sure how can I do this without touching the Python vm or library, though I have wrote a proof-of-concept by adding a built-in function and test it manually: https://github.com/colorfulappl/cpython/tree/unpack_keywords_oob_bug_poc .
As for commit https://github.com/python/cpython/pull/32092/commits/6793f3838077e3cf12d4e44059396478757522b2, I'm looking at test_linic.py and will have a try later.
I think we should have functional tests for AC but currently there aren't any so that's a separate issue.
+1
Would you mind opening an issue[^1]?
[^1]: assuming there is none open already
I took the liberty of editing the NEWS entry. Leave a thumbs-up if it looks ok to you.
I'll land this in a couple of days. Kumar, do you think Eric is interested in reviewing this? I guess he is busy with other PRs and PEPs, so I'll refrain from pinging him.
I'll land this in a couple of days. Kumar, do you think Eric is interested in reviewing this? I guess he is busy with other PRs and PEPs, so I'll refrain from pinging him.
Ya, he is busy with other work and PEP. This is small so I think safe to land.
Ok, then I'll think we're ready for landing; I'll merge it later tonight.
I see that Batuhan self-requested a review; I'll give him a couple of more days to do it before landing. cc. @isidentical
(I'm working my way through some PRs which have been approved and are labeled "awaiting merge", hence my seemingly bolt from the blue comment. Why? Read here.)
I looked through the comments. Is it still waiting for further changes?
Is it still waiting for further changes?
Since gh-96178 has recently been merged, we finally have a test framework for functional clinic tests, so I'd say this PR should add some tests before we are ready to land.
I added some test cases and did another change in a48971d, so this PR may be necessary to be reviewed again. @erlend-aasland @isidentical @kumaraditya303
Another change in e6a4e13:
Argument Clinic does not generate noptargs when there is a vararg and no optional argument.
Thanks @colorfulappl for the PR, and @erlend-aasland for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11. 🐍🍒⛏🤖
Sorry, @colorfulappl and @erlend-aasland, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 0da728387c99fe6c127b070f2d250dc5bdd62ee5 3.11
Sorry @colorfulappl and @erlend-aasland, I had trouble checking out the 3.10 backport branch.
Please retry by removing and re-adding the "needs backport to 3.10" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 0da728387c99fe6c127b070f2d250dc5bdd62ee5 3.10
@kumaraditya303 I'm inclined to backport the functional tests for AC to 3.11 and 3.10. What do you think? We've backported tests to increase coverage in the maintained branches before; it would help backporting the AC fixes (and by the way, we should backport the AC bug fixes that we landed earlier today)
I have backported this to 3.11 (#100368). And since 3.10 Argument Clinic does not support varargs parsing, there is no need to backport this to 3.10, IMHO.