Support Timestamp and Duration types #434

npars · 2021-04-30T14:05:59Z

Implement Timestamp and Duration types (See Support for more common data types #348)
For Rust, map Timestamp to Rust SystemTime type and Duration to Rust
Duration type
For Kotlin, map Timestamp to Java Instant type and Duration to Java
Duration type
For Python, map Timestamp to naive Python datetime type and Duration
to Python timedelta type
Implement bindings for Swift
~~Implement bindings for Gecko~~
Update documentation

npars · 2021-04-30T14:06:53Z

This PR adds the common Timestamp and Duration types to Uniffi, before I proceed any further there were a few open questions I wanted to discuss:

The current implementation for Python loses precision because Python's internal datetime type uses a floating point number to store the timestamp internally (or at least for doing some of it's math). This could be a gotcha for those expecting to have the exact nanoseconds as kept by Rust.

I there think is a trade-off we need to decide on here:
- A) Always use the internal types for each language, warning users that they may experience some loss of precision
- B) Roll our own types for each language (like gRPC does for Timestamp) and force the user to make their own decisions on how to handle any conversions that may result in loss of precision
The Duration type for Swift (TimeInterval) is just a typealias on Double. Since we already have bindings for Double in our templates this causes a namespace collision if I try to add them for TimeInterval. Any suggestions on the best way forward here would be appreciated. Should I try rewriting the Swift templates so that the function names are explicit? IE: liftDouble, liftTimeInterval, etc.

mhammond · 2021-05-03T23:00:37Z

Thanks for this PR. This looks reasonable to me, and the fact that Python is going to lose nanoseconds doesn't seem that much of a big deal to me. However, for various reasons the people who are best suited to dig in to this aren't going to be fully available for a couple of weeks, so please don't see any delays here as representing a lack of interest!

npars · 2021-05-13T23:28:08Z

@mhammond do you think it would be acceptable to leave the Gecko bindings unimplemented in this PR? It doesn't seem like we have standardized on testing in the examples like we have with Swift, Kotlin, and Python, which make me a bit wary about implementing.

mhammond · 2021-05-13T23:30:34Z

@mhammond do you think it would be acceptable to leave the Gecko bindings unimplemented in this PR?

Yes, I think that would be fine.

rfk

@npars thanks for diving in! This is a really well executed PR. I've left detailed comments below, but in summary, the "request" changes here is mainly for:

Please consider moving the test coverage from ./examples to ./fixtures.
I think at least the Kotlin code needs to be more careful around handling negative durations.

Also, a broader thought that I figured I'd share....some experimenting suggests that Rust is quite happy to represent SystemTime instances that are before the unix epoch. Given that all three of the target languages here require code to explicitly disallow pre-epoch times, I wonder if it's worth doing the opposite and explicitly supporting them, making the Rust code a bit more complicated but the code in each of the binding languages a bit simpler.

For example, we might serialize a timestamp as a signed number of seconds and unsigned nanoseconds, then in the rust code, check for positive or negative duration and calculate the resulting SystemTime via UNIX_EPOCH + duration or UNIX_EPOCH - duration as appropriate.

What do you think?

examples/time/Cargo.toml

examples/time/uniffi.toml

uniffi/src/lib.rs

uniffi_bindgen/src/bindings/gecko_js/webidl.rs

uniffi_bindgen/src/bindings/kotlin/templates/RustBufferHelpers.kt

uniffi_bindgen/src/bindings/swift/gen_swift.rs

uniffi_bindgen/src/bindings/swift/templates/RustBufferHelper.swift

npars · 2021-05-18T18:05:24Z

Also, a broader thought that I figured I'd share....some experimenting suggests that Rust is quite happy to represent SystemTime instances that are before the unix epoch. Given that all three of the target languages here require code to explicitly disallow pre-epoch times, I wonder if it's worth doing the opposite and explicitly supporting them, making the Rust code a bit more complicated but the code in each of the binding languages a bit simpler.

For example, we might serialize a timestamp as a signed number of seconds and unsigned nanoseconds, then in the rust code, check for positive or negative duration and calculate the resulting SystemTime via UNIX_EPOCH + duration or UNIX_EPOCH - duration as appropriate.

What do you think?

That's a good catch, I mistakenly made an assumption based on the Rust Duration type that SystemTime was stored internally as a u64 as well. After digging a bit it looks like SystemTime's implementation is actually specific to each target environment. In the playground you posted I verified that the max value for that env is:

std::time::UNIX_EPOCH + Duration::from_secs(i64::MAX as u64)

As a side note, it looks like there is an existing issue around this behaviour.

I think it makes sense to support pre-epoch times. That change will still leave us with positive only Duration, but that's perhaps a bit easier to swallow.

rfk · 2021-05-19T01:32:58Z

