GeezerVison

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

GeezerVison is showing github.com — gh-133548: fix handling of empty and 1-item tuples for `sys.exit` by picnixz · Pull Request #135789 · python/cpython

Conversation

picnixz commented Jun 21, 2025 •

edited by bedevere-app Bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

cc @markshannon @iritkatriel

Issue: sys.exit unpacks its argument if it is a 0- or 1-element tuple #133548

Sorry, something went wrong.

Uh oh!

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

All reactions


          fix handling of empty and 1-item tuples for sys.exit

ae37e95

picnixz requested a review from ericsnowcurrently as a code owner

June 21, 2025 12:18

bedevere-app Bot added the awaiting core review label

Jun 21, 2025

bedevere-app Bot mentioned this pull request

Jun 21, 2025

sys.exit unpacks its argument if it is a 0- or 1-element tuple #133548

Open

picnixz added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label

Jun 21, 2025

bedevere-bot commented Jun 21, 2025

Copy link

Copy Markdown

🤖 New build scheduled with the buildbot fleet by @picnixz for commit ae37e95 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F135789%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label

Jun 21, 2025

picnixz requested review from iritkatriel and markshannon

June 21, 2025 16:29


          reduce a bit the diff

6b65314

picnixz force-pushed the fix/exception/sys-exit-133548 branch from bcc2970 to 6b65314 Compare

June 23, 2025 07:52

iritkatriel reviewed

Jun 23, 2025

View reviewed changes

Comment thread

Python/sysmodule.c

		}
		else {
		PyErr_SetObject(PyExc_SystemExit, status);
		}

iritkatriel Jun 23, 2025

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.

Isn't this a problem with how PyErr_SetObject creates the exception? Shouldn't it be fixed there?

Sorry, something went wrong.

Uh oh!

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

All reactions

picnixz Jun 24, 2025

Copy link

Copy Markdown

Member Author

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.

I think it's a bit unfortunate because that's how _PyErr_CreateException handles tuples:

static PyObject*
_PyErr_CreateException(PyObject *exception_type, PyObject *value)
{
    PyObject *exc;

    if (value == NULL || value == Py_None) {
        exc = _PyObject_CallNoArgs(exception_type);
    }
    else if (PyTuple_Check(value)) {
        exc = PyObject_Call(exception_type, value, NULL);
    }
    else {
        exc = PyObject_CallOneArg(exception_type, value);
    }

    if (exc != NULL && !PyExceptionInstance_Check(exc)) {
        PyErr_Format(PyExc_TypeError,
                     "calling %R should have returned an instance of "
                     "BaseException, not %s",
                     exception_type, Py_TYPE(exc)->tp_name);
        Py_CLEAR(exc);
    }

    return exc;
}

Unless you want me to change _PyErr_CreateException to prevent the fast paths, it could be annoying for downstream users.

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner May 11, 2026

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.

Changing PyErr_SetObject() is a much larger change, it should be carefully prepared since it would affect many C extensions.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Misc/NEWS.d/next/Library/2025-06-21-14-15-57.gh-issue-133548.zuMuLb.rst Resolved

Uh oh!

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

picnixz added 2 commits

June 24, 2025 14:15


          add What's New entry


          improve comment.

a5c1cd8

iritkatriel reviewed

Jun 24, 2025

View reviewed changes

Comment thread

Python/sysmodule.c

		if (PyTuple_Check(status)) {
		/* Make sure that tuples are not flattened during normalization. */
		/* Make sure that tuples are not flattened during normalization
		* due to the fast path for tuples in _PyErr_CreateException(). */

iritkatriel Jun 24, 2025

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.

It's not a "fast path" if removing it changes behaviour. We need to understand whether _PyErr_CreateException has a bug or not.

Sorry, something went wrong.

Uh oh!

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

All reactions

picnixz Jun 24, 2025

Copy link

Copy Markdown

Member Author

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.

I think it was designed as such for conveniency. It was added in 3a84097 but AFAICT, that was just refactoring of c0dc92a (1997) which already had this fast path.

Sorry, something went wrong.

Uh oh!

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

All reactions

github-actions Bot commented Apr 26, 2026

Copy link

Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

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

github-actions Bot added the stale Stale PR or inactive for long period of time. label

Apr 26, 2026

vstinner reviewed

May 11, 2026

View reviewed changes

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

I like this approach but I have remarks on the implementation.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Doc/whatsnew/3.15.rst

		* Several error messages incorrectly using the term "argument" have been corrected.
		(Contributed by Stan Ulbrych in :gh:`133382`.)

		* :func:`sys.exit` and :exc:`SystemExit` now correctly handle empty and 1-item

vstinner May 11, 2026

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.

There is no need to mention SystemExit, the PR only changes sys.exit().

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Python/sysmodule.c

		* due to the fast path for tuples in _PyErr_CreateException(). */
		PyObject *exc = PyObject_CallOneArg(PyExc_SystemExit, status);
		PyErr_SetObject(PyExc_SystemExit, exc);
		Py_DECREF(exc);

vstinner May 11, 2026

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.

exc can be NULL. You can use:

    PyObject *exc = PyObject_CallOneArg(PyExc_SystemExit, status);
    if (exc != NULL) {
        PyErr_SetObject(PyExc_SystemExit, exc);
        Py_DECREF(exc);
    }

I don't think that we need a special path if PyTuple_Check(). Use the same code path for any status.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Python/sysmodule.c

		}
		else {
		PyErr_SetObject(PyExc_SystemExit, status);
		}

vstinner May 11, 2026

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.

Changing PyErr_SetObject() is a much larger change, it should be carefully prepared since it would affect many C extensions.

Sorry, something went wrong.

Uh oh!

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

All reactions

github-actions Bot removed the stale Stale PR or inactive for long period of time. label

May 15, 2026

github-actions Bot commented Aug 17, 2026

Copy link

Copy Markdown

This PR is stale because it has been open for 90 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

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

github-actions Bot added the stale Stale PR or inactive for long period of time. label

Aug 17, 2026

Reviewers

vstinner

vstinner left review comments

iritkatriel

iritkatriel left review comments

ericsnowcurrently

Awaiting requested review from ericsnowcurrently

ericsnowcurrently is a code owner

markshannon

Awaiting requested review from markshannon

Uh oh!

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

Assignees

No one assigned

Labels

awaiting core review stale

Stale PR or inactive for long period of time.

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.

4 participants

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.

Conversation

picnixz commented Jun 21, 2025 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Jun 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 26, 2026

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 17, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

picnixz commented Jun 21, 2025 •

edited by bedevere-app Bot

Loading

Uh oh!

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