From f82b6a8378506c3eef97a9363d03299474824690 Mon Sep 17 00:00:00 2001
From: Xavier L'Heureux <xavier.lheureux@icloud.com>
Date: Thu, 27 Jun 2019 15:45:13 -0400
Subject: [PATCH] Use by-module error types

---
 src/lib/parser/mod.rs                |  2 +-
 src/lib/parser/statement/mod.rs      | 20 ++++++------
 src/lib/parser/statement/parse.rs    | 16 +++++-----
 src/lib/parser/statement/splitter.rs | 46 ++++++++++++++--------------
 src/lib/shell/mod.rs                 | 12 ++++----
 src/lib/shell/pipe_exec/mod.rs       |  8 -----
 6 files changed, 48 insertions(+), 56 deletions(-)

diff --git a/src/lib/parser/mod.rs b/src/lib/parser/mod.rs
index d5a82eff..9cb75ebd 100644
--- a/src/lib/parser/mod.rs
+++ b/src/lib/parser/mod.rs
@@ -13,7 +13,7 @@ mod statement;
 
 pub use self::{
     quotes::Terminator,
-    statement::{is_valid_name, parse_and_validate, ParseError, StatementSplitter},
+    statement::{is_valid_name, parse_and_validate, Error, StatementSplitter},
 };
 
 #[cfg(fuzzing)]
diff --git a/src/lib/parser/statement/mod.rs b/src/lib/parser/statement/mod.rs
index 542f7120..18883805 100644
--- a/src/lib/parser/statement/mod.rs
+++ b/src/lib/parser/statement/mod.rs
@@ -15,11 +15,11 @@ use crate::{builtins::BuiltinMap, shell::flow_control::Statement};
 use err_derive::Error;
 use std::char;
 
-pub type Result<'a> = std::result::Result<Statement<'a>, ParseError>;
+pub type Result<'a> = std::result::Result<Statement<'a>, Error>;
 
 /// An Error occured during parsing
 #[derive(Debug, Error, PartialEq, Eq, Hash, Clone)]
-pub enum ParseError {
+pub enum Error {
     /// The command name is illegal
     #[error(display = "illegal command name: {}", _0)]
     IllegalCommandName(String),
@@ -61,7 +61,7 @@ pub enum ParseError {
     NoInKeyword,
     /// Error with match statements
     #[error(display = "case error: {}", _0)]
-    CaseError(#[error(cause)] CaseError),
+    Case(#[error(cause)] CaseError),
     /// The provided function name was invalid
     #[error(
         display = "'{}' is not a valid function name
@@ -74,19 +74,19 @@ pub enum ParseError {
     InvalidFunctionArgument(#[error(cause)] FunctionParseError),
     /// Error occured during parsing of a pipeline
     #[error(display = "{}", _0)]
-    PipelineParsingError(#[error(cause)] PipelineParsingError),
+    Pipeline(#[error(cause)] PipelineParsingError),
 }
 
-impl From<FunctionParseError> for ParseError {
-    fn from(cause: FunctionParseError) -> Self { ParseError::InvalidFunctionArgument(cause) }
+impl From<FunctionParseError> for Error {
+    fn from(cause: FunctionParseError) -> Self { Error::InvalidFunctionArgument(cause) }
 }
 
-impl From<CaseError> for ParseError {
-    fn from(cause: CaseError) -> Self { ParseError::CaseError(cause) }
+impl From<CaseError> for Error {
+    fn from(cause: CaseError) -> Self { Error::Case(cause) }
 }
 
-impl From<PipelineParsingError> for ParseError {
-    fn from(cause: PipelineParsingError) -> Self { ParseError::PipelineParsingError(cause) }
+impl From<PipelineParsingError> for Error {
+    fn from(cause: PipelineParsingError) -> Self { Error::Pipeline(cause) }
 }
 
 /// Parses a given statement string and return's the corresponding mapped
diff --git a/src/lib/parser/statement/parse.rs b/src/lib/parser/statement/parse.rs
index 586fbdce..7488f538 100644
--- a/src/lib/parser/statement/parse.rs
+++ b/src/lib/parser/statement/parse.rs
@@ -1,7 +1,7 @@
 use super::{
     super::pipelines,
     functions::{collect_arguments, parse_function},
-    ParseError,
+    Error,
 };
 use crate::{
     builtins::BuiltinMap,
@@ -24,7 +24,7 @@ pub fn parse<'a>(code: &str, builtins: &BuiltinMap<'a>) -> super::Result<'a> {
         "end" => Ok(Statement::End),
         "break" => Ok(Statement::Break),
         "continue" => Ok(Statement::Continue),
-        "for" | "match" | "case" => Err(ParseError::IncompleteFlowControl),
+        "for" | "match" | "case" => Err(Error::IncompleteFlowControl),
         "let" => Ok(Statement::Let(LocalAction::List)),
         _ if cmd.starts_with("let ") => {
             // Split the let expression and ensure that the statement is valid.
@@ -38,8 +38,8 @@ pub fn parse<'a>(code: &str, builtins: &BuiltinMap<'a>) -> super::Result<'a> {
                         vals.into(),
                     )))
                 }
-                None if op.is_none() => Err(ParseError::NoOperatorSupplied),
-                _ => Err(ParseError::NoValueSupplied),
+                None if op.is_none() => Err(Error::NoOperatorSupplied),
+                _ => Err(Error::NoValueSupplied),
             }
         }
         "export" => Ok(Statement::Export(ExportAction::List)),
@@ -54,8 +54,8 @@ pub fn parse<'a>(code: &str, builtins: &BuiltinMap<'a>) -> super::Result<'a> {
                 (None, Some(keys), None) => {
                     Ok(Statement::Export(ExportAction::LocalExport(keys.into())))
                 }
-                (None, Some(_), Some(_)) => Err(ParseError::NoValueSupplied),
-                (None, None, _) => Err(ParseError::NoKeySupplied),
+                (None, Some(_), Some(_)) => Err(Error::NoValueSupplied),
+                (None, None, _) => Err(Error::NoKeySupplied),
                 _ => unreachable!(),
             }
         }
@@ -97,7 +97,7 @@ pub fn parse<'a>(code: &str, builtins: &BuiltinMap<'a>) -> super::Result<'a> {
                     values: ArgumentSplitter::new(cmd.trim()).map(types::Str::from).collect(),
                     statements: Vec::new(),
                 }),
