Repository: time-rs/time (language: rust)

Issue:
Tighten leap second handling when parsing from RFC 3339
The current implementation of `format_description::well_known::Rfc3339` allows second 60 to occur in any time, which is then rolled back to the last nanosecond of 59. As leap seconds are only really allowed at 23:59:60 UTC, this behaviour feels way too loose.

Fortunately, there is always the full parsed date for the `Rfc3339` format parser to look at. So it could compute the UTC time and check if the hour is 23 and the minute is 59, otherwise it should reject the parsed time. It might make sense to also check if it's the last day of the month, though I don't know if the language in ITU-R TF.460-6 is considered normative by the IERS.


Discussion / hints:
In ~a later breaking change~ an added API, I'd prefer the parsing rules to be restricted so that an RFC 3339 string with the second part of `60` is rejected with an error variant that would allow the application to perform its own recovery, perhaps with a method to retrieve the library-recommended date approximation.
This shouldn't be too difficult. The main thing to keep note of is that the time is parsed in the local time zone, while leap seconds are always xxxx-12-31 23:59:60Z.

With regard to rejecting the leap second, that will not happen, even in a breaking change. If it's possible to do something reasonable (in this case using the a value one nanosecond prior) then we will do so. Leap second support may be implemented in the future, mind you.
> With regard to rejecting the leap second, that will not happen, even in a breaking change. If it's possible to do something reasonable (in this case using the a value one nanosecond prior) then we will do so.

The rollback may be up to a full second, as 23:59:60.999999999 is a valid leap second time. Depending on the application, the meaning of "reasonable" here may feel... stretched.

> Leap second support may be implemented in the future, mind you.

