fix(config): improve error handling in Cfg::list_toolchains()

src/cli/common.rs

@@ -321,7 +321,7 @@ pub(crate) async fn list_toolchains(
     verbose: bool,
     quiet: bool,
 ) -> anyhow::Result<ExitCode> {
-    let toolchains = cfg.list_toolchains()?;
+    let mut toolchains = cfg.list_toolchains(quiet)?;
     if toolchains.is_empty() {
         writeln!(cfg.process.stdout().lock(), "no installed toolchains")?;
     } else {
@@ -335,6 +335,7 @@ pub(crate) async fn list_toolchains(
                 None
             };
 
+        toolchains.sort();
         for toolchain in toolchains {
             let is_default_toolchain = default_toolchain_name.as_ref() == Some(&toolchain);
             let is_active_toolchain = active_toolchain_name.as_ref() == Some(&toolchain);

src/cli/rustup_mode.rs

@@ -890,7 +890,7 @@ pub async fn main(
         }
     }?;
 
-    if should_warn && cfg.list_toolchains()?.is_empty() && cfg.get_default()?.is_none() {
+    if should_warn && cfg.list_toolchains(true)?.is_empty() && cfg.get_default()?.is_none() {
         warn!("no toolchain installed and no default toolchain set\n{DEFAULT_STABLE_HINT}");
     }
 
@@ -902,7 +902,8 @@ pub async fn main(
 }
 
 fn completion_command(cfg: &Cfg<'_>) -> clap::Command {
-    let toolchains = cfg.list_toolchains().unwrap_or_default();
+    let mut toolchains = cfg.list_toolchains(true).unwrap_or_default();
+    toolchains.sort();
     Rustup::command().mut_arg("+toolchain", move |arg| {
         arg.add(ArgValueCompleter::new(move |current: &OsStr| {
             let Some(prefix) = current.to_str() else {
@@ -1280,7 +1281,8 @@ async fn show(cfg: &Cfg<'_>, verbose: bool) -> anyhow::Result<ExitCode> {
         writeln!(t)?;
     }
 
-    let installed_toolchains = cfg.list_toolchains()?;
+    let mut installed_toolchains = cfg.list_toolchains(cfg.quiet)?;
+    installed_toolchains.sort();
     let active_toolchain_and_source: Option<(ToolchainName, ActiveSource)> =
         if let Ok(Some((LocalToolchainName::Named(toolchain_name), source))) =
             cfg.maybe_ensure_active_toolchain(None).await

src/cli/self_update.rs

@@ -972,7 +972,7 @@ pub(crate) fn uninstall(
     }
 
     info!("removing toolchains");
-    for toolchain in cfg.list_toolchains()? {
+    for toolchain in cfg.list_toolchains(true)? {
         Toolchain::ensure_removed(cfg, toolchain.into())?;
     }
 

src/config.rs

@@ -1,6 +1,5 @@
 use std::{
     fmt::{self, Debug, Display},
-    io,
     io::Write,
     ops::Deref,
     path::{Path, PathBuf},
@@ -755,7 +754,11 @@ impl<'a> Cfg<'a> {
                         // disabling this and backing out https://github.com/rust-lang/rustup/pull/2141 (but provide
                         // the base name in the error to help users)
                         let resolved_name = &ToolchainName::from_str(toolchain_name_str)?;
-                        if !self.list_toolchains()?.iter().any(|s| s == resolved_name) {
+                        if !self
+                            .list_toolchains(true)?
+                            .iter()
+                            .any(|s| s == resolved_name)
+                        {
                             return Err(anyhow!(format!("target tuple in channel name '{name}'")));
                         }
                     }
@@ -976,51 +979,75 @@ impl<'a> Cfg<'a> {
         Ok(Some(ResolvableToolchainName::from_str(toolchain)?))
     }
 
-    /// List all the installed toolchains: that is paths in the toolchain dir
-    /// that are:
-    /// - not files
-    /// - named with a valid resolved toolchain name
-    /// Currently no notification of incorrect names or entry type is done.
+    /// Lists all the installed toolchains.
+    ///
+    /// # Note
+    ///
+    /// This function returns every valid toolchain name that has a corresponding non-file entry in
+    /// the toolchains directory. These names may be returned in any order.
     #[tracing::instrument(level = "trace", skip_all)]
-    pub(crate) fn list_toolchains(&self) -> anyhow::Result<Vec<ToolchainName>> {
-        if utils::is_directory(&self.toolchains_dir) {
-            let mut toolchains: Vec<_> = utils::read_dir("toolchains", &self.toolchains_dir)?
-                // TODO: this discards errors reading the directory, is that
-                // correct? could we get a short-read and report less toolchains
-                // than exist?
-                .filter_map(io::Result::ok)
-                .filter(|e| e.file_type().map(|f| !f.is_file()).unwrap_or(false))
-                .filter_map(|e| e.file_name().into_string().ok())
-                .filter_map(|n| ToolchainName::from_str(&n).ok())
-                .collect();
-
-            toolchains.sort();
-
-            Ok(toolchains)
-        } else {
-            Ok(Vec::new())
+    pub(crate) fn list_toolchains(&self, quiet: bool) -> anyhow::Result<Vec<ToolchainName>> {
+        if !utils::is_directory(&self.toolchains_dir) {
+            return Ok(vec![]);
         }
+
+        let mut toolchains = vec![];
+        for entry in utils::read_dir("toolchains", &self.toolchains_dir)? {
+            let entry = match entry {
+                Ok(entry) => entry,
+                Err(e) => {
+                    if !quiet {
+                        warn!("failed to read toolchain FS entry: {e}");
+                    }
+                    continue;
+                }
+            };
+            if !entry.file_type().is_ok_and(|t| !t.is_file()) {
+                continue;
+            }
+            let tc = match entry.file_name().into_string() {
+                Ok(tc) => tc,
+                Err(e) => {
+                    if !quiet {
+                        warn!(
+                            "ignoring invalid potential toolchain name `{}`",
+                            e.display()
+                        );
+                    }
+                    continue;
+                }
+            };
+            match ToolchainName::from_str(&tc) {
+                Ok(tc) => toolchains.push(tc),
+                Err(e) => {
+                    if !quiet {
+                        warn!("ignoring invalid toolchain: {e}")
+                    }
+                }
+            }
+        }
+
+        Ok(toolchains)
     }
 
     pub(crate) fn list_channels(
         &self,
     ) -> anyhow::Result<Vec<(ToolchainDesc, DistributableToolchain<'_>)>> {
-        self.list_toolchains()?
+        let mut channels = self
+            .list_toolchains(true)?
             .into_iter()
-            .filter_map(|t| {
-                if let ToolchainName::Official(desc) = t {
-                    Some(desc)
-                } else {
-                    None
+            .filter_map(|t| match t {
+                ToolchainName::Official(n) if n.is_tracking() => {
+                    Some(DistributableToolchain::new(self, n.clone()).map(|t| (n, t)))
                 }
+                _ => None,
             })
-            .filter(ToolchainDesc::is_tracking)
-            .map(|n| {
-                DistributableToolchain::new(self, n.clone())
-                    .map_err(Into::into)
-                    .map(|t| (n.clone(), t))
-            })
-            .collect::<anyhow::Result<Vec<_>>>()
+            .collect::<Result<Vec<_>, _>>()?;
+
+        // HACK: `.sort_by_key()` is impossible here without cloning.
+        // See: <https://users.rust-lang.org/t/cannot-call-vec-sort-by-key-without-cloning-data/60455/4>
+        channels.sort_by(|(n, _), (m, _)| n.cmp(m));
+        Ok(channels)
     }
 
     /// Create an override for a toolchain

tests/suite/cli_v2.rs

@@ -285,6 +285,30 @@ nightly-[HOST_TUPLE] (active, default)
         .is_ok();
 }
 
+#[tokio::test]
+async fn list_toolchains_with_illegal_names() {
+    let cx = CliTestContext::new(Scenario::SimpleV2).await;
+    cx.config
+        .expect(["rustup", "update", "nightly"])
+        .await
+        .is_ok();
+
+    fs::create_dir(cx.config.rustupdir.join("toolchains/--illegal-name")).unwrap();
+
+    cx.config
+        .expect(["rustup", "toolchain", "list"])
+        .await
+        .with_stdout(snapbox::str![[r#"
+nightly-[HOST_TUPLE] (active, default)
+
+"#]])
+        .with_stderr(snapbox::str![[r#"
+warn: ignoring invalid toolchain: invalid toolchain name '--illegal-name'; valid toolchain names do not start with '-'
+
+"#]])
+        .is_ok();
+}
+
 #[tokio::test]
 async fn list_toolchains_with_none() {
     let cx = CliTestContext::new(Scenario::SimpleV2).await;