Split TorClientConfig out of ArtiConfig, and Resolvable trait

This gets rid of `#[serde(flatten)]` which prevents serde_ignored (and
other kinds of introspection) from working properly.

The price is now that the toplevel has to deal with two configuration
objects.

The Resolvable trait is overkill right now, but is going to do More
Things in a moment.  In particular, we need the impl on tuples, so
that the whole config can be processed in one go.

crates/arti-bench/src/main.rs

@@ -36,7 +36,7 @@
 #![allow(clippy::unwrap_used)]
 
 use anyhow::{anyhow, Result};
-use arti::cfg::ArtiConfig;
+use arti::cfg::ArtiCombinedConfig;
 use arti_client::{IsolationToken, TorAddr, TorClient, TorClientConfig};
 use clap::{App, Arg};
 use futures::StreamExt;
@@ -367,8 +367,7 @@ fn main() -> Result<()> {
     config_sources.set_mistrust(mistrust);
 
     let cfg = config_sources.load()?;
-    let config: ArtiConfig = cfg.try_into()?;
-    let tcc = config.tor_client_config()?;
+    let (_config, tcc) = tor_config::resolve::<ArtiCombinedConfig>(cfg)?;
     info!("Binding local TCP listener...");
     let listener = TcpListener::bind("0.0.0.0:0")?;
     let local_addr = listener.local_addr()?;

crates/arti-client/src/config.rs

@@ -288,6 +288,10 @@ pub struct TorClientConfig {
 }
 impl_standard_builder! { TorClientConfig }
 
+impl tor_config::load::TopLevel for TorClientConfig {
+    type Builder = TorClientConfigBuilder;
+}
+
 /// Helper to convert convert_override_net_params
 fn convert_override_net_params(
     builder: &HashMap<String, i32>,

crates/arti-testing/src/main.rs

@@ -84,7 +84,7 @@ mod dirfilter;
 mod rt;
 mod traces;
 
-use arti::ArtiConfig;
+use arti::ArtiCombinedConfig;
 use arti_client::TorClient;
 use futures::task::SpawnExt;
 use rt::badtcp::BrokenTcpProvider;
@@ -222,9 +222,9 @@ struct Job {
 impl Job {
     /// Make a new unbootstrapped client for this job.
     fn make_client<R: Runtime>(&self, runtime: R) -> Result<TorClient<R>> {
-        let config: ArtiConfig = self.config.load()?.try_into()?;
+        let (_arti, tcc) = tor_config::resolve::<ArtiCombinedConfig>(self.config.load()?)?;
         let client = TorClient::with_runtime(runtime)
-            .config(config.tor_client_config()?)
+            .config(tcc)
             .dirfilter(self.dir_filter.clone())
             .create_unbootstrapped()?;
         Ok(client)

crates/arti/src/cfg.rs

@@ -5,7 +5,6 @@
 use derive_builder::Builder;
 use serde::{Deserialize, Serialize};
 
-use arti_client::config::TorClientConfigBuilder;
 use arti_client::TorClientConfig;
 use tor_config::{impl_standard_builder, ConfigBuildError};
 
@@ -111,37 +110,17 @@ pub struct ArtiConfig {
     #[builder(sub_builder)]
     #[builder_field_attr(serde(default))]
     pub(crate) system: SystemConfig,
-
-    /// Configuration of the actual Tor client
-    #[builder(sub_builder)]
-    #[builder_field_attr(serde(flatten))]
-    pub(crate) tor: TorClientConfig,
 }
 impl_standard_builder! { ArtiConfig }
 
-impl TryFrom<config::Config> for ArtiConfig {
-    type Error = config::ConfigError;
-    fn try_from(cfg: config::Config) -> Result<ArtiConfig, Self::Error> {
-        let builder: ArtiConfigBuilder = cfg.try_deserialize()?;
-        builder
-            .build()
-            .map_err(|e| config::ConfigError::Foreign(Box::new(e)))
-    }
+impl tor_config::load::TopLevel for ArtiConfig {
+    type Builder = ArtiConfigBuilder;
 }
 
-// This handwritten impl ought not to exist, but it is needed until #374 is done.
-impl From<ArtiConfigBuilder> for TorClientConfigBuilder {
-    fn from(cfg: ArtiConfigBuilder) -> TorClientConfigBuilder {
-        cfg.tor
-    }
-}
+/// Convenience alias
+pub type ArtiCombinedConfig = (ArtiConfig, TorClientConfig);
 
 impl ArtiConfig {
-    /// Construct a [`TorClientConfig`] based on this configuration.
-    pub fn tor_client_config(&self) -> Result<TorClientConfig, ConfigBuildError> {
-        Ok(self.tor.clone())
-    }
-
     /// Return the [`ApplicationConfig`] for this configuration.
     pub fn application(&self) -> &ApplicationConfig {
         &self.application
@@ -163,6 +142,7 @@ mod test {
     #![allow(clippy::unwrap_used)]
 
     use arti_client::config::dir;
+    use arti_client::config::TorClientConfigBuilder;
     use regex::Regex;
     use std::time::Duration;
 
@@ -179,7 +159,7 @@ mod test {
     #[test]
     fn default_config() {
         let empty_config = config::Config::builder().build().unwrap();
-        let empty_config: ArtiConfig = empty_config.try_into().unwrap();
+        let empty_config: ArtiCombinedConfig = tor_config::resolve(empty_config).unwrap();
 
         let example = uncomment_example_settings(ARTI_EXAMPLE_CONFIG);
         let cfg = config::Config::builder()
@@ -199,19 +179,18 @@ mod test {
         // Also we should ideally test that every setting from the config appears here in
         // the file.  Possibly that could be done with some kind of stunt Deserializer,
         // but it's not trivial.
-        let parsed: ArtiConfig = cfg.try_into().unwrap();
-        let default = ArtiConfig::default();
+        let parsed: ArtiCombinedConfig = tor_config::resolve(cfg).unwrap();
+
+        let default = (ArtiConfig::default(), TorClientConfig::default());
         assert_eq!(&parsed, &default);
         assert_eq!(&parsed, &empty_config);
 
-        let built_default = ArtiConfigBuilder::default().build().unwrap();
+        let built_default = (
+            ArtiConfigBuilder::default().build().unwrap(),
+            TorClientConfigBuilder::default().build().unwrap(),
+        );
         assert_eq!(&parsed, &built_default);
         assert_eq!(&default, &built_default);
-
-        // Make sure that the client configuration this gives us is the default one.
-        let client_config = parsed.tor_client_config().unwrap();
-        let dflt_client_config = TorClientConfig::default();
-        assert_eq!(&client_config, &dflt_client_config);
     }
 
     #[test]
@@ -231,11 +210,11 @@ mod test {
             .push("127.0.0.7:7".parse().unwrap());
 
         let mut bld = ArtiConfig::builder();
+        let mut bld_tor = TorClientConfig::builder();
 
         bld.proxy().socks_port(Some(9999));
         bld.logging().console("warn");
 
-        let bld_tor = bld.tor();
         bld_tor.tor_network().set_authorities(vec![auth]);
         bld_tor.tor_network().set_fallback_caches(vec![fallback]);
         bld_tor

crates/arti/src/lib.rs

@@ -125,8 +125,8 @@ pub mod watch_cfg;
 use std::fmt::Write;
 
 pub use cfg::{
-    ApplicationConfig, ApplicationConfigBuilder, ArtiConfig, ArtiConfigBuilder, ProxyConfig,
-    ProxyConfigBuilder, SystemConfig, SystemConfigBuilder, ARTI_EXAMPLE_CONFIG,
+    ApplicationConfig, ApplicationConfigBuilder, ArtiCombinedConfig, ArtiConfig, ArtiConfigBuilder,
+    ProxyConfig, ProxyConfigBuilder, SystemConfig, SystemConfigBuilder, ARTI_EXAMPLE_CONFIG,
 };
 pub use logging::{LoggingConfig, LoggingConfigBuilder};
 
@@ -371,13 +371,13 @@ pub fn main_main() -> Result<()> {
     };
 
     let cfg = cfg_sources.load()?;
-
-    let config: ArtiConfig = cfg.try_into().context("read configuration")?;
+    let (config, client_config) =
+        tor_config::resolve::<ArtiCombinedConfig>(cfg).context("read configuration")?;
 
     let log_mistrust = if fs_mistrust_disabled {
         fs_mistrust::Mistrust::new_dangerously_trust_everyone()
     } else {
-        config.tor.fs_mistrust().clone()
+        client_config.fs_mistrust().clone()
     };
     let _log_guards = logging::setup_logging(
         config.logging(),
@@ -401,8 +401,6 @@ pub fn main_main() -> Result<()> {
             (None, None) => 0,
         };
 
-        let client_config = config.tor_client_config()?;
-
         info!(
             "Starting Arti {} in SOCKS proxy mode on port {}...",
             env!("CARGO_PKG_VERSION"),

crates/arti/src/watch_cfg.rs

@@ -12,7 +12,7 @@ use tor_config::ConfigurationSources;
 use tor_rtcompat::Runtime;
 use tracing::{debug, info, warn};
 
-use crate::ArtiConfig;
+use crate::{ArtiCombinedConfig, ArtiConfig};
 
 /// How long (worst case) should we take to learn about configuration changes?
 const POLL_INTERVAL: Duration = Duration::from_secs(10);
@@ -85,14 +85,13 @@ fn reconfigure<R: Runtime>(
     client: &TorClient<R>,
 ) -> anyhow::Result<bool> {
     let config = sources.load()?;
-    let config: ArtiConfig = config.try_into()?;
+    let (config, client_config) = tor_config::resolve::<ArtiCombinedConfig>(config)?;
     if config.proxy() != original.proxy() {
         warn!("Can't (yet) reconfigure proxy settings while arti is running.");
     }
     if config.logging() != original.logging() {
         warn!("Can't (yet) reconfigure logging settings while arti is running.");
     }
-    let client_config = config.tor_client_config()?;
     client.reconfigure(&client_config, Reconfigure::WarnOnFailures)?;
 
     if !config.application().watch_configuration {

crates/tor-config/src/lib.rs

@@ -49,6 +49,7 @@
 pub mod cmdline;
 mod err;
 pub mod list_builder;
+pub mod load;
 mod mut_cfg;
 mod path;
 pub mod sources;
@@ -57,6 +58,7 @@ pub use cmdline::CmdLine;
 pub use config as config_crate;
 pub use educe;
 pub use err::{ConfigBuildError, ReconfigureError};
+pub use load::resolve;
 pub use mut_cfg::MutCfg;
 pub use paste::paste;
 pub use path::{CfgPath, CfgPathError};

crates/tor-config/src/load.rs

@@ -27,3 +27,88 @@ pub trait Builder {
     /// Often shadows an inherent `build` method
     fn build(&self) -> Result<Self::Built, ConfigBuildError>;
 }
+
+/// Collection of configuration settings that can be deseriealized and then built
+///
+/// *Do not implement directly.*
+/// Instead, implement [`TopLevel`]: this engages the blanket impl
+/// for (loosely) `TopLevel + Builder`.
+///
+/// Each `Resolvable` corresponds to one or more configuration consumers.
+///
+/// Ultimately, one `Resolvable` for all the configuration consumers in an entire
+/// program will be resolved from a single configuration tree (usually parsed from TOML).
+///
+/// Multiple config collections can be resolved from the same configuartion,
+/// via the implementation of `Resolvable` on tuples of `Resolvable`s.
+/// Use this rather than `#[serde(flatten)]`; the latter prevents useful introspection
+/// (necessary for reporting ignored configuration keys, and testing).
+///
+/// (The `resolve` method will be called only from within the `tor_config::load` module.)
+pub trait Resolvable: Sized {
+    /// Deserialize and build from a configuration
+    fn resolve(input: &mut ResolveContext) -> Result<Self, ConfigResolveError>;
+}
+
+/// Top-level configuration struct, made from a deserializable builder
+///
+/// One configuration consumer's configuration settings.
+///
+/// Implementing this trait only for top-level configurations,
+/// which are to be parsed at the root level of a (TOML) config file taxonomy.
+pub trait TopLevel {
+    /// The `Builder` which can be used to make a `Self`
+    ///
+    /// Should satisfy `&'_ Self::Builder: Builder<Built=Self>`
+    type Builder: DeserializeOwned;
+}
+
+/// `define_for_tuples!{ A B - C D.. }`
+///
+/// expands to
+///  1. `define_for_tuples!{ A B - }`: defines for tuple `(A,B,)`
+///  2. `define_for_tuples!{ A B C - D.. }`: recurses to geenrate longer tuples
+macro_rules! define_for_tuples {
+    { $( $A:ident )* - $B:ident $( $C:ident )* } => {
+        define_for_tuples!{ $($A)* - }
+        define_for_tuples!{ $($A)* $B - $($C)* }
+    };
+    { $( $A:ident )* - } => {
+        impl < $($A,)* > Resolvable for ( $($A,)* )
+        where $( $A: Resolvable, )*
+        {
+            fn resolve(cfg: &mut ResolveContext) -> Result<Self, ConfigResolveError> {
+                Ok(( $( $A::resolve(cfg)?, )* ))
+            }
+        }
+
+    };
+}
+define_for_tuples! { A - B C D E }
+
+/// Config resolultion context, not used outside `tor_config::load`
+///
+/// This is public only because it appears in the [`Resolvable`] trait.
+/// You don't want to try to obtain one.
+pub struct ResolveContext(config::Config);
+
+/// Deserialize and build overall configuration from config sources
+pub fn resolve<T>(input: config::Config) -> Result<T, ConfigResolveError>
+where
+    T: Resolvable,
+{
+    let mut lc = ResolveContext(input);
+    Resolvable::resolve(&mut lc)
+}
+
+impl<T> Resolvable for T
+where
+    T: TopLevel,
+    T::Builder: Builder<Built = Self>,
+{
+    fn resolve(input: &mut ResolveContext) -> Result<T, ConfigResolveError> {
+        let builder: T::Builder = input.0.clone().try_deserialize()?;
+        let built = (&builder).build()?;
+        Ok(built)
+    }
+}