Plumb a FromfileExpander into option source readers (#20814)

This is done by splitting each of the structs representing
option sources (Args, Env, Config) into two:

The existing {Args/Env/Config} is what the user creates
and passes in to OptionParser::new. These handle
loading their data from the CLI/env vars/config files.

The actual reading of option values is now handled
by three new structs: {Args/Env/Config}Reader.
These structs have a FromfileExpander, but don't
use it yet (that will be taken care of in a followup).

src/rust/engine/options/src/args.rs

@@ -5,7 +5,7 @@ use std::env;
 
 use super::id::{is_valid_scope_name, NameTransform, OptionId, Scope};
 use super::{DictEdit, OptionsSource};
-use crate::fromfile::{expand, expand_to_dict, expand_to_list};
+use crate::fromfile::{expand, expand_to_dict, expand_to_list, FromfileExpander};
 use crate::parse::{ParseError, Parseable};
 use crate::ListEdit;
 use core::iter::once;
@@ -153,16 +153,33 @@ impl Args {
         args.next(); // Consume the process name (argv[0]).
         Self::new(env::args().collect::<Vec<_>>())
     }
+}
+
+pub(crate) struct ArgsReader {
+    args: Args,
+    #[allow(dead_code)]
+    fromfile_expander: FromfileExpander,
+}
+
+impl ArgsReader {
+    pub fn new(args: Args, fromfile_expander: FromfileExpander) -> Self {
+        Self {
+            args,
+            fromfile_expander,
+        }
+    }
 
+    #[allow(dead_code)]
     pub fn get_passthrough_args(&self) -> Option<Vec<&str>> {
-        self.passthrough_args
+        self.args
+            .passthrough_args
             .as_ref()
             .map(|v| Vec::from_iter(v.iter().map(String::as_str)))
     }
 
     fn get_list<T: Parseable>(&self, id: &OptionId) -> Result<Option<Vec<ListEdit<T>>>, String> {
         let mut edits = vec![];
-        for arg in &self.args {
+        for arg in &self.args.args {
             if arg.matches(id) {
                 let value = arg.value.as_ref().ok_or_else(|| {
                     format!("Expected list option {} to have a value.", self.display(id))
@@ -182,7 +199,7 @@ impl Args {
     }
 }
 
-impl OptionsSource for Args {
+impl OptionsSource for ArgsReader {
     fn display(&self, id: &OptionId) -> String {
         format!(
             "--{}{}",
@@ -197,7 +214,7 @@ impl OptionsSource for Args {
     fn get_string(&self, id: &OptionId) -> Result<Option<String>, String> {
         // We iterate in reverse so that the rightmost arg wins in case an option
         // is specified multiple times.
-        for arg in self.args.iter().rev() {
+        for arg in self.args.args.iter().rev() {
             if arg.matches(id) {
                 return expand(arg.value.clone().ok_or_else(|| {
                     format!("Expected list option {} to have a value.", self.display(id))
@@ -211,7 +228,7 @@ impl OptionsSource for Args {
     fn get_bool(&self, id: &OptionId) -> Result<Option<bool>, String> {
         // We iterate in reverse so that the rightmost arg wins in case an option
         // is specified multiple times.
-        for arg in self.args.iter().rev() {
+        for arg in self.args.args.iter().rev() {
             if arg.matches(id) {
                 return arg.to_bool().map_err(|e| e.render(&arg.flag));
             } else if arg.matches_negation(id) {
@@ -242,7 +259,7 @@ impl OptionsSource for Args {
 
     fn get_dict(&self, id: &OptionId) -> Result<Option<Vec<DictEdit>>, String> {
         let mut edits = vec![];
-        for arg in self.args.iter() {
+        for arg in self.args.args.iter() {
             if arg.matches(id) {
                 let value = arg.value.clone().ok_or_else(|| {
                     format!("Expected dict option {} to have a value.", self.display(id))

src/rust/engine/options/src/args_tests.rs

@@ -4,18 +4,26 @@
 use core::fmt::Debug;
 use maplit::hashmap;
 
-use crate::args::Args;
+use crate::args::{Args, ArgsReader};
 use crate::fromfile::test_util::write_fromfile;
+use crate::fromfile::FromfileExpander;
 use crate::{option_id, DictEdit, DictEditAction, Val};
 use crate::{ListEdit, ListEditAction, OptionId, OptionsSource};
 
-fn mk_args<I: IntoIterator<Item = &'static str>>(args: I) -> Args {
-    Args::new(args.into_iter().map(str::to_owned))
+fn mk_args<I>(args: I) -> ArgsReader
+where
+    I: IntoIterator,
+    I::Item: ToString,
+{
+    ArgsReader::new(
+        Args::new(args.into_iter().map(|x| x.to_string())),
+        FromfileExpander::new(),
+    )
 }
 
 #[test]
 fn test_display() {
-    let args = mk_args([]);
+    let args = mk_args::<Vec<&str>>(vec![]);
     assert_eq!("--global".to_owned(), args.display(&option_id!("global")));
     assert_eq!(
         "--scope-name".to_owned(),
@@ -194,29 +202,35 @@ fn test_scalar_fromfile() {
     fn do_test<T: PartialEq + Debug>(
         content: &str,
         expected: T,
-        getter: fn(&Args, &OptionId) -> Result<Option<T>, String>,
+        getter: fn(&ArgsReader, &OptionId) -> Result<Option<T>, String>,
         negate: bool,
     ) {
         let (_tmpdir, fromfile_path) = write_fromfile("fromfile.txt", content);
-        let args = Args::new(vec![format!(
+        let args = mk_args(vec![format!(
             "--{}foo=@{}",
             if negate { "no-" } else { "" },
             fromfile_path.display()
-        )]);
+        )
+        .as_str()]);
         let actual = getter(&args, &option_id!("foo")).unwrap().unwrap();
         assert_eq!(expected, actual)
     }
 
-    do_test("true", true, Args::get_bool, false);
-    do_test("false", false, Args::get_bool, false);
-    do_test("true", false, Args::get_bool, true);
-    do_test("false", true, Args::get_bool, true);
-    do_test("-42", -42, Args::get_int, false);
-    do_test("3.14", 3.14, Args::get_float, false);
-    do_test("EXPANDED", "EXPANDED".to_owned(), Args::get_string, false);
+    do_test("true", true, ArgsReader::get_bool, false);
+    do_test("false", false, ArgsReader::get_bool, false);
+    do_test("true", false, ArgsReader::get_bool, true);
+    do_test("false", true, ArgsReader::get_bool, true);
+    do_test("-42", -42, ArgsReader::get_int, false);
+    do_test("3.14", 3.14, ArgsReader::get_float, false);
+    do_test(
+        "EXPANDED",
+        "EXPANDED".to_owned(),
+        ArgsReader::get_string,
+        false,
+    );
 
     let (_tmpdir, fromfile_path) = write_fromfile("fromfile.txt", "BAD INT");
-    let args = Args::new(vec![format!("--foo=@{}", fromfile_path.display())]);
+    let args = mk_args(vec![format!("--foo=@{}", fromfile_path.display())]);
     assert_eq!(
         args.get_int(&option_id!("foo")).unwrap_err(),
         "Problem parsing --foo int value:\n1:BAD INT\n  ^\n\
@@ -228,7 +242,7 @@ fn test_scalar_fromfile() {
 fn test_list_fromfile() {
     fn do_test(content: &str, expected: &[ListEdit<i64>], filename: &str) {
         let (_tmpdir, fromfile_path) = write_fromfile(filename, content);
-        let args = Args::new(vec![format!("--foo=@{}", &fromfile_path.display())]);
+        let args = mk_args(vec![format!("--foo=@{}", &fromfile_path.display()).as_str()]);
         let actual = args.get_int_list(&option_id!("foo")).unwrap().unwrap();
         assert_eq!(expected.to_vec(), actual)
     }
@@ -299,9 +313,9 @@ fn test_dict_fromfile() {
         ];
 
         let (_tmpdir, fromfile_path) = write_fromfile(filename, content);
-        let args = Args::new(vec![
-            format!("--foo=@{}", &fromfile_path.display()),
-            "--foo=+{'KEY':'VALUE'}".to_string(),
+        let args = mk_args(vec![
+            &format!("--foo=@{}", &fromfile_path.display()),
+            "--foo=+{'KEY':'VALUE'}",
         ]);
         let actual = args.get_dict(&option_id!("foo")).unwrap().unwrap();
         assert_eq!(expected, actual)
@@ -335,7 +349,7 @@ fn test_dict_fromfile() {
     }];
 
     let (_tmpdir, fromfile_path) = write_fromfile("fromfile.txt", "+{'FOO':42}");
-    let args = Args::new(vec![format!("--foo=@{}", &fromfile_path.display())]);
+    let args = mk_args(vec![format!("--foo=@{}", &fromfile_path.display()).as_str()]);
     assert_eq!(
         expected_add,
         args.get_dict(&option_id!("foo")).unwrap().unwrap()
@@ -344,14 +358,14 @@ fn test_dict_fromfile() {
 
 #[test]
 fn test_nonexistent_required_fromfile() {
-    let args = Args::new(vec!["--foo=@/does/not/exist".to_string()]);
+    let args = mk_args(vec!["--foo=@/does/not/exist"]);
     let err = args.get_string(&option_id!("foo")).unwrap_err();
     assert!(err.starts_with("Problem reading /does/not/exist for --foo: No such file or directory"));
 }
 
 #[test]
 fn test_nonexistent_optional_fromfile() {
-    let args = Args::new(vec!["--foo=@?/does/not/exist".to_string()]);
+    let args = mk_args(vec!["--foo=@?/does/not/exist"]);
     assert!(args.get_string(&option_id!("foo")).unwrap().is_none());
 }
 

src/rust/engine/options/src/config.rs

@@ -11,7 +11,7 @@ use toml::value::Table;
 use toml::Value;
 
 use super::{DictEdit, DictEditAction, ListEdit, ListEditAction, OptionsSource, Val};
-use crate::fromfile::{expand, expand_to_dict, expand_to_list};
+use crate::fromfile::{expand, expand_to_dict, expand_to_list, FromfileExpander};
 use crate::id::{NameTransform, OptionId};
 use crate::parse::Parseable;
 
@@ -101,7 +101,7 @@ struct ValueConversionError<'a> {
 trait FromValue: Parseable {
     fn from_value(value: &Value) -> Result<Self, ValueConversionError>;
 
-    fn from_config(config: &Config, id: &OptionId) -> Result<Option<Self>, String> {
+    fn from_config(config: &ConfigReader, id: &OptionId) -> Result<Option<Self>, String> {
         if let Some(value) = config.get_value(id) {
             if value.is_str() {
                 match expand(value.as_str().unwrap().to_owned())
@@ -315,13 +315,29 @@ impl Config {
             value: Value::Table(new_table),
         })
     }
+}
+
+pub(crate) struct ConfigReader {
+    config: Config,
+    #[allow(dead_code)]
+    fromfile_expander: FromfileExpander,
+}
+
+impl ConfigReader {
+    pub fn new(config: Config, fromfile_expander: FromfileExpander) -> Self {
+        Self {
+            config,
+            fromfile_expander,
+        }
+    }
 
     fn option_name(id: &OptionId) -> String {
         id.name("_", NameTransform::None)
     }
 
     fn get_value(&self, id: &OptionId) -> Option<&Value> {
-        self.value
+        self.config
+            .value
             .get(id.scope.name())
             .and_then(|table| table.get(Self::option_name(id)))
     }
@@ -331,7 +347,7 @@ impl Config {
         id: &OptionId,
     ) -> Result<Option<Vec<ListEdit<T>>>, String> {
         let mut list_edits = vec![];
-        if let Some(table) = self.value.get(id.scope.name()) {
+        if let Some(table) = self.config.value.get(id.scope.name()) {
             let option_name = Self::option_name(id);
             if let Some(value) = table.get(&option_name) {
                 match value {
@@ -378,7 +394,7 @@ impl Config {
     }
 }
 
-impl OptionsSource for Config {
+impl OptionsSource for ConfigReader {
     fn display(&self, id: &OptionId) -> String {
         format!("{id}")
     }
@@ -416,7 +432,7 @@ impl OptionsSource for Config {
     }
 
     fn get_dict(&self, id: &OptionId) -> Result<Option<Vec<DictEdit>>, String> {
-        if let Some(table) = self.value.get(id.scope.name()) {
+        if let Some(table) = self.config.value.get(id.scope.name()) {
             let option_name = Self::option_name(id);
             if let Some(value) = table.get(&option_name) {
                 match value {

src/rust/engine/options/src/config_tests.rs

@@ -13,11 +13,12 @@ use crate::{
     option_id, DictEdit, DictEditAction, ListEdit, ListEditAction, OptionId, OptionsSource, Val,
 };
 
-use crate::config::Config;
+use crate::config::{Config, ConfigReader};
 use crate::fromfile::test_util::write_fromfile;
+use crate::fromfile::FromfileExpander;
 use tempfile::TempDir;
 
-fn maybe_config(file_content: &str) -> Result<Config, String> {
+fn maybe_config(file_content: &str) -> Result<ConfigReader, String> {
     let dir = TempDir::new().unwrap();
     let path = dir.path().join("pants.toml");
     File::create(&path)
@@ -31,9 +32,10 @@ fn maybe_config(file_content: &str) -> Result<Config, String> {
             ("seed2".to_string(), "seed2val".to_string()),
         ]),
     )
+    .map(|config| ConfigReader::new(config, FromfileExpander::new()))
 }
 
-fn config(file_content: &str) -> Config {
+fn config(file_content: &str) -> ConfigReader {
     maybe_config(file_content).unwrap()
 }
 
@@ -178,18 +180,18 @@ fn test_scalar_fromfile() {
     fn do_test<T: PartialEq + Debug>(
         content: &str,
         expected: T,
-        getter: fn(&Config, &OptionId) -> Result<Option<T>, String>,
+        getter: fn(&ConfigReader, &OptionId) -> Result<Option<T>, String>,
     ) {
         let (_tmpdir, fromfile_path) = write_fromfile("fromfile.txt", content);
         let conf = config(format!("[GLOBAL]\nfoo = '@{}'\n", fromfile_path.display()).as_str());
         let actual = getter(&conf, &option_id!("foo")).unwrap().unwrap();
         assert_eq!(expected, actual)
     }
 
-    do_test("true", true, Config::get_bool);
-    do_test("-42", -42, Config::get_int);
-    do_test("3.14", 3.14, Config::get_float);
-    do_test("EXPANDED", "EXPANDED".to_owned(), Config::get_string);
+    do_test("true", true, ConfigReader::get_bool);
+    do_test("-42", -42, ConfigReader::get_int);
+    do_test("3.14", 3.14, ConfigReader::get_float);
+    do_test("EXPANDED", "EXPANDED".to_owned(), ConfigReader::get_string);
 }
 
 #[test]

src/rust/engine/options/src/env.rs

@@ -7,7 +7,7 @@ use std::ffi::OsString;
 
 use super::id::{NameTransform, OptionId, Scope};
 use super::{DictEdit, OptionsSource};
-use crate::fromfile::{expand, expand_to_dict, expand_to_list};
+use crate::fromfile::{expand, expand_to_dict, expand_to_list, FromfileExpander};
 use crate::parse::Parseable;
 use crate::ListEdit;
 
@@ -51,6 +51,21 @@ impl Env {
         }
         (Self::new(env), dropped)
     }
+}
+
+pub(crate) struct EnvReader {
+    env: Env,
+    #[allow(dead_code)]
+    fromfile_expander: FromfileExpander,
+}
+
+impl EnvReader {
+    pub(crate) fn new(env: Env, fromfile_expander: FromfileExpander) -> Self {
+        Self {
+            env,
+            fromfile_expander,
+        }
+    }
 
     fn env_var_names(id: &OptionId) -> Vec<String> {
         let name = id.name("_", NameTransform::ToUpper);
@@ -70,7 +85,7 @@ impl Env {
 
     fn get_list<T: Parseable>(&self, id: &OptionId) -> Result<Option<Vec<ListEdit<T>>>, String> {
         for env_var_name in &Self::env_var_names(id) {
-            if let Some(value) = self.env.get(env_var_name) {
+            if let Some(value) = self.env.env.get(env_var_name) {
                 return expand_to_list::<T>(value.to_owned())
                     .map_err(|e| e.render(self.display(id)));
             }
@@ -88,14 +103,14 @@ impl From<&Env> for Vec<(String, String)> {
     }
 }
 
-impl OptionsSource for Env {
+impl OptionsSource for EnvReader {
     fn display(&self, id: &OptionId) -> String {
         Self::env_var_names(id).pop().unwrap()
     }
 
     fn get_string(&self, id: &OptionId) -> Result<Option<String>, String> {
         for env_var_name in &Self::env_var_names(id) {
-            if let Some(value) = self.env.get(env_var_name) {
+            if let Some(value) = self.env.env.get(env_var_name) {
                 return expand(value.to_owned()).map_err(|e| e.render(self.display(id)));
             }
         }
@@ -130,7 +145,7 @@ impl OptionsSource for Env {
 
     fn get_dict(&self, id: &OptionId) -> Result<Option<Vec<DictEdit>>, String> {
         for env_var_name in &Self::env_var_names(id) {
-            if let Some(value) = self.env.get(env_var_name) {
+            if let Some(value) = self.env.env.get(env_var_name) {
                 return expand_to_dict(value.to_owned()).map_err(|e| e.render(self.display(id)));
             }
         }