Skip to content

Commit d53363d

Browse files
jhprattjhpratt
authored
Resolve clippy::fallible_impl_from (#1771)
1 parent d545a1c commit d53363d

File tree

6 files changed

+92
-73
lines changed

6 files changed

+92
-73
lines changed

‎.bazelrc‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,6 @@ build --@rules_rust//:clippy_flag=-Wclippy::use_debug
127127
build --@rules_rust//:clippy_flag=-Dclippy::missing_const_for_fn
128128

129129
# Nursery overrides that are either buggy or we want to fix
130-
build --@rules_rust//:clippy_flag=-Aclippy::fallible_impl_from # TODO(jhpratt) Flip
131130
build --@rules_rust//:clippy_flag=-Aclippy::iter_with_drain # TODO(jhpratt): Flip
132131
build --@rules_rust//:clippy_flag=-Aclippy::option_if_let_else # rust-lang/rust-clippy#8829
133132
build --@rules_rust//:clippy_flag=-Aclippy::redundant_pub_crate # rust-lang/rust-clippy#5369

‎Cargo.toml‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,6 @@ use-debug = "warn"
152152
missing-const-for-fn = "deny"
153153

154154
# Nursery overrides that are either buggy or we want to fix
155-
fallible-impl-from = { level = "allow", priority = 1 } # TODO(jhpratt) Flip
156155
iter-with-drain = { level = "allow", priority = 1 } # TODO(jhpratt) Flip
157156
option-if-let-else = { level = "allow", priority = 1 } # rust-lang/rust-clippy#8829
158157
redundant-pub-crate = { level = "allow", priority = 1 } # rust-lang/rust-clippy#5369

‎nativelink-scheduler/tests/cache_lookup_scheduler_test.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ fn make_cache_scheduler() -> Result<TestContext, Error> {
6262
async fn add_action_handles_skip_cache() -> Result<(), Error> {
6363
let context = make_cache_scheduler()?;
6464
let action_info = make_base_action_info(UNIX_EPOCH, DigestInfo::zero_digest());
65-
let action_result = ProtoActionResult::from(ActionResult::default());
65+
let action_result = ProtoActionResult::try_from(ActionResult::default())?;
6666
context
6767
.ac_store
6868
.update_oneshot(action_info.digest(), action_result.encode_to_vec().into())

‎nativelink-util/src/action_messages.rs‎

Lines changed: 80 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -424,19 +424,20 @@ pub struct FileInfo {
424424
pub is_executable: bool,
425425
}
426426

427-
//TODO: Make this TryFrom.
428-
impl From<FileInfo> for FileNode {
429-
fn from(val: FileInfo) -> Self {
430-
let NameOrPath::Name(name) = val.name_or_path else {
431-
panic!(
427+
impl TryFrom<FileInfo> for FileNode {
428+
type Error = Error;
429+
430+
fn try_from(val: FileInfo) -> Result<Self, Error> {
431+
match val.name_or_path {
432+
NameOrPath::Path(_) => Err(make_input_err!(
432433
"Cannot return a FileInfo that uses a NameOrPath::Path(), it must be a NameOrPath::Name()"
433-
);
434-
};
435-
Self {
436-
name,
437-
digest: Some((&val.digest).into()),
438-
is_executable: val.is_executable,
439-
node_properties: Option::default(), // Not supported.
434+
)),
435+
NameOrPath::Name(name) => Ok(Self {
436+
name,
437+
digest: Some((&val.digest).into()),
438+
is_executable: val.is_executable,
439+
node_properties: None, // Not supported.
440+
}),
440441
}
441442
}
442443
}
@@ -456,20 +457,21 @@ impl TryFrom<OutputFile> for FileInfo {
456457
}
457458
}
458459

459-
//TODO: Make this TryFrom.
460-
impl From<FileInfo> for OutputFile {
461-
fn from(val: FileInfo) -> Self {
462-
let NameOrPath::Path(path) = val.name_or_path else {
463-
panic!(
460+
impl TryFrom<FileInfo> for OutputFile {
461+
type Error = Error;
462+
463+
fn try_from(val: FileInfo) -> Result<Self, Error> {
464+
match val.name_or_path {
465+
NameOrPath::Name(_) => Err(make_input_err!(
464466
"Cannot return a FileInfo that uses a NameOrPath::Name(), it must be a NameOrPath::Path()"
465-
);
466-
};
467-
Self {
468-
path,
469-
digest: Some((&val.digest).into()),
470-
is_executable: val.is_executable,
471-
contents: Bytes::default(),
472-
node_properties: Option::default(), // Not supported.
467+
)),
468+
NameOrPath::Path(path) => Ok(Self {
469+
path,
470+
digest: Some((&val.digest).into()),
471+
is_executable: val.is_executable,
472+
contents: Bytes::default(),
473+
node_properties: None, // Not supported.
474+
}),
473475
}
474476
}
475477
}
@@ -493,18 +495,19 @@ impl TryFrom<SymlinkNode> for SymlinkInfo {
493495
}
494496
}
495497

496-
// TODO: Make this TryFrom.
497-
impl From<SymlinkInfo> for SymlinkNode {
498-
fn from(val: SymlinkInfo) -> Self {
499-
let NameOrPath::Name(name) = val.name_or_path else {
500-
panic!(
498+
impl TryFrom<SymlinkInfo> for SymlinkNode {
499+
type Error = Error;
500+
501+
fn try_from(val: SymlinkInfo) -> Result<Self, Error> {
502+
match val.name_or_path {
503+
NameOrPath::Path(_) => Err(make_input_err!(
501504
"Cannot return a SymlinkInfo that uses a NameOrPath::Path(), it must be a NameOrPath::Name()"
502-
);
503-
};
504-
Self {
505-
name,
506-
target: val.target,
507-
node_properties: Option::default(), // Not supported.
505+
)),
506+
NameOrPath::Name(name) => Ok(Self {
507+
name,
508+
target: val.target,
509+
node_properties: None, // Not supported.
510+
}),
508511
}
509512
}
510513
}
@@ -520,18 +523,21 @@ impl TryFrom<OutputSymlink> for SymlinkInfo {
520523
}
521524
}
522525

523-
// TODO: Make this TryFrom.
524-
impl From<SymlinkInfo> for OutputSymlink {
525-
fn from(val: SymlinkInfo) -> Self {
526-
let NameOrPath::Path(path) = val.name_or_path else {
527-
panic!(
528-
"Cannot return a SymlinkInfo that uses a NameOrPath::Path(), it must be a NameOrPath::Name()"
529-
);
530-
};
531-
Self {
532-
path,
533-
target: val.target,
534-
node_properties: Option::default(), // Not supported.
526+
impl TryFrom<SymlinkInfo> for OutputSymlink {
527+
type Error = Error;
528+
529+
fn try_from(val: SymlinkInfo) -> Result<Self, Error> {
530+
match val.name_or_path {
531+
NameOrPath::Path(path) => {
532+
Ok(Self {
533+
path,
534+
target: val.target,
535+
node_properties: None, // Not supported.
536+
})
537+
}
538+
NameOrPath::Name(_) => Err(make_input_err!(
539+
"Cannot return a SymlinkInfo that uses a NameOrPath::Name(), it must be a NameOrPath::Path()"
540+
)),
535541
}
536542
}
537543
}
@@ -850,7 +856,7 @@ pub fn to_execute_response(action_result: ActionResult) -> ExecuteResponse {
850856
let message = action_result.message.clone();
851857
ExecuteResponse {
852858
server_logs: logs_from(action_result.server_logs.clone()),
853-
result: Some(action_result.into()),
859+
result: action_result.try_into().ok(),
854860
cached_result: false,
855861
status,
856862
message,
@@ -880,35 +886,48 @@ impl From<ActionStage> for ExecuteResponse {
880886
}
881887
}
882888

883-
impl From<ActionResult> for ProtoActionResult {
884-
fn from(val: ActionResult) -> Self {
889+
impl TryFrom<ActionResult> for ProtoActionResult {
890+
type Error = Error;
891+
892+
fn try_from(val: ActionResult) -> Result<Self, Error> {
885893
let mut output_symlinks = Vec::with_capacity(
886894
val.output_file_symlinks.len() + val.output_directory_symlinks.len(),
887895
);
888896
output_symlinks.extend_from_slice(val.output_file_symlinks.as_slice());
889897
output_symlinks.extend_from_slice(val.output_directory_symlinks.as_slice());
890898

891-
Self {
892-
output_files: val.output_files.into_iter().map(Into::into).collect(),
899+
Ok(Self {
900+
output_files: val
901+
.output_files
902+
.into_iter()
903+
.map(TryInto::try_into)
904+
.collect::<Result<_, _>>()?,
893905
output_file_symlinks: val
894906
.output_file_symlinks
895907
.into_iter()
896-
.map(Into::into)
897-
.collect(),
898-
output_symlinks: output_symlinks.into_iter().map(Into::into).collect(),
899-
output_directories: val.output_folders.into_iter().map(Into::into).collect(),
908+
.map(TryInto::try_into)
909+
.collect::<Result<_, _>>()?,
910+
output_symlinks: output_symlinks
911+
.into_iter()
912+
.map(TryInto::try_into)
913+
.collect::<Result<_, _>>()?,
914+
output_directories: val
915+
.output_folders
916+
.into_iter()
917+
.map(TryInto::try_into)
918+
.collect::<Result<_, _>>()?,
900919
output_directory_symlinks: val
901920
.output_directory_symlinks
902921
.into_iter()
903-
.map(Into::into)
904-
.collect(),
922+
.map(TryInto::try_into)
923+
.collect::<Result<_, _>>()?,
905924
exit_code: val.exit_code,
906925
stdout_raw: Bytes::default(),
907926
stdout_digest: Some(val.stdout_digest.into()),
908927
stderr_raw: Bytes::default(),
909928
stderr_digest: Some(val.stderr_digest.into()),
910929
execution_metadata: Some(val.execution_metadata.into()),
911-
}
930+
})
912931
}
913932
}
914933

@@ -1026,7 +1045,7 @@ impl TryFrom<ExecuteResponse> for ActionStage {
10261045
};
10271046

10281047
if execute_response.cached_result {
1029-
return Ok(Self::CompletedFromCache(action_result.into()));
1048+
return Ok(Self::CompletedFromCache(action_result.try_into()?));
10301049
}
10311050
Ok(Self::Completed(action_result))
10321051
}

‎nativelink-worker/src/running_actions_manager.rs‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -433,12 +433,14 @@ fn upload_directory<'a, P: AsRef<Path> + Debug + Send + Sync + Clone + 'a>(
433433
.await
434434
.err_tip(|| format!("Could not open file {full_path:?}"))?;
435435
upload_file(cas_store, &full_path, hasher, metadata)
436-
.map_ok(Into::into)
437-
.await
436+
.map_ok(TryInto::try_into)
437+
.await?
438438
});
439439
} else if file_type.is_symlink() {
440-
symlink_futures
441-
.push(upload_symlink(full_path, &full_work_directory).map_ok(Into::into));
440+
symlink_futures.push(
441+
upload_symlink(full_path, &full_work_directory)
442+
.map(|symlink| symlink?.try_into()),
443+
);
442444
}
443445
}
444446
}

‎nativelink-worker/tests/running_actions_manager_test.rs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2066,7 +2066,7 @@ async fn caches_results_in_action_cache_store() -> Result<(), Box<dyn core::erro
20662066
let retrieved_result =
20672067
get_and_decode_digest::<ProtoActionResult>(ac_store.as_ref(), action_digest.into()).await?;
20682068

2069-
let proto_result: ProtoActionResult = action_result.into();
2069+
let proto_result: ProtoActionResult = action_result.try_into()?;
20702070
assert_eq!(proto_result, retrieved_result);
20712071

20722072
Ok(())
@@ -2138,7 +2138,7 @@ async fn failed_action_does_not_cache_in_action_cache() -> Result<(), Box<dyn co
21382138
let retrieved_result =
21392139
get_and_decode_digest::<ProtoActionResult>(ac_store.as_ref(), action_digest.into()).await?;
21402140

2141-
let proto_result: ProtoActionResult = action_result.into();
2141+
let proto_result: ProtoActionResult = action_result.try_into()?;
21422142
assert_eq!(proto_result, retrieved_result);
21432143

21442144
Ok(())
@@ -2237,7 +2237,7 @@ async fn success_does_cache_in_historical_results() -> Result<(), Box<dyn core::
22372237
HistoricalExecuteResponse {
22382238
action_digest: Some(action_digest.into()),
22392239
execute_response: Some(ExecuteResponse {
2240-
result: Some(action_result.into()),
2240+
result: Some(action_result.try_into()?),
22412241
status: Some(Status::default()),
22422242
..Default::default()
22432243
}),
@@ -2351,7 +2351,7 @@ async fn infra_failure_does_cache_in_historical_results() -> Result<(), Box<dyn
23512351
HistoricalExecuteResponse {
23522352
action_digest: Some(action_digest.into()),
23532353
execute_response: Some(ExecuteResponse {
2354-
result: Some(action_result.into()),
2354+
result: Some(action_result.try_into()?),
23552355
status: Some(make_input_err!("test error").into()),
23562356
..Default::default()
23572357
}),
@@ -2407,7 +2407,7 @@ async fn action_result_has_used_in_message() -> Result<(), Box<dyn core::error::
24072407
get_and_decode_digest::<ProtoActionResult>(ac_store.as_ref(), action_result_digest.into())
24082408
.await?;
24092409

2410-
let proto_result: ProtoActionResult = action_result.into();
2410+
let proto_result: ProtoActionResult = action_result.try_into()?;
24112411
assert_eq!(proto_result, retrieved_result);
24122412
Ok(())
24132413
}

0 commit comments

Comments
 (0)