From f8d5e37ec2dce1a910d56a94dc7ffedf4a7a997a Mon Sep 17 00:00:00 2001 From: Benjy Weinberger Date: Mon, 25 Nov 2024 13:14:51 -0800 Subject: [PATCH] Simplify OptionParser fields. (#21689) Previously the OptionParser held explicit references to the `passthrough_args` and `args_tracker` of its ArgsReader. However this denormalization can lead to inconsistency if we were to want to replace the ArgsReader during options bootstrapping. Now that we have the `as_any()` mechanism we can downcast the ArgsReader and get those fields out of it as needed, so now there is only one reference into the args reader and its internals. This PR also adds a field to the `Args` struct to store the original strings the `Args` was parsed from. Both of these are preliminaries for an upcoming change that ports the CLI Alias mechanism to Rust. --- src/rust/engine/options/src/args.rs | 20 +++++++++----- src/rust/engine/options/src/args_tests.rs | 4 +-- src/rust/engine/options/src/lib.rs | 32 ++++++++++++----------- src/rust/engine/src/externs/options.rs | 11 +++++--- 4 files changed, 40 insertions(+), 27 deletions(-) diff --git a/src/rust/engine/options/src/args.rs b/src/rust/engine/options/src/args.rs index 4d06f9315d6..19977eb18be 100644 --- a/src/rust/engine/options/src/args.rs +++ b/src/rust/engine/options/src/args.rs @@ -89,6 +89,11 @@ impl Arg { #[derive(Debug)] pub struct Args { + // The arg strings this struct was instantiated with. + #[allow(dead_code)] + arg_strs: Vec, + + // The structured args parsed from the arg strings. args: Vec, passthrough_args: Option>, } @@ -97,14 +102,16 @@ impl Args { // Create an Args instance with the provided args, which must *not* include the // argv[0] process name. pub fn new>(arg_strs: I) -> Self { + let arg_strs = arg_strs.into_iter().collect::>(); let mut args: Vec = vec![]; let mut passthrough_args: Option> = None; let mut scope = Scope::Global; - let mut args_iter = arg_strs.into_iter(); + + let mut args_iter = arg_strs.iter(); while let Some(arg_str) = args_iter.next() { if arg_str == "--" { // We've hit the passthrough args delimiter (`--`). - passthrough_args = Some(args_iter.collect::>()); + passthrough_args = Some(args_iter.cloned().collect::>()); break; } else if arg_str.starts_with("--") { let mut components = arg_str.splitn(2, '='); @@ -129,8 +136,8 @@ impl Args { Some(value.to_string()) }, }); - } else if is_valid_scope_name(&arg_str) { - scope = Scope::Scope(arg_str) + } else if is_valid_scope_name(arg_str) { + scope = Scope::Scope(arg_str.to_string()) } else { // The arg is a spec, so revert to global context for any trailing flags. scope = Scope::Global; @@ -138,6 +145,7 @@ impl Args { } Self { + arg_strs, args, passthrough_args, } @@ -200,8 +208,8 @@ impl ArgsReader { } } - pub fn get_passthrough_args(&self) -> Option<&Vec> { - self.args.passthrough_args.as_ref() + pub fn get_passthrough_args(&self) -> Option> { + self.args.passthrough_args.clone() } pub fn get_tracker(&self) -> Arc { diff --git a/src/rust/engine/options/src/args_tests.rs b/src/rust/engine/options/src/args_tests.rs index b2fd70a5049..f71ccce036b 100644 --- a/src/rust/engine/options/src/args_tests.rs +++ b/src/rust/engine/options/src/args_tests.rs @@ -391,7 +391,7 @@ fn test_passthrough_args() { assert_string("debug", option_id!(-'l', "level")); assert_eq!( - Some(&vec![ + Some(vec![ "--passthrough0".to_string(), "passthrough1".to_string(), "-p".to_string(), @@ -404,7 +404,7 @@ fn test_passthrough_args() { fn test_empty_passthrough_args() { let args = mk_args(["-ldebug", "--foo=bar", "--"]); - assert_eq!(Some(&vec![]), args.get_passthrough_args()); + assert_eq!(Some(vec![]), args.get_passthrough_args()); } #[test] diff --git a/src/rust/engine/options/src/lib.rs b/src/rust/engine/options/src/lib.rs index 2e89ab5030d..c75b51f4f3e 100644 --- a/src/rust/engine/options/src/lib.rs +++ b/src/rust/engine/options/src/lib.rs @@ -49,7 +49,6 @@ pub use self::config::ConfigSource; use self::config::{Config, ConfigReader}; pub use self::env::Env; use self::env::EnvReader; -use crate::args::ArgsTracker; use crate::fromfile::FromfileExpander; use crate::parse::Parseable; pub use build_root::BuildRoot; @@ -310,8 +309,6 @@ pub struct DictOptionValue<'a> { pub struct OptionParser { sources: BTreeMap>, include_derivation: bool, - passthrough_args: Option>, - args_tracker: Arc, } impl OptionParser { @@ -338,8 +335,6 @@ impl OptionParser { ); let args_reader = ArgsReader::new(args, fromfile_expander.clone()); - let args_tracker = args_reader.get_tracker(); - let passthrough_args = args_reader.get_passthrough_args().cloned(); let mut sources: BTreeMap> = BTreeMap::new(); sources.insert( @@ -350,8 +345,6 @@ impl OptionParser { let mut parser = OptionParser { sources: sources.clone(), include_derivation: false, - passthrough_args: None, - args_tracker: args_tracker.clone(), }; fn path_join(prefix: &str, suffix: &str) -> String { @@ -424,8 +417,6 @@ impl OptionParser { parser = OptionParser { sources: sources.clone(), include_derivation: false, - passthrough_args: None, - args_tracker: args_tracker.clone(), }; if allow_pantsrc @@ -458,11 +449,10 @@ impl OptionParser { } } } + Ok(OptionParser { sources, include_derivation, - passthrough_args, - args_tracker, }) } @@ -702,12 +692,24 @@ impl OptionParser { }) } - pub fn get_passthrough_args(&self) -> Option<&Vec> { - self.passthrough_args.as_ref() + pub fn get_passthrough_args(&self) -> Result>, String> { + Ok(self.get_args_reader()?.get_passthrough_args()) + } + + pub fn get_unconsumed_flags(&self) -> Result>, String> { + Ok(self.get_args_reader()?.get_tracker().get_unconsumed_flags()) } - pub fn get_unconsumed_flags(&self) -> HashMap> { - self.args_tracker.get_unconsumed_flags() + fn get_args_reader(&self) -> Result<&ArgsReader, String> { + if let Some(args_reader) = self + .sources + .get(&Source::Flag) + .and_then(|source| source.as_any().downcast_ref::()) + { + Ok(args_reader) + } else { + Err("This OptionParser does not have command-line args as a source".to_string()) + } } // Given a map from section name to valid keys for that section, diff --git a/src/rust/engine/src/externs/options.rs b/src/rust/engine/src/externs/options.rs index c1288d6a0c2..b60ac8df943 100644 --- a/src/rust/engine/src/externs/options.rs +++ b/src/rust/engine/src/externs/options.rs @@ -1,6 +1,7 @@ // Copyright 2024 Pants project contributors (see CONTRIBUTORS.md). // Licensed under the Apache License, Version 2.0 (see LICENSE). +use pyo3::exceptions::PyException; use pyo3::types::{PyBool, PyDict, PyFloat, PyInt, PyList, PyString, PyTuple}; use pyo3::{prelude::*, BoundObject}; @@ -499,13 +500,15 @@ impl PyOptionParser { } fn get_passthrough_args(&self) -> PyResult>> { - Ok(self.0.get_passthrough_args().cloned()) + self.0.get_passthrough_args().map_err(PyException::new_err) } - fn get_unconsumed_flags(&self) -> HashMap> { + fn get_unconsumed_flags(&self) -> PyResult>> { // The python side expects an empty string to represent the GLOBAL scope. - self.0 + Ok(self + .0 .get_unconsumed_flags() + .map_err(PyException::new_err)? .into_iter() .map(|(k, v)| { ( @@ -513,7 +516,7 @@ impl PyOptionParser { v, ) }) - .collect() + .collect()) } fn validate_config(