Skip to content

refactor(interpreter): extract handle_loop_control_flow helper#891

Closed
chaliy wants to merge 0 commit intomainfrom
refactor/issue-881-errexit-suppression-helper
Closed

refactor(interpreter): extract handle_loop_control_flow helper#891
chaliy wants to merge 0 commit intomainfrom
refactor/issue-881-errexit-suppression-helper

Conversation

@chaliy
Copy link
Copy Markdown
Contributor

@chaliy chaliy commented Mar 28, 2026

Summary

  • Extract handle_loop_control_flow() helper with LoopAction enum to centralize break/continue/return propagation and errexit_suppressed tracking
  • Replaces duplicated pattern across ForCommand, ArithmeticForCommand, and WhileCommand
  • Net -33 lines (92 added, 125 removed)

Test plan

  • All 1810 bash spec tests pass (100%)
  • All 9 set -e tests pass
  • Full cargo test --all-features passes
  • cargo fmt --check and cargo clippy clean

Closes #881

Copy link
Copy Markdown

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.

Pull request overview

Refactors the bash interpreter’s loop execution logic by extracting shared break/continue/return propagation and errexit_suppressed tracking into a centralized helper, reducing duplication across loop command implementations.

Changes:

  • Introduces LoopAction and handle_loop_control_flow() to centralize loop control-flow handling.
  • Updates execute_for, execute_arithmetic_for, and execute_condition_loop to use the new helper.
  • Preserves loop-level errexit_suppressed propagation via last_errexit_suppressed.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +2723 to +2729
LoopAction::Return(ExecResult {
stdout: stdout.to_string(),
stderr: stderr.to_string(),
exit_code,
control_flow: ControlFlow::Break(n - 1),
..Default::default()
})
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

handle_loop_control_flow builds an ExecResult for propagated break/continue/return by cloning the aggregated stdout/stderr via to_string(). Previously these early-return paths moved the existing Strings, so this introduces an O(n) copy and extra memory proportional to accumulated output. Consider changing the helper to return only the decremented ControlFlow (or an action + control-flow value) so callers can return ExecResult { stdout, stderr, ... } by moving the existing buffers, or accept &mut String and use std::mem::take on return paths.

Copilot uses AI. Check for mistakes.
/// Handle control flow from a loop body iteration result.
///
/// Tracks `errexit_suppressed` from the result. Returns:
/// - `LoopAction::Continue` — proceed to next iteration
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

The docstring says LoopAction::Continue means “proceed to next iteration”, but at least in execute_arithmetic_for it intentionally falls through to run the step expression before the next iteration. Consider rewording this to something like “continue loop normally” to avoid implying it skips post-body logic.

Suggested change
/// - `LoopAction::Continue` — proceed to next iteration
/// - `LoopAction::Continue` — continue the loop normally (no break/return)

Copilot uses AI. Check for mistakes.
@chaliy chaliy closed this Mar 30, 2026
@chaliy chaliy force-pushed the refactor/issue-881-errexit-suppression-helper branch from 43f8372 to 3b61c38 Compare March 30, 2026 16:49
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.

Extract errexit suppression propagation helper for compound commands

2 participants