fix(core)!: centralize assignment expansion + overhaul declaration builtins

[reubeno]

Assignment expansion lived in three inline helpers in interp.rs and was applied uniformly, regardless of what the assignment targeted. That made two things impossible: a declaration builtin could not apply the subscript rules of the array type it was about to create, and the interpreter could not expand an operand's words at the moment a shell does — before the command runs.

This centralizes the logic in expansion.rs and splits it into the two passes a shell actually performs:

• a word pass, run by the interpreter before the command executes, expanding values and compound keys;
• a subscript pass (Shell::resolve_assignment_subscripts), run by the declaration builtin once its options reveal the target array type — arithmetic for indexed arrays, ordinary word expansion for associative keys.

Ordinary name=value statements compose both in one call (Shell::expand_assignment).

The ordering is the point: a shell expands an assignment's words before evaluating any arithmetic subscript, so side effects in a subscript are not visible to the value being assigned. i=0; a[i++]=$i now stores 0, as bash does, where it stored 1.

declare, export, local, readonly and unset all route through the shared resolver, so they agree with bash on array conversion, readonly refusals, bad subscripts, and compound values that stop at a bad key.

Assignment and declaration

• Declaration operands are expanded where a shell expands them. declare -a 'arr=(${X})', declare -a "arr=$(printf '(%s)' '${X}')", and command declare x=$v all work. Splitting at the right moment is what makes IFS=, declare -a arr=($X) keep the value whole while IFS=, declare -a 'arr=(${X})' splits it.
• Subscripts resolve against the target's kind, with -a/-A overriding an existing type, so declare -A 'map[$key]=v' uses the literal key.
• +a/+A refuse with a reason instead of an internal error, and a value merely ending in ] is no longer read as a subscripted name.
• Readonly is enforced for array elements. arr=(one); readonly arr; arr[0]=x silently mutated the array — on main too. The guard now sits where every element-assignment path converges, covering the declaration, arithmetic, assignment-expansion, and mapfile paths.
• local -I resolves subscripts against the variable it inherits, not the local about to shadow it, so an associative key is no longer arithmetically evaluated to 0.
• a[0]=(9 9) reports "cannot assign list to array member" instead of "not yet implemented".
• Attribute display honors -t (previously a copy-paste of the -r filter) and lists declared-but-unassigned arrays.

Which failures abandon the command list

An assignment error is a shell's own refusal to assign, and only that abandons the rest of the command list. Any failure on an unquoted compound operand used to qualify, so an unimplemented case stopped the whole script instead of failing one command. ErrorKind now names the two classes and callers ask.

Refusals also name the variable they concern wherever they are raised, so read, mapfile and the arithmetic assignment operators report r: readonly variable rather than nothing useful.

unset

unset a[i] carried its own copy of the subscript rule. It now uses the shared resolver, so a[*], a[@] and an empty subscript behave as in bash. A readonly variable or element is refused and reported without stopping the remaining names. Readonly functions are refused too.

Also here

• BASH_COMMAND reports the command as written rather than as expanded, so a DEBUG trap sees echo $greeting, not echo hello.
• $(( )) evaluates to 0. The empty-expression rule matched only at end of input, so $(( )) parsed but $(( )) did not. A blank expression is now 0 wherever it comes from — $(( )), let '', or a blank array subscript.
• declare.rs is split. The display half moves to declare/display.rs; local and readonly get their own command structs, so each accepts only the options it really takes.
• Each builtin feature builds on its own again. mod local's cfg attribute had landed above mod pushd's, gating each on the other's feature. (builtin.dirs, builtin.exec, builtin.popd and builtin.pushd still do not build alone; that predates this branch.)
• fancy-regex is dropped from brush-builtins; the parser now reads name[index]=value.

API

brush-core gains Shell::{expand_assignment, resolve_assignment_subscripts, resolve_array_subscript}, env::ShellEnvironment::{subscript_kind, unset_all_indices}, expansion::ResolvedAssignment, variables::{ArrayKind, ScalarConversionPolicy, ShellValue::array_kind, ShellVariable::assign_at, ShellVariable::unset_all_indices}, ExecutionContext::trace_extra_line, and readonly/trace accessors on functions::Registration.

Breaking:

