add: duration_checked_sub RFC

[benaryorg]

This would as described in the RFC add the method Duration::checked_sub() and as an unresolved question if a few other, similar methods should be added.

[aturon]

Rendered

Thanks for the RFC! This seems like a pretty reasonable addition to me -- I was actually slightly surprised it wasn't already provided. I think if we add this method, we should at least provide the other checked methods.

Regarding the lack of a CheckedSub trait: the Sub trait is there primarily for operator overloading, and only to some degree for generic programming. We haven't been pushing hard on generic programming over integer types, but I could see traits sprouting up for the other method variants at some point in the future.

[aturon]

cc @wycats

[benaryorg]

Regardung the trait: I do know that, I just wanted to point out that this raises the amount of similar methods in stdlib and that others might ask why.

Also, isn't it a little problematic to have those methods not umplemented as traits, as I can imagine it makes it harder for e.g. the num crate to provide these, even though I'm not sure.

Also when doing any maths in a generic way (outside the stdlib of course) for example to provide a complex mathematical operation in a way that it is not dependent on datatypes which might overflow it would be annoyingly hard to test for that when not having that trait at hand.

Back to the scope of this RFC:

You mentioned that at least all checked operations should be implemented, where I would at least want to see the saturated too as there is AFAIK no constant for the maximum duration as there is for primitive types.

[benaryorg]

In my opinion the corresponding add, sub, mul and div should actually be all implemented. I wasn't sure if you were thinking similarly so I did only propose it as a side note.

[wycats]

I'm a little concerned about the noisiness of these additions.

Maybe we could have a duration.checked() (CheckedDuration) type that implements the regular math operators (and returns Result) would be better? I don't love the word Checked for this kind of type. Can anyone think of a better name?

As an aside, I think this type should return a Result<Duration, Overflow> rather than an Option.

[benaryorg]

I think the word you are looking for is Safe as in SafeDuration, but I actually used the existing methods of primitive types as a template: u64::checked_sub.

[benaryorg]

What you are looking for might be the u64::overflowing_add method.

[nagisa]

Well, for one primitive types are primitive and Duration primitive is not, thus comparing them is not very desirable.

It seems to make sense to me for Duration and other non-primitive-number-like types to only provide safe operations and require casting to Checked<Duration>/Wrapping<Duration>/etc or something to enable all the other weird operations with desired semantics.

[benaryorg]

[…] to only provide safe operations and require casting to […]

By "safe" you mean panicking on strange values?

[nagisa]

@benaryorg We haven’t anything like Checked yet, but I do not see why Checked::sub (NB: not ::std::ops::Sub::sub) couldn’t return an Option<Self> or a similar associated type.

[benaryorg]

So, this Checked<T> construct, how would this work with borrowing/lifetime? I mean, it can't be immutable obviously, but it can't be owned, you might still want to access the original value and due to current scopes it would be pretty annoying to have use a wrapped and a normal Duration.
Aside that it would be impossible work with Overflowing<T> and Checked<T> simultaneously.

[wycats]

I think the word you are looking for is Safe as in SafeDuration

I try not to use "safe" to mean something other than the same as the meaning of unsafe, and this doesn't qualify.

In general, I feel we would benefit from having some terminology for this, and I don't think checked is too bad: it has some precedent in existing methods, and I chose the name CheckedValue<T> for a similar purpose in Helix.

I'm also a fan of moving these concepts in their own types, and having their failures produce Results, rather than a zoo of functions that sometimes panic.

[benaryorg]

I try not to use "safe" to mean something other than the same as the meaning of unsafe, and this doesn't qualify.

I definitely do agree on that.

Say, @wycats, what does your CheckedValue<T> do?
Also, is it a struct or a trait because in case of a generic type for this, a struct would need to be implemented for every type directly at the CheckedValue's implementation, which is kind of hard to use then (no user defined types and all of the definitions for completely unrelated types in one place, separated from the type's definition).
A trait however would be implementable for each type on it's own which would result in the mentioned Checked trait that could be implemented for each type.

Implementing it for each type would have the advantage of keeping that code separated from the other fns and making generic programming a little easier because you can constrain your types like this.
Edit: This would also make the implementation individual, enabling differing signatures for types that do support more and types that do support less checked operations on them.

Both the struct and the trait however could have an impl for all types that are Add so that they are simply added the way they'd panic, but the panic caught and None/Err returned.

Please do not hesitate to correct me.

[alexcrichton]

🔔 This RFC is now entering its final comment period 🔔

The libs teams's inclination is to add this, along with other checked_* methods corresponding to the other ops impls on Duration

[benaryorg]

I'm supporting that. So, should I update the file to reflect the other methods?

[sfackler]

Please do!

[alexcrichton]

I believe the signature here should return Option<Duration>, right?

(similarly Some(Duration) below)

[alexcrichton]

Discussed during libs triage yesterday the decision was to merge.

@benaryorg could you update with the one minor nit we saw, and after that I'll merge the PR?

[benaryorg]

Okay, so, sorry for both messing up GitHub (on mobile right now, hit the close button by accident) and the original RFC, of course it should be Option and Some.

[alexcrichton]

Thanks @benaryorg!

Tracking issue