Resolve clippy lints, change to #[expect]

jhpratt · jhpratt · commit 580f9250cdc6 · 2025-04-03T13:35:20.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-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/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()
@@ -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/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/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
@@ -34,7 +34,7 @@ where
     T: Send + 'static,
     F: Future<Output = T> + Send + 'static,
 {
-    #[allow(clippy::disallowed_methods)]
+    #[expect(clippy::disallowed_methods, reason = "purpose of the method")]
     tokio::spawn(ContextAwareFuture::new(ctx, f.instrument(span)))
 }
 
@@ -51,7 +51,7 @@ where
     F: FnOnce() -> T + Send + 'static,
     T: Send + 'static,
 {
-    #[allow(clippy::disallowed_methods)]
+    #[expect(clippy::disallowed_methods, reason = "purpose of the method")]
     spawn_blocking(move || span.in_scope(f))
 }
 
diff --git a/nativelink-worker/tests/running_actions_manager_test.rs b/nativelink-worker/tests/running_actions_manager_test.rs
@@ -2156,7 +2156,10 @@ async fn success_does_cache_in_historical_results() -> Result<(), Box<dyn std::e
                 upload_historical_results_strategy: Some(
                     nativelink_config::cas_server::UploadCacheResultsStrategy::SuccessOnly,
                 ),
-                #[allow(clippy::literal_string_with_formatting_args)]
+                #[expect(
+                    clippy::literal_string_with_formatting_args,
+                    reason = "passed to `formatx` crate for runtime interpretation"
+                )]
                 success_message_template: "{historical_results_hash}-{historical_results_size}"
                     .to_string(),
                 ..Default::default()
@@ -2257,7 +2260,6 @@ async fn failure_does_not_cache_in_historical_results() -> Result<(), Box<dyn st
                 upload_historical_results_strategy: Some(
                     nativelink_config::cas_server::UploadCacheResultsStrategy::SuccessOnly,
                 ),
-                #[allow(clippy::literal_string_with_formatting_args)]
                 success_message_template: "{historical_results_hash}-{historical_results_size}"
                     .to_string(),
                 ..Default::default()
@@ -2299,7 +2301,10 @@ async fn infra_failure_does_cache_in_historical_results() -> Result<(), Box<dyn
                 upload_historical_results_strategy: Some(
                     nativelink_config::cas_server::UploadCacheResultsStrategy::FailuresOnly,
                 ),
-                #[allow(clippy::literal_string_with_formatting_args)]
+                #[expect(
+                    clippy::literal_string_with_formatting_args,
+                    reason = "passed to `formatx` crate for runtime interpretation"
+                )]
                 failure_message_template: "{historical_results_hash}-{historical_results_size}"
                     .to_string(),
                 ..Default::default()
@@ -2367,7 +2372,6 @@ async fn action_result_has_used_in_message() -> Result<(), Box<dyn std::error::E
             upload_action_result_config: &nativelink_config::cas_server::UploadActionResultConfig {
                 upload_ac_results_strategy:
                     nativelink_config::cas_server::UploadCacheResultsStrategy::SuccessOnly,
-                #[allow(clippy::literal_string_with_formatting_args)]
                 success_message_template: "{action_digest_hash}-{action_digest_size}".to_string(),
                 ..Default::default()
             },
diff --git a/src/bin/nativelink.rs b/src/bin/nativelink.rs
