GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-11874 argparse assertion failure by wimglenn · Pull Request #1826 · python/cpython

Conversation

@wimglenn

@wimglenn wimglenn commented May 27, 2017 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor 
import argparse

parser = argparse.ArgumentParser('prog'*8)
parser.add_argument('--proxy', metavar='<http[s]://example:1234>')

args = parser.parse_args(['--help'])

The script above triggers assertion errors from argparse library code (specifically, from here).

This bug that has been floating around for years, and has already bitten several people before it bit me. See for example:

http://bugs.python.org/issue25058
http://bugs.python.org/issue14046
http://bugs.python.org/issue24089
http://bugs.python.org/issue11874

There are a few ways we could deal with it:

  1. Just remove the asserts, which are bogus.
  2. Disallow using characters (, [, ), ] in metavar strings.
  3. Tighten up the part_regexp pattern.
  4. Don't use regex.

1 - The only consequence here would be that wrapped usage messages might be jacked-up if people use characters like ) and ] in their metavar strings. But this seems like a sloppy and poor resolution.
2 - a sound approach, but it's not backwards compat. Using those troublesome characters in metavars only causes problems when there is line-wrapping, so users may have existing scripts which don't currently hit the issue, and those scripts would suddenly start failing. That's a deal-breaker.
4 - probably the most ideal approach, but it requires rewriting a large chunk of argparse code. Some other guys tried this in issue11874. Their patches have just been sitting there for 5 years with little interest, so I'm hesitant to go down that route again.

