Kotlin / KEEP

Kotlin Evolution and Enhancement Process
Apache License 2.0
3.35k stars 359 forks source link

Duration and time measurement API #190

Open ilya-g opened 5 years ago

ilya-g commented 5 years ago

This issue is to discuss the proposal to introduce Duration, TimeSource, TimeMark and time measurement API.

JakeWharton commented 5 years ago

Can you detail why the stdlib is the target for this API rather than a standalone kotlinx library?

I don't think there's a clear delineation between when something should target the stdlib vs. kotlinx. Coroutines were split between the two, but leaned aggressively away from the stdlib in everything but the fundamental building blocks. Looking ahead, the multiplatform file/fs abstraction seems to be targeting kotlinx entirely. Nothing about the proposal makes it obvious why the stdlib is where this should live. I'm not opposed to it being there, to be clear, but I think it's worth capturing the arguments as to why something should live in the stdlib vs. kotlinx, applying them to this proposal, and seeing whether it still makes sense.

fluidsonic commented 5 years ago

As far as I understand this KEEP is mostly about duration and timing related to measurement, timeouts etc. and not for real-world date/time application.

I suggest to be careful with the naming (e.g. Duration, Clock) because it could conflict with a potential date/time library in the future if such a library will intend to implement these in a different way or with different functionality but will have to pick another name. This can become a source of confusion.

A Duration for measuring something short-term maybe work differently and have different functionality as a Duration used for representing real-world date/time differences. Same for Clock which may use completely different approaches and offer different functionality in each case.

For a date/time library it could for example be decided that using Double for durations is not sufficient, hence a re-use of Duration may or may not be possible.

Wasabi375 commented 5 years ago

Well some possible names to solve this kind of conflict would be Stopwatch and maybe MeasuredDuration, MeasuredTimeInterval. While I kind of like the name Stopwatch I think MeasuredDuration might be to verbose. But then how often is it necessary to both deal with monotonic time and real world time. It might be simply enough the different "timing" implementations in different packages (as they would be anyways). Worst case you can always add typealiases for the implementation you use less.

markov commented 5 years ago

FWIW I made a little lib a while ago that implements a large part of this proposal for the Duration class: https://github.com/markov/lucid-time

The major difference is that I chose the internal value of the inline class to be a Long not a Double - I feel like most applications do not need to represent time differences larger than 292 years. Also this is consistent to how java's own TimeUnit enum would handle unit transformations and I wanted to have 100% same results if someone chose to replace all usages of TimeUnit with my lib or like I use it simply provide adapted extension functions to existing APIs from other libs.

Also I like Duration as a name. Keep it simple.

ilya-g commented 5 years ago

Can you detail why the stdlib is the target for this API rather than a standalone kotlinx library?

@JakeWharton We believe that the API for working with date and times is a cornerstone thing and ideally it should be provided in the standard library. Unfortunately, the full blown date-time support is too heavy to be included in the stdlib, so we're trying to find a balance here. Duration and time measurement seem to have a moderate API surface and cover a lot of time use cases, so placing them in the stdlib looks reasonable.

@fluidsonic When considering the naming of kotlin.time entities we keep in mind their potential integration with a kotlinx date/time library. Duration can be a suitable type for the result of taking difference between two instants of real-world time from that library, but to express a date interval duration we think it'd better to introduce a type with another name. The interface Clock can also be extended to represent real-world time sources, see the section "Future advancements" in the KEEP.

If you have concrete examples of how the date/time concepts can conflict with the proposed interfaces/classes, let's discuss 'em here.

voddan commented 5 years ago

I think the proposal needs bit more details on how Duration works around infinity. Some questions that should be clarified:

Maybe the API should include its own Duration.INFINITY which would represent the infinitely big duration.

ilya-g commented 5 years ago

Is Double.POSITIVE_INFINITY.minutes equal to Double.POSITIVE_INFINITY.seconds

Given that they both represent the same Duration.INFINITE value, they are equal.

Is Float.POSITIVE_INFINITY.minutes equal to Double.POSITIVE_INFINITY.minutes

We do not have functions/properties to construct a duration from Float, but if we had, they would be equal.

What will Double.POSITIVE_INFINITY.minutes.toLongMilliseconds() return?

