Repository navigation
subprocess: CalledProcessError.__str__ crashes when returncode is None #153970
Description
Activity
I'm not entirely sure you're meant to create
CalledProcessErroryourself. The docs say:Subclass of SubprocessError, raised when a process run by check_call(), check_output(), or run() (with check=True) returns a non-zero exit status
So I don't thinkt he constructor is meant to be part of the public API. Note that the constructor is not documented either.
@picnixz Fair point, and you're right that the constructor isn't documented, and that subprocess itself always builds this with a real integer return code.
The reasoning is less "people construct this directly" and more that an exception's
__str__shouldn't itself raise.CalledProcessErrorand itsreturncodeattribute are public and documented, and exceptions do get constructed and re-raised by test code and by third-party libraries that imitatesubprocess. If one of those ends up withreturncode=Noneand propagates, the traceback machinery callsstr()on it and you get a confusing chainedTypeErrorthat hides the real error instead of a readable message.I'm not entirely sure you're meant to create
CalledProcessErroryourself.Me neither, but if someone is creating a new subclass they could benefit from not having an admittedly hard to hit footgun. E.g.:
class StrictCompletedProcess(subprocess.CompletedProcess): def check_returncode(self): if self.returncode != 0: # looks more explicit/correct... raise subprocess.CalledProcessError( self.returncode, self.args, self.stdout, self.stderr) cp = StrictCompletedProcess(args=["mycommand"], returncode=None) cp.check_returncode()
I'm still unsure about this. In this case, I would rather leave it as is because it's already documented as being an exit code, so an integer. We don't do overly defensive programming in the stdlib.
Reacted by Vyron VasileiadisShould I close the issue?
I personally think it's not relevant here. We can improve the documentation though, but I don't see a reason for raising a
ValueErrorin__str__for None exit codes. If such code can be legitimately obtained by the public API, then we ought to fix it, sure. But such fix should not raise aValueErroreither and rather present a different__str__.cc @gpshead
If one of those ends up with returncode=None and propagates, the traceback machinery calls str() on it and you get a confusing chained TypeError that hides the real error instead of a readable message.
FTR, we would still consider a GIGO case. The exception would still point at the
__str__method so it wouldn't help for much more for debugging either.After looking at Victor's suggestion on the PR, I think we can just use
%sinstead of%d(or an f-string) for the return code. It would make it more friendly while allowing arbitrary types.5 remaining items
- added 4 commits that reference this issue
on Jul 22, 2026 - added a commit that references this issue
on Aug 7, 2026 - added a commit that references this issue
on Aug 17, 2026 It needs a backport to 3.15 (which may be blocked by other backports).
It needs a backport to 3.15 (which may be blocked by other backports).
Yes. I already had this PR in my "3.15 backport queue". The PR cannot be created just with
git cherry-pick -x 7c653e2540cbe4180efb3bd83b63865a1f675650, other changes should be backported to 3.15 first.- added a commit that references this issue
on Sep 29, 2026 - added a commit that references this issue
on Oct 9, 2026 It needs a backport to 3.15 (which may be blocked by other backports).
I just merged the 3.15 backport. Thanks for the reminder!
Calling
str()on aCalledProcessErrorcrashes if itsreturncodeisNone.The
__str__method usesif self.returncode and self.returncode < 0for the "died with a signal" case, and otherwise formats the return code with%d. WhenreturncodeisNone, that first check is falsy, so it falls through to the%dbranch, and%dcan't formatNone, so it raisesTypeError. Having the exception's own string representation blow up is a pretty bad way to fail.The fix is to check for
returncode is Nonebefore the%dbranch and return a plain message instead.Found while going through devdanzin's audit of the standard library, item 9: https://gist.github.com/devdanzin/3198710e3c0128fda5e0a7b4e0768e5f
Linked PRs