Skip to content

fix(exit): correct exit semantics in various compund statements#347

Merged
reubeno merged 1 commit into
mainfrom
exit-semantics
Jan 21, 2025
Merged

fix(exit): correct exit semantics in various compund statements#347
reubeno merged 1 commit into
mainfrom
exit-semantics

Conversation

@reubeno
Copy link
Copy Markdown
Owner

@reubeno reubeno commented Jan 21, 2025

There were a handful of cases where exit was not being honored; this attempts to resolve the lot of them, as well as add a handful of test cases to confirm the behavior matches that of bash.

Notably, we need to ignore exit-shell requests from subshells, but need to honor them when coming from within the same shell.

(Note: the way in which we have separate representations for command spawn results, execution results, and builtin execution results... leaves room for improvement. The present goal is to reach correct behavior and then we should, in time, strive to simplify the logic.)
 
Resolves #337

@reubeno reubeno requested a review from Copilot January 21, 2025 17:11
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jan 21, 2025

Performance Benchmark Report

Benchmark name Baseline (μs) Test/PR (μs) Delta (μs) Delta %
clone_shell_object 16.29 μs 16.21 μs -0.08 μs 🟢 -0.51%
eval_arithmetic 0.18 μs 0.19 μs 0.01 μs ⚪ Unchanged
expand_one_string 1.70 μs 1.67 μs -0.03 μs ⚪ Unchanged
for_loop 32.38 μs 32.82 μs 0.44 μs ⚪ Unchanged
function_call 3.46 μs 3.40 μs -0.06 μs ⚪ Unchanged
instantiate_shell 48.34 μs 48.89 μs 0.55 μs ⚪ Unchanged
instantiate_shell_with_init_scripts 22130.35 μs 23366.18 μs 1235.83 μs 🟠 +5.58%
parse_bash_completion 1659.83 μs 1681.74 μs 21.90 μs ⚪ Unchanged
parse_sample_script 1.77 μs 1.77 μs 0.00 μs ⚪ Unchanged
run_echo_builtin_command 16.47 μs 16.29 μs -0.18 μs ⚪ Unchanged
run_one_external_command 2292.20 μs 7456.53 μs 5164.34 μs 🟠 +225.30%
tokenize_sample_script 2.86 μs 2.85 μs -0.00 μs ⚪ Unchanged

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
brush-core/src/builtins/set.rs 🟠 61.93% 🟠 63.07% 🟢 1.14%
brush-core/src/commands.rs 🟢 86.83% 🟢 86.9% 🟢 0.07%
brush-core/src/interp.rs 🟢 91.02% 🟢 91.25% 🟢 0.23%
brush-core/src/jobs.rs 🔴 43.1% 🔴 37.07% 🔴 -6.03%
brush-core/src/shell.rs 🟢 82.49% 🟢 82.2% 🔴 -0.29%
brush-interactive/src/interactive_shell.rs 🟠 73.42% 🟠 74.68% 🟢 1.26%
Overall Coverage 🟢 77.03% 🟢 76.97% 🔴 -0.06%

Minimum allowed coverage is 70%, this run produced 76.97%

@github-actions
Copy link
Copy Markdown

Test Results

    3 files     14 suites   5m 1s ⏱️
  612 tests   612 ✅ 0 💤 0 ❌
1 822 runs  1 822 ✅ 0 💤 0 ❌

Results for commit 62f8295.

@reubeno reubeno merged commit a36cb56 into main Jan 21, 2025
@reubeno reubeno deleted the exit-semantics branch January 21, 2025 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

exit does not exit

2 participants