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

Use SystemTime to implement DateTime and provide interop #817

Closed
wants to merge 3 commits into from

Conversation

fghzxm
Copy link
Contributor

@fghzxm fghzxm commented May 22, 2021

std::time::SystemTime is implemented as a repr(transparent) wrapper
around FILETIME on Windows[1], so we can in turn wrap
std::time::SystemTime in Windows::Foundation::DateTime and provide
interop therebetween.

Note that [1] is actually std's implementation detail, so in principle
we have to constantly monitor std's changes to SystemTime.
Hopefully such changes will never happen. Also there are no other reasonable
ways to do it because std::time::SystemTime has a completely opaque
API.

It's difficult for me to provide a proper unit test right now because a
test like tests/winrt/tests/time_span.rs depends on methods on
TestComponent.TestRunner.

fghzxm added 3 commits May 22, 2021 16:27
`std::time::SystemTime` is implemented as a `repr(transparent)` wrapper
around `FILETIME` on Windows[1], so we can in turn wrap
`std::time::SystemTime` in `Windows::Foundation::DateTime` and provide
interop therebetween.

Note that [1] is actually `std`'s implementation detail, so in principle
we have to constantly monitor `std`'s changes to `SystemTime`.
Hopefully that will never happen.  Also there are no other reasonable
ways to do it because `std::time::SystemTime` has a completely opaque
API.

It's difficult for me to provide a proper unit test right now because a
test like `tests/winrt/tests/time_span.rs` depends on methods on
`TestComponent.TestRunner`.
@fghzxm
Copy link
Contributor Author

fghzxm commented May 22, 2021

Extra: can we get completely rid of Windows::Foundation::{DateTime, TimeSpan}? Right now passing the std types to WinRT methods works, but values got from WinRT methods have the Windows types, and therefore Rust methods on their std counterparts can't be directly called:

// does not work
if usage.ConnectionDuration()?.is_zero() {
    // ...
}

It is possible to mitigate the case of method calls by implementing Deref on {DateTime, TimeSpan}, but other situations still require into calls, and most frustratingly type inference does not work in every case:

// does not work
let _: bool = usage.ConnectionDuration()?.into() == Duration::ZERO;

// works, equivalent to Duration::ZERO.eq(&usage.ConnectionDuration()?.into())
let _: bool = Duration::ZERO == usage.ConnectionDuration()?.into();

@samlh
Copy link

samlh commented May 22, 2021

Note that [1] is actually std's implementation detail, so in principle we have to constantly monitor std's changes to SystemTime.
Hopefully such changes will never happen.

I'd recommend against making this sort of assumption - it will very likely cause issues.

See here for an ongoing issue caused by people doing similar things:
rust-lang/rust#83524

If the maintainers are fine with this feature, I expect a different implementation should be used that does not depend on standard library internals.

@fghzxm
Copy link
Contributor Author

fghzxm commented May 23, 2021

@samlh If we want to avoid std internals while providing interop then we probably have to persuade rust-lang/rust to expand SystemTime's API.

Also note that subtracting and adding UNIX_EPOCH is probably not gonna work easily either, since subtracting two SystemTimes gives a std::time::Duration, which by design can only hold non-negative values, but a FILETIME on Windows can be earlier than the Unix epoch.

@kennykerr
Copy link
Collaborator

Like @samlh I'm concerned about taking a dependency on an implementation detail of another library. Probably better to provide convertibility as TimeSpan does. Anyway, better to start by opening an issue to discuss the suggestion.

@kennykerr kennykerr closed this May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants