fix: support export -a NAME (declare-as-array + export); unblocks mise activate bash#1133
Draft
melderan wants to merge 1 commit into
Draft
fix: support export -a NAME (declare-as-array + export); unblocks mise activate bash#1133melderan wants to merge 1 commit into
export -a NAME (declare-as-array + export); unblocks mise activate bash#1133melderan wants to merge 1 commit into
Conversation
The `export` builtin now accepts `-a`, mirroring bash's combined "declare as indexed array + mark exported" behavior. This is what `mise activate bash` relies on when seeding `chpwd_functions` early in its initialization, so before this change every mise-activated brush session printed `error: unexpected argument '-a' found` to stderr (mise still loaded, but the `chpwd_functions` array wasn't properly set up). Behavior: - `export -a NAME` on an existing variable: convert it to an indexed array and mark it exported. - `export -a NAME` on a name that doesn't yet exist: create it as an unset indexed array with the export attribute. - `export -a NAME=value` (scalar): auto-promote to indexed array with `[0]=value` and mark exported, matching bash's `declare -ax`. - `export -a NAME=(a b c)` (array literal): standard array initialization with the export attribute. - `export -an NAME`: combined with `-n`, removes the export attribute on an indexed array. Adds compat tests for each shape under `brush-shell/tests/cases/compat/builtins/export.yaml`. Note: bash is *lazy* about showing the `-a` attribute in `declare -p` output for a declared-but-unassigned indexed array (it shows `-x` until the array is populated, then upgrades to `-ax`); brush is *eager* (shows `-ax` immediately after the declaration). The functional behavior is identical — only the cosmetic `declare -p` output for an empty declared array differs. The new tests exercise the assignment paths, where bash and brush agree exactly. Closes reubeno#1132. Assisted-by: Claude (Anthropic) (JMO's friend)
Owner
|
Thanks for the contribution, @melderan! We particularly appreciate the rest additions and clear example use case positively impacted by the improvement. I've unblocked the CI checks to run. When you're ready for a formal review, please feel free to take this out of draft. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the export builtin to accept -a and apply “declare as indexed array” semantics while also setting/removing the export attribute, matching bash behavior needed by mise activate bash (e.g., export -a chpwd_functions).
Changes:
- Add
-aflag parsing toexportand implement array promotion / array declaration + export behavior for both bare names and assignments. - Create missing variables as unset indexed arrays when
export -a NAMEis used. - Add compat test cases covering
export -awith element assignment, scalar auto-promotion, array literal assignment, scalar→array conversion, and unexport behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| brush-builtins/src/export.rs | Adds -a support to export, including array conversion and creating unset indexed arrays when needed. |
| brush-shell/tests/cases/compat/builtins/export.yaml | Adds bash-oracle compat cases for the new export -a behavior. |
Comment on lines
+104
to
+118
| // If `-a` was passed and the name doesn't yet exist, create it as an unset | ||
| // indexed array with the export attribute (mirrors `declare -ax NAME`). This | ||
| // is what bash does for `export -a NAME` and what `mise activate bash` relies | ||
| // on when seeding `chpwd_functions`. | ||
| else if self.make_indexed_array { | ||
| let mut var = | ||
| ShellVariable::new(ShellValue::Unset(ShellValueUnsetType::IndexedArray)); | ||
| if !self.unexport { | ||
| var.export(); | ||
| } | ||
| context | ||
| .shell | ||
| .env_mut() | ||
| .add(s.clone(), var, EnvironmentScope::Global)?; | ||
| } |
Comment on lines
+143
to
+145
| // When `-a` is set, ensure the variable is converted to an indexed array | ||
| // before the assignment lands (so a scalar assignment like `export -a foo=x` | ||
| // ends up as `foo=([0]="x")`, matching bash's `declare -ax foo=([0]="x")`). |
| - name: "export -an unmarks an indexed array's export attribute" | ||
| stdin: | | ||
| export -a arr=(x y z) | ||
| export -n arr |
Performance Benchmark Report
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is Test Summary: bash-completion test suite
|
Owner
|
Hi @melderan -- are you okay taking this out of draft for formal review? It was looking in relatively good shape CI-wise. |
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
Implements
export -a NAME(declare-as-array combined with export attribute), so the builtin no longer rejects the-aflag. This matches bash's behavior and unblocksmise activate bash, which emitsexport -a chpwd_functionsearly in its setup — without this fix, every mise-activated brush session printserror: unexpected argument '-a' foundto stderr (mise still loads, but thechpwd_functionsarray isn't properly seeded).Closes #1132.
Behavior
export -a NAMEon an existing variable: convert to indexed array, mark exportedexport -a NAMEon a name that doesn't yet exist: create as unset indexed array with export attributeexport -a NAME=value(scalar): auto-promote to indexed array with[0]=valueand mark exported, matching bash'sdeclare -ax NAME=([0]="value")export -a NAME=(a b c)(array literal): standard array initialization with export attributeexport -an NAME: combined with-n, removes the export attribute on an indexed arrayTests
Added five compat tests under
brush-shell/tests/cases/compat/builtins/export.yaml:export -afollowed by element assignments (arr[0]=x; arr[1]=y)export -a NAME=valuescalar auto-promoteexport -a NAME=(a b c)array literalexport -aconverting an existing scalar to an indexed arrayexport -anremoving the export attribute from an indexed arrayEach is verified against bash via the compat oracle.
Known cosmetic divergence (documented, not fixed in this PR)
Bash is lazy about showing the
-aattribute indeclare -pfor a declared-but-unassigned indexed array — it printsdeclare -x foountil the array is populated, then upgrades todeclare -ax foo=([0]="..."). Brush is eager — it printsdeclare -ax fooimmediately afterexport -a foo. The functional behavior is identical (the array is correctly typed and exported in both cases); only thedeclare -prepresentation for an empty declared array differs. The new tests therefore exercise the assignment paths, where bash and brush agree exactly. Happy to follow up with a "lazy attribute display" change if the maintainer wants exactdeclare -pparity, but it felt out of scope for this fix.Local verification
cargo xtask ci pre-commit— fmt, build, lint, deps, schemas, unit tests all green; integration tests pass except for the pre-existingbrush-interactive-tests::run_in_bg_then_fgflake unrelated to this change (also fails onmainon this dev machine; the local interactive.bashrcreferencesmise/direnv/atuin/zoxidehook functions that aren't present in CI).mise activate bash | brush -c '. /dev/stdin && type mise'now succeeds with no stderr noise.Filed as a draft for early input.
Assisted-by: Claude (Anthropic) (JMO's friend)