diff --git a/src/exec.rs b/src/exec.rs index 81bc490c2..5c173fcc1 100644 --- a/src/exec.rs +++ b/src/exec.rs @@ -864,9 +864,7 @@ fn exec_external_command( Err(err) => { safe_report_exec_error(err.0, &actual_cmd, &argv, &envv); p.status - .update(&ProcStatus::from_exit_code(exit_code_from_exec_error( - err.0, - ))); + .update(ProcStatus::from_exit_code(exit_code_from_exec_error(err.0))); return Err(()); } }; @@ -965,6 +963,23 @@ fn function_restore_environment(parser: &Parser, block: BlockId) { Option<&mut OutputStream>, ) -> ProcStatus; +// Return a function which may be to run the given block node process 'p'. +fn get_performer_for_block_node(p: &Process, job: &Job, io_chain: &IoChain) -> Box { + let ProcessType::BlockNode(node) = &p.typ else { + panic!("Expected a block node process"); + }; + + // We want to capture the job group. + let job_group = job.group.clone(); + let io_chain = io_chain.clone(); + let node = node.clone(); + Box::new(move |parser: &Parser, _p: &Process, _out, _err| { + parser + .eval_node(&node, &io_chain, job_group.as_ref(), BlockType::top) + .status + }) +} + // Return a function which may be to run the given process 'p'. // May return None in the rare case that the to-be called fish function no longer // exists. This is just an artifact of the fact that we only capture the functions name, not its @@ -974,53 +989,37 @@ fn function_restore_environment(parser: &Parser, block: BlockId) { // function foo; echo hi; end // foo (functions --erase foo) // -fn get_performer_for_process( +fn get_performer_for_function( p: &Process, job: &Job, io_chain: &IoChain, -) -> Option> { - assert!( - p.is_block_node() || p.is_function(), - "Unexpected process type" - ); +) -> Result, ()> { + assert!(p.is_function()); // We want to capture the job group. let job_group = job.group.clone(); let io_chain = io_chain.clone(); + // This may occur if the function was erased as part of its arguments or in other strange edge cases. + let Some(props) = function::get_props(p.argv0().unwrap()) else { + FLOG!( + error, + wgettext_fmt!("Unknown function '%ls'", p.argv0().unwrap()) + ); + return Err(()); + }; + Ok(Box::new(move |parser: &Parser, p: &Process, _out, _err| { + let argv = p.argv(); + // Pull out the job list from the function. + let fb = function_prepare_environment(parser, argv.clone(), &props); + let body_node = props.func_node.child_ref(|n| &n.jobs); + let mut res = parser.eval_node(&body_node, &io_chain, job_group.as_ref(), BlockType::top); + function_restore_environment(parser, fb); - if let ProcessType::BlockNode(node) = &p.typ { - let node = node.clone(); - Some(Box::new( - move |parser: &Parser, _p: &Process, _out, _err| { - parser - .eval_node(&node, &io_chain, job_group.as_ref(), BlockType::top) - .status - }, - )) - } else { - assert!(p.is_function()); - let Some(props) = function::get_props(p.argv0().unwrap()) else { - FLOG!( - error, - wgettext_fmt!("Unknown function '%ls'", p.argv0().unwrap()) - ); - return None; - }; - Some(Box::new(move |parser: &Parser, p: &Process, _out, _err| { - let argv = p.argv(); - // Pull out the job list from the function. - let fb = function_prepare_environment(parser, argv.clone(), &props); - let body_node = props.func_node.child_ref(|n| &n.jobs); - let mut res = - parser.eval_node(&body_node, &io_chain, job_group.as_ref(), BlockType::top); - function_restore_environment(parser, fb); - - // If the function did not execute anything, treat it as success. - if res.was_empty { - res = EvalRes::new(ProcStatus::from_exit_code(EXIT_SUCCESS)); - } - res.status - })) - } + // If the function did not execute anything, treat it as success. + if res.was_empty { + res = EvalRes::new(ProcStatus::from_exit_code(EXIT_SUCCESS)); + } + res.status + })) } /// Execute a block node or function "process". @@ -1032,6 +1031,7 @@ fn exec_block_or_func_process( mut io_chain: IoChain, piped_output_needs_buffering: bool, ) -> LaunchResult { + assert!(p.is_block_node() || p.is_function()); // Create an output buffer if we're piping to another process. let mut block_output_bufferfill = None; if piped_output_needs_buffering { @@ -1046,14 +1046,13 @@ fn exec_block_or_func_process( } } - // Get the process performer, and just execute it directly. - // Do it in this scoped way so that the performer function can be eagerly deallocating releasing - // its captured io chain. - if let Some(performer) = get_performer_for_process(p, j, &io_chain) { - p.status.update(&performer(parser, p, None, None)); + let performer = if p.is_block_node() { + get_performer_for_block_node(p, j, &io_chain) } else { - return Err(()); - } + // Note this may fail if the function was erased. + get_performer_for_function(p, j, &io_chain)? + }; + p.status.update(performer(parser, p, None, None)); // If we have a block output buffer, populate it now. let mut buffer_contents = vec![]; @@ -1167,7 +1166,7 @@ fn exec_builtin_process( let performer = get_performer_for_builtin(p, j, io_chain); let status = performer(parser, p, Some(&mut out), Some(&mut err)); - p.status.update(&status); + p.status.update(status); handle_builtin_output(parser, j, p, io_chain, &out, &err); Ok(()) } diff --git a/src/proc.rs b/src/proc.rs index d4aa5e46c..cddad08fe 100644 --- a/src/proc.rs +++ b/src/proc.rs @@ -33,6 +33,7 @@ use once_cell::sync::Lazy; #[cfg(not(target_has_atomic = "64"))] use portable_atomic::AtomicU64; +use std::borrow::Borrow; use std::cell::{Cell, Ref, RefCell, RefMut}; use std::fs; use std::io::{Read, Write}; @@ -104,7 +105,7 @@ pub fn clock_ticks_to_seconds(ticks: ClockTicks) -> f64 { pub type JobGroupRef = Arc; -/// A proc_status_t is a value type that encapsulates logic around exited vs stopped vs signaled, +/// A ProcStatus is a value type that encapsulates logic around exited vs stopped vs signaled, /// etc. /// /// It contains two fields packed into an AtomicU64 to allow interior mutability, `status: i32` and @@ -143,9 +144,12 @@ pub fn is_empty(&self) -> bool { } /// Replace the current `ProcStatus` with that of `other`. - pub fn update(&self, other: &ProcStatus) { - self.value - .store(other.value.load(Ordering::Relaxed), Ordering::Relaxed); + /// The 'Borrow' means that `other` can be a `ProcStatus` or a reference to one. + pub fn update(&self, other: impl Borrow) { + self.value.store( + other.borrow().value.load(Ordering::Relaxed), + Ordering::Relaxed, + ); } fn set_status(&self, status: i32) {