Merge branch 'help_msg_on_backslash' into 'main'

Improve error from bad escapes in a toml config.

Closes #549

See merge request tpo/core/arti!695

crates/tor-config/semver.md

@@ -8,3 +8,5 @@ BREAKING: load::resolve_ignore_unrecognized renamed resolve_ignore_warnings
 BREAKING: load::resolve_return_unrecognized replaced with resolve_return_results
 BREAKING: load::UnrecognizedKey renamed to DisfavouredKey
 ADDED: Support for tracking deprecated config keys
+BREAKING: Return different error type on load().
+

crates/tor-config/src/err.rs

@@ -1,5 +1,7 @@
 //! Declare error types.
 
+use std::sync::Arc;
+
 use tor_error::{ErrorKind, HasKind};
 
 /// An error related to an option passed to Arti via a configuration
@@ -102,6 +104,40 @@ impl HasKind for ReconfigureError {
     }
 }
 
+/// Wrapper for [`config::ConfigError`] with a more helpful error message.
+#[derive(Debug, Clone)]
+pub struct ConfigError(Arc<config::ConfigError>);
+
+impl From<config::ConfigError> for ConfigError {
+    fn from(err: config::ConfigError) -> Self {
+        ConfigError(Arc::new(err))
+    }
+}
+
+impl std::fmt::Display for ConfigError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        let s = self.0.to_string();
+        write!(f, "{}", s)?;
+        if s.contains("invalid escape") || s.contains("invalid hex escape") {
+            write!(f, "   (If you wanted to include a literal \\ character, you need to escape it by writing two in a row: \\\\)")?;
+        }
+        Ok(())
+    }
+}
+
+impl std::error::Error for ConfigError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        Some(&self.0)
+    }
+}
+
+impl ConfigError {
+    /// Return the inner [`config::ConfigError`] that this is wrapping.
+    pub fn inner(&self) -> &config::ConfigError {
+        self.0.as_ref()
+    }
+}
+
 #[cfg(test)]
 mod test {
     #![allow(clippy::unwrap_used)]

crates/tor-config/src/lib.rs

@@ -105,7 +105,7 @@ pub mod sources;
 pub use cmdline::CmdLine;
 pub use config as config_crate;
 pub use educe;
-pub use err::{ConfigBuildError, ReconfigureError};
+pub use err::{ConfigBuildError, ConfigError, ReconfigureError};
 pub use itertools::Itertools;
 pub use load::{resolve, resolve_ignore_warnings, resolve_return_results};
 pub use misc::*;

crates/tor-config/src/load.rs

@@ -104,13 +104,19 @@ use crate::ConfigBuildError;
 pub enum ConfigResolveError {
     /// Deserialize failed
     #[error("Config contents not as expected")]
-    Deserialize(#[from] config::ConfigError),
+    Deserialize(#[from] crate::ConfigError),
 
     /// Build failed
     #[error("Config semantically incorrect")]
     Build(#[from] ConfigBuildError),
 }
 
+impl From<config::ConfigError> for ConfigResolveError {
+    fn from(err: config::ConfigError) -> Self {
+        crate::ConfigError::from(err).into()
+    }
+}
+
 /// A type that can be built from a builder via a build method
 pub trait Builder {
     /// The type that this builder constructs

crates/tor-config/src/sources.rs

@@ -25,10 +25,9 @@
 use std::ffi::OsString;
 use std::{fs, io};
 
+use crate::err::ConfigError;
 use crate::CmdLine;
 
-use config::ConfigError;
-
 /// The synchronous configuration builder type we use.
 ///
 /// (This is a type alias that config should really provide.)
@@ -242,16 +241,13 @@ impl ConfigurationSources {
             // or Ok(()) if we should ignore the error and skip the file.
             let handle_io_error = |e: io::Error| {
                 if e.kind() == io::ErrorKind::NotFound && !required {
-                    Ok(())
+                    Result::<_, crate::ConfigError>::Ok(())
                 } else {
-                    Err(ConfigError::Foreign(
-                        anyhow::anyhow!(format!(
-                            "unable to access config path: {:?}: {}",
-                            &source.as_path(),
-                            e
-                        ))
-                        .into(),
-                    ))
+                    Err(foreign_err(anyhow::anyhow!(format!(
+                        "unable to access config path: {:?}: {}",
+                        &source.as_path(),
+                        e
+                    ))))
                 }
             };
 
@@ -341,7 +337,7 @@ impl FoundConfigFiles<'_> {
             {
                 Ok(()) => {}
                 Err(fs_mistrust::Error::NotFound(_)) if !required => {}
-                Err(e) => return Err(ConfigError::Foreign(e.into())),
+                Err(e) => return Err(foreign_err(e)),
             }
 
             // Not going to use File::with_name here, since it doesn't
@@ -363,7 +359,7 @@ impl FoundConfigFiles<'_> {
     pub fn load(self) -> Result<config::Config, ConfigError> {
         let mut builder = config::Config::builder();
         builder = self.add_sources(builder)?;
-        builder.build()
+        Ok(builder.build()?)
     }
 }
 
@@ -393,6 +389,14 @@ fn is_syntactically_directory(p: &Path) -> bool {
     }
 }
 
+/// Convert an error `E` into a [`ConfigError`](crate::ConfigErr).
+fn foreign_err<E>(err: E) -> crate::ConfigError
+where
+    E: Into<Box<dyn std::error::Error + Send + Sync>>,
+{
+    crate::ConfigError::from(config::ConfigError::Foreign(err.into()))
+}
+
 #[cfg(test)]
 mod test {
     #![allow(clippy::unwrap_used)]
@@ -429,7 +433,7 @@ friends = 4242
     fn load_nodefaults<P: AsRef<Path>>(
         files: &[(P, MustRead)],
         opts: &[String],
-    ) -> Result<config::Config, config::ConfigError> {
+    ) -> Result<config::Config, crate::ConfigError> {
         sources_nodefaults(files, opts).load()
     }