Skip to content

Commit 29f26e4

Browse files
committed
fix(load): report the actual object-load status
.zi-load-object ended with ___retval+=$? return __retval `__retval' is assigned nowhere. Zsh evaluates a bare name in `return' arithmetically and an unset parameter is 0, so the helper always reported success: zsh -fc 'f(){ local ___retval=3; return __retval; }; f; print $?' -> 0 zsh -fc 'f(){ local ___retval=3; return ___retval; }; f; print $?' -> 3 The two sibling returns a few lines away use the correct three-underscore name. The sole caller already does the right thing: it stores the result in ___last_retval, adds it to ___retval once, and gates turbo scheduling on `0 == ___last_retval'. All of that was defeated by the constant 0, so a failed immediate load counted as success and turbo scheduling still went ahead. Fixing only the return would have double-counted, because the helper also added to the caller's dynamically scoped ___retval. Give the helper its own local result, return that, and drop the accumulation so the caller owns it, which is the contract the caller was already written for. tests/load-object-status.zsh stubs .zi-load and .zi-load-snippet, so the status under test is unambiguously the one supplied. It covers plugin and snippet on both the success and failure paths, and pins that the helper leaves the caller's ___retval alone. Against the unfixed helper it reports `plugin failure: expected 7, got 0'. Closes #446
1 parent 8104e93 commit 29f26e4

3 files changed

Lines changed: 106 additions & 2 deletions

File tree

.github/workflows/zsh-n.yml

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ on:
1212
- "tests/archive-extraction.zsh"
1313
- "tests/completion-refresh.zsh"
1414
- "tests/hook-ownership.zsh"
15+
- "tests/load-object-status.zsh"
1516
- "tests/message-formatting.zsh"
1617
- "tests/path-resolution.zsh"
1718
- "tests/parallel-update.zsh"
@@ -33,6 +34,7 @@ on:
3334
- "tests/archive-extraction.zsh"
3435
- "tests/completion-refresh.zsh"
3536
- "tests/hook-ownership.zsh"
37+
- "tests/load-object-status.zsh"
3638
- "tests/message-formatting.zsh"
3739
- "tests/path-resolution.zsh"
3840
- "tests/parallel-update.zsh"
@@ -147,6 +149,17 @@ jobs:
147149
run: sudo apt update && sudo apt-get install -yq zsh
148150
- name: Test autoload ice forms
149151
run: zsh -f tests/plugin-autoload-ice.zsh
152+
load-object-status:
153+
name: Load Object Status
154+
runs-on: ubuntu-latest
155+
steps:
156+
- name: Check out code
157+
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
158+
- name: Install Zsh
159+
run: sudo apt update && sudo apt-get install -yq zsh
160+
- name: Test load object status
161+
run: zsh -f tests/load-object-status.zsh
162+
150163
nested-load-state:
151164
name: Nested Load State
152165
runs-on: ubuntu-latest

tests/load-object-status.zsh

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
#!/usr/bin/env zsh
2+
# -*- mode: zsh; sh-indentation: 2; indent-tabs-mode: nil; sh-basic-offset: 2; -*-
3+
# vim: ft=zsh sw=2 ts=2 et
4+
5+
builtin emulate -R zsh
6+
setopt pipe_fail
7+
8+
fail() {
9+
builtin print -u2 -r -- "not ok - $1"
10+
exit 1
11+
}
12+
13+
typeset project_root="${ZI_TEST_CHECKOUT:-${0:A:h:h}}"
14+
typeset temp_root
15+
temp_root="$(command mktemp -d "${TMPDIR:-/tmp}/zi-load-object-test.XXXXXXXX")" ||
16+
fail "create temporary directory"
17+
trap 'command rm -rf -- "$temp_root"' EXIT INT TERM
18+
19+
command mkdir -p \
20+
"${temp_root}/home" \
21+
"${temp_root}/cache" \
22+
"${temp_root}/config" \
23+
"${temp_root}/data" \
24+
"${temp_root}/zdotdir" || fail "create isolated environment"
25+
26+
# .zi-load-object is a thin dispatcher over .zi-load and .zi-load-snippet. Stub
27+
# both so the status it reports is unambiguously the one it was given, with no
28+
# dependency on a real plug-in, the network, or the filesystem.
29+
env \
30+
HOME="${temp_root}/home" \
31+
XDG_CACHE_HOME="${temp_root}/cache" \
32+
XDG_CONFIG_HOME="${temp_root}/config" \
33+
XDG_DATA_HOME="${temp_root}/data" \
34+
ZDOTDIR="${temp_root}/zdotdir" \
35+
ZI_TEST_CHECKOUT="$project_root" \
36+
zsh -f <<'ZSH' || fail ".zi-load-object does not report the status of the load it performed"
37+
builtin emulate -R zsh
38+
setopt pipe_fail
39+
40+
builtin source "${ZI_TEST_CHECKOUT}/zi.zsh" || return 1
41+
.zi-prepare-home || return 1
42+
43+
integer stub_status=0
44+
.zi-load() { return $stub_status; }
45+
.zi-load-snippet() { return $stub_status; }
46+
47+
check() { # check <label> <type> <stubbed status> <expected status>
48+
local label="$1" type="$2"
49+
stub_status=$3
50+
local -i expected=$4 actual
51+
.zi-load-object "$type" some-id
52+
actual=$?
53+
[[ $actual -eq $expected ]] || {
54+
builtin print -u2 -r -- "${label}: expected ${expected}, got ${actual}"
55+
return 1
56+
}
57+
}
58+
59+
check "plugin success" plugin 0 0 || return 1
60+
check "snippet success" snippet 0 0 || return 1
61+
# The failure paths are the point. Before this was fixed the helper returned the
62+
# undefined `__retval', which zsh evaluates arithmetically as 0, so every load
63+
# reported success and turbo scheduling proceeded after a failed immediate load.
64+
check "plugin failure" plugin 7 7 || return 1
65+
check "snippet failure" snippet 5 5 || return 1
66+
67+
# The caller adds the reported status to its own accumulator exactly once. The
68+
# helper must not also add to a dynamically scoped ___retval, which would
69+
# double-count every failure.
70+
() {
71+
integer ___retval=0
72+
stub_status=3
73+
.zi-load-object plugin some-id
74+
integer reported=$?
75+
[[ $reported -eq 3 ]] || {
76+
builtin print -u2 -r -- "aggregation: expected a reported status of 3, got ${reported}"
77+
return 1
78+
}
79+
(( ___retval == 0 )) || {
80+
builtin print -u2 -r -- "aggregation: the helper modified the caller's \$___retval to ${___retval}"
81+
return 1
82+
}
83+
} || return 1
84+
ZSH
85+
86+
builtin print -r -- "ok - .zi-load-object reports its own load status and leaves aggregation to the caller"

zi.zsh

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1406,13 +1406,18 @@ builtin setopt no_aliases
14061406
local ___type="$1" ___id=$2
14071407
local -a ___opt
14081408
___opt=( ${@[3,-1]} )
1409+
integer ___object_retval=0
14091410
if [[ $___type == snippet ]] {
14101411
.zi-load-snippet $___opt "$___id"
14111412
} elif [[ $___type == plugin ]] {
14121413
.zi-load "$___id" "" $___opt
14131414
}
1414-
___retval+=$?
1415-
return __retval
1415+
___object_retval=$?
1416+
# Report only this load's status. The sole caller owns aggregation: it stores
1417+
# the value in ___last_retval, adds it to ___retval once, and gates turbo
1418+
# scheduling on it. Adding to the caller's ___retval here as well would
1419+
# double-count every failure.
1420+
return ___object_retval
14161421
} # ]]]
14171422
# FUNCTION:.zi-set-m-func() [[[
14181423
# Sets and withdraws the temporary, atclone/atpull time function `m`.

0 commit comments

Comments
 (0)