gh-127527: Improve error handling in time_strftime1 #127528

hoodmane · 2024-12-02T18:52:17Z

This fixes python -m test test_datetime -mtest_strftime_trailing_percent on Emscripten. On other musl platforms it will:

if HAVE_WCSFTIME is true, change strftime("format string ending in %") from returning empty string to raising ValueError("Invalid format string")
if HAVE_WCSFTIME is false, change strftime("format string ending in %") from raising ValueError("embedded null byte") to ValueError("invalid format string").

It also should change the behavior for other unsupported behaviors from returning empty string to raising Invalid format string. Also, if the result is actually empty, the exit condition (*outbuf)[buflen] == '\0' is always true the first time and allows us to avoid resizing the buffer and calling format_time again.

Issue: Musl libc strftime for format strings ending in % #127527

nineteendo · 2024-12-02T18:57:19Z

Can you add tests and a news entry?

hoodmane · 2024-12-02T19:02:42Z

I can add a news entry. I'll have to think about tests. There's already coverage for this in any job that uses Musl such as the wasi job. Hopefully if I read the glibc implementation of strftime / wcsftime I can find a format string that will hit the new codepath on glibc as well as musl.

nineteendo · 2024-12-02T19:11:08Z

Well, in any case these tests in test.test_strptime.StrptimeTests need to be updated:

test_date_locale
test_date_locale2
test_date_time_locale
test_date_time_locale2
test_month_locale
test_time_locale
test_weekday_locale

======================================================================
ERROR: test_date_locale (test.test_strptime.StrptimeTests.test_date_locale) (locale='fr_FR')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/cpython/cpython/Lib/test/support/__init__.py", line 968, in wrapper
    func(self, *args, **kwargs)
    ~~~~^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 542, in test_date_locale
    self.roundtrip('%x', slice(0, 3), time.localtime(now))
    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 306, in roundtrip
    strp_output = _strptime._strptime_time(strf_output, fmt)
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 670, in _strptime_time
    tt = _strptime(data_string, format)[0]
         ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 431, in _strptime
    _TimeRE_cache = TimeRE()
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 285, in __init__
    self.locale_time = LocaleTime()
                       ~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 86, in __init__
    self.__calc_am_pm()
    ~~~~~~~~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 119, in __calc_am_pm
    am_pm.append(time.strftime("%p", time_tuple).lower().strip())
                 ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
ValueError: Invalid format string
======================================================================
ERROR: test_date_locale2 (test.test_strptime.StrptimeTests.test_date_locale2) (locale='fr_FR')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/cpython/cpython/Lib/test/support/__init__.py", line 968, in wrapper
    func(self, *args, **kwargs)
    ~~~~^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 565, in test_date_locale2
    self.roundtrip('%x', slice(0, 3), (1900, 1, 1, 0, 0, 0, 0, 1, 0))
    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 306, in roundtrip
    strp_output = _strptime._strptime_time(strf_output, fmt)
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 670, in _strptime_time
    tt = _strptime(data_string, format)[0]
         ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 431, in _strptime
    _TimeRE_cache = TimeRE()
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 285, in __init__
    self.locale_time = LocaleTime()
                       ~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 86, in __init__
    self.__calc_am_pm()
    ~~~~~~~~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 119, in __calc_am_pm
    am_pm.append(time.strftime("%p", time_tuple).lower().strip())
                 ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
ValueError: Invalid format string

======================================================================
ERROR: test_date_time_locale (test.test_strptime.StrptimeTests.test_date_time_locale) (locale='fr_FR')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/cpython/cpython/Lib/test/support/__init__.py", line 968, in wrapper
    func(self, *args, **kwargs)
    ~~~~^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 502, in test_date_time_locale
    self.roundtrip('%c', slice(0, 6), time.localtime(now))
    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 306, in roundtrip
    strp_output = _strptime._strptime_time(strf_output, fmt)
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 670, in _strptime_time
    tt = _strptime(data_string, format)[0]
         ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 431, in _strptime
    _TimeRE_cache = TimeRE()
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 285, in __init__
    self.locale_time = LocaleTime()
                       ~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 86, in __init__
    self.__calc_am_pm()
    ~~~~~~~~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 119, in __calc_am_pm
    am_pm.append(time.strftime("%p", time_tuple).lower().strip())
                 ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
ValueError: Invalid format string