Went with 3 in this PR. The tightened up regex also matches a space (or end) after the closing bracket. In fact it is not possible to solve the underlying issue exactly with regex, since metavar can theoretically be any arbitrary string. After my patch, the AssertionError can still fire if a metavar contains a substring like ] -v [. That would be a much rarer/pathological case, unlike the current failure mode which users can and will bump into fairly easily. argparse.py code is not pretty but the test coverage is quite good (1500+ tests) so I'm confident that improving the regex at least doesn't cause any extra issues.

This is the "practicality beats purity" approach.

https://bugs.python.org/issue11874

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
maxfischer2781, jordancoll, funkyfuture, and waynew reacted with thumbs up emoji
All reactions
  • 👍 4 reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 27, 2017
@mention-bot

mention-bot commented May 27, 2017

Copy link
Copy Markdown

@wimglenn, thanks for your PR! By analyzing the history of the files in this pull request, we identified @benjaminp, @bethard and @bitdancer to be potential reviewers.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@wimglenn wimglenn changed the title Argparse assertion failure issue11874 argparse assertion failure May 27, 2017
@wimglenn wimglenn changed the title issue11874 argparse assertion failure bpo-11874 argparse assertion failure May 27, 2017
@wimglenn wimglenn mentioned this pull request Jun 6, 2017
bpo-30583: s/datetuil/dateutil #1972
Merged
merwok
merwok reviewed Sep 27, 2017
View reviewed changes
Comment thread Lib/argparse.py Outdated

@merwok merwok Sep 27, 2017

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.

Minor: you can use automatic string concatenation (`'ab' 'c' == 'abc'), or a multiline regex with the re.M flag.

Sorry, something went wrong.

Uh oh!

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

@wimglenn wimglenn Sep 27, 2017

Copy link
Copy Markdown
Contributor 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 the '|'.join([...]) is OK because it shows the parts of the regex like this OR that OR the_other

Sorry, something went wrong.

Uh oh!

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

@ncoghlan ncoghlan May 20, 2018

Copy link
Copy Markdown
Contributor

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 believe Ezio's point was that you can get that benefit without the runtime cost of the join by writing it as:

part_regexp = (
               r'\(.*?\)+\s|'
               r'\[.*?\]+\s|'
               r'\(.*?\)+$|'
               r'\[.*?\]+$|'
               r'\S+'
           )

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added the awaiting core review label Sep 27, 2017
@wimglenn

wimglenn commented May 16, 2018

Copy link
Copy Markdown
Contributor Author

@ncoghlan Hi Nick, this is the PR I mentioned at PyCon. It seems to have the "awaiting core review" tag already.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ncoghlan
ncoghlan requested changes May 20, 2018
View reviewed changes

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

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.

Thanks for the PR (and it was lovely to meet you at PyCon!). The new test case looks good to me, but I think it may be possible to avoid the new calls to str.strip() by adjusting the way the regex is defined.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/argparse.py Outdated

@ncoghlan ncoghlan May 20, 2018

Copy link
Copy Markdown
Contributor

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 believe Ezio's point was that you can get that benefit without the runtime cost of the join by writing it as:

part_regexp = (
               r'\(.*?\)+\s|'
               r'\[.*?\]+\s|'
               r'\(.*?\)+$|'
               r'\[.*?\]+$|'
               r'\S+'
           )

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_argparse.py Outdated

@ncoghlan ncoghlan May 20, 2018

Copy link
Copy Markdown
Contributor

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 clear how these string -> tuple changes relate to the rest of the PR. They give the impression that tests needed to be changed in order to avoid test failures (which I don't believe to be the case).

Sorry, something went wrong.

Uh oh!

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

@wimglenn wimglenn May 23, 2018

Copy link
Copy Markdown
Contributor 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.

That's why I've isolated them in a separate commit - the tests were bogus, not testing what they claimed to be testing, and actually 4 of the assertions were opposite of what they should have been. Just cleaning up whilst I was touching this file anyway.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/argparse.py Outdated

@ncoghlan ncoghlan May 20, 2018

Copy link
Copy Markdown
Contributor

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 would be desirable to be able to avoid these strip calls by tweaking the way that part_regexp is defined to look for the trailing whitespace, but not actually capture it. I've added a suggestion above for how to do that with a lookahead assertion.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/argparse.py Outdated

@ncoghlan ncoghlan May 20, 2018

Copy link
Copy Markdown
Contributor

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.

Given my comment on the strip calls below, it would be desirable to use a lookahead assertion here so you can require trailing whitespace (or the end of the string) without capturing it in the result. Something like:

r'\(.*?\)+(?=\s|$)'

(Note: I haven't tested that specific regex, but it's hopefully close enough to be a useful starting point)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot

bedevere-bot commented May 20, 2018

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting core review labels May 20, 2018
@ncoghlan

ncoghlan commented May 20, 2018

Copy link
Copy Markdown
Contributor

Regarding the bedevere/news check, note that this will need a bugfix entry in Misc/NEWS.d. See https://devguide.python.org/committing/#what-s-new-and-news-entries for more info on how to generate that.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ncoghlan ncoghlan added needs backport to 3.6 labels May 20, 2018
wimglenn and others added 3 commits May 23, 2018 00:30
@wimglenn
fix some incorrect tests that did not make 1-element tuples correctly
ab9cd7f 
Signed-off-by: Wim Glenn <[email protected]>
@wimglenn
bugfix and test for fragile asserts in argparse library code (see iss…
d536c8f 
…ues 24089, 14046, 25058, 11874)
@wimglenn
Addressed reviewer comments. Added the NEWS.d entry
0fc1cad
@bedevere-bot bedevere-bot removed the awaiting changes label May 23, 2018
@bedevere-bot

bedevere-bot commented May 23, 2018

Copy link
Copy Markdown

Thanks for making the requested changes!

@ncoghlan: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting change review label May 23, 2018
ncoghlan
ncoghlan approved these changes May 23, 2018
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels May 23, 2018
@ncoghlan

ncoghlan commented May 23, 2018

Copy link
Copy Markdown
Contributor

Travis failure was unrelated, so I've restarted the job. (Note for when merging the commit: due to the squash-based workflow, we'll only have one commit in the actual repo, but given the explanation in the review comments, I'm OK with just noting the extra test fixes in the commit message)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ned-deily

ned-deily commented Jun 7, 2018

Copy link
Copy Markdown
Member

@ncoghlan Is this ready to merge?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ncoghlan
ncoghlan merged commit 66f02aa into python:master Jun 8, 2018
@miss-islington

miss-islington commented Jun 8, 2018

Copy link
Copy Markdown
Contributor

Thanks @wimglenn for the PR, and @ncoghlan for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label Jun 8, 2018
@ncoghlan

ncoghlan commented Jun 8, 2018

Copy link
Copy Markdown
Contributor

@ned-deily Indeed - thanks for the reminder!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 8, 2018
@wimglenn @miss-islington
bpo-11874: fix assertion failure in argparse metavar handling (python…
4d52f68 
…GH-1826)

- bugfix and test for fragile metavar handling in argparse (see
  bpo-24089, bpo-14046, bpo-25058, bpo-11874)
- also fixes some incorrect tests that did not make 1-element tuples correctly
(cherry picked from commit 66f02aa)

Co-authored-by: wim glenn <[email protected]>
@bedevere-bot

bedevere-bot commented Jun 8, 2018

Copy link
Copy Markdown

GH-7527 is a backport of this pull request to the 3.7 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.7 label Jun 8, 2018
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 8, 2018
@wimglenn @miss-islington
bpo-11874: fix assertion failure in argparse metavar handling (python…
6967101 
…GH-1826)

- bugfix and test for fragile metavar handling in argparse (see
  bpo-24089, bpo-14046, bpo-25058, bpo-11874)
- also fixes some incorrect tests that did not make 1-element tuples correctly
(cherry picked from commit 66f02aa)

Co-authored-by: wim glenn <[email protected]>
@bedevere-bot

bedevere-bot commented Jun 8, 2018

Copy link
Copy Markdown

GH-7528 is a backport of this pull request to the 3.6 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.6 label Jun 8, 2018
miss-islington added a commit that referenced this pull request Jun 8, 2018
@miss-islington @wimglenn
bpo-11874: fix assertion failure in argparse metavar handling (GH-1826)
376c272 
- bugfix and test for fragile metavar handling in argparse (see
  bpo-24089, bpo-14046, bpo-25058, bpo-11874)
- also fixes some incorrect tests that did not make 1-element tuples correctly
(cherry picked from commit 66f02aa)

Co-authored-by: wim glenn <[email protected]>
@ncoghlan ncoghlan added the needs backport to 3.7 label Jun 8, 2018
@miss-islington

miss-islington commented Jun 8, 2018

Copy link
Copy Markdown
Contributor

Thanks @wimglenn for the PR, and @ncoghlan for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 8, 2018
@wimglenn @miss-islington
bpo-11874: fix assertion failure in argparse metavar handling (python…
9617eb4 
…GH-1826)

- bugfix and test for fragile metavar handling in argparse (see
  bpo-24089, bpo-14046, bpo-25058, bpo-11874)
- also fixes some incorrect tests that did not make 1-element tuples correctly
(cherry picked from commit 66f02aa)

Co-authored-by: wim glenn <[email protected]>
@bedevere-bot

bedevere-bot commented Jun 8, 2018

Copy link
Copy Markdown

GH-7530 is a backport of this pull request to the 3.7 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.7 label Jun 8, 2018
@ncoghlan

ncoghlan commented Jun 8, 2018

Copy link
Copy Markdown
Contributor

(Something odd happened on GH-7527 that led to miss-islington deleting the 3.7 backport branch without merging it first, so I readded the 3.7 label to try again)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
miss-islington added a commit that referenced this pull request Jun 8, 2018
@miss-islington @wimglenn
bpo-11874: fix assertion failure in argparse metavar handling (GH-1826)
842985f 
- bugfix and test for fragile metavar handling in argparse (see
  bpo-24089, bpo-14046, bpo-25058, bpo-11874)
- also fixes some incorrect tests that did not make 1-element tuples correctly
(cherry picked from commit 66f02aa)

Co-authored-by: wim glenn <[email protected]>
@wimglenn
wimglenn deleted the argparse_assertion_failure branch June 8, 2018 16:09
@ncoghlan ncoghlan added the needs backport to 2.7 label Jun 9, 2018
@miss-islington

miss-islington commented Jun 9, 2018

Copy link
Copy Markdown
Contributor

Thanks @wimglenn for the PR, and @ncoghlan for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Jun 9, 2018

Copy link
Copy Markdown

GH-7553 is a backport of this pull request to the 2.7 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 2.7 label Jun 9, 2018
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 9, 2018
@wimglenn @miss-islington
bpo-11874: fix assertion failure in argparse metavar handling (python…
e38ee54 
…GH-1826)

- bugfix and test for fragile metavar handling in argparse (see
  bpo-24089, bpo-14046, bpo-25058, bpo-11874)
- also fixes some incorrect tests that did not make 1-element tuples correctly
(cherry picked from commit 66f02aa)

Co-authored-by: wim glenn <[email protected]>
miss-islington added a commit that referenced this pull request Jun 9, 2018
@miss-islington @wimglenn
bpo-11874: fix assertion failure in argparse metavar handling (GH-1826)
4e6bd24 
- bugfix and test for fragile metavar handling in argparse (see
  bpo-24089, bpo-14046, bpo-25058, bpo-11874)
- also fixes some incorrect tests that did not make 1-element tuples correctly
(cherry picked from commit 66f02aa)

Co-authored-by: wim glenn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merwok merwok merwok left review comments
@ncoghlan ncoghlan ncoghlan approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

8 participants

@wimglenn @mention-bot @bedevere-bot @ncoghlan @ned-deily @miss-islington @merwok @the-knights-who-say-ni
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.