gh-152060: Fix datetime.fromisoformat() raising AssertionError in pure Python - #152061
Conversation
Sorry, something went wrong.
…in pure Python _pydatetime._parse_isoformat_date() asserted the date portion length, leaking a bare AssertionError out of datetime.fromisoformat() for some malformed strings (e.g. '2020-2020'), and behaving differently under -O. The C accelerator already raises ValueError. Replace the assert with an explicit ValueError so both implementations agree on all builds.
| # see the comment on Modules/_datetimemodule.c:_find_isoformat_datetime_separator | ||
| assert len(dtstr) in (7, 8, 10) | ||
| if len(dtstr) not in (7, 8, 10): | ||
| raise ValueError(f"Invalid isoformat string: {dtstr!r}") |
There was a problem hiding this comment.
{dtstr!r} won't match the C impl, but it doesn’t matter as fromisoformat re-raises anyway.
| raise ValueError(f"Invalid isoformat string: {dtstr!r}") | |
| raise ValueError(f"Invalid isoformat string") |
Sorry, something went wrong.
There was a problem hiding this comment.
Done, thanks!
Sorry, something went wrong.
| '2009-04-19T12:30:45-00:90:00', # Time zone field out from range | ||
| '2009-04-19T12:30:45-00:00:90', # Time zone field out from range | ||
| '2020-2020', # Ambiguous 9-char date portion | ||
| '2020-1234', # Ambiguous 9-char date portion |
There was a problem hiding this comment.
| '2020-1234', # Ambiguous 9-char date portion |
These cases are identical.
Sorry, something went wrong.
There was a problem hiding this comment.
Done, thanks!
Sorry, something went wrong.
|
Thanks @tonghuaroot for the PR, and @StanFromIreland for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14, 3.15. |
Sorry, something went wrong.
|
GH-152081 is a backport of this pull request to the 3.15 branch. |
Sorry, something went wrong.
|
Sorry, @tonghuaroot and @StanFromIreland, I could not cleanly backport this to 3.14 due to a conflict. cherry_picker ff781d52d451db56e154aac35ae7f2c41b1695a4 3.14 |
Sorry, something went wrong.
|
Sorry, @tonghuaroot and @StanFromIreland, I could not cleanly backport this to 3.13 due to a conflict. cherry_picker ff781d52d451db56e154aac35ae7f2c41b1695a4 3.13 |
Sorry, something went wrong.
|
@tonghuaroot are you able to do the backports? |
Sorry, something went wrong.
|
@tonghuaroot oh sorry, these should have been done first before #152765 and #152766. |
Sorry, something went wrong.
|
GH-152786 is a backport of this pull request to the 3.14 branch. |
Sorry, something went wrong.
|
GH-152787 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
|
Done: GH-152786 (3.14) and GH-152787 (3.13). Cherry-picked manually since the bad_strs list in datetimetester.py had diverged from main; built both branches and ran test_datetime (green on each). If landing these first makes GH-152765/GH-152766 conflict, I’ll rebase that pair. |
Sorry, something went wrong.
Fixes #152060.
_pydatetime._parse_isoformat_date() asserted that the date portion has length 7, 8 or
10. For some malformed inputs (e.g. '2020-2020', '2020-1234') the separator finder
yields a 9-character date portion, so the assert fires and a bare AssertionError escapes
datetime.fromisoformat(), which is documented to raise only ValueError. The C
accelerator already raises ValueError. Because the check is an assert, the input also
raises ValueError instead under python -O, so the behaviour differed by build flag.
This replaces the assert with an explicit ValueError whose message matches the C
implementation, so both implementations agree on every build. This is the sibling assert
site to the one fixed in #151771 (gh-151770).
date.fromisoformat() already validates the length before calling
_parse_isoformat_date(), so it is unaffected.
A regression test is added to test_fromisoformat_fails_datetime, which runs under both
the C (_Fast) and pure-Python (_Pure) test classes.