Skip to content

Ensure better ISO 8601 compliance #330

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Ensure better ISO 8601 compliance #330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 42 additions & 2 deletions core/common/src/Instant.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ package kotlinx.datetime
import kotlinx.datetime.internal.*
import kotlinx.datetime.serializers.InstantIso8601Serializer
import kotlinx.serialization.Serializable
import kotlin.math.*
import kotlin.time.*

/**
Expand Down Expand Up @@ -221,7 +222,17 @@ public fun String.toInstant(): Instant = Instant.parse(this)
* @throws DateTimeArithmeticException if this value or the results of intermediate computations are too large to fit in
* [LocalDateTime].
*/
public expect fun Instant.plus(period: DateTimePeriod, timeZone: TimeZone): Instant
public fun Instant.plus(period: DateTimePeriod, timeZone: TimeZone): Instant {
val ldt = toLocalDateTimeFailing(timeZone)
return with(period) {
val newDate = ldt.date
.run { if (totalMonths != 0) plus(totalMonths, DateTimeUnit.MONTH) else this }
.run { if (days != 0) plus(days, DateTimeUnit.DAY) else this }
LocalDateTime(newDate, ldt.time).toInstant(timeZone).run {
if (totalNanoseconds != 0L) plus(totalNanoseconds, DateTimeUnit.NANOSECOND) else this
}.check(timeZone)
}
}

/**
* Returns an instant that is the result of subtracting components of [DateTimePeriod] from this instant. The components
Expand Down Expand Up @@ -255,7 +266,23 @@ public fun Instant.minus(period: DateTimePeriod, timeZone: TimeZone): Instant =
* @throws DateTimeArithmeticException if `this` or [other] instant is too large to fit in [LocalDateTime].
* Or (only on the JVM) if the number of months between the two dates exceeds an Int.
*/
public expect fun Instant.periodUntil(other: Instant, timeZone: TimeZone): DateTimePeriod
public fun Instant.periodUntil(other: Instant, timeZone: TimeZone): DateTimePeriod {
val thisLdt = toLocalDateTimeFailing(timeZone)
val otherLdt = other.toLocalDateTimeFailing(timeZone)
var thisDate = thisLdt.date
val otherDate = otherLdt.date
val months = thisDate.until(otherDate, DateTimeUnit.MONTH) // `until` on dates never fails
thisDate = thisDate.plus(months, DateTimeUnit.MONTH) // won't throw: thisLdt + months <= otherLdt, which is known to be valid
val days = thisDate.until(otherDate, DateTimeUnit.DAY) // `until` on dates never fails
if (days.absoluteValue > 31) {
throw DateTimeArithmeticException("The number of months between $this and $other does not fit in an Int")
}
thisDate = thisDate.plus(days, DateTimeUnit.DAY) // won't throw: thisLdt + days <= otherLdt
val thisInstant = LocalDateTime(thisDate, thisLdt.time).toInstant(timeZone)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this toInstant use the same resolve mode as ZDT.plus(days) that was used before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly.

Imagine a situation:

  • UTC offset before is +1:00;
  • An overlap happens;
  • UTC offset after is +0:00.

If we subtract, say, a day and land into this overlap, then, as is currently implemented, the +0:00 offset will be used. In this PR, the UTC offset +1:00 will be used instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it would be better to preserve resolve behavior in these operations, so that
Instant + DatePeriod(days = 1) == Instant.plus(1, DAY)

val nanoseconds = thisInstant.until(other, DateTimeUnit.NANOSECOND) // |otherLdt - thisLdt| < 24h

return buildDateTimePeriod(months, days, nanoseconds)
}

/**
* Returns the whole number of the specified date or time [units][unit] between `this` and [other] instants
Expand Down Expand Up @@ -521,3 +548,16 @@ internal const val DISTANT_FUTURE_SECONDS = 3093527980800
* Be careful: this function may throw for some values of the [Instant].
*/
internal expect fun Instant.toStringWithOffset(offset: UtcOffset): String

/** Check that [Instant] fits in [LocalDateTime].
* This is done on the results of computations for consistency with other platforms.
*/
internal fun Instant.check(zone: TimeZone): Instant = [email protected] {
toLocalDateTimeFailing(zone)
}

internal fun Instant.toLocalDateTimeFailing(zone: TimeZone): LocalDateTime = try {
toLocalDateTime(zone)
} catch (e: IllegalArgumentException) {
throw DateTimeArithmeticException("Can not convert instant $this to LocalDateTime to perform computations", e)
}
10 changes: 10 additions & 0 deletions core/common/test/InstantTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,16 @@ class InstantTest {
assertEquals(end, end2)
}