The value will be clamped to the Long range, same as Double.toLong() does, so the result will be Long.MAX_VALUE

Will Double.POSITIVE_INFINITY.minutes - Double.POSITIVE_INFINITY.minutes be a legal operation?

We prohibit having NaN as a duration value, so this operation will result in exception. Currently this result is not checked due to not being able to place an init block in an inline class, where this check could be made.

Most of these questions are answered in the API docs of the corresponding members, see Duration.kt.

elizarov commented 5 years ago

It would seem extremely useful to add String.toDuration() extension that can reverse both the result of Duration.toString() and the result of Duration.toIsoString(). It would be a lossy conversion in general case, but I think it is Ok. One can also consider making "default" toString() into a non-lossy one, but that would make it less useful for logs. I'd rather keep it as is.

quicknir commented 5 years ago

I would really, really, strongly suggest reviewing the C++ date library (to be standardized in C++20 IIRC), as well as the stuff that is already standardized under std::chrono. Maybe C++ is not the first language that Kotlin looks at for examples, but these libraries in C++ are simply designed incredibly well. Some specific things that stuck out to me:

The property value stores a Double number denoting the number of nanoseconds in this time interval.

Such internal representation is a good compromise between having high precision for very small durations and good enough precision for very big durations. For example, nanoseconds can be stored precisely for durations up to 104 days, and seconds for up to 146 years.

It's not really a very good compromise (IMHO). The problem is that once you go from just durations, to also time points, these are all measured from the epoch, which is some 50 years ago. So if you want to get the duration between two different time points today, you are basically getting some catastrophic cancellation. You can end up with durations of 10 nanoseconds, that are actually mostly noise. I would actually suggest making Duration generic on the underlying stored value type. Rather than using an enum to express the units, all you really need is some way to express the ratio of stored ticks, to seconds (this could either be a generic parameter, or it could be a stored value, depending on priorities). Check out the approach in std::chrono: https://en.cppreference.com/w/cpp/chrono/duration. Type aliases can be used to provide convenient typedefs so that in the "simple case" users don't need to deal with all this.

Clock is an interface with a single method:

Wouldn't you expect that a Clock would actually be able to tell you the time? If you instead have a Clock that returns time points, you can call now on the clock twice and subtract those figures to get a duration. Something like mark/elapsed can be implemented in terms of this, but not vice versa. And obviously, at some point you're going to wait something that can tell you the actual current unix time, not just time elapsed.

But again, this interface works much better with integers, rather than floating point, because of catastrophic cancellation issues. This is just one example IMHO of a more general thing, which is that durations, time points, dates, timezones, etc, need to be designed together. If you try to just design durations on their own, when you introduce time points you may see that your duration design does not play well with it.

Another approach that was considered is introducing the class TimeStamp and the arithmetic operations on it that return Duration. However, it was concluded to be error-prone because it would be too easy to mix two timestamps taken from unrelated time sources in a single expression and get nonsense in the result.

