Fix 13 audit findings; per-instance group/subparser copies#47
Merged
Conversation
Metaclass (parser.py): - Copy a shared Argument() instance before inferring type/nargs/choices: one instance bound to several fields/classes was mutated in place, so `b: str = arg` silently required the int type inferred for `a: int`. - Reject arguments that would shadow methods/properties (parse_args, print_help, user-defined helpers): the metaclass used to replace them with `...` placeholders, silently destroying the parser API. - Reject a bare `serve: Serve` annotation (no instance): it silently became a required CLI argument with type=<Parser class>. - Inherit unannotated arguments, groups, and subparsers: previously a subclass lost every `tags = Argument(...)`, `db = DB()` or `serve = Serve()` declared on its base class. - Preserve inherited plain defaults: `name: str = "x"` used to become a *required* argument in every subclass because the processed class attribute had been replaced with an Ellipsis placeholder. - Wrap each element of Secret(nargs=...) lists in SecretString; only scalar secrets were masked, so repr(list) leaked the values. - Remove dead code in the Optional-unwrap branch. Config parsers (defaults.py, actions.py): - Make strict=False consistent: malformed INI/TOML files are now skipped like JSON ones instead of raising; strict=True still raises. - Alias INIDefaultsParser.BOOL_TRUE_VALUES to types.TEXT_TRUE_VALUES (the two identical sets could drift apart). - ConfigAction caches results via a None sentinel instead of truthiness, so an empty config is parsed once. Factory (factory.py): - EnumArgument(lowercase=True) now uses an explicit name map instead of str.upper() round-trips, fixing enums with non-uppercase member names; enum aliases (e.g. LogLevelEnum.WARN) remain accepted. Docs (pitfalls.md): - Unselected subcommand attributes raise AttributeError; the page claimed they "retain their default values". - Document that Group/subparser attributes are class-level singletons shared by all instances of a Parser class. tests/test_audit_fixes.py covers every fix; 21 of its 38 tests fail against the pre-fix code.
The Group/Parser instances written in a class body were registered by the metaclass and shared by every instance of that Parser class: a second parse_args() — on another instance — overwrote the first instance's group and subcommand values. Parser.__init__ now clones the declared member tree (recursively for nested groups and subparser trees) so every Parser instance owns its parsed state. The class-level instances become pristine prototypes. Because every binding gets its own copy, assigning one Group instance to several attributes is now supported — each location keeps independent parsed state. The previous "referenced more than once" error is narrowed to the only unclonable shape: a cyclic group tree (detected at construction time, with a parse-time backstop for cycles wired into instance mappings after __init__). Behavior changes: - parser.group is no longer the class-body instance; code that kept a module-level reference to a Group/subparser instance must read values via the parser instance. - Reusing one Group instance across attributes works instead of raising (the copies share the prototype's title/prefix, so an explicit prefix= would still produce conflicting CLI flags). - Cycle errors fire at Parser construction instead of parse_args().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two changes on top of 1.10.0: a batch of fixes for latent bugs found by a
full-source audit, and a behavioral improvement that gives every Parser
instance its own copies of declared groups and subparsers.
Audit fixes (13 findings, each verified by a failing test against the pre-fix code)
Metaclass:
Argument()instance bound to several fields/classes was mutatedin place —
b: str = argsilently required theintinferred fora: int.The metaclass now copies before inferring type/nargs/choices.
parse_args,print_help, user-defined helpers): previously they were silently replacedwith
...placeholders, destroying the parser API.serve: Serveannotation (no instance) raises instead of silentlybecoming a required CLI argument with
type=<Parser class>.(previously a subclass lost every
tags = Argument(...)/db = DB()/serve = Serve()from its base).name: str = "x"no longer becomesa required argument in subclasses.
Secret(nargs=...)wraps each list element inSecretString(repr nolonger leaks values).
Config parsers:
strict=False: malformed INI/TOML files are skipped like JSONinstead of raising;
strict=Truestill propagates.INIDefaultsParser.BOOL_TRUE_VALUESaliases the sharedtypes.TEXT_TRUE_VALUES.ConfigActioncaches via aNonesentinel (an empty config parses once).Factory:
EnumArgument(lowercase=True)uses an explicit name map instead ofstr.upper()round-trips, fixing enums with non-uppercase member names;enum aliases (
WARN→WARNING) remain accepted.Docs: corrected the false claim that unselected subcommands "retain their
default values" (they raise
AttributeError).Per-instance copies & group reuse
Parser.__init__now clones the declared member tree (recursively for nestedgroups and subparser trees), so:
subcommand values; the class-body instances become pristine prototypes.
each binding is an independent copy. Only cyclic group trees raise
(at construction time, with a parse-time backstop).
Behavior changes to note
parser.groupis no longer the class-body instance; module-level referencesto a Group/subparser must read values via the parser instance.
Parser()construction instead ofparse_args().(such parsers could not work correctly before).
Verification
tomllibbaseline only);coverage at the 100% gate.