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

Fix clippy::missing-panics-doc #1377

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SchahinRohani
Copy link
Contributor

@SchahinRohani SchahinRohani commented Sep 30, 2024

How Has This Been Tested?

bazel test ...

Checklist

  • Updated documentation if needed
  • Tests added/amended
  • bazel test //... passes locally
  • PR is contained in a single commit, using git amend see some docs

This change is Reviewable

Copy link
Member

@aaronmondal aaronmondal left a 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 case
  • map_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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants