Use the FromfileExpander to expand @fromfile values. (pantsbuild#20815)

Now that it's plumbed through, we can use it directly instead of the free functions, which are now deleted (or more precisely, moved into the test file, for convenience there). A final change will actually add buildroot context to the FromfileExpander, and use it to correctly handle @relpath/to/fromfile situations.
krishnan-chandra · Apr 19, 2024 · 572627f · 572627f
1 parent 093d536
commit 572627f
Show file tree

Hide file tree

Showing 5 changed files with 65 additions and 46 deletions.
diff --git a/src/rust/engine/options/src/args.rs b/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, FromfileExpander};
+use crate::fromfile::FromfileExpander;
 use crate::parse::{ParseError, Parseable};
 use crate::ListEdit;
 use core::iter::once;
@@ -81,18 +81,6 @@ impl Arg {
     fn matches_negation(&self, id: &OptionId) -> bool {
         self._matches(id, true)
     }
-
-    fn to_bool(&self) -> Result<Option<bool>, ParseError> {
-        // An arg can represent a bool either by having an explicit value parseable as a bool,
-        // or by having no value (in which case it represents true).
-        match &self.value {
-            Some(value) => match expand(value.to_string())? {
-                Some(s) => bool::parse(&s).map(Some),
-                _ => Ok(None),
-            },
-            None => Ok(Some(true)),
-        }
-    }
 }
 
 #[derive(Debug)]
@@ -177,15 +165,29 @@ impl ArgsReader {
             .map(|v| Vec::from_iter(v.iter().map(String::as_str)))
     }
 
+    fn to_bool(&self, arg: &Arg) -> Result<Option<bool>, ParseError> {
+        // An arg can represent a bool either by having an explicit value parseable as a bool,
+        // or by having no value (in which case it represents true).
+        match &arg.value {
+            Some(value) => match self.fromfile_expander.expand(value.to_string())? {
+                Some(s) => bool::parse(&s).map(Some),
+                _ => Ok(None),
+            },
+            None => Ok(Some(true)),
+        }
+    }
+
     fn get_list<T: Parseable>(&self, id: &OptionId) -> Result<Option<Vec<ListEdit<T>>>, String> {
         let mut edits = vec![];
         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))
                 })?;
-                if let Some(es) =
-                    expand_to_list::<T>(value.to_string()).map_err(|e| e.render(&arg.flag))?
+                if let Some(es) = self
+                    .fromfile_expander
+                    .expand_to_list::<T>(value.to_string())
+                    .map_err(|e| e.render(&arg.flag))?
                 {
                     edits.extend(es);
                 }
@@ -216,10 +218,12 @@ impl OptionsSource for ArgsReader {
         // is specified multiple times.
         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))
-                })?)
-                .map_err(|e| e.render(&arg.flag));
+                return self
+                    .fromfile_expander
+                    .expand(arg.value.clone().ok_or_else(|| {
+                        format!("Expected list option {} to have a value.", self.display(id))
+                    })?)
+                    .map_err(|e| e.render(&arg.flag));
             };
         }
         Ok(None)
