From 2de9e71fbf1cfd8d7bb03de0524849d884e2a8ba Mon Sep 17 00:00:00 2001 From: Benjy Weinberger Date: Sat, 7 Aug 2021 13:22:48 -0700 Subject: [PATCH] Remove the recursive options and option inheritance features. (#12519) This is not needed in v2, and simplifies the options handling code and tests quite a bit. It also simplifies things for end users, who don't have to understand these concepts any more. This should also improve invalidation, since scopes don't all inherit global scope options any more. [ci skip-rust] [ci skip-build-wheels] --- src/python/pants/goal/run_tracker.py | 2 +- src/python/pants/help/help_info_extracter.py | 7 +- .../pants/help/help_info_extracter_test.py | 2 - src/python/pants/option/errors.py | 11 - src/python/pants/option/options.py | 103 ++---- src/python/pants/option/options_test.py | 343 ++---------------- src/python/pants/option/parser.py | 92 +---- 7 files changed, 75 insertions(+), 485 deletions(-) diff --git a/src/python/pants/goal/run_tracker.py b/src/python/pants/goal/run_tracker.py index a405c839bf4..b995ab411e6 100644 --- a/src/python/pants/goal/run_tracker.py +++ b/src/python/pants/goal/run_tracker.py @@ -215,7 +215,7 @@ def _get_option_to_record(self, scope, option=None): scope_to_look_up = scope if scope != GLOBAL_SCOPE_CONFIG_SECTION else "" try: value = self._all_options.for_scope( - scope_to_look_up, inherit_from_enclosing_scope=False + scope_to_look_up, check_deprecations=False ).as_dict() if option is None: return value diff --git a/src/python/pants/help/help_info_extracter.py b/src/python/pants/help/help_info_extracter.py index 5d40a8549f7..35ae8fdd67e 100644 --- a/src/python/pants/help/help_info_extracter.py +++ b/src/python/pants/help/help_info_extracter.py @@ -360,12 +360,7 @@ def get_option_scope_help_info(self, description: str, parser: Parser, is_goal: ohi = dataclasses.replace(ohi, value_history=history) if ohi.deprecation_active: deprecated_options.append(ohi) - elif kwargs.get("advanced") or ( - kwargs.get("recursive") and not kwargs.get("recursive_root") - ): - # In order to keep the regular help output uncluttered, we treat recursive - # options as advanced. The concept of recursive options is not widely used - # and not clear to the end user, so it's best not to expose it as a concept. + elif kwargs.get("advanced"): advanced_options.append(ohi) else: basic_options.append(ohi) diff --git a/src/python/pants/help/help_info_extracter_test.py b/src/python/pants/help/help_info_extracter_test.py index 40940125c20..205c1b39a65 100644 --- a/src/python/pants/help/help_info_extracter_test.py +++ b/src/python/pants/help/help_info_extracter_test.py @@ -221,8 +221,6 @@ def exp_to_len(exp): do_test({}, expected_basic=True) do_test({"advanced": False}, expected_basic=True) do_test({"advanced": True}, expected_advanced=True) - do_test({"recursive_root": True}, expected_basic=True) - do_test({"advanced": True, "recursive_root": True}, expected_advanced=True) def test_get_all_help_info(): diff --git a/src/python/pants/option/errors.py b/src/python/pants/option/errors.py index b4c613caaf8..e827a40254e 100644 --- a/src/python/pants/option/errors.py +++ b/src/python/pants/option/errors.py @@ -87,17 +87,6 @@ class PassthroughType(RegistrationError): """Options marked passthrough must be typed as a string list.""" -class RecursiveSubsystemOption(RegistrationError): - """Subsystem option cannot specify 'recursive'. - - Subsystem options are always recursive. - """ - - -class Shadowing(RegistrationError): - """Option shadows an option in {outer_scope}.""" - - # ----------------------------------------------------------------------- # Flag parsing errors # ----------------------------------------------------------------------- diff --git a/src/python/pants/option/options.py b/src/python/pants/option/options.py index ac6d5e2bcba..5d3c60e3521 100644 --- a/src/python/pants/option/options.py +++ b/src/python/pants/option/options.py @@ -14,7 +14,7 @@ from pants.option.option_util import is_list_option from pants.option.option_value_container import OptionValueContainer, OptionValueContainerBuilder from pants.option.parser import Parser -from pants.option.parser_hierarchy import ParserHierarchy, all_enclosing_scopes, enclosing_scope +from pants.option.parser_hierarchy import ParserHierarchy, all_enclosing_scopes from pants.option.scope import GLOBAL_SCOPE, GLOBAL_SCOPE_CONFIG_SECTION, ScopeInfo from pants.util.memo import memoized_method from pants.util.ordered_set import FrozenOrderedSet, OrderedSet @@ -86,7 +86,7 @@ def complete_scopes(cls, scope_infos: Iterable[ScopeInfo]) -> FrozenOrderedSet[S """ ret: OrderedSet[ScopeInfo] = OrderedSet() original_scopes: Dict[str, ScopeInfo] = {} - for si in sorted(scope_infos, key=lambda si: si.scope): + for si in sorted(scope_infos, key=lambda _si: _si.scope): ret.add(si) if si.scope in original_scopes: raise cls.DuplicateScopeError( @@ -126,6 +126,7 @@ def create( :param args: a list of cmd-line args; defaults to `sys.argv` if None is supplied. :param bootstrap_option_values: An optional namespace containing the values of bootstrap options. We can use these values when registering other options. + :param allow_unknown_options: Whether to ignore or error on unknown cmd-line flags. """ # We need parsers for all the intermediate scopes, so inherited option values # can propagate through them. @@ -242,12 +243,7 @@ def verify_configs(self, global_config: Config) -> None: for section in config.sections(): scope = GLOBAL_SCOPE if section == GLOBAL_SCOPE_CONFIG_SECTION else section try: - # TODO(#10834): this is broken for subscopes. Once we fix global options to no - # longer be included in self.for_scope(), we should set - # inherit_from_enclosing_scope=True. - valid_options_under_scope = set( - self.for_scope(scope, inherit_from_enclosing_scope=False) - ) + valid_options_under_scope = set(self.for_scope(scope, check_deprecations=False)) # Only catch ConfigValidationError. Other exceptions will be raised directly. except Config.ConfigValidationError: error_log.append(f"Invalid scope [{section}] in {config.config_path}") @@ -270,26 +266,6 @@ def verify_configs(self, global_config: Config) -> None: "remove.\n(Specify --no-verify-config to disable this check.)" ) - def drop_flag_values(self) -> Options: - """Returns a copy of these options that ignores values specified via flags. - - Any pre-cached option values are cleared and only option values that come from option - defaults, the config or the environment are used. - """ - # An empty scope_to_flags to force all values to come via the config -> env hierarchy alone - # and empty values in case we already cached some from flags. - no_flags: Dict[str, List[str]] = {} - return Options( - goals=self._goals, - scope_to_flags=no_flags, - specs=self._specs, - passthru=self._passthru, - help_request=self._help_request, - parser_hierarchy=self._parser_hierarchy, - bootstrap_option_values=self._bootstrap_option_values, - known_scope_to_info=self._known_scope_to_info, - ) - def is_known_scope(self, scope: str) -> bool: """Whether the given scope is known by this instance. @@ -349,9 +325,7 @@ def _check_and_apply_deprecations(self, scope, values): # If this Scope is itself deprecated, report that. if si.removal_version: - explicit_keys = self.for_scope( - scope, inherit_from_enclosing_scope=False - ).get_explicit_keys() + explicit_keys = self.for_scope(scope, check_deprecations=False).get_explicit_keys() if explicit_keys: warn_or_error( removal_version=si.removal_version, @@ -365,14 +339,13 @@ def _check_and_apply_deprecations(self, scope, values): # check that scope != deprecated_scope to prevent infinite recursion. deprecated_scope = si.deprecated_scope if deprecated_scope is not None and scope != deprecated_scope: - # Do the deprecation check only on keys that were explicitly set on the deprecated scope - # (and not on its enclosing scopes). + # Do the deprecation check only on keys that were explicitly set + # on the deprecated scope. explicit_keys = self.for_scope( - deprecated_scope, inherit_from_enclosing_scope=False + deprecated_scope, check_deprecations=False ).get_explicit_keys() if explicit_keys: - # Update our values with those of the deprecated scope (now including values inherited - # from its enclosing scope). + # Update our values with those of the deprecated scope. # Note that a deprecated val will take precedence over a val of equal rank. # This makes the code a bit neater. values.update(self.for_scope(deprecated_scope)) @@ -397,11 +370,7 @@ def _make_parse_args_request( # TODO: Eagerly precompute backing data for this? @memoized_method - def for_scope( - self, - scope: str, - inherit_from_enclosing_scope: bool = True, - ) -> OptionValueContainer: + def for_scope(self, scope: str, check_deprecations: bool = True) -> OptionValueContainer: """Return the option values for the given scope. Values are attributes of the returned object, e.g., options.foo. @@ -410,19 +379,13 @@ def for_scope( :API: public """ - # First get enclosing scope's option values, if any. - if scope == GLOBAL_SCOPE or not inherit_from_enclosing_scope: - values_builder = OptionValueContainerBuilder() - else: - values_builder = self.for_scope(enclosing_scope(scope)).to_builder() - - # Now add our values. + values_builder = OptionValueContainerBuilder() flags_in_scope = self._scope_to_flags.get(scope, []) parse_args_request = self._make_parse_args_request(flags_in_scope, values_builder) values = self._parser_hierarchy.get_parser_by_scope(scope).parse_args(parse_args_request) # Check for any deprecation conditions, which are evaluated using `self._flag_matchers`. - if inherit_from_enclosing_scope: + if check_deprecations: values_builder = values.to_builder() self._check_and_apply_deprecations(scope, values_builder) values = values_builder.build() @@ -431,7 +394,7 @@ def for_scope( def get_fingerprintable_for_scope( self, - bottom_scope: str, + scope: str, daemon_only: bool = False, ): """Returns a list of fingerprintable (option type, option value) pairs for the given scope. @@ -441,35 +404,25 @@ def get_fingerprintable_for_scope( This method also searches enclosing options scopes of `bottom_scope` to determine the set of fingerprintable pairs. - :param bottom_scope: The scope to gather fingerprintable options for. + :param scope: The scope to gather fingerprintable options for. :param daemon_only: If true, only look at daemon=True options. """ pairs = [] - - # Note that we iterate over options registered at `bottom_scope` and at all - # enclosing scopes, since option-using code can read those values indirectly - # via its own OptionValueContainer, so they can affect that code's output. - for registration_scope in all_enclosing_scopes(bottom_scope): - parser = self._parser_hierarchy.get_parser_by_scope(registration_scope) - # Sort the arguments, so that the fingerprint is consistent. - for (_, kwargs) in sorted(parser.option_registrations_iter()): - if kwargs.get("recursive", False) and not kwargs.get("recursive_root", False): - continue # We only need to fprint recursive options once. - if not kwargs.get("fingerprint", True): - continue - if daemon_only and not kwargs.get("daemon", False): - continue - # Note that we read the value from scope, even if the registration was on an enclosing - # scope, to get the right value for recursive options (and because this mirrors what - # option-using code does). - val = self.for_scope(bottom_scope)[kwargs["dest"]] - # If we have a list then we delegate to the fingerprinting implementation of the members. - if is_list_option(kwargs): - val_type = kwargs.get("member_type", str) - else: - val_type = kwargs.get("type", str) - pairs.append((val_type, val)) + parser = self._parser_hierarchy.get_parser_by_scope(scope) + # Sort the arguments, so that the fingerprint is consistent. + for (_, kwargs) in sorted(parser.option_registrations_iter()): + if not kwargs.get("fingerprint", True): + continue + if daemon_only and not kwargs.get("daemon", False): + continue + val = self.for_scope(scope)[kwargs["dest"]] + # If we have a list then we delegate to the fingerprinting implementation of the members. + if is_list_option(kwargs): + val_type = kwargs.get("member_type", str) + else: + val_type = kwargs.get("type", str) + pairs.append((val_type, val)) return pairs def __getitem__(self, scope: str) -> OptionValueContainer: diff --git a/src/python/pants/option/options_test.py b/src/python/pants/option/options_test.py index 42c41a6c8e3..3d21bad9523 100644 --- a/src/python/pants/option/options_test.py +++ b/src/python/pants/option/options_test.py @@ -38,8 +38,6 @@ OptionNameDash, OptionNameDoubleDash, ParseError, - RecursiveSubsystemOption, - Shadowing, ) from pants.option.global_options import GlobalOptions from pants.option.options import Options @@ -542,37 +540,6 @@ def register(opts: Options) -> None: assert not caplog.records -def test_scope_deprecation_dependency(caplog) -> None: - # Test that a dependency scope can be deprecated. - class Subsystem1(Subsystem): - options_scope = "scope" - - def register(opts: Options) -> None: - opts.register(Subsystem1.options_scope, "--foo") - - opts = create_options( - [GLOBAL_SCOPE], - register, - ["--scope-sub-foo=vv"], - extra_scope_infos=[ - Subsystem1.get_scope_info(), - # A deprecated, scoped dependency on `Subsystem1`. This - # imitates the construction of Subsystem.known_scope_infos. - ScopeInfo( - Subsystem1.subscope("sub"), - Subsystem1, - removal_version="9999.9.9.dev0", - removal_hint="Sayonara!", - ), - ], - ) - - caplog.clear() - assert opts.for_scope(Subsystem1.subscope("sub")).foo == "vv" - assert len(caplog.records) == 1 - assert Subsystem1.subscope("sub") in caplog.text - - class OptionsTest(unittest.TestCase): @staticmethod def _create_config(config: dict[str, dict[str, str]] | None = None) -> Config: @@ -603,10 +570,9 @@ def _parse( ScopeInfo(scope) for scope in ( GLOBAL_SCOPE, + "anotherscope", "compile", "compile.java", - "compile.scala", - "cache.compile.scala", "stale", "test", "test.junit", @@ -633,8 +599,11 @@ def _register(self, options): def register_global(*args, **kwargs): options.register(GLOBAL_SCOPE, *args, **kwargs) - register_global("-z", "--verbose", type=bool, help="Verbose output.", recursive=True) - register_global("-n", "--num", type=int, default=99, recursive=True) + register_global("-z", "--verbose", type=bool, help="Verbose output.") + register_global("-n", "--num", type=int, default=99) + # To test that we can use the same name on the global scope and another scope. + options.register("anotherscope", "-n", "--num", type=int, default=99) + register_global("--y", type=list, member_type=int) register_global( "--v2", help="Two-letter long-form option, used to test option name suggestions." @@ -671,10 +640,6 @@ def register_global(*args, **kwargs): # Implicit value. register_global("--implicit-valuey", default="default", implicit_value="implicit") - # For the design doc example test. - register_global("--a", type=int, recursive=True) - register_global("--b", type=int, recursive=True) - # Mutual Exclusive options register_global("--mutex-foo", mutually_exclusive_group="mutex") register_global("--mutex-bar", mutually_exclusive_group="mutex") @@ -683,20 +648,17 @@ def register_global(*args, **kwargs): register_global("--new-name") register_global("--old-name", mutually_exclusive_group="new_name") - # For the design doc example test. - options.register("compile", "--c", type=int) - # Test mutual exclusive options with a scope options.register("stale", "--mutex-a", mutually_exclusive_group="scope_mutex") options.register("stale", "--mutex-b", mutually_exclusive_group="scope_mutex") options.register("stale", "--crufty-old", mutually_exclusive_group="crufty_new") options.register("stale", "--crufty-new") - # For task identity test - options.register("compile.scala", "--modifycompile") - options.register("compile.scala", "--modifylogs", fingerprint=False) + # For scoped fingerprintable test + options.register("compile", "--modifycompile") + options.register("compile", "--modifylogs", fingerprint=False) options.register( - "compile.scala", + "compile", "--modifypassthrough", passthrough=True, type=list, @@ -718,12 +680,12 @@ def register_global(*args, **kwargs): options.register("fromfile", "--appendvalue", type=list, member_type=int) # For fingerprint tests - options.register(GLOBAL_SCOPE, "--implicitly-fingerprinted") - options.register(GLOBAL_SCOPE, "--explicitly-fingerprinted", fingerprint=True) - options.register(GLOBAL_SCOPE, "--explicitly-not-fingerprinted", fingerprint=False) - options.register(GLOBAL_SCOPE, "--implicitly-not-daemoned") - options.register(GLOBAL_SCOPE, "--explicitly-not-daemoned", daemon=False) - options.register(GLOBAL_SCOPE, "--explicitly-daemoned", daemon=True) + register_global("--implicitly-fingerprinted") + register_global("--explicitly-fingerprinted", fingerprint=True) + register_global("--explicitly-not-fingerprinted", fingerprint=False) + register_global("--implicitly-not-daemoned") + register_global("--explicitly-not-daemoned", daemon=False) + register_global("--explicitly-daemoned", daemon=True) # For enum tests options.register("enum-opt", "--some-enum", type=self.SomeEnumOption) @@ -772,21 +734,10 @@ def test_arg_scoping(self) -> None: with self.assertRaises(ParseError): self._parse(flags="--unregistered-option compile").for_global_scope() - # Scoping of different values of the same option. - # Also tests the --no-* boolean flag inverses. - options = self._parse(flags="--verbose compile.java --no-verbose") - self.assertEqual(True, options.for_global_scope().verbose) - self.assertEqual(True, options.for_scope("compile").verbose) - self.assertEqual(False, options.for_scope("compile.java").verbose) - - options = self._parse( - flags="--verbose compile --no-verbose compile.java -z test test.junit --no-verbose" - ) - self.assertEqual(True, options.for_global_scope().verbose) - self.assertEqual(False, options.for_scope("compile").verbose) - self.assertEqual(True, options.for_scope("compile.java").verbose) - self.assertEqual(True, options.for_scope("test").verbose) - self.assertEqual(False, options.for_scope("test.junit").verbose) + # Scoping of different values of options with the same name in different scopes. + options = self._parse(flags="--num=11 anotherscope --num=22") + self.assertEqual(11, options.for_global_scope().num) + self.assertEqual(22, options.for_scope("anotherscope").num) # Test list-typed option. global_options = self._parse(config={"DEFAULT": {"y": ["88", "-99"]}}).for_global_scope() @@ -1087,35 +1038,20 @@ def check( def test_defaults(self) -> None: # Hard-coded defaults. - options = self._parse(flags="compile.java -n33") - self.assertEqual(99, options.for_global_scope().num) - self.assertEqual(99, options.for_scope("compile").num) - self.assertEqual(33, options.for_scope("compile.java").num) - self.assertEqual(99, options.for_scope("test").num) - self.assertEqual(99, options.for_scope("test.junit").num) - - options = self._parse(flags="compile -n22 compile.java -n33") + options = self._parse(flags="anotherscope") self.assertEqual(99, options.for_global_scope().num) - self.assertEqual(22, options.for_scope("compile").num) - self.assertEqual(33, options.for_scope("compile.java").num) + self.assertEqual(99, options.for_scope("anotherscope").num) # Get defaults from config and environment. - config = {"DEFAULT": {"num": "88"}, "compile": {"num": "77"}, "compile.java": {"num": "66"}} - options = self._parse(flags="compile.java -n22", config=config) + config = {"DEFAULT": {"num": "88"}, "anotherscope": {"num": "77"}} + options = self._parse(flags="anotherscope", config=config) self.assertEqual(88, options.for_global_scope().num) - self.assertEqual(77, options.for_scope("compile").num) - self.assertEqual(22, options.for_scope("compile.java").num) + self.assertEqual(77, options.for_scope("anotherscope").num) - env = {"PANTS_COMPILE_NUM": "55"} - options = self._parse(flags="compile", env=env, config=config) + env = {"PANTS_ANOTHERSCOPE_NUM": "55"} + options = self._parse(flags="anotherscope", env=env, config=config) self.assertEqual(88, options.for_global_scope().num) - self.assertEqual(55, options.for_scope("compile").num) - self.assertEqual(55, options.for_scope("compile.java").num) - - options = self._parse(flags="compile.java -n44", env=env, config=config) - self.assertEqual(88, options.for_global_scope().num) - self.assertEqual(55, options.for_scope("compile").num) - self.assertEqual(44, options.for_scope("compile.java").num) + self.assertEqual(55, options.for_scope("anotherscope").num) def test_choices(self) -> None: options = self._parse(flags="--str-choices=foo") @@ -1179,82 +1115,12 @@ def test_shadowing(self) -> None: options.register("", "--opt1") options.register("foo", "-o", "--opt2") - def assert_raises_shadowing(*, scope: str, args: List[str]) -> None: - with self.assertRaises(Shadowing): - options.register(scope, *args) - - assert_raises_shadowing(scope="", args=["--opt2"]) - assert_raises_shadowing(scope="bar", args=["--opt1"]) - assert_raises_shadowing(scope="foo.bar", args=["--opt1"]) - assert_raises_shadowing(scope="foo.bar", args=["--opt2"]) - assert_raises_shadowing(scope="foo.bar", args=["--opt1", "--opt3"]) - assert_raises_shadowing(scope="foo.bar", args=["--opt3", "--opt2"]) - - def test_recursion(self) -> None: - # Recursive option. - options = self._parse(flags="-n=5 compile -n=6") - self.assertEqual(5, options.for_global_scope().num) - self.assertEqual(6, options.for_scope("compile").num) - - # Non-recursive option. - options = self._parse(flags="--bar-baz=foo") - self.assertEqual("foo", options.for_global_scope().bar_baz) - options = self._parse(flags="compile --bar-baz=foo") - with self.assertRaises(ParseError): - options.for_scope("compile") - - def test_no_recursive_subsystem_options(self) -> None: - options = Options.create( - env={}, - config=self._create_config(), - known_scope_infos=[global_scope(), subsystem("foo")], - args=["./pants"], - ) - # All subsystem options are implicitly recursive (a subscope of subsystem scope represents - # a separate instance of the subsystem, so it needs all the options). - # We disallow explicit specification of recursive (even if set to True), to avoid confusion. - with self.assertRaises(RecursiveSubsystemOption): - options.register("foo", "--bar", recursive=False) - options.for_scope("foo") - with self.assertRaises(RecursiveSubsystemOption): - options.register("foo", "--baz", recursive=True) - options.for_scope("foo") - def test_is_known_scope(self) -> None: options = self._parse() for scope_info in self._known_scope_infos: self.assertTrue(options.is_known_scope(scope_info.scope)) self.assertFalse(options.is_known_scope("nonexistent_scope")) - def test_designdoc_example(self) -> None: - # The example from the design doc. - # Get defaults from config and environment. - config = { - "DEFAULT": {"b": "99"}, - "compile": {"a": "88", "c": "77"}, - } - - env = {"PANTS_COMPILE_C": "66"} - - options = self._parse( - flags="--a=1 compile --b=2 compile.java --a=3 --c=4", - env=env, - config=config, - ) - - self.assertEqual(1, options.for_global_scope().a) - self.assertEqual(99, options.for_global_scope().b) - with self.assertRaises(AttributeError): - options.for_global_scope().c - - self.assertEqual(1, options.for_scope("compile").a) - self.assertEqual(2, options.for_scope("compile").b) - self.assertEqual(66, options.for_scope("compile").c) - - self.assertEqual(3, options.for_scope("compile.java").a) - self.assertEqual(2, options.for_scope("compile.java").b) - self.assertEqual(4, options.for_scope("compile.java").c) - def test_file_spec_args(self) -> None: with temporary_file(binary_mode=False) as tmp: tmp.write( @@ -1364,39 +1230,6 @@ def check_scoped_spam(scope, expected_val, env): check_scoped_spam("scoped.a.bit", "value", {"PANTS_SCOPED_A_BIT_SPAM": "value"}) check_scoped_spam("scoped.and-dashed", "value", {"PANTS_SCOPED_AND_DASHED_SPAM": "value"}) - def test_drop_flag_values(self) -> None: - options = self._parse( - flags="--bar-baz=fred -n33 --pants-foo=red enum-opt --some-enum=another-value simple -n1", - env={"PANTS_FOO": "BAR"}, - config={"simple": {"num": 42}, "enum-opt": {"some-enum": "a-value"}}, - ) - defaulted_only_options = options.drop_flag_values() - - # No option value supplied in any form. - self.assertEqual("fred", options.for_global_scope().bar_baz) - self.assertIsNone(defaulted_only_options.for_global_scope().bar_baz) - - # A defaulted option value. - self.assertEqual(33, options.for_global_scope().num) - self.assertEqual(99, defaulted_only_options.for_global_scope().num) - - # A config specified option value. - self.assertEqual(1, options.for_scope("simple").num) - self.assertEqual(42, defaulted_only_options.for_scope("simple").num) - - # An env var specified option value. - self.assertEqual("red", options.for_global_scope().pants_foo) - self.assertEqual("BAR", defaulted_only_options.for_global_scope().pants_foo) - - # Overriding an enum option value. - self.assertEqual(self.SomeEnumOption.another_value, options.for_scope("enum-opt").some_enum) - - # Getting the default value for an enum option. - self.assertEqual( - self.SomeEnumOption.a_value, - defaulted_only_options.for_scope("separate-enum-opt-scope").some_enum_with_default, - ) - def test_enum_option_type_parse_error(self) -> None: with pytest.raises(ParseError) as exc: options = self._parse(flags="enum-opt --some-enum=invalid-value") @@ -1409,7 +1242,7 @@ def test_enum_option_type_parse_error(self) -> None: def test_non_enum_option_type_parse_error(self) -> None: with pytest.raises(ParseError) as exc: - options = self._parse(flags="--a=not-a-number") + options = self._parse(flags="--num=not-a-number") options.for_global_scope() assert ( @@ -1490,70 +1323,6 @@ def assert_option_set( assert_option_set("--mutex-foo=orz", "mutex_baz", None) assert_option_set("--mutex-bar=orz", "mutex_bar", "orz") - def test_middle_scoped_options(self) -> None: - """Make sure the rules for inheriting from a hierarchy of scopes. - - Values should follow - 1. A short circuit scan for a value from the following sources in-order: - flags, env, config, hardcoded defaults - 2. Values for each source follow the . hierarchy scoping rule - within that source. - """ - - # Short circuit using command line. - options = self._parse(flags="--a=100 compile --a=99") - self.assertEqual(100, options.for_global_scope().a) - self.assertEqual(99, options.for_scope("compile").a) - self.assertEqual(99, options.for_scope("compile.java").a) - - options = self._parse(config={"DEFAULT": {"a": 100}, "compile": {"a": 99}}) - self.assertEqual(100, options.for_global_scope().a) - self.assertEqual(99, options.for_scope("compile").a) - self.assertEqual(99, options.for_scope("compile.java").a) - - options = self._parse(env={"PANTS_A": "100", "PANTS_COMPILE_A": "99"}) - self.assertEqual(100, options.for_global_scope().a) - self.assertEqual(99, options.for_scope("compile").a) - self.assertEqual(99, options.for_scope("compile.java").a) - - # Command line has precedence over config. - options = self._parse(flags="compile --a=99", config={"DEFAULT": {"a": 100}}) - self.assertEqual(100, options.for_global_scope().a) - self.assertEqual(99, options.for_scope("compile").a) - self.assertEqual(99, options.for_scope("compile.java").a) - - # Command line has precedence over environment. - options = self._parse(flags="compile --a=99", env={"PANTS_A": "100"}) - self.assertEqual(100, options.for_global_scope().a) - self.assertEqual(99, options.for_scope("compile").a) - self.assertEqual(99, options.for_scope("compile.java").a) - - # Env has precedence over config. - options = self._parse(config={"DEFAULT": {"a": 100}}, env={"PANTS_COMPILE_A": "99"}) - self.assertEqual(100, options.for_global_scope().a) - self.assertEqual(99, options.for_scope("compile").a) - self.assertEqual(99, options.for_scope("compile.java").a) - - # Command line global overrides the middle scope setting in then env. - options = self._parse(flags="--a=100", env={"PANTS_COMPILE_A": "99"}) - self.assertEqual(100, options.for_global_scope().a) - self.assertEqual(100, options.for_scope("compile").a) - self.assertEqual(100, options.for_scope("compile.java").a) - - # Command line global overrides the middle scope in config. - options = self._parse(flags="--a=100 ", config={"compile": {"a": 99}}) - self.assertEqual(100, options.for_global_scope().a) - self.assertEqual(100, options.for_scope("compile").a) - self.assertEqual(100, options.for_scope("compile.java").a) - - # Env global overrides the middle scope in config. - options = self._parse( - flags="--a=100 ", config={"compile": {"a": 99}}, env={"PANTS_A": "100"} - ) - self.assertEqual(100, options.for_global_scope().a) - self.assertEqual(100, options.for_scope("compile").a) - self.assertEqual(100, options.for_scope("compile.java").a) - def test_complete_scopes(self) -> None: self.assertEqual( {intermediate("foo"), intermediate("foo.bar"), task("foo.bar.baz")}, @@ -1578,27 +1347,15 @@ def test_complete_scopes(self) -> None: set(Options.complete_scopes({task("foo.bar.baz"), task("qux.quux")})), ) - @pytest.mark.skip("Temporarily skip until recursive options are removed, see comment.") def test_get_fingerprintable_for_scope(self) -> None: - # Note that until a recent change to fingerprinting code, this test operated under the - # old assumption that options are not fingerprintable by default. The change in question - # eliminated the ability to make this assumption in the tests (since that assumption was - # not true in non-test code). As a result, the fingerprintables now include all the - # registered options on compile.scala and all enclosing scopes, including global scope. - # There are too many of those to enumerate in this test, so we temporarily skip it, until - # we remove recursive options and this test becomes tractable again. - - # Note: tests handling recursive and non-recursive options from enclosing scopes correctly. options = self._parse( - flags='--store-true-flag --num=88 compile.scala --num=77 --modifycompile="blah blah blah" ' + flags='--store-true-flag --num=88 compile --modifycompile="blah blah blah" ' '--modifylogs="durrrr" -- -d -v' ) # NB: Passthrough args end up on our `--modifypassthrough` arg. - pairs = options.get_fingerprintable_for_scope("compile.scala") - self.assertEqual( - [(str, "blah blah blah"), (str, ["-d", "-v"]), (bool, True), (int, 77)], pairs - ) + pairs = options.get_fingerprintable_for_scope("compile") + self.assertEqual([(str, "blah blah blah"), (str, ["-d", "-v"])], pairs) def test_fingerprintable(self) -> None: options = self._parse( @@ -1606,7 +1363,7 @@ def test_fingerprintable(self) -> None: "--explicitly-fingerprinted=also_shall_be_fingerprinted " "--explicitly-not-fingerprinted=shant_be_fingerprinted" ) - pairs = options.get_fingerprintable_for_scope("fingerprinting") + pairs = options.get_fingerprintable_for_scope(GLOBAL_SCOPE) self.assertIn((str, "shall_be_fingerprinted"), pairs) self.assertIn((str, "also_shall_be_fingerprinted"), pairs) self.assertNotIn((str, "shant_be_fingerprinted"), pairs) @@ -1746,44 +1503,16 @@ def test_ranked_value_equality(self) -> None: self.assertNotEqual(some, RankedValue(Rank.HARDCODED, "few")) self.assertNotEqual(some, RankedValue(Rank.CONFIG, "some")) - def test_pants_global_designdoc_example(self) -> None: - # The example from the design doc. - # Get defaults from config and environment. - config = { - "GLOBAL": {"b": "99"}, - "compile": {"a": "88", "c": "77"}, - } - - env = {"PANTS_COMPILE_C": "66"} - - options = self._parse( - flags="--a=1 compile --b=2 compile.java --a=3 --c=4", - env=env, - config=config, - ) - - self.assertEqual(1, options.for_global_scope().a) - self.assertEqual(99, options.for_global_scope().b) - with self.assertRaises(AttributeError): - options.for_global_scope().c - - self.assertEqual(1, options.for_scope("compile").a) - self.assertEqual(2, options.for_scope("compile").b) - self.assertEqual(66, options.for_scope("compile").c) - - self.assertEqual(3, options.for_scope("compile.java").a) - self.assertEqual(2, options.for_scope("compile.java").b) - self.assertEqual(4, options.for_scope("compile.java").c) - def test_pants_global_with_default(self) -> None: """This test makes sure values under [DEFAULT] still gets read.""" # This cast shouldn't be necessary - likely a bug in MyPy. Once this gets fixed, MyPy will # tell us that we can remove the cast. config = cast( - Dict[str, Dict[str, Any]], {"DEFAULT": {"b": "99"}, "GLOBAL": {"store_true_flag": True}} + Dict[str, Dict[str, Any]], + {"DEFAULT": {"num": "99"}, "GLOBAL": {"store_true_flag": True}}, ) global_options = self._parse(config=config).for_global_scope() - self.assertEqual(99, global_options.b) + self.assertEqual(99, global_options.num) self.assertTrue(global_options.store_true_flag) def test_double_registration(self) -> None: diff --git a/src/python/pants/option/parser.py b/src/python/pants/option/parser.py index 495f7878c8b..0b354987eb8 100644 --- a/src/python/pants/option/parser.py +++ b/src/python/pants/option/parser.py @@ -47,9 +47,7 @@ OptionNameDoubleDash, ParseError, PassthroughType, - RecursiveSubsystemOption, RegistrationError, - Shadowing, UnknownFlagsError, ) from pants.option.option_util import flatten_shlexed_list, is_dict_option, is_list_option @@ -71,13 +69,9 @@ def final_value(self) -> RankedValue: class Parser: """An argument parser in a hierarchy. - Each node in the hierarchy is a 'scope': the root is the global scope, and the parent of - a node is the scope it's immediately contained in. E.g., the 'compile.java' scope is - a child of the 'compile' scope, which is a child of the global scope. - - Options registered on a parser are also registered transitively on all the scopes it encloses. - We forbid registering options that shadow other options, and registration walks up and down the - hierarchy to enforce that. + Each node in the hierarchy is a 'scope': the root is the global scope, and the parent of a node + is the scope it's immediately contained in. E.g., the 'compile.java' scope is a child of the + 'compile' scope, which is a child of the global scope. """ @staticmethod @@ -116,11 +110,6 @@ def _invert(cls, s: bool | str | None) -> bool | None: def scope_str(cls, scope: str) -> str: return "global scope" if scope == GLOBAL_SCOPE else f"scope '{scope}'" - @classmethod - def _check_shadowing(cls, parent_scope, parent_known_args, child_scope, child_known_args): - for arg in parent_known_args & child_known_args: - raise Shadowing(child_scope, arg, outer_scope=cls.scope_str(parent_scope)) - def __init__( self, env: Mapping[str, str], @@ -141,7 +130,7 @@ def __init__( self._scope_info = scope_info self._scope = self._scope_info.scope - # All option args registered with this parser. Used to prevent shadowing args in inner scopes. + # All option args registered with this parser. Used to prevent conflicts. self._known_args: Set[str] = set() # List of (args, kwargs) registration pairs, exactly as captured at registration time. @@ -164,10 +153,6 @@ def scope_info(self) -> ScopeInfo: def scope(self) -> str: return self._scope - @property - def known_args(self) -> Set[str]: - return self._known_args - def history(self, dest: str) -> OptionValueHistory | None: return self._history.get(dest) @@ -235,7 +220,7 @@ def parse_args(self, parse_args_request: ParseArgsRequest) -> OptionValueContain namespace = parse_args_request.namespace mutex_map: DefaultDict[str, List[str]] = defaultdict(list) - for args, kwargs in self._unnormalized_option_registrations_iter(): + for args, kwargs in self._option_registrations: self._validate(args, kwargs) dest = self.parse_dest(*args, **kwargs) @@ -326,10 +311,6 @@ def option_registrations_iter(self): will be normalized in the following ways: - It will always have 'dest' explicitly set. - It will always have 'default' explicitly set, and the value will be a RankedValue. - - For recursive options, the original registrar will also have 'recursive_root' set. - - Note that recursive options we inherit from a parent will also be yielded here, with - the correctly-scoped default value. """ def normalize_kwargs(orig_args, orig_kwargs): @@ -347,53 +328,11 @@ def normalize_kwargs(orig_args, orig_kwargs): nkwargs["default"] = RankedValue(Rank.HARDCODED, default_val) return nkwargs - # First yield any recursive options we inherit from our parent. - if self._parent_parser: - for args, kwargs in self._parent_parser._recursive_option_registration_args(): - yield args, normalize_kwargs(args, kwargs) - - # Then yield our directly-registered options. - # This must come after yielding inherited recursive options, so we can detect shadowing. + # Yield our directly-registered options. for args, kwargs in self._option_registrations: normalized_kwargs = normalize_kwargs(args, kwargs) - if "recursive" in normalized_kwargs: - # If we're the original registrar, make sure we can distinguish that. - normalized_kwargs["recursive_root"] = True yield args, normalized_kwargs - def _unnormalized_option_registrations_iter(self): - """Returns an iterator over the raw registration arguments of each option in this parser. - - Each yielded item is an (args, kwargs) pair, exactly as passed to register(), except for - substituting list and dict types with list_option/dict_option. - - Note that recursive options we inherit from a parent will also be yielded here. - """ - # First yield any recursive options we inherit from our parent. - if self._parent_parser: - for args, kwargs in self._parent_parser._recursive_option_registration_args(): - yield args, kwargs - # Then yield our directly-registered options. - for args, kwargs in self._option_registrations: - if "recursive" in kwargs and self._scope_info.scope != GLOBAL_SCOPE: - raise RecursiveSubsystemOption(self.scope, args[0]) - yield args, kwargs - - def _recursive_option_registration_args(self): - """Yield args, kwargs pairs for just our recursive options. - - Includes all the options we inherit recursively from our ancestors. - """ - if self._parent_parser: - for args, kwargs in self._parent_parser._recursive_option_registration_args(): - yield args, kwargs - for args, kwargs in self._option_registrations: - # Note that all subsystem options are implicitly recursive: a subscope of a subsystem - # scope is another (optionable-specific) instance of the same subsystem, so it needs - # all the same options. - if self._scope_info.scope != GLOBAL_SCOPE or "recursive" in kwargs: - yield args, kwargs - def register(self, *args, **kwargs) -> None: """Register an option.""" if args: @@ -413,14 +352,7 @@ def register(self, *args, **kwargs) -> None: # Record the args. We'll do the underlying parsing on-demand. self._option_registrations.append((args, kwargs)) - # Look for shadowing options up and down the hierarchy. - args_set = set(args) - for parent in self._parents_transitive(): - self._check_shadowing(parent.scope, parent._known_args, self.scope, args_set) - for child in self._children_transitive(): - self._check_shadowing(self.scope, args_set, child.scope, child._known_args) - - # And look for direct conflicts + # Look for direct conflicts. for arg in args: if arg in self._known_args: raise OptionAlreadyRegistered(self.scope, arg) @@ -449,8 +381,6 @@ def _check_deprecated(self, dest: str, kwargs, print_warning: bool = True) -> No "metavar", "help", "advanced", - "recursive", - "recursive_root", "fingerprint", "removal_version", "removal_hint", @@ -550,12 +480,8 @@ def error( if kwarg not in self._allowed_registration_kwargs: error(InvalidKwarg, kwarg=kwarg) - # Ensure `daemon=True` can't be passed on non-global scopes (except for `recursive=True`). - if ( - kwarg == "daemon" - and self._scope != GLOBAL_SCOPE - and kwargs.get("recursive") is False - ): + # Ensure `daemon=True` can't be passed on non-global scopes. + if kwarg == "daemon" and self._scope != GLOBAL_SCOPE: error(InvalidKwargNonGlobalScope, kwarg=kwarg) removal_version = kwargs.get("removal_version")