Resolve clippy lints, change to #[expect] (TraceMachina#1661)

jhpratt · web-flow · commit 144d289805b1 · 2025-04-03T19:08:28.000+02:00
All remaining clippy lints are converted to `#[expect]` with reasons
attached. The only exceptions are a handful of `#[allow]` attributes
in testing due to the manner in which tests are compiled.
diff --git a/nativelink-macro/src/lib.rs b/nativelink-macro/src/lib.rs
@@ -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.
diff --git a/nativelink-proto/gen_lib_rs_tool.py b/nativelink-proto/gen_lib_rs_tool.py
@@ -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
 )]
 """
diff --git a/nativelink-proto/genproto/lib.rs b/nativelink-proto/genproto/lib.rs
@@ -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
 )]
 
diff --git a/nativelink-scheduler/tests/utils/mock_scheduler.rs b/nativelink-scheduler/tests/utils/mock_scheduler.rs
@@ -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>),
@@ -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
diff --git a/nativelink-scheduler/tests/utils/scheduler_utils.rs b/nativelink-scheduler/tests/utils/scheduler_utils.rs
@@ -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>,
diff --git a/nativelink-service/src/ac_server.rs b/nativelink-service/src/ac_server.rs
@@ -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,
@@ -199,7 +198,6 @@ impl ActionCache for AcServer {
         resp
     }
 
-    #[allow(clippy::blocks_in_conditions)]
     #[instrument(
         err,
         ret(level = Level::INFO),
diff --git a/nativelink-service/src/bep_server.rs b/nativelink-service/src/bep_server.rs
@@ -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),
@@ -219,7 +218,6 @@ impl PublishBuildEvent for BepServer {
             .map_err(Error::into)
     }
 
-    #[allow(clippy::blocks_in_conditions)]
     #[instrument(
       err,
       level = Level::ERROR,
diff --git a/nativelink-service/src/bytestream_server.rs b/nativelink-service/src/bytestream_server.rs
@@ -580,7 +580,6 @@ impl ByteStreamServer {
 impl ByteStream for ByteStreamServer {
     type ReadStream = ReadStream;
 
-    #[allow(clippy::blocks_in_conditions)]
     #[instrument(
         err,
         level = Level::ERROR,
@@ -637,7 +636,6 @@ impl ByteStream for ByteStreamServer {
         resp
     }
 
-    #[allow(clippy::blocks_in_conditions)]
     #[instrument(
         err,
         level = Level::ERROR,
@@ -697,7 +695,6 @@ impl ByteStream for ByteStreamServer {
         resp
     }
 
-    #[allow(clippy::blocks_in_conditions)]
     #[instrument(
         err,
         ret(level = Level::INFO),
diff --git a/nativelink-service/src/capabilities_server.rs b/nativelink-service/src/capabilities_server.rs
@@ -91,7 +91,6 @@ impl CapabilitiesServer {
 
 #[tonic::async_trait]
 impl Capabilities for CapabilitiesServer {
-    #[allow(clippy::blocks_in_conditions)]
     #[instrument(
         err,
         ret(level = Level::INFO),
diff --git a/nativelink-service/src/cas_server.rs b/nativelink-service/src/cas_server.rs
@@ -305,7 +305,6 @@ impl CasServer {
 impl ContentAddressableStorage for CasServer {
     type GetTreeStream = GetTreeStream;
 
-    #[allow(clippy::blocks_in_conditions)]
     #[instrument(
         err,
         ret(level = Level::INFO),
@@ -332,7 +331,6 @@ impl ContentAddressableStorage for CasServer {
         resp
     }
 
-    #[allow(clippy::blocks_in_conditions)]
     #[instrument(
         err,
         ret(level = Level::INFO),
@@ -359,7 +357,6 @@ impl ContentAddressableStorage for CasServer {
         resp
     }
 
-    #[allow(clippy::blocks_in_conditions)]
     #[instrument(
         err,
         ret(level = Level::INFO),
@@ -386,7 +383,6 @@ impl ContentAddressableStorage for CasServer {
         resp
     }
 
-    #[allow(clippy::blocks_in_conditions)]
     #[instrument(
         err,
         level = Level::ERROR,
diff --git a/nativelink-service/src/execution_server.rs b/nativelink-service/src/execution_server.rs
@@ -321,7 +321,6 @@ impl Execution for ExecutionServer {
     type ExecuteStream = ExecuteStream;
     type WaitExecutionStream = ExecuteStream;
 
-    #[allow(clippy::blocks_in_conditions)]
     #[instrument(
         err,
         level = Level::ERROR,
@@ -349,7 +348,6 @@ impl Execution for ExecutionServer {
         resp
     }
 
-    #[allow(clippy::blocks_in_conditions)]
     #[instrument(
         err,
         level = Level::ERROR,
diff --git a/nativelink-service/src/worker_api_server.rs b/nativelink-service/src/worker_api_server.rs
@@ -256,7 +256,6 @@ impl WorkerApiServer {
 impl WorkerApi for WorkerApiServer {
     type ConnectWorkerStream = ConnectWorkerStream;
 
-    #[allow(clippy::blocks_in_conditions)]
     #[instrument(
         err,
         level = Level::ERROR,
@@ -277,7 +276,6 @@ impl WorkerApi for WorkerApiServer {
         resp
     }
 
-    #[allow(clippy::blocks_in_conditions)]
     #[instrument(
         err,
         ret(level = Level::INFO),
@@ -294,7 +292,6 @@ impl WorkerApi for WorkerApiServer {
             .map_err(Into::into)
     }
 
-    #[allow(clippy::blocks_in_conditions)]
     #[instrument(
         err,
         ret(level = Level::INFO),
@@ -311,7 +308,6 @@ impl WorkerApi for WorkerApiServer {
             .map_err(Into::into)
     }
 
-    #[allow(clippy::blocks_in_conditions)]
     #[instrument(
         err,
         ret(level = Level::INFO),
diff --git a/nativelink-service/tests/worker_api_server_test.rs b/nativelink-service/tests/worker_api_server_test.rs
@@ -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))
 }
diff --git a/nativelink-store/src/completeness_checking_store.rs b/nativelink-store/src/completeness_checking_store.rs
@@ -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();
                     }
 
@@ -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();
                         },
                     )
diff --git a/nativelink-store/src/filesystem_store.rs b/nativelink-store/src/filesystem_store.rs
@@ -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>,
diff --git a/nativelink-store/src/redis_store.rs b/nativelink-store/src/redis_store.rs
@@ -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}")
 }
diff --git a/nativelink-store/tests/filesystem_store_test.rs b/nativelink-store/tests/filesystem_store_test.rs
@@ -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()
@@ -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"
     );
 
@@ -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";
diff --git a/nativelink-store/tests/memory_store_test.rs b/nativelink-store/tests/memory_store_test.rs
@@ -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(())
diff --git a/nativelink-store/tests/verify_store_test.rs b/nativelink-store/tests/verify_store_test.rs
@@ -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!(
diff --git a/nativelink-util/src/action_messages.rs b/nativelink-util/src/action_messages.rs
@@ -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,
diff --git a/nativelink-util/src/fastcdc.rs b/nativelink-util/src/fastcdc.rs
@@ -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;
             }
diff --git a/nativelink-util/src/origin_event_publisher.rs b/nativelink-util/src/origin_event_publisher.rs
@@ -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();
diff --git a/nativelink-util/src/retry.rs b/nativelink-util/src/retry.rs
@@ -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,
diff --git a/nativelink-util/src/task.rs b/nativelink-util/src/task.rs
diff --git a/nativelink-util/tests/proto_stream_utils_test.rs b/nativelink-util/tests/proto_stream_utils_test.rs
diff --git a/nativelink-worker/tests/running_actions_manager_test.rs b/nativelink-worker/tests/running_actions_manager_test.rs
diff --git a/src/bin/nativelink.rs b/src/bin/nativelink.rs
