Conversation
Port benchmarks/three-way.py and benchmarks/real-world/*.sh from mine/bpaf-port (superset of the experiment/usage-parser scripts): startup, interp-loop, wordops, config-lint-500, deploy-sim with interleaved pinned sampling, oracle parity check, and VmHWM RSS. Add --numa-node to pin runs via numactl --cpunodebind/--membind. Anchor (94f9418, clap baseline, node 3/cpu 96, 15 samples) saved under benchmarks/results/anchor-94f94188-clap/. Assisted-by: Muse Spark:muse-spark-1.3-contributor-free [opencode]
Introduces brush_core::args with the contract 'words -> T or ArgsError': builtins will consume only this trait while argument-parsing engines provide implementations. ArgsError distinguishes usage failures from help/version requests. Includes a transitional blanket implementation (clap::Parser types satisfy FromArgs automatically) so existing builtins keep working unchanged during migration; it is removed once migration completes. Assisted-by: ox-alpha (opencode)
Replaces the clap::Parser supertrait on builtins::Command with the new brush_core::args::FromArgs contract, and neutralizes Command::new's error type to ArgsError (usage failure vs help request). Transitional scaffolding, all removable per-builtin as migration proceeds: - blanket impl: clap-derived types satisfy FromArgs unchanged - clap_content helper + one-line get_content impls preserve current help rendering from clap metadata - the four builtins overriding new() (echo/getopts/set/test) keep their '--' workaround via try_parse_known Behavior is unchanged; compat suite: 1854 passed / 0 failed. Assisted-by: ox-alpha (opencode)
Interleaved bash + anchor-clap(94f9418) + abstract-clap(c7e566e), 15 samples, parity-checked. Pairwise anchor vs abstract medians: startup 1.03x, interp-loop 1.02x, wordops 1.02x, config-lint 0.99x, deploy-sim 1.00x - all within MAD noise. RSS deltas <= 128 KiB. A standalone abstract-clap run earlier in the session showed apparent regressions (e.g. deploy-sim 53 -> 67ms), but bash drifted by the same margin in that window; same-run interleaving removes the confound. Assisted-by: Muse Spark:muse-spark-1.3-contributor-free [opencode]
echo becomes the reference for the converted shape: - echo.rs holds only the plain argument struct and execute logic; no engine types or derives - args/clap.rs (per-engine module) owns word binding via FromArgs and the mirror type carrying option/help metadata, preserving current output byte-for-byte including the '--' workaround - other engines add their own module (args/bpaf.rs, args/usage.rs) implementing the same FromArgs impls Compat suite: 1854 passed / 0 failed. Assisted-by: ox-alpha (opencode)
… types Per review feedback: instead of mirror structs + conversions, builtin argument structs carry the selected engine's instrumentation directly via cfg_attr, and the parser-clap/parser-bpaf/parser-usage feature trio (exactly-one guard included) selects which set compiles. Consequence: with clap selected, existing derives already satisfy the FromArgs contract through the blanket impl — no per-builtin binding code remains for simple builtins. Only genuine shell quirks (echo's '--' workaround) keep engine-specific overrides, now feature-gated. Help rendering stays on the transitional clap_content path, gated per engine until brush grows its own help model. Selecting an engine without bindings (e.g. parser-bpaf today) fails the build loudly, listing the builtins still lacking support. Assisted-by: ox-alpha (opencode)
Adopts the reviewed layout: each builtin keeps its logic in {builtin}.rs
and its engine-specific instrumented structs (plus any impls) in sibling
{builtin}/{clap,bpaf,usage}.rs files. A small arg_impl!(TheType) macro
declares the engine-gated sibling modules and re-exports the selected
engine's type, so builtin code and factory registrations stay
engine-agnostic.
With clap selected, blanket FromArgs covers plain derives; only shell
quirks (echo's '--' handling) live as overrides inside the engine file.
Other engines will provide their own struct instrumentation and quirk
handling in their own files.
Assisted-by: ox-alpha (opencode)
Completes the bulk migration to the reviewed layout: each builtin's
logic lives in {builtin}.rs with only pure command behavior, while the
engine-instrumented argument struct, trait impls, and any shell-quirk
overrides live in {builtin}/clap.rs behind parser-clap. arg_impl!
selects the module set and re-exports the type so factory registrations
stay unchanged.
Notable non-mechanical cases:
- declare/export/set/umask: clap-flag helper structs generated by
macros (minus_or_plus_flag_arg etc.) move into the engine module
- bind/complete/unset: shared arg types referenced from logic gain
explicit cross-module imports; fields exposed pub(super)
- exec/popd/pushd: cross-builtin invocation switches to UFCS
builtins::Command::execute
- history/bind: unit tests follow their execute implementations
SimpleCommand-style builtins (colon/true/false) have no arguments and
remain single-file.
Compat suite: 1854 passed / 0 failed. Workspace clippy/fmt clean.
(interactive suspend test failure is pre-existing on main)
Assisted-by: ox-alpha (opencode)
Infrastructure complete: args support module (BpafArgs trait, runner, splitter, help rendering), optional bpaf dep wired to parser-bpaf, generated modules for 45 structurally-matching builtins plus hand-written cd/pwd/getopts/set adapters. Remaining known issues tracked for next session: complete/bpaf needs FromStr impls + inherent parser impl placement; bind BindError unification; cd mode mapping; set named_option_section construction; assorted import pruning under the bpaf-only feature set. Assisted-by: ox-alpha (opencode)
Resolves the remaining engine-module issues: declaration-command impls transplanted for export/declare/builtin_, tri-state flag structs for the set/declare families via minus_or_plus_flag_bpaf, FromStr for BindKeyMap, per-file import repairs, and cfg-gating of bpaf-only helpers so the clap-default build stays clean. Both cargo configurations now compile: default (parser-clap) and parser-bpaf with all builtin features. Assisted-by: ox-alpha (opencode)
…t green
Ports experiment/usage-parser's #[derive(usage::Cli)] instrumentation
into per-engine {builtin}/usage.rs modules behind parser-usage:
- args/usage_support.rs: UsageArgs trait + impl_usage_parse! macro +
runner + splitter + help rendering (ported from that branch's core)
- generated modules harvest struct instrumentation from the branch
- declaration builtins carry DeclarationCommand impls
Remaining known issues (~45 errors, all mechanical): missing helper-type
imports (declare/set flag structs via minus_or_plus_flag_bpaf, bind
types from clap sibling, complete CommonCompleteCommandArgs placement),
duplicate builtins imports, exit String/i64 set_trailing_args leftover,
read OpenFile::read trait import, unset shape alignment.
Assisted-by: ox-alpha (opencode)
…ors left - ports experiment/usage-parser's 4-arg minus_or_plus_flag_arg macro (usage::Args derive) as usage_minus_or_plus_flag_arg!, wired into set/declare usage modules with accumulated -o/+o SetOption fixes - args/usage_support: correct 6.1.1 error/help rendering - builtin modules exposed pub(crate) so engine modules can reach shared helper types; import dedup across generated files Remaining 9 errors documented in /tmp capture: complete CommonArgs placement + duplicates, bind enum copies, command duplicate inherent, factory bounds following from those. Assisted-by: ox-alpha (opencode)
Assisted-by: ox-alpha (opencode)
Remaining errors are concentrated in three files: bind/usage.rs (BindKeyMap needs usage ValueEnum derive + BindError ownership), command/usage.rs (duplicate inherent 'command' fn), complete/usage.rs (CommonCompleteCommandArgs + CompGen/CompOpt definitions). Fix pattern is proven from unset: shared types move to the parent module pub(crate), engines import them. Assisted-by: ox-alpha (opencode)
- command.rs: cross-builtin .command() helper restored in each engine module; stale renamed method removed from parent - complete/usage.rs: CommonCompleteCommandArgs struct + CompGen/CompOpt definitions added with usage::Cli derives parser-usage still has ~15 architectural errors (ValueEnum impls needed for core types, derive-macro-vs-trait confusion in spec() calls) that require design decisions about type ownership across engines. Assisted-by: ox-alpha (opencode)
Completes the usage-rs engine port: - completion.rs types instrumented via cfg_attr per parser feature (no standalone modules needed — cfg_attr suffices for type-level derives) - brush-core gains parser-clap/bpaf/usage features with optional deps, forwarding from brush-builtins - clap-only code paths in core gated behind parser-clap - bind/usage.rs: BindKeyMap with usage::ValueEnum derive + variant attrs - command/usage.rs: fields pub(crate) for cross-builtin access; inherent helper renamed first_arg to avoid derive collision - complete/usage.rs: CommonCompleteCommandArgs + CompGen/CompOpt with usage::Cli derives, execute bodies transplanted from experiment branch All three configurations build with zero errors: cargo check -p brush-builtins # parser-clap cargo check -p brush-builtins --no-default-features --features parser-bpaf,$FEATS # parser-bpaf cargo check -p brush-builtins --no-default-features --features parser-usage,$FEATS # parser-usage Assisted-by: ox-alpha (opencode)
Assisted-by: ox-alpha (opencode)
Assisted-by: ox-alpha (opencode)
…sions #[expect(...)] errors when the lint condition is unfulfilled, which varies across environments and feature resolutions. #[allow(...)] is the appropriate choice for transitional code. Also fixes clippy const-fn suggestions in declare.rs. Assisted-by: ox-alpha (opencode)
The compile_error! guard prevented cargo from building with --all-features (CI enables all features). Without the guard, the arg_impl! macro's cfg-gated module declarations naturally select only the engines whose features are active; with --all-features, multiple engines compile and the imp glob picks one deterministically. Assisted-by: ox-alpha (opencode)
The nix::sys::stat::Mode import was compiled out on Linux/Android but dead everywhere else, failing non-Linux builds (macOS, BSDs) run with -D warnings. Assisted-by: Maki:zai/glm-5.3-flash
The bpaf/clap/usage engine modules are parallel same-named type hierarchies, and builtin glue code binds against one selected engine, so enabling several together has no coherent interpretation yet (name collisions and monomorphic shared signatures). Enforce exactly-one via compile_error! guards so misuse fails fast with a clear message instead of dozens of downstream errors. This replaces the removed exactly-one-engine guard: wholesale --all-features builds are intentionally rejected now; workspace checks adapt in the following xtask change. Assisted-by: Maki:zai/glm-5.3-flash
check lint/build/unused-deps ran with a blanket --all-features, which now conflicts with the engine exclusivity enforced in brush-builtins (cargo-udeps was the first CI job to trip over it). Sweeps now run four passes that jointly cover the same ground: - one wholesale sweep of every crate except brush-core and brush-builtins (--workspace --exclude ... --all-features), then - one pass per parser engine over just those two crates with explicit, fully-qualified feature selections derived from cargo metadata. Feature lists are resolved dynamically so new features stay covered. Adds unit tests for metadata parsing, qualified feature selection, and sweep construction. Assisted-by: Maki:zai/glm-5.3-flash
Enables ergonomic '?' conversion into anyhow::Result contexts, which engine-agnostic builtin argument tests rely on under any parser engine. Assisted-by: Maki:zai/glm-5.3-flash
Replace the exactly-one-engine compile_error! guards with a fixed priority order when several engine features are enabled at once: parser-usage wins over parser-bpaf, which wins over parser-clap. arg_impl! now declares only the winning engine's sibling module, so a single unambiguous type namespace reaches both parent glue (via imp::) and cross-module re-exports (complete's CompGenCommand/CompOptCommand). Helpers tied to a specific engine (bpaf tri-state helpers, bpaf_support, OptionBoolExt) are now gated on engine *selection* rather than feature presence, keeping them alive exactly when used - including mixed setups like default+parser-bpaf that previously failed to compile. The history parse test now goes through the engine-agnostic FromArgs contract instead of clap's try_parse_from, so it runs under whichever engine is selected. All four feature combinations (default, each single engine, all combined) plus workspace --all-features --all-targets builds now pass; this also restores viability of blanket --all-features CI checks such as cargo-udeps. Assisted-by: Maki:zai/glm-5.3-flash
Parser engines now resolve by fixed priority, so workspace --all-features builds compile again; the per-engine sweep machinery added earlier is no longer needed and the simpler upstream checks (lint/build/unused-deps with --all-features) provide equivalent coverage. Assisted-by: Maki:zai/glm-5.3-flash
bpaf-only builds of the crate (a la carte consumption without builtin.set/builtin.declare) left tri_state_flag and minus_or_plus_flag orphaned, failing -D warnings. Gate their dead-code allowances on consumer presence so lint coverage stays strict in normal builds. Assisted-by: Maki:zai/glm-5.3-flash
Two defects made bpaf builds fail scripts that run fine under the clap/usage engines: - named_option_section kept a flag-less alternative first, so 'set' always parsed named options as Some(empty vec) and entered bash's list-all mode on every invocation, dumping the option tables. - bpaf implements no POSIX short-option clustering, so tokens like '-euo' failed to parse, silently aborting scripts at the ubiquitous 'set -euo pipefail' preamble with exit code 2. Clusters are now pre-split into individual switches (with value-taking 'o' handled at group boundaries), and the section parser gates purely on flag presence. Verified: probe scripts and both real-world benchmark scripts now pass identically under all three engines. Assisted-by: Maki:zai/glm-5.3-flash
Adds parser-clap/parser-bpaf/parser-usage forwarding features so a single source tree can produce binaries with any engine selected, enabling apples-to-apples engine comparisons and targeted testing. Assisted-by: Maki:zai/glm-5.3-flash
make_expectrl_output_readable previously panicked via unwrap() when a shell-under-test produced escaping that differs from expectrl's own (observed when running comparisons against non-default argument parsing engines). Keep the raw text (ANSI-stripped) so the comparison still runs and reports differences instead of aborting the whole run. Assisted-by: Maki:zai/glm-5.3-flash
Compat suite: 82 failures -> 0 (2310 cases; 456 known-fail excluded), matching the clap baseline exactly. Root causes were local port bugs, not usage-rs limitations: - declare/usage.rs: MakeIndexedArrayFlag was registered as 'i', a duplicate of MakeIntegerFlag, leaving 'a' unregistered; this broke '-ai' clusters and misbound '-i' to the array flag. - set/declare: opt into plus-cluster expansion via the new 'plus_options' arm of impl_usage_parse!; expansion now happens in UsageArgs::from_words where bash's boundary semantics (stop at plain word / '-' / '--') are honored, emitting long spellings (--+x) the parser matches. The dispatcher-level rename in builtins.rs was boundary-blind and leaked literal tokens into positionals. - set: bare '-o'/'+o' list-all via default_missing sentinel shared through BARE_OPTION/wants_list_all; route parsing through the trailing-args split so bare 'set --' no longer degrades into a full listing, with value_taking_short_options overridden so '-o NAME' keeps its operand on the options side of the boundary. - split_option_section: treat bare '-' as a boundary (kept verbatim in the trailing stream) and value-taking '--o NAME' longs as consuming their following token. - echo/test/getopts: double_dash = "preserve" so bash-literal '--' operands survive; shared getopts already skips a delivered '--'. - printf: hand-rolled leading-option scanner matching bash's rule (at most one leading '--' dropped, unknown leading options rejected with exit code 2); the generic argv parser stripped every '--'. - complete/compgen enums: usage(rename_all = "lowercase") so BashDefault/dirnames-style bash spellings parse and round-trip. - help topics: usage get_content falls back to concise spec summary when --help is swallowed as a positional instead of erroring (rc 99). Also drop a needless iter().copied().collect() flagged under the new lint surface. Assisted-by: Maki:zai/glm-5.3-flash
Compat suite: 3 printf failures plus 4 unrecorded ones -> 0 (2310 cases; 456 known-fail excluded), matching the clap and usage engine baselines. - printf: replace the generic split-then-parse route with a manual leading-option scanner mirroring bash's rule: only '-v' is accepted in the option zone, at most one *leading* '--' is dropped, unknown leading options are rejected, and an empty operand list is a usage error (exit code 2). The previous route kept the leading '--' in the trailing words, so it was mistaken for the format string. - command: drop one leading '--' from the captured operands when using the bpaf engine, matching clap's separator convention; bash resolves 'command -- cmd' to 'cmd' rather than executing '--'. - completion: teach CompleteOption::from_str the 'bashdefault', 'bash-default', and 'nospace' spellings so compgen/complete accept every bash option word on engines that parse options via FromStr. Assisted-by: Maki:zai/glm-5.3-flash
- echo/clap.rs: replace the strict dead_code expectation on the '-E' parity field with an allow; older toolchains in the supported range treat the derive-written field as live while newer ones consider it dead, so no single expectation holds across versions. - brush-core/brush-builtins manifests: declare bpaf as an ignored normal dependency for cargo-udeps. With all parser-engine features enabled at once, engine selection compiles only the priority winner's argument modules, so the losing engines' optional deps are legitimately unreferenced from udeps' point of view. Verified locally: cargo check/clippy --workspace --all-features --all-targets clean, and cargo +nightly udeps --workspace --all-targets --all-features exits 0. Assisted-by: Maki:zai/glm-5.3-flash
usage-rs (any published 6.x) declares rustc 1.91 as its MSRV while this repo's baseline stays 1.88.0, so a blanket --all-features check cannot resolve on the baseline job. Keep full-features validation on stable and give older toolchains an explicit scope covering the default build plus the remaining parser engines, mirroring how clippy already runs stable-only. Verified locally that all three substitute check invocations resolve. Assisted-by: Maki:zai/glm-5.3-flash
…-v/-V - command.rs: rename inherent command() helper to command_word(); the usage::Cli derive generates its own command() method on the same type. - completion.rs: drop now-unused clap::ValueEnum import (enums moved to engine-gated derives plus FromStr). - command/bpaf.rs, command/usage.rs: resolve -v/-V ties last-wins from the raw option words (clusters count left-to-right), covering the 'command -v/-V: last of -v/-V wins' case newer than the original port. Also drop the stale local FoundCommand from the bpaf module; the shared lookup framework owns that logic now. Assisted-by: Muse Spark:muse-spark-1.3-contributor-free [opencode]
…-usage - minus_or_plus_flag_arg!/tri_state_flag!: from_bool becomes const fn (missing_const_for_fn under current stable clippy). - lib.rs: merge duplicated doc comment lines (empty_line_after_doc_comments). - command.rs: drop unfulfilled unused_async expectation; the shared execute awaits execute_command. - .gitignore: cover target-usage/ bench build dir. Assisted-by: Muse Spark:muse-spark-1.3-contributor-free [opencode]
Interleaved bash + clap + bpaf + usage, 15 samples, oracle parity byte-identical across all four shells. Medians in benchmarks/results/backends-c986f537/ (report + JSON + build flags). Headline (config-lint-500, the parse-densest workload): usage 62ms, bpaf 79ms, clap 107ms. startup/interp-loop/deploy-sim within noise across engines; wordops favors clap with usage noisiest. Assisted-by: Muse Spark:muse-spark-1.3-contributor-free [opencode]
Public API changes for crate: brush-coreAdded itemsChanged itemsPerformance Benchmark ReportCode Coverage Report: Only Changed Files listed
Minimum allowed coverage is Test Summary: bash-completion test suite
|
The multi-line script with backslash continuations is a PowerShell
parse error on Windows runners ('Missing expression after unary
operator --'); git-bash is available there, matching existing
shell: bash precedent in this workflow.
Assisted-by: Muse Spark:muse-spark-1.3-contributor-free [opencode]
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
Redo of the
wip/bpaf-engineexperiment on top of currentorigin/main, importable in reviewable pieces:brush_core::args::{FromArgs, ArgsError};builtins::Command: FromArgswith a transitional clap blanket impl andclap_content()help helper.{builtin}.rs, engine structs live in{builtin}/{clap,bpaf,usage}.rs, selected byarg_impl!with global priority usage > bpaf > clap (per-builtin mixing explicitly deferred).args/bpaf_support.rs,args/usage_support.rs(plus-cluster expansion, option/trailing split,--handling).bpaf-port(benchmarks/three-way.py+benchmarks/real-world/*, plus--numa-nodepinning) with recorded results.Verification (all pinned to NUMA node 3 / cpu 96)
benchmarks/results/backends-c986f537/(parse-denseconfig-lint-500: usage 62ms, bpaf 79ms, clap 107ms).cargo fmt --checkandcargo clippyclean for all three engine configs.Notes for reviewers
lookupframework (command,type),declare -I,readUTF-8 rework,printf %qrework.command -v/-Vlast-wins in both non-clap backends.interp-looptiming anomaly (usage inexplicably faster on a parse-free workload) needs a re-run before quoting.Assisted-by: Muse Spark:muse-spark-1.3-contributor-free [opencode]