executor: the ERR trap reports a failing command, not a propagating status (#729) - #739
Merged
Merged
Conversation
…tatus (#729) The ERR pseudo-signal notifies that a command FAILED. lush fired on any non-zero status that reached a statement walker, which is a different thing, and the gap showed in four shapes. A compound command carrying a failure outward is the same event being re-reported, so one failing command notified once per enclosing level: trap 'echo T' ERR; for i in 1; do for j in 1; do false; done; done before: T T T bash, zsh: T The count tracked nesting depth rather than failures. The rule is per failing command, not per compound status -- a loop that SUCCEEDS still reports the failures inside it (`for i in 1 2; do false; true; done` notifies twice in all three shells), which lush already had right. A command whose failure is being TESTED is not an error; the shell is asking whether it succeeds. lush reported the tested position in three of its four forms: a short-circuited `&& `, the operand of `!`, and any construct nested in a condition all notified where no other shell does. Tested-ness is INHERITED, which is why this is a depth rather than a flag: in `true && false || true` the inner `&&` sits in the tested left position of the `||`, so its final operand is not final overall. The same root produced the opposite defect. A `case` arm ran its statements through no firing walker at all, so two failing statements in an arm notified ONCE where bash notifies twice. A function body had the mirror problem: it fired once for the body's aggregate result without asking what produced it, so with errtrace on a failing loop body notified three times. Without errtrace fire_err_trap suppresses in-function notifications, which is why that one looked correct. The fix is one rule applied at every site: report a failure where it originates, never where a status passes through, and never while testing. g_err_tested_depth marks tested evaluation (conditions, the operand of `!`, non-final `&&`/`||` operands) and gates every firing site; err_trap_status_is_propagated names the constructs that report their own inner failures. A subshell, a function call, `select`, `time`, a plain command and a pipeline are deliberately not listed -- their status originates where the walker sees it. bash and zsh agree on every shape, and this is lush's own model of what the trap means, so the fix is mode-invariant and gates nothing. tests/integration/test_err_trap_per_failure.c: 41 checks -- nesting depth, per-failure counting through a succeeding loop, case arms, every tested position including the inherited one, pipelines, function calls with and without errtrace, and errexit ordering. 23 fail against the parent build.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Closes #729.
The rule
The ERR pseudo-signal notifies that a command failed. lush fired on any non-zero status that reached a statement walker -- a different thing -- and the gap showed in four shapes.
1. Propagation was re-reported (the filed defect)
The count tracked nesting depth rather than failures. Note the rule is per failing command, not per compound status:
for i in 1 2; do false; true; donenotifies twice in all three shells even though the loop succeeds. lush already had that right, which is what pins the semantics.2. Tested positions were reported (found while fixing #1)
A command whose failure is being tested is not an error -- the shell is asking whether it succeeds. lush reported three of the four tested forms:
false && true! trueif { false; }; then :; fiif false; then :; fiTested-ness is inherited, which is why the fix is a depth rather than a flag: in
true && false || truethe inner&&sits in the tested left position of the||, so its final operand is not final overall.3 and 4. The same root, in the opposite direction
casearm ran its statements through no firing walker at all:case x in x) false; false;; esacnotified once, where bash notifies twice.set -Ea failing loop body notified three times. Without errtracefire_err_trapsuppresses in-function notifications, which is exactly why that one looked correct.The fix
One rule at every site: report a failure where it originates, never where a status passes through, and never while testing.
g_err_tested_depthmarks tested evaluation (conditions, the operand of!, non-final&&/||operands) and gates every firing site.err_trap_status_is_propagatednames the constructs that report their own inner failures.select,time, a plain command and a pipeline are deliberately not listed -- their status originates where the walker sees it.bash and zsh agree on every shape, and this is lush's own model of what the trap means, so it is mode-invariant and gates nothing.
Verification
set -Eandset -e, pipelines and nesting.tests/integration/test_err_trap_per_failure.c-- 41 checks, and 23 fail against the parent build with a control proving the binaries differ.errexitordering preserved: the notification still precedes the abort, and a tested failure still does not abort (set -e; false || truereaches the next command).