Move option registration validation earlier

src/python/pants/help/help_formatter_test.py

@@ -60,7 +60,7 @@ def test_format_help_choices(self) -> None:
     @classmethod
     def _get_parser(cls) -> Tuple[Parser, NativeOptionParser]:
         return Parser(
-            scope_info=GlobalOptions.get_scope_info(),
+            scope=GlobalOptions.options_scope,
         ), NativeOptionParser(
             args=[], env={}, config_sources=[], allow_pantsrc=False, include_derivation=True
         )

src/python/pants/help/help_info_extracter_test.py

@@ -101,7 +101,7 @@ def do_test(args, kwargs, expected_default_str):
         # Defaults are computed in the parser and added into the kwargs, so we
         # must jump through this hoop in this test.
         parser = Parser(
-            scope_info=GlobalOptions.get_scope_info(),
+            scope=GlobalOptions.options_scope,
         )
         native_parser = NativeOptionParser([], {}, [], allow_pantsrc=False, include_derivation=True)
         parser.register(*args, **kwargs)
@@ -198,9 +198,7 @@ def do_test(kwargs, expected_basic=False, expected_advanced=False):
         def exp_to_len(exp):
             return int(exp)  # True -> 1, False -> 0.
 
-        parser = Parser(
-            scope_info=GlobalOptions.get_scope_info(),
-        )
+        parser = Parser(scope=GlobalOptions.options_scope)
         native_parser = NativeOptionParser([], {}, [], allow_pantsrc=False, include_derivation=True)
         parser.register("--foo", **kwargs)
         oshi = HelpInfoExtracter("").get_option_scope_help_info(

src/python/pants/help/help_tools_test.py

@@ -16,9 +16,7 @@
 
 @pytest.fixture
 def parser() -> Parser:
-    return Parser(
-        scope_info=GlobalOptions.get_scope_info(),
-    )
+    return Parser(scope=GlobalOptions.options_scope)
 
 
 @pytest.fixture
@@ -33,8 +31,8 @@ def extracter() -> HelpInfoExtracter:
 
 @pytest.fixture
 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}")
+    parser.register("--version", type=str, default="1.0")
+    parser.register("--url-template", type=str, default="https://download/{version}")
     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

src/python/pants/option/native_options.py

@@ -23,7 +23,7 @@
 logger = logging.getLogger()
 
 
-def parse_dest(*args, **kwargs):
+def parse_dest(*args: str, **kwargs) -> str:
     """Return the dest for an option registration.
 
     If an explicit `dest` is specified, returns that and otherwise derives a default from the
@@ -36,7 +36,7 @@ def parse_dest(*args, **kwargs):
     """
     dest = kwargs.get("dest")
     if dest:
-        return dest
+        return str(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])

src/python/pants/option/options.py

@@ -15,7 +15,7 @@
 from pants.option.errors import ConfigValidationError, UnknownFlagsError
 from pants.option.native_options import NativeOptionParser
 from pants.option.option_util import is_list_option
-from pants.option.option_value_container import OptionValueContainer, OptionValueContainerBuilder
+from pants.option.option_value_container import OptionValueContainer
 from pants.option.parser import Parser
 from pants.option.scope import GLOBAL_SCOPE, GLOBAL_SCOPE_CONFIG_SECTION, ScopeInfo
 from pants.util.memo import memoized_method
@@ -164,7 +164,7 @@ def create(
                             [line for line in [line.strip() for line in f] if line]
                         )
 
-        parser_by_scope = {si.scope: Parser(si) for si in complete_known_scope_infos}
+        parser_by_scope = {si.scope: Parser(si.scope) for si in complete_known_scope_infos}
         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()
@@ -392,16 +392,6 @@ def _check_and_apply_deprecations(self, scope, values):
                     hint=f"Use scope {scope} instead (options: {', '.join(explicit_keys)})",
                 )
 
