Skip to content

GH-132775: Fix argument parsing for _interpqueues.put() #137686

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 21, 2025

Conversation

AA-Turner
Copy link
Member

@AA-Turner AA-Turner commented Aug 12, 2025

serhiy-storchaka

This comment was marked as resolved.

@ericsnowcurrently
Copy link
Member

Just be sure I understood, the problem is that the arg spec for _queues.put() has one too many items in it. Is that right?

I'm pretty sure the $p shouldn't be there. The "fallback" arg is an int flag 1, so it should have an "i" in the spec. Thus, the spec for queuesmod_create() should not change and for queuesmod_put() it should be "O&O|ii:put".

FWIW, at the time, with the feature freeze at hand, I didn't worry about ironing out little API quirks (e.g. placeholder args) in the low-level module. It isn't exposed to users, right?

Footnotes

  1. See resolve_fallback() in Modules/_interpreters_common.h.

@ericsnowcurrently
Copy link
Member

To be clear, I left room for other "fallback" options later, rather than it being a definite binary choice. Sorry that wasn't more clear. Then again, that decision was probably a premature accommodation for a possible future. I figure it doesn't matter much though.

@AA-Turner

This comment was marked as resolved.

@AA-Turner
Copy link
Member Author

Just be sure I understood, the problem is that the arg spec for _queues.put() has one too many items in it. Is that right?

Yes, specifically that the format spec for PyArg_ParseTupleAndKeywords expected five args, but kwlist gave only four.

I'm pretty sure the $p shouldn't be there. The "fallback" arg is an int flag 1, so it should have an "i" in the spec. Thus, the spec for queuesmod_create() should not change and for queuesmod_put() it should be "O&O|ii:put".

I've updated the PR. I don't think we need the test anymore given this is the current behaviour -- we are just removing the errant $p. Please would you be able to review/approve the updated PR?

FWIW, at the time, with the feature freeze at hand, I didn't worry about ironing out little API quirks (e.g. placeholder args) in the low-level module. It isn't exposed to users, right?

Definitely the correct choice. _interpqueues is technically exposed to users by dint of being importable, but is entirely undocumented and of course a private module. This PR is just a correctness fix that Serhiy spotted, but it would nevertheless be good to include it in 3.14.0 candidate 3 / final.

A

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

@AA-Turner AA-Turner merged commit 79aeeb8 into python:main Aug 21, 2025
46 checks passed
@miss-islington-app
Copy link

Thanks @AA-Turner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

@AA-Turner AA-Turner deleted the clinic/interpqueues-put branch August 21, 2025 22:01
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 21, 2025
…thonGH-137686)

(cherry picked from commit 79aeeb8)

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@bedevere-app
Copy link

bedevere-app bot commented Aug 21, 2025

GH-138034 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Aug 21, 2025
@serhiy-storchaka
Copy link
Member

And I think that it would be better to remove passing explicit -1 as the fallback argument to _queue.create().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants