-
Notifications
You must be signed in to change notification settings - Fork 127
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
Fix clippy::missing-panics-doc #1377
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to have pointed out a few areas where we could potentially improve the code. I'd be a bit hesitant to leave this as "just doc changes". WDYT about trying to remove most of the panics in separate PRs?
Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and all files reviewed, and 16 discussions need to be resolved
nativelink-scheduler/src/worker.rs
line 128 at r1 (raw file):
/// This function will panic if the system time is earlier than the Unix epoch. /// The panic occurs due to the call to `SystemTime::now().duration_since(UNIX_EPOCH).unwrap()`, /// which will fail if the system clock is misconfigured or set to a time before the Unix epoch.
Don't explain code-level internals.
Suggestion:
which might occur if the system clock is misconfigured or set to a time before the Unix epoch.
nativelink-service/src/worker_api_server.rs
line 64 at r1 (raw file):
/// # Errors /// /// Returns an error if the server could not be created with the given configuration and schedulers.
This comment is incorrect. The panic only occurs during creation of schedulers, but the one at self::new_with_now_fn
is an error.
expect
panics in the unexpected casemap_error
maps to an error :D
You probably want something like this:
/// Creates a new `WorkerApiServer` with the given configuration and schedulers.
///
/// # Panics
///
/// This function panics if the system time is earlier than the Unix epoch when retrieving the system
/// time for scheduler timeouts (`SystemTime::now().duration_since(UNIX_EPOCH)`).
///
/// # Errors
///
/// Returns an error in the following situations:
/// - If the server could not be created with the given configuration and schedulers.
/// - If the system time is invalid during server creation (i.e., it's before the Unix epoch).
nativelink-store/src/s3_store.rs
line 268 at r1 (raw file):
/// /// This function will panic if the application name provided to the AWS SDK is invalid. /// The panic occurs with the message `"valid app name"` in the call to `AppName::new()`.
Omit explanations at the code-level. Also, since "nativelink" is hardcoded, it should be mentioned that hitting this panic should be impossible
Suggestion:
/// This function will panic if the application name "nativelink" isn't accepted by the AWS SDK which should never happen.
nativelink-store/src/shard_store.rs
line 57 at r1 (raw file):
/// # Errors /// /// Returns an error if the number of `config.stores` does not match the number of `stores`, or if `config.stores` is empty.
For multiple error cases use a list to clarify how many cases could occur:
/// Returns an error in the following cases:
/// - If the number of stores in `config.stores` does not match the length of the `stores` vector.
/// - If `config.stores` is empty.
The panic in this function is so impossible to hit that we should probably try to find another way to implement this without unwrap.
nativelink-store/src/shard_store.rs
line 84 at r1 (raw file):
*state += weight; Some(*state) })
I believe constructive approach should let us remove this panic:
let weights: Vec<u32> = {
let mut cumulative_weight = 0u32;
let len = config.stores.len();
config
.stores
.iter()
.enumerate()
.map(|(i, shard_config)| {
let weight = (u32::MAX as u64 * shard_config.weight.unwrap_or(1) as u64 / total_weight) as u32;
cumulative_weight = if i == len - 1 {
u32::MAX
} else {
cumulative_weight.saturating_add(weight)
};
cumulative_weight
})
.collect()
};
nativelink-util/src/buf_channel.rs
line 280 at r1 (raw file):
/// # Panics /// /// This function will panic if the `recent_data` is not empty after draining it.
/// This function will panic if internal invariants are violated, such as:
///
/// - If `recent_data` is not empty after draining. This indicates a bug in the implementation.
/// - If the sum of the sizes of `recent_data` does not equal `bytes_received`. This also indicates a bug.
nativelink-util/src/buf_channel.rs
line 281 at r1 (raw file):
/// /// This function will panic if the `recent_data` is not empty after draining it. /// Specifically, it panics with the message `"Recent_data should be empty"`.
No need reiterate the string.
nativelink-util/src/buf_channel.rs
line 286 at r1 (raw file):
/// /// Returns an error if `bytes_received` exceeds `max_recent_data_size`. pub fn try_reset_stream(&mut self) -> Result<(), Error> {
I'm actually not sure whether we should leave this function like this. Asserts in non-test code are not ideal and this function could potentially be in the critical path. This is just draft-level, but maybe something like this could work to handle this without panics:
pub fn try_reset_stream(&mut self) -> Result<(), Error> {
if self.bytes_received > self.max_recent_data_size {
return Err(make_err!(
Code::Internal,
"Cannot reset stream, max_recent_data_size exceeded"
));
}
let mut data_sum = 0u64;
let mut chunks = Vec::with_capacity(self.recent_data.len());
for chunk in self.recent_data.iter().rev() {
data_sum += chunk.len() as u64;
chunks.push(chunk.clone());
}
if data_sum != self.bytes_received {
return Err(make_err!(
Code::Internal,
"Sum of recent_data bytes does not equal bytes_received"
));
}
self.recent_data.clear();
for chunk in chunks {
self.queued_data.push_front(Ok(chunk));
}
self.bytes_received = 0;
Ok(())
}
cc @allada
nativelink-util/src/buf_channel.rs
line 328 at r1 (raw file):
/// /// This function will panic if `queued_data` is empty after attempting to receive data. /// The panic occurs with the message `"Should have data in the queue"`.
nativelink-util/src/evicting_map.rs
line 217 at r1 (raw file):
/// /// This function will panic if the `btree` index is not properly initialized and is `None` after attempting to rebuild it. /// The panic occurs due to calling `unwrap()` on a `None` value.
Hmm unwrapping here seems like a bad idea as well. Maybe we can find a better way? However, we probably need benchmarking infra if we want to change this.
nativelink-util/src/fastcdc.rs
line 59 at r1 (raw file):
/// /// This function will panic if: /// - `min_size` is not less than `avg_size`, in which case it panics with the message `"Expected {min_size} < {avg_size}"`.
nativelink-util/src/fastcdc.rs
line 60 at r1 (raw file):
/// This function will panic if: /// - `min_size` is not less than `avg_size`, in which case it panics with the message `"Expected {min_size} < {avg_size}"`. /// - `avg_size` is not less than `max_size`, in which case it panics with the message `"Expected {avg_size} < {max_size}"`.
nativelink-util/src/metrics_utils.rs
line 331 at r1 (raw file):
/// # Panics /// /// This function will panic if the system time is earlier than the Unix epoch.
As above
nativelink-util/src/origin_context.rs
line 327 at r1 (raw file):
/// /// This function will panic if the `ActiveOriginContext` is not set (i.e., it returns `None`). /// The panic occurs with the message `"OriginContext must be set"`.
Suggestion:
/// This function will panic if the `ActiveOriginContext` is not set.
nativelink-worker/src/worker_utils.rs
line 34 at r1 (raw file):
/// /// This function will panic if the process fails and the exit status code cannot be retrieved /// (i.e., `process_output.status.code().unwrap()` returns `None`). This might happen if the process was terminated by a signal and no exit code was available.
nativelink-worker/src/worker_utils.rs
line 41 at r1 (raw file):
/// - A worker property query command cannot be parsed. /// - A worker property query command fails to execute. /// - The process output cannot be converted to UTF-8.
nit
Suggestion:
/// Returns an error if:
/// - A worker property query command can't be parsed.
/// - A worker property query command fails to run.
/// - The process output can't be converted to UTF-8.
How Has This Been Tested?
bazel test ...
Checklist
bazel test //...
passes locallygit amend
see some docsThis change is