Move job errors into ShellError::Job variant (#15820)

- related #10698

# Description


In my endeavor to make the `ShellError` less crowded I moved the job
related errors into `ShellError::Job` with a `JobError` enum. Mostly I
just moved the codes, errors and labels over to the new enum.

crates/nu-command/src/experimental/job_kill.rs

@@ -34,21 +34,13 @@ impl Command for JobKill {
     ) -> Result<PipelineData, ShellError> {
         let head = call.head;
 
-        let id_arg: Spanned<i64> = call.req(engine_state, stack, 0)?;
+        let id_arg: Spanned<usize> = call.req(engine_state, stack, 0)?;
-
+        let id = JobId::new(id_arg.item);
-        if id_arg.item < 0 {
-            return Err(ShellError::NeedsPositiveValue { span: id_arg.span });
-        }
-
-        let id: JobId = JobId::new(id_arg.item as usize);
 
         let mut jobs = engine_state.jobs.lock().expect("jobs lock is poisoned!");
 
         if jobs.lookup(id).is_none() {
-            return Err(ShellError::JobNotFound {
+            return Err(JobError::NotFound { span: head, id }.into());
-                id: id.get(),
-                span: head,
-            });
         }
 
         jobs.kill_and_remove(id).map_err(|err| {

crates/nu-command/src/experimental/job_recv.rs

@@ -145,7 +145,7 @@ fn recv_instantly(
 ) -> Result<PipelineData, ShellError> {
     match mailbox.try_recv(tag) {
         Ok(value) => Ok(value),
-        Err(TryRecvError::Empty) => Err(ShellError::RecvTimeout { span }),
+        Err(TryRecvError::Empty) => Err(JobError::RecvTimeout { span }.into()),
         Err(TryRecvError::Disconnected) => Err(ShellError::Interrupted { span }),
     }
 }
@@ -175,7 +175,7 @@ fn recv_with_time_limit(
         }
 
         if time_until_deadline.is_zero() {
-            return Err(ShellError::RecvTimeout { span });
+            return Err(JobError::RecvTimeout { span }.into());
         }
     }
 }

crates/nu-command/src/experimental/job_send.rs

@@ -52,14 +52,10 @@ This command never blocks.
     ) -> Result<PipelineData, ShellError> {
         let head = call.head;
 
-        let id_arg: Spanned<i64> = call.req(engine_state, stack, 0)?;
+        let id_arg: Spanned<usize> = call.req(engine_state, stack, 0)?;
         let tag_arg: Option<Spanned<i64>> = call.get_flag(engine_state, stack, "tag")?;
 
-        let id = id_arg.item;
+        let id = JobId::new(id_arg.item);
-
-        if id < 0 {
-            return Err(ShellError::NeedsPositiveValue { span: id_arg.span });
-        }
 
         if let Some(tag) = tag_arg {
             if tag.item < 0 {
@@ -69,7 +65,7 @@ This command never blocks.
 
         let tag = tag_arg.map(|it| it.item as FilterTag);
 
-        if id == 0 {
+        if id == JobId::ZERO {
             engine_state
                 .root_job_sender
                 .send((tag, input))
@@ -77,7 +73,7 @@ This command never blocks.
         } else {
             let jobs = engine_state.jobs.lock().expect("failed to acquire lock");
 
-            if let Some(job) = jobs.lookup(JobId::new(id as usize)) {
+            if let Some(job) = jobs.lookup(id) {
                 match job {
                     nu_protocol::engine::Job::Thread(thread_job) => {
                         // it is ok to send this value while holding the lock, because
@@ -85,17 +81,19 @@ This command never blocks.
                         let _ = thread_job.sender.send((tag, input));
                     }
                     nu_protocol::engine::Job::Frozen(_) => {
-                        return Err(ShellError::JobIsFrozen {
+                        return Err(JobError::AlreadyFrozen {
-                            id: id as usize,
                             span: id_arg.span,
-                        });
+                            id,
+                        }
+                        .into());
                     }
                 }
             } else {
-                return Err(ShellError::JobNotFound {
+                return Err(JobError::NotFound {
-                    id: id as usize,
                     span: id_arg.span,
-                });
+                    id,
+                }
+                .into());
             }
         }
 

crates/nu-command/src/experimental/job_tag.rs

@@ -38,26 +38,15 @@ impl Command for JobTag {
     ) -> Result<PipelineData, ShellError> {
         let head = call.head;
 
-        let id_arg: Spanned<i64> = call.req(engine_state, stack, 0)?;
+        let id_arg: Spanned<usize> = call.req(engine_state, stack, 0)?;
-
+        let id = JobId::new(id_arg.item);
-        if id_arg.item < 0 {
-            return Err(ShellError::NeedsPositiveValue { span: id_arg.span });
-        }
-
-        let id: JobId = JobId::new(id_arg.item as usize);
 
         let tag: Option<String> = call.req(engine_state, stack, 1)?;
 
         let mut jobs = engine_state.jobs.lock().expect("jobs lock is poisoned!");
 
         match jobs.lookup_mut(id) {
-            None => {
+            None => return Err(JobError::NotFound { span: head, id }.into()),
-                return Err(ShellError::JobNotFound {
-                    id: id.get(),
-                    span: head,
-                });
-            }
-
             Some(job) => job.assign_tag(tag),
         }
 

crates/nu-command/src/experimental/job_unfreeze.rs

@@ -39,33 +39,18 @@ impl Command for JobUnfreeze {
     ) -> Result<PipelineData, ShellError> {
         let head = call.head;
 
-        let option_id: Option<Spanned<i64>> = call.opt(engine_state, stack, 0)?;
-
         let mut jobs = engine_state.jobs.lock().expect("jobs lock is poisoned!");
 
-        if let Some(id_arg) = option_id {
+        let id: Option<usize> = call.opt(engine_state, stack, 0)?;
-            if id_arg.item < 0 {
+        let id = id
-                return Err(ShellError::NeedsPositiveValue { span: id_arg.span });
+            .map(JobId::new)
-            }
-        }
-
-        let id = option_id
-            .map(|it| JobId::new(it.item as usize))
             .or_else(|| jobs.most_recent_frozen_job_id())
-            .ok_or_else(|| ShellError::NoFrozenJob { span: head })?;
+            .ok_or(JobError::NoneToUnfreeze { span: head })?;
 
         let job = match jobs.lookup(id) {
-            None => {
+            None => return Err(JobError::NotFound { span: head, id }.into()),
-                return Err(ShellError::JobNotFound {
-                    id: id.get(),
-                    span: head,
-                });
-            }
             Some(Job::Thread(ThreadJob { .. })) => {
-                return Err(ShellError::JobNotFrozen {
+                return Err(JobError::CannotUnfreeze { span: head, id }.into());
-                    id: id.get(),
-                    span: head,
-                });
             }
             Some(Job::Frozen(FrozenJob { .. })) => jobs
                 .remove_job(id)
@@ -107,11 +92,7 @@ fn unfreeze_job(
     span: Span,
 ) -> Result<(), ShellError> {
     match job {
-        Job::Thread(ThreadJob { .. }) => Err(ShellError::JobNotFrozen {
+        Job::Thread(ThreadJob { .. }) => Err(JobError::CannotUnfreeze { span, id: old_id }.into()),
-            id: old_id.get(),
-            span,
-        }),
-
         Job::Frozen(FrozenJob {
             unfreeze: handle,
             tag,

crates/nu-engine/src/command_prelude.rs

@@ -6,5 +6,5 @@ pub use nu_protocol::{
     ast::CellPath,
     engine::{Call, Command, EngineState, Stack, StateWorkingSet},
     record,
-    shell_error::io::*,
+    shell_error::{io::*, job::*},
 };

crates/nu-protocol/src/errors/shell_error/io.rs

@@ -8,7 +8,7 @@ use thiserror::Error;
 
 use crate::Span;
 
-use super::{ShellError, location::Location};
+use super::location::Location;
 
 /// Represents an I/O error in the [`ShellError::Io`] variant.
 ///
@@ -494,12 +494,6 @@ impl Diagnostic for IoError {
     }
 }
 
-impl From<IoError> for ShellError {
-    fn from(value: IoError) -> Self {
-        ShellError::Io(value)
-    }
-}
-
 impl From<IoError> for std::io::Error {
     fn from(value: IoError) -> Self {
         Self::new(value.kind.into(), value)

crates/nu-protocol/src/errors/shell_error/job.rs

@@ -0,0 +1,45 @@
+use miette::Diagnostic;
+use thiserror::Error;
+
+use crate::{JobId, Span};
+
+/// Errors when working working with jobs.
+#[derive(Debug, Clone, Copy, PartialEq, Error, Diagnostic)]
+pub enum JobError {
+    #[error("Job {id} not found")]
+    #[diagnostic(
+        code(nu::shell::job::not_found),
+        help(
+            "The operation could not be completed, there is no job currently running with this id"
+        )
+    )]
+    NotFound { span: Span, id: JobId },
+
+    #[error("No frozen job to unfreeze")]
+    #[diagnostic(
+        code(nu::shell::job::none_to_unfreeze),
+        help("There is currently no frozen job to unfreeze")
+    )]
+    NoneToUnfreeze { span: Span },
+
+    #[error("Job {id} is not frozen")]
+    #[diagnostic(
+        code(nu::shell::job::cannot_unfreeze),
+        help("You tried to unfreeze a job which is not frozen")
+    )]
+    CannotUnfreeze { span: Span, id: JobId },
+
+    #[error("The job {id} is frozen")]
+    #[diagnostic(
+        code(nu::shell::job::already_frozen),
+        help("This operation cannot be performed because the job is frozen")
+    )]
+    AlreadyFrozen { span: Span, id: JobId },
+
+    #[error("No message was received in the requested time interval")]
+    #[diagnostic(
+        code(nu::shell::job::recv_timeout),
+        help("No message arrived within the specified time limit")
+    )]
+    RecvTimeout { span: Span },
+}

crates/nu-protocol/src/errors/shell_error/mod.rs

@@ -3,6 +3,7 @@ use crate::{
     ConfigError, LabeledError, ParseError, Span, Spanned, Type, Value, ast::Operator,
     engine::StateWorkingSet, format_shell_error, record,
 };
+use job::JobError;
 use miette::Diagnostic;
 use serde::{Deserialize, Serialize};
 use std::num::NonZeroI32;
@@ -10,6 +11,7 @@ use thiserror::Error;
 
 pub mod bridge;
 pub mod io;
+pub mod job;
 pub mod location;
 
 /// The fundamental error type for the evaluation engine. These cases represent different kinds of errors
@@ -917,7 +919,7 @@ pub enum ShellError {
     /// This is the main I/O error, for further details check the error kind and additional context.
     #[error(transparent)]
     #[diagnostic(transparent)]
-    Io(io::IoError),
+    Io(#[from] io::IoError),
 
     /// A name was not found. Did you mean a different name?
     ///
@@ -1381,60 +1383,9 @@ On Windows, this would be %USERPROFILE%\AppData\Roaming"#
         span: Option<Span>,
     },
 
-    #[error("Job {id} not found")]
+    #[error(transparent)]
-    #[diagnostic(
+    #[diagnostic(transparent)]
-        code(nu::shell::job_not_found),
+    Job(#[from] JobError),
-        help(
-            "The operation could not be completed, there is no job currently running with this id"
-        )
-    )]
-    JobNotFound {
-        id: usize,
-        #[label = "job not found"]
-        span: Span,
-    },
-
-    #[error("No frozen job to unfreeze")]
-    #[diagnostic(
-        code(nu::shell::no_frozen_job),
-        help("There is currently no frozen job to unfreeze")
-    )]
-    NoFrozenJob {
-        #[label = "no frozen job"]
-        span: Span,
-    },
-
-    #[error("Job {id} is not frozen")]
-    #[diagnostic(
-        code(nu::shell::job_not_frozen),
-        help("You tried to unfreeze a job which is not frozen")
-    )]
-    JobNotFrozen {
-        id: usize,
-        #[label = "job not frozen"]
-        span: Span,
-    },
-
-    #[error("The job {id} is frozen")]
-    #[diagnostic(
-        code(nu::shell::job_is_frozen),
-        help("This operation cannot be performed because the job is frozen")
-    )]
-    JobIsFrozen {
-        id: usize,
-        #[label = "This job is frozen"]
-        span: Span,
-    },
-
-    #[error("No message was received in the requested time interval")]
-    #[diagnostic(
-        code(nu::shell::recv_timeout),
-        help("No message arrived within the specified time limit")
-    )]
-    RecvTimeout {
-        #[label = "timeout"]
-        span: Span,
-    },
 
     #[error(transparent)]
     #[diagnostic(transparent)]

crates/nu-protocol/src/id.rs

@@ -37,6 +37,10 @@ where
     }
 }
 
+impl<M> Id<M, usize> {
+    pub const ZERO: Self = Self::new(0);
+}
+
 impl<M, V> Debug for Id<M, V>
 where
     V: Display,
@@ -115,6 +119,12 @@ pub type JobId = Id<marker::Job>;
 /// Note: `%0` is allocated with the block input at the beginning of a compiled block.
 pub type RegId = Id<marker::Reg, u32>;
 
+impl Display for JobId {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}", self.inner)
+    }
+}
+
 impl Display for RegId {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         write!(f, "%{}", self.get())