From 2a86257816be939876228203269cdc132fab82d0 Mon Sep 17 00:00:00 2001
From: Xavier L'Heureux <xavier.lheureux@icloud.com>
Date: Mon, 10 Jun 2019 12:43:22 -0400
Subject: [PATCH] Add function errors and follow the error styleguide
---
examples/fn.out | 4 +--
src/lib/shell/flow_control.rs | 25 +++++-----------
src/lib/shell/mod.rs | 44 +++++++++++++--------------
src/lib/shell/pipe_exec/fork.rs | 13 ++++----
src/lib/shell/pipe_exec/mod.rs | 48 ++++++++++--------------------
src/lib/shell/pipe_exec/pipes.rs | 2 +-
src/lib/shell/pipe_exec/streams.rs | 4 +--
7 files changed, 55 insertions(+), 85 deletions(-)
diff --git a/examples/fn.out b/examples/fn.out
index d593d50c..7a2e55aa 100644
--- a/examples/fn.out
+++ b/examples/fn.out
@@ -15,5 +15,5 @@ goodbye
9
16
25
-ion: function argument has invalid type: expected int, found value '$num'
-ion: function argument has invalid type: expected int, found value '$num'
+ion: could not run function: argument has invalid type: expected int, found value '$num'
+ion: could not run function: argument has invalid type: expected int, found value '$num'
diff --git a/src/lib/shell/flow_control.rs b/src/lib/shell/flow_control.rs
index 69967e24..cb2f2502 100644
--- a/src/lib/shell/flow_control.rs
+++ b/src/lib/shell/flow_control.rs
@@ -6,7 +6,6 @@ use crate::{
};
use small;
use smallvec::SmallVec;
-use std::fmt::{self, Display, Formatter};
#[derive(Debug, PartialEq, Clone)]
pub struct ElseIf<'a> {
@@ -344,20 +343,12 @@ pub struct Function<'a> {
statements: Block<'a>,
}
-#[derive(Debug, PartialEq, Clone)]
+#[derive(Debug, PartialEq, Clone, Error)]
pub enum FunctionError {
+ #[error(display = "invalid number of arguments supplied")]
InvalidArgumentCount,
- InvalidArgumentType(Primitive, String),
-}
-
-impl Display for FunctionError {
- fn fmt(&self, fmt: &mut Formatter) -> fmt::Result {
- use self::FunctionError::*;
- match *self {
- InvalidArgumentCount => write!(fmt, "invalid number of arguments"),
- InvalidArgumentType(ref t, ref value) => write!(fmt, "{} is not of type {}", value, t),
- }
- }
+ #[error(display = "argument has invalid type: expected {}, found value '{}'", t, val)]
+ InvalidArgumentType { t: Primitive, val: String },
}
impl<'a> Function<'a> {
@@ -380,10 +371,10 @@ impl<'a> Function<'a> {
if let Ok(value) = value_check(shell, value.as_ref(), &type_.kind) {
Ok((type_.clone(), value))
} else {
- Err(FunctionError::InvalidArgumentType(
- type_.kind.clone(),
- value.as_ref().into(),
- ))
+ Err(FunctionError::InvalidArgumentType {
+ t: type_.kind.clone(),
+ val: value.as_ref().into(),
+ })
}
})
.collect::<Result<SmallVec<[_; 8]>, _>>()?;
diff --git a/src/lib/shell/mod.rs b/src/lib/shell/mod.rs
index ffd1f292..98b17e19 100644
--- a/src/lib/shell/mod.rs
+++ b/src/lib/shell/mod.rs
@@ -323,18 +323,19 @@ impl<'a> Shell<'a> {
let command_start_time = SystemTime::now();
pipeline.expand(self);
- // Branch if -> input == shell command i.e. echo
- let exit_status = if let Some(main) = self.builtins.get(pipeline.items[0].command()) {
+ // A string representing the command is stored here.
+ if self.opts.print_comms {
+ eprintln!("> {}", pipeline);
+ }
+
+ // Don't execute commands when the `-n` flag is passed.
+ let exit_status = if self.opts.no_exec {
+ Ok(Status::SUCCESS)
+ // Branch else if -> input == shell command i.e. echo
+ } else if let Some(main) = self.builtins.get(pipeline.items[0].command()) {
// Run the 'main' of the command and set exit_status
if !pipeline.requires_piping() {
- if self.opts.print_comms {
- eprintln!("> {}", pipeline.to_string());
- }
- if self.opts.no_exec {
- Status::SUCCESS
- } else {
- main(&pipeline.items[0].job.args, self)
- }
+ Ok(main(&pipeline.items[0].job.args, self))
} else {
self.execute_pipeline(pipeline)
}
@@ -343,25 +344,20 @@ impl<'a> Shell<'a> {
self.variables.get_ref(&pipeline.items[0].job.args[0]).cloned()
{
if !pipeline.requires_piping() {
- match function.execute(self, &pipeline.items[0].job.args) {
- Ok(()) => self.previous_status,
- Err(FunctionError::InvalidArgumentCount) => {
- Status::error("ion: invalid number of function arguments supplied")
- }
- Err(FunctionError::InvalidArgumentType(expected_type, value)) => {
- Status::error(format!(
- "ion: function argument has invalid type: expected {}, found value \
- \'{}\'",
- expected_type, value
- ))
- }
- }
+ function
+ .execute(self, &pipeline.items[0].job.args)
+ .map(|_| self.previous_status)
+ .map_err(Into::into)
} else {
self.execute_pipeline(pipeline)
}
} else {
self.execute_pipeline(pipeline)
- };
+ }
+ .unwrap_or_else(|err| {
+ eprintln!("{}", err);
+ Status::COULD_NOT_EXEC
+ });
if let Some(ref callback) = self.on_command {
if let Ok(elapsed_time) = command_start_time.elapsed() {
diff --git a/src/lib/shell/pipe_exec/fork.rs b/src/lib/shell/pipe_exec/fork.rs
index d6f44d8d..4cd2f48f 100644
--- a/src/lib/shell/pipe_exec/fork.rs
+++ b/src/lib/shell/pipe_exec/fork.rs
@@ -12,12 +12,7 @@ impl<'a> Shell<'a> {
/// Forks the shell, adding the child to the parent's background list, and executing
/// the given commands in the child fork.
- pub(super) fn fork_pipe(
- &mut self,
- pipeline: Pipeline<'a>,
- command_name: String,
- state: ProcessState,
- ) -> Status {
+ pub(super) fn fork_pipe(&mut self, pipeline: Pipeline<'a>, state: ProcessState) -> Status {
match unsafe { sys::fork() } {
Ok(0) => {
self.opts_mut().is_background_shell = true;
@@ -42,7 +37,11 @@ impl<'a> Shell<'a> {
Ok(pid) => {
if state != ProcessState::Empty {
// The parent process should add the child fork's PID to the background.
- self.send_to_background(BackgroundProcess::new(pid, state, command_name));
+ self.send_to_background(BackgroundProcess::new(
+ pid,
+ state,
+ pipeline.to_string(),
+ ));
}
Status::SUCCESS
}
diff --git a/src/lib/shell/pipe_exec/mod.rs b/src/lib/shell/pipe_exec/mod.rs
index 884346ec..a4a1c7df 100644
--- a/src/lib/shell/pipe_exec/mod.rs
+++ b/src/lib/shell/pipe_exec/mod.rs
@@ -65,9 +65,11 @@ pub enum RedirectError {
#[derive(Debug, Error)]
pub enum PipeError {
#[error(display = "ion: {}", cause)]
- RedirectError { cause: RedirectError },
+ RedirectPipeError { cause: RedirectError },
#[error(display = "ion: could not create pipe: {}", cause)]
- CreateError { cause: io::Error },
+ CreatePipeError { cause: io::Error },
+ #[error(display = "ion: could not run function: {}", cause)]
+ RunFunctionError { cause: FunctionError },
}
impl fmt::Display for OutputError {
@@ -100,7 +102,11 @@ impl From<InputError> for RedirectError {
}
impl From<RedirectError> for PipeError {
- fn from(cause: RedirectError) -> Self { PipeError::RedirectError { cause } }
+ fn from(cause: RedirectError) -> Self { PipeError::RedirectPipeError { cause } }
+}
+
+impl From<FunctionError> for PipeError {
+ fn from(cause: FunctionError) -> Self { PipeError::RunFunctionError { cause } }
}
/// Create an OS pipe and write the contents of a byte slice to one end
@@ -332,15 +338,7 @@ impl<'b> Shell<'b> {
if let Some(Value::Function(function)) = self.variables.get_ref(name).cloned() {
match function.execute(self, args) {
Ok(()) => Status::SUCCESS,
- Err(FunctionError::InvalidArgumentCount) => {
- Status::error(format!("ion: invalid number of function arguments supplied"))
- }
- Err(FunctionError::InvalidArgumentType(expected_type, value)) => {
- Status::error(format!(
- "ion: function argument has invalid type: expected {}, found value \'{}\'",
- expected_type, value
- ))
- }
+ Err(why) => Status::error(format!("{}", why)),
}
} else {
unreachable!()
@@ -409,31 +407,17 @@ impl<'b> Shell<'b> {
/// If a job is stopped, the shell will add that job to a list of background jobs and
/// continue to watch the job in the background, printing notifications on status changes
/// of that job over time.
- pub fn execute_pipeline(&mut self, pipeline: Pipeline<'b>) -> Status {
- // Don't execute commands when the `-n` flag is passed.
- if self.opts.no_exec {
- return Status::SUCCESS;
- }
-
- // A string representing the command is stored here.
- let command = pipeline.to_string();
- if self.opts.print_comms {
- eprintln!("> {}", command);
- }
-
+ pub fn execute_pipeline(&mut self, pipeline: Pipeline<'b>) -> Result<Status, PipeError> {
// While active, the SIGTTOU signal will be ignored.
let _sig_ignore = SignalHandler::new();
// If the given pipeline is a background task, fork the shell.
match pipeline.pipe {
- PipeType::Disown => self.fork_pipe(pipeline, command, ProcessState::Empty),
- PipeType::Background => self.fork_pipe(pipeline, command, ProcessState::Running),
+ PipeType::Disown => Ok(self.fork_pipe(pipeline, ProcessState::Empty)),
+ PipeType::Background => Ok(self.fork_pipe(pipeline, ProcessState::Running)),
// Execute each command in the pipeline, giving each command the foreground.
PipeType::Normal => {
- let exit_status = self.pipe(pipeline).unwrap_or_else(|err| {
- eprintln!("{}", err);
- Status::COULD_NOT_EXEC
- });
+ let exit_status = self.pipe(pipeline);
// Set the shell as the foreground process again to regain the TTY.
if !self.opts.is_background_shell {
let _ = sys::tcsetpgrp(0, process::id());
@@ -484,7 +468,7 @@ impl<'b> Shell<'b> {
} else {
// Pipe the previous command's stdin to this commands stdout/stderr.
let (reader, writer) = sys::pipe2(sys::O_CLOEXEC)
- .map_err(|cause| PipeError::CreateError { cause })?;
+ .map_err(|cause| PipeError::CreatePipeError { cause })?;
if is_external {
append_external_stdio_pipe(&mut ext_stdio_pipes, writer);
}
@@ -664,7 +648,7 @@ where
*current_pid = pid;
Ok(())
}
- Err(cause) => Err(PipeError::CreateError { cause }),
+ Err(cause) => Err(PipeError::CreatePipeError { cause }),
}
}
diff --git a/src/lib/shell/pipe_exec/pipes.rs b/src/lib/shell/pipe_exec/pipes.rs
index aab2550b..ed73646c 100644
--- a/src/lib/shell/pipe_exec/pipes.rs
+++ b/src/lib/shell/pipe_exec/pipes.rs
@@ -26,7 +26,7 @@ impl<'a, 'b> TeePipe<'a, 'b> {
F: FnMut(&mut RefinedJob<'b>, File),
{
let (reader, writer) =
- sys::pipe2(sys::O_CLOEXEC).map_err(|cause| PipeError::CreateError { cause })?;
+ sys::pipe2(sys::O_CLOEXEC).map_err(|cause| PipeError::CreatePipeError { cause })?;
(*tee).source = Some(unsafe { File::from_raw_fd(reader) });
action(self.parent, unsafe { File::from_raw_fd(writer) });
if self.is_external {
diff --git a/src/lib/shell/pipe_exec/streams.rs b/src/lib/shell/pipe_exec/streams.rs
index a4c65ae6..0736557c 100644
--- a/src/lib/shell/pipe_exec/streams.rs
+++ b/src/lib/shell/pipe_exec/streams.rs
@@ -23,12 +23,12 @@ pub fn duplicate_streams() -> Result<(Option<File>, File, File), PipeError> {
let stdout = unsafe {
File::from_raw_fd(
- sys::dup(sys::STDOUT_FILENO).map_err(|cause| PipeError::CreateError { cause })?,
+ sys::dup(sys::STDOUT_FILENO).map_err(|cause| PipeError::CreatePipeError { cause })?,
)
};
let stderr = unsafe {
File::from_raw_fd(
- sys::dup(sys::STDERR_FILENO).map_err(|cause| PipeError::CreateError { cause })?,
+ sys::dup(sys::STDERR_FILENO).map_err(|cause| PipeError::CreatePipeError { cause })?,
)
};
// And then meld stderr alongside stdin and stdout
--
GitLab