This is can be solved (again, in chrono/date: https://en.cppreference.com/w/cpp/chrono/time_point). Basically, a TimeStamp can be regarded as being generic on a Duration type (which it stores), and an Epoch type. The Epoch type is really little more than a "tag". But basically functions are defined so that TimeStamp's of different epochs cannot be subtracted. If you want to do that, you need to explicitly convert the TimePoint to a duration (by calling durationSinceEpoch() or whatever), and then construct a different kind of TimePoint from it. This approach also becomes invaluable once you set out to deal with timezones (again, the holistic design issues). Because at that point, you need to have TimePoints, but some of these will represent UTC or unix time (i.e. a "real" time), some of them will be attached to timezones, and some of them will be local times that have not yet been attached to timezones. This Epoch tagging approach lets you work with all of these in a convenient and type safe manner, and allays current concerns about TimePoint vs Duration in this API. More information about this approach here: https://howardhinnant.github.io/date/tz.html.

quicknir commented 5 years ago

A quick example of catastrophic cancellation; I wrote the following trivial C++ program to show what happens if you had two durations 10 nanoseconds apart, today:

int main(int argc, char const *argv[]) {
  auto x = static_cast<double>(1562709653) * 1000000000;
  auto y = static_cast<double>(1562709653) * 1000000000 + 10;
  std::cerr << y - x;
}

This program just printed 0 (for me): http://coliru.stacked-crooked.com/a/2d5c2492ca14464f. So basically, a duration based on double is not a duration that can serve as the basis for good TimePoint's, but this already means that the design of everything is compromised (IMHO).

ilya-g commented 5 years ago

The fact that duration value is stored as a double doesn't imply that time points will be stored as doubles too. I believe this cancels catastrophic cancellation issues.

Wouldn't you expect that a Clock would actually be able to tell you the time?

Clock interface isn't intended to tell the world time, but there can be an interface that extends Clock and has an additional method to obtain the current instant of time.

quicknir commented 5 years ago

The fact that duration value is stored as a double doesn't imply that time points will be stored as doubles too. I believe this cancels catastrophic cancellation issues.

You obviously can do it this way, but it's just making things harder on yourself (and less performant). One way or another, a time point is going to be a duration from a fixed moment (the epoch). If you think you've designed a good Duration type, it makes sense to use your own Duration type for that, does it not? It seems problematic that a time point essentially stores a duration and you can't use your own duration type for that. Your time point will have to use something else to store the duration, and then it will raise the question of why you can't simply use whatever the time point stores as your duration type. Etc. A TimePoint being a Duration + epoch is a very clean and logical design, and you should be able to use the same Duration inside the TimePoint, as generally throughout the library.

Btw, note that neither Java nor C# use floating point for their duration, and C++ keeps it generic. So, of all the reviewed libraries, Klock is the only one that decided to back Duration with floating point (exclusively). The first 3 are standard libraries for some of the most used languages in the world, subject to unbelievable amounts of criticism, design discussion, etc. Klock obviously is not playing in that ballpark. So I think this is something you should consider.

Clock interface isn't intended to tell the world time, but there can be an interface that extends Clock and has an additional method to obtain the current instant of time.

You can do it this way, but what's the advantage? You now have two classes, and two interfaces. VS, you could have a single interface, and an extension method that will work with all Clocks. Note that Clock's returning a time_point is the interface that is actually more representative of how system clocks work; at the system level usually a clock is something that you ask for a time, and it gives you a time. You don't communicate with the system clock by asking it to "mark" the start, etc.

Again, I would really urge you to review chrono/date. I know there's "always another" library, but it's a modern take on this stuff in a major language (with a very critical standardization process), it deserves to be on the short list of prior art.

mikehearn commented 5 years ago

For people who mostly just use Kotlin as a better Java, is it maybe possible to put this duplicative stuff into kotlinx or other modules? I get that when targeted JavaScript or LLVM there are almost no usable libraries, but, the java.time package has very comprehensive support and types for all these things and ending up with split APIs, where some code uses some lightweight almost-but-not-quite-the-same Kotlin stdlib copy, and others use the Java APIs, is going to be kind of annoying in projects that aren't targeted the browser or LLVM. If it's in separate modules a project can just say "we don't use that library" and stay consistent. If it's in the stdlib developers will end up confused about which one to use.

ilya-g commented 5 years ago

If it's in separate modules a project can just say "we don't use that library" and stay consistent.

I believe this can be achieved without such radical measure as evicting Duration type from the standard library: its usage requires an explicit import and to avoid importing it automatically on completion the entire kotlin.time package or some of its members can be blacklisted in IDEA settings (Editor -> General -> Auto import -> Exclude from import and completion).

If it's in the stdlib developers will end up confused about which one to use.

Since Kotlin 1.3 we have Random class in kotlin.random package, which is named the same as Java's Random. We have not yet seen that it caused a big confusion among developers.

ilya-g commented 5 years ago

@quicknir, Could you tell from your experience working with std::chrono what real world use cases does a time point being representable as a duration since epoch enable?

mikehearn commented 5 years ago

I doubt such confusions would be reported, as such reports would be clearly non-actionable. And Kotlin 1.3 is quite new. My project still hasn't upgraded to it yet :(

quicknir commented 5 years ago

Ilya, I'm not sure exactly what you are asking. How can you define a point in time other than an epoch and duration? It's like defining a point in space; you have to pick coordinate axes and state your distances in those coordinates.

Are you asking what the advantage is in using the same specific duration type inside of the time point class, instead of something else?

I'll post later with more details but some of the things accomplished by Chrono sadly can't be done in kotlin because of type system limitations. That said i still think using a floating point type for duration is a huge misstep.

cpovirk commented 5 years ago

Hello from the GoodTime team at Google! We work on libraries and static analysis to fight date- and time-related bugs in Java, and we see Kotlin as an opportunity to provide our users the same safety right out of the box. (Thanks to @JakeWharton for looping us in on this proposal.)

Most bugs we find are unit mismatches, and the main way we prevent mismatches is to make APIs accept a Duration instead of double seconds, long millis, long timeout, TimeUnit unit, etc. So we're excited to see a Duration in the Kotlin API.

One obstacle that we've hit in the most performance-sensitive code (like core concurrency and RPC tracing) is that those systems don't want their elapsed-time calculations to allocate. To be clear, this is a rare concern: Most users of those APIs are happy to use overloads or wrapper APIs that do allocate. But the implementations themselves avoid allocating. From what I understand, this often works as well as a "proper" Duration+Clock approach: System.nanoTime() is fairly convenient (aside from the chance of overflow!), and low-level APIs like Future.get(...) wouldn't respect a fake Clock injected for testing, anyway.

But of course it would be nice to have a Duration that's fast enough for that case, too. My impression is that the Kotlin Duration comes very close, thanks to being an inline class that wraps a Double. (Double has its downsides, of course, but let's focus on the advantages.) This is no surprise, given the proposal's goals :)

