diff --git a/src/python/pants/engine/internals/native_engine.pyi b/src/python/pants/engine/internals/native_engine.pyi index 86aa2ef8702..a10e1bf7ac1 100644 --- a/src/python/pants/engine/internals/native_engine.pyi +++ b/src/python/pants/engine/internals/native_engine.pyi @@ -659,11 +659,16 @@ class PyOptionId: class PyConfigSource: def __init__(self, path: str, content: bytes) -> None: ... +# See src/rust/engine/src/externs/options.rs for the Rust-side versions of these types. T = TypeVar("T") -# A pair of (option value, rank). See src/python/pants/option/ranked_value.py. -OptionValue = Tuple[Optional[T], int] -OptionListValue = Tuple[list[T], int] -OptionDictValue = Tuple[dict[str, Any], int] + +# List of pairs of (value, ranks of the sources of the value). +# A scalar value will always have a single source. A list/dict value +# may come from multiple sources, if its elements were appended. +OptionValueDerivation = list[Tuple[T, list[int]]] + +# A tuple (value, rank of value, optional derivation of value). +OptionValue = Tuple[Optional[T], int, Optional[OptionValueDerivation]] class PyOptionParser: def __init__( @@ -672,6 +677,7 @@ class PyOptionParser: env: dict[str, str], configs: Optional[Sequence[PyConfigSource]], allow_pantsrc: bool, + include_derivation: bool, ) -> None: ... def get_bool(self, option_id: PyOptionId, default: Optional[bool]) -> OptionValue[bool]: ... def get_int(self, option_id: PyOptionId, default: Optional[int]) -> OptionValue[int]: ... @@ -679,15 +685,15 @@ class PyOptionParser: def get_string(self, option_id: PyOptionId, default: Optional[str]) -> OptionValue[str]: ... def get_bool_list( self, option_id: PyOptionId, default: list[bool] - ) -> OptionListValue[bool]: ... - def get_int_list(self, option_id: PyOptionId, default: list[int]) -> OptionListValue[int]: ... + ) -> OptionValue[list[bool]]: ... + def get_int_list(self, option_id: PyOptionId, default: list[int]) -> OptionValue[list[int]]: ... def get_float_list( self, option_id: PyOptionId, default: list[float] - ) -> OptionListValue[float]: ... + ) -> OptionValue[list[float]]: ... def get_string_list( self, option_id: PyOptionId, default: list[str] - ) -> OptionListValue[str]: ... - def get_dict(self, option_id: PyOptionId, default: dict[str, Any]) -> OptionDictValue: ... + ) -> OptionValue[list[str]]: ... + def get_dict(self, option_id: PyOptionId, default: dict[str, Any]) -> OptionValue[dict]: ... def get_passthrough_args(self) -> Optional[list[str]]: ... def get_unconsumed_flags(self) -> dict[str, list[str]]: ... diff --git a/src/python/pants/help/help_formatter_test.py b/src/python/pants/help/help_formatter_test.py index 3075a6812a2..0b525045925 100644 --- a/src/python/pants/help/help_formatter_test.py +++ b/src/python/pants/help/help_formatter_test.py @@ -3,11 +3,13 @@ from __future__ import annotations from dataclasses import replace +from typing import Tuple from pants.help.help_formatter import HelpFormatter from pants.help.help_info_extracter import HelpInfoExtracter, OptionHelpInfo from pants.option.config import Config from pants.option.global_options import GlobalOptions +from pants.option.native_options import NativeOptionParser from pants.option.option_value_container import OptionValueContainerBuilder from pants.option.parser import OptionValueHistory, Parser from pants.option.ranked_value import Rank, RankedValue @@ -58,43 +60,51 @@ def test_format_help_choices(self) -> None: assert default_line.lstrip() == "default: kiwi" @classmethod - def _get_parser(cls) -> Parser: + def _get_parser(cls) -> Tuple[Parser, NativeOptionParser]: return Parser( env={}, config=Config.load([]), scope_info=GlobalOptions.get_scope_info(), + ), NativeOptionParser( + args=[], env={}, config_sources=[], allow_pantsrc=False, include_derivation=True ) @classmethod def _format_for_global_scope( cls, show_advanced: bool, show_deprecated: bool, args: list[str], kwargs ) -> list[str]: - parser = cls._get_parser() + parser, native_parser = cls._get_parser() parser.register(*args, **kwargs) return cls._format_for_global_scope_with_parser( - parser, show_advanced=show_advanced, show_deprecated=show_deprecated + parser, native_parser, show_advanced=show_advanced, show_deprecated=show_deprecated ) @classmethod def _format_for_global_scope_with_parser( - cls, parser: Parser, show_advanced: bool, show_deprecated: bool + cls, + parser: Parser, + native_parser: NativeOptionParser, + show_advanced: bool, + show_deprecated: bool, ) -> list[str]: # Force a parse to generate the derivation history. parser.parse_args(Parser.ParseArgsRequest((), OptionValueContainerBuilder(), [], False)) - oshi = HelpInfoExtracter("").get_option_scope_help_info("", parser, False, "help.test") + oshi = HelpInfoExtracter("").get_option_scope_help_info( + "", parser, native_parser, False, "help.test" + ) return HelpFormatter( show_advanced=show_advanced, show_deprecated=show_deprecated, color=False ).format_options(oshi) def test_suppress_advanced(self) -> None: - parser = self._get_parser() + parser, native_parser = self._get_parser() parser.register("--foo", advanced=True) # must have a non advanced option to be able to supress showing advanced options. parser.register("--jerry", advanced=False) - lines = self._format_for_global_scope_with_parser(parser, False, False) + lines = self._format_for_global_scope_with_parser(parser, native_parser, False, False) assert len(lines) == 15 assert not any("--foo" in line for line in lines) - lines = self._format_for_global_scope_with_parser(parser, True, False) + lines = self._format_for_global_scope_with_parser(parser, native_parser, True, False) assert len(lines) == 24 def test_suppress_deprecated(self) -> None: diff --git a/src/python/pants/help/help_info_extracter.py b/src/python/pants/help/help_info_extracter.py index 9594bfec403..18fff9a7233 100644 --- a/src/python/pants/help/help_info_extracter.py +++ b/src/python/pants/help/help_info_extracter.py @@ -41,9 +41,11 @@ from pants.engine.rules import Rule, TaskRule from pants.engine.target import Field, RegisteredTargetTypes, StringField, Target, TargetGenerator from pants.engine.unions import UnionMembership, UnionRule, is_union +from pants.option.native_options import NativeOptionParser from pants.option.option_util import is_dict_option, is_list_option from pants.option.options import Options from pants.option.parser import OptionValueHistory, Parser +from pants.option.ranked_value import Rank, RankedValue from pants.option.scope import ScopeInfo from pants.util.frozendict import LazyFrozenDict from pants.util.strutil import first_paragraph, strval @@ -505,6 +507,7 @@ def load() -> OptionScopeHelpInfo: return HelpInfoExtracter(scope_info.scope).get_option_scope_help_info( scope_info.description, options.get_parser(scope_info.scope), + options.native_parser.with_derivation(), # `filter` should be treated as a subsystem for `help`, even though it still # works as a goal for backwards compatibility. scope_info.is_goal if scope_info.scope != "filter" else False, @@ -999,6 +1002,7 @@ def get_option_scope_help_info( self, description: str, parser: Parser, + native_parser: NativeOptionParser, is_goal: bool, provider: str = "", deprecated_scope: Optional[str] = None, @@ -1009,7 +1013,37 @@ def get_option_scope_help_info( advanced_options = [] deprecated_options = [] for args, kwargs in parser.option_registrations_iter(): - history = parser.history(kwargs["dest"]) + derivation = native_parser.get_derivation( + scope=parser.scope, registration_args=args, registration_kwargs=kwargs + ) + # Massage the derivation structure returned by the NativeParser into an + # OptionValueHistory as returned by the legacy parser. + # TODO: Once we get rid of the legacy parser we can probably simplify by + # using the native structure directly. + ranked_values = [] + + # Adding this constant, empty history entry is silly, but it appears in the + # legacy parser's results as an implementation artifact, and we want to be + # consistent with its tests until we get rid of it. + is_list = kwargs.get("type") == list + is_dict = kwargs.get("type") == dict + empty_val: list | dict | None = [] if is_list else {} if is_dict else None + empty_details = "" if (is_list or is_dict) else None + ranked_values.append(RankedValue(Rank.NONE, empty_val, empty_details)) + + for value, ranks in derivation: + if len(ranks) == 0: + rank = Rank.NONE + details = None + else: + rank = ranks[-1] + # Again, distinguishing between '' vs None in the details field + # does not matter, but we want to be consistent with the idiosyncratic + # behavior of the legacy parser, until we get rid of it. + details = rank.description() or empty_details + ranked_values.append(RankedValue(rank, value, details)) + history = OptionValueHistory(tuple(ranked_values)) + # history = parser.history(kwargs["dest"]) ohi = self.get_option_help_info(args, kwargs) ohi = dataclasses.replace(ohi, value_history=history) if ohi.deprecation_active: diff --git a/src/python/pants/help/help_info_extracter_test.py b/src/python/pants/help/help_info_extracter_test.py index d0722b4b635..d8105bb3e88 100644 --- a/src/python/pants/help/help_info_extracter_test.py +++ b/src/python/pants/help/help_info_extracter_test.py @@ -14,6 +14,7 @@ from pants.help.help_info_extracter import HelpInfoExtracter, pretty_print_type_hint, to_help_str from pants.option.config import Config from pants.option.global_options import GlobalOptions, LogLevelOption +from pants.option.native_options import NativeOptionParser from pants.option.option_types import BoolOption, IntOption, StrListOption from pants.option.options import Options from pants.option.parser import Parser @@ -103,9 +104,10 @@ def do_test(args, kwargs, expected_default_str): config=Config.load([]), scope_info=GlobalOptions.get_scope_info(), ) + native_parser = NativeOptionParser([], {}, [], allow_pantsrc=False, include_derivation=True) parser.register(*args, **kwargs) oshi = HelpInfoExtracter(parser.scope).get_option_scope_help_info( - "description", parser, False, "provider" + "description", parser, native_parser, False, "provider" ) assert oshi.description == "description" assert oshi.provider == "provider" @@ -202,8 +204,11 @@ def exp_to_len(exp): config=Config.load([]), scope_info=GlobalOptions.get_scope_info(), ) + native_parser = NativeOptionParser([], {}, [], allow_pantsrc=False, include_derivation=True) parser.register("--foo", **kwargs) - oshi = HelpInfoExtracter("").get_option_scope_help_info("", parser, False, "") + oshi = HelpInfoExtracter("").get_option_scope_help_info( + "", parser, native_parser, False, "" + ) assert exp_to_len(expected_basic) == len(oshi.basic) assert exp_to_len(expected_advanced) == len(oshi.advanced) @@ -260,6 +265,7 @@ class BazLibrary(Target): known_scope_infos=[Global.get_scope_info(), Foo.get_scope_info(), Bar.get_scope_info()], args=["./pants", "--backend-packages=['internal_plugins.releases']"], bootstrap_option_values=None, + include_derivation=True, ) Global.register_options_on_scope(options, UnionMembership({})) Foo.register_options_on_scope(options, UnionMembership({})) diff --git a/src/python/pants/help/help_tools_test.py b/src/python/pants/help/help_tools_test.py index a7fab517dd5..71ff0b1964c 100644 --- a/src/python/pants/help/help_tools_test.py +++ b/src/python/pants/help/help_tools_test.py @@ -11,6 +11,7 @@ from pants.help.maybe_color import MaybeColor from pants.option.config import Config from pants.option.global_options import GlobalOptions +from pants.option.native_options import NativeOptionParser from pants.option.parser import Parser @@ -23,23 +24,28 @@ def parser() -> Parser: ) +@pytest.fixture +def native_parser() -> NativeOptionParser: + return NativeOptionParser([], {}, [], allow_pantsrc=False, include_derivation=True) + + @pytest.fixture def extracter() -> HelpInfoExtracter: return HelpInfoExtracter("test") @pytest.fixture -def tool_info(extracter, parser) -> ToolHelpInfo: +def tool_info(extracter, parser, native_parser) -> ToolHelpInfo: parser.register("version", typ=str, default="1.0") parser.register("url-template", typ=str, default="https://download/{version}") - oshi = extracter.get_option_scope_help_info("Test description.", parser, False) + oshi = extracter.get_option_scope_help_info("Test description.", parser, native_parser, False) tool_info = ToolHelpInfo.from_option_scope_help_info(oshi) assert tool_info is not None return tool_info -def test_no_tool_help_info(extracter, parser) -> None: - oshi = extracter.get_option_scope_help_info("", parser, False) +def test_no_tool_help_info(extracter, parser, native_parser) -> None: + oshi = extracter.get_option_scope_help_info("", parser, native_parser, False) assert ToolHelpInfo.from_option_scope_help_info(oshi) is None diff --git a/src/python/pants/option/native_options.py b/src/python/pants/option/native_options.py index 57cf416ac55..b622e84b396 100644 --- a/src/python/pants/option/native_options.py +++ b/src/python/pants/option/native_options.py @@ -23,6 +23,26 @@ logger = logging.getLogger() +def parse_dest(*args, **kwargs): + """Return the dest for an option registration. + + If an explicit `dest` is specified, returns that and otherwise derives a default from the + option flags where '--foo-bar' -> 'foo_bar' and '-x' -> 'x'. + + The dest is used for: + - The name of the field containing the option value. + - The key in the config file. + - Computing the name of the env var used to set the option name. + """ + dest = kwargs.get("dest") + if dest: + return dest + # No explicit dest, so compute one based on the first long arg, or the short arg + # if that's all there is. + arg = next((a for a in args if a.startswith("--")), args[0]) + return arg.lstrip("-").replace("-", "_") + + class NativeOptionParser: """A Python wrapper around the Rust options parser.""" @@ -41,7 +61,16 @@ def __init__( env: Mapping[str, str], config_sources: Optional[Sequence[ConfigSource]], allow_pantsrc: bool, + include_derivation: bool, ): + # Remember these args so this object can clone itself in with_derivation() below. + self._args, self._env, self._config_sources, self._allow_pantsrc = ( + args, + env, + config_sources, + allow_pantsrc, + ) + py_config_sources = ( None if config_sources is None @@ -52,6 +81,7 @@ def __init__( dict(get_strict_env(env, logger)), py_config_sources, allow_pantsrc, + include_derivation, ) # (type, member_type) -> native get for that type. @@ -67,7 +97,46 @@ def __init__( (dict, None): self._native_parser.get_dict, } - def get( + def with_derivation(self) -> NativeOptionParser: + """Return a clone of this object but with value derivation enabled.""" + # We may be able to get rid of this method once we remove the legacy parser entirely. + # For now it is convenient to allow the help mechanism to get derivations via the + # existing Options object, which otherwise does not need derivations. + return NativeOptionParser( + args=None if self._args is None else tuple(self._args), + env=dict(self._env), + config_sources=None if self._config_sources is None else tuple(self._config_sources), + allow_pantsrc=self._allow_pantsrc, + include_derivation=True, + ) + + def get_value(self, *, scope, registration_args, registration_kwargs) -> Tuple[Any, Rank]: + val, rank, _ = self._get_value_and_derivation(scope, registration_args, registration_kwargs) + return (val, rank) + + def get_derivation( + self, *, scope, registration_args, registration_kwargs + ) -> list[Tuple[Any, list[Rank]]]: + _, _, derivation = self._get_value_and_derivation( + scope, registration_args, registration_kwargs + ) + return derivation + + def _get_value_and_derivation( + self, scope, registration_args, registration_kwargs + ) -> Tuple[Any, Rank, list[Tuple[Any, list[Rank]]]]: + return self._get( + scope=scope, + dest=parse_dest(*registration_args, **registration_kwargs), + flags=registration_args, + default=registration_kwargs.get("default"), + option_type=registration_kwargs.get("type"), + member_type=registration_kwargs.get("member_type"), + choices=registration_kwargs.get("choices"), + passthrough=registration_kwargs.get("passthrough"), + ) + + def _get( self, *, scope, @@ -78,7 +147,7 @@ def get( member_type=None, choices=None, passthrough=False, - ) -> Tuple[Any, Rank]: + ) -> Tuple[Any, Rank, list[Tuple[Any, list[Rank]]]]: def scope_str() -> str: return "global scope" if scope == GLOBAL_SCOPE else f"scope '{scope}'" @@ -150,19 +219,28 @@ def apply_callable(callable_type, val_str): suffix = f" with member type {rust_member_type}" if rust_option_type is list else "" raise OptionsError(f"Unsupported type: {rust_option_type}{suffix}") - val, rank_int = getter(option_id, default) # type:ignore + val, rank_int, derivation = getter(option_id, default) # type:ignore rank = self.int_to_rank[rank_int] - if val is not None: + def process_value(v): if option_type is list: if member_type == shell_str: - val = _flatten_shlexed_list(val) + v = _flatten_shlexed_list(v) elif callable(member_type): - val = [apply_callable(member_type, x) for x in val] + v = [apply_callable(member_type, x) for x in v] if passthrough: - val += self._native_parser.get_passthrough_args() or [] + v += self._native_parser.get_passthrough_args() or [] elif callable(option_type): - val = apply_callable(option_type, val) + v = apply_callable(option_type, v) + return v + + if derivation: + derivation = [ + (process_value(v), [self.int_to_rank[r] for r in rs]) for (v, rs) in derivation + ] + + if val is not None: + val = process_value(val) # Validate the value. @@ -194,7 +272,7 @@ def check_scalar_value(val, choices): else: check_scalar_value(val, choices) - return (val, rank) + return (val, rank, derivation) def get_unconsumed_flags(self) -> dict[str, tuple[str, ...]]: return {k: tuple(v) for k, v in self._native_parser.get_unconsumed_flags().items()} diff --git a/src/python/pants/option/options.py b/src/python/pants/option/options.py index 692bf858b27..20fce7d0bbf 100644 --- a/src/python/pants/option/options.py +++ b/src/python/pants/option/options.py @@ -121,6 +121,7 @@ def create( # for user-facing behavior. native_options_validation: NativeOptionsValidation = NativeOptionsValidation.error, native_options_config_discovery: bool = True, + include_derivation: bool = False, ) -> Options: """Create an Options instance. @@ -134,6 +135,7 @@ def create( :param native_options_validation: How to validate the native options parser against the legacy Python parser. :param native_options_config_discovery: Whether to discover config files in the native parser or use the ones supplied + :param include_derivation: Whether to gather option value derivation information. """ # We need parsers for all the intermediate scopes, so inherited option values # can propagate through them. @@ -167,8 +169,13 @@ def create( known_scope_to_info = {s.scope: s for s in complete_known_scope_infos} config_to_pass = None if native_options_config_discovery else config.sources() + native_parser = NativeOptionParser( - args, env, config_sources=config_to_pass, allow_pantsrc=True + args, + env, + config_sources=config_to_pass, + allow_pantsrc=True, + include_derivation=include_derivation, ) return cls( @@ -218,6 +225,10 @@ def __init__( self._known_scope_to_info = known_scope_to_info self._allow_unknown_options = allow_unknown_options + @property + def native_parser(self) -> NativeOptionParser: + return self._native_parser + @property def specs(self) -> list[str]: """The specifications to operate on, e.g. the target addresses and the file names. diff --git a/src/python/pants/option/parser.py b/src/python/pants/option/parser.py index 85c88e56966..90c6a7c1b30 100644 --- a/src/python/pants/option/parser.py +++ b/src/python/pants/option/parser.py @@ -49,7 +49,12 @@ RegistrationError, UnknownFlagsError, ) -from pants.option.native_options import NativeOptionParser, check_dir_exists, check_file_exists +from pants.option.native_options import ( + NativeOptionParser, + check_dir_exists, + check_file_exists, + parse_dest, +) from pants.option.option_util import is_dict_option, is_list_option from pants.option.option_value_container import OptionValueContainer, OptionValueContainerBuilder from pants.option.ranked_value import Rank, RankedValue @@ -61,7 +66,7 @@ @dataclass(frozen=True) class OptionValueHistory: - ranked_values: tuple[RankedValue] + ranked_values: tuple[RankedValue, ...] @property def final_value(self) -> RankedValue: @@ -203,15 +208,8 @@ def parse_args_native( for args, kwargs in self._option_registrations: self._validate(args, kwargs) dest = self.parse_dest(*args, **kwargs) - val, rank = native_parser.get( - scope=self.scope, - dest=dest, - flags=args, - default=kwargs.get("default"), - option_type=kwargs.get("type"), - member_type=kwargs.get("member_type"), - choices=kwargs.get("choices"), - passthrough=kwargs.get("passthrough"), + val, rank = native_parser.get_value( + scope=self.scope, registration_args=args, registration_kwargs=kwargs ) # If the option is explicitly given, check mutual exclusion. @@ -534,13 +532,7 @@ def parse_dest(*args, **kwargs): - The key in the config file. - Computing the name of the env var used to set the option name. """ - dest = kwargs.get("dest") - if dest: - return dest - # No explicit dest, so compute one based on the first long arg, or the short arg - # if that's all there is. - arg = next((a for a in args if a.startswith("--")), args[0]) - return arg.lstrip("-").replace("-", "_") + return parse_dest(*args, **kwargs) @staticmethod def _convert_member_type(member_type, value): diff --git a/src/python/pants/option/ranked_value.py b/src/python/pants/option/ranked_value.py index bb65f61b948..eb6fa13bf8d 100644 --- a/src/python/pants/option/ranked_value.py +++ b/src/python/pants/option/ranked_value.py @@ -33,6 +33,18 @@ def __lt__(self, other: Any) -> bool: return NotImplemented return self._rank < other._rank + def description(self) -> Optional[str]: + """The source descriptions used to display option value derivation to users.""" + # These specific strings are for compatibility with the legacy parser's tests. + # We may revisit them once that is gone. + if self == Rank.CONFIG: + return "from config" + if self == Rank.ENVIRONMENT: + return "from an env var" + if self == Rank.FLAG: + return "from command-line flag" + return None + Value = Union[str, int, float, None, Dict, Enum, List] ValueAndDetails = Tuple[Optional[Value], Optional[str]] diff --git a/src/rust/engine/options/src/lib.rs b/src/rust/engine/options/src/lib.rs index f81fe69787d..46ba36fbcab 100644 --- a/src/rust/engine/options/src/lib.rs +++ b/src/rust/engine/options/src/lib.rs @@ -225,6 +225,44 @@ impl Source { } } +pub fn apply_list_edits( + remover: fn(&mut Vec, &[T]), + list_edits: impl Iterator>, +) -> Vec { + let mut list = vec![]; + // Removals from any source apply after adds from any source (but are themselves + // overridden by later replacements), so we collect them here and apply them later. + let mut removal_lists: Vec> = vec![]; + + for list_edit in list_edits { + match list_edit.action { + ListEditAction::Replace => { + list = list_edit.items; + removal_lists.clear(); + } + ListEditAction::Add => list.extend(list_edit.items), + ListEditAction::Remove => removal_lists.push(list_edit.items), + } + } + + for removals in removal_lists { + remover(&mut list, &removals); + } + + list +} + +pub fn apply_dict_edits(dict_edits: impl Iterator) -> HashMap { + let mut dict = HashMap::new(); + for dict_edit in dict_edits { + match dict_edit.action { + DictEditAction::Replace => dict = dict_edit.items, + DictEditAction::Add => dict.extend(dict_edit.items), + } + } + dict +} + #[derive(Debug)] pub struct OptionValue { pub derivation: Option>, @@ -522,16 +560,15 @@ impl OptionParser { id: &OptionId, default: Vec, getter: fn(&Arc, &OptionId) -> Result>>, String>, - remover: fn(&mut Vec, &Vec), + remover: fn(&mut Vec, &[T]), ) -> Result, String> { - let mut list = default; let mut derivation = None; if self.include_derivation { let mut derivations = vec![( Source::Default, vec![ListEdit { action: ListEditAction::Replace, - items: list.clone(), + items: default.clone(), }], )]; for (source_type, source) in self.sources.iter() { @@ -544,33 +581,22 @@ impl OptionParser { derivation = Some(derivations); } - // Removals from any source apply after adds from any source (but are themselves - // overridden by later replacements), so we collect them here and apply them later. - let mut removal_lists: Vec> = vec![]; - let mut highest_priority_source = Source::Default; + let mut edits: Vec> = vec![ListEdit { + action: ListEditAction::Replace, + items: default, + }]; for (source_type, source) in self.sources.iter() { if let Some(list_edits) = getter(source, id)? { highest_priority_source = source_type.clone(); - for list_edit in list_edits { - match list_edit.action { - ListEditAction::Replace => { - list = list_edit.items; - removal_lists.clear(); - } - ListEditAction::Add => list.extend(list_edit.items), - ListEditAction::Remove => removal_lists.push(list_edit.items), - } - } + edits.extend(list_edits); } } - for removals in removal_lists { - remover(&mut list, &removals); - } + Ok(ListOptionValue { derivation, source: highest_priority_source, - value: list, + value: apply_list_edits(remover, edits.into_iter()), }) } @@ -638,14 +664,13 @@ impl OptionParser { id: &OptionId, default: HashMap, ) -> Result { - let mut dict = default; let mut derivation = None; if self.include_derivation { let mut derivations = vec![( Source::Default, vec![DictEdit { action: DictEditAction::Replace, - items: dict.clone(), + items: default.clone(), }], )]; for (source_type, source) in self.sources.iter() { @@ -656,21 +681,20 @@ impl OptionParser { derivation = Some(derivations); } let mut highest_priority_source = Source::Default; + let mut edits: Vec = vec![DictEdit { + action: DictEditAction::Replace, + items: default, + }]; for (source_type, source) in self.sources.iter() { if let Some(dict_edits) = source.get_dict(id)? { highest_priority_source = source_type.clone(); - for dict_edit in dict_edits { - match dict_edit.action { - DictEditAction::Replace => dict = dict_edit.items, - DictEditAction::Add => dict.extend(dict_edit.items), - } - } + edits.extend(dict_edits); } } Ok(DictOptionValue { derivation, source: highest_priority_source, - value: dict, + value: apply_dict_edits(edits.into_iter()), }) } diff --git a/src/rust/engine/src/externs/options.rs b/src/rust/engine/src/externs/options.rs index f4af0f23706..01eaeeeda35 100644 --- a/src/rust/engine/src/externs/options.rs +++ b/src/rust/engine/src/externs/options.rs @@ -5,8 +5,9 @@ use pyo3::prelude::*; use pyo3::types::{PyBool, PyDict, PyFloat, PyInt, PyList, PyString, PyTuple}; use options::{ - Args, ConfigSource, Env, ListOptionValue, OptionId, OptionParser, OptionalOptionValue, Scope, - Val, + apply_dict_edits, apply_list_edits, Args, ConfigSource, DictEdit, DictEditAction, Env, + ListEdit, ListEditAction, ListOptionValue, OptionId, OptionParser, OptionalOptionValue, Scope, + Source, Val, }; use std::collections::HashMap; @@ -20,6 +21,8 @@ pub(crate) fn register(m: &Bound<'_, PyModule>) -> PyResult<()> { Ok(()) } +// The (nested) values of a dict-valued option are represented by Val. +// This function converts them to equivalent Python types. fn val_to_py_object(py: Python, val: &Val) -> PyResult { let res = match val { Val::Bool(b) => b.into_py(py), @@ -44,6 +47,18 @@ fn val_to_py_object(py: Python, val: &Val) -> PyResult { Ok(res) } +// Converts a string->Val dict into a Python type. +fn dict_into_py(py: Python, vals: HashMap) -> PyResult { + vals.into_iter() + .map(|(k, v)| match val_to_py_object(py, &v) { + Ok(pyobj) => Ok((k, pyobj)), + Err(err) => Err(err), + }) + .collect::>>() +} + +// Converts a Python object into a Val, which is necessary for receiving +// the default values of dict-valued options from Python. pub(crate) fn py_object_to_val(obj: &Bound<'_, PyAny>) -> Result { // TODO: If this is_instance_of chain shows up as significant in CPU profiles, // we can use a lookup table of PyTypeObject -> conversion func instead. @@ -146,11 +161,121 @@ impl PyConfigSource { #[pyclass] struct PyOptionParser(OptionParser); -type RankedVal = (T, isize); +// The pythonic value of a dict-typed option. +type PyDictVal = HashMap; + +// The derivation of the option value, as a vec of (value, vec of source ranks) tuples. +// A scalar value will always have a single source. A list/dict value may have elements +// appended across multiple sources. +// In ascending rank order, so the last value is the final value of the option. +type OptionValueDerivation = Vec<(T, Vec)>; + +// A tuple (final value, rank of final value, optional derivation of value). +// +// Note: The final value and its rank could be computed from the derivation (see above), +// but the full derivation is not itself computed in normal usage. +// We could get rid of this tuple type by representing the final value and its rank as +// a singleton derivation (in the case where we don't otherwise need the full derivation). +// But that would allocate two unnecessary Vecs for every option. +type OptionValue = (Option, isize, Option>); + +// Condense list value derivation across sources, so that it reflects merges vs. replacements +// in a useful way. E.g., if we merge [a, b] and [c], and then replace it with [d, e], +// the derivation will show: +// - [d, e] (from command-line flag) +// - [a, b, c] (from env var, from config) +fn condense_list_value_derivation( + derivation: Vec<(Source, Vec>)>, +) -> OptionValueDerivation> { + let mut ret: OptionValueDerivation> = vec![]; + let mut cur_group = vec![]; + let mut cur_ranks = vec![]; + + // In this case, for simplicity, we always use the "inefficient" O(M*N) remover, + // even for hashable values. This is very unlikely to have a noticeable performance impact + // in practice. And if it does, it would only be when we generate option value derivation + // for help display, and not in regular usage. + // See comments on OptionParser::parse_list_hashable() for context. + fn remover(list: &mut Vec, to_remove: &[T]) { + list.retain(|item| !to_remove.contains(item)); + } + + for (source, list_edits) in derivation.into_iter() { + for list_edit in list_edits { + if list_edit.action == ListEditAction::Replace { + if !cur_group.is_empty() { + ret.push(( + apply_list_edits::(remover, cur_group.into_iter()), + cur_ranks, + )); + } + cur_group = vec![]; + cur_ranks = vec![]; + } + cur_group.push(list_edit); + cur_ranks.push(source.rank() as isize); + } + } + if !cur_group.is_empty() { + ret.push(( + apply_list_edits::(remover, cur_group.into_iter()), + cur_ranks, + )); + } + + ret +} -fn to_py(res: Result, String>) -> PyResult>> { +// Condense dict value derivation across sources, so that it reflects merges vs. replacements +// in a useful way. E.g., if we merge {a: 1, b: 2] and {c: 3}, and then replace it with {d: 4}, +// the derivation will show: +// - {d: 4} (from command-line flag) +// - {a: 1, b: 2, c: 3} (from env var, from config) +fn condense_dict_value_derivation( + py: Python, + derivation: Vec<(Source, Vec)>, +) -> PyResult> { + let mut ret: OptionValueDerivation = vec![]; + let mut cur_group = vec![]; + let mut cur_ranks = vec![]; + + for (source, dict_edits) in derivation.into_iter() { + for dict_edit in dict_edits { + if dict_edit.action == DictEditAction::Replace { + if !cur_group.is_empty() { + ret.push(( + dict_into_py(py, apply_dict_edits(cur_group.into_iter()))?, + cur_ranks, + )); + } + cur_group = vec![]; + cur_ranks = vec![]; + } + cur_group.push(dict_edit); + cur_ranks.push(source.rank() as isize); + } + } + if !cur_group.is_empty() { + ret.push(( + dict_into_py(py, apply_dict_edits(cur_group.into_iter()))?, + cur_ranks, + )); + } + + Ok(ret) +} + +fn into_py(res: Result, String>) -> PyResult> { let val = res.map_err(ParseError::new_err)?; - Ok((val.value, val.source.rank() as isize)) + Ok(( + val.value, + val.source.rank() as isize, + val.derivation.map(|d| { + d.into_iter() + .map(|(source, val)| (val, vec![source.rank() as isize])) + .collect() + }), + )) } #[allow(clippy::type_complexity)] @@ -164,22 +289,30 @@ impl PyOptionParser { &OptionId, Vec, ) -> Result, String>, - ) -> PyResult<(Vec, isize)> { + ) -> PyResult>> + where + ::Owned: PartialEq, + { let opt_val = getter(&self.0, &option_id.borrow().0, default).map_err(ParseError::new_err)?; - Ok((opt_val.value, opt_val.source.rank() as isize)) + Ok(( + Some(opt_val.value), + opt_val.source.rank() as isize, + opt_val.derivation.map(condense_list_value_derivation), + )) } } #[pymethods] impl PyOptionParser { #[new] - #[pyo3(signature = (args, env, configs, allow_pantsrc))] + #[pyo3(signature = (args, env, configs, allow_pantsrc, include_derivation))] fn __new__<'py>( args: Vec, env: &Bound<'py, PyDict>, configs: Option>>, allow_pantsrc: bool, + include_derivation: bool, ) -> PyResult { let env = env .items() @@ -192,7 +325,7 @@ impl PyOptionParser { Env::new(env), configs.map(|cs| cs.iter().map(|c| c.borrow().0.clone()).collect()), allow_pantsrc, - false, + include_derivation, None, ) .map_err(ParseError::new_err)?; @@ -204,8 +337,8 @@ impl PyOptionParser { &self, option_id: &Bound<'_, PyOptionId>, default: Option, - ) -> PyResult>> { - to_py(self.0.parse_bool_optional(&option_id.borrow().0, default)) + ) -> PyResult> { + into_py(self.0.parse_bool_optional(&option_id.borrow().0, default)) } #[pyo3(signature = (option_id, default))] @@ -213,8 +346,8 @@ impl PyOptionParser { &self, option_id: &Bound<'_, PyOptionId>, default: Option, - ) -> PyResult>> { - to_py(self.0.parse_int_optional(&option_id.borrow().0, default)) + ) -> PyResult> { + into_py(self.0.parse_int_optional(&option_id.borrow().0, default)) } #[pyo3(signature = (option_id, default))] @@ -222,8 +355,8 @@ impl PyOptionParser { &self, option_id: &Bound<'_, PyOptionId>, default: Option, - ) -> PyResult>> { - to_py(self.0.parse_float_optional(&option_id.borrow().0, default)) + ) -> PyResult> { + into_py(self.0.parse_float_optional(&option_id.borrow().0, default)) } #[pyo3(signature = (option_id, default))] @@ -231,15 +364,15 @@ impl PyOptionParser { &self, option_id: &Bound<'_, PyOptionId>, default: Option<&str>, - ) -> PyResult>> { - to_py(self.0.parse_string_optional(&option_id.borrow().0, default)) + ) -> PyResult> { + into_py(self.0.parse_string_optional(&option_id.borrow().0, default)) } fn get_bool_list( &self, option_id: &Bound<'_, PyOptionId>, default: Vec, - ) -> PyResult>> { + ) -> PyResult>> { self.get_list::(option_id, default, |op, oid, def| { op.parse_bool_list(oid, def) }) @@ -249,7 +382,7 @@ impl PyOptionParser { &self, option_id: &Bound<'_, PyOptionId>, default: Vec, - ) -> PyResult>> { + ) -> PyResult>> { self.get_list::(option_id, default, |op, oid, def| { op.parse_int_list(oid, def) }) @@ -259,7 +392,7 @@ impl PyOptionParser { &self, option_id: &Bound<'_, PyOptionId>, default: Vec, - ) -> PyResult>> { + ) -> PyResult>> { self.get_list::(option_id, default, |op, oid, def| { op.parse_float_list(oid, def) }) @@ -269,7 +402,7 @@ impl PyOptionParser { &self, option_id: &Bound<'_, PyOptionId>, default: Vec, - ) -> PyResult>> { + ) -> PyResult>> { self.get_list::(option_id, default, |op, oid, def| { op.parse_string_list(oid, def) }) @@ -280,7 +413,7 @@ impl PyOptionParser { py: Python, option_id: &Bound<'_, PyOptionId>, default: &Bound<'_, PyDict>, - ) -> PyResult>> { + ) -> PyResult> { let default = default .items() .into_iter() @@ -289,19 +422,21 @@ impl PyOptionParser { Ok::<(String, Val), PyErr>((k, py_object_to_val(&v)?)) }) .collect::, _>>()?; + let opt_val = self .0 .parse_dict(&option_id.borrow().0, default) .map_err(ParseError::new_err)?; - let opt_val_py = opt_val - .value - .into_iter() - .map(|(k, v)| match val_to_py_object(py, &v) { - Ok(pyobj) => Ok((k, pyobj)), - Err(err) => Err(err), - }) - .collect::>>()?; - Ok((opt_val_py, opt_val.source.rank() as isize)) + let opt_val_py = dict_into_py(py, opt_val.value)?; + + Ok(( + Some(opt_val_py), + opt_val.source.rank() as isize, + match opt_val.derivation { + Some(d) => Some(condense_dict_value_derivation(py, d)?), + None => None, + }, + )) } fn get_passthrough_args(&self) -> PyResult>> {