-                None => Err(ParseError::NoInKeyword),
+                None => Err(Error::NoInKeyword),
             }
         }
         _ if cmd.starts_with("case ") => {
@@ -112,7 +112,7 @@ pub fn parse<'a>(code: &str, builtins: &BuiltinMap<'a>) -> super::Result<'a> {
             let pos = cmd.find(char::is_whitespace).unwrap_or_else(|| cmd.len());
             let name = &cmd[..pos];
             if !is_valid_name(name) {
-                return Err(ParseError::InvalidFunctionName(name.into()));
+                return Err(Error::InvalidFunctionName(name.into()));
             }
 
             let (args, description) = parse_function(&cmd[pos..]);
diff --git a/src/lib/parser/statement/splitter.rs b/src/lib/parser/statement/splitter.rs
index 5c4d1ed0..8c5a4997 100644
--- a/src/lib/parser/statement/splitter.rs
+++ b/src/lib/parser/statement/splitter.rs
@@ -2,7 +2,7 @@
 // - Rewrite this in the same style as shell_expand::words.
 // - Validate syntax in methods
 
-use super::ParseError;
+use super::Error;
 
 #[derive(Debug, Clone, Copy, Hash, Eq, PartialEq)]
 enum LogicalOp {
@@ -72,7 +72,7 @@ impl<'a> StatementSplitter<'a> {
 }
 
 impl<'a> Iterator for StatementSplitter<'a> {
-    type Item = Result<StatementVariant<'a>, ParseError>;
+    type Item = Result<StatementVariant<'a>, Error>;
 
     fn next(&mut self) -> Option<Self::Item> {
         let start = self.read;
@@ -100,7 +100,7 @@ impl<'a> Iterator for StatementSplitter<'a> {
                 {
                     // If we are just ending the braced section continue as normal
                     if error.is_none() {
-                        error = Some(ParseError::InvalidCharacter(character as char, i + 1))
+                        error = Some(Error::InvalidCharacter(character as char, i + 1))
                     }
                 }
                 // Toggle quotes and stop matching variables.
@@ -119,7 +119,7 @@ impl<'a> Iterator for StatementSplitter<'a> {
                 }
                 b'(' if self.variable => self.paren_level += 1,
                 b'(' if error.is_none() && !self.quotes => {
-                    error = Some(ParseError::InvalidCharacter(character as char, i + 1))
+                    error = Some(Error::InvalidCharacter(character as char, i + 1))
                 }
                 b')' if self.math_paren_level == 1 => match bytes.peek() {
                     Some(&(_, b')')) => {
@@ -127,9 +127,9 @@ impl<'a> Iterator for StatementSplitter<'a> {
                         self.skip = true;
                     }
                     Some(&(_, next)) if error.is_none() => {
-                        error = Some(ParseError::InvalidCharacter(next as char, i + 1));
+                        error = Some(Error::InvalidCharacter(next as char, i + 1));
                     }
-                    None if error.is_none() => error = Some(ParseError::UnterminatedArithmetic),
+                    None if error.is_none() => error = Some(Error::UnterminatedArithmetic),
                     _ => {}
                 },
                 b'(' if self.math_paren_level != 0 => {
@@ -137,7 +137,7 @@ impl<'a> Iterator for StatementSplitter<'a> {
                 }
                 b')' if self.paren_level == 0 => {
                     if !self.variable && error.is_none() && !self.quotes {
-                        error = Some(ParseError::InvalidCharacter(character as char, i + 1))
+                        error = Some(Error::InvalidCharacter(character as char, i + 1))
                     }
                     self.variable = false;
                 }
@@ -150,7 +150,7 @@ impl<'a> Iterator for StatementSplitter<'a> {
                 b'}' => {
                     if self.brace_level == 0 {
                         if error.is_none() {
-                            error = Some(ParseError::InvalidCharacter(character as char, i + 1))
+                            error = Some(Error::InvalidCharacter(character as char, i + 1))
                         }
                     } else {
                         self.brace_level -= 1;
@@ -184,23 +184,23 @@ impl<'a> Iterator for StatementSplitter<'a> {
         self.read = self.data.len();
         error.map(Err).or_else(|| {
             if self.paren_level != 0 && self.variable {
-                Some(Err(ParseError::UnterminatedMethod))
+                Some(Err(Error::UnterminatedMethod))
             } else if self.paren_level != 0 {
-                Some(Err(ParseError::UnterminatedSubshell))
+                Some(Err(Error::UnterminatedSubshell))
             } else if self.vbrace {
-                Some(Err(ParseError::UnterminatedBracedVar))
+                Some(Err(Error::UnterminatedBracedVar))
             } else if self.brace_level != 0 {
-                Some(Err(ParseError::UnterminatedBrace))
+                Some(Err(Error::UnterminatedBrace))
             } else if self.math_paren_level != 0 {
-                Some(Err(ParseError::UnterminatedArithmetic))
+                Some(Err(Error::UnterminatedArithmetic))
             } else {
                 let output = self.data[start..].trim();
                 output.as_bytes().get(0).map(|c| match c {
-                    b'>' | b'<' | b'^' => Err(ParseError::ExpectedCommandButFound("redirection")),
-                    b'|' => Err(ParseError::ExpectedCommandButFound("pipe")),
-                    b'&' => Err(ParseError::ExpectedCommandButFound("&")),
+                    b'>' | b'<' | b'^' => Err(Error::ExpectedCommandButFound("redirection")),
+                    b'|' => Err(Error::ExpectedCommandButFound("pipe")),
+                    b'&' => Err(Error::ExpectedCommandButFound("&")),
                     b'*' | b'%' | b'?' | b'{' | b'}' => {
-                        Err(ParseError::IllegalCommandName(String::from(output)))
+                        Err(Error::IllegalCommandName(String::from(output)))
                     }
                     _ => {
                         let stmt = self.get_statement_from(output);
@@ -217,20 +217,20 @@ impl<'a> Iterator for StatementSplitter<'a> {
 fn syntax_errors() {
     let command = "echo (echo one); echo $( (echo one); echo ) two; echo $(echo one";
     let results = StatementSplitter::new(command).collect::<Vec<_>>();
-    assert_eq!(results[0], Err(ParseError::InvalidCharacter('(', 6)));
-    assert_eq!(results[1], Err(ParseError::InvalidCharacter('(', 26)));
-    assert_eq!(results[2], Err(ParseError::InvalidCharacter(')', 43)));
-    assert_eq!(results[3], Err(ParseError::UnterminatedSubshell));
+    assert_eq!(results[0], Err(Error::InvalidCharacter('(', 6)));
+    assert_eq!(results[1], Err(Error::InvalidCharacter('(', 26)));
+    assert_eq!(results[2], Err(Error::InvalidCharacter(')', 43)));
+    assert_eq!(results[3], Err(Error::UnterminatedSubshell));
     assert_eq!(results.len(), 4);
 
     let command = ">echo";
     let results = StatementSplitter::new(command).collect::<Vec<_>>();
-    assert_eq!(results[0], Err(ParseError::ExpectedCommandButFound("redirection")));
+    assert_eq!(results[0], Err(Error::ExpectedCommandButFound("redirection")));
     assert_eq!(results.len(), 1);
 
     let command = "echo $((foo bar baz)";
     let results = StatementSplitter::new(command).collect::<Vec<_>>();
-    assert_eq!(results[0], Err(ParseError::UnterminatedArithmetic));
+    assert_eq!(results[0], Err(Error::UnterminatedArithmetic));
     assert_eq!(results.len(), 1);
 }
 
diff --git a/src/lib/shell/mod.rs b/src/lib/shell/mod.rs
index 522181bf..a830174b 100644
--- a/src/lib/shell/mod.rs
+++ b/src/lib/shell/mod.rs
@@ -17,13 +17,13 @@ pub mod variables;
 pub(crate) use self::job::Job;
 use self::{
     directory_stack::DirectoryStack,
-    flow::BlockError,
     flow_control::{Block, Function, FunctionError, Statement},
     fork::{Fork, IonResult},
     pipe_exec::foreground,
     variables::Variables,
 };
 pub use self::{
+    flow::BlockError,
     fork::Capture,
     pipe_exec::{job_control::BackgroundProcess, PipelineError},
     variables::Value,
@@ -31,10 +31,10 @@ pub use self::{
 use crate::{
     assignments::value_check,
     builtins::{BuiltinMap, Status},
-    expansion::{self, pipelines::Pipeline},
+    expansion::{pipelines::Pipeline, Error as ExpansionError},
     parser::{
         lexers::{Key, Primitive},
-        ParseError, Terminator,
+        Error as ParseError, Terminator,
     },
 };
 use err_derive::Error;
@@ -67,7 +67,7 @@ pub enum IonError {
     PipelineExecutionError(#[error(cause)] PipelineError),
     /// Could not properly expand to a pipeline
     #[error(display = "expansion error: {}", _0)]
-    ExpansionError(#[error(cause)] expansion::Error<IonError>),
+    ExpansionError(#[error(cause)] ExpansionError<IonError>),
 }
 
 impl From<ParseError> for IonError {
@@ -86,8 +86,8 @@ impl From<PipelineError> for IonError {
     fn from(cause: PipelineError) -> Self { IonError::PipelineExecutionError(cause) }
 }
 
-impl From<expansion::Error<IonError>> for IonError {
-    fn from(cause: expansion::Error<Self>) -> Self { IonError::ExpansionError(cause) }
+impl From<ExpansionError<IonError>> for IonError {
+    fn from(cause: ExpansionError<Self>) -> Self { IonError::ExpansionError(cause) }
 }
 
 /// Options for the shell
diff --git a/src/lib/shell/pipe_exec/mod.rs b/src/lib/shell/pipe_exec/mod.rs
index f7ca7064..2a2e7892 100644
--- a/src/lib/shell/pipe_exec/mod.rs
+++ b/src/lib/shell/pipe_exec/mod.rs
@@ -13,7 +13,6 @@ pub mod streams;
 
 use self::{job_control::ProcessState, pipes::TeePipe};
 use super::{
-    flow_control::FunctionError,
     job::{Job, RefinedJob, TeeItem, Variant},
     signals::{self, SignalHandler},
     Shell, Value,
@@ -84,9 +83,6 @@ pub enum PipelineError {
     /// Failed to create a fork
     #[error(display = "could not fork: {}", _0)]
     CreateForkError(#[error(cause)] nix::Error),
-    /// Failed to run function
-    #[error(display = "could not run function: {}", _0)]
-    RunFunctionError(#[error(cause)] FunctionError),
     /// Failed to terminate the jobs after a termination
     #[error(display = "failed to terminate foreground jobs: {}", _0)]
     TerminateJobsError(#[error(cause)] nix::Error),
@@ -158,10 +154,6 @@ impl From<RedirectError> for PipelineError {
     fn from(cause: RedirectError) -> Self { PipelineError::RedirectPipeError(cause) }
 }
 
-impl From<FunctionError> for PipelineError {
-    fn from(cause: FunctionError) -> Self { PipelineError::RunFunctionError(cause) }
-}
-
 /// Create an OS pipe and write the contents of a byte slice to one end
 /// such that reading from this pipe will produce the byte slice. Return
 /// A file descriptor representing the read end of the pipe.
-- 
GitLab