Improve error handling (#194)

Display error messages manually instead of panicing
Add context to file errors
Display correct file name for output log messages

.gitignore

@@ -1,5 +1,6 @@
 /target
 /out
 out.*
+temp
 temp.*
 Cargo.lock

Cargo.toml

@@ -13,6 +13,7 @@ regex = "1.4.3"
 zip = { version = "0.6.3", default-features = false, features=["deflate"] }
 hexf-parse = "0.2.1"
 typed-arena = "2.0.1"
+anyhow = "1.0.70"
 
 
 [[bin]]

src/ass_main.rs

@@ -1,6 +1,7 @@
-use std::path::PathBuf;
-
+use anyhow::bail;
+use anyhow::Result;
 use clap::Parser;
+use std::path::PathBuf;
 
 use crate::file_input_util;
 use crate::file_output_util::Writer;
@@ -14,10 +15,10 @@ pub struct AssemblerCli {
     out: PathBuf,
 }
 
-pub fn assembler_main(cli: AssemblerCli) -> i32 {
+pub fn assembler_main(cli: AssemblerCli) -> Result<()> {
     let opts = AssemblerOptions {};
 
-    let mut writer = Writer::new(&cli.out);
+    let mut writer = Writer::new(&cli.out)?;
     let mut error_count = 0;
     file_input_util::read_files(&cli.input, "j", |fname, data| {
         let data = std::str::from_utf8(data).expect(".j files must be utf8-encoded");
@@ -28,22 +29,20 @@ pub fn assembler_main(cli: AssemblerCli) -> i32 {
             Err(err) => {
                 err.display(fname, data);
                 error_count += 1;
-                return;
+                return Ok(());
             }
         };
         println!("got {} classes", classes.len());
 
         for (name, out) in classes {
             let name = name.map(|name| format!("{}.class", name));
-            writer.write(name.as_deref(), &out);
-            println!("Wrote {} bytes to {}", out.len(), name.as_deref().unwrap_or("file"));
+            writer.write(name.as_deref(), &out)?;
         }
-    });
+        Ok(())
+    })?;
 
-    // set exit code 1 if there were errors
     if error_count > 0 {
-        1
-    } else {
-        0
+        bail!("Finished with {} errors", error_count);
     }
+    Ok(())
 }

src/dis_main.rs

@@ -1,6 +1,7 @@
-use std::path::PathBuf;
-
+use anyhow::bail;
+use anyhow::Result;
 use clap::Parser;
+use std::path::PathBuf;
 
 use crate::file_input_util;
 use crate::file_output_util::Writer;
@@ -21,7 +22,7 @@ pub struct DisassemblerCli {
     no_short_code_attr: bool,
 }
 
-pub fn disassembler_main(cli: DisassemblerCli) -> i32 {
+pub fn disassembler_main(cli: DisassemblerCli) -> Result<()> {
     let opts = DisassemblerOptions {
         roundtrip: cli.roundtrip,
     };
@@ -29,7 +30,7 @@ pub fn disassembler_main(cli: DisassemblerCli) -> i32 {
         no_short_code_attr: cli.no_short_code_attr,
     };
 
-    let mut writer = Writer::new(&cli.out);
+    let mut writer = Writer::new(&cli.out)?;
     let mut error_count = 0;
     file_input_util::read_files(&cli.input, "class", |fname, data| {
         println!("disassemble {}", fname);
@@ -38,18 +39,16 @@ pub fn disassembler_main(cli: DisassemblerCli) -> i32 {
             Err(err) => {
                 eprintln!("Parse error in {}: {}", fname, err.0);
                 error_count += 1;
-                return;
+                return Ok(());
             }
         };
         let name = name.map(|name| format!("{}.j", name));
-        writer.write(name.as_deref(), &out);
-        println!("Wrote {} bytes to {}", out.len(), name.as_deref().unwrap_or("file"));
-    });
+        writer.write(name.as_deref(), &out)?;
+        Ok(())
+    })?;
 
-    // set exit code 1 if there were errors
     if error_count > 0 {
-        1
-    } else {
-        0
+        bail!("Finished with {} errors", error_count);
     }
+    Ok(())
 }

src/file_input_util.rs

@@ -1,26 +1,29 @@
+use anyhow::anyhow;
+use anyhow::bail;
+use anyhow::Result;
 use std::fs;
 use std::io::Read;
 use std::path::Path;
 
 // pub fn read_files<E>(p: &Path, ext: &str, mut cb: impl FnMut(&[u8]) -> Result<(), E>) -> Result<(), E> {
-pub fn read_files(p: &Path, ext: &str, mut cb: impl FnMut(&str, &[u8])) {
+pub fn read_files(p: &Path, ext: &str, mut cb: impl FnMut(&str, &[u8]) -> Result<()>) -> Result<()> {
     let input_ext = p
         .extension()
         .and_then(|s| s.to_str())
-        .expect("Missing input filename extension");
+        .ok_or_else(|| anyhow!("Missing input file extension for '{}'", p.display()))?;
     let input_ext = input_ext.to_ascii_lowercase();
 
     if input_ext == ext {
-        let data = fs::read(p).expect("Error reading input file");
-        cb(&p.to_string_lossy(), &data);
+        let data = fs::read(p)?;
+        cb(&p.to_string_lossy(), &data)?;
     } else if input_ext == "jar" || input_ext == "zip" {
         let mut inbuf = Vec::new();
-        let file = fs::File::open(p).expect("Error reading input file");
-        let mut zip = zip::ZipArchive::new(file).expect("Error parsing archive");
+        let file = fs::File::open(p)?;
+        let mut zip = zip::ZipArchive::new(file)?;
         let ext = format!(".{}", ext); // temp hack
 
         for i in 0..zip.len() {
-            let mut file = zip.by_index(i).expect("Error parsing archive");
+            let mut file = zip.by_index(i)?;
             // println!("found {} {:?} {} {}", i, file.name(), file.size(), file.compressed_size());
 
             let name = file.name().to_owned();
@@ -30,13 +33,13 @@ pub fn read_files(p: &Path, ext: &str, mut cb: impl FnMut(&str, &[u8])) {
 
             inbuf.clear();
             inbuf.reserve(file.size() as usize);
-            file.read_to_end(&mut inbuf).unwrap();
+            file.read_to_end(&mut inbuf)?;
             // println!("read {} bytes", inbuf.len());
 
-            cb(&name, &inbuf);
+            cb(&name, &inbuf)?;
         }
     } else {
-        panic!("Unsupported input extension {}", input_ext)
+        bail!("Unsupported input extension {}", input_ext)
     }
-    // Ok(())
+    Ok(())
 }

src/file_output_util.rs

@@ -3,73 +3,107 @@ use std::io::Write;
 use std::path::Path;
 use std::path::PathBuf;
 
-pub enum Writer {
+use anyhow::anyhow;
+use anyhow::bail;
+use anyhow::Context;
+use anyhow::Result;
+
+pub enum Writer<'a> {
     Dir(PathBuf),
-    Jar(zip::ZipWriter<fs::File>),
-    Merged(fs::File),
-    Single(fs::File, bool),
+    Jar(&'a Path, zip::ZipWriter<fs::File>),
+    Merged(&'a Path, fs::File),
+    Single(&'a Path, fs::File, bool),
 }
-impl Writer {
-    pub fn new(p: &Path) -> Self {
-        fs::create_dir_all(p.parent().unwrap()).unwrap();
+impl<'a> Writer<'a> {
+    pub fn new(p: &'a Path) -> Result<Self> {
+        create_parent(p)?;
         if p.is_dir() {
-            return Self::Dir(p.into());
+            return Ok(Self::Dir(p.into()));
         }
 
-        let f = fs::File::create(p).unwrap();
+        let f = create_file(p)?;
 
         let ext = p.extension().and_then(|s| s.to_str());
-        if let Some(s) = ext {
+        Ok(if let Some(s) = ext {
             match s.to_ascii_lowercase().as_str() {
-                "jar" | "zip" => Self::Jar(zip::ZipWriter::new(f)),
-                "j" => Self::Merged(f),
-                "class" => Self::Single(f, false),
-                _ => panic!("Unsupported output extension {}", s),
+                "jar" | "zip" => Self::Jar(p, zip::ZipWriter::new(f)),
+                "j" => Self::Merged(p, f),
+                "class" => Self::Single(p, f, false),
+                _ => bail!(
+                    "Unsupported output extension {} for {}, expected directory, .jar, .zip, .j, or .class",
+                    s,
+                    p.display()
+                ),
             }
         } else {
-            panic!("Unsupported output extension {:?}", ext)
-        }
+            bail!(
+                "Unsupported output extension None for {}, expected directory, .jar, .zip, .j, or .class",
+                p.display()
+            )
+        })
     }
 
-    pub fn write(&mut self, name: Option<&str>, data: &[u8]) {
+    pub fn write(&mut self, name: Option<&str>, data: &[u8]) -> Result<()> {
         use Writer::*;
         match self {
             Dir(dir) => {
-                let name =
-                    name.expect("Class has missing or invalid name. Try specifying a single file output name explicitly.");
+                let name = name.ok_or_else(|| {
+                    anyhow!("Class has missing or invalid name. Try specifying a single file output name explicitly.")
+                })?;
                 if name.contains("..") {
                     panic!("Invalid path {}. Try outputting to a zip file instead.", name)
                 } else {
                     let p = dir.join(name);
                     println!("Writing to {}", p.display());
-                    fs::create_dir_all(p.parent().unwrap())
-                        .expect("Unable to create directory. Try outputting to a zip file instead");
-                    let mut f = fs::File::create(p).expect("Unable to create file. Try outputting to a zip file instead");
-                    f.write_all(data).unwrap();
+                    create_parent(&p)?;
+                    let mut f = create_file(&p)?;
+                    f.write_all(data)?;
                 }
             }
-            Jar(zw) => {
-                let name =
-                    name.expect("Class has missing or invalid name. Try specifying a single file output name explicitly.");
+            Jar(p, zw) => {
+                let name = name.ok_or_else(|| {
+                    anyhow!("Class has missing or invalid name. Try specifying a single file output name explicitly.")
+                })?;
                 let options = zip::write::FileOptions::default()
                     .compression_method(zip::CompressionMethod::Stored)
                     .last_modified_time(zip::DateTime::default());
 
-                zw.start_file(name, options).unwrap();
-                zw.write_all(data).unwrap();
+                zw.start_file(name, options)?;
+                zw.write_all(data)?;
+                println!("Wrote {} bytes to {} in {}", data.len(), name, p.display());
             }
-            Merged(f) => {
-                f.write_all(data).unwrap();
+            Merged(p, f) => {
+                write(p, f, data)?;
             }
-            Single(f, used) => {
+            Single(p, f, used) => {
                 if *used {
-                    panic!(
+                    bail!(
                         "Error: Attempting to write multiple classes to single file. Try outputting to a zip file instead."
                     )
                 }
-                f.write_all(data).unwrap();
+                write(p, f, data)?;
                 *used = true;
             }
         }
+        Ok(())
     }
 }
+
+fn create_parent(p: &Path) -> Result<()> {
+    let parent = p
+        .parent()
+        .ok_or_else(|| anyhow!("Unable to determine parent directory for {}", p.display()))?;
+    fs::create_dir_all(parent)
+        .with_context(|| format!("Failed to create parent directory {} for {}", parent.display(), p.display()))
+}
+
+fn create_file(p: &Path) -> Result<std::fs::File> {
+    fs::File::create(p).with_context(|| format!("Failed to create output file {}", p.display()))
+}
+
+fn write(p: &Path, f: &mut std::fs::File, data: &[u8]) -> Result<()> {
+    f.write_all(data)
+        .with_context(|| format!("Failed to write output to {}", p.display()))?;
+    println!("Wrote {} bytes to {}", data.len(), p.display());
+    Ok(())
+}

src/lib/classfile/code.rs

@@ -155,7 +155,9 @@ impl SwitchArena {
         i.try_into().unwrap()
     }
 
-    pub fn table(&self, i: u32) -> &SwitchTable {&self.tables[i as usize]}
+    pub fn table(&self, i: u32) -> &SwitchTable {
+        &self.tables[i as usize]
+    }
 
     fn alloc_map(&mut self, v: SwitchMap) -> u32 {
         let i = self.maps.len();
@@ -163,13 +165,11 @@ impl SwitchArena {
         i.try_into().unwrap()
     }
 
-    pub fn map(&self, i: u32) -> &SwitchMap {&self.maps[i as usize]}
+    pub fn map(&self, i: u32) -> &SwitchMap {
+        &self.maps[i as usize]
+    }
 }
 
-
-
-
-
 #[derive(Clone, Copy, Debug)]
 pub enum NewArrayTag {
     Boolean,

src/main.rs

@@ -32,9 +32,16 @@ enum Command {
 
 fn real_main() -> i32 {
     let cli = Cli::parse();
-    match cli.command {
+    let res = match cli.command {
         Command::Asm(cli) => assembler_main(cli),
         Command::Dis(cli) => disassembler_main(cli),
+    };
+    if let Err(err) = res {
+        println!("Error: {:?}", err);
+        // set exit code 1 if there were errors
+        1
+    } else {
+        0
     }
 }
 fn main() {