@@ -230,10 +234,10 @@ impl OptionsSource for ArgsReader {
         // is specified multiple times.
         for arg in self.args.args.iter().rev() {
             if arg.matches(id) {
-                return arg.to_bool().map_err(|e| e.render(&arg.flag));
+                return self.to_bool(arg).map_err(|e| e.render(&arg.flag));
             } else if arg.matches_negation(id) {
-                return arg
-                    .to_bool()
+                return self
+                    .to_bool(arg)
                     .map(|ob| ob.map(|b| b ^ true))
                     .map_err(|e| e.render(&arg.flag));
             }
@@ -264,7 +268,11 @@ impl OptionsSource for ArgsReader {
                 let value = arg.value.clone().ok_or_else(|| {
                     format!("Expected dict option {} to have a value.", self.display(id))
                 })?;
-                if let Some(es) = expand_to_dict(value).map_err(|e| e.render(&arg.flag))? {
+                if let Some(es) = self
+                    .fromfile_expander
+                    .expand_to_dict(value)
+                    .map_err(|e| e.render(&arg.flag))?
+                {
                     edits.extend(es);
                 }
             }

diff --git a/src/rust/engine/options/src/config.rs b/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, FromfileExpander};
+use crate::fromfile::FromfileExpander;
 use crate::id::{NameTransform, OptionId};
 use crate::parse::Parseable;
 
@@ -104,7 +104,9 @@ trait FromValue: Parseable {
     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())
+                match config
+                    .fromfile_expander
+                    .expand(value.as_str().unwrap().to_owned())
                     .map_err(|e| e.render(config.display(id)))?
                 {
                     Some(expanded_value) => Ok(Some(
@@ -319,7 +321,6 @@ impl Config {
 
 pub(crate) struct ConfigReader {
     config: Config,
-    #[allow(dead_code)]
     fromfile_expander: FromfileExpander,
 }
 
@@ -377,7 +378,9 @@ impl ConfigReader {
                         }
                     }
                     Value::String(v) => {
-                        if let Some(es) = expand_to_list::<T>(v.to_string())
+                        if let Some(es) = self
+                            .fromfile_expander
+                            .expand_to_list::<T>(v.to_string())
                             .map_err(|e| e.render(self.display(id)))?
                         {
                             list_edits.extend(es);
@@ -451,7 +454,9 @@ impl OptionsSource for ConfigReader {
                         }]));
                     }
                     Value::String(v) => {
-                        return expand_to_dict(v.to_owned())
+                        return self
+                            .fromfile_expander
+                            .expand_to_dict(v.to_owned())
                             .map_err(|e| e.render(self.display(id)));
                     }
                     _ => {

diff --git a/src/rust/engine/options/src/env.rs b/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, FromfileExpander};
+use crate::fromfile::FromfileExpander;
 use crate::parse::Parseable;
 use crate::ListEdit;
 
@@ -86,7 +86,9 @@ impl EnvReader {
     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.env.get(env_var_name) {
-                return expand_to_list::<T>(value.to_owned())
+                return self
+                    .fromfile_expander
+                    .expand_to_list::<T>(value.to_owned())
                     .map_err(|e| e.render(self.display(id)));
             }
         }
@@ -111,7 +113,10 @@ impl OptionsSource for EnvReader {
     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.env.get(env_var_name) {
-                return expand(value.to_owned()).map_err(|e| e.render(self.display(id)));
+                return self
+                    .fromfile_expander
+                    .expand(value.to_owned())
+                    .map_err(|e| e.render(self.display(id)));
             }
         }
         Ok(None)
@@ -146,7 +151,10 @@ impl OptionsSource for EnvReader {
     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.env.get(env_var_name) {
-                return expand_to_dict(value.to_owned()).map_err(|e| e.render(self.display(id)));
+                return self
+                    .fromfile_expander
+                    .expand_to_dict(value.to_owned())
+                    .map_err(|e| e.render(self.display(id)));
             }
         }
         Ok(None)

diff --git a/src/rust/engine/options/src/fromfile.rs b/src/rust/engine/options/src/fromfile.rs
@@ -134,20 +134,6 @@ impl FromfileExpander {
     }
 }
 
-pub(crate) fn expand(value: String) -> Result<Option<String>, ParseError> {
-    FromfileExpander::new().expand(value)
-}
-
-pub(crate) fn expand_to_list<T: Parseable>(
-    value: String,
-) -> Result<Option<Vec<ListEdit<T>>>, ParseError> {
-    FromfileExpander::new().expand_to_list(value)
-}
-
-pub(crate) fn expand_to_dict(value: String) -> Result<Option<Vec<DictEdit>>, ParseError> {
-    FromfileExpander::new().expand_to_dict(value)
-}
-
 #[cfg(test)]
 pub(crate) mod test_util {
     use std::fs::File;

diff --git a/src/rust/engine/options/src/fromfile_tests.rs b/src/rust/engine/options/src/fromfile_tests.rs
@@ -21,6 +21,18 @@ macro_rules! check_err {
     };
 }
 
+fn expand(value: String) -> Result<Option<String>, ParseError> {
+    FromfileExpander::new().expand(value)
+}
+
+fn expand_to_list<T: Parseable>(value: String) -> Result<Option<Vec<ListEdit<T>>>, ParseError> {
+    FromfileExpander::new().expand_to_list(value)
+}
+
+fn expand_to_dict(value: String) -> Result<Option<Vec<DictEdit>>, ParseError> {
+    FromfileExpander::new().expand_to_dict(value)
+}
+
 #[test]
 fn test_expand_fromfile() {
     let (_tmpdir, fromfile_pathbuf) = write_fromfile("fromfile.txt", "FOO");