My concern is with Clock: If we want to avoid allocation, then ClockMark would have to be an inline class, too. I understand that you want a "full" object to detect mixing of timestamps from unrelated clocks, but to accomplish that, I think you are giving up the main benefit of an inline Duration class.

I can see going in any of a few directions:

Or maybe there's a clever way to get everything we could want?

If we're stuck choosing between these options, maybe it would help to define the scope of this proposal even more precisely. For example, should it be usable for nanobenchmarks that check whether one arithmetic operator outperforms another? for high-performance RPC systems? etc.

webleaf commented 5 years ago

The proposed type Duration makes it explicit that a duration value is being expected and solves the problem of specifying its unit.

  1. There are more general problem: Units and Measurement. Apple and Android APIs has their own implementations (Apple API looks more clear for me). Сreating own units of measurement, converters from one unit to another etc. And when I write MPP code in Kotlin, I need implement my own API for such purpose. I think, such Units and Measurement API should be implemented first. And only after that and based on that, should be implemented this Duration and time measurement API.

  2. Proposed API does not mention time zones (there are should be ability of time zone library to reuse proposed API entities).

erikc5000 commented 5 years ago

Just saw this KEEP recently and figured I'd weigh in as I've been working on a pure Kotlin time library that draws heavily from java.time.

  1. While a floating point representation for a duration is fine for some applications, I tend to agree with @quicknir that it's a misstep to make that the default. Time and duration are closely related and accuracy is often quite important when manipulating times with durations. Using a floating point representation (at least without opt-in as with C++) doesn't seem to be a common choice in other high quality libraries from what I've seen -- I'm assuming for that reason.

    Lack of allocations in JavaScript is cited as part the rationale for using a floating point, but even Moment.js uses an object to represent durations -- granted, they've combined date and time based math into one with it.

  2. You lose the ability to express granularity when automatically converting Int.days, Int.hours, and such to a general "Duration". There are situations where you might want to preserve or enforce a certain granularity and optimizations can sometimes be enabled or complexity reduced by doing so.

    As it stands, the standard library would provide a conflicting definition of Int.-unit- that can't enforce granularity at compile time. That could make for some ugly declarations and general confusion if you start interacting with parts of the standard library that adopt Duration within the same file.

  3. In addition to the above, mapping Int.days to a duration isn't necessarily correct or expected behavior. Often you're more interested in a "conceptual day", which may not be exactly 24 hours depending on time changes or leap seconds -- the difference between day-based and time-based measures, or "period" and "duration" In java.time.

    So echoing the last bullet point, a more full-featured time library will probably not want to automatically convert Int.days to a Duration , so the standard library would provide a conflicting definition.

  4. It seems like time zones should be a property of a clock. While not spelled out, I'm guessing the idea is that you could have a WallClock subclass that adds it? Or would that be a future addition?

