feat(core): stateful builtin infrastructure#1151
Conversation
Public API changes for crate: brush-builtinsRemoved itemsAdded itemsPublic API changes for crate: brush-coreRemoved itemsAdded itemsChanged itemsPublic API changes for crate: brush-experimental-builtinsRemoved itemsAdded itemsPerformance Benchmark Report
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is Test Summary: bash-completion test suite
|
|
This is a breaking change, if we can take more of the pending PRs (including the async one) we might have a quick 0.5 while with the larger PRs like the winnow parsers wait for more time for review. |
Agreed that it would be better to get in the breaks sooner than later. I really like trying to type the state. Do you think we might have need for multiple builtins to share the same state? I.e., same shared state but different commands that operate on it. Or would you expect that's the responsibility of the builtins to make happen? |
367941f to
60c177e
Compare
60c177e to
79c9c2f
Compare
|
|
||
| /// Registers experimental built-in commands on the given shell. | ||
| pub fn register_experimental_builtins<SE: brush_core::extensions::ShellExtensions>( | ||
| shell: &mut brush_core::Shell<SE>, |
There was a problem hiding this comment.
seems a false positive...
|
This branch now support both per-builtin and cross-builtin shared state, if you like its shape would be nice to land it so it is one less thing from my long pending set... ^^ |
Add a per-builtin state mechanism so builtins can carry typed Rust state that persists across invocations within a shell instance. - Add AnyState trait with blanket impl for Clone+Send+Sync+'static types - Add Command::State associated type (defaults to () via explicit opt-in) - Add Command::state() / state_mut() ergonomic helpers (no turbofish) - Add state_init fn pointer to Registration, seeded at registration time - Add builtin_states HashMap to Shell with typed accessors - Seed builtin_states in Shell::new from builder-provided registrations - Add ExecutionContext::builtin_state[_mut] helpers - Add BuiltinStateNotRegistered error variant - Update all 53 existing Command impls with type State = () Note: Box<dyn AnyState> satisfies the blanket impl bounds, so all accessors use explicit deref (**state) to dispatch through the vtable rather than the blanket impl on Box itself. The Clone impl for Box<dyn AnyState> uses the same pattern to avoid infinite recursion. Ref: reubeno#1149
Replace the hidden shell variables (`__GETOPTS_NEXT_CHAR` and `__GETOPTS_LAST_OPTIND`) with a typed `GetOptsState` struct stored via the new stateful builtin infrastructure. The state holds: - `next_char_index`: position within a combined-flag argument - `last_optind`: the raw OPTIND value we last set (as i32), to detect external changes including OPTIND=0 Uses the ergonomic `self.state(&context)` / `self.state_mut(&mut context)` helpers from the Command trait, requiring no turbofish. Ref: reubeno#1149
Add type-safe shared state to brush-core's builtin system, allowing multiple builtins to share state (e.g. eclass AST caches) across a shell and its clones via SharedBuilder/SharedHandle. - Registration<SE, S, L> with phantom types for shared-state routing and local-state typestate (NeedsLocalState<St> / HasLocalState) - with_state(state: St) for custom per-builtin initial state - SharedBuilder<T> consuming builder for bulk shared registration - SharedHandle<T> for runtime addition against existing shared state - Shell::shared_states HashMap<TypeId, Box<dyn AnyState>> - ExecutionContext::shared::<T>() for immutable shared state access - SharedStateNotRegistered error variant - Removed register_builtin_with_state (superseded by with_state())
79c9c2f to
8572790
Compare
Summary
Closes #1149
Adds a per-builtin state mechanism so builtins can carry typed Rust state that persists across invocations within a shell instance, without resorting to hidden shell variables or global mutable state.
Infrastructure (
bb64b510)AnyStatetrait with blanket impl forClone + Send + Sync + 'statictypes — any suitable struct can be stored as builtin state with zero boilerplateCommand::Stateassociated type on the existingCommandtrait — defaults to()for all 53 existing builtins (no behavior change)Command::state()/state_mut()ergonomic helpers — builtins callself.state(&context)?with no turbofishstate_initfn pointer onRegistration— seeded automatically when builtins are registered viaShell::register_builtinorShell::newbuiltin_states: HashMap<String, Box<dyn AnyState>>onShellwith typed accessors (builtin_state_of::<B>,builtin_state_mut::<B>,builtin_state::<T>,builtin_state_mut::<T>)ExecutionContexthelpers —context.builtin_state::<Self>()/context.builtin_state_mut::<Self>()BuiltinStateNotRegisterederror variantSmoke test: getopts conversion (
1e2ed4a3)Converts
getoptsfrom hidden shell variables (__GETOPTS_NEXT_CHAR,__GETOPTS_LAST_OPTIND) to a typedGetOptsStatestruct:Usage pattern — borrow state, copy out, release, do work, borrow mutably, write back:
Net result: 48 fewer lines, no hidden variables leaking into the shell environment, type-safe state access.
Testing
AnyStateunit tests (clone roundtrip, mut roundtrip, wrong type, downcast, complex type)cargo clippy,cargo fmt --check, schema check — all clean