From bfdb0d9bf2818d94c32f59c97072392f15cc2598 Mon Sep 17 00:00:00 2001 From: rami3l Date: Sat, 22 Feb 2025 11:49:18 +0800 Subject: [PATCH 1/2] style: partially migrate away from `if-let` Following , I have manually audited all `if_let_rescope` cases and found out that certain existing cases can be rewritten to make it clearer semantically. So these migrations have been made in advance so that the only cases of `if-let` remaining are false positives of this lint. The crux of determining false positives here is that, according to , `if let Some()` and other similar cases where the `else` drop is not significant are not affected by the Edition change. --- src/diskio/immediate.rs | 62 ++++++++++++++---------------- src/dist/component/package.rs | 71 +++++++++++++++++------------------ src/dist/mod.rs | 6 +-- 3 files changed, 65 insertions(+), 74 deletions(-) diff --git a/src/diskio/immediate.rs b/src/diskio/immediate.rs index 2bc2a5d415..e3fba7f2fc 100644 --- a/src/diskio/immediate.rs +++ b/src/diskio/immediate.rs @@ -81,21 +81,20 @@ impl Executor for ImmediateUnpacker { // If there is a pending error, return it, otherwise stash the // Item for eventual return when the file is finished. let mut guard = self.incremental_state.lock().unwrap(); - if let Some(ref mut state) = *guard { - if state.err.is_some() { - let err = state.err.take().unwrap(); - item.result = err; - item.finish = item - .start - .map(|s| Instant::now().saturating_duration_since(s)); - *guard = None; - Box::new(Some(CompletedIo::Item(item)).into_iter()) - } else { - state.item = Some(item); - Box::new(None.into_iter()) - } + let Some(ref mut state) = *guard else { + unreachable!() + }; + if state.err.is_some() { + let err = state.err.take().unwrap(); + item.result = err; + item.finish = item + .start + .map(|s| Instant::now().saturating_duration_since(s)); + *guard = None; + Box::new(Some(CompletedIo::Item(item)).into_iter()) } else { - unreachable!(); + state.item = Some(item); + Box::new(None.into_iter()) } }; } @@ -181,9 +180,7 @@ impl IncrementalFileWriter { if (self.state.lock().unwrap()).is_none() { return false; } - let chunk = if let FileBuffer::Immediate(v) = chunk { - v - } else { + let FileBuffer::Immediate(chunk) = chunk else { unreachable!() }; match self.write(chunk) { @@ -203,25 +200,24 @@ impl IncrementalFileWriter { fn write(&mut self, chunk: Vec) -> std::result::Result { let mut state = self.state.lock().unwrap(); - if let Some(ref mut state) = *state { - if let Some(ref mut file) = self.file.as_mut() { - // Length 0 vector is used for clean EOF signalling. - if chunk.is_empty() { - trace_scoped!("close", "name:": self.path_display); - drop(std::mem::take(&mut self.file)); - state.finished = true; - } else { - trace_scoped!("write_segment", "name": self.path_display, "len": chunk.len()); - file.write_all(&chunk)?; - - state.completed_chunks.push(chunk.len()); - } - Ok(true) + let Some(ref mut state) = *state else { + unreachable!() + }; + if let Some(ref mut file) = self.file.as_mut() { + // Length 0 vector is used for clean EOF signalling. + if chunk.is_empty() { + trace_scoped!("close", "name:": self.path_display); + drop(std::mem::take(&mut self.file)); + state.finished = true; } else { - Ok(false) + trace_scoped!("write_segment", "name": self.path_display, "len": chunk.len()); + file.write_all(&chunk)?; + + state.completed_chunks.push(chunk.len()); } + Ok(true) } else { - unreachable!(); + Ok(false) } } } diff --git a/src/dist/component/package.rs b/src/dist/component/package.rs index a3759eb388..82bc823862 100644 --- a/src/dist/component/package.rs +++ b/src/dist/component/package.rs @@ -229,10 +229,9 @@ fn filter_result(op: &mut CompletedIo) -> io::Result<()> { // mkdir of e.g. ~/.rustup already existing is just fine; // for others it would be better to know whether it is // expected to exist or not -so put a flag in the state. - if let Kind::Directory = op.kind { - Ok(()) - } else { - Err(e) + match op.kind { + Kind::Directory => Ok(()), + _ => Err(e), } } _ => Err(e), @@ -454,40 +453,40 @@ fn unpack_without_first_dir( let item = loop { // Create the full path to the entry if it does not exist already - if let Some(parent) = item.full_path.to_owned().parent() { - match directories.get_mut(parent) { - None => { - // Tar has item before containing directory - // Complain about this so we can see if these exist. - writeln!( - process.stderr().lock(), - "Unexpected: missing parent '{}' for '{}'", - parent.display(), - entry.path()?.display() - )?; - directories.insert(parent.to_owned(), DirStatus::Pending(vec![item])); - item = Item::make_dir(parent.to_owned(), 0o755); - // Check the parent's parent - continue; - } - Some(DirStatus::Exists) => { - break Some(item); - } - Some(DirStatus::Pending(pending)) => { - // Parent dir is being made - pending.push(item); - if incremental_file_sender.is_none() { - // take next item from tar - continue 'entries; - } else { - // don't submit a new item for processing, but do be ready to feed data to the incremental file. - break None; - } + let full_path = item.full_path.to_owned(); + let Some(parent) = full_path.parent() else { + // We should never see a path with no parent. + unreachable!() + }; + match directories.get_mut(parent) { + None => { + // Tar has item before containing directory + // Complain about this so we can see if these exist. + writeln!( + process.stderr().lock(), + "Unexpected: missing parent '{}' for '{}'", + parent.display(), + entry.path()?.display() + )?; + directories.insert(parent.to_owned(), DirStatus::Pending(vec![item])); + item = Item::make_dir(parent.to_owned(), 0o755); + // Check the parent's parent + continue; + } + Some(DirStatus::Exists) => { + break Some(item); + } + Some(DirStatus::Pending(pending)) => { + // Parent dir is being made + pending.push(item); + if incremental_file_sender.is_none() { + // take next item from tar + continue 'entries; + } else { + // don't submit a new item for processing, but do be ready to feed data to the incremental file. + break None; } } - } else { - // We should never see a path with no parent. - panic!(); } }; diff --git a/src/dist/mod.rs b/src/dist/mod.rs index bfcfdb9585..e65ca52cf3 100644 --- a/src/dist/mod.rs +++ b/src/dist/mod.rs @@ -335,11 +335,7 @@ impl FromStr for ParsedToolchainDesc { } }); - if let Some(d) = d { - Ok(d) - } else { - Err(RustupError::InvalidToolchainName(desc.to_string()).into()) - } + d.ok_or_else(|| RustupError::InvalidToolchainName(desc.to_string()).into()) } } From 41f0298245d781271f7dd60baa6f10150c85d71f Mon Sep 17 00:00:00 2001 From: rami3l Date: Sat, 22 Feb 2025 15:11:42 +0800 Subject: [PATCH 2/2] DON'T MERGE[ci skip]: demonstrate `if_let_rescope` auto migration --- Cargo.toml | 3 +- src/cli/common.rs | 9 +++--- src/cli/rustup_mode.rs | 15 +++++---- src/config.rs | 56 +++++++++++++++------------------- src/diskio/immediate.rs | 18 +++++------ src/dist/download.rs | 6 ++-- src/dist/manifestation.rs | 8 ++--- src/dist/mod.rs | 6 ++-- src/toolchain.rs | 6 ++-- src/toolchain/distributable.rs | 6 ++-- 10 files changed, 62 insertions(+), 71 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 6bdb354e03..98c98ac3c2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -155,7 +155,8 @@ url = "2.4" walkdir = "2" [workspace.lints.rust] -rust_2018_idioms = "deny" +rust_2018_idioms = { level = "deny", priority = -1 } +if_let_rescope = { level = "warn", priority = -2 } [workspace.lints.clippy] # `dbg!()` and `todo!()` clearly shouldn't make it to production: diff --git a/src/cli/common.rs b/src/cli/common.rs index 8c37d4ac80..623906ac26 100644 --- a/src/cli/common.rs +++ b/src/cli/common.rs @@ -417,13 +417,12 @@ pub(crate) fn list_toolchains( } else { let default_toolchain_name = cfg.get_default()?; let active_toolchain_name: Option = - if let Ok(Some((LocalToolchainName::Named(toolchain), _reason))) = - cfg.find_active_toolchain() - { + match cfg.find_active_toolchain() + { Ok(Some((LocalToolchainName::Named(toolchain), _reason))) => { Some(toolchain) - } else { + } _ => { None - }; + }}; for toolchain in toolchains { let is_default_toolchain = default_toolchain_name.as_ref() == Some(&toolchain); diff --git a/src/cli/rustup_mode.rs b/src/cli/rustup_mode.rs index 43a867cb1e..405ffea35d 100644 --- a/src/cli/rustup_mode.rs +++ b/src/cli/rustup_mode.rs @@ -727,7 +727,7 @@ async fn default_( ) -> Result { common::warn_if_host_is_emulated(cfg.process); - if let Some(toolchain) = toolchain { + match toolchain { Some(toolchain) => { match toolchain.to_owned() { MaybeResolvableToolchainName::None => { cfg.set_default(None)?; @@ -756,12 +756,12 @@ async fn default_( info!("note that the toolchain '{toolchain}' is currently in use ({reason})"); } } - } else { + } _ => { let default_toolchain = cfg .get_default()? .ok_or_else(|| anyhow!("no default toolchain is configured"))?; writeln!(cfg.process.stdout().lock(), "{default_toolchain} (default)")?; - } + }} Ok(utils::ExitCode(0)) } @@ -961,13 +961,12 @@ fn show(cfg: &Cfg<'_>, verbose: bool) -> Result { let installed_toolchains = cfg.list_toolchains()?; let active_toolchain_and_reason: Option<(ToolchainName, ActiveReason)> = - if let Ok(Some((LocalToolchainName::Named(toolchain_name), reason))) = - cfg.find_active_toolchain() - { + match cfg.find_active_toolchain() + { Ok(Some((LocalToolchainName::Named(toolchain_name), reason))) => { Some((toolchain_name, reason)) - } else { + } _ => { None - }; + }}; let (active_toolchain_name, _active_reason) = active_toolchain_and_reason .as_ref() diff --git a/src/config.rs b/src/config.rs index 8787d80981..a5579eb2f3 100644 --- a/src/config.rs +++ b/src/config.rs @@ -518,42 +518,34 @@ impl<'a> Cfg<'a> { &self, ) -> Result> { Ok( - if let Some((override_config, reason)) = self.find_override_config()? { + match self.find_override_config()? { Some((override_config, reason)) => { Some((override_config.into_local_toolchain_name(), reason)) - } else { + } _ => { self.get_default()? .map(|x| (x.into(), ActiveReason::Default)) - }, + }}, ) } fn find_override_config(&self) -> Result> { let override_config: Option<(OverrideCfg, ActiveReason)> = // First check +toolchain override from the command line - if let Some(ref name) = self.toolchain_override { + match self.toolchain_override { Some(ref name) => { let override_config = name.resolve(&self.get_default_host_triple()?)?.into(); Some((override_config, ActiveReason::CommandLine)) - } - // Then check the RUSTUP_TOOLCHAIN environment variable - else if let Some(ref name) = self.env_override { + } _ => { match self.env_override { Some(ref name) => { // Because path based toolchain files exist, this has to support // custom, distributable, and absolute path toolchains otherwise // rustup's export of a RUSTUP_TOOLCHAIN when running a process will // error when a nested rustup invocation occurs Some((name.clone().into(), ActiveReason::Environment)) - } - // Then walk up the directory tree from 'path' looking for either the - // directory in the override database, or a `rust-toolchain{.toml}` file, - // in that order. - else if let Some((override_cfg, active_reason)) = self.settings_file.with(|s| { + } _ => { match self.settings_file.with(|s| { self.find_override_from_dir_walk(&self.current_dir, s) - })? { + })? { Some((override_cfg, active_reason)) => { Some((override_cfg, active_reason)) - } - // Otherwise, there is no override. - else { + } _ => { None - }; + }}}}}}; Ok(override_config) } @@ -753,15 +745,15 @@ impl<'a> Cfg<'a> { force_non_host: bool, verbose: bool, ) -> Result<(LocalToolchainName, ActiveReason)> { - if let Some((override_config, reason)) = self.find_override_config()? { + match self.find_override_config()? { Some((override_config, reason)) => { let toolchain = override_config.clone().into_local_toolchain_name(); - if let OverrideCfg::Official { + match override_config + { OverrideCfg::Official { toolchain, components, targets, profile, - } = override_config - { + } => { self.ensure_installed( &toolchain, components, @@ -771,22 +763,22 @@ impl<'a> Cfg<'a> { verbose, ) .await?; - } else { + } _ => { Toolchain::with_reason(self, toolchain.clone(), &reason)?; - } + }} Ok((toolchain, reason)) - } else if let Some(toolchain) = self.get_default()? { + } _ => { match self.get_default()? { Some(toolchain) => { let reason = ActiveReason::Default; - if let ToolchainName::Official(desc) = &toolchain { + match &toolchain { ToolchainName::Official(desc) => { self.ensure_installed(desc, vec![], vec![], None, force_non_host, verbose) .await?; - } else { + } _ => { Toolchain::with_reason(self, toolchain.clone().into(), &reason)?; - } + }} Ok((toolchain.into(), reason)) - } else { + } _ => { Err(no_toolchain_error(self.process)) - } + }}}} } // Returns a Toolchain matching the given ToolchainDesc, installing it and @@ -894,11 +886,11 @@ impl<'a> Cfg<'a> { self.list_toolchains()? .into_iter() .filter_map(|t| { - if let ToolchainName::Official(desc) = t { + match t { ToolchainName::Official(desc) => { Some(desc) - } else { + } _ => { None - } + }} }) .filter(ToolchainDesc::is_tracking) .map(|n| { diff --git a/src/diskio/immediate.rs b/src/diskio/immediate.rs index e3fba7f2fc..d07bcaf904 100644 --- a/src/diskio/immediate.rs +++ b/src/diskio/immediate.rs @@ -38,7 +38,7 @@ impl ImmediateUnpacker { fn deque(&self) -> Box> { let mut guard = self.incremental_state.lock().unwrap(); // incremental file in progress - if let Some(ref mut state) = *guard { + match *guard { Some(ref mut state) => { // Case 1: pending errors if state.finished { let mut item = state.item.take().unwrap(); @@ -59,9 +59,9 @@ impl ImmediateUnpacker { completed_chunks.append(&mut state.completed_chunks); Box::new(completed_chunks.into_iter().map(CompletedIo::Chunk)) } - } else { + } _ => { Box::new(None.into_iter()) - } + }} } } @@ -187,13 +187,13 @@ impl IncrementalFileWriter { Ok(v) => v, Err(e) => { let mut state = self.state.lock().unwrap(); - if let Some(ref mut state) = *state { + match *state { Some(ref mut state) => { state.err.replace(Err(e)); state.finished = true; false - } else { + } _ => { false - } + }} } } } @@ -203,7 +203,7 @@ impl IncrementalFileWriter { let Some(ref mut state) = *state else { unreachable!() }; - if let Some(ref mut file) = self.file.as_mut() { + match self.file.as_mut() { Some(ref mut file) => { // Length 0 vector is used for clean EOF signalling. if chunk.is_empty() { trace_scoped!("close", "name:": self.path_display); @@ -216,8 +216,8 @@ impl IncrementalFileWriter { state.completed_chunks.push(chunk.len()); } Ok(true) - } else { + } _ => { Ok(false) - } + }} } } diff --git a/src/dist/download.rs b/src/dist/download.rs index 47c924b7bf..87a3ea4556 100644 --- a/src/dist/download.rs +++ b/src/dist/download.rs @@ -162,14 +162,14 @@ impl<'a> DownloadCfg<'a> { if let Some(hash_file) = update_hash { if utils::is_file(hash_file) { - if let Ok(contents) = utils::read_file("update hash", hash_file) { + match utils::read_file("update hash", hash_file) { Ok(contents) => { if contents == partial_hash { // Skip download, update hash matches return Ok(None); } - } else { + } _ => { (self.notify_handler)(Notification::CantReadUpdateHash(hash_file)); - } + }} } else { (self.notify_handler)(Notification::NoUpdateHash(hash_file)); } diff --git a/src/dist/manifestation.rs b/src/dist/manifestation.rs index 210f8e98ac..73f8dd245c 100644 --- a/src/dist/manifestation.rs +++ b/src/dist/manifestation.rs @@ -369,15 +369,15 @@ impl Manifestation { // component name plus the target triple. let name = component.name_in_manifest(); let short_name = component.short_name_in_manifest(); - if let Some(c) = self.installation.find(&name)? { + match self.installation.find(&name)? { Some(c) => { tx = c.uninstall(tx, process)?; - } else if let Some(c) = self.installation.find(short_name)? { + } _ => { match self.installation.find(short_name)? { Some(c) => { tx = c.uninstall(tx, process)?; - } else { + } _ => { notify_handler(Notification::MissingInstalledComponent( &component.short_name(manifest), )); - } + }}}} Ok(tx) } diff --git a/src/dist/mod.rs b/src/dist/mod.rs index e65ca52cf3..347991f8bd 100644 --- a/src/dist/mod.rs +++ b/src/dist/mod.rs @@ -971,13 +971,13 @@ pub(crate) async fn update_from_dist( if try_next < last_manifest { // Wouldn't be an update if we go further back than the user's current nightly. - if let Some(e) = first_err { + match first_err { Some(e) => { break Err(e); - } else { + } _ => { // In this case, all newer nightlies are missing, which means there are no // updates, so the user is already at the latest nightly. break Ok(None); - } + }} } toolchain.date = Some(try_next.format("%Y-%m-%d").to_string()); diff --git a/src/toolchain.rs b/src/toolchain.rs index 2f0bde1e87..13295fa41f 100644 --- a/src/toolchain.rs +++ b/src/toolchain.rs @@ -353,16 +353,16 @@ impl<'a> Toolchain<'a> { #[cfg_attr(feature="otel", tracing::instrument(err,fields(binary, recursion=self.cfg.process.var("RUST_RECURSION_COUNT").ok())))] fn create_command + Debug>(&self, binary: T) -> Result { // Create the path to this binary within the current toolchain sysroot - let binary = if let Some(binary_str) = binary.as_ref().to_str() { + let binary = match binary.as_ref().to_str() { Some(binary_str) => { if binary_str.to_lowercase().ends_with(EXE_SUFFIX) { binary.as_ref().to_owned() } else { OsString::from(format!("{binary_str}{EXE_SUFFIX}")) } - } else { + } _ => { // Very weird case. Non-unicode command. binary.as_ref().to_owned() - }; + }}; let bin_path = self.path.join("bin").join(&binary); let path = if utils::is_file(&bin_path) { diff --git a/src/toolchain/distributable.rs b/src/toolchain/distributable.rs index 4d51a2fee8..b5bc2c569f 100644 --- a/src/toolchain/distributable.rs +++ b/src/toolchain/distributable.rs @@ -231,7 +231,7 @@ impl<'a> DistributableToolchain<'a> { const MAX_DISTANCE: usize = 3; let components = manifest.query_components(&self.desc, config); - if let Ok(components) = components { + match components { Ok(components) => { let short_name_distance = components .iter() .filter(|c| !only_installed || c.installed) @@ -296,9 +296,9 @@ impl<'a> DistributableToolchain<'a> { } else { Some(closest_match) } - } else { + } _ => { None - } + }} } #[tracing::instrument(level = "trace", skip_all)]