======================================================================
ERROR: test_date_time_locale2 (test.test_strptime.StrptimeTests.test_date_time_locale2) (locale='fr_FR')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/cpython/cpython/Lib/test/support/__init__.py", line 968, in wrapper
    func(self, *args, **kwargs)
    ~~~~^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 529, in test_date_time_locale2
    self.roundtrip('%c', slice(0, 6), (1900, 1, 1, 0, 0, 0, 0, 1, 0))
    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 306, in roundtrip
    strp_output = _strptime._strptime_time(strf_output, fmt)
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 670, in _strptime_time
    tt = _strptime(data_string, format)[0]
         ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 431, in _strptime
    _TimeRE_cache = TimeRE()
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 285, in __init__
    self.locale_time = LocaleTime()
                       ~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 86, in __init__
    self.__calc_am_pm()
    ~~~~~~~~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 119, in __calc_am_pm
    am_pm.append(time.strftime("%p", time_tuple).lower().strip())
                 ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
ValueError: Invalid format string

======================================================================
ERROR: test_month_locale (test.test_strptime.StrptimeTests.test_month_locale) (locale='fr_FR')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/cpython/cpython/Lib/test/support/__init__.py", line 968, in wrapper
    func(self, *args, **kwargs)
    ~~~~^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 335, in test_month_locale
    self.roundtrip('%B', 1)
    ~~~~~~~~~~~~~~^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 306, in roundtrip
    strp_output = _strptime._strptime_time(strf_output, fmt)
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 670, in _strptime_time
    tt = _strptime(data_string, format)[0]
         ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 431, in _strptime
    _TimeRE_cache = TimeRE()
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 285, in __init__
    self.locale_time = LocaleTime()
                       ~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 86, in __init__
    self.__calc_am_pm()
    ~~~~~~~~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 119, in __calc_am_pm
    am_pm.append(time.strftime("%p", time_tuple).lower().strip())
                 ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
ValueError: Invalid format string

======================================================================
ERROR: test_time_locale (test.test_strptime.StrptimeTests.test_time_locale) (locale='fr_FR')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/cpython/cpython/Lib/test/support/__init__.py", line 968, in wrapper
    func(self, *args, **kwargs)
    ~~~~^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 590, in test_time_locale
    self.roundtrip('%X', pos, time.localtime(now))
    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 306, in roundtrip
    strp_output = _strptime._strptime_time(strf_output, fmt)
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 670, in _strptime_time
    tt = _strptime(data_string, format)[0]
         ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 431, in _strptime
    _TimeRE_cache = TimeRE()
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 285, in __init__
    self.locale_time = LocaleTime()
                       ~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 86, in __init__
    self.__calc_am_pm()
    ~~~~~~~~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 119, in __calc_am_pm
    am_pm.append(time.strftime("%p", time_tuple).lower().strip())
                 ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
ValueError: Invalid format string

======================================================================
ERROR: test_weekday_locale (test.test_strptime.StrptimeTests.test_weekday_locale) (locale='fr_FR')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/cpython/cpython/Lib/test/support/__init__.py", line 968, in wrapper
    func(self, *args, **kwargs)
    ~~~~^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 378, in test_weekday_locale
    self.roundtrip('%A', 6)
    ~~~~~~~~~~~~~~^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 306, in roundtrip
    strp_output = _strptime._strptime_time(strf_output, fmt)
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 670, in _strptime_time
    tt = _strptime(data_string, format)[0]
         ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 431, in _strptime
    _TimeRE_cache = TimeRE()
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 285, in __init__
    self.locale_time = LocaleTime()
                       ~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 86, in __init__
    self.__calc_am_pm()
    ~~~~~~~~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 119, in __calc_am_pm
    am_pm.append(time.strftime("%p", time_tuple).lower().strip())
                 ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
ValueError: Invalid format string

hoodmane · 2024-12-02T21:35:06Z

Or maybe it's a sign that the patch is wrong...

hoodmane · 2024-12-02T21:36:49Z

Hmm so the failures are all of the French locale?

hoodmane · 2024-12-03T10:14:34Z

If something is wrong with the format string, on Windows it raises a ValueError and on all other platforms it returns an empty string. I think ideally it we should always raise a ValueError but it is a backwards incompatible change...

nineteendo · 2024-12-03T10:15:09Z

Please don't force push (https://devguide.python.org/getting-started/pull-request-lifecycle):

In order to keep the commit history intact, please avoid squashing or amending history and then force-pushing to the PR. Reviewers often want to look at individual commits.

encukou · 2024-12-10T15:25:33Z

Doc/library/datetime.rst

+Handling of format strings containing Unicode code points that can't be
+represented in the charset of the current locale is also platform-dependent. On
+some platforms such code points are preserved intact in the output, while on
+others ``strftime`` may raise :exc:`UnicodeError` or return an empty string
+instead.


Is this still true? According to test_strftime_special, these should be always preserved.

encukou · 2024-12-10T15:26:03Z

Could you add tests for the newly documented behaviour?
There is test_strftime_format_check that checks there is no crash, but it would be good to check the results match the platform -- if only to verify the docs.

(Let me know if you'd like me to continue this.)

hoodmane · 2024-12-11T08:47:33Z

What platforms with musl or BSD libc are supported? MacOS uses BSD and wasi uses musl and they both have buildbots. Then we could also use windows and glibc linux and add tests for the edge cases for each of these?

encukou · 2024-12-11T10:37:29Z

Think of the tests as a tool for maintainers of platforms that CPython doesn't support. If they fail, they'll know that they need a patch -- even if it's just for the docs.
And we can merge patches for unsupported platforms, if they aren't invasive.

hoodmane · 2024-12-11T12:12:07Z

tool for maintainers of platforms that CPython doesn't support

Well we already have divergent behavior in the platforms that we do support. One reasonable approach would be to classify unsupported platforms as glibc-like, musl-like, bsd-like, or windows-like and assert that they agree in behavior with the most similar supported platform...

encukou · 2024-12-11T12:18:54Z

Maybe assert that unknown platforms behave like any known platform?
The list of behaviours in the docs looks exclusive.

hoodmane · 2024-12-11T12:50:19Z

Okay. I will look into that.

pythongh-127527: Improve error handling in time_strftime1

4615f6d

hoodmane requested review from pganssle and abalkin as code owners December 2, 2024 18:52

bedevere-app bot mentioned this pull request Dec 2, 2024

Musl libc strftime for format strings ending in % #127527

Open

bedevere-app bot added the awaiting review label Dec 2, 2024

hoodmane added 3 commits December 3, 2024 09:59

Debug

a7cf1b3

Double buffer size

ca016d7

Restore backwards compatibility

d3b0056

hoodmane force-pushed the strftime-error-handling branch from 81e12cd to d3b0056 Compare December 3, 2024 09:56

hoodmane added 6 commits December 3, 2024 11:36

Update comment

95c7213

Update documentation

d0bd247

Whitespace

a2daaf4

Add news entry

c3955c5

More docs updates

5e5e243

Fix lint

4d67df4

hoodmane requested a review from vstinner December 3, 2024 11:25

hoodmane mentioned this pull request Dec 3, 2024

Handle trailing % in strftime emscripten-core/emscripten#23045

Merged

hoodmane requested a review from encukou December 3, 2024 12:15

encukou reviewed Dec 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

gh-127527: Improve error handling in time_strftime1 #127528

gh-127527: Improve error handling in time_strftime1 #127528

Uh oh!

hoodmane commented Dec 2, 2024 •

edited

Loading

Uh oh!

nineteendo commented Dec 2, 2024 •

edited

Loading

Uh oh!

hoodmane commented Dec 2, 2024

Uh oh!

nineteendo commented Dec 2, 2024

Uh oh!

hoodmane commented Dec 2, 2024

Uh oh!

hoodmane commented Dec 2, 2024

Uh oh!

hoodmane commented Dec 3, 2024

Uh oh!

nineteendo commented Dec 3, 2024

Uh oh!

encukou Dec 10, 2024

Uh oh!

encukou commented Dec 10, 2024

Uh oh!

hoodmane commented Dec 11, 2024

Uh oh!

encukou commented Dec 11, 2024

Uh oh!

hoodmane commented Dec 11, 2024

Uh oh!

encukou commented Dec 11, 2024

Uh oh!

hoodmane commented Dec 11, 2024

Uh oh!

Uh oh!

Uh oh!

gh-127527: Improve error handling in time_strftime1 #127528

Are you sure you want to change the base?

gh-127527: Improve error handling in time_strftime1 #127528

Uh oh!

Conversation

hoodmane commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nineteendo commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoodmane commented Dec 2, 2024

Uh oh!

nineteendo commented Dec 2, 2024

Uh oh!

hoodmane commented Dec 2, 2024

Uh oh!

hoodmane commented Dec 2, 2024

Uh oh!

hoodmane commented Dec 3, 2024

Uh oh!

nineteendo commented Dec 3, 2024

Uh oh!

encukou Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

encukou commented Dec 10, 2024

Uh oh!

hoodmane commented Dec 11, 2024

Uh oh!

encukou commented Dec 11, 2024

Uh oh!

hoodmane commented Dec 11, 2024

Uh oh!

encukou commented Dec 11, 2024

Uh oh!

hoodmane commented Dec 11, 2024

Uh oh!

Uh oh!

hoodmane commented Dec 2, 2024 •

edited

Loading

nineteendo commented Dec 2, 2024 •

edited

Loading