-    def _make_parse_args_request(
-        self, flags_in_scope, namespace: OptionValueContainerBuilder
-    ) -> Parser.ParseArgsRequest:
-        return Parser.ParseArgsRequest(
-            flags_in_scope=flags_in_scope,
-            namespace=namespace,
-            passthrough_args=self._passthru,
-            allow_unknown_flags=self._allow_unknown_options,
-        )
-
     # TODO: Eagerly precompute backing data for this?
     @memoized_method
     def for_scope(

src/python/pants/option/options_test.py

@@ -1014,7 +1014,7 @@ def test_parse_dest() -> None:
 
 
 def test_validation() -> None:
-    def assertError(expected_error, *args, **kwargs):
+    def assert_error(expected_error, *args, **kwargs):
         with pytest.raises(expected_error):
             options = Options.create(
                 args=["./pants"],
@@ -1025,17 +1025,17 @@ def assertError(expected_error, *args, **kwargs):
             options.register(GLOBAL_SCOPE, *args, **kwargs)
             options.for_global_scope()
 
-    assertError(NoOptionNames)
-    assertError(OptionNameDoubleDash, "badname")
-    assertError(OptionNameDoubleDash, "-badname")
-    assertError(InvalidKwarg, "--foo", badkwarg=42)
-    assertError(BooleanOptionNameWithNo, "--no-foo", type=bool)
-    assertError(MemberTypeNotAllowed, "--foo", member_type=int)
-    assertError(MemberTypeNotAllowed, "--foo", type=dict, member_type=int)
-    assertError(InvalidMemberType, "--foo", type=list, member_type=set)
-    assertError(InvalidMemberType, "--foo", type=list, member_type=list)
-    assertError(HelpType, "--foo", help=())
-    assertError(HelpType, "--foo", help=("Help!",))
+    assert_error(NoOptionNames)
+    assert_error(OptionNameDoubleDash, "badname")
+    assert_error(OptionNameDoubleDash, "-badname")
+    assert_error(InvalidKwarg, "--foo", badkwarg=42)
+    assert_error(BooleanOptionNameWithNo, "--no-foo", type=bool)
+    assert_error(MemberTypeNotAllowed, "--foo", member_type=int)
+    assert_error(MemberTypeNotAllowed, "--foo", type=dict, member_type=int)
+    assert_error(InvalidMemberType, "--foo", type=list, member_type=set)
+    assert_error(InvalidMemberType, "--foo", type=list, member_type=list)
+    assert_error(HelpType, "--foo", help=())
+    assert_error(HelpType, "--foo", help=("Help!",))
 
 
 def test_shadowing() -> None:
@@ -1046,7 +1046,7 @@ def test_shadowing() -> None:
         args=["./pants"],
     )
     options.register("", "--opt1")
-    options.register("foo", "-o", "--opt2")
+    options.register("foo", "--opt2")
 
 
 def test_is_known_scope() -> None:

src/python/pants/option/parser.py

@@ -10,7 +10,7 @@
 from collections import defaultdict
 from dataclasses import dataclass
 from enum import Enum
-from typing import Any, Iterable, Mapping
+from typing import Any, Mapping
 
 from pants.base.deprecated import validate_deprecation_semver, warn_or_error
 from pants.option.custom_types import (
@@ -42,7 +42,7 @@
 from pants.option.native_options import NativeOptionParser, parse_dest
 from pants.option.option_value_container import OptionValueContainer, OptionValueContainerBuilder
 from pants.option.ranked_value import Rank, RankedValue
-from pants.option.scope import GLOBAL_SCOPE, ScopeInfo
+from pants.option.scope import GLOBAL_SCOPE
 from pants.util.strutil import softwrap
 
 logger = logging.getLogger(__name__)
@@ -92,16 +92,12 @@ def _invert(cls, s: bool | str | None) -> bool | None:
         b = cls.ensure_bool(s)
         return not b
 
-    def __init__(
-        self,
-        scope_info: ScopeInfo,
-    ) -> None:
+    def __init__(self, scope: str) -> None:
         """Create a Parser instance.
 
         :param scope_info: the scope this parser acts for.
         """
-        self._scope_info = scope_info
-        self._scope = self._scope_info.scope
+        self._scope = scope
 
         # All option args registered with this parser.  Used to prevent conflicts.
         self._known_args: set[str] = set()
@@ -112,10 +108,6 @@ def __init__(
         # Map of dest -> history.
         self._history: dict[str, OptionValueHistory] = {}
 
-    @property
-    def scope_info(self) -> ScopeInfo:
-        return self._scope_info
-
     @property
     def scope(self) -> str:
         return self._scope
@@ -125,38 +117,13 @@ def known_scoped_args(self) -> frozenset[str]:
         prefix = f"{self.scope}-" if self.scope != GLOBAL_SCOPE else ""
         return frozenset(f"--{prefix}{arg.lstrip('--')}" for arg in self._known_args)
 
-    def history(self, dest: str) -> OptionValueHistory | None:
-        return self._history.get(dest)
-
-    @dataclass(frozen=True)
-    class ParseArgsRequest:
-        namespace: OptionValueContainerBuilder
-        passthrough_args: list[str]
-        allow_unknown_flags: bool
-
-        def __init__(
-            self,
-            flags_in_scope: Iterable[str],
-            namespace: OptionValueContainerBuilder,
-            passthrough_args: list[str],
-            allow_unknown_flags: bool,
-        ) -> None:
-            """
-            :param flags_in_scope: Iterable of arg strings to parse into flag values.
-            :param namespace: The object to register the flag values on
-            """
-            object.__setattr__(self, "namespace", namespace)
-            object.__setattr__(self, "passthrough_args", passthrough_args)
-            object.__setattr__(self, "allow_unknown_flags", allow_unknown_flags)
-
     def parse_args_native(
         self,
         native_parser: NativeOptionParser,
     ) -> OptionValueContainer:
         namespace = OptionValueContainerBuilder()
         mutex_map = defaultdict(list)
         for args, kwargs in self._option_registrations:
-            self._validate(args, kwargs)
             dest = parse_dest(*args, **kwargs)
             val, rank = native_parser.get_value(
                 scope=self.scope, registration_args=args, registration_kwargs=kwargs
@@ -213,9 +180,9 @@ def normalize_kwargs(orig_args, orig_kwargs):
 
     def register(self, *args, **kwargs) -> None:
         """Register an option."""
-        if args:
-            dest = parse_dest(*args, **kwargs)
-            self._check_deprecated(dest, kwargs, print_warning=False)
+        self._validate(args, kwargs)
+        dest = parse_dest(*args, **kwargs)
+        self._check_deprecated(dest, kwargs, print_warning=False)
 
         if self.is_bool(kwargs):
             default = kwargs.get("default")
@@ -316,15 +283,19 @@ def error(
         default_value = kwargs.get("default")
         if default_value is not None:
             if isinstance(default_value, str) and type_arg != str:
-                # attempt to parse default value, for correctness..
+                # attempt to parse default value, for correctness.
                 # custom function types may implement their own validation
                 default_value = self.to_value_type(default_value, type_arg, member_type)
                 if hasattr(default_value, "val"):
                     default_value = default_value.val
 
                 # fall through to type check, to verify that custom types returned a value of correct type
 
-            if isinstance(type_arg, type) and not isinstance(default_value, type_arg):
+            if (
+                isinstance(type_arg, type)
+                and not isinstance(default_value, type_arg)
+                and not (issubclass(type_arg, bool) and default_value == UnsetBool)
+            ):
                 error(
                     DefaultValueType,
                     option_type=type_arg.__name__,