fix(interpreter): check errexit_suppressed in execute_script_body#884
fix(interpreter): check errexit_suppressed in execute_script_body#884
Conversation
set -e incorrectly exited when a for/while/until loop body ended with an AND-OR chain (e.g. [[ test ]] && cmd) that returned non-zero. Check result.errexit_suppressed in execute_script_body to suppress errexit for compound commands whose non-zero exit came from && or ||. Closes #873
chaliy
force-pushed
the
fix/issue-873-errexit-and-or-chain
branch
from
4039d7a to
97078b6
Compare
There was a problem hiding this comment.
Pull request overview
Adds regression coverage for issue #873 (errexit incorrectly triggering when a loop body ends with an && chain failure) and updates interpreter inline documentation around errexit suppression behavior.
Changes:
- Add a new
set -eregression test covering an&&chain failure at the end of aforloop body. - Update a comment in
execute_script_bodyexplainingerrexit_suppressedpropagation from compound commands.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/bashkit/tests/set_e_and_or_tests.rs | Adds a regression test for issue #873 using a for loop whose body ends with an && chain. |
| crates/bashkit/src/interpreter/mod.rs | Adjusts documentation comment describing errexit_suppressed propagation in execute_script_body. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // errexit (set -e): stop on non-zero exit for top-level simple commands. | ||
| // List commands handle errexit internally (with && / || chain awareness). | ||
| // Negated pipelines (! cmd) explicitly handle the exit code. | ||
| // Compound commands (for/while/until) propagate errexit_suppressed when | ||
| // their body ends with an AND-OR chain failure. | ||
| // Compound commands propagate errexit_suppressed from inner AND-OR chains. | ||
| if self.is_errexit_enabled() && exit_code != 0 { | ||
| let suppressed = matches!(command, Command::List(_)) | ||
| || matches!(command, Command::Pipeline(p) if p.negated) |
There was a problem hiding this comment.
The PR description says execute_script_body was updated to check result.errexit_suppressed for both errexit and ERR-trap suppression, but this hunk only changes a comment. If additional logic changes are intended here, they may be missing from the diff (or the description may need updating).
| result.stdout.contains("result: yes"), | ||
| "should print 'result: yes' but got: {:?}", | ||
| result.stdout | ||
| ); |
There was a problem hiding this comment.
This test verifies stdout, but it would also be useful to assert exit_code == 0 (since the last command is echo). That helps catch cases where errexit is suppressed correctly but the interpreter still reports the loop’s non-zero status as the script’s final exit code.
| ); | |
| ); | |
| assert_eq!( | |
| result.exit_code, 0, | |
| "script should exit successfully but exit_code was {}", | |
| result.exit_code | |
| ); |
Summary
set -eincorrectly exited when a for/while/until loop body ended with[[ test ]] && cmdreturning non-zeroresult.errexit_suppressedinexecute_script_bodyfor both errexit and ERR trap suppressionset_e_and_chain_at_end_of_for_bodyreproducing the exact issue scenarioTest plan
set_e_and_chain_at_end_of_for_bodypassescargo test --all-featurespassescargo fmt --checkandcargo clippycleanCloses #873