Skip to content
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.

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

Missing panic condition on API docs #623

Open
HeeillWang opened this issue Sep 21, 2023 · 4 comments
Open

Missing panic condition on API docs #623

HeeillWang opened this issue Sep 21, 2023 · 4 comments
Labels
A-docs Area: documentation E-help-wanted 📣 Community assistance is desired and welcomed.

Comments

@HeeillWang
Copy link

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.

time/time/src/date.rs

Lines 1304 to 1307 in 6248992

fn add(self, duration: Duration) -> Self::Output {
self.checked_add(duration)
.expect("overflow adding duration to date")
}

time/time/src/duration.rs

Lines 515 to 535 in 6248992

/// Creates a new `Duration` from the specified number of seconds represented as `f32`.
///
/// ```rust
/// # use time::{Duration, ext::NumericalDuration};
/// assert_eq!(Duration::seconds_f32(0.5), 0.5.seconds());
/// assert_eq!(Duration::seconds_f32(-0.5), (-0.5).seconds());
/// ```
pub fn seconds_f32(seconds: f32) -> Self {
try_from_secs!(
secs = seconds,
mantissa_bits = 23,
exponent_bits = 8,
offset = 41,
bits_ty = u32,
bits_ty_signed = i32,
double_ty = u64,
float_ty = f32,
is_nan = crate::expect_failed("passed NaN to `time::Duration::seconds_f32`"),
is_overflow = crate::expect_failed("overflow constructing `time::Duration`"),
)
}

time/time/src/duration.rs

Lines 493 to 513 in 6248992

/// Creates a new `Duration` from the specified number of seconds represented as `f64`.
///
/// ```rust
/// # use time::{Duration, ext::NumericalDuration};
/// assert_eq!(Duration::seconds_f64(0.5), 0.5.seconds());
/// assert_eq!(Duration::seconds_f64(-0.5), -0.5.seconds());
/// ```
pub fn seconds_f64(seconds: f64) -> Self {
try_from_secs!(
secs = seconds,
mantissa_bits = 52,
exponent_bits = 11,
offset = 44,
bits_ty = u64,
bits_ty_signed = i64,
double_ty = u128,
float_ty = f64,
is_nan = crate::expect_failed("passed NaN to `time::Duration::seconds_f64`"),
is_overflow = crate::expect_failed("overflow constructing `time::Duration`"),
)
}

time/time/src/duration.rs

Lines 441 to 453 in 6248992

/// Create a new `Duration` with the given number of days. Equivalent to
/// `Duration::seconds(days * 86_400)`.
///
/// ```rust
/// # use time::{Duration, ext::NumericalDuration};
/// assert_eq!(Duration::days(1), 86_400.seconds());
/// ```
pub const fn days(days: i64) -> Self {
Self::seconds(expect_opt!(
days.checked_mul(Second.per(Day) as _),
"overflow constructing `time::Duration`"
))
}

@jhpratt
Copy link
Member

jhpratt commented Sep 21, 2023

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

@jhpratt jhpratt added the A-docs Area: documentation label Sep 21, 2023
@HeeillWang
Copy link
Author

More cases :

time/time/src/duration.rs

Lines 1311 to 1314 in c96bb1a

fn sub(self, rhs: Self) -> Self::Output {
self.checked_sub(rhs)
.expect("overflow when subtracting durations")
}

time/time/src/duration.rs

Lines 1266 to 1269 in c96bb1a

fn add(self, rhs: Self) -> Self::Output {
self.checked_add(rhs)
.expect("overflow when adding durations")
}

time/time/src/ext.rs

Lines 239 to 279 in c96bb1a

impl NumericalStdDuration for f64 {
fn std_nanoseconds(self) -> StdDuration {
assert!(self >= 0.);
StdDuration::from_nanos(self as _)
}
fn std_microseconds(self) -> StdDuration {
assert!(self >= 0.);
StdDuration::from_nanos((self * Nanosecond::per(Microsecond) as Self) as _)
}
fn std_milliseconds(self) -> StdDuration {
assert!(self >= 0.);
StdDuration::from_nanos((self * Nanosecond::per(Millisecond) as Self) as _)
}
fn std_seconds(self) -> StdDuration {
assert!(self >= 0.);
StdDuration::from_nanos((self * Nanosecond::per(Second) as Self) as _)
}
fn std_minutes(self) -> StdDuration {
assert!(self >= 0.);
StdDuration::from_nanos((self * Nanosecond::per(Minute) as Self) as _)
}
fn std_hours(self) -> StdDuration {
assert!(self >= 0.);
StdDuration::from_nanos((self * Nanosecond::per(Hour) as Self) as _)
}
fn std_days(self) -> StdDuration {
assert!(self >= 0.);
StdDuration::from_nanos((self * Nanosecond::per(Day) as Self) as _)
}
fn std_weeks(self) -> StdDuration {
assert!(self >= 0.);
StdDuration::from_nanos((self * Nanosecond::per(Week) as Self) as _)
}
}

@HeeillWang
Copy link
Author

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.

@jhpratt
Copy link
Member

jhpratt commented Sep 24, 2023

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 jhpratt added the E-help-wanted 📣 Community assistance is desired and welcomed. label Sep 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation E-help-wanted 📣 Community assistance is desired and welcomed.
Projects
None yet
Development

No branches or pull requests

2 participants