Support @fromfile option values in the Rust options parser. #20634

benjyw · 2024-03-02T04:54:43Z

As part of this, streamlines away some previous function pointer
clunkiness via a new Parseable trait that implements parsing
of scalars and list of scalars for the scalar types bool/i64/f64/String.

While messing with this code: this change also implements the
"source" field for list- and dict-valued option values.
Previously this was only provided for scalar-valued options. However
the highest-priority source is still a valid and useful concept even
for compound values that might be composed across multiple sources.

benjyw · 2024-03-02T05:12:38Z

Reviewers - thanks for your attention and patience. The diff size seems scary, but the vast majority of it is in new tests, and the Cargo.lock changes.

I recommend starting by looking at the changes in parse.rs (and my GH comments there), then lib.rs, then the changes in args.rs/env.rs/config.rs should be straightforward applications of those, and then there are the new tests, which are very uniform across args_tests.rs/env_tests.rs/config_tests.rs.

benjyw · 2024-03-02T05:14:13Z

src/rust/engine/options/src/parse.rs

@@ -274,49 +278,188 @@ fn format_parse_error(
    ))
 }

-#[allow(dead_code)]
-pub(crate) fn parse_bool(value: &str) -> Result<bool, ParseError> {


Previously we called these for spot-parsing in various places, or passed these around as function pointers in generic code. Instead, we now implement these via a new trait, Parseable, which allows the straightforward bool::parse() and bool::parse_list() instead.

benjyw · 2024-03-02T05:15:06Z

src/rust/engine/options/src/parse.rs

-#[allow(dead_code)]
-pub(crate) fn parse_bool(value: &str) -> Result<bool, ParseError> {
-    option_value_parser::bool(value).map_err(|e| format_parse_error("bool", value, e))
+pub(crate) fn parse_dict(value: &str) -> Result<DictEdit, ParseError> {


We do still need an explicit parse_dict() though.

Because dict values are heterogeneous, so there is no scalar type to attach their conversion to, like we do for lists. We could implement parse() (but not parse_list()) on HashMap<String, Val> but that doesn't really buy us anything. The point of the other cleanups was to get rid of boilerplate via type parametrization, but that doesn't apply here.

benjyw · 2024-03-02T05:17:21Z

src/rust/engine/options/src/parse.rs

+    }
+}
+
+pub(crate) trait ListMember: Parseable + DeserializeOwned {}


This is just a tagging trait for list members, to ensure they are also deserializable into an owned instance by serde, when we read them from json/yaml fromfiles.

I know of three goals with this sort of trait:

an alias that's more convenient than writing T: Parseable + DeserializeOwned

wrapping multiple traits up into a single one, for use in a trait object (i.e. &dyn ListMember works, but &dyn (Parseable + DeserializeOwned) doesn't; the + syntax only supports built-in traits like Send)

an "assertion" that particular types implement the parent traits

Is this usage fitting into one of those categories? If the first or second one, an option for the impl might be a blanket impl, something like:

impl<T: Parseable + DeserializeOwned + ?Sized> ListMember for T {}

(With the ?Sized bound only required to support trait objects well, I think. So not necessary.)

If the third one, an another option is something like:

const fn assert_is_list_member<T: Parseable + DeserializeOwned>() {} const _ASSERT_BOOL: () = assert_is_list_member::<bool>(); ...

but now that I write it out, that's not amazingly nicer. 🤷‍♂️

It was #1.

But this code evolved while I was working on it, and now that I look at where it ended up, ListMember isn't actually needed anyway. Every Parseable can be a list member (because of its parse_list()), so it's easier to make Parseable a subtrait of DeserializeOwned and be done with it.

Good call!

benjyw · 2024-03-02T05:18:01Z

src/rust/engine/options/src/parse.rs

-pub(crate) fn parse_bool_list(value: &str) -> Result<Vec<ListEdit<bool>>, ParseError> {
-    option_value_parser::bool_list_edits(value)
-        .map_err(|e| format_parse_error("bool list", value, e))
+fn maybe_expand(value: String) -> Result<ExpandedValue, ParseError> {


A helper function for the expand* functions below that are the ones actually used by args/env/config.

benjyw · 2024-03-02T05:19:32Z

src/rust/engine/options/src/parse.rs

-#[allow(dead_code)]
-pub(crate) fn parse_int_list(value: &str) -> Result<Vec<ListEdit<i64>>, ParseError> {
-    option_value_parser::int_list_edits(value).map_err(|e| format_parse_error("int list", value, e))
+pub(crate) fn expand(value: String) -> Result<Option<String>, ParseError> {


This does no type conversion, just expands "path to @fromfile" -> "content of @fromfile". See usage in args/env/config.rs.

benjyw · 2024-03-02T05:20:19Z

src/rust/engine/options/src/parse.rs

+        if let Some(ext) = path.extension() {
+            if ext == "json" {
+                deserialized_items =
+                    Some(serde_json::from_str(&value).map_err(|e| mk_parse_err(e, &path))?)


serde is freakin' magic! This (plus a minor annotation on the Val class in lib.rs) is all I needed to get Rust to parse json and yaml into our existing data structures!

benjyw · 2024-03-02T05:21:36Z

src/rust/engine/options/src/parse.rs

+        if let Some(ext) = path.extension() {
+            if ext == "json" {
+                deserialized_items =
+                    Some(serde_json::from_str(&value).map_err(|e| mk_parse_err(e, &path))?)


There's a little bit of repetition here vis the previous function, but the differences are just subtle enough that it became more confusing than it was worth to try and unify them.

What do you think about at least pulling out the file-type-identification? Maybe something like

enum FileType { Json, Yalm } impl FileType { fn guess(path: &Path) -> Option<FileType> { match path.extension() { "json" => Some(FileType::Json), "yaml" | "yml" => ..., _ => None, } } }

and then this section of the code in both could could be something like:

let deserialized_items = match path_opt.flat_map(|p| FileType::guess(&p)) { Some(FileType::Json) => ..., Some(FileType::Yaml) => ..., None => None, };

One could potentially then even move the "call serde_json or serde_yaml" decision to a generic method on FileType, and have that be shared.

I toyed with this some more and was able to create a generic deserialization helper function that works for lists and dicts, so hooray Rust type system!

benjyw · 2024-03-02T05:22:51Z

src/rust/engine/options/src/parse_tests.rs

@@ -38,7 +40,18 @@ fn check_with_arg<T: PartialEq + Debug>(
 #[test]
 fn test_parse_quoted_string() {
    fn check_str(expected: &str, input: &str) {
-        check!(expected.to_string(), parse_quoted_string(input));
+        // This is slightly convoluted: quoted strings appear as list items,


This is so I could delete the otherwise dead code parse_quoted_string(). It makes sense to specifically test that we handle quoted strings correctly, but now we do so where we actually encounter them - inside lists.

benjyw · 2024-03-02T05:23:38Z

src/rust/engine/options/src/parse_tests.rs

@@ -648,3 +661,190 @@ fn test_parse_heterogeneous_dict() {
        )
    );
 }
+
+#[test]


From here we have some very exhaustive tests of the fromfile mechanism. We also test this mechanism's application in args/env/config in the appropriate test files.

Great!

In addition to this set, could we have a test or two covering some more error cases beyond just "not existing": the major class I can think of is when the file contents is invalid, both as a scalar, and, probably more interestingly, as a list or dict (e.g. where serde json/yaml can't even parse it).

adding some list/dict cases as you suggest. There is no such thing as an invalid scalar at this level, since we don't convert to the type but just expand string to string. Can test for this in args/config/env maybe. I'll look into adding something.

benjyw · 2024-03-02T05:24:01Z

src/rust/engine/options/src/lib.rs

-///
-/// A source of option values.
-///
-/// This is currently a subset of the types of options the Pants python option system handles.


It is no longer a subset! So this whole comment can go.

benjyw · 2024-03-02T05:24:50Z

src/rust/engine/options/src/lib.rs

    pub source: Source,
    pub value: T,
 }

 #[derive(Debug)]
 pub struct ListOptionValue<T> {
    pub derivation: Option<Vec<(Source, Vec<ListEdit<T>>)>>,
+    // The highest-priority source that provided edits for this value.


Not directly needed for fromfile, but will be needed for replacing the Python parser, and it was easy to sneak in here...

benjyw · 2024-03-02T05:25:45Z

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

-        id: &OptionId,
-        parse_list: fn(&str) -> Result<Vec<ListEdit<T>>, ParseError>,
-    ) -> Result<Option<Vec<ListEdit<T>>>, String> {
+    fn get_list<T: ListMember>(&self, id: &OptionId) -> Result<Option<Vec<ListEdit<T>>>, String> {


No more need for the parse_list function pointer - ListMember has the functionality we need (via its Parseable supertrait).

benjyw · 2024-03-02T05:27:27Z

src/rust/engine/options/src/lib.rs

@@ -54,7 +56,8 @@ pub use types::OptionType;
 // We only use this for parsing values in dicts, as in other cases we know that the type must
 // be some scalar or string, or a uniform list of one type of scalar or string, so we can
 // parse as such.
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug, PartialEq, Deserialize)]
+#[serde(untagged)]


All I needed to do to get json and yaml deserialization working!

huonw

Really nice!

huonw · 2024-03-03T22:10:52Z

src/rust/engine/options/src/parse.rs

-#[allow(dead_code)]
-pub(crate) fn parse_bool(value: &str) -> Result<bool, ParseError> {
-    option_value_parser::bool(value).map_err(|e| format_parse_error("bool", value, e))
+pub(crate) fn parse_dict(value: &str) -> Result<DictEdit, ParseError> {


huonw · 2024-03-03T22:18:45Z

src/rust/engine/options/src/parse.rs

+    }
+}
+
+pub(crate) trait ListMember: Parseable + DeserializeOwned {}


I know of three goals with this sort of trait:

an alias that's more convenient than writing T: Parseable + DeserializeOwned

wrapping multiple traits up into a single one, for use in a trait object (i.e. &dyn ListMember works, but &dyn (Parseable + DeserializeOwned) doesn't; the + syntax only supports built-in traits like Send)

an "assertion" that particular types implement the parent traits

Is this usage fitting into one of those categories? If the first or second one, an option for the impl might be a blanket impl, something like:

impl<T: Parseable + DeserializeOwned + ?Sized> ListMember for T {}

(With the ?Sized bound only required to support trait objects well, I think. So not necessary.)

If the third one, an another option is something like:

const fn assert_is_list_member<T: Parseable + DeserializeOwned>() {} const _ASSERT_BOOL: () = assert_is_list_member::<bool>(); ...

but now that I write it out, that's not amazingly nicer. 🤷‍♂️

huonw · 2024-03-03T22:21:37Z

src/rust/engine/options/src/parse.rs

+
+fn mk_parse_err(err: impl Display, path: &Path) -> ParseError {
+    ParseError::new(format!(
+        "Problem reading {path} for {{name}}: {err_msg}",


What's the {{name}} mean here?

A placeholder that ParseError.render() fills in with the option name, rendered suitably for the source type (e.g., --foo vs PANTS_FOO vs [GLOBAL] foo . This was all preexisting before any of my recent changes.

huonw · 2024-03-03T23:40:02Z

src/rust/engine/options/src/parse.rs

+                Some(subsuffix) => {
+                    // @? means the path is allowed to not exist.
+                    let path = PathBuf::from(subsuffix);
+                    match fs::read_to_string(&path) {


I note that this is doing synchronous IO, but generally pants does async. Is the sync IO acceptable here?

Probably? We expect very few fromfile options, and they are read synchronously today in the Python code.

The trickier part is invalidating options when the fromfile changes (which we don't to today either). Making this async though would be a pretty big change that would be best done as its own project. For now we're no worse off than today.

huonw · 2024-03-03T23:48:16Z

src/rust/engine/options/src/parse.rs

+        if let Some(ext) = path.extension() {
+            if ext == "json" {
+                deserialized_items =
+                    Some(serde_json::from_str(&value).map_err(|e| mk_parse_err(e, &path))?)


What do you think about at least pulling out the file-type-identification? Maybe something like

enum FileType { Json, Yalm } impl FileType { fn guess(path: &Path) -> Option<FileType> { match path.extension() { "json" => Some(FileType::Json), "yaml" | "yml" => ..., _ => None, } } }

and then this section of the code in both could could be something like:

let deserialized_items = match path_opt.flat_map(|p| FileType::guess(&p)) { Some(FileType::Json) => ..., Some(FileType::Yaml) => ..., None => None, };

One could potentially then even move the "call serde_json or serde_yaml" decision to a generic method on FileType, and have that be shared.

huonw · 2024-03-03T23:56:38Z

src/rust/engine/options/src/parse_tests.rs

+        Ok(Some("FOO".to_string())),
+        expand(format!("@{}", fromfile_path_str))
+    );
+    assert_eq!(Ok(None), expand("@?/does/not/exist".to_string()));


Could we also having a test for an optional file that does exist, i.e. make sure that we're stripping off the ? correctly? (I could imagine a future refactoring making a typo that causes us to pass in ?/does/not/exist literally as the file path.)

In that vein, is it worth having "edge-case tests" that at least lock down the behaviour for "weird" things like @@path/to/file reading @path/to/file and similarly @??path/to/file (optionally) reading ?path/to/file?

Done. Note that @@ is already covered - that is the escape hatch for a literal value that starts with @, so we never allow such a fromfile.

huonw · 2024-03-03T23:58:59Z

src/rust/engine/options/src/parse_tests.rs

@@ -648,3 +661,190 @@ fn test_parse_heterogeneous_dict() {
        )
    );
 }
+
+#[test]


Great!

In addition to this set, could we have a test or two covering some more error cases beyond just "not existing": the major class I can think of is when the file contents is invalid, both as a scalar, and, probably more interestingly, as a list or dict (e.g. where serde json/yaml can't even parse it).

benjyw · 2024-03-04T07:10:34Z

Thanks for the helpful review! I've addressed all your comments. PTAL.

huonw

Awesome

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

src/rust/engine/options/src/parse.rs

Support @fromfile option values in the Rust options parser.

0f84bba

benjyw added the category:internal CI, fixes for not-yet-released features, etc. label Mar 2, 2024

Get rid of dead code

61a08ce

benjyw commented Mar 2, 2024

View reviewed changes

benjyw requested review from huonw and tgolsson March 2, 2024 05:26

benjyw commented Mar 2, 2024

View reviewed changes

huonw reviewed Mar 3, 2024

View reviewed changes

Address code review comments

16a93c6

Log a missing optional fromfile, per pantsbuild#20571

27bfbc5

huonw approved these changes Mar 4, 2024

View reviewed changes

src/rust/engine/options/src/args_tests.rs Show resolved Hide resolved

src/rust/engine/options/src/parse.rs Outdated Show resolved Hide resolved

benjyw added 2 commits March 4, 2024 15:25

Remove commented-out code

a55b8ab

Merge branch 'main' into rust_options_fromfile2

4d1829c

benjyw merged commit af1b5c7 into pantsbuild:main Mar 5, 2024
24 checks passed

benjyw deleted the rust_options_fromfile2 branch March 5, 2024 01:04

purajit mentioned this pull request Jun 26, 2024

Bump default terraform version to 1.9.0, add all known terraform versions inbetween #21110

Merged

@@ @@ -648,3 +661,190 @@ fn test_parse_heterogeneous_dict() { @@
                       )
                   );
               }
+              #[test]

Support @fromfile option values in the Rust options parser. #20634

Support @fromfile option values in the Rust options parser. #20634

Conversation

benjyw commented Mar 2, 2024 • edited Loading

benjyw commented Mar 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw Mar 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw Mar 3, 2024 • edited Loading

Choose a reason for hiding this comment

benjyw commented Mar 4, 2024

huonw left a comment

Choose a reason for hiding this comment

benjyw commented Mar 2, 2024 •

edited

Loading

huonw Mar 3, 2024 •

edited

Loading

huonw Mar 3, 2024 •

edited

Loading