gh-103648: Make datetime.timestamp() raise OverflowError when overflow on Windows

[gaogaotiantian]

Currently on Windows, if localtime_s() returns an error in _PyTime_localtime, we just set an OS error. However, the docs says it should raise an OverflowError. According to Microsoft docs, localtime_s only fails if either pointer is NULL, or it overflows. So we can confidently set the error to overflow if none of the pointer is NULL.

• Issue: Invalid arument error for datetime.timestamp() method in windows #103648

[gaogaotiantian]

Hi @pganssle and @abalkin , do you think you could give this a review?

[gaogaotiantian]

Hi @vstinner , could you give this PR a review as you have been working on this file recently? Thanks!

[vstinner]

How is it possible that tm becomes NULL after calling localtime_s()? Is it a macro which sets tm on error? _PyTime_localtime() must not be called with tm=NULL.

[gaogaotiantian]

This is the case listed by localtime_s. I did not find any existing usage that could possibly call _PyTime_localtime with tm = NULL, but I did not find any documentation for it either. Of course it does not quite make sense to call it with tm = NULL, but if someone ran into the issue in the future(for example, did a _PyTime_localtime(t, tm_ptr) and forgot to check the pointer), they would get an OverflowError instead of an OSError, which would be confusing.

So two things we can do:

1. Leave it as it is as a sanity check - the code path is kind of dead as of now, but could benefit in the future.
2. Document that tm can't be NULL in the comments and/or do an assertion in the function.

I'm fine with either solution.

[vstinner]

Please add an assertion to ensure that the function is never called with tm=NULL. A runtime check is not needed. Right, private functions are not documented.

[gaogaotiantian]

Done, thanks for the review!

[gaogaotiantian]

@vstinner circling back to this, do you think this is ready to merge? Thanks!

[vstinner]

I skip my turn. I don't know if it's better to raise an OSError or an OverflowError. I'm not an expert in time/datetime modules.

Maybe @pganssle or @abalkin have an opinion about it.

[brandtbucher]

Also not a domain expert, but given that this has been raising an OSError for so long, I really think we should avoid breaking code that might be catching the OSError.

Since the docs themselves say that OSError can be raised, I'm not sure that there's really a bug to fix here.

[bedevere-app]

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.

[gaogaotiantian]

Since the docs themselves say that OSError can be raised, I'm not sure that there's really a bug to fix here.

Yeah it was added in the docs last year #94818. Personally I believe this is an obvious overflow error, and OSError does not describe it correctly. I understand the backward compatibility concern and it is a rare case. The message OSError: [Errno 22] Invalid argument is confusing to me though.

I'm okay if this is not merged.

[vstinner]

I close the issue.

I always had doubts about this PR, that why I didn't merge it. @brandtbucher put the right words on what I'm thinking about it.

The message OSError: [Errno 22] Invalid argument is confusing to me though.

Python is a thin wrapper to operating system functions. And yeah, sometimes the OS reports surprising errors. Usually, the solution is more to provide better documentation and guide users on how to solve their issue.

Maybe there are portable Python modules which don't rely on the Python time module and so don't inherit Windows limitation, but have a portable behavior (support all datetime range on all platforms).