Conversation
added 10 commits
August 25, 2026 09:15
Introduce an argument-description model (brush-core::argmodel) so that builtins declare their arguments as data and materialize themselves from parsed matches, with the actual parsing crate as a compile-time implementation detail: - CommandSpec/ArgSpec/PositionalSpec describe switches, value-taking options (with hidden aliases), and positionals; Matches exposes typed lookups plus verbatim trailing operands - ArgParserBackend trait with three implementations selected by cargo features: parser-bpaf (default), parser-usage, parser-clap - SpecCommand trait + spec_builtin() registration: declare() builds the spec, from_matches() builds the command; help content renders through the backend - shell-style option/operand splitting and '+x' group expansion stay in core, shared by every backend Converts echo to the new contract as the reference implementation and rewrites the custom-builtin example accordingly; remaining builtins continue using the legacy bpaf-bound trait until converted. The example now runs identically under all three backends. Assisted-by: ox-alpha (opencode)
…tract Every builtin now declares its argument surface as data (brush-core::argmodel) and materializes from parsed Matches; the legacy bpaf-bound Command/DeclarationCommand traits are no longer used by any registration (factory.rs is all spec_builtin, with special markers and declaration routing preserved). Notable conversions: - set: 19 +/- pairs via declare_plus_minus helpers; -o/+o tri-state and bare '--' detection preserved through an overridden new() - complete/compgen/compopt: shared arg surface flattened into each spec; flag-looking-value joining kept in overridden new() - kill: -l/-L as one spec arg with hidden short alias - history: -a/-n/-r/-w stay independent flags (matching old behavior); bare -X lifted out of the option section in overridden new() - test/[ and unimp: verbatim capture via overridden new() - pwd/cd: manual last-wins mode scan ported to overridden new() - export/declare/builtin_: uses_declarations() routes raw Vec<CommandArg> operands through set_declarations Core support added for the contract: uses_declarations()/set_declarations hooks on SpecCommand with declaration-aware execution routing, plus declare_plus_minus/read_plus_minus helpers replacing the bpaf-era macro. All suites green: compat 1854 passed / 0 failed (baseline parity), unit/integration tests pass, clippy -D warnings clean. Assisted-by: ox-alpha (opencode)
Introduce an argument-description model (brush-core::argmodel) so that builtins declare their arguments as data and materialize themselves from parsed matches, with the actual parsing crate as a compile-time implementation detail: - CommandSpec/ArgSpec/PositionalSpec describe switches, value-taking options (with hidden aliases), and positionals; Matches exposes typed lookups plus verbatim trailing operands - ArgParserBackend trait with three implementations selected by cargo features: parser-bpaf (default), parser-usage, parser-clap - SpecCommand trait + spec_builtin() registration: declare() builds the spec, from_matches() builds the command; help content renders through the backend - shell-style option/operand splitting and '+x' group expansion stay in core, shared by every backend Converts echo to the new contract as the reference implementation and rewrites the custom-builtin example accordingly; remaining builtins continue using the legacy bpaf-bound trait until converted. The example now runs identically under all three backends. Assisted-by: ox-alpha (opencode)
- backend selection becomes priority-based (usage > clap > bpaf) instead of compile_error, so workspace feature unification cannot break builds when several backends end up linked; a separate bpaf-linked feature keeps the bpaf backend compilable whenever the crate is in the graph (the shell CLI still uses bpaf directly for its own options) - brush-builtins/brush-shell forward parser-* features; brush-builtins no longer hard-depends on bpaf - pwd converts to SpecCommand (overridden new() keeps last-wins -L/-P) Assisted-by: ox-alpha (opencode)
bpaf_backend now compiles only when bpaf is the *selected* builtin backend (no parser-usage/parser-clap), so workspace feature unification that links bpaf for the shell CLI does not produce dead-code errors in usage-only builds. active() priority: usage > clap > bpaf. Assisted-by: ox-alpha (opencode)
usage backend: bind Event::Arg by positional id into positional slots; ParsedValues gains positional storage + accessors (positional_values/value_of_positional/push_positional_*). Converted builtins that read positionals via the named-arg getters (compgen word, mapfile array var, read var names, break/continue/caller/return, etc.) now use the positional accessors. Backend-parity tests extended to the usage backend. Assisted-by: ox-alpha (opencode)
The usage backend previously rebuilt and leaked the engine's Command<'static> graph on every builtin invocation, so scripts calling shift/getopts in loops leaked one allocation set per call. Graphs are now interned in a process-wide cache keyed by (spec address, invocation name), bounding total leakage to one graph per registered builtin — matching what usage's derive achieves with compile-time statics. Also adds backend-parity tests for the usage backend (single positional binding; graph reuse across parses). Assisted-by: ox-alpha (opencode)
The clap backend predates the positional-slot redesign and its tests were
stale, masking two bugs:
- Positional values were stored through the named-value API instead of
positional slots, so plain operands never reached callers.
- 'allow_hyphen_values' made flag-like leading words bind as operand
values where bash and every other backend reject them ('echo -x' must
error); trailing_var_arg alone keeps post-first-word words verbatim.
Tests are updated to the current pattern and now cover the same cases as
the bpaf and usage suites. Compat suite: 1854 passed / 0 failed.
Assisted-by: ox-alpha (opencode)
Assisted-by: ox-alpha (opencode)
Assisted-by: ox-alpha (opencode)
Owner
Author
|
Wrong venue — moving to upstream. |
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.
Stacked on reubeno#1302 (bpaf port) — base is
bpaf-port, so this diff shows only the contract work. Draft for inspection; benchmark data below.What this does
Replaces per-builtin hand-rolled argument parsing with a backend-neutral contract:
A build selects one engine via cargo features (
parser-usage/parser-clap/parser-bpaf, priority in that order);ArgParserBackendtranslates the static spec into the engine's structures, parses, and maps results into slot-indexedParsedValues. All ~49 builtins are converted.On "this reimplements usage's approach"
Correct observation — deliberately so. usage-rs's derive already proved that static spec → parser structure is the fast shape; what it doesn't give you is engine neutrality. This contract keeps the shape (specs are compile-time
statics, exactly what usage's derive would emit) but makes the target swappable:staticParsedValues(+ shell-specific semantics live in core, shared by all engines)The measured cost of that indirection vs native single-engine builds is within noise (below).
Benchmark data
benchmarks/three-way.py(committed): pinned (taskset), interleaved sampling across all shells (drift hits every build equally), byte-for-byte output parity check before timing, medians of 12 samples. Workloads inbenchmarks/real-world/.7 shells, same machine, same run:
¹ upstream
main(clap derive per builtin) · ² reubeno#1302 head (native bpaf impls) · ³ usage fork with#[derive(Cli)]builtinsReading:
Commandwins over the derive path).Correctness
allow_hyphen_valuesswallowing-x) — fixed here.(spec, name); total leakage matches what derive emits as statics (~50 small graphs), verified by pointer-stability test.Known trade-offs / open questions for review
declare/readonly/localshare one spec under three names (cache keyed accordingly).Assisted-by: ox-alpha (opencode)