refactor: pass path conversion errors up the stack

.github/actions/install-llvm/dist/index.js

@@ -975,7 +975,7 @@ async function execute(cmd) {
 (async () => {
     try {
         if(isLinux) {
-            await exec.exec("sudo apt install llvm-7 llvm-7-* lld-7 liblld-7*");
+            await exec.exec("sudo apt install llvm-7 llvm-7-* liblld-7*");
         } else if(isMacOS) {
             await exec.exec("brew install llvm@7")
             let llvmPath = await execute("brew --prefix llvm@7");

.github/actions/install-llvm/index.js

@@ -30,7 +30,7 @@ export async function execute(cmd) {
 (async () => {
     try {
         if(isLinux) {
-            await exec.exec("sudo apt install llvm-7 llvm-7-* lld-7 liblld-7*");
+            await exec.exec("sudo apt install llvm-7 llvm-7-* liblld-7*");
         } else if(isMacOS) {
             await exec.exec("brew install llvm@7")
             let llvmPath = await execute("brew --prefix llvm@7");

README.md

@@ -88,7 +88,7 @@ runners](.github/actions/install-llvm/index.js):
 * ***nix**: Package managers of recent *nix distros can install binary versions of LLVM, e.g.:
   ```bash
   # Ubuntu 18.04
-  sudo apt install llvm-7 llvm-7-* lld-7 liblld-7*
+  sudo apt install llvm-7 llvm-7-* liblld-7*
   ```
 * **macOS**: [Brew](https://brew.sh/) contains a binary distribution of LLVM 7.1.0. However, as it's
   not the latest version, it won't be added to the path. We are using

crates/mun_codegen/src/code_gen.rs

@@ -1,3 +1,4 @@
+use crate::code_gen::linker::LinkerError;
 use crate::IrDatabase;
 use failure::Fail;
 use inkwell::module::Module;
@@ -13,8 +14,10 @@ mod linker;
 
 #[derive(Debug, Fail)]
 enum CodeGenerationError {
-    #[fail(display = "linker error: {}", 0)]
-    LinkerError(String),
+    #[fail(display = "{}", 0)]
+    LinkerError(#[fail(cause)] LinkerError),
+    #[fail(display = "error linking modules: {}", 0)]
+    ModuleLinkerError(String),
     #[fail(display = "unknown target triple: {}", 0)]
     UnknownTargetTriple(String),
     #[fail(display = "error creating target machine")]
@@ -25,6 +28,12 @@ enum CodeGenerationError {
     CodeGenerationError(String),
 }
 
+impl From<LinkerError> for CodeGenerationError {
+    fn from(e: LinkerError) -> Self {
+        CodeGenerationError::LinkerError(e)
+    }
+}
+
 /// Construct a shared object for the given `hir::FileId` at the specified output file location.
 pub fn write_module_shared_object(
     db: &impl IrDatabase,
@@ -46,7 +55,7 @@ pub fn write_module_shared_object(
     let module = db.module_ir(file_id);
     assembly_module
         .link_in_module(module.llvm_module.clone())
-        .map_err(|e| CodeGenerationError::LinkerError(e.to_string()))?;
+        .map_err(|e| CodeGenerationError::ModuleLinkerError(e.to_string()))?;
 
     // Generate the `get_info` method.
     symbols::gen_reflection_ir(
@@ -97,14 +106,13 @@ pub fn write_module_shared_object(
 
     // Construct a linker for the target
     let mut linker = linker::create_with_target(&target);
-    linker.add_object(obj_file.path());
+    linker.add_object(obj_file.path())?;
 
     // Link the object
-    linker.build_shared_object(&output_file_path);
+    linker.build_shared_object(&output_file_path)?;
+    linker.finalize()?;
 
-    linker
-        .finalize()
-        .map_err(|e| CodeGenerationError::LinkerError(e).into())
+    Ok(())
 }
 
 /// Optimizes the specified LLVM `Module` using the default passes for the given

crates/mun_codegen/src/code_gen/linker.rs

@@ -1,6 +1,30 @@
+use failure::Fail;
 use mun_target::spec;
 use mun_target::spec::LinkerFlavor;
-use std::path::Path;
+use std::fmt;
+use std::path::{Path, PathBuf};
+
+#[derive(Fail, Debug)]
+pub enum LinkerError {
+    /// Error emitted by the linker
+    LinkError(String),
+
+    /// Error in path conversion
+    PathError(PathBuf),
+}
+
+impl fmt::Display for LinkerError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
+        match self {
+            LinkerError::LinkError(e) => write!(f, "{}", e),
+            LinkerError::PathError(path) => write!(
+                f,
+                "path contains invalid UTF-8 characters: {}",
+                path.display()
+            ),
+        }
+    }
+}
 
 pub fn create_with_target(target: &spec::Target) -> Box<dyn Linker> {
     match target.linker_flavor {
@@ -11,9 +35,9 @@ pub fn create_with_target(target: &spec::Target) -> Box<dyn Linker> {
 }
 
 pub trait Linker {
-    fn add_object(&mut self, path: &Path);
-    fn build_shared_object(&mut self, path: &Path);
-    fn finalize(&mut self) -> Result<(), String>;
+    fn add_object(&mut self, path: &Path) -> Result<(), LinkerError>;
+    fn build_shared_object(&mut self, path: &Path) -> Result<(), LinkerError>;
+    fn finalize(&mut self) -> Result<(), LinkerError>;
 }
 
 struct LdLinker {
@@ -29,21 +53,34 @@ impl LdLinker {
 }
 
 impl Linker for LdLinker {
-    fn add_object(&mut self, path: &Path) {
-        self.args.push(path.to_string_lossy().to_string());
+    fn add_object(&mut self, path: &Path) -> Result<(), LinkerError> {
+        let path_str = path
+            .to_str()
+            .ok_or_else(|| LinkerError::PathError(path.to_owned()))?
+            .to_owned();
+        self.args.push(path_str);
+        Ok(())
     }
 
-    fn build_shared_object(&mut self, path: &Path) {
+    fn build_shared_object(&mut self, path: &Path) -> Result<(), LinkerError> {
+        let path_str = path
+            .to_str()
+            .ok_or_else(|| LinkerError::PathError(path.to_owned()))?;
+
         // Link as dynamic library
-        self.args.push("--shared".to_string());
+        self.args.push("--shared".to_owned());
 
         // Specify output path
-        self.args.push("-o".to_string());
-        self.args.push(path.to_str().unwrap().to_string());
+        self.args.push("-o".to_owned());
+        self.args.push(path_str.to_owned());
+
+        Ok(())
     }
 
-    fn finalize(&mut self) -> Result<(), String> {
-        mun_lld::link(mun_lld::LldFlavor::Elf, &self.args).ok()
+    fn finalize(&mut self) -> Result<(), LinkerError> {
+        mun_lld::link(mun_lld::LldFlavor::Elf, &self.args)
+            .ok()
+            .map_err(LinkerError::LinkError)
     }
 }
 
@@ -60,22 +97,35 @@ impl Ld64Linker {
 }
 
 impl Linker for Ld64Linker {
-    fn add_object(&mut self, path: &Path) {
-        self.args.push(path.to_string_lossy().to_string());
+    fn add_object(&mut self, path: &Path) -> Result<(), LinkerError> {
+        let path_str = path
+            .to_str()
+            .ok_or_else(|| LinkerError::PathError(path.to_owned()))?
+            .to_owned();
+        self.args.push(path_str);
+        Ok(())
     }
 
-    fn build_shared_object(&mut self, path: &Path) {
+    fn build_shared_object(&mut self, path: &Path) -> Result<(), LinkerError> {
+        let path_str = path
+            .to_str()
+            .ok_or_else(|| LinkerError::PathError(path.to_owned()))?;
+
         // Link as dynamic library
-        self.args.push("-dylib".to_string());
-        self.args.push("-lsystem".to_string());
+        self.args.push("-dylib".to_owned());
+        self.args.push("-lsystem".to_owned());
 
         // Specify output path
-        self.args.push("-o".to_string());
-        self.args.push(path.to_str().unwrap().to_string());
+        self.args.push("-o".to_owned());
+        self.args.push(path_str.to_owned());
+
+        Ok(())
     }
 
-    fn finalize(&mut self) -> Result<(), String> {
-        mun_lld::link(mun_lld::LldFlavor::MachO, &self.args).ok()
+    fn finalize(&mut self) -> Result<(), LinkerError> {
+        mun_lld::link(mun_lld::LldFlavor::MachO, &self.args)
+            .ok()
+            .map_err(LinkerError::LinkError)
     }
 }
 
@@ -92,22 +142,35 @@ impl MsvcLinker {
 }
 
 impl Linker for MsvcLinker {
-    fn add_object(&mut self, path: &Path) {
-        self.args.push(path.to_string_lossy().to_string());
+    fn add_object(&mut self, path: &Path) -> Result<(), LinkerError> {
+        let path_str = path
+            .to_str()
+            .ok_or_else(|| LinkerError::PathError(path.to_owned()))?
+            .to_owned();
+        self.args.push(path_str);
+        Ok(())
     }
 
-    fn build_shared_object(&mut self, path: &Path) {
-        self.args.push("/DLL".to_string());
-        self.args.push("/NOENTRY".to_string());
-        self.args.push("/EXPORT:get_info".to_string());
-        self.args.push(format!(
-            "/IMPLIB:{}",
-            path.with_extension("dll.lib").to_string_lossy()
-        ));
-        self.args.push(format!("/OUT:{}", path.to_string_lossy()));
+    fn build_shared_object(&mut self, path: &Path) -> Result<(), LinkerError> {
+        let dll_path_str = path
+            .to_str()
+            .ok_or_else(|| LinkerError::PathError(path.to_owned()))?;
+
+        let dll_lib_path_str = path
+            .to_str()
+            .ok_or_else(|| LinkerError::PathError(path.to_owned()))?;
+
+        self.args.push("/DLL".to_owned());
+        self.args.push("/NOENTRY".to_owned());
+        self.args.push("/EXPORT:get_info".to_owned());
+        self.args.push(format!("/IMPLIB:{}", dll_lib_path_str));
+        self.args.push(format!("/OUT:{}", dll_path_str));
+        Ok(())
     }
 
-    fn finalize(&mut self) -> Result<(), String> {
-        mun_lld::link(mun_lld::LldFlavor::Coff, &self.args).ok()
+    fn finalize(&mut self) -> Result<(), LinkerError> {
+        mun_lld::link(mun_lld::LldFlavor::Coff, &self.args)
+            .ok()
+            .map_err(LinkerError::LinkError)
     }
 }

crates/mun_lld/Cargo.toml

@@ -7,14 +7,8 @@ edition = "2018"
 [dependencies]
 libc = "0.2.65"
 
-[build-dependencies.cc]
-version = "1.0"
-
-[build-dependencies.lazy_static]
-version = "1.0"
-
-[build-dependencies.regex]
-version = "1.0"
-
-[build-dependencies.semver]
-version = "0.9"
+[build-dependencies]
+cc = "1.0"
+lazy_static = "1.4"
+regex = "1.3"
+semver = "0.9"

crates/mun_lld/build.rs

@@ -284,33 +284,6 @@ fn get_link_libraries() -> Vec<String> {
         .collect::<Vec<String>>()
 }
 
-// fn get_llvm_cflags() -> String {
-//     let output = llvm_config("--cflags");
-
-//     // llvm-config includes cflags from its own compilation with --cflags that
-//     // may not be relevant to us. In particularly annoying cases, these might
-//     // include flags that aren't understood by the default compiler we're
-//     // using. Unless requested otherwise, clean CFLAGS of options that are
-//     // known to be possibly-harmful.
-//     let no_clean = env::var_os(format!(
-//         "LLVM_SYS_{}_NO_CLEAN_CFLAGS",
-//         env!("CARGO_PKG_VERSION_MAJOR")
-//     ))
-//     .is_some();
-//     if no_clean || cfg!(target_env = "msvc") {
-//         // MSVC doesn't accept -W... options, so don't try to strip them and
-//         // possibly strip something that should be retained. Also do nothing if
-//         // the user requests it.
-//         return output;
-//     }
-
-//     llvm_config("--cflags")
-//         .split(&[' ', '\n'][..])
-//         .filter(|word| !word.starts_with("-W"))
-//         .collect::<Vec<_>>()
-//         .join(" ")
-// }
-
 fn get_llvm_cxxflags() -> String {
     let output = llvm_config("--cxxflags");