Making Duration::seconds and similar functions const

[msdrigg]

This commit updates Duration::seconds and siblings to be const. This requires the const-panic api that only stabilized in 1.57, so it may be best to hide this behid a feature flag somehow so as to not require a MSRV of 1.57. This is my first pr so I don't know the policy around this.

This pr also adds a bit of mess to the functions because we can't use .expect("error") or Datetime::partial_cmp. I think it is worth it because const durations would be really nice to have.

This also resolves https://github.com/chronotope/chrono/issues/309

• [ x ] Added the change to the changelog?
• [ x ] Added a test

[hoon-prestolabs]

It seems there's an error with lint. It doesn't finish for months.

[djc]

As mentioned in #309, this will need to be conditional on an opt-in feature flag, otherwise we have to raise the MSRV too fast. Once that change is made I'm open to merging something like this.

[msdrigg]

I'm pretty busy this week. But this coming weekend I'll figure out how to do a opt-in feature flag and make the change.

[GeorgeHahn]

@djc Would you consider a dependency on a crate like rustversion or const_fn?

[djc]

I'd prefer not to take on a procedural macro dependency. Maybe a declarative macro will work for this, something that can be called with an optional token for the const and dump a whole impl block with/without it?

[mattfbacon]

Has this stagnated? I'd love to have this feature.

[djc]

There has been no progress as far as I know. @msdrigg did you want to move this forward? @mattfbacon if not, would you be willing to open a new PR based on this branch and incorporate the review feedback?

[msdrigg]

I dont think I will have time for this for the foreseeable future. If @mattfbacon can take the reigns, it would be much appreciated

[mattfbacon]

Sure, I could take a look. I'll try to figure out if a const distinction can be done with an MBE macro.

[mattfbacon]

Const on debug, non-const on release. I couldn't figure out how to support meta and vis tokens due to parsing ambiguity so I think they need to be added individually, as I did for pub.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=14d3de0dcf41949dee89664e1333ea63

[esheppa]

As another way forward here, potentially we could have a "maybe-const" feature which then activates a dependency on const_fn as specified here. const_fn itself avoids syn and quote so it is quite lightweight. We may be able to do a similar thing for rustversion #[cfg_attr(feature = "...", const_fn::const_fn("1.xx"))]

If we want to use a macro_rules! to emulate this behavior, as far as I can see we'd still need to add a build.rs to chrono to get the version of the rust compiler being used, which may also be something we'd like to avoid.

[djc]

Yeah, not super excited about pulling in a dependency or a build script for this stuff.

[esheppa]

As an extra note, I'm experimenting with ways to use core::time::Duration, which could potentially fully or partly replace the existing Duration in an 0.5 release. Then people using more recent rust versions would automatically get these functions as const and we essentially farm out working out the const/non-const to the core/std library!

@mattfbacon / @msdrigg - would having a Duration type in chrono that was either core::time::Duration or implemented From<core::time::Duration> provide similar utility as the changes here? If so, perhaps we close this in favour implementing the use of core::time::Duration in 0.5

[djc]

core::time::Duration doesn't support negative Durations, right? I thought there was a fairly fundamental difference.

[esheppa]

Correct - I'm trying out two options: a) use core::time::Duration directly and remove all uses of negative duration - cleaner but causes more breakage b) create a new SignedDuration that wraps the core::time::Duration - at the cost of some stack space

pub enum SignedDuration {
   Forwards(Duration),
   Backwards(Duration),
}

once I've got all the tests passing, I'll create some PR's to collect feedback on whether any of those might be feasible, or if we can potentially find a path forward from one of those options to something feasible.

[pitdicker]

I played with this PR, hoping to help push it over the finish line now that our MSRV is 1.56 and 1.57 is maybe possible.

But I now think it is not good to do this in the 0.4.x branch. We can only make the methods const for our own implementation, which you get with the --no-default-features cargo flag. Otherwise the non-const methods from time 0.1 will be used.

Suppose you use the const methods in your crate. When you add a dependency that somewhere in its dependency graph depends on chrono and doesn't set the flag, your crate suddenly fails to compile. The situation where enabling a feature flag makes less code compile does not seem smart.

[pitdicker]

Our MSRV is now 1.57, and we no longer have the time 0.1 dependency. So this change has become possible.

https://github.com/chronotope/chrono/pull/1137 adds try_* methods besides the panicking ones, and makes use of some of the macros that were added to make writing const code a bit nicer.

@msdrigg I have included you out_of_bounds helper method and test_duration_const in a commit in https://github.com/chronotope/chrono/pull/1137#issuecomment-1653760288.

Thank you for working on this!

[msdrigg]

Glad to see the progress on this!