Skip to content

gh-153967: handle invalid file object in argparse._print_message - #153969

Merged
savannahostrowski merged 15 commits into
python:mainfrom
ptim0626:argparse-invalid-file-error
Aug 7, 2026
Merged

gh-153967: handle invalid file object in argparse._print_message#153969
savannahostrowski merged 15 commits into
python:mainfrom
ptim0626:argparse-invalid-file-error

Conversation

@ptim0626

@ptim0626ptim0626 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

The fix for the above issue, which raises a ValueError if an invalid file is explicitly passed to argparse.print_usage and argparse.print_help. Tests added.

@bedevere-app

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@python-cla-bot

python-cla-botBot commented Jul 18, 2026

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

Comment threadLib/test/test_argparse.py
Comment threadLib/test/test_argparse.py Outdated
Comment threadLib/argparse.py Outdated
@ZeroIntensity

Copy link
Copy Markdown
Member

This is also a user-facing change; please add a news entry.

@bedevere-app

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@ptim0626

Copy link
Copy Markdown
ContributorAuthor

This is also a user-facing change; please add a news entry.

Thanks, and this was added.

Comment threadMisc/NEWS.d/next/Library/2026-07-18-16-05-38.gh-issue-153967.-OUNXe.rst Outdated
@ptim0626

Copy link
Copy Markdown
ContributorAuthor

Now I have no idea why test_ctypes failed in the CI...

@ZeroIntensity

Copy link
Copy Markdown
Member

That would be #154106. I just updated the branch, which should fix it.

@ptim0626

Copy link
Copy Markdown
ContributorAuthor

The merge fixed the test_ctypes but the sanitizer build failed with test_external_inspection.

@chris-eibl

chris-eibl commented Jul 19, 2026

Copy link
Copy Markdown
Member

No worries. test_external_inspection is a known flaky test. Also the readthedocs hiccuped - I hit "update branch" to trigger CI again and hope for the best :)

Comment threadLib/argparse.py Outdated
if file is None:
file = _sys.stderr
if file is not None:
file.write(message)

@savannahostrowskisavannahostrowskiAug 5, 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.

Could we continue to suppress OSError? If writing to sys.stderr raises OSError during ArgumentParser.exit(), the exception escapes before _sys.exit(status) is reached, changing the exception from SystemExit to OSError. We should also add a test for this, probably.

Suggested change
file.write(message)
try:
file.write(message)
exceptOSError:
pass

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks for catching this so the existing behaviour is kept. I have put a test and made the change accordingly.

One issue of catching OSError inside _print_message is that if, e.g. a non-writable file object, is passed to print_usage/print_help, it will silently fail. The users may benefit from getting a clearer message about what's gone wrong if an error message io.UnsupportedOperation: not writable is emitted (an example of passing a non-writable file). A lot of file-related exceptions are inherited from OSError such as PermissionError etc. Instead of putting the try ... except inside _print_message, could we do

defexit(self, status=0, message=None):
ifmessage:
try:
self._print_message(message, _sys.stderr)
exceptOSError:
pass_sys.exit(status)

This will:

  • ensure argparse.exit still raises SystemExit when sys.stderr raises OSError
  • give clearer exception message when invalid file object (e.g. permission issue, non-writable, wrong file path etc) is passed to print_usage/print_help and not silently failed

Happy to keep the current state if it is more appropriate.

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.

That makes sense for explicitly supplied output destinations, but I think changing where OSError propagates is broader than the original fix. Catching it only in exit() also would not cover error(), since that calls print_usage(_sys.stderr) before reaching exit().

I’d prefer to keep the current behavior in this PR and handle more precise OSError propagation separately, where we can consider explicit destinations, default streams, and the other output-and-exit paths together.

@github-project-automationgithub-project-automationBot moved this from Todo to In Progress in SprintAug 7, 2026
@savannahostrowski
savannahostrowski enabled auto-merge (squash) August 7, 2026 18:08
@savannahostrowski
savannahostrowski merged commit 115400b into python:mainAug 7, 2026
50 checks passed
@github-project-automationgithub-project-automationBot moved this from In Progress to Done in SprintAug 7, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants

@ptim0626@ZeroIntensity@chris-eibl@tomasr8@savannahostrowski