I see. The desired behaviour depends on the application:
- most will probably accept the current behaviour and not worry about this obscure issue;
- some would expect a time realization with a [leap smear](https://developers.google.com/time/smear) and want to reject non-compliant timestamps;
- others will want support for leap seconds in the data model in some way (not necessarily the one chrono thrusts onto its not always suspecting users).
Sure, it could be up to a full second. Either way, I'm not comfortable rejecting an otherwise valid moment just because it's a leap second. Every other restriction comes from either the specification or functional limits in the code — namely restricting the UTC offset to ±23:59:59 at most, which is not an issue in practice as no location has ever observed such an extreme offset to my knowledge. I've no problem limiting it to be on June 30 or December 31 at 23:59:60, as the spec requires values to be semantically valid. Anything further restriction is unlikely.

If leap seconds are "properly" supported in the future, algorithms that work around a leap second wouldn't be. That would include leap smear, which inherently distorts the objectively correct time. Barring international agreement on changing how leap seconds work (or eliminating them altogether), my ultimate goal is to acknowledge reality within the proleptic Gregorian calendar.
> reality within the proleptic Gregorian calendar.

With computers on the internet, that's usually subject to local clock drift and some approximation of globally synchronized time usually achieved over NTP.

Regardless of international agreements, popular [protocols](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#google.protobuf.Timestamp) already exist that require their time representation to use leap smear and avoid over-59 seconds. Time-rs implementing workarounds or a future IERS-compliant support for UTC, with no alternatives, is actually in disagreement with the protobuf specification.

I think Google, AWS, and others are trying to establish a de-facto workaround that would make time accounting friendlier to computing. Furthermore, their NTP servers already provide smeared time, so anyone relying on those is in on the workaround whether or not their time stack expects otherwise.

I think the above calls for a separate `Rfc3339NoLeap` format parser at least, to give the application choice on how to handle the matter.
Yes, there is clock drift and synchronization concerns, but that is an unavoidable reality that has nothing to do with this crate. I am taking the clock time on the user's machine as ground truth. NTP is not handled here, nor will it be in the future.

Leap seconds _do_ exist, at least for the next few years until IERS meets with regard to its future (I wouldn't be surprised if they just delay again). Leap smear is not universally accepted, and even where it is used the form it takes varies. Once tzdb handling is implemented, I'm absolutely not going to make it so that adding one second doesn't actually add one second of clock time — imagine the bugs that could cause. Proper handling of leap seconds is possible and will eventually be implemented.

I am currently using a workaround that uses the closest value within the same calendar day (and hour and minute). The fact that certain protocols use leap smearing doesn't change the goal of this crate: to provide as accurate information as possible. Google's protocol using Google's leap smear isn't surprising. If dedicated protobuf handling is implemented in the future (unlikely), tzdb information can be used to leap smear then. Until that point, I'm providing the best information I can within the confines of reality, RFCs, and constraints of the time crate. I can't be in disagreement with a specification (provided by a company, not a standards body nonetheless) that I never claim to support.
If you _truly_ want to handle it yourself, I can add a method to the `Parsed` struct that accepts well known formats. That is low-level and not recommended for the overwhelming majority of users, however.
> I can't be in disagreement with a specification (provided by a company, not a standards body nonetheless) that I never claim to support.

Fortunately, there is no need to do any breaking changes to support the protobuf well-known types specification, other than adding a variant of the RFC 3339 format parser that would reject leap seconds. Even that is not even essential to the binary data format in protobuf, but less strict parsing of string inputs may result in unwanted ambiguity in mapping. Formatting and the internal data do not currently produce leap second times.
> If you _truly_ want to handle it yourself, I can add a method to the `Parsed` struct that accepts well known formats.

How about adding an `Option` member with the original subsecond part in `Some`, set in case when the rollback to 59th second has been applied to the existing second and subsecond values? This should be enough for advanced users and is backward-compatible.
I'll try to submit a PR with proposed changes some time later.
> How about adding an Option member with the original subsecond part in Some, set in case when the rollback to 59th second has been applied to the existing second and subsecond values?

This would implicitly guarantee the current behavior, which is not desirable. I can investigate delaying the leap second normalization, but this would require some internal information that I'm not sure I want to store.

> I'll try to submit a PR with proposed changes some time later.

Be aware that you are likely wasting your time. Adding another format for the sole purpose of rejecting a valid format will not be accepted. 23:59:60Z is a valid time and will always be accepted where the specification requires it. Duplicating all well-known formats would be a significant overhead and result in code and binary bloat, in addition to increased maintenance burden.

My goal is not to support every method of handling time, it's to support the _correct_ method. Leap smear is not how time currently works despite what Google may want others to think.
> This would implicitly guarantee the current behavior, which is not desirable.

A change of behavior to expose leap seconds without a rollback would break the current API, as the same input string will map to different time component values. So why not add an escape hatch for 0.3, which can be deprecated or removed when leap seconds are properly supported in the revised API?
How would it break the current API? The result of the parsing is currently inaccurate and will be so until tzdb handling is implemented. Making something _more_ accurate isn't a breaking change. That's the exact reasoning I used to change the parser from using `59` to `59.999_999_999`. Yes, the return value changed, but it's more accurate so it's fine.
> The result of the parsing is currently inaccurate

Yes, and the API user is left completely unaware about this, with no recourse. This can make the library unsuitable for some applications, with real world examples I tried to point out above. No matter how we may dislike Google, Amazon, et al., the alternative where every computer system in the world has to independently follow the pronouncements of a committee with 6 months advance notice is no longer universally accepted.

> That's the exact reasoning I used to change the parser from using `59` to `59.999_999_999`

The behavior pre-change was faulty considering the intent, so that arguably was a bug fix.
The more strict parser would also have the benefit of making the map between `OffsetDateTime` values and RFC 3339 strings bijective, up to the equivalences in both domains.
I proposed a way that would let you handle it but received no direct answer. Please address this if you would like the ability to handle the leap second case manually. I am not willing to take on the maintenance burden of duplicating all well-known formats, especially when the specification requires succeeding on a valid value. The only time I will knowingly _fail_ on an otherwise valid value is when the UTC offset is beyond ±24 hours, as no such time zone has ever existed. But that is not relevant here. Leap seconds are valid, so I parse them.

If Google, Amazon, etc. want to use leap smear, _that's on them_ because they are the ones not following reality. They're lying about what time it actually is, whereas my end goal is to not do so. If I have to choose between following some big corporation who thinks they can change things unilaterally and an internationally accepted standard, I'm choosing the latter ten times out of ten. It has nothing to do with "dislike"; it's just that I prefer reality over things that are made up in the name of "simplicity". You can continue yelling into the void about how some large corporations think we should do it this way, but the simple reality is that's not how clocks currently operate.

To be perfectly clear, _I am not and will not implement leap smear until it becomes an internationally recognized standard_. Nor will I accept a PR that does so.

> The behavior pre-change was faulty considering the intent, so that arguably was a bug fix.

My point exactly. Improving something is considered a bug fix because the previous behavior was not fully correct.

---

I feel like we're going in circles here, so take everything I say in this comment at face value. If you continue to push for something I've already rejected, I'll just ignore that point from here forwards.
> I can add a method to the `Parsed` struct that accepts well known formats.

Without more detail, I can't see how that would help. It seems more complicated to me than addressing it in a specific well-known format parser.
> To be perfectly clear, _I am not and will not implement leap smear until it becomes an internationally recognized standard_.

That's not what I'm proposing to do. I want to give the programmer a choice to reject leap second times if their application expressly forbids dealing with them. How they receive their time inputs should be completely outside the scope of the `time` library.
> Without more detail, I can't see how that would help. It seems more complicated to me than addressing it in a specific well-known format parser.

It would let you handle it manually. The vast majority of users will not need to do this, which is why going through the `Parsed` struct is reasonable. Is it more verbose? Yes, but that's the price you pay for explicitly not following the spec.

> That's not what I'm proposing to do. I want to give the user a choice to reject leap second times if their application expressly forbids dealing with them. How they receive their time inputs should be completely outside the scope of the time library.

Adding a method to `Parsed` would let you reject any value for any reason.

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index b2b213827..99afb28b4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,13 @@ The format is based on [Keep a Changelog]. This project adheres to [Semantic Ver
 
 ---
 
+## Unreleased
+
+### Changed
+
+- Well-known formats that support leap seconds now return the `TryFromParsed::ComponentRange` error
+  variant if the leap second could not occur at that given moment.
+
 ## 0.3.7 [2021-01-26]
 
 ### Fixed
diff --git a/src/error/component_range.rs b/src/error/component_range.rs
index 756c7fad9..3d092a992 100644
--- a/src/error/component_range.rs
+++ b/src/error/component_range.rs
@@ -28,6 +28,12 @@ impl ComponentRange {
     pub const fn name(self) -> &'static str {
         self.name
     }
+
+    /// Whether the value's permitted range is conditional, i.e. whether an input with this
+    /// value could have succeeded if the values of other components were different.
+    pub const fn is_conditional(self) -> bool {
+        self.conditional_range
+    }
 }
 
 impl fmt::Display for ComponentRange {
diff --git a/src/offset_date_time.rs b/src/offset_date_time.rs
index ec2d6c678..6d9008b55 100644
--- a/src/offset_date_time.rs
+++ b/src/offset_date_time.rs
@@ -16,6 +16,8 @@ use std::time::SystemTime;
 use crate::formatting::Formattable;
 #[cfg(feature = "parsing")]
 use crate::parsing::Parsable;
+#[cfg(feature = "parsing")]
+use crate::util;
 use crate::{error, Date, Duration, Month, PrimitiveDateTime, Time, UtcOffset, Weekday};
 
 /// The Julian day of the Unix epoch.
@@ -949,6 +951,18 @@ impl OffsetDateTime {
     ) -> Result<Self, error::Parse> {
         description.parse_offset_date_time(input.as_bytes())
     }
+
+    /// A helper method to check if the `OffsetDateTime` is a valid representation of a leap second.
+    /// Leap seconds, when parsed, are represented as the preceding nanosecond. However, leap
+    /// seconds can only occur as the last second of a month UTC.
+    pub(crate) const fn is_valid_leap_second_stand_in(self) -> bool {
+        let udt = self.utc_datetime;
+        udt.hour() == 23
+            && udt.minute() == 59
+            && udt.second() == 59
+            && udt.nanosecond() == 999_999_999
+            && udt.day() == util::days_in_year_month(udt.year(), udt.month())
+    }
 }
 
 impl fmt::Display for OffsetDateTime {
diff --git a/src/parsing/parsable.rs b/src/parsing/parsable.rs
index 230d9a94f..7d9420e2f 100644
--- a/src/parsing/parsable.rs
+++ b/src/parsing/parsable.rs
@@ -217,12 +217,8 @@ impl sealed::Sealed for Rfc2822 {
             cfws(input).ok_or(InvalidLiteral)?.into_inner()
         };
 
-        // The RFC explicitly allows leap seconds. We don't currently support them, so treat it as
-        // the previous moment.
-        if parsed.second == Some(60) {
-            parsed.second = Some(59);
-            parsed.subsecond = Some(999_999_999);
-        }
+        // The RFC explicitly allows leap seconds.
+        parsed.leap_second_allowed = true;
 
         let zone_literal = first_match(
             [
@@ -331,12 +327,8 @@ impl sealed::Sealed for Rfc3339 {
             input
         };
 
-        // The RFC explicitly allows leap seconds. We don't currently support them, so treat it as
-        // the previous moment.
-        if parsed.second == Some(60) {
-            parsed.second = Some(59);
-            parsed.subsecond = Some(999_999_999);
-        }
+        // The RFC explicitly allows leap seconds.
+        parsed.leap_second_allowed = true;
 
         if let Some(ParsedItem(input, ())) = ascii_char_ignore_case::<b'Z'>(input) {
             parsed.offset_hour = Some(0);
@@ -447,18 +439,36 @@ impl sealed::Sealed for Rfc3339 {
             return Err(error::Parse::UnexpectedTrailingCharacters);
         }
 
-        // The RFC explicitly allows leap seconds. We don't currently support them, so treat it as
-        // the previous moment.
-        if second == 60 {
+        // The RFC explicitly permits leap seconds. We don't currently support them, so treat it as
+        // the preceding nanosecond. However, leap seconds can only occur as the last second of the
+        // month UTC.
+        let leap_second_input = if second == 60 {
             second = 59;
             nanosecond = 999_999_999;
-        }
+            true
+        } else {
+            false
+        };
 
-        Ok(Month::from_number(month)
+        let dt = Month::from_number(month)
             .and_then(|month| Date::from_calendar_date(year as _, month, day))
             .and_then(|date| date.with_hms_nano(hour, minute, second, nanosecond))
             .map(|date| date.assume_offset(offset))
-            .map_err(TryFromParsed::ComponentRange)?)
+            .map_err(TryFromParsed::ComponentRange)?;
+
+        if leap_second_input && !dt.is_valid_leap_second_stand_in() {
+            return Err(error::Parse::TryFromParsed(TryFromParsed::ComponentRange(
+                error::ComponentRange {
+                    name: "second",
+                    minimum: 0,
+                    maximum: 59,
+                    value: 60,
+                    conditional_range: true,
+                },
+            )));
+        }
+
+        Ok(dt)
     }
 }
 // endregion well-known formats
diff --git a/src/parsing/parsed.rs b/src/parsing/parsed.rs
index be7d31045..59a45aafd 100644
--- a/src/parsing/parsed.rs
+++ b/src/parsing/parsed.rs
@@ -63,6 +63,9 @@ pub struct Parsed {
     pub(crate) offset_minute: Option<u8>,
     /// Seconds within the minute of the UTC offset.
     pub(crate) offset_second: Option<u8>,
+    /// Indicates whether a leap second is permitted to be parsed. This is required by some
+    /// well-known formats.
+    pub(crate) leap_second_allowed: bool,
 }
 
 impl Parsed {
@@ -89,6 +92,7 @@ impl Parsed {
             offset_hour: None,
             offset_minute: None,
             offset_second: None,
+            leap_second_allowed: false,
         }
     }
 
@@ -473,7 +477,29 @@ impl TryFrom<Parsed> for PrimitiveDateTime {
 impl TryFrom<Parsed> for OffsetDateTime {
     type Error = error::TryFromParsed;
 
-    fn try_from(parsed: Parsed) -> Result<Self, Self::Error> {
-        Ok(PrimitiveDateTime::try_from(parsed)?.assume_offset(parsed.try_into()?))
+    fn try_from(mut parsed: Parsed) -> Result<Self, Self::Error> {
+        // Some well-known formats explicitly allow leap seconds. We don't currently support them,
+        // so treat it as the nearest preceding moment that can be represented. Because leap seconds
+        // always fall at the end of a month UTC, reject any that are at other times.
+        let leap_second_input = if parsed.leap_second_allowed && parsed.second == Some(60) {
+            parsed.second = Some(59);
+            parsed.subsecond = Some(999_999_999);
+            true
+        } else {
+            false
+        };
+        let dt = PrimitiveDateTime::try_from(parsed)?.assume_offset(parsed.try_into()?);
+        if leap_second_input && !dt.is_valid_leap_second_stand_in() {
+            return Err(error::TryFromParsed::ComponentRange(
+                error::ComponentRange {
+                    name: "second",
+                    minimum: 0,
+                    maximum: 59,
+                    value: 60,
+                    conditional_range: true,
+                },
+            ));
+        }
+        Ok(dt)
     }
 }


Test patch:
diff --git a/tests/integration/parsing.rs b/tests/integration/parsing.rs
index 08fb1859c..bd0241f47 100644
--- a/tests/integration/parsing.rs
+++ b/tests/integration/parsing.rs
@@ -213,6 +213,21 @@ fn rfc_2822_err() {
         OffsetDateTime::parse("Mon, 02 Jan 2021 03:04:05 -060", &Rfc2822),
         invalid_component!("offset minute")
     ));
+    assert!(matches!(
+        OffsetDateTime::parse("Fri, 31 Dec 2021 23:59:61 Z", &Rfc2822),
+        Err(error::Parse::TryFromParsed(error::TryFromParsed::ComponentRange(component)))
+            if component.name() == "second" && !component.is_conditional()
+    ));
+    assert!(matches!(
+        OffsetDateTime::parse("Fri, 31 Dec 2021 03:04:60 Z", &Rfc2822),
+        Err(error::Parse::TryFromParsed(error::TryFromParsed::ComponentRange(component)))
+            if component.name() == "second" && component.is_conditional()
+    ));
+    assert!(matches!(
+        OffsetDateTime::parse("Fri, 30 Dec 2021 23:59:60 Z", &Rfc2822),
+        Err(error::Parse::TryFromParsed(error::TryFromParsed::ComponentRange(component)))
+            if component.name() == "second" && component.is_conditional()
+    ));
 }
 
 #[test]
@@ -222,8 +237,12 @@ fn rfc_3339() -> time::Result<()> {
         datetime!(2021-01-02 03:04:05 UTC),
     );
     assert_eq!(
-        OffsetDateTime::parse("2021-01-02T03:04:60Z", &Rfc3339)?,
-        datetime!(2021-01-02 03:04:59.999_999_999 UTC),
+        OffsetDateTime::parse("2021-12-31T23:59:60Z", &Rfc3339)?,
+        datetime!(2021-12-31 23:59:59.999_999_999 UTC),
+    );
+    assert_eq!(
+        OffsetDateTime::parse("2015-07-01T00:59:60+01:00", &Rfc3339)?,
+        datetime!(2015-06-30 23:59:59.999_999_999 UTC),
     );
     assert_eq!(
         OffsetDateTime::parse("2021-01-02T03:04:05.1Z", &Rfc3339)?,
@@ -282,10 +301,6 @@ fn rfc_3339() -> time::Result<()> {
         Date::parse("2021-01-02T03:04:05.123-01:02", &Rfc3339)?,
         date!(2021 - 01 - 02),
     );
-    assert_eq!(
-        Time::parse("2021-01-02T03:04:60Z", &Rfc3339)?,
-        time!(03:04:59.999_999_999)
-    );
 
     Ok(())
 }
@@ -364,6 +379,21 @@ fn rfc_3339_err() {
         PrimitiveDateTime::parse("2021-13-01T00:00:00Z", &Rfc3339),
         Err(error::Parse::TryFromParsed(error::TryFromParsed::ComponentRange(component))) if component.name() == "month"
     ));
+    assert!(matches!(
+        PrimitiveDateTime::parse("2021-01-02T03:04:60Z", &Rfc3339),
+        Err(error::Parse::TryFromParsed(error::TryFromParsed::ComponentRange(component))) if component.name() == "second"
+    ));
+
+    // Conversions to offset-unaware types do not perform special treatment for leap seconds
+    // even if the input could refer to one.
+    assert!(matches!(
+        PrimitiveDateTime::parse("2022-01-01T00:59:60+01:00", &Rfc3339),
+        Err(error::Parse::TryFromParsed(error::TryFromParsed::ComponentRange(component))) if component.name() == "second"
+    ));
+    assert!(matches!(
+        Time::parse("2021-12-31T23:04:60Z", &Rfc3339),
+        Err(error::Parse::TryFromParsed(error::TryFromParsed::ComponentRange(component))) if component.name() == "second"
+    ));
 
     assert!(matches!(
         OffsetDateTime::parse("2021-01-02T03:04:05Z ", &Rfc3339),
@@ -449,6 +479,22 @@ fn rfc_3339_err() {
         OffsetDateTime::parse("2021-13-01T00:00:00Z", &Rfc3339),
         Err(error::Parse::TryFromParsed(error::TryFromParsed::ComponentRange(component))) if component.name() == "month"
     ));
+    assert!(matches!(
+        OffsetDateTime::parse("2021-12-31T23:59:61Z", &Rfc3339),
+        Err(error::Parse::TryFromParsed(error::TryFromParsed::ComponentRange(component))) if component.name() == "second" && !component.is_conditional()
+    ));
+    assert!(matches!(
+        OffsetDateTime::parse("2021-01-02T23:59:60Z", &Rfc3339),
+        Err(error::Parse::TryFromParsed(error::TryFromParsed::ComponentRange(component))) if component.name() == "second" && component.is_conditional()
+    ));
+    assert!(matches!(
+        OffsetDateTime::parse("2021-12-31T03:04:60Z", &Rfc3339),
+        Err(error::Parse::TryFromParsed(error::TryFromParsed::ComponentRange(component))) if component.name() == "second" && component.is_conditional()
+    ));
+    assert!(matches!(
+        OffsetDateTime::parse("2021-12-31T23:59:60+01:00", &Rfc3339),
+        Err(error::Parse::TryFromParsed(error::TryFromParsed::ComponentRange(component))) if component.name() == "second" && component.is_conditional()
+    ));
 }
 
 #[test]
diff --git a/tests/integration/serde.rs b/tests/integration/serde.rs
index f01f37c58..870859f56 100644
--- a/tests/integration/serde.rs
+++ b/tests/integration/serde.rs
@@ -396,6 +396,27 @@ fn offset_date_time_error() {
         ],
         "invalid value: integer `24`, expected a value in the range -23..=23",
     );
+    // the Deserialize impl does not recognize leap second times as valid
+    assert_de_tokens_error::<Compact<OffsetDateTime>>(
+        &[
+            Token::Tuple { len: 9 },
+            Token::I32(2021),
+            Token::U16(365),
+            Token::U8(23),
+            Token::U8(59),
+            Token::U8(60),
+            Token::U32(0),
+            Token::I8(0),
+            Token::I8(0),
+            Token::I8(0),
+            Token::TupleEnd,
+        ],
+        "invalid value: integer `60`, expected a value in the range 0..=59",
+    );
+    assert_de_tokens_error::<Readable<OffsetDateTime>>(
+        &[Token::BorrowedStr("2021-12-31 23:59:60.0 +00:00:00")],
+        "second must be in the range 0..=59",
+    );
 }
 
 #[test]