Anyway, while it's pretty early in development and certainly not ready for anyone to consume, this is the source code for my library: https://github.com/erikc5000/island-time. I figure it may at least provide some ideas on possible ways of dealing with durations, periods, and unit granularity in Kotlin to help inform discussion here -- and perhaps, get a picture of how a date-time library might build on the standard library primitives covered by the proposal.

I'm also open to collaborating on or using it as a test bed for a future kotlinx library that offers more complete date-time functionality. Writing and maintaining a time library is a pretty big time commitment (see what I did there :-) ), so while a fun hobby project right now, ultimately, I'd very much like to offload some of that.

RafaelKr commented 5 years ago

I just came across this KEEP after reading the Kotlin 1.3.50 released Blog post.

When I saw 100.milliseconds and 1.seconds I thought: "Wow, that reads pretty nicely". At second thought I noticed, that these must be getters on the Number types, which maybe isn't ideal.

In the comments section "Marcel" suggested to use e.g. 100ms, which would need a new primitive type in Kotlin. IMO this solution would be very explicit and even better to read. So if the Kotlin team would go for that, I would be really happy.

But if the Kotlin team doesn't plan to implement it this way, it's worth taking a look at the implementation in Golang. They can use 100 * time.Millisecond to get a value of type Duration, which also reads pretty nicely without polluting the Number types. The downside of their implementation is that the limit of a Duration is approximately 290 years. But maybe there's a way to solve this when implementing it in Kotlin.

package time

[...]

// A Duration represents the elapsed time between two instants
// as an int64 nanosecond count. The representation limits the
// largest representable duration to approximately 290 years.
type Duration int64

const (
    minDuration Duration = -1 << 63
    maxDuration Duration = 1<<63 - 1
)

// Common durations. There is no definition for units of Day or larger
// to avoid confusion across daylight savings time zone transitions.
//
// To count the number of units in a Duration, divide:
//  second := time.Second
//  fmt.Print(int64(second/time.Millisecond)) // prints 1000
//
// To convert an integer number of units to a Duration, multiply:
//  seconds := 10
//  fmt.Print(time.Duration(seconds)*time.Second) // prints 10s
//
const (
    Nanosecond  Duration = 1
    Microsecond          = 1000 * Nanosecond
    Millisecond          = 1000 * Microsecond
    Second               = 1000 * Millisecond
    Minute               = 60 * Second
    Hour                 = 60 * Minute
)

Source: https://github.com/golang/go/blob/master/src/time/time.go

fvasco commented 4 years ago

I played a bit with the current implementation.

I focused on some use case where duration are constants in the code:

withTimeout(30.seconds)

the actual value for Duration's constructor is evaluated at runtime, inline functions may help. Unfortunately 30.toDouble() is inlined but not evaluated at compile time.

So this is a use case for a further Kotlin enhancement.

reitzig commented 4 years ago

At second thought I noticed, that these must be getters on the Number types, which maybe isn't ideal.

Why?

Any running time concerns would, by my estimate, be taken care of by JIT at the latest. Inline classes are there to provide zero-cost abstractions, after all.

CommanderTvis commented 4 years ago

I think it shouldn't be in Standard Library, because 1) it doesn't require any special compiler features or inspections, 2) this feauture is implemented on the platforms (java.time for JVM, Date for JS, chrono.h for C++).

ghost commented 4 years ago

Do we need symmetrical multiplication operator overloads number * Duration = Duration?

hammer * time;
harmonize * duration;
adjust * timeout;
tieskedh commented 4 years ago

I like the Time being in the standard-library, as it gives a standard and hopefully more cross-platform libraries.

I don't like it to have extension-functions on the Number-class as it polutes the Number-type. By creating it the old-fashioned way Seconds.of(4) only the ones who really use the topic will have access to it.

By combining this with a kotlin-library that is a non-standard library, the types can still be extension-functions for the developers who want that. By making this additional library an official Kotlin-library, the API will be the standard way to do this. (Developers that use the Time-library will also have a choice to make number a God-object, this way)

