Make Event::caller_exit take a JobId, not an i32

A JobId is not supposed to convert to other types.

Since this type is defined as NonZeroU32 (which cannot be -1), we need to
add some conversion functions to match the C++ behavior.

Overall, it would have been better to keep using the C++ type.

fish-rust/src/event.rs

@@ -7,6 +7,7 @@
 use autocxx::WithinUniquePtr;
 use cxx::{CxxVector, CxxWString, UniquePtr};
 use libc::pid_t;
+use std::num::NonZeroU32;
 use std::pin::Pin;
 use std::sync::atomic::{AtomicBool, AtomicU32, Ordering};
 use std::sync::{Arc, Mutex};
@@ -16,9 +17,11 @@ use crate::builtins::shared::io_streams_t;
 use crate::common::{escape_string, scoped_push, EscapeFlags, EscapeStringStyle, ScopeGuard};
 use crate::ffi::{self, block_t, parser_t, signal_check_cancel, signal_handle, Repin};
 use crate::flog::FLOG;
+use crate::job_group::{JobId, MaybeJobId};
 use crate::signal::Signal;
 use crate::termsize;
 use crate::wchar::{wstr, WString, L};
+use crate::wchar_ext::ToWString;
 use crate::wchar_ffi::{wcharz_t, AsWstr, WCharFromFFI, WCharToFFI};
 use crate::wutil::sprintf;
 
@@ -409,7 +412,7 @@ impl Event {
         }
     }
 
-    pub fn caller_exit(internal_job_id: u64, job_id: i32) -> Self {
+    pub fn caller_exit(internal_job_id: u64, job_id: MaybeJobId) -> Self {
         Self {
             desc: EventDescription {
                 typ: EventType::CallerExit {
@@ -418,7 +421,7 @@ impl Event {
             },
             arguments: vec![
                 "JOB_EXIT".into(),
-                job_id.to_string().into(),
+                job_id.to_wstring(),
                 "0".into(), // historical
             ],
         }
@@ -459,7 +462,16 @@ fn new_event_job_exit(pgid: i32, jid: u64) -> Box<Event> {
 }
 
 fn new_event_caller_exit(internal_job_id: u64, job_id: i32) -> Box<Event> {
-    Box::new(Event::caller_exit(internal_job_id, job_id))
+    Box::new(Event::caller_exit(
+        internal_job_id,
+        MaybeJobId(if job_id == -1 {
+            None
+        } else {
+            Some(JobId::new(
+                NonZeroU32::new(u32::try_from(job_id).unwrap()).unwrap(),
+            ))
+        }),
+    ))
 }
 
 impl Event {

fish-rust/src/job_group.rs

@@ -2,6 +2,7 @@ use self::ffi::pgid_t;
 use crate::common::{assert_send, assert_sync};
 use crate::signal::Signal;
 use crate::wchar::WString;
+use crate::wchar_ext::ToWString;
 use crate::wchar_ffi::{WCharFromFFI, WCharToFFI};
 use cxx::{CxxWString, UniquePtr};
 use std::num::NonZeroU32;
@@ -68,6 +69,44 @@ fn create_job_group_with_job_control_ffi(command: &CxxWString, wants_term: bool)
 #[repr(transparent)]
 pub struct JobId(NonZeroU32);
 
+#[derive(Clone, Copy, Debug)]
+pub struct MaybeJobId(pub Option<JobId>);
+
+impl std::ops::Deref for MaybeJobId {
+    type Target = Option<JobId>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl std::fmt::Display for MaybeJobId {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        self.0
+            .map(|j| i64::from(u32::from(j.0)))
+            .unwrap_or(-1)
+            .fmt(f)
+    }
+}
+
+impl ToWString for MaybeJobId {
+    fn to_wstring(&self) -> WString {
+        self.0
+            .map(|j| i64::from(u32::from(j.0)))
+            .unwrap_or(-1)
+            .to_wstring()
+    }
+}
+
+impl<'a> printf_compat::args::ToArg<'a> for MaybeJobId {
+    fn to_arg(self) -> printf_compat::args::Arg<'a> {
+        self.0
+            .map(|j| i64::from(u32::from(j.0)))
+            .unwrap_or(-1)
+            .to_arg()
+    }
+}
+
 /// `JobGroup` is conceptually similar to the idea of a process group. It represents data which
 /// is shared among all of the "subjobs" that may be spawned by a single job.
 /// For example, two fish functions in a pipeline may themselves spawn multiple jobs, but all will
@@ -99,7 +138,7 @@ pub struct JobGroup {
     pub command: WString,
     /// Our job id, if any. `None` here should evaluate to `-1` for ffi purposes.
     /// "Simple block" groups like function calls do not have a job id.
-    pub job_id: Option<JobId>,
+    pub job_id: MaybeJobId,
     /// The signal causing the group to cancel or `0` if none.
     /// Not using an `Option<Signal>` to be able to atomically load/store to this field.
     signal: AtomicI32,
@@ -255,11 +294,15 @@ impl JobGroup {
 static CONSUMED_JOB_IDS: Mutex<Vec<JobId>> = Mutex::new(Vec::new());
 
 impl JobId {
-    const NONE: Option<JobId> = None;
+    const NONE: MaybeJobId = MaybeJobId(None);
+
+    pub fn new(value: NonZeroU32) -> Self {
+        JobId(value)
+    }
 
     /// Return a `JobId` that is greater than all extant job ids stored in [`CONSUMED_JOB_IDS`].
     /// The `JobId` should be freed with [`JobId::release()`] when it is no longer in use.
-    fn acquire() -> Option<Self> {
+    fn acquire() -> MaybeJobId {
         let mut consumed_job_ids = CONSUMED_JOB_IDS.lock().expect("Poisoned mutex!");
 
         // The new job id should be greater than the largest currently used id (#6053). The job ids
@@ -269,7 +312,7 @@ impl JobId {
             .map(JobId::next)
             .unwrap_or(JobId(1.try_into().unwrap()));
         consumed_job_ids.push(job_id);
-        return Some(job_id);
+        return MaybeJobId(Some(job_id));
     }
 
     /// Remove the provided `JobId` from [`CONSUMED_JOB_IDS`].
@@ -289,7 +332,7 @@ impl JobId {
 }
 
 impl JobGroup {
-    pub fn new(command: WString, id: Option<JobId>, job_control: bool, wants_term: bool) -> Self {
+    pub fn new(command: WString, id: MaybeJobId, job_control: bool, wants_term: bool) -> Self {
         // We *can* have a job id without job control, but not the reverse.
         if job_control {
             assert!(id.is_some(), "Cannot have job control without a job id!");
@@ -341,7 +384,7 @@ impl JobGroup {
 
 impl Drop for JobGroup {
     fn drop(&mut self) {
-        if let Some(job_id) = self.job_id {
+        if let Some(job_id) = *self.job_id {
             JobId::release(job_id);
         }
     }