That change will still leave us with positive only Duration, but that's perhaps a bit easier to swallow.

Agreed, I don't think there's much we can do about that for Duration, and also it seems reasonable to reflect the Rust code's semantics in that case.

npars · 2021-05-24T16:27:54Z

@rfk I think I've addressed all of your comments. There's probably some room for optimization in the conversion code for each language, but I think it's ok for a first pass.

- Implement Timestamp and Duration types - For Rust, map Timestamp to Rust SystemTime type and Duration to Rust Duration type - For Kotlin, map Timestamp to Java Instant type and Duration to Java Duration type - For Python, map Timestamp to naive Python datetime type and Duration to Python timedelta type

- Add Timestamp and Duration support to Swift bindings - Add simple test for Timestamp and Duration Swift bindings

- Move time tests to fixtures directory - Remove redundant uniffi.toml from time fixture

rfk · 2021-05-25T04:04:40Z

There's probably some room for optimization in the conversion code for each language

This could be said about many things in the current state of this repo, I don't think it will be a blocker for merge 👍🏻

rfk

Looks great, thank you!

I left a couple of requests for more comments below, and one place where I think the name of a variable is misleading, but otherwise this is in really good shape. I'm going to tick "approve" here because I don't need to look at this again in any detail, please just adjust for those small comments and let me know when it's pushed, and I'll go ahead and merge.

docs/manual/src/internals/lifting_and_lowering.md

uniffi/src/lib.rs

uniffi_bindgen/src/bindings/swift/templates/RustBufferHelper.swift

- Fix outdated timestamp description in book - Add documentation around timestamp calculation - Simplify Swift timestamp calculation for pre-epoch times

rfk · 2021-05-26T00:08:58Z

uniffi_bindgen/src/bindings/swift/templates/RustBufferHelper.swift

        }
    }

    fileprivate func write(into buf: Writer) {
        var delta = self.timeIntervalSince1970
        var sign: Int64 = 1
        if delta < 0 {
+            // The nanoseconds portion of the epoch offset must always be
+            // positive, to simplify the calculation we will use the absolute
+            // value of the offset.


rfk · 2021-05-26T00:09:12Z

uniffi/src/lib.rs

+/// offset should be combined with the nanosecond portion. This is because
+/// the sign of the seconds portion represents the direction of the offset
+/// overall. The sign of the seconds portion can then be used to determine
+/// if the total offset should be added to or subtracted from the unix epoch.


Perfect, thanks so much for adding these!

npars force-pushed the time branch 2 times, most recently from 5521811 to 3b9e5cf Compare May 15, 2021 23:47

npars marked this pull request as ready for review May 15, 2021 23:47

rfk requested changes May 18, 2021

View reviewed changes

npars added 8 commits May 24, 2021 13:59

Add Timestamp and Duration Swift support

1f989c6

- Add Timestamp and Duration support to Swift bindings - Add simple test for Timestamp and Duration Swift bindings

Add Timestamp and Duration to docs

0ddc579

Fix cargo format errors

0add4d0

Move time tests to fixtures directory

a2a1565

- Move time tests to fixtures directory - Remove redundant uniffi.toml from time fixture

Use canonical name for Swift Duration bindings

8349031

Support pre-epoch timestamps

940d931

Bump fixture version

833aab5

npars force-pushed the time branch from 894e0f1 to 833aab5 Compare May 24, 2021 17:09

npars requested a review from rfk May 24, 2021 23:04

rfk approved these changes May 25, 2021

View reviewed changes

Add Timestamp calculation documentation

9f2453e

- Fix outdated timestamp description in book - Add documentation around timestamp calculation - Simplify Swift timestamp calculation for pre-epoch times

rfk approved these changes May 26, 2021

View reviewed changes

rfk merged commit b1c516d into mozilla:main May 26, 2021

This was referenced May 26, 2021

Add codegen backend for Ruby. #464

Merged

Implement Timestamp and Duration types in Ruby backend #469

Closed

data-sync-user mentioned this pull request Jun 23, 2021

Implement Timestamp and Duration types in Ruby backend mozilla/application-services#4235

Closed

Support Timestamp and Duration types #434

Support Timestamp and Duration types #434

Uh oh!

Conversation

npars commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

npars commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhammond commented May 3, 2021

Uh oh!

npars commented May 13, 2021

Uh oh!

mhammond commented May 13, 2021

Uh oh!

rfk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

npars commented May 18, 2021

Uh oh!

rfk commented May 19, 2021

Uh oh!

npars commented May 24, 2021

Uh oh!

rfk commented May 25, 2021

Uh oh!

rfk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rfk May 26, 2021

Choose a reason for hiding this comment

Uh oh!

rfk May 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

npars commented Apr 30, 2021 •

edited

Loading

npars commented Apr 30, 2021 •

edited

Loading