diff --git a/.bazelrc b/.bazelrc index 6ba648cc0..db9eed0a5 100644 --- a/.bazelrc +++ b/.bazelrc @@ -127,7 +127,6 @@ build --@rules_rust//:clippy_flag=-Wclippy::use_debug build --@rules_rust//:clippy_flag=-Dclippy::missing_const_for_fn # Nursery overrides that are either buggy or we want to fix -build --@rules_rust//:clippy_flag=-Aclippy::fallible_impl_from # TODO(jhpratt) Flip build --@rules_rust//:clippy_flag=-Aclippy::iter_with_drain # TODO(jhpratt): Flip build --@rules_rust//:clippy_flag=-Aclippy::option_if_let_else # rust-lang/rust-clippy#8829 build --@rules_rust//:clippy_flag=-Aclippy::redundant_pub_crate # rust-lang/rust-clippy#5369 diff --git a/Cargo.toml b/Cargo.toml index f83feacc8..fdd28778a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -153,7 +153,6 @@ use-debug = "warn" missing-const-for-fn = "deny" # Nursery overrides that are either buggy or we want to fix -fallible-impl-from = { level = "allow", priority = 1 } # TODO(jhpratt) Flip iter-with-drain = { level = "allow", priority = 1 } # TODO(jhpratt) Flip option-if-let-else = { level = "allow", priority = 1 } # rust-lang/rust-clippy#8829 redundant-pub-crate = { level = "allow", priority = 1 } # rust-lang/rust-clippy#5369 diff --git a/nativelink-scheduler/tests/cache_lookup_scheduler_test.rs b/nativelink-scheduler/tests/cache_lookup_scheduler_test.rs index 910c5b704..a857fec4e 100644 --- a/nativelink-scheduler/tests/cache_lookup_scheduler_test.rs +++ b/nativelink-scheduler/tests/cache_lookup_scheduler_test.rs @@ -62,7 +62,7 @@ fn make_cache_scheduler() -> Result { async fn add_action_handles_skip_cache() -> Result<(), Error> { let context = make_cache_scheduler()?; let action_info = make_base_action_info(UNIX_EPOCH, DigestInfo::zero_digest()); - let action_result = ProtoActionResult::from(ActionResult::default()); + let action_result = ProtoActionResult::try_from(ActionResult::default())?; context .ac_store .update_oneshot(action_info.digest(), action_result.encode_to_vec().into()) diff --git a/nativelink-util/src/action_messages.rs b/nativelink-util/src/action_messages.rs index ac56066e5..a2da781f7 100644 --- a/nativelink-util/src/action_messages.rs +++ b/nativelink-util/src/action_messages.rs @@ -424,19 +424,20 @@ pub struct FileInfo { pub is_executable: bool, } -//TODO: Make this TryFrom. -impl From for FileNode { - fn from(val: FileInfo) -> Self { - let NameOrPath::Name(name) = val.name_or_path else { - panic!( +impl TryFrom for FileNode { + type Error = Error; + + fn try_from(val: FileInfo) -> Result { + match val.name_or_path { + NameOrPath::Path(_) => Err(make_input_err!( "Cannot return a FileInfo that uses a NameOrPath::Path(), it must be a NameOrPath::Name()" - ); - }; - Self { - name, - digest: Some((&val.digest).into()), - is_executable: val.is_executable, - node_properties: Option::default(), // Not supported. + )), + NameOrPath::Name(name) => Ok(Self { + name, + digest: Some((&val.digest).into()), + is_executable: val.is_executable, + node_properties: None, // Not supported. + }), } } } @@ -456,20 +457,21 @@ impl TryFrom for FileInfo { } } -//TODO: Make this TryFrom. -impl From for OutputFile { - fn from(val: FileInfo) -> Self { - let NameOrPath::Path(path) = val.name_or_path else { - panic!( +impl TryFrom for OutputFile { + type Error = Error; + + fn try_from(val: FileInfo) -> Result { + match val.name_or_path { + NameOrPath::Name(_) => Err(make_input_err!( "Cannot return a FileInfo that uses a NameOrPath::Name(), it must be a NameOrPath::Path()" - ); - }; - Self { - path, - digest: Some((&val.digest).into()), - is_executable: val.is_executable, - contents: Bytes::default(), - node_properties: Option::default(), // Not supported. + )), + NameOrPath::Path(path) => Ok(Self { + path, + digest: Some((&val.digest).into()), + is_executable: val.is_executable, + contents: Bytes::default(), + node_properties: None, // Not supported. + }), } } } @@ -493,18 +495,19 @@ impl TryFrom for SymlinkInfo { } } -// TODO: Make this TryFrom. -impl From for SymlinkNode { - fn from(val: SymlinkInfo) -> Self { - let NameOrPath::Name(name) = val.name_or_path else { - panic!( +impl TryFrom for SymlinkNode { + type Error = Error; + + fn try_from(val: SymlinkInfo) -> Result { + match val.name_or_path { + NameOrPath::Path(_) => Err(make_input_err!( "Cannot return a SymlinkInfo that uses a NameOrPath::Path(), it must be a NameOrPath::Name()" - ); - }; - Self { - name, - target: val.target, - node_properties: Option::default(), // Not supported. + )), + NameOrPath::Name(name) => Ok(Self { + name, + target: val.target, + node_properties: None, // Not supported. + }), } } } @@ -520,18 +523,21 @@ impl TryFrom for SymlinkInfo { } } -// TODO: Make this TryFrom. -impl From for OutputSymlink { - fn from(val: SymlinkInfo) -> Self { - let NameOrPath::Path(path) = val.name_or_path else { - panic!( - "Cannot return a SymlinkInfo that uses a NameOrPath::Path(), it must be a NameOrPath::Name()" - ); - }; - Self { - path, - target: val.target, - node_properties: Option::default(), // Not supported. +impl TryFrom for OutputSymlink { + type Error = Error; + + fn try_from(val: SymlinkInfo) -> Result { + match val.name_or_path { + NameOrPath::Path(path) => { + Ok(Self { + path, + target: val.target, + node_properties: None, // Not supported. + }) + } + NameOrPath::Name(_) => Err(make_input_err!( + "Cannot return a SymlinkInfo that uses a NameOrPath::Name(), it must be a NameOrPath::Path()" + )), } } } @@ -850,7 +856,7 @@ pub fn to_execute_response(action_result: ActionResult) -> ExecuteResponse { let message = action_result.message.clone(); ExecuteResponse { server_logs: logs_from(action_result.server_logs.clone()), - result: Some(action_result.into()), + result: action_result.try_into().ok(), cached_result: false, status, message, @@ -880,35 +886,48 @@ impl From for ExecuteResponse { } } -impl From for ProtoActionResult { - fn from(val: ActionResult) -> Self { +impl TryFrom for ProtoActionResult { + type Error = Error; + + fn try_from(val: ActionResult) -> Result { let mut output_symlinks = Vec::with_capacity( val.output_file_symlinks.len() + val.output_directory_symlinks.len(), ); output_symlinks.extend_from_slice(val.output_file_symlinks.as_slice()); output_symlinks.extend_from_slice(val.output_directory_symlinks.as_slice()); - Self { - output_files: val.output_files.into_iter().map(Into::into).collect(), + Ok(Self { + output_files: val + .output_files + .into_iter() + .map(TryInto::try_into) + .collect::>()?, output_file_symlinks: val .output_file_symlinks .into_iter() - .map(Into::into) - .collect(), - output_symlinks: output_symlinks.into_iter().map(Into::into).collect(), - output_directories: val.output_folders.into_iter().map(Into::into).collect(), + .map(TryInto::try_into) + .collect::>()?, + output_symlinks: output_symlinks + .into_iter() + .map(TryInto::try_into) + .collect::>()?, + output_directories: val + .output_folders + .into_iter() + .map(TryInto::try_into) + .collect::>()?, output_directory_symlinks: val .output_directory_symlinks .into_iter() - .map(Into::into) - .collect(), + .map(TryInto::try_into) + .collect::>()?, exit_code: val.exit_code, stdout_raw: Bytes::default(), stdout_digest: Some(val.stdout_digest.into()), stderr_raw: Bytes::default(), stderr_digest: Some(val.stderr_digest.into()), execution_metadata: Some(val.execution_metadata.into()), - } + }) } } @@ -1026,7 +1045,7 @@ impl TryFrom for ActionStage { }; if execute_response.cached_result { - return Ok(Self::CompletedFromCache(action_result.into())); + return Ok(Self::CompletedFromCache(action_result.try_into()?)); } Ok(Self::Completed(action_result)) } diff --git a/nativelink-worker/src/running_actions_manager.rs b/nativelink-worker/src/running_actions_manager.rs index 03d4d5416..ec99bbaee 100644 --- a/nativelink-worker/src/running_actions_manager.rs +++ b/nativelink-worker/src/running_actions_manager.rs @@ -433,12 +433,14 @@ fn upload_directory<'a, P: AsRef + Debug + Send + Sync + Clone + 'a>( .await .err_tip(|| format!("Could not open file {full_path:?}"))?; upload_file(cas_store, &full_path, hasher, metadata) - .map_ok(Into::into) - .await + .map_ok(TryInto::try_into) + .await? }); } else if file_type.is_symlink() { - symlink_futures - .push(upload_symlink(full_path, &full_work_directory).map_ok(Into::into)); + symlink_futures.push( + upload_symlink(full_path, &full_work_directory) + .map(|symlink| symlink?.try_into()), + ); } } } diff --git a/nativelink-worker/tests/running_actions_manager_test.rs b/nativelink-worker/tests/running_actions_manager_test.rs index f55db4912..a7ee2c9dc 100644 --- a/nativelink-worker/tests/running_actions_manager_test.rs +++ b/nativelink-worker/tests/running_actions_manager_test.rs @@ -2066,7 +2066,7 @@ async fn caches_results_in_action_cache_store() -> Result<(), Box(ac_store.as_ref(), action_digest.into()).await?; - let proto_result: ProtoActionResult = action_result.into(); + let proto_result: ProtoActionResult = action_result.try_into()?; assert_eq!(proto_result, retrieved_result); Ok(()) @@ -2138,7 +2138,7 @@ async fn failed_action_does_not_cache_in_action_cache() -> Result<(), Box(ac_store.as_ref(), action_digest.into()).await?; - let proto_result: ProtoActionResult = action_result.into(); + let proto_result: ProtoActionResult = action_result.try_into()?; assert_eq!(proto_result, retrieved_result); Ok(()) @@ -2237,7 +2237,7 @@ async fn success_does_cache_in_historical_results() -> Result<(), Box Result<(), Box Result<(), Box(ac_store.as_ref(), action_result_digest.into()) .await?; - let proto_result: ProtoActionResult = action_result.into(); + let proto_result: ProtoActionResult = action_result.try_into()?; assert_eq!(proto_result, retrieved_result); Ok(()) }