Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SystemTime conversions invite programmer error #52522

Open
ssokolow opened this issue Jul 19, 2018 · 2 comments
Open

SystemTime conversions invite programmer error #52522

ssokolow opened this issue Jul 19, 2018 · 2 comments
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@ssokolow
Copy link

Yesterday, I found and reported a panic bug in serde_json which traces its origins to the mechanism for extracting usable information from SystemTime.

Specifically, using the best method they believed to be available, it would panic when I tried to serialize metadata for files with negative mtime values. (I have one file with an mtime of -1 and another with an mtime so far in the future that it overflows into a large negative number if the Rust code is compiled for the i686-unknown-linux-musl target.

The current design of the API encourages users to assume that sources of SystemTime values use unsigned values internally (ie. that modifications to the system clock are the only source of Err with valid input), and, further more, in the bug I reported for serde_json, it led @lambda to the incorrect conclusion that there is currently no way to losslessly serialize and deserialize SystemTime values.

If nothing else, the documentation should be improved in the following ways:

  1. The documentation for both UNIX_EPOCH definitions should explicitly mention that the underlying OS may return valid SystemTime values prior to the epoch and code should be prepared for such an eventuality. (Perhaps with the concrete example of ext3 filesystems allowing negative mtime values to be stored)

  2. The documentation for SystemTime::duration_since should use "may return SystemTimeError" rather than "may fail" to avoid the connotation that an Err result is erroneous in any sense other than being an "until" rather than a "since".

    That is, "failure" connotes a response that should prompt either retry or error-propagation, while it is not necessary to re-run the operation with the arguments reversed in order to extract the Duration and, depending on the input, nothing may be wrong at all. (ie. the Result discriminant is semantically equivalent to a sign bit in this case.)

  3. "and the error contains how far from self the time is" is too easy to overlook, given how important it is for robust software to handle this eventuality. It should explicitly mention SystemTimeError::duration to make it more eye-catching.

    Perhaps "Returns an Err if earlier is later than self. A Duration representing how far self is before earlier can be retrieved via the duration() function on the resulting SystemTimeError.

There are probably other ways to better tune the writing to guard against human error, but I don't want to be here all night trying to perfect this when you might think of the same improvements immediately, simply by virtue of having a different perspective.

@retep998
Copy link
Member

Another concrete example of valid SystemTime values prior to the unix epoch is Windows whose epoch is well before the unix epoch.

@jonas-schievink jonas-schievink added the A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools label Jan 27, 2019
@jonas-schievink jonas-schievink added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Mar 6, 2020
@steveklabnik
Copy link
Member

Triage: not aware of any changes here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants