time icon indicating copy to clipboard operation
time copied to clipboard

Missing panic condition on API docs

Open HeeillWang opened this issue 1 year ago • 4 comments

While I executed fuzzing, I faced some panics by expect. Though those are intentional crash, I think it's better to mention on docs as well. Some of APIs are already explicitly mention the panic condition.

I'll just list up the panic-condition-missing APIs, without reproduce condition as it's too obvious.

https://github.com/time-rs/time/blob/6248992b918638bc4f9e06e326c1fdace9c56fee/time/src/date.rs#L1304-L1307

https://github.com/time-rs/time/blob/6248992b918638bc4f9e06e326c1fdace9c56fee/time/src/duration.rs#L515-L535

https://github.com/time-rs/time/blob/6248992b918638bc4f9e06e326c1fdace9c56fee/time/src/duration.rs#L493-L513

https://github.com/time-rs/time/blob/6248992b918638bc4f9e06e326c1fdace9c56fee/time/src/duration.rs#L441-L453

HeeillWang avatar Sep 21 '23 01:09 HeeillWang

Doc improvements are always welcome! It's not a priority for me at the moment.

jhpratt avatar Sep 21 '23 04:09 jhpratt

More cases :

https://github.com/time-rs/time/blob/c96bb1a4474b9af1289edbdf34514fbfe95fa833/time/src/duration.rs#L1311-L1314

https://github.com/time-rs/time/blob/c96bb1a4474b9af1289edbdf34514fbfe95fa833/time/src/duration.rs#L1266-L1269

https://github.com/time-rs/time/blob/c96bb1a4474b9af1289edbdf34514fbfe95fa833/time/src/ext.rs#L239-L279

HeeillWang avatar Sep 24 '23 08:09 HeeillWang

I'd like to defer adding doc for this at the moment, as I'm still running fuzzers, more cases may come out.

However, anyone is welcomed to create pr for this.

HeeillWang avatar Sep 24 '23 08:09 HeeillWang

For panics that are deliberate, it's probably quickest to just check for assert! and .expect with ripgrep. Any other panics are likely unintentional — I can't think of any edge cases off the top of my head. .unwrap is not used anywhere, and this is enforced by clippy.

jhpratt avatar Sep 24 '23 08:09 jhpratt