Below are a couple of libraries: implementation library stars
JavaFX.time tornadoFX 2.8k
Java.time kxdate 186
custom kizitonwose/Time 913

I chose these as these overlap the extension-vals in this proposal a lot. This means that even if the developer would want a God-object for time, having the methods duplicated for two different implementations would probably be annoying for them as well.

ilya-g commented 4 years ago

Following the feedback from Google's GoodTime team, we've decided to rename Clock interface to TimeSource, see PR #205. The original plan was that WallClock class from date/time library could implement Clock interface, but now we think that it would be a source of confusion to have Clock.markNow() method along with the other methods of WallClock in the latter type. Another point is that the use case of measuring durations using non-monotonic WallClock seems to be not so in demand to introduce "is-a" relationship between WallClock and Clock. For that rare case we could provide an adapter that wraps WallClock as a TimeSource instead. So, we're renaming Clock to TimeSource, ClockMark to TimeMark, and changing all other derived type names respectively. The old names will remain for some time as deprecated type aliases to the new types.

PaulWoitaschek commented 3 years ago

The loss of precision currently has lead to a bug in our business logic.

A now failing test case boils down to this:

val nanos = (200.days  + 5.nanoseconds - 200.days).toLongNanoseconds()
check(nanos ==5L){
  "$nanos should be 5"
}
elizarov commented 3 years ago

@PaulWoitaschek Please, share the details on the domain you are working with and the problem you are trying to solve in your domain where having such computations to be precise is important.

ilya-g commented 3 years ago

One of concerns often voiced in the feedback regarding the Duration type is that the extension properties on numeric types used to construct a Duration could pollute the completion of expressions where these types are receivers. To address that we're now considering deprecating these extensions and providing static-like factory functions in Duration companion object, e.g. Duration.seconds(value), Duration.minutes(value) and so on. You can take a look at this PR to see how this would affect the code creating durations.

Let us know what you think about this change.

altavir commented 3 years ago

@ilya-g. Indeed, there is a minor problem with that. Additionally, it is not always obvious how to construct that duration. When I don't know how to construct something, I type its name (like Duration) and see autocomplete options. I think that longer Duration.seconds is a small price for discoverability.

hfhbd commented 3 years ago

I did love the extension versions! It was short, human readable and easy to understand, even for non developers without a deep knowledge of Duration. I will miss it.

val timeToFix = 2.days
val timeNow = Duration.days(2)
PaulWoitaschek commented 3 years ago

Yeah we really miss them too. Usage of time in apps is so common that we really do not care about the namespace pollution.

janvladimirmostert commented 3 years ago

Wouldn't 2.toDuration(...) type of syntax give us the best of both worlds?

It doesn't pollute the namespace, is similar to toInt, toLong, .... and it's easily discoverable via the IDE 2.toDuration(days = true) or 2.toDuration(Duration.DAYS) or something similar alongside Duration.days which is not a bad syntax either

On Apr 10 2021, at 11:04 am, Philip Wedemann @.***> wrote:

I did love the extension versions! It was short, human readable and easy to understand, even for non developers without a deep knowledge of Duration. I will miss it. val timeToFix = 2.days val timeNow = Duration.days(2) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub @./0?redirect=https%3A%2F%2Fgithub.com%2FKotlin%2FKEEP%2Fissues%2F190%23issuecomment-817104477&recipient=cmVwbHkrQUFSTEM1U1c0WU1aSjRUUDJCNlZRUjU2UFZGUUxFVkJOSEhCV0FGTzdRQHJlcGx5LmdpdGh1Yi5jb20%3D), or unsubscribe @./1?redirect=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAARLC5UJDCSCBIABSUMWFU3TIAIALANCNFSM4HVB6OYA&recipient=cmVwbHkrQUFSTEM1U1c0WU1aSjRUUDJCNlZRUjU2UFZGUUxFVkJOSEhCV0FGTzdRQHJlcGx5LmdpdGh1Yi5jb20%3D).

KarelPeeters commented 3 years ago

Or maybe all of the extension functions can be put into separate package or even just the Duration companion object, then they can easily be wildcard imported where they are actually needed?

