Skip to content

bpo-29298: Fix crash for required subparsers in argparse #18564

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

Closed
wants to merge 3 commits into from
Closed

bpo-29298: Fix crash for required subparsers in argparse #18564

wants to merge 3 commits into from

Conversation

BenSokol
Copy link

@BenSokol BenSokol commented Feb 20, 2020

When an instance of subparsers is has required=True, and no subparser is found,
an uncaught exception was thrown.

OS: Windows - Cygwin
Python Version: 3.8.0b4
This issue still appears to be present in master

Fixes the following stack trace:
...
File "/usr/lib/python3.8/argparse.py", line 1760, in parse_args
args, argv = self.parse_known_args(args, namespace)
File "/usr/lib/python3.8/argparse.py", line 1792, in parse_known_args
namespace, args = self._parse_known_args(args, namespace)
File "/usr/lib/python3.8/argparse.py", line 2027, in _parse_known_args
', '.join(required_actions))
TypeError: sequence item 0: expected str instance, NoneType found

https://bugs.python.org/issue29298

When an instance of subparsers is has required=True, and no subparser is found,
an uncaught exception was thrown.
@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@BenSokol

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@codecov
Copy link

codecov bot commented Feb 20, 2020

Codecov Report

Merging #18564 into master will increase coverage by 0.98%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #18564       +/-   ##
===========================================
+ Coverage   82.15%   83.13%    +0.98%     
===========================================
  Files        1958     1571      -387     
  Lines      589837   414808   -175029     
  Branches    44458    44459        +1     
===========================================
- Hits       484560   344844   -139716     
+ Misses      95636    60337    -35299     
+ Partials     9641     9627       -14     
Impacted Files Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py 30.00% <0.00%> (-65.00%) ⬇️
Lib/distutils/command/bdist_rpm.py 7.63% <0.00%> (-56.88%) ⬇️
Lib/test/test_urllib2net.py 76.92% <0.00%> (-13.85%) ⬇️
Lib/test/test_smtpnet.py 78.57% <0.00%> (-12.86%) ⬇️
Lib/ftplib.py 63.85% <0.00%> (-6.06%) ⬇️
Lib/test/test_ftplib.py 87.11% <0.00%> (-4.72%) ⬇️
Lib/dbm/__init__.py 66.66% <0.00%> (-4.45%) ⬇️
Tools/scripts/db2pickle.py 17.82% <0.00%> (-3.97%) ⬇️
Lib/test/test_socket.py 71.94% <0.00%> (-3.87%) ⬇️
Tools/scripts/pickle2db.py 16.98% <0.00%> (-3.78%) ⬇️
... and 428 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2ee21d...3b4e42b. Read the comment docs.

Copy link
Contributor

@aeros aeros left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and welcome @BenSokol!

It looks like another open PR that addresses this issue (or another part of it) in #3680 is currently open and is not inactive (last comment from author 25 days ago). Does this crash still occur with their patch? At a glance, it looks like they address separate parts of the same BPO issue, but I'd like to be certain.

In the meantime, could you also include a new unit test for this in Lib/test/test_argparse.py, to ensure that the fix functions as intended? This is typically needed for most PRs that make code changes in the standard library, particularly for bug fixes or ones that implement some new functionality. It helps to ensure that the fix works in the first place, and in the long term ensure that the issue remains fixed (to avoid regressions as much as possible).

@aeros aeros added the type-bug An unexpected behavior, bug, or error label Feb 21, 2020
@matrixise
Copy link
Member

Hi @BenSokol

As @aeros said, could you add a unittest?

Thank you

@@ -0,0 +1,2 @@
Fix crash in :mod:`argparse` when using subparsers and its required attribute
Copy link
Member

Choose a reason for hiding this comment

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

creash means segfault, not unhandled exception.

@encukou
Copy link
Member

encukou commented Jul 27, 2021

This was fixed in GH-3680, which includes tests.

@encukou encukou closed this Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants