Skip to content

Commit

Permalink
[osh/cmd_exec] Fix bug with break/continue and subshell.
Browse files Browse the repository at this point in the history
OSH used to print the wrong result, but it's now more strict than any
shell.

The subshell exits 1 when the control flow isn't wrapped in a loop.
Then 'set -o errexit' will cause the program to exit early, which you
want for this programming error.

Fixes issue #302.
  • Loading branch information
Andy Chu committed May 16, 2019
1 parent cbb57d2 commit 2365ab0
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 8 deletions.
3 changes: 3 additions & 0 deletions core/process.py
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,10 @@ def Run(self):
# NOTE: may NOT return due to exec().
if self.disable_errexit:
self.ex.exec_opts.errexit.Disable()

self.ex.ExecuteAndCatch(self.node, fork_external=False)
# NOTE: We ignore the is_fatal return value. The user should set -o
# errexit so failures in subprocesses cause failures in the parent.

# Raises SystemExit, so we still have time to write a crash dump.
sys.exit(self.ex.LastStatus())
Expand Down
9 changes: 8 additions & 1 deletion osh/cmd_exec.py
Original file line number Diff line number Diff line change
Expand Up @@ -1192,7 +1192,14 @@ def ExecuteAndCatch(self, node, fork_external=True):
is_control_flow = True
status = e.StatusCode()
else:
raise # Invalid
# Invalid control flow
self.errfmt.Print(
"Loop and control flow can't be in a different processes",
span_id=e.token.span_id)
is_fatal = True
# All shells exit 0 here. It could be hidden behind
# strict-control-flow if the incompatibility causes problems.
status = 1
except util.ParseError as e:
self.dumper.MaybeCollect(self, e) # Do this before unwinding stack
raise
Expand Down
53 changes: 48 additions & 5 deletions spec/loop.test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -178,30 +178,73 @@ one
## END

#### continue in subshell
for i in $(seq 3); do
for i in $(seq 2); do
echo "> $i"
( if true; then continue; fi; echo "Should not print" )
echo subshell status=$?
echo ". $i"
done
## STDOUT:
# osh lets you fail
> 1
subshell status=1
. 1
> 2
subshell status=1
. 2
## END
## OK dash/bash/zsh STDOUT:
> 1
subshell status=0
. 1
> 2
subshell status=0
. 2
> 3
. 3
## END
## BUG mksh STDOUT:
> 1
Should not print
subshell status=0
. 1
> 2
Should not print
subshell status=0
. 2
## END

#### continue in subshell aborts with errexit
# The other shells don't let you recover from this programming error!
set -o errexit
for i in $(seq 2); do
echo "> $i"
( if true; then continue; fi; echo "Should not print" )
echo 'should fail after subshell'
echo ". $i"
done
## STDOUT:
> 1
## END
## status: 1
## BUG dash/bash/zsh STDOUT:
> 1
should fail after subshell
. 1
> 2
should fail after subshell
. 2
> 3
## END
## BUG dash/bash/zsh status: 0
## BUG mksh STDOUT:
> 1
Should not print
should fail after subshell
. 1
> 2
Should not print
. 3
should fail after subshell
. 2
## END
## BUG mksh status: 0

#### bad arg to break
x=oops
Expand Down
10 changes: 9 additions & 1 deletion test/runtime-errors.sh
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,14 @@ strict_control_flow_warnings() {
break
}

control_flow_subshell() {
set -o errexit
for i in $(seq 2); do
echo $i
( break; echo 'oops')
done
}

#
# TEST DRIVER
#
Expand Down Expand Up @@ -622,7 +630,7 @@ all() {
builtin_popd builtin_unset builtin_alias_unalias builtin_help \
builtin_trap builtin_getopts builtin_wait \
strict_word_eval_warnings strict_arith_warnings \
strict_control_flow_warnings; do
strict_control_flow_warnings control_flow_subshell; do
_run_test $t
done
}
Expand Down
2 changes: 1 addition & 1 deletion test/spec.sh
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ quote() {
}

loop() {
sh-spec spec/loop.test.sh --osh-failures-allowed 1 \
sh-spec spec/loop.test.sh \
${REF_SHELLS[@]} $ZSH $OSH_LIST "$@"
}

Expand Down

0 comments on commit 2365ab0

Please sign in to comment.