• ErrorKind gains variants and is not non_exhaustive.
• Error::source() yields the kind's own source rather than the kind, so printing the chain no longer repeats the message.
• EvalError::FailedToUpdateEnvironment carries a String.
• ShellVariable::convert_to_indexed_array and convert_to_associative_array are replaced by convert_to_array_kind.
• Shell::define_func and Shell::undefine_func return Result, refusing a readonly function.

Testing

Compat cases go from 2521 to 2691, and known_failure markers from 482 to 435. Seventy-nine cases previously marked known_failure now pass — 56 in declare, the rest across set -x, functrace, export, command, trap, readonly and extdebug. No previously passing case regresses.

Thirty-two of the new cases are recorded as known failures rather than fixed here, covering pre-existing gaps this work surfaced: local -g, namerefs as assignment targets, declare's +X mode options, the function trace attribute, field splitting inside a compound value, statuses reported from inside a pipeline, and an arithmetic error abandoning the rest of a ;-separated list.

Nine cases are annotated min_oracle_version: "5.3". Bash 5.3 changed how a bad compound-array key and a readonly variable's attributes are handled; brush follows 5.3, and the aarch64 CI runner's oracle is 5.2.

[github-actions]

Test Results

    5 files     49 suites   23m 59s ⏱️
3 133 tests 3 133 ✅ 0 💤 0 ❌
9 694 runs  9 694 ✅ 0 💤 0 ❌

Results for commit 388a2ca.

♻️ This comment has been updated with latest results.

[github-actions]

Public API changes for crate: brush-core

Removed items

-pub fn brush_core::variables::ShellVariable::convert_to_associative_array(&mut self) -> core::result::Result<(), brush_core::error::Error>
-pub fn brush_core::variables::ShellVariable::convert_to_associative_array(&mut self) -> core::result::Result<(), brush_core::error::Error>
-pub fn brush_core::variables::ShellVariable::convert_to_indexed_array(&mut self) -> core::result::Result<(), brush_core::error::Error>
-pub fn brush_core::variables::ShellVariable::convert_to_indexed_array(&mut self) -> core::result::Result<(), brush_core::error::Error>

Added items