@Test
fun dateTimePeriodWithGapBetweenMonthsAndDays() {
val zone = TimeZone.of("America/New_York")
// LocalDateTime(2019, 3, 10, 2, 0) is a gap.
// If months and days are not added atomically, the result will be adjusted.
val start = Instant.parse("2019-02-10T02:00:00-05:00")
val end = start.plus(DateTimePeriod(months = 1, days = 1), zone)
assertEquals(Instant.parse("2019-03-11T02:00:00-04:00"), end)
}

@Test
fun diffInvariant() {
repeat(STRESS_TEST_ITERATIONS) {
Expand Down
32 changes: 1 addition & 31 deletions core/commonJs/src/Instant.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,23 +118,6 @@ public actual class Instant internal constructor(internal val value: jtInstant)
}
}


public actual fun Instant.plus(period: DateTimePeriod, timeZone: TimeZone): Instant = try {
val thisZdt = jsTry { this.value.atZone(timeZone.zoneId) }
with(period) {
thisZdt
.run { if (totalMonths != 0) jsTry { plusMonths(totalMonths) } else this }
.run { if (days != 0) jsTry { plusDays(days) } else this }
.run { if (hours != 0) jsTry { plusHours(hours) } else this }
.run { if (minutes != 0) jsTry { plusMinutes(minutes) } else this }
.run { if (seconds != 0) jsTry { plusSeconds(seconds) } else this }
.run { if (nanoseconds != 0) jsTry { plusNanos(nanoseconds.toDouble()) } else this }
}.toInstant().let(::Instant)
} catch (e: Throwable) {
if (e.isJodaDateTimeException()) throw DateTimeArithmeticException(e)
throw e
}

private fun Instant.atZone(zone: TimeZone): jtZonedDateTime = jsTry { value.atZone(zone.zoneId) }
private fun jtInstant.checkZone(zone: TimeZone): jtInstant = apply { jsTry { atZone(zone.zoneId) } }

Expand Down Expand Up @@ -193,19 +176,6 @@ public actual fun Instant.plus(value: Long, unit: DateTimeUnit.TimeBased): Insta
if (value > 0) Instant.MAX else Instant.MIN
}

public actual fun Instant.periodUntil(other: Instant, timeZone: TimeZone): DateTimePeriod = try {
var thisZdt = jsTry { this.value.atZone(timeZone.zoneId) }
val otherZdt = jsTry { other.value.atZone(timeZone.zoneId) }

val months = thisZdt.until(otherZdt, jtChronoUnit.MONTHS); thisZdt = jsTry { thisZdt.plusMonths(months) }
val days = thisZdt.until(otherZdt, jtChronoUnit.DAYS); thisZdt = jsTry { thisZdt.plusDays(days) }
val nanoseconds = thisZdt.until(otherZdt, jtChronoUnit.NANOS)

buildDateTimePeriod(months.toInt(), days.toInt(), nanoseconds.toLong())
} catch (e: Throwable) {
if (e.isJodaDateTimeException()) throw DateTimeArithmeticException(e) else throw e
}

public actual fun Instant.until(other: Instant, unit: DateTimeUnit, timeZone: TimeZone): Long = try {
val thisZdt = this.atZone(timeZone)
val otherZdt = other.atZone(timeZone)
Expand All @@ -221,4 +191,4 @@ public actual fun Instant.until(other: Instant, unit: DateTimeUnit, timeZone: Ti
}

internal actual fun Instant.toStringWithOffset(offset: UtcOffset): String =
jtOffsetDateTime.ofInstant(this.value, offset.zoneOffset).toString()
jtOffsetDateTime.ofInstant(this.value, offset.zoneOffset).toString()
29 changes: 1 addition & 28 deletions core/jvm/src/Instant.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import kotlinx.datetime.internal.*
import kotlinx.datetime.serializers.InstantIso8601Serializer
import kotlinx.serialization.Serializable
import java.time.DateTimeException
import java.time.ZonedDateTime
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accidental import?

import java.time.format.DateTimeParseException
import java.time.temporal.ChronoUnit
import kotlin.time.*
Expand Down Expand Up @@ -107,20 +108,6 @@ private fun Instant.atZone(zone: TimeZone): java.time.ZonedDateTime = try {
throw DateTimeArithmeticException(e)
}

public actual fun Instant.plus(period: DateTimePeriod, timeZone: TimeZone): Instant {
try {
val thisZdt = atZone(timeZone)
return with(period) {
thisZdt
.run { if (totalMonths != 0) plusMonths(totalMonths.toLong()) else this }
.run { if (days != 0) plusDays(days.toLong()) else this }
.run { if (totalNanoseconds != 0L) plusNanos(totalNanoseconds) else this }
}.toInstant().let(::Instant)
} catch (e: DateTimeException) {
throw DateTimeArithmeticException(e)
}
}

@Deprecated("Use the plus overload with an explicit number of units", ReplaceWith("this.plus(1, unit, timeZone)"))
public actual fun Instant.plus(unit: DateTimeUnit, timeZone: TimeZone): Instant =
plus(1L, unit, timeZone)
Expand Down Expand Up @@ -157,20 +144,6 @@ public actual fun Instant.plus(value: Long, unit: DateTimeUnit.TimeBased): Insta
if (value > 0) Instant.MAX else Instant.MIN
}

