Skip to content

Commit 1d64333

Browse files
ChemaclassChemaclass
authored
ref(bash): codify dynamic-scope safety for outvar helpers (#675)
1 parent 1400383 commit 1d64333

5 files changed

Lines changed: 231 additions & 96 deletions

File tree

‎.claude/rules/bash-style.md‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,94 @@ Constants -> Globals -> Private functions -> Public functions
4949

5050
Source deps relative to script: `"$(dirname "${BASH_SOURCE[0]}")/dep.sh"`
5151

52+
## Returning values from a helper (dynamic-scope safety)
53+
54+
Bash `local` is **dynamically scoped**, not lexically scoped. A `local` inside a callee
55+
shadows the caller's same-named variable for the duration of the call. Same trap fires
56+
with `${!name}` reads and `eval "$name=..."`/`printf -v "$name"`/`export "$name"=...`
57+
writes — they all resolve against the dynamic scope.
58+
59+
We need **all three** properties on the hot path:
60+
61+
1. **No subshell fork.** Returning via stdout and capturing with `$(...)` costs a fork
62+
per call, which dominates per-test cost.
63+
2. **No collision with caller locals.** A helper that silently overwrites or reads
64+
the wrong variable is the worst kind of bug — no error, just stale data.
65+
3. **Bash 3.0+ portable.** Rules out `declare -n` (4.3+) and `printf -v` (3.1+, and
66+
it has the same dynamic-scope shadowing as `eval "$name=..."` anyway).
67+
68+
### Preferred: dedicated global return slot
69+
70+
```bash
71+
_BASHUNIT_PKG_THING_OUT=""
72+
73+
# Writes the result into _BASHUNIT_PKG_THING_OUT.
74+
# Arguments: $1 input
75+
function bashunit::pkg::do_thing() {
76+
local input=$1 # natural name, no prefix needed
77+
local val="${input%%]*}"
78+
val="${val#[}"
79+
_BASHUNIT_PKG_THING_OUT=$val
80+
}
81+
82+
# Caller
83+
bashunit::pkg::do_thing "$payload"
84+
local thing=$_BASHUNIT_PKG_THING_OUT
85+
```
86+
87+
- Zero forks per call.
88+
- Helper has natural local names; no `__bu_` noise.
89+
- Caller cannot accidentally shadow the slot because the `_BASHUNIT_*` namespace
90+
is reserved for the framework.
91+
- A dedicated slot per helper (rather than one shared `_BASHUNIT_OUT`) means
92+
adjacent or nested calls can't clobber each other. Cheap: globals are free.
93+
94+
Examples in tree: `src/runner.sh` (`_BASHUNIT_RUNNER_FIELD_OUT`,
95+
`_BASHUNIT_RUNNER_TOTAL_OUT`, `_BASHUNIT_RUNNER_TYPE_OUT`, `_BASHUNIT_RUNNER_OUTPUT_OUT`),
96+
`src/coverage.sh` (`_BASHUNIT_BRANCH_ARMS_OUT`).
97+
98+
### When the helper builds dynamic variable names (mock/spy state)
99+
100+
Use a clear namespace prefix on the **constructed** name, not on the helper's locals:
101+
102+
```bash
103+
# Spy state lives in _BASHUNIT_SPY_${variable}_TIMES_FILE, not ${variable}_times_file.
104+
# A caller doing `local foo_times_file=...` is therefore harmless: the helper
105+
# resolves a different global.
106+
export "_BASHUNIT_SPY_${variable}_TIMES_FILE"="$times_file"
107+
```
108+
109+
Example in tree: `src/test_doubles.sh` (`_BASHUNIT_SPY_*`).
110+
111+
### Last-resort fallback: outvar by name + `__bu_` prefix on locals
112+
113+
Only use this when neither a fixed return slot nor a namespaced constructed name is
114+
practical (e.g. a generic helper called from many call sites with different output
115+
variables and no shared global is appropriate):
116+
117+
```bash
118+
function bashunit::pkg::do_thing() {
119+
local __bu_out=$1
120+
local __bu_in=$2
121+
local __bu_val="${__bu_in%%]*}"
122+
eval "$__bu_out=\$__bu_val"
123+
}
124+
```
125+
126+
- All internal locals MUST be `__bu_`-prefixed; otherwise dynamic-scope shadowing
127+
silently breaks the caller's outvar (see PR #672 — that exact bug caused 12 parallel
128+
test failures in #662).
129+
- Include a regression test that calls the helper with one of its own documented
130+
internal names as the outvar.
131+
132+
### Intentional dynamic-scope mutation is a separate pattern
133+
134+
The coverage branch helpers in `src/coverage.sh` (`_branch_push_if` and friends) deliberately
135+
mutate caller locals (`if_decision_line`, `if_arms`, `if_depth`, `if_arm_start`). That is
136+
documented inline at `src/coverage.sh:818-821` and is **not** the outvar pattern — the
137+
helper has no `$1`-named outvar argument; the caller agrees to share state by convention.
138+
Don't introduce new instances of this pattern without an inline justification comment.
139+
52140
## ShellCheck
53141

54142
All code must pass `make sa`. Use directives sparingly with reason:

‎CHANGELOG.md‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,15 @@
22

33
## Unreleased
44

5+
### Internal
6+
- Codify global-slot return pattern for hot-path helpers; namespace mock/spy state under `_BASHUNIT_SPY_*` (#674)
7+
58
### Performance
6-
- Faster runtime-error detection: single `case` glob instead of 23-iteration loop in `detect_runtime_error` (#668)
7-
- Hot-path coverage flag now cached in `_BASHUNIT_COVERAGE_ON`, removing a function dispatch per call (#664)
8-
- Parallel runner blocks on `wait -n` on Bash 4.3+ instead of polling `jobs -r`, removing sleep-induced slot-release latency (#667)
9-
- Hot-path result helpers (`extract_encoded_field`, `extract_subshell_type`, `format_subshell_output`, `compute_total_assertions`) use outvar pattern, dropping a fork per call per test (#662)
10-
- `generate_id` and `normalize_variable_name` drop `grep` and `random_str` forks via pure-bash globbing/inlining (#663)
9+
- Faster runtime-error detection: single `case` glob (#668)
10+
- Cache coverage-enabled flag in hot path (#664)
11+
- Parallel runner uses `wait -n` on Bash 4.3+ instead of polling (#667)
12+
- Outvar pattern in hot-path result helpers, dropping a fork per call (#662)
13+
- Drop `grep`/`random_str` forks in `generate_id` and `normalize_variable_name` (#663)
1114

1215
## [0.36.0](https://github.com/TypedDevs/bashunit/compare/0.35.0...0.36.0) - 2026-05-07
1316

‎src/runner.sh‎

Lines changed: 69 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -57,71 +57,76 @@ function bashunit::runner::apply_interpolated_title() {
5757
printf '%s' "$interpolated"
5858
}
5959

60-
# All four helpers below use the outvar pattern (first argument is the name of
61-
# the variable to assign into) so callers can avoid the per-test $(...) subshell
62-
# capture in the hot path. Internal locals use a `__bu_` prefix to avoid name
63-
# collisions with caller variables passed by name.
64-
65-
# Writes the value of an encoded field (##KEY=value##) into the named outvar.
66-
# Arguments: $1 outvar name, $2 test_execution_result, $3 key
60+
# Hot-path result helpers below return their value via a dedicated global slot
61+
# (`_BASHUNIT_RUNNER_*_OUT`) instead of stdout. This avoids the per-test
62+
# `$(...)` subshell capture that dominated the result-parsing hot path. Callers
63+
# invoke the helper and immediately read the slot:
64+
#
65+
# bashunit::runner::extract_subshell_type "$subshell_output"
66+
# type=$_BASHUNIT_RUNNER_TYPE_OUT
67+
#
68+
# A dedicated slot per helper (rather than one shared slot) means nested or
69+
# adjacent calls cannot clobber each other and callers don't need to copy out
70+
# before every other helper runs.
71+
_BASHUNIT_RUNNER_FIELD_OUT=""
72+
_BASHUNIT_RUNNER_TOTAL_OUT=""
73+
_BASHUNIT_RUNNER_TYPE_OUT=""
74+
_BASHUNIT_RUNNER_OUTPUT_OUT=""
75+
76+
# Writes the value of an encoded field (##KEY=value##) into _BASHUNIT_RUNNER_FIELD_OUT.
77+
# Arguments: $1 test_execution_result, $2 key
6778
function bashunit::runner::extract_encoded_field() {
68-
local __bu_out=$1
69-
local __bu_in=$2
70-
local __bu_key=$3
71-
local __bu_marker="##${__bu_key}="
72-
local __bu_val=""
73-
case "$__bu_in" in
74-
*"$__bu_marker"*)
75-
local __bu_rest="${__bu_in#*"$__bu_marker"}"
76-
__bu_val="${__bu_rest%%##*}"
79+
local test_execution_result=$1
80+
local key=$2
81+
local marker="##${key}="
82+
case "$test_execution_result" in
83+
*"$marker"*)
84+
local rest="${test_execution_result#*"$marker"}"
85+
_BASHUNIT_RUNNER_FIELD_OUT="${rest%%##*}"
7786
;;
87+
*) _BASHUNIT_RUNNER_FIELD_OUT="" ;;
7888
esac
79-
eval "$__bu_out=\$__bu_val"
8089
}
8190

82-
# Writes the sum of all ASSERTIONS_* counters into the named outvar.
83-
# Arguments: $1 outvar name, $2 test_execution_result
91+
# Writes the sum of all ASSERTIONS_* counters into _BASHUNIT_RUNNER_TOTAL_OUT.
92+
# Arguments: $1 test_execution_result
8493
function bashunit::runner::compute_total_assertions() {
85-
local __bu_out=$1
86-
local __bu_in=$2
87-
local __bu_failed __bu_passed __bu_skipped __bu_incomplete __bu_snapshot
88-
__bu_failed="${__bu_in##*##ASSERTIONS_FAILED=}"
89-
__bu_failed="${__bu_failed%%##*}"
90-
__bu_passed="${__bu_in##*##ASSERTIONS_PASSED=}"
91-
__bu_passed="${__bu_passed%%##*}"
92-
__bu_skipped="${__bu_in##*##ASSERTIONS_SKIPPED=}"
93-
__bu_skipped="${__bu_skipped%%##*}"
94-
__bu_incomplete="${__bu_in##*##ASSERTIONS_INCOMPLETE=}"
95-
__bu_incomplete="${__bu_incomplete%%##*}"
96-
__bu_snapshot="${__bu_in##*##ASSERTIONS_SNAPSHOT=}"
97-
__bu_snapshot="${__bu_snapshot%%##*}"
98-
local __bu_val
99-
__bu_val=$((${__bu_failed:-0} + ${__bu_passed:-0} + ${__bu_skipped:-0}))
100-
__bu_val=$((__bu_val + ${__bu_incomplete:-0} + ${__bu_snapshot:-0}))
101-
eval "$__bu_out=\$__bu_val"
94+
local test_execution_result=$1
95+
local failed passed skipped incomplete snapshot
96+
failed="${test_execution_result##*##ASSERTIONS_FAILED=}"
97+
failed="${failed%%##*}"
98+
passed="${test_execution_result##*##ASSERTIONS_PASSED=}"
99+
passed="${passed%%##*}"
100+
skipped="${test_execution_result##*##ASSERTIONS_SKIPPED=}"
101+
skipped="${skipped%%##*}"
102+
incomplete="${test_execution_result##*##ASSERTIONS_INCOMPLETE=}"
103+
incomplete="${incomplete%%##*}"
104+
snapshot="${test_execution_result##*##ASSERTIONS_SNAPSHOT=}"
105+
snapshot="${snapshot%%##*}"
106+
local total
107+
total=$((${failed:-0} + ${passed:-0} + ${skipped:-0}))
108+
total=$((total + ${incomplete:-0} + ${snapshot:-0}))
109+
_BASHUNIT_RUNNER_TOTAL_OUT=$total
102110
}
103111

104-
# Writes the subshell type marker (text inside leading [...]) into the named outvar.
105-
# Arguments: $1 outvar name, $2 subshell_output
112+
# Writes the subshell type marker (text inside leading [...]) into _BASHUNIT_RUNNER_TYPE_OUT.
113+
# Arguments: $1 subshell_output
106114
function bashunit::runner::extract_subshell_type() {
107-
local __bu_out=$1
108-
local __bu_in=$2
109-
local __bu_val="${__bu_in%%]*}"
110-
__bu_val="${__bu_val#[}"
111-
eval "$__bu_out=\$__bu_val"
115+
local subshell_output=$1
116+
local type="${subshell_output%%]*}"
117+
_BASHUNIT_RUNNER_TYPE_OUT="${type#[}"
112118
}
113119

114120
# Writes the subshell output (minus the leading [type] marker, with embedded
115-
# status markers replaced by newlines) into the named outvar.
116-
# Arguments: $1 outvar name, $2 subshell_output
121+
# status markers replaced by newlines) into _BASHUNIT_RUNNER_OUTPUT_OUT.
122+
# Arguments: $1 subshell_output
117123
function bashunit::runner::format_subshell_output() {
118-
local __bu_out=$1
119-
local __bu_in=$2
120-
local __bu_val="${__bu_in#*]}"
121-
__bu_val=${__bu_val//\[failed\]/$'\n'}
122-
__bu_val=${__bu_val//\[skipped\]/$'\n'}
123-
__bu_val=${__bu_val//\[incomplete\]/$'\n'}
124-
eval "$__bu_out=\$__bu_val"
124+
local subshell_output=$1
125+
local line="${subshell_output#*]}"
126+
line=${line//\[failed\]/$'\n'}
127+
line=${line//\[skipped\]/$'\n'}
128+
line=${line//\[incomplete\]/$'\n'}
129+
_BASHUNIT_RUNNER_OUTPUT_OUT=$line
125130
}
126131

127132
function bashunit::runner::detect_runtime_error() {
@@ -837,9 +842,10 @@ function bashunit::runner::run_test() {
837842
local subshell_output=$(bashunit::runner::decode_subshell_output "$test_execution_result")
838843

839844
if [ -n "$subshell_output" ]; then
840-
local type
841-
bashunit::runner::extract_subshell_type type "$subshell_output"
842-
bashunit::runner::format_subshell_output subshell_output "$subshell_output"
845+
bashunit::runner::extract_subshell_type "$subshell_output"
846+
local type=$_BASHUNIT_RUNNER_TYPE_OUT
847+
bashunit::runner::format_subshell_output "$subshell_output"
848+
subshell_output=$_BASHUNIT_RUNNER_OUTPUT_OUT
843849
if ! bashunit::env::is_failures_only_enabled; then
844850
bashunit::state::print_line "$type" "$subshell_output"
845851
fi
@@ -854,13 +860,15 @@ function bashunit::runner::run_test() {
854860

855861
local test_exit_code="$_BASHUNIT_TEST_EXIT_CODE"
856862

857-
local total_assertions
858-
bashunit::runner::compute_total_assertions total_assertions "$test_execution_result"
863+
bashunit::runner::compute_total_assertions "$test_execution_result"
864+
local total_assertions=$_BASHUNIT_RUNNER_TOTAL_OUT
859865

860-
local encoded_test_title hook_failure encoded_hook_message
861-
bashunit::runner::extract_encoded_field encoded_test_title "$test_execution_result" "TEST_TITLE"
862-
bashunit::runner::extract_encoded_field hook_failure "$test_execution_result" "TEST_HOOK_FAILURE"
863-
bashunit::runner::extract_encoded_field encoded_hook_message "$test_execution_result" "TEST_HOOK_MESSAGE"
866+
bashunit::runner::extract_encoded_field "$test_execution_result" "TEST_TITLE"
867+
local encoded_test_title=$_BASHUNIT_RUNNER_FIELD_OUT
868+
bashunit::runner::extract_encoded_field "$test_execution_result" "TEST_HOOK_FAILURE"
869+
local hook_failure=$_BASHUNIT_RUNNER_FIELD_OUT
870+
bashunit::runner::extract_encoded_field "$test_execution_result" "TEST_HOOK_MESSAGE"
871+
local encoded_hook_message=$_BASHUNIT_RUNNER_FIELD_OUT
864872

865873
local test_title=""
866874
[ -n "$encoded_test_title" ] && test_title="$(bashunit::helper::decode_base64 "$encoded_test_title")"

‎src/test_doubles.sh‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,11 @@
22

33
declare -a _BASHUNIT_MOCKED_FUNCTIONS=()
44

5+
# Spy/mock state is keyed by the sanitized command name and stored in globals
6+
# prefixed with `_BASHUNIT_SPY_` so they cannot collide with caller locals.
7+
# A test that does `local foo_times_file=...` is harmless because the helper
8+
# resolves `_BASHUNIT_SPY_foo_TIMES_FILE` instead.
9+
510
function bashunit::unmock() {
611
local command=$1
712

@@ -16,8 +21,8 @@ function bashunit::unmock() {
1621
unset -f "$command"
1722
local variable
1823
variable="$(bashunit::helper::normalize_variable_name "$command")"
19-
local times_file_var="${variable}_times_file"
20-
local params_file_var="${variable}_params_file"
24+
local times_file_var="_BASHUNIT_SPY_${variable}_TIMES_FILE"
25+
local params_file_var="_BASHUNIT_SPY_${variable}_PARAMS_FILE"
2126
[ -f "${!times_file_var-}" ] && rm -f "${!times_file_var}"
2227
[ -f "${!params_file_var-}" ] && rm -f "${!params_file_var}"
2328
unset "$times_file_var"
@@ -54,8 +59,8 @@ function bashunit::spy() {
5459
params_file=$(bashunit::temp_file "${test_id}_${variable}_params")
5560
echo 0 >"$times_file"
5661
: >"$params_file"
57-
export "${variable}_times_file"="$times_file"
58-
export "${variable}_params_file"="$params_file"
62+
export "_BASHUNIT_SPY_${variable}_TIMES_FILE"="$times_file"
63+
export "_BASHUNIT_SPY_${variable}_PARAMS_FILE"="$params_file"
5964

6065
local body_suffix=""
6166
if [[ "$exit_code_or_impl" =~ ^[0-9]+$ ]]; then
@@ -89,7 +94,7 @@ function assert_have_been_called() {
8994
local command=$1
9095
local variable
9196
variable="$(bashunit::helper::normalize_variable_name "$command")"
92-
local file_var="${variable}_times_file"
97+
local file_var="_BASHUNIT_SPY_${variable}_TIMES_FILE"
9398
local times=0
9499
if [ -f "${!file_var-}" ]; then
95100
times=$(cat "${!file_var}" 2>/dev/null || builtin echo 0)
@@ -119,7 +124,7 @@ function assert_have_been_called_with() {
119124

120125
local variable
121126
variable="$(bashunit::helper::normalize_variable_name "$command")"
122-
local file_var="${variable}_params_file"
127+
local file_var="_BASHUNIT_SPY_${variable}_PARAMS_FILE"
123128
local line=""
124129
if [ -f "${!file_var-}" ]; then
125130
if [ -n "$index" ]; then
@@ -147,7 +152,7 @@ function assert_have_been_called_times() {
147152
local command=$2
148153
local variable
149154
variable="$(bashunit::helper::normalize_variable_name "$command")"
150-
local file_var="${variable}_times_file"
155+
local file_var="_BASHUNIT_SPY_${variable}_TIMES_FILE"
151156
local times=0
152157
if [ -f "${!file_var-}" ]; then
153158
times=$(cat "${!file_var}" 2>/dev/null || builtin echo 0)
@@ -172,8 +177,8 @@ function assert_have_been_called_nth_with() {
172177

173178
local variable
174179
variable="$(bashunit::helper::normalize_variable_name "$command")"
175-
local times_file_var="${variable}_times_file"
176-
local file_var="${variable}_params_file"
180+
local times_file_var="_BASHUNIT_SPY_${variable}_TIMES_FILE"
181+
local file_var="_BASHUNIT_SPY_${variable}_PARAMS_FILE"
177182
local label
178183
label="$(bashunit::helper::normalize_test_function_name "${FUNCNAME[1]}")"
179184

0 commit comments

Comments
 (0)