+pub async fn brush_core::commands::ExecutionContext<'_, SE>::trace_extra_line(&mut self, alloc::string::String)
+pub async fn brush_core::commands::ExecutionContext<'_, SE>::trace_extra_line(&mut self, alloc::string::String)
+pub fn brush_core::env::ShellEnvironment::subscript_kind(&self, &str) -> brush_core::variables::ArrayKind
+pub fn brush_core::env::ShellEnvironment::unset_all_indices(&mut self, &str) -> core::result::Result<bool, brush_core::error::Error>
+pub brush_core::error::ErrorKind::AssigningToNonNumericIndex(alloc::string::String)
+pub brush_core::error::ErrorKind::BadArraySubscript(alloc::string::String)
+pub brush_core::error::ErrorKind::ReadonlyFunction(alloc::string::String)
+impl brush_core::error::ErrorKind
+impl brush_core::error::ErrorKind
+pub const fn brush_core::error::ErrorKind::is_assignment_failure(&self) -> bool
+pub const fn brush_core::error::ErrorKind::is_assignment_failure(&self) -> bool
+pub const fn brush_core::error::ErrorKind::is_bad_element_key(&self) -> bool
+pub const fn brush_core::error::ErrorKind::is_bad_element_key(&self) -> bool
+pub fn brush_core::error::Error::for_variable(self, &str, core::option::Option<&str>) -> Self
+pub fn brush_core::error::Error::for_variable(self, &str, core::option::Option<&str>) -> Self
+pub const fn brush_core::error::Error::into_assignment_error(self) -> Self
+pub const fn brush_core::error::Error::into_assignment_error(self) -> Self
+pub fn brush_core::error::Error::is_assignment_error(&self) -> bool
+pub fn brush_core::error::Error::is_assignment_error(&self) -> bool
+pub fn brush_core::error::Error::is_assignment_error(&self) -> bool
+pub fn brush_core::error::Error::is_assignment_error(&self) -> bool
+pub fn brush_core::error::Error::is_assignment_error(&self) -> bool
+pub fn brush_core::error::Error::is_assignment_error(&self) -> bool
+impl core::error::Error for brush_core::error::Error
+impl core::error::Error for brush_core::error::Error
+pub fn brush_core::error::Error::source(&self) -> core::option::Option<&(dyn core::error::Error + 'static)>
+pub fn brush_core::error::Error::source(&self) -> core::option::Option<&(dyn core::error::Error + 'static)>
+impl core::fmt::Display for brush_core::error::Error
+impl core::fmt::Display for brush_core::error::Error
+pub fn brush_core::error::Error::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result
+pub fn brush_core::error::Error::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result
+pub fn brush_core::error::BuiltinError::is_assignment_error(&self) -> bool
+pub struct brush_core::expansion::ResolvedAssignment
+pub brush_core::expansion::ResolvedAssignment::assignment: brush_parser::ast::Assignment
+pub brush_core::expansion::ResolvedAssignment::stopped_by: core::option::Option<brush_core::error::Error>
+pub fn brush_core::functions::Registration::attribute_flags(&self) -> alloc::string::String
+pub const fn brush_core::functions::Registration::disable_trace(&mut self)
+pub const fn brush_core::functions::Registration::enable_trace(&mut self)
+pub const fn brush_core::functions::Registration::is_readonly(&self) -> bool
+pub const fn brush_core::functions::Registration::is_trace_enabled(&self) -> bool
+pub const fn brush_core::functions::Registration::set_readonly(&mut self)
+pub enum brush_core::variables::ArrayKind
+pub brush_core::variables::ArrayKind::Associative
+pub brush_core::variables::ArrayKind::Indexed
+impl core::convert::From<brush_core::variables::ArrayKind> for brush_core::variables::ShellValueUnsetType
+impl core::convert::From<brush_core::variables::ArrayKind> for brush_core::variables::ShellValueUnsetType
+pub fn brush_core::variables::ShellValueUnsetType::from(brush_core::variables::ArrayKind) -> Self
+pub fn brush_core::variables::ShellValueUnsetType::from(brush_core::variables::ArrayKind) -> Self
+pub enum brush_core::variables::ScalarConversionPolicy
+pub brush_core::variables::ScalarConversionPolicy::Discard
+pub brush_core::variables::ScalarConversionPolicy::PromoteToElementZero
+pub const fn brush_core::variables::ShellValue::array_kind(&self) -> core::option::Option<brush_core::variables::ArrayKind>
+pub const fn brush_core::variables::ShellValue::array_kind(&self) -> core::option::Option<brush_core::variables::ArrayKind>
+impl core::convert::From<brush_parser::ast::AssignmentValue> for brush_core::variables::ShellValueLiteral
+pub fn brush_core::variables::ShellValueLiteral::from(brush_parser::ast::AssignmentValue) -> Self
+pub fn brush_core::variables::ShellVariable::assign_at(&mut self, core::option::Option<alloc::string::String>, brush_core::variables::ShellValueLiteral, bool) -> core::result::Result<(), brush_core::error::Error>
+pub fn brush_core::variables::ShellVariable::assign_at(&mut self, core::option::Option<alloc::string::String>, brush_core::variables::ShellValueLiteral, bool) -> core::result::Result<(), brush_core::error::Error>
+pub fn brush_core::variables::ShellVariable::convert_to_array_kind(&mut self, brush_core::variables::ArrayKind, brush_core::variables::ScalarConversionPolicy) -> core::result::Result<(), brush_core::error::Error>
+pub fn brush_core::variables::ShellVariable::convert_to_array_kind(&mut self, brush_core::variables::ArrayKind, brush_core::variables::ScalarConversionPolicy) -> core::result::Result<(), brush_core::error::Error>
+pub fn brush_core::variables::ShellVariable::resolve_dynamic(&mut self, &brush_core::Shell<impl brush_core::extensions::ShellExtensions>)
+pub fn brush_core::variables::ShellVariable::resolve_dynamic(&mut self, &brush_core::Shell<impl brush_core::extensions::ShellExtensions>)
+pub fn brush_core::variables::ShellVariable::unset_all_indices(&mut self) -> core::result::Result<bool, brush_core::error::Error>
+pub fn brush_core::variables::ShellVariable::unset_all_indices(&mut self) -> core::result::Result<bool, brush_core::error::Error>
+pub brush_core::ErrorKind::AssigningToNonNumericIndex(alloc::string::String)
+pub brush_core::ErrorKind::BadArraySubscript(alloc::string::String)
+pub brush_core::ErrorKind::ReadonlyFunction(alloc::string::String)
+pub async fn brush_core::Shell<SE>::expand_assignment(&mut self, &brush_core::ExecutionParameters, &brush_parser::ast::Assignment, brush_core::variables::ArrayKind) -> core::result::Result<brush_core::expansion::ResolvedAssignment, brush_core::error::Error>
+pub async fn brush_core::Shell<SE>::resolve_array_subscript(&mut self, &brush_core::ExecutionParameters, &str, brush_core::variables::ArrayKind) -> core::result::Result<alloc::string::String, brush_core::error::Error>
+pub async fn brush_core::Shell<SE>::resolve_assignment_subscripts(&mut self, &brush_core::ExecutionParameters, brush_parser::ast::Assignment, brush_core::variables::ArrayKind) -> core::result::Result<brush_core::expansion::ResolvedAssignment, brush_core::error::Error>
+pub fn brush_core::BuiltinError::is_assignment_error(&self) -> bool

Changed items

-pub brush_core::arithmetic::EvalError::FailedToUpdateEnvironment
+pub brush_core::arithmetic::EvalError::FailedToUpdateEnvironment(alloc::string::String)
-pub fn brush_core::Shell<SE>::define_func(&mut self, impl core::convert::Into<alloc::string::String>, brush_parser::ast::FunctionDefinition, &brush_core::sourceinfo::SourceInfo)
+pub fn brush_core::Shell<SE>::define_func(&mut self, impl core::convert::Into<alloc::string::String>, brush_parser::ast::FunctionDefinition, &brush_core::sourceinfo::SourceInfo) -> core::result::Result<(), brush_core::error::Error>
-pub fn brush_core::Shell<SE>::undefine_func(&mut self, &str) -> bool
+pub fn brush_core::Shell<SE>::undefine_func(&mut self, &str) -> core::result::Result<bool, brush_core::error::Error>

Performance Benchmark Report

Code Coverage Report: Only Changed Files listed

Package	Base Coverage	New Coverage	Difference
brush-core\src\builtins.rs	🔴 5.08%	🔴 5.03%	🔴 -0.05%
brush-core\src\env.rs	🔴 37.31%	🔴 36.06%	🔴 -1.25%
brush-core\src\error.rs	🔴 0%	🟠 50%	🟢 50%
brush-core\src\expansion.rs	🔴 27.91%	🔴 25.78%	🔴 -2.13%
brush-core\src\functions.rs	🔴 6%	🔴 3.66%	🔴 -2.34%
brush-core\src\shell\expansion.rs	🔴 27.27%	🔴 13.04%	🔴 -14.23%
brush-core\src\shell\funcs.rs	🔴 4.48%	🔴 4.05%	🔴 -0.43%
brush-core\src\variables.rs	🔴 10.81%	🔴 9.71%	🔴 -1.1%
brush-parser\src\arithmetic.rs	🔴 0%	🔴 23.3%	🟢 23.3%
Overall Coverage	🟢 28.38%	🟢 28.04%	🔴 -0.34%

Minimum allowed coverage is 20%, this run produced 28.04%
Maximum allowed coverage difference is -5%, this run produced -0.34%
brush-core/src/error.rs | 🟢 90.91% | 🟢 97.56% | 🟢 6.65% |
| brush-core/src/expansion.rs | 🟢 97.12% | 🟢 97.34% | 🟢 0.22% |
| brush-core/src/functions.rs | 🟢 80% | 🟢 85.37% | 🟢 5.37% |
| brush-core/src/interp.rs | 🟢 92.38% | 🟢 91.99% | 🔴 -0.39% |
| brush-core/src/shell/execution.rs | 🟢 100% | 🟢 96.34% | 🔴 -3.66% |
| brush-core/src/shell/expansion.rs | 🔴 27.27% | 🟠 65.22% | 🟢 37.95% |
| brush-core/src/shell/funcs.rs | 🟢 91.04% | 🟢 91.89% | 🟢 0.85% |
| brush-core/src/variables.rs | 🟢 93.14% | 🟢 91.53% | 🔴 -1.61% |
| brush-parser/src/arithmetic.rs | 🟢 92.63% | 🟢 93.2% | 🟢 0.57% |
| brush-parser/src/ast.rs | 🟠 58.16% | 🟠 65.25% | 🟢 7.09% |
| Overall Coverage | 🟢 77.15% | 🟢 77.96% | 🟢 0.81% |

Minimum allowed coverage is 70%, this run produced 77.96%
Maximum allowed coverage difference is -5%, this run produced 0.81%

Test Summary: bash-completion test suite

Outcome	Count	Percentage
✅ Pass	1597	75.72
❗️ Error	18	0.85
❌ Fail	140	6.64
⏩ Skip	339	16.07
❎ Expected Fail	13	0.62
✔️ Unexpected Pass	2	0.09
📊 Total	2109	100.00

[Copilot]

Pull request overview

Centralizes assignment expansion in brush-core and improves BASH_COMMAND source reporting.

Changes:

• Adds target-aware assignment expansion APIs.
• Updates interpreter command expansion and tracing.
• Enables previously failing compatibility cases.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
brush-core/src/expansion.rs	Adds centralized assignment expansion.
brush-core/src/shell/expansion.rs	Exposes assignment APIs through Shell.
brush-core/src/interp.rs	Uses centralized expansion and source text.
brush-core/src/commands.rs	Updates tracing and command arguments.
brush-shell/tests/cases/compat/options/functrace.yaml	Enables compatibility cases.
brush-shell/tests/cases/compat/options/extdebug.yaml	Enables a BASH_COMMAND case.
brush-shell/tests/cases/compat/builtins/trap.yaml	Enables an ERR-trap case.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Readonly array elements can be mutated, and inherited associative arrays use incorrect subscript resolution.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 19/19 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Clippy failure, feature coupling, and unversioned public API breaks remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 40/41 changed files
• Comments generated: 6
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Error chaining loses several underlying causes, while new API documentation contains private links and an advertised interface mismatch.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

brush-core/src/shell/expansion.rs:67

• This public rustdoc links to the crate-private expansion::resolve_array_subscript helper. Remove the private link (or deliberately expose the helper) so public documentation passes the denied private_intra_doc_links lint.

    /// Resolves one array subscript against the kind of the array it names. See
    /// [`expansion::resolve_array_subscript`].

brush-core/src/shell/expansion.rs:84

• This public rustdoc links to the crate-private expansion::resolve_assignment_subscripts helper, which can fail the workspace's denied private-link rustdoc lint. Keep this documentation self-contained instead.

    /// Resolves the subscripts of an assignment whose words were already expanded, leaving its
    /// values untouched. See [`expansion::resolve_assignment_subscripts`].

• Files reviewed: 40/41 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Associative wildcard unsets are incorrect, and failed compound assignments can retain attributes that should be rolled back.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

brush-builtins/src/declare/mod.rs:774

• The pre-assignment attributes are mutated before stopped_by is turned into an assignment error, and the early return in update_variable never rolls them back. Consequently declare -i i=(1 []=2) leaves i integer-typed even though the new compatibility case specifies that an unquoted compound refusal grants no option attributes; the same leak affects the case-transform, nameref, and trace flags. Apply these attributes only to temporary assignment state, then retain or restore them according to the final outcome.
  brush-builtins/src/unset.rs:186
• This treats * and @ as “clear all” for every array kind. For an associative array, Bash instead treats them as literal keys, so declare -A m=(['*']=v); unset 'm[*]' must remove that element; the current branch calls unset_all_indices, whose associative case is a no-op. Restrict the clear-all path to indexed arrays and let associative arrays flow through normal key resolution/removal.

• Files reviewed: 40/41 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Readonly empty-subscript unsets, failed compound attributes, and alias-expanded BASH_COMMAND remain incorrect.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

brush-core/src/interp.rs:1318

• self.to_string() is the pre-alias AST, while alias expansion has already replaced the command in next_args above. Bash exposes the alias-expanded command in BASH_COMMAND, so with alias e='echo'; e hi, the DEBUG trap will see e hi here instead of echo hi. Track source text after alias substitution but before ordinary word expansion rather than always using the original AST.

• Files reviewed: 40/41 changed files
• Comments generated: 2
• Review effort level: Balanced