From 34f113a0e1b67359d1cb9310562910d0297a7188 Mon Sep 17 00:00:00 2001 From: Benjy Weinberger Date: Sun, 10 Nov 2024 19:51:54 -0800 Subject: [PATCH] Use option value derivation from native parser. (#21627) The derivation (previously inaptly called "history" on the python side) is displayed to users in option help. It shows what the option value provided by each source type (config/env/flag) was, as well as the final value (from the highest ranked source). For scalar values this is relatively simple, as values can only be overridden. For list and dict values this is more complicated, as values from one source can be appended to by another source. So a final value (or any intermediate value) may come from multiple sources. The goal in this PR was to be compatible with the existing help system tests, so we can be confident that we have equivalent behavior. Once the legacy parser is gone we can consider streamlining parts of this. --- .../pants/engine/internals/native_engine.pyi | 24 ++- src/python/pants/help/help_formatter_test.py | 26 ++- src/python/pants/help/help_info_extracter.py | 36 +++- .../pants/help/help_info_extracter_test.py | 10 +- src/python/pants/help/help_tools_test.py | 14 +- src/python/pants/option/native_options.py | 96 ++++++++- src/python/pants/option/options.py | 13 +- src/python/pants/option/parser.py | 28 +-- src/python/pants/option/ranked_value.py | 12 ++ src/rust/engine/options/src/lib.rs | 84 +++++--- src/rust/engine/src/externs/options.rs | 197 +++++++++++++++--- 11 files changed, 427 insertions(+), 113 deletions(-) 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>> {