ilya-g commented 3 years ago

While storing a duration value as a Double number provides accuracy enough for practical purposes for both very short and very long durations, it may still feel unfortunate that adding or subtracting one nanosecond to/from a duration d slightly more than a hundred days resulted in rounding so that the result was equal to the same d.

To counter that, we've decided to change the internal representation to a Long value that stores either a number of nanoseconds or a number of milliseconds. This allowed us to represent durations up to ~146 years with a nanosecond precision and durations up to ~146 million years with a millisecond precision.

But by doing so, we had to sacrifice the sub-nanosecond precision of very short durations, and some implementation efficiency, especially in Kotlin/JS, which doesn't have an efficient Long implementation and has to emulate it with an object with two Int properties.

Given that the internal value is now represented with a Long number, we feel that the properties that return the duration value expressed in particular units should also have Long return type, so we introduced new Long-returning properties inWholeMinutes, inWholeSeconds and, so on, instead of the existing Double-returning properties inMinutes, inSeconds, etc. It is still possible to get the duration value expressed as Double with the toDouble(DurationUnit) function.

See the changes in the proposal text in this PR: #249

hrach commented 3 years ago

To address that we're now considering deprecating these extensions and providing static-like factory functions in Duration companion object, e.g. Duration.seconds(value), Duration.minutes(value) and so on. You can take a look at this PR to see how this would affect the code creating durations.

Let us know what you think about this change.

I 👍 on this, but in last weeks I've changed my mind. I like the current API, the new one seems too much verbose and reads really bad ("Duration (missing "in") seconds (missing "from") two" instead of very nice "two seconds" ). The namespace pollution wasn't ever a problem for me.

JakeWharton commented 3 years ago

especially in Kotlin/JS, which doesn't have an efficient Long implementation and has to emulate it with an object with two Int properties.

Is the type designed to have its backing implementation changed to the Duration type in the new temporal API once it's generally available?

ilya-g commented 3 years ago

@JakeWharton No, we don't have such plans. That Duration type has vastly different semantics, it's more like DateTimePeriod from kotlinx-datetime.

eygraber commented 3 years ago

@ilya-g adding my voice to the negative feedback chorus regarding the extension properties on numeric types for Duration. I've found using them to be particularly fun, and they really help readability (delay(1.seconds) vs delay(Duration.seconds(1))).

Are there any plans to reconsider? I know this could be easily provided by a 3rd party or even Kotlin itself as a separate artifact, but I am curious about what the response is to the feedback from the community.

janvladimirmostert commented 3 years ago

I'd be happy to import the extension functions via an extra dependency, this is something I use a lot

On Apr 21 2021, at 3:25 am, Eliezer Graber @.***> wrote:

@ilya-g @./0?redirect=https%3A%2F%2Fgithub.com%2Filya-g&recipient=cmVwbHkrQUFSTEM1VE81VEwyUU43UkpBNUlBQU42Uk5RQlBFVkJOSEhCV0FGTzdRQHJlcGx5LmdpdGh1Yi5jb20%3D) adding my voice to the negative feedback chorus regarding the extension properties on numeric types for Duration. I've found using them to be particularly fun, and they really help readability (delay(1.seconds) vs delay(Duration.seconds(1))). Are there any plans to reconsider? I know this could be easily provided by a 3rd party or even Kotlin itself as a separate artifact, but I am curious about what the response is to the feedback from the community. — You are receiving this because you commented. Reply to this email directly, view it on GitHub @./1?redirect=https%3A%2F%2Fgithub.com%2FKotlin%2FKEEP%2Fissues%2F190%23issuecomment-823707762&recipient=cmVwbHkrQUFSTEM1VE81VEwyUU43UkpBNUlBQU42Uk5RQlBFVkJOSEhCV0FGTzdRQHJlcGx5LmdpdGh1Yi5jb20%3D), or unsubscribe @.***/2?redirect=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAARLC5T56EXYR4SJG7PPFL3TJYSRRANCNFSM4HVB6OYA&recipient=cmVwbHkrQUFSTEM1VE81VEwyUU43UkpBNUlBQU42Uk5RQlBFVkJOSEhCV0FGTzdRQHJlcGx5LmdpdGh1Yi5jb20%3D).

