Skip to content

fix(builtins): handle quoted assignment arguments in declare#1147

Open
lu-zero wants to merge 1 commit into
reubeno:mainfrom
lu-zero:fix-quoted-declare-assignment
Open

fix(builtins): handle quoted assignment arguments in declare#1147
lu-zero wants to merge 1 commit into
reubeno:mainfrom
lu-zero:fix-quoted-declare-assignment

Conversation

@lu-zero

@lu-zero lu-zero commented May 6, 2026

Copy link
Copy Markdown
Contributor

When declare receives a single-quoted argument like 'arr=()', the parser cannot recognize it as an AssignmentWord (leading quote is not a valid variable name start). After word expansion strips the quotes, the builtin receives CommandArg::String("arr=(${X})") but never detected the = sign, treating the entire string as a variable name and failing validation.

Extend declaration_to_name_and_value to split on = for CommandArg::String args, handling both name=value and name[index]=value patterns. For scalar values the expanded string is used as-is (word expansion already handled quoting correctly). For array values with -a, add lift_string_array_assignment that reconstructs a declare command and runs it through the shell to let the parser and expander handle array parsing and parameter expansion at runtime.

This fixes the Gentoo rpm.eclass pattern:
IFS=, declare -a 'types=()'

@github-actions

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown

Performance Benchmark Report

Benchmark name Baseline (μs) Test/PR (μs) Delta (μs) Delta %
clone_shell_object 17.25 μs 17.47 μs 0.22 μs ⚪ Unchanged
eval_arithmetic 0.15 μs 0.16 μs 0.01 μs ⚪ Unchanged
expand_one_string 1.56 μs 1.51 μs -0.05 μs ⚪ Unchanged
for_loop 30.72 μs 29.66 μs -1.06 μs 🟢 -3.46%
full_peg_complex 57.76 μs 57.59 μs -0.17 μs ⚪ Unchanged
full_peg_for_loop 6.31 μs 6.30 μs -0.01 μs ⚪ Unchanged
full_peg_nested_expansions 16.47 μs 16.34 μs -0.13 μs ⚪ Unchanged
full_peg_pipeline 4.20 μs 4.21 μs 0.01 μs ⚪ Unchanged
full_peg_simple 1.79 μs 1.76 μs -0.03 μs ⚪ Unchanged
function_call 3.33 μs 3.25 μs -0.08 μs ⚪ Unchanged
instantiate_shell 52.80 μs 53.81 μs 1.01 μs 🟠 +1.91%
instantiate_shell_with_init_scripts 28227.40 μs 26160.81 μs -2066.59 μs 🟢 -7.32%
parse_peg_bash_completion 2104.71 μs 2095.56 μs -9.15 μs ⚪ Unchanged
parse_peg_complex 20.56 μs 19.38 μs -1.18 μs 🟢 -5.74%
parse_peg_for_loop 2.02 μs 2.03 μs 0.00 μs ⚪ Unchanged
parse_peg_pipeline 2.06 μs 2.06 μs -0.00 μs ⚪ Unchanged
parse_peg_simple 1.12 μs 1.10 μs -0.01 μs ⚪ Unchanged
run_echo_builtin_command 17.19 μs 17.11 μs -0.08 μs ⚪ Unchanged
tokenize_sample_script 3.39 μs 3.40 μs 0.00 μs ⚪ Unchanged

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
brush-builtins/src/declare.rs 🟢 87.4% 🟢 87.47% 🟢 0.07%
brush-core/src/results.rs 🟢 83.33% 🟢 80.16% 🔴 -3.17%
Overall Coverage 🟢 75.4% 🟢 75.42% 🟢 0.02%

Minimum allowed coverage is 70%, this run produced 75.42%

Test Summary: bash-completion test suite

Outcome Count Percentage
✅ Pass 1594 75.58
❗️ Error 18 0.85
❌ Fail 143 6.78
⏩ Skip 339 16.07
❎ Expected Fail 13 0.62
✔️ Unexpected Pass 2 0.09
📊 Total 2109 100.00

@lu-zero lu-zero force-pushed the fix-quoted-declare-assignment branch from 7af336b to 7e011dd Compare May 6, 2026 19:50
@reubeno

reubeno commented May 7, 2026

Copy link
Copy Markdown
Owner

Thanks for putting this together; I hit this while looking at bash completion tests.

@lu-zero lu-zero force-pushed the fix-quoted-declare-assignment branch from 7e011dd to 59dffc7 Compare May 8, 2026 04:53
Comment thread brush-builtins/src/declare.rs Outdated
Comment on lines +353 to +357
if let brush_core::CommandArg::String(s) = declaration {
if let Some((_name, value)) = s.split_once('=') {
return value.starts_with('(');
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be collapsed:

Suggested change
if let brush_core::CommandArg::String(s) = declaration {
if let Some((_name, value)) = s.split_once('=') {
return value.starts_with('(');
}
}
if let brush_core::CommandArg::String(s) = declaration
&& let Some((_, value)) = s.split_once('=')
{
return value.starts_with('(');
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, let me fold it in. Thank you :)

When declare receives a single-quoted argument like 'arr=()', the
parser cannot recognize it as an AssignmentWord (leading quote is not a
valid variable name start). After word expansion strips the quotes, the
builtin receives CommandArg::String("arr=(${X})") but never detected
the = sign, treating the entire string as a variable name and failing
validation.

Extend declaration_to_name_and_value to split on = for CommandArg::String
args, handling both name=value and name[index]=value patterns. For scalar
values the expanded string is used as-is (word expansion already handled
quoting correctly). For array values with -a, add lift_string_array_assignment
that reconstructs a declare command and runs it through the shell to let
the parser and expander handle array parsing and parameter expansion at
runtime.

This fixes the Gentoo rpm.eclass pattern:
  IFS=, declare -a 'types=()'
@lu-zero lu-zero force-pushed the fix-quoted-declare-assignment branch 2 times, most recently from ba20ad1 to 2417eff Compare May 8, 2026 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants