Skip to content

Resolve, rework, explain clippy lints #1661

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

Merged
merged 1 commit into from
Apr 3, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion nativelink-macro/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@ pub fn nativelink_test(attr: TokenStream, item: TokenStream) -> TokenStream {

let expanded = quote! {
#(#fn_attr)*
#[allow(clippy::disallowed_methods)]
#[expect(
clippy::disallowed_methods,
reason = "`tokio::test` uses `tokio::runtime::Runtime::block_on`"
)]
#[tokio::test(#attr)]
async fn #fn_name(#fn_inputs) #fn_output {
// Error means already initialized, which is ok.
Expand Down
2 changes: 2 additions & 0 deletions nativelink-proto/gen_lib_rs_tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,13 @@
// `bazel run nativelink-proto:update_protos`

#![allow(
unknown_lints,
clippy::default_trait_access,
clippy::doc_lazy_continuation,
clippy::doc_markdown,
clippy::large_enum_variant,
clippy::missing_const_for_fn,
clippy::doc_overindented_list_items,
rustdoc::invalid_html_tags
)]
"""
Expand Down
2 changes: 2 additions & 0 deletions nativelink-proto/genproto/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@
// `bazel run nativelink-proto:update_protos`

#![allow(
unknown_lints,
clippy::default_trait_access,
clippy::doc_lazy_continuation,
clippy::doc_markdown,
clippy::large_enum_variant,
clippy::missing_const_for_fn,
clippy::doc_overindented_list_items,
rustdoc::invalid_html_tags
)]

Expand Down
11 changes: 7 additions & 4 deletions nativelink-scheduler/tests/utils/mock_scheduler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,18 @@ use nativelink_util::operation_state_manager::{
};
use tokio::sync::{mpsc, Mutex};

#[allow(clippy::large_enum_variant)]
#[allow(dead_code)] // See https://github.com/rust-lang/rust/issues/46379
#[expect(
clippy::large_enum_variant,
reason = "testing, so this doesn't really matter"
)]
#[allow(dead_code, reason = "https://github.com/rust-lang/rust/issues/46379")]
enum ActionSchedulerCalls {
GetGetKnownProperties(String),
AddAction((OperationId, ActionInfo)),
FilterOperations(OperationFilter),
}

#[allow(dead_code)] // See https://github.com/rust-lang/rust/issues/46379
#[allow(dead_code, reason = "https://github.com/rust-lang/rust/issues/46379")]
enum ActionSchedulerReturns {
GetGetKnownProperties(Result<Vec<String>, Error>),
AddAction(Result<Box<dyn ActionStateResult>, Error>),
Expand Down Expand Up @@ -66,7 +69,7 @@ impl MockActionScheduler {
}
}

#[allow(dead_code)] // See https://github.com/rust-lang/rust/issues/46379
#[allow(dead_code, reason = "https://github.com/rust-lang/rust/issues/46379")]
pub async fn expect_get_known_properties(&self, result: Result<Vec<String>, Error>) -> String {
let mut rx_call_lock = self.rx_call.lock().await;
let ActionSchedulerCalls::GetGetKnownProperties(req) = rx_call_lock
Expand Down
4 changes: 1 addition & 3 deletions nativelink-scheduler/tests/utils/scheduler_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,7 @@ pub struct TokioWatchActionStateResult {
}

impl TokioWatchActionStateResult {
// Note: This function is only used in tests, but for some reason
// rust doesn't detect it as used.
#[allow(dead_code)]
#[allow(dead_code, reason = "https://github.com/rust-lang/rust/issues/46379")]
pub const fn new(
client_operation_id: OperationId,
action_info: Arc<ActionInfo>,
Expand Down
2 changes: 0 additions & 2 deletions nativelink-service/src/ac_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,6 @@ impl AcServer {

#[tonic::async_trait]
impl ActionCache for AcServer {
#[allow(clippy::blocks_in_conditions)]
#[instrument(
ret(level = Level::INFO),
level = Level::ERROR,
Expand Down Expand Up @@ -199,7 +198,6 @@ impl ActionCache for AcServer {
resp
}

#[allow(clippy::blocks_in_conditions)]
#[instrument(
err,
ret(level = Level::INFO),
Expand Down
2 changes: 0 additions & 2 deletions nativelink-service/src/bep_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,6 @@ type PublishBuildToolEventStreamStream = Pin<
impl PublishBuildEvent for BepServer {
type PublishBuildToolEventStreamStream = PublishBuildToolEventStreamStream;

#[allow(clippy::blocks_in_conditions)]
#[instrument(
err,
ret(level = Level::INFO),
Expand All @@ -219,7 +218,6 @@ impl PublishBuildEvent for BepServer {
.map_err(Error::into)
}

#[allow(clippy::blocks_in_conditions)]
#[instrument(
err,
level = Level::ERROR,
Expand Down
3 changes: 0 additions & 3 deletions nativelink-service/src/bytestream_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,6 @@ impl ByteStreamServer {
impl ByteStream for ByteStreamServer {
type ReadStream = ReadStream;

#[allow(clippy::blocks_in_conditions)]
#[instrument(
err,
level = Level::ERROR,
Expand Down Expand Up @@ -637,7 +636,6 @@ impl ByteStream for ByteStreamServer {
resp
}

#[allow(clippy::blocks_in_conditions)]
#[instrument(
err,
level = Level::ERROR,
Expand Down Expand Up @@ -697,7 +695,6 @@ impl ByteStream for ByteStreamServer {
resp
}

#[allow(clippy::blocks_in_conditions)]
#[instrument(
err,
ret(level = Level::INFO),
Expand Down
1 change: 0 additions & 1 deletion nativelink-service/src/capabilities_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ impl CapabilitiesServer {

#[tonic::async_trait]
impl Capabilities for CapabilitiesServer {
#[allow(clippy::blocks_in_conditions)]
#[instrument(
err,
ret(level = Level::INFO),
Expand Down
4 changes: 0 additions & 4 deletions nativelink-service/src/cas_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,6 @@ impl CasServer {
impl ContentAddressableStorage for CasServer {
type GetTreeStream = GetTreeStream;

#[allow(clippy::blocks_in_conditions)]
#[instrument(
err,
ret(level = Level::INFO),
Expand All @@ -332,7 +331,6 @@ impl ContentAddressableStorage for CasServer {
resp
}

#[allow(clippy::blocks_in_conditions)]
#[instrument(
err,
ret(level = Level::INFO),
Expand All @@ -359,7 +357,6 @@ impl ContentAddressableStorage for CasServer {
resp
}

#[allow(clippy::blocks_in_conditions)]
#[instrument(
err,
ret(level = Level::INFO),
Expand All @@ -386,7 +383,6 @@ impl ContentAddressableStorage for CasServer {
resp
}

#[allow(clippy::blocks_in_conditions)]
#[instrument(
err,
level = Level::ERROR,
Expand Down
2 changes: 0 additions & 2 deletions nativelink-service/src/execution_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,6 @@ impl Execution for ExecutionServer {
type ExecuteStream = ExecuteStream;
type WaitExecutionStream = ExecuteStream;

#[allow(clippy::blocks_in_conditions)]
#[instrument(
err,
level = Level::ERROR,
Expand Down Expand Up @@ -349,7 +348,6 @@ impl Execution for ExecutionServer {
resp
}

#[allow(clippy::blocks_in_conditions)]
#[instrument(
err,
level = Level::ERROR,
Expand Down
4 changes: 0 additions & 4 deletions nativelink-service/src/worker_api_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,6 @@ impl WorkerApiServer {
impl WorkerApi for WorkerApiServer {
type ConnectWorkerStream = ConnectWorkerStream;

#[allow(clippy::blocks_in_conditions)]
#[instrument(
err,
level = Level::ERROR,
Expand All @@ -277,7 +276,6 @@ impl WorkerApi for WorkerApiServer {
resp
}

#[allow(clippy::blocks_in_conditions)]
#[instrument(
err,
ret(level = Level::INFO),
Expand All @@ -294,7 +292,6 @@ impl WorkerApi for WorkerApiServer {
.map_err(Into::into)
}

#[allow(clippy::blocks_in_conditions)]
#[instrument(
err,
ret(level = Level::INFO),
Expand All @@ -311,7 +308,6 @@ impl WorkerApi for WorkerApiServer {
.map_err(Into::into)
}

#[allow(clippy::blocks_in_conditions)]
#[instrument(
err,
ret(level = Level::INFO),
Expand Down
5 changes: 4 additions & 1 deletion nativelink-service/tests/worker_api_server_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,10 @@ struct TestContext {
worker_id: WorkerId,
}

#[allow(clippy::unnecessary_wraps)]
#[expect(
clippy::unnecessary_wraps,
reason = "`setup_api_server` requires a method that returns a `Result`"
)]
const fn static_now_fn() -> Result<Duration, Error> {
Ok(Duration::from_secs(BASE_NOW_S))
}
Expand Down
4 changes: 2 additions & 2 deletions nativelink-store/src/completeness_checking_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ impl CompletenessCheckingStore {
}
state
.digests_to_check_idxs
.extend(iter::repeat(i).take(rep_len));
.extend(iter::repeat_n(i, rep_len));
state.notify.notify_one();
}

Expand All @@ -209,7 +209,7 @@ impl CompletenessCheckingStore {
state.digests_to_check.extend(digest_infos);
state
.digests_to_check_idxs
.extend(iter::repeat(i).take(rep_len));
.extend(iter::repeat_n(i, rep_len));
state.notify.notify_one();
},
)
Expand Down
4 changes: 2 additions & 2 deletions nativelink-store/src/filesystem_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -701,8 +701,8 @@ impl<Fe: FileEntry> FilesystemStore<Fe> {
.ok_or_else(|| make_err!(Code::NotFound, "{digest} not found in filesystem store"))
}

async fn update_file<'a>(
self: Pin<&'a Self>,
async fn update_file(
self: Pin<&Self>,
mut entry: Fe,
mut temp_file: fs::FileSlot,
final_key: StoreKey<'static>,
Expand Down
5 changes: 4 additions & 1 deletion nativelink-store/src/redis_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,10 @@ const DEFAULT_COMMAND_TIMEOUT_MS: u64 = 10_000;
/// Note: If this changes it should be updated in the config documentation.
const DEFAULT_MAX_CHUNK_UPLOADS_PER_UPDATE: usize = 10;

#[allow(clippy::trivially_copy_pass_by_ref)]
#[expect(
clippy::trivially_copy_pass_by_ref,
reason = "must match method signature expected"
)]
fn to_hex(value: &u32) -> String {
format!("{value:08x}")
}
Expand Down
5 changes: 2 additions & 3 deletions nativelink-store/tests/filesystem_store_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ impl<Hooks: FileEntryHooks + 'static + Sync + Send> Drop for TestFileEntry<Hooks
}
.instrument(tracing::error_span!("test_file_entry_drop")),
);
#[allow(clippy::disallowed_methods)]
#[expect(clippy::disallowed_methods, reason = "testing implementation")]
let thread_handle = {
std::thread::spawn(move || {
let rt = tokio::runtime::Builder::new_current_thread()
Expand Down Expand Up @@ -454,7 +454,7 @@ async fn file_continues_to_stream_on_content_replace_test() -> Result<(), Error>

assert_eq!(
&remaining_file_data,
VALUE1[1..].as_bytes(),
&VALUE1.as_bytes()[1..],
"Expected file content to match"
);

Expand Down Expand Up @@ -667,7 +667,6 @@ async fn eviction_on_insert_calls_unref_once() -> Result<(), Error> {
}

#[nativelink_test]
#[allow(clippy::await_holding_refcell_ref)]
async fn rename_on_insert_fails_due_to_filesystem_error_proper_cleanup_happens() -> Result<(), Error>
{
const INITIAL_CONTENT: &str = "hello";
Expand Down
4 changes: 2 additions & 2 deletions nativelink-store/tests/memory_store_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,10 @@ async fn read_partial() -> Result<(), Error> {
let store_data = store.get_part_unchunked(digest, 1, Some(2)).await?;

assert_eq!(
VALUE1[1..3].as_bytes(),
&VALUE1.as_bytes()[1..3],
store_data,
"Expected partial data to match, expected '{:#x?}' got: {:#x?}'",
VALUE1[1..3].as_bytes(),
&VALUE1.as_bytes()[1..3],
store_data,
);
Ok(())
Expand Down
2 changes: 1 addition & 1 deletion nativelink-store/tests/verify_store_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ async fn verify_fails_immediately_on_too_much_data_sent_update() -> Result<(), E
tx.send(VALUE.into()).await?;
pending::<()>().await;
panic!("Should not reach here");
#[allow(unreachable_code)]
#[expect(unreachable_code, reason = "needed to avoid inference errors")]
Ok(())
};
let result = try_join!(
Expand Down
7 changes: 4 additions & 3 deletions nativelink-util/src/action_messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -727,11 +727,12 @@ impl Default for ActionResult {
}
}

// TODO(allada) Remove the need for clippy argument by making the ActionResult and ProtoActionResult
// a Box.
/// The execution status/stage. This should match `ExecutionStage::Value` in `remote_execution.proto`.
#[derive(PartialEq, Debug, Clone, Serialize, Deserialize)]
#[allow(clippy::large_enum_variant)]
#[expect(
clippy::large_enum_variant,
reason = "TODO box the two relevant variants in a breaking release"
)]
pub enum ActionStage {
/// Stage is unknown.
Unknown,
Expand Down
2 changes: 1 addition & 1 deletion nativelink-util/src/fastcdc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ impl FastCDC {
assert!(min_size < avg_size, "Expected {min_size} < {avg_size}");
assert!(avg_size < max_size, "Expected {avg_size} < {max_size}");
let norm_size = {
let mut offset = min_size + ((min_size + 1) / 2);
let mut offset = min_size + min_size.div_ceil(2);
if offset > avg_size {
offset = avg_size;
}
Expand Down
15 changes: 8 additions & 7 deletions nativelink-util/src/origin_event_publisher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,14 @@ impl OriginEventPublisher {
async fn handle_batch(&self, batch: &mut Vec<OriginEvent>) {
let uuid = Uuid::now_v6(&get_node_id(None));
let events = OriginEvents {
// Clippy wants us to use use `mem::take`, but this would
// move all capacity as well to the new vector. Since it is
// much more likely that we will have a small number of events
// in the batch, we prefer to use `drain` and `collect` here,
// so we only need to allocate the exact amount of memory needed
// and let the batch vector's capacity be reused.
#[allow(clippy::drain_collect)]
#[expect(
clippy::drain_collect,
reason = "Clippy wants us to use use `mem::take`, but this would move all capacity \
as well to the new vector. Since it is much more likely that we will have a \
small number of events in the batch, we prefer to use `drain` and `collect` \
here, so we only need to allocate the exact amount of memory needed and let \
the batch vector's capacity be reused."
)]
events: batch.drain(..).collect(),
};
let mut data = BytesMut::new();
Expand Down
12 changes: 5 additions & 7 deletions nativelink-util/src/retry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,11 @@ impl Retrier {
.take(self.config.max_retries) // Remember this is number of retries, so will run max_retries + 1.
}

// Clippy complains that this function can be `async fn`, but this is not true.
// If we use `async fn`, other places in our code will fail to compile stating
// something about the async blocks not matching.
// This appears to happen due to a compiler bug while inlining, because the
// function that it complained about was calling another function that called
// this one.
#[allow(clippy::manual_async_fn)]
#[expect(
clippy::manual_async_fn,
reason = "making an `async fn` results in a potential compiler bug in seemingly unrelated \
code"
)]
pub fn retry<'a, T: Send>(
&'a self,
operation: impl futures::stream::Stream<Item = RetryResult<T>> + Send + 'a,
Expand Down
Loading
Loading