time.strftime(arg) fails when arg is not valid format string

[jopemachine]

Expected Result

In cpython, when arg is not a valid format string, strftime returns arg itself.

>>> import time
>>> time.strftime("%Y")
'2022'
>>> time.strftime("%4Y")
'4Y'

Actual Result

In RustPython, strftime raise panic when arg is not a valid format string

>>>>> import time
>>>>> time.strftime("%Y")
'2022'
>>>>> time.strftime("%4Y")
thread 'main' panicked at 'a Display implementation returned an error unexpectedly: Error', /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/alloc/src/string.rs:2490:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[jopemachine]

I think this could be related to the below issue.

chronotope/chrono#815

[jopemachine]

Now, I'm waiting for the below PR to be merged and v0.5 to be released.

chronotope/chrono#827

[itsankitkp]

Should we add a validation check in fn strptime for supported formats?
This would require a list valid_time_formats = [..] which we can get from https://docs.python.org/3/library/time.html#time.strftime
if validation succeeds then we can continue normal flow otherwise we can return an arg

[DimitrisJim]

Should we add a validation check in fn strptime for supported formats?

There's a simpler solution for now, we can use write! to create the formatted date instead of to_string, that returns a result which we can handle.

Do you want to give it a shot? If so, lmk and I'll assign you.

[itsankitkp]

Did some more digging. CPython uses wcsftime for formatting time (if system supports it)

After building latest python 3.12, I got following

time.strftime("%4Y")
'2023'

Same for Python 3.10 as well

Playing around with wcsftime for same input also yields same result
https://en.cppreference.com/w/c/chrono/wcsftime

Now, as for chrono in rust:
I attempted write!
write!(&mut formatted_time, "{}", instant.format(format.as_str())).unwrap();
It panicked at %4Y (worked at other cases so at least I am not passing it wrong)

I played around chrono too, it doesn't support %4Y at all.

What can be done now is to add exception handling so that rustpython don't panic if there is an exception in chrono.
In case of exception, it can return output as arg. Whenever chrono gets updated, we will get correct results.

Do you want to give it a shot? If so, lmk and I'll assign you.

Sure, I have raised PR #4474
which includes above solution with a test case

Pending: one test case is failing viz embedded null character
I think null characters should have been handled in earlier layers (not related to change in this PR), I will check this.

[fanninpm]

After reading a bit more, it seems that the "%4Y" format is platform-specific.

[itsankitkp]

After reading a bit more, it seems that the "%4Y" format is platform-specific.

+1 and on compiler version

/* Define to 1 if you have the `wcsftime' function. */
#if defined(_MSC_VER) && _MSC_VER >= 1310
#define HAVE_WCSFTIME 1
#endif

[DimitrisJim]

yeah, crono should be the one handling the platform specific stuff. Our main goal should be to avoid the panic for now and try and return the format as CPython does (i.e they probably remove the leading % on return if I remember correctly last I checked it)

[itsankitkp]

yeah, crono should be the one handling the platform specific stuff. Our main goal should be to avoid the panic for now and try and return the format as CPython does (i.e they probably remove the leading % on return if I remember correctly last I checked it)

Yes, current version implements this behavior.

[fanninpm]

Did #4530 fix this?

[DimitrisJim]

Did #4530 fix this?

it did go from panicking to simply failing so partially yes? 😄