Skip to content

gh-153970: Fix str() of CalledProcessError when returncode is not an integer #153971

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.

Sign up for GitHub

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

Open
fedonman wants to merge 12 commits into
base: main
Choose a base branch
from
+18 −5
Open

gh-153970: Fix str() of CalledProcessError when returncode is not an integer #153971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e4ebd4f
gh-153970: Fix CalledProcessError.__str__ crash when returncode is None
fedonman Jul 18, 2026
7162066
change implementation per comment by vstinner
fedonman Jul 18, 2026
a6d21b9
Merge branch 'main' into fix-gh-153970-calledprocesserror-none
fedonman Jul 18, 2026
fa1a5d6
gh-153970: Align test and NEWS with the f-string message
fedonman Jul 18, 2026
da53d9b
Merge branch 'fix-gh-153970-calledprocesserror-none' of github.com:fe…
fedonman Jul 18, 2026
8dbfbe3
Merge branch 'main' into fix-gh-153970-calledprocesserror-none
fedonman Jul 21, 2026
f3bd991
gh-153970: Document that CalledProcessError.returncode is an integer
fedonman Jul 22, 2026
5dffa29
Merge branch 'main' into fix-gh-153970-calledprocesserror-none
fedonman Jul 22, 2026
19a86c2
gh-153970: Validate the returncode type in CalledProcessError
fedonman Jul 22, 2026
46d6a68
Merge branch 'main' into fix-gh-153970-calledprocesserror-none
fedonman Jul 22, 2026
0aea374
Merge branch 'main' into fix-gh-153970-calledprocesserror-none
fedonman Jul 26, 2026
fdad6bd
gh-153970: Make str() of CalledProcessError work for any returncode
fedonman Jul 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Doc/library/subprocess.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,8 +236,8 @@ underlying :class:`Popen` interface can be used directly.

.. attribute:: returncode

Exit status of the child process. If the process exited due to a
signal, this will be the negative signal number.
Exit status of the child process, an integer. If the process
exited due to a signal, this will be the negative signal number.

.. attribute:: cmd

Expand Down
6 changes: 3 additions & 3 deletions Lib/subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,16 +143,16 @@ def __init__(self, returncode, cmd, output=None, stderr=None):
self.stderr = stderr

def __str__(self):
if self.returncode and self.returncode < 0:
if isinstance(self.returncode, int) and self.returncode < 0:
try:
return "Command %r died with %r." % (
self.cmd, signal.Signals(-self.returncode))
except ValueError:
return "Command %r died with unknown signal %d." % (
self.cmd, -self.returncode)
else:
return "Command %r returned non-zero exit status %d." % (
self.cmd, self.returncode)
return (f"Command {self.cmd!r} returned non-zero "
f"exit status {self.returncode}.")

@vstinner vstinner Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This two lines changes is the only needed change IMO.

@picnixz picnixz Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But then we are still having the possibility of the TypeError. In this case I would rather just document that we need an error code that is an integer and leave it as is.

@vstinner vstinner Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But then we are still having the possibility of the TypeError

Would you mind to elaborate which operation would raise TypeError? The issue is about str() and the change above does fix str().

With the change above, CalledProcessError can be called with any returncode, it just works.

@picnixz picnixz Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The issue with <0 branch. If you pass [1] you get a type error there

@vstinner vstinner Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh I'm sorry, I completely missed the error on self.returncode < 0.

Well, in that case, I suggest replacing:

if self.returncode and self.returncode < 0:

with:

if isinstance(self.returncode, int) and self.returncode < 0:

IMO we should accept any returncode value in the CalledProcessError constructor and str() should not fail.

@picnixz picnixz Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's slightly awkward for me to accept arbitrary return codes. I would assume them to be integers, and not None or other things, and would rather change the docs. In the first place, the class signature is not exposed so I don't consider it as part of the public API, whether subclassing or not.

But, if you really want to support arbitrary types, I'm ok with this change (+ the __repr__ one), but I really think it's a bit too niche.

@vstinner vstinner Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's slightly awkward for me to accept arbitrary return codes.

We can get a type other than int if there is a bug in subprocess under specific conditions. In that case, I would prefer to be able to create CalledProcessError which stores the full process state (returncode, stdout, stderr), rather than raising a new exception (ex: TypeError) which removes all subprocess context. Running a subprocess is an expensive operation, and you might not able to get the same stdout/stderr if you run the same command twice which would make debugging way harder.

You can also get a non-int returncode when mocking the Popen object. Example:

import unittest.mock
import subprocess

with unittest.mock.patch.object(subprocess, 'Popen'):
    subprocess.check_call(['true'])

Output:

Traceback (most recent call last):
  File "/home/vstinner/python/main/x.py", line 5, in <module>
    subprocess.check_call(['true'])
    ~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
  File "/home/vstinner/python/main/Lib/subprocess.py", line 534, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: <exception str() failed>

@picnixz picnixz Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, the mocking argument convinces me.


@property
def stdout(self):
Expand Down
10 changes: 10 additions & 0 deletions Lib/test/test_subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2449,6 +2449,16 @@ def test_CalledProcessError_str(self):
err = subprocess.CalledProcessError(-9876543, "fake cmd")
self.assertEqual(str(err), "Command 'fake cmd' died with unknown signal 9876543.")

# returncode which is not an integer, which happens for example when
# Popen is mocked: str() must not fail
for returncode in (None, "2", 2.5, [2]):
with self.subTest(returncode=returncode):
err = subprocess.CalledProcessError(returncode, "fake cmd")
self.assertEqual(
str(err),
f"Command 'fake cmd' returned non-zero "
f"exit status {returncode}.")

def test_preexec(self):
# DISCLAIMER: Setting environment variables is *not* a good use
# of a preexec_fn. This is merely a test.
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Library/2026-07-22-12-00-00.gh-issue-153970.Kq7Wn3.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Calling :func:`str` on a :exc:`subprocess.CalledProcessError` no longer
raises :exc:`TypeError` when its :attr:`!returncode` is not an integer, such
as ``None``.
Loading