battery-staple commented 3 years ago

Just wanted to note my agreement with @eygraber—1.seconds and the like was a huge boon to readability. I'm not 100% sure if this was a specific feature, either, but it seemed like IntelliJ would refrain from autocompleting the extension methods until it was essentially unambiguous (after typing 1.secon rather than 1.se). Thus, for me, namespace pollution was also a non-issue.

I would love to see it return, even as an independent dependency. :-)

ilya-g commented 3 years ago

We can consider returning these extensions in a more limited scope form, for example in a companion object or in another nested object in Duration, as @KarelPeeters has proposed:

Or maybe all of the extension functions can be put into separate package or even just the Duration companion object, then they can easily be wildcard imported where they are actually needed?

After that we can introduce a helper function for bringing this object into context, e.g.

fun duration(Duration.ConstructionExtensions.() -> Duration): Duration

val interval = duration { 1.minutes + 5.seconds }
eygraber commented 3 years ago

For anyone that is interested, I made a library to provide the factory extensions (as well as simple extension properties to retrieve the Duration value as a Double).

https://github.com/eygraber/kotlin-duration-extensions

gmk57 commented 3 years ago

There is an issue on YouTrack about keeping the extension properties on numeric types.

JavierSegoviaCordoba commented 3 years ago

applicable not only for number literals, but also values stored in variables/fields, which looks arguably weird (here)

The compiler/linter can just show a warning/error in this case, no?

rnett commented 3 years ago

applicable not only for number literals, but also values stored in variables/fields, which looks arguably weird (here)

The compiler/linter can just show a warning/error in this case, no?

This is a broader problem for wherever these literal suffix style extensions are used (CSS, Compose, etc), it might be a good idea to have an annotation that would limit them to being used only on literals (properties and whatnot could have a toSeconds() or w/e method, which imo looks nicer).

ilya-g commented 3 years ago

We can consider returning these extensions in a more limited scope form, for example in a companion object or in another nested object in Duration

We have explored this option a bit more, and it turned out that even if we put the extension properties into the Duration companion object or into a nested object, the IDE is still smart enough that it finds them anyway and proposes them in completion, inserting an import of something like kotlin.time.Duration.Companion.seconds automatically. So this way is not much different from having these extensions on top level, except additional imports and some extra overhead added by companion object access.

However, we could still pursue this approach if we bet on context sensitive resolution, KT-16768. If it was implemented, the compiler would resolve an expression like val d: Duration = 10.seconds in a context where Duration type is expected by looking into the static scope of Duration, i.e. in its companion object. There it would find the suitable extension method and use it even without an import (or maybe with an import of kotlin.time.Duration class itself). After that we could teach IDE not to import such companion members automatically in order to reduce completion pollution in other places, where Duration type is not expected. Note that this plan is risky because it tries to guess how the convention of not yet implemented feature would look like.

joffrey-bion commented 2 years ago

Thanks @ilya-g for the update!

IMO the problem addressed here is the weaker of the 2. Autocomplete imprecision is a write-time issue, while reading the boilerplate Duration.seconds(10) is a read-time issue, which statistically has way more impact. Also, it seemed to me that autocomplete in the IDE was already prioritizing methods and extensions that had a return type of the proper expected type depending on the call site, so we don't really need context sensitive resolution to mitigate the problem.

The other problem (limiting such extensions to number literals) is a valid concern. But while we're waiting for such a feature (if it is ever implemented at all), it's always nice to give the option to the developer to choose between Duration.seconds(...) and Int.seconds (and thus remove the deprecation). The choice is not a problem in itself, but bad usage is. It may even be possible to provide an IDE inspection that warns if using Int.seconds on an expression that is not a number literal: "please prefer using Duration.seconds(...) on complex expressions for clarity".

RafaelKr commented 2 years ago

We have explored this option a bit more, and it turned out that even if we put the extension properties into the Duration companion object or into a nested object, the IDE is still smart enough that it founds them anyway and proposes them in completion, inserting an import of something like kotlin.time.Duration.Companion.seconds automatically.

And this is also something which could probably be fixed in the IDE. The IDE should only show the completions when the import is there.