CLI print warnings + errors and proper exit codes

Also use builtin parse errors from pest and general cleanup in CLI

Author: ian-h-chamberlain

src/cli.rs

@@ -1,19 +1,24 @@
-use std::{error::Error, fs, path::PathBuf};
+use std::{
+    error::Error,
+    fs,
+    io::{self, Read},
+    path::PathBuf,
+};
 
 use lazy_static::lazy_static;
 use structopt::StructOpt;
 
 use crate::parser;
 
 lazy_static! {
-    static ref STDOUT: PathBuf = PathBuf::from("-");
+    static ref IO_SENTINEL: PathBuf = PathBuf::from("-");
 }
 
 #[derive(Debug, StructOpt)]
 #[structopt(rename_all = "kebab")]
 /// Parse, assemble, and save Redcode files
 struct CliOptions {
-    /// Input file
+    /// Input file; use '-' to read from stdin
     #[structopt(parse(from_os_str))]
     input_file: PathBuf,
 
@@ -26,8 +31,8 @@ enum Command {
     /// Save/print a program in 'load file' format
     #[structopt(name = "dump")]
     Dump {
-        /// Output file - defaults to stdout ('-')
-        #[structopt(long, short, parse(from_os_str), default_value = STDOUT.to_str().unwrap())]
+        /// Output file; defaults to stdout ('-')
+        #[structopt(long, short, parse(from_os_str), default_value = IO_SENTINEL.to_str().unwrap())]
         output_file: PathBuf,
 
         /// Whether labels, expressions, macros, etc. should be resolved and
@@ -40,21 +45,32 @@ enum Command {
 pub fn run() -> Result<(), Box<dyn Error>> {
     let cli_options = CliOptions::from_args();
 
-    let input_program = fs::read_to_string(cli_options.input_file)?;
+    let mut input = String::new();
 
-    let mut parsed_core = parser::parse(input_program.as_str())?;
+    if cli_options.input_file == *IO_SENTINEL {
+        io::stdin().read_to_string(&mut input)?;
+    } else {
+        input = fs::read_to_string(cli_options.input_file)?;
+    }
+
+    let mut parsed_core = parser::parse(input.as_str())?;
+
+    // TODO colored output?
+    for warning in parsed_core.warnings.iter() {
+        eprintln!("Warning:\n  {}", warning);
+    }
 
     match cli_options.command {
         Command::Dump {
             output_file,
             no_expand,
         } => {
             if !no_expand {
-                parsed_core.resolve()?;
+                parsed_core.result.resolve()?;
             }
 
-            if output_file == *STDOUT {
-                println!("{}", parsed_core);
+            if output_file == *IO_SENTINEL {
+                print!("{}", parsed_core);
             } else {
                 fs::write(output_file, format!("{}", parsed_core))?;
             };

src/load_file.rs

@@ -1,7 +1,4 @@
-use std::{
-    collections::{hash_map::Entry, HashMap},
-    fmt,
-};
+use std::{collections::HashMap, fmt};
 
 mod types;
 pub use types::{AddressMode, Modifier, Opcode, Value};
@@ -169,12 +166,10 @@ impl Core {
             ));
         }
 
-        match self.labels.entry(label) {
-            Entry::Occupied(entry) => Err(format!("Label '{}' already exists", entry.key())),
-            Entry::Vacant(entry) => {
-                entry.insert(index);
-                Ok(())
-            }
+        if self.labels.insert(label.clone(), index).is_some() {
+            Err(format!("Label '{}' already exists", label))
+        } else {
+            Ok(())
         }
     }
 
@@ -252,11 +247,13 @@ mod tests {
         core.add_label(256, "goblin".into())
             .expect_err("Should fail to add labels > 200");
         core.add_label(5, "baz".into())
-            .expect_err("Should fail to add duplicate label");
+            .expect_err("Should error duplicate label");
 
         assert_eq!(core.label_address("foo").unwrap(), 0);
         assert_eq!(core.label_address("bar").unwrap(), 0);
-        assert_eq!(core.label_address("baz").unwrap(), 123);
+
+        // The _last_ version of a label will be the one we use
+        assert_eq!(core.label_address("baz").unwrap(), 5);
 
         assert!(core.label_address("goblin").is_none());
         assert!(core.label_address("never_mentioned").is_none());

src/main.rs

@@ -1,9 +1,16 @@
 extern crate corewa_rs;
 
-use std::error::Error;
-
 use corewa_rs::cli;
 
-fn main() -> Result<(), Box<dyn Error>> {
-    cli::run()
+fn main() {
+    std::process::exit(
+        // TODO use exitcode lib or something like that
+        if let Err(err) = cli::run() {
+            eprintln!("Error: {}", err);
+            -1
+        } else {
+            // TODO use exit codes for warnings?
+            0
+        },
+    )
 }

src/parser.rs

@@ -1,16 +1,13 @@
 use std::{error, fmt, str::FromStr};
 
 use itertools::Itertools;
-use pest::{
-    error::{Error as PestError, ErrorVariant, LineColLocation},
-    iterators::{Pair, Pairs},
-};
+use pest::iterators::{Pair, Pairs};
 
 use crate::load_file::{AddressMode, Core, Field, Instruction, Modifier, Opcode, Value};
 
 mod grammar;
 
-#[derive(Debug)]
+#[derive(Debug, PartialEq)]
 pub struct Error {
     details: String,
 }
@@ -31,41 +28,39 @@ impl Error {
     }
 }
 
-impl From<PestError<grammar::Rule>> for Error {
-    fn from(pest_error: PestError<grammar::Rule>) -> Error {
+pub trait IntoError: fmt::Display {}
+
+impl<T: pest::RuleType> IntoError for pest::error::Error<T> {}
+impl IntoError for String {}
+impl IntoError for &str {}
+
+impl<T: IntoError> From<T> for Error {
+    fn from(displayable_error: T) -> Self {
         Error {
-            details: format!(
-                "Parse error: {} {}",
-                match pest_error.variant {
-                    ErrorVariant::ParsingError {
-                        positives,
-                        negatives,
-                    } => format!("expected one of {:?}, none of {:?}", positives, negatives),
-                    ErrorVariant::CustomError { message } => message,
-                },
-                match pest_error.line_col {
-                    LineColLocation::Pos((line, col)) => format!("at line {} column {}", line, col),
-                    LineColLocation::Span((start_line, start_col), (end_line, end_col)) => format!(
-                        "from line {} column {} to line {} column {}",
-                        start_line, start_col, end_line, end_col
-                    ),
-                }
-            ),
+            details: format!("{}", displayable_error),
         }
     }
 }
 
-impl From<String> for Error {
-    fn from(details: String) -> Error {
-        Error { details }
+#[derive(Debug)]
+pub struct ParsedCore {
+    pub result: Core,
+    pub warnings: Vec<Error>,
+}
+
+impl fmt::Display for ParsedCore {
+    fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+        write!(formatter, "{}", self.result)
     }
 }
 
-pub fn parse(file_contents: &str) -> Result<Core, Error> {
+pub fn parse(file_contents: &str) -> Result<ParsedCore, Error> {
     if file_contents.is_empty() {
         return Err(Error::no_input());
     }
 
+    let mut warnings = Vec::new();
+
     let mut core = Core::default();
 
     let parse_result = grammar::parse(grammar::Rule::File, file_contents)?
@@ -83,7 +78,9 @@ pub fn parse(file_contents: &str) -> Result<Core, Error> {
             .map(|pair| pair.as_str().to_owned());
 
         for label in label_pairs {
-            core.add_label(i, label.to_string())?;
+            if let Err(failed_add) = core.add_label(i, label.to_string()) {
+                warnings.push(failed_add.into())
+            }
         }
 
         if line_pair.peek().is_some() {
@@ -92,7 +89,10 @@ pub fn parse(file_contents: &str) -> Result<Core, Error> {
         }
     }
 
-    Ok(core)
+    Ok(ParsedCore {
+        result: core,
+        warnings,
+    })
 }
 
 fn parse_instruction(mut instruction_pairs: Pairs<grammar::Rule>) -> Instruction {
@@ -188,10 +188,10 @@ mod tests {
 
     #[test]
     fn parse_empty() {
-        let result = parse("");
-        assert!(result.is_err());
+        let parsed = parse("");
+        assert!(parsed.is_err());
 
-        assert_eq!(result.unwrap_err().details, "No input found");
+        assert_eq!(parsed.unwrap_err().details, "No input found");
     }
 
     #[test]
@@ -201,7 +201,14 @@ mod tests {
             label1  dat 0,0
         ";
 
-        parse(simple_input).expect_err("Should fail for duplicate label");
+        let parsed = parse(simple_input).expect("Failed to parse");
+
+        assert_eq!(
+            parsed.warnings,
+            vec![Error::from("Label 'label1' already exists")]
+        );
+
+        assert_eq!(parsed.result.label_address("label1"), Some(1));
     }
 
     #[test]
@@ -249,8 +256,10 @@ mod tests {
             .expect("Should resolve a core with no labels");
 
         let mut parsed = parse(simple_input).expect("Should parse simple file");
-        parsed.resolve().expect("Parsed file should resolve");
+        parsed.result.resolve().expect("Parsed file should resolve");
+
+        assert!(parsed.warnings.is_empty());
 
-        assert_eq!(parsed, expected_core);
+        assert_eq!(parsed.result, expected_core);
     }
 }

tests/dump_test.rs

@@ -4,15 +4,18 @@ fn run_test(input: &str, expected_output: &str) {
     eprintln!("Parsing warrior:");
     eprintln!("{}", input);
 
-    let mut core = corewa_rs::parse(input).expect("Failed to parse input");
-    core.resolve().expect("Failed to resolve parsed input");
+    let mut parsed_core = corewa_rs::parse(input).unwrap_or_else(|e| panic!("Parse error:\n{}", e));
+    parsed_core
+        .result
+        .resolve()
+        .unwrap_or_else(|e| panic!("{}", e));
 
     // TODO: dump and check output of pre-resolved core
 
     eprintln!("Loaded core:");
-    eprintln!("{:?}", core);
+    dbg!(&parsed_core);
 
-    assert_eq!(format!("{}", core), expected_output);
+    assert_eq!(format!("{}", parsed_core), expected_output);
 }
 
 #[test]