From a36cb562e11737d7c4da1bf84c7a87c9a8214509 Mon Sep 17 00:00:00 2001 From: reuben olinsky Date: Tue, 21 Jan 2025 09:33:18 -0800 Subject: [PATCH] fix(exit): correct exit semantics in various compund statements (#347) * Adjust several cases of compound commands where the exit shell request wasn't propagated. * Add a number of compat test cases to confirm broader behavior. --- brush-core/src/commands.rs | 10 ++- brush-core/src/interp.rs | 22 ++++-- brush-shell/tests/cases/builtins/exit.yaml | 68 +++++++++++++++++++ brush-shell/tests/cases/builtins/jobs.yaml | 1 + .../tests/cases/compound_cmds/subshell.yaml | 7 ++ 5 files changed, 100 insertions(+), 8 deletions(-) diff --git a/brush-core/src/commands.rs b/brush-core/src/commands.rs index e1aef8f1..18c13494 100644 --- a/brush-core/src/commands.rs +++ b/brush-core/src/commands.rs @@ -547,7 +547,15 @@ pub(crate) async fn invoke_shell_function( // Restore positional parameters. context.shell.positional_parameters = prior_positional_params; - Ok(CommandSpawnResult::ImmediateExit(result?.exit_code)) + // Get the actual execution result from the body of the function. + let result = result?; + + // Report back the exit code, and honor any requests to exit the whole shell. + Ok(if result.exit_shell { + CommandSpawnResult::ExitShell(result.exit_code) + } else { + CommandSpawnResult::ImmediateExit(result.exit_code) + }) } pub(crate) async fn invoke_command_in_subshell_and_get_output( diff --git a/brush-core/src/interp.rs b/brush-core/src/interp.rs index 7be2b11d..8e8df574 100644 --- a/brush-core/src/interp.rs +++ b/brush-core/src/interp.rs @@ -193,7 +193,7 @@ impl Execute for ast::CompoundList { } // Check for early return. - if result.return_from_function_or_script { + if result.exit_shell || result.return_from_function_or_script { break; } @@ -525,7 +525,11 @@ impl Execute for ast::CompoundCommand { ast::CompoundCommand::Subshell(ast::SubshellCommand(s)) => { // Clone off a new subshell, and run the body of the subshell there. let mut subshell = shell.clone(); - s.execute(&mut subshell, params).await + let subshell_result = s.execute(&mut subshell, params).await?; + + // Preserve the subshell's exit code, but don't honor any of its requests to exit + // the shell, break out of loops, etc. + Ok(ExecutionResult::new(subshell_result.exit_code)) } ast::CompoundCommand::ForClause(f) => f.execute(shell, params).await, ast::CompoundCommand::CaseClause(c) => c.execute(shell, params).await, @@ -578,7 +582,7 @@ impl Execute for ast::ForClauseCommand { )?; result = self.body.0.execute(shell, params).await?; - if result.return_from_function_or_script { + if result.exit_shell || result.return_from_function_or_script { break; } @@ -721,16 +725,20 @@ impl Execute for (WhileOrUntil, &ast::WhileOrUntilClauseCommand) { loop { let condition_result = test_condition.execute(shell, params).await?; - if condition_result.is_success() != is_while { + if condition_result.exit_shell || condition_result.return_from_function_or_script { + result.exit_code = condition_result.exit_code; + result.exit_shell = condition_result.exit_shell; + result.return_from_function_or_script = + condition_result.return_from_function_or_script; break; } - if condition_result.return_from_function_or_script { + if condition_result.is_success() != is_while { break; } result = body.0.execute(shell, params).await?; - if result.return_from_function_or_script { + if result.exit_shell || result.return_from_function_or_script { break; } @@ -796,7 +804,7 @@ impl Execute for ast::ArithmeticForClauseCommand { } result = self.body.0.execute(shell, params).await?; - if result.return_from_function_or_script { + if result.exit_shell || result.return_from_function_or_script { break; } diff --git a/brush-shell/tests/cases/builtins/exit.yaml b/brush-shell/tests/cases/builtins/exit.yaml index 53e5e0f6..b2c4c303 100644 --- a/brush-shell/tests/cases/builtins/exit.yaml +++ b/brush-shell/tests/cases/builtins/exit.yaml @@ -7,3 +7,71 @@ cases: - name: "Exit with code" stdin: | exit 10 + + - name: "Exit in for loop" + stdin: | + for i in 1 2 3; do + exit 42 + echo "Got past exit in loop" + done + echo "Got past loop" + + - name: "Exit in arithmetic for loop body" + stdin: | + for ((i = 0; i < 10; i++)); do + exit 42 + echo "Got past exit in loop" + done + echo "Got past loop" + + - name: "Exit in while loop condition" + stdin: | + while exit 42; do + echo "In loop" + done + echo "Got past loop" + + - name: "Exit in while loop body" + stdin: | + while true; do + exit 42 + echo "Got past exit in body" + break + done + echo "Got past loop" + + - name: "Exit in sequence" + stdin: | + exit 42; echo "Should not be printed" + + - name: "Exit in function" + stdin: | + myfunc() { + exit 42 + echo "Got past exit in function" + } + + myfunc + echo "Got past function call" + + - name: "Exit in subshell" + stdin: | + (exit 42) + echo "Got past subshell" + + - name: "Exit in command substitution" + stdin: | + output=$(echo hi; exit 42; echo there) + echo "Got past command substitution" + + - name: "Exit in and/or" + stdin: | + exit 42 || echo "Got past exit" + + - name: "Exit in brace group" + stdin: | + { + exit 42 + echo "Got past exit" + } + echo "Got past brace group" diff --git a/brush-shell/tests/cases/builtins/jobs.yaml b/brush-shell/tests/cases/builtins/jobs.yaml index b13ceb8c..683ded6b 100644 --- a/brush-shell/tests/cases/builtins/jobs.yaml +++ b/brush-shell/tests/cases/builtins/jobs.yaml @@ -2,6 +2,7 @@ name: "Builtins: job-related builtins" cases: - name: "Basic async job" stdin: | + set +m echo hi & wait jobs diff --git a/brush-shell/tests/cases/compound_cmds/subshell.yaml b/brush-shell/tests/cases/compound_cmds/subshell.yaml index f202e1eb..1ac49c98 100644 --- a/brush-shell/tests/cases/compound_cmds/subshell.yaml +++ b/brush-shell/tests/cases/compound_cmds/subshell.yaml @@ -23,3 +23,10 @@ cases: - name: "Piped subshell usage" stdin: | (echo hi) | wc -l + + - name: "Breaks in subshell" + stdin: | + for i in 1 2 3; do + echo $i + (for i in 1 2 3; do break 2; done) + done