public actual fun Instant.periodUntil(other: Instant, timeZone: TimeZone): DateTimePeriod {
var thisZdt = this.atZone(timeZone)
val otherZdt = other.atZone(timeZone)

val months = thisZdt.until(otherZdt, ChronoUnit.MONTHS); thisZdt = thisZdt.plusMonths(months)
val days = thisZdt.until(otherZdt, ChronoUnit.DAYS); thisZdt = thisZdt.plusDays(days)
val nanoseconds = thisZdt.until(otherZdt, ChronoUnit.NANOS)

if (months > Int.MAX_VALUE || months < Int.MIN_VALUE) {
throw DateTimeArithmeticException("The number of months between $this and $other does not fit in an Int")
}
return buildDateTimePeriod(months.toInt(), days.toInt(), nanoseconds)
}

public actual fun Instant.until(other: Instant, unit: DateTimeUnit, timeZone: TimeZone): Long = try {
val thisZdt = this.atZone(timeZone)
val otherZdt = other.atZone(timeZone)
Expand Down
34 changes: 0 additions & 34 deletions core/native/src/Instant.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,27 +252,6 @@ private fun Instant.toZonedDateTime(zone: TimeZone): ZonedDateTime {
return ZonedDateTime(toLocalDateTimeImpl(currentOffset), zone, currentOffset)
}

/** Check that [Instant] fits in [ZonedDateTime].
* This is done on the results of computations for consistency with other platforms.
*/
private fun Instant.check(zone: TimeZone): Instant = [email protected] {
toZonedDateTimeFailing(zone)
}

public actual fun Instant.plus(period: DateTimePeriod, timeZone: TimeZone): Instant = try {
with(period) {
val withDate = toZonedDateTimeFailing(timeZone)
.run { if (totalMonths != 0) plus(totalMonths, DateTimeUnit.MONTH) else this }
.run { if (days != 0) plus(days, DateTimeUnit.DAY) else this }
withDate.toInstant()
.run { if (totalNanoseconds != 0L) plus(0, totalNanoseconds).check(timeZone) else this }
}.check(timeZone)
} catch (e: ArithmeticException) {
throw DateTimeArithmeticException("Arithmetic overflow when adding CalendarPeriod to an Instant", e)
} catch (e: IllegalArgumentException) {
throw DateTimeArithmeticException("Boundaries of Instant exceeded when adding CalendarPeriod", e)
}

@Deprecated("Use the plus overload with an explicit number of units", ReplaceWith("this.plus(1, unit, timeZone)"))
public actual fun Instant.plus(unit: DateTimeUnit, timeZone: TimeZone): Instant =
plus(1L, unit, timeZone)
Expand Down Expand Up @@ -307,19 +286,6 @@ public actual fun Instant.plus(value: Long, unit: DateTimeUnit.TimeBased): Insta
if (value > 0) Instant.MAX else Instant.MIN
}

public actual fun Instant.periodUntil(other: Instant, timeZone: TimeZone): DateTimePeriod {
var thisLdt = toZonedDateTimeFailing(timeZone)
val otherLdt = other.toZonedDateTimeFailing(timeZone)

val months = thisLdt.until(otherLdt, DateTimeUnit.MONTH).toInt() // `until` on dates never fails
thisLdt = thisLdt.plus(months, DateTimeUnit.MONTH) // won't throw: thisLdt + months <= otherLdt, which is known to be valid
val days = thisLdt.until(otherLdt, DateTimeUnit.DAY).toInt() // `until` on dates never fails
thisLdt = thisLdt.plus(days, DateTimeUnit.DAY) // won't throw: thisLdt + days <= otherLdt
val nanoseconds = thisLdt.until(otherLdt, DateTimeUnit.NANOSECOND) // |otherLdt - thisLdt| < 24h

return buildDateTimePeriod(months, days, nanoseconds)
}

public actual fun Instant.until(other: Instant, unit: DateTimeUnit, timeZone: TimeZone): Long =
when (unit) {
is DateTimeUnit.DateBased ->
Expand Down