Leverage OsString much more deeply througout

This initially started out tackling mozilla#93 and ended up plumbing the `OsStr` type
basically everywhere! This knocked out a TODO or two along the way and should
remove some pesky unwraps and/or error handling around handling of arguments and
paths that may not be unicode.

Some notable other points are:

* A few utilities were added to `src/util.rs` to assist with handling os strings
* The Rust implementation doesn't deal with os strings in argument parsing at
  all, but otherwise internally stores os strings for maximal compatibility
  (more comments in the code).
* Some unsafe transmutes were removed in util in favor of a more stable
  implementation.
* The `output_file` function was renamed to `output_pretty` as it turns out it's
  not actually the actual output file but rather just a pretty description of it
  (for logging and such).

Closes mozilla#93

src/commands.rs

@@ -404,11 +404,9 @@ fn request_compile<W, X, Y>(conn: &mut ServerConnection, exe: W, args: &Vec<X>,
           Y: AsRef<Path>,
 {
     let req = Request::Compile(Compile {
-        exe: exe.as_ref().to_str().ok_or("bad exe")?.to_owned(),
-        cwd: cwd.as_ref().to_str().ok_or("bad cwd")?.to_owned(),
-        args: args.iter().map(|a| {
-            a.as_ref().to_str().ok_or("bad arg".into()).map(|s| s.to_owned())
-        }).collect::<Result<_>>()?,
+        exe: exe.as_ref().to_owned().into(),
+        cwd: cwd.as_ref().to_owned().into(),
+        args: args.iter().map(|a| a.as_ref().to_owned()).collect(),
         env_vars: env_vars,
     });
     trace!("request_compile: {:?}", req);

src/compiler/c.rs

@@ -21,9 +21,10 @@ use std::borrow::Cow;
 use std::collections::{HashMap, HashSet};
 use std::ffi::{OsStr, OsString};
 use std::fmt;
-use std::path::Path;
+use std::hash::Hash;
+use std::path::{Path, PathBuf};
 use std::process;
-use util::{os_str_bytes, sha1_digest};
+use util::{HashToSha1, sha1_digest};
 
 use errors::*;
 
@@ -32,7 +33,7 @@ use errors::*;
 pub struct CCompiler<I>
     where I: CCompilerImpl,
 {
-    executable: String,
+    executable: PathBuf,
     executable_digest: String,
     compiler: I,
 }
@@ -43,7 +44,7 @@ pub struct CCompilerHasher<I>
     where I: CCompilerImpl,
 {
     parsed_args: ParsedArguments,
-    executable: String,
+    executable: PathBuf,
     executable_digest: String,
     compiler: I,
 }
@@ -53,31 +54,34 @@ pub struct CCompilerHasher<I>
 #[derive(Debug, PartialEq, Clone)]
 pub struct ParsedArguments {
     /// The input source file.
-    pub input: String,
+    pub input: PathBuf,
     /// The file extension of the input source file.
     pub extension: String,
     /// The file in which to generate dependencies.
-    pub depfile: Option<String>,
+    pub depfile: Option<PathBuf>,
     /// Output files, keyed by a simple name, like "obj".
-    pub outputs: HashMap<&'static str, String>,
+    pub outputs: HashMap<&'static str, PathBuf>,
     /// Commandline arguments for the preprocessor.
-    pub preprocessor_args: Vec<String>,
+    pub preprocessor_args: Vec<OsString>,
     /// Commandline arguments for the preprocessor or the compiler.
-    pub common_args: Vec<String>,
+    pub common_args: Vec<OsString>,
     /// Whether or not the `-showIncludes` argument is passed on MSVC
     pub msvc_show_includes: bool,
 }
 
 impl ParsedArguments {
-    pub fn output_file(&self) -> Cow<str> {
-        self.outputs.get("obj").and_then(|o| Path::new(o).file_name().map(|f| f.to_string_lossy())).unwrap_or(Cow::Borrowed("Unknown filename"))
+    pub fn output_pretty(&self) -> Cow<str> {
+        self.outputs.get("obj")
+            .and_then(|o| o.file_name())
+            .map(|s| s.to_string_lossy())
+            .unwrap_or(Cow::Borrowed("Unknown filename"))
     }
 }
 
 /// A generic implementation of the `Compilation` trait for C/C++ compilers.
 struct CCompilation<I: CCompilerImpl> {
     parsed_args: ParsedArguments,
-    executable: String,
+    executable: PathBuf,
     /// The output from running the preprocessor.
     preprocessor_result: process::Output,
     compiler: I,
@@ -100,25 +104,25 @@ pub trait CCompilerImpl: Clone + fmt::Debug + Send + 'static {
     fn kind(&self) -> CCompilerKind;
     /// Determine whether `arguments` are supported by this compiler.
     fn parse_arguments(&self,
-                       arguments: &[String],
+                       arguments: &[OsString],
                        cwd: &Path) -> CompilerArguments<ParsedArguments>;
     /// Run the C preprocessor with the specified set of arguments.
     fn preprocess<T>(&self,
                      creator: &T,
-                     executable: &str,
+                     executable: &Path,
                      parsed_args: &ParsedArguments,
-                     cwd: &str,
+                     cwd: &Path,
                      env_vars: &[(OsString, OsString)],
                      pool: &CpuPool)
                      -> SFuture<process::Output> where T: CommandCreatorSync;
     /// Run the C compiler with the specified set of arguments, using the
     /// previously-generated `preprocessor_output` as input if possible.
     fn compile<T>(&self,
                   creator: &T,
-                  executable: &str,
+                  executable: &Path,
                   preprocessor_result: process::Output,
                   parsed_args: &ParsedArguments,
-                  cwd: &str,
+                  cwd: &Path,
                   env_vars: &[(OsString, OsString)],
                   pool: &CpuPool)
                   -> SFuture<(Cacheable, process::Output)>
@@ -128,7 +132,7 @@ pub trait CCompilerImpl: Clone + fmt::Debug + Send + 'static {
 impl <I> CCompiler<I>
     where I: CCompilerImpl,
 {
-    pub fn new(compiler: I, executable: String, pool: &CpuPool) -> SFuture<CCompiler<I>>
+    pub fn new(compiler: I, executable: PathBuf, pool: &CpuPool) -> SFuture<CCompiler<I>>
     {
         Box::new(sha1_digest(executable.clone(), &pool).map(move |digest| {
             CCompiler {
@@ -143,7 +147,7 @@ impl <I> CCompiler<I>
 impl<T: CommandCreatorSync, I: CCompilerImpl> Compiler<T> for CCompiler<I> {
     fn kind(&self) -> CompilerKind { CompilerKind::C(self.compiler.kind()) }
     fn parse_arguments(&self,
-                       arguments: &[String],
+                       arguments: &[OsString],
                        cwd: &Path) -> CompilerArguments<Box<CompilerHasher<T> + 'static>> {
         match self.compiler.parse_arguments(arguments, cwd) {
             CompilerArguments::Ok(args) => {
@@ -170,26 +174,26 @@ impl<T, I> CompilerHasher<T> for CCompilerHasher<I>
 {
     fn generate_hash_key(self: Box<Self>,
                          creator: &T,
-                         cwd: &str,
+                         cwd: &Path,
                          env_vars: &[(OsString, OsString)],
                          pool: &CpuPool)
                          -> SFuture<HashResult<T>>
     {
         let me = *self;
         let CCompilerHasher { parsed_args, executable, executable_digest, compiler } = me;
         let result = compiler.preprocess(creator, &executable, &parsed_args, cwd, env_vars, pool);
-        let out_file = parsed_args.output_file().into_owned();
+        let out_pretty = parsed_args.output_pretty().into_owned();
         let env_vars = env_vars.to_vec();
         let result = result.map_err(move |e| {
-            debug!("[{}]: preprocessor failed: {:?}", out_file, e);
+            debug!("[{}]: preprocessor failed: {:?}", out_pretty, e);
             e
         });
-        let out_file = parsed_args.output_file().into_owned();
+        let out_pretty = parsed_args.output_pretty().into_owned();
         Box::new(result.or_else(move |err| {
             match err {
                 Error(ErrorKind::ProcessError(output), _) => {
                     debug!("[{}]: preprocessor returned error status {:?}",
-                           out_file,
+                           out_pretty,
                            output.status.code());
                     // Drop the stdout since it's the preprocessor output, just hand back stderr and
                     // the exit status.
@@ -202,17 +206,14 @@ impl<T, I> CompilerHasher<T> for CCompilerHasher<I>
             }
         }).and_then(move |preprocessor_result| {
             trace!("[{}]: Preprocessor output is {} bytes",
-                   parsed_args.output_file(),
+                   parsed_args.output_pretty(),
                    preprocessor_result.stdout.len());
 
-            // Remove object file from arguments before hash calculation
             let key = {
-                let out_file = parsed_args.output_file();
-                let arguments = parsed_args.common_args.iter()
-                    .filter(|a| **a != out_file)
-                    .map(|a| a.as_str())
-                    .collect::<String>();
-                hash_key(&executable_digest, &arguments, &env_vars, &preprocessor_result.stdout)
+                hash_key(&executable_digest,
+                         &parsed_args.common_args,
+                         &env_vars,
+                         &preprocessor_result.stdout)
             };
             Ok(HashResult {
                 key: key,
@@ -226,9 +227,9 @@ impl<T, I> CompilerHasher<T> for CCompilerHasher<I>
         }))
     }
 
-    fn output_file(&self) -> Cow<str>
+    fn output_pretty(&self) -> Cow<str>
     {
-        self.parsed_args.output_file()
+        self.parsed_args.output_pretty()
     }
 
     fn box_clone(&self) -> Box<CompilerHasher<T>>
@@ -240,7 +241,7 @@ impl<T, I> CompilerHasher<T> for CCompilerHasher<I>
 impl<T: CommandCreatorSync, I: CCompilerImpl> Compilation<T> for CCompilation<I> {
     fn compile(self: Box<Self>,
                creator: &T,
-               cwd: &str,
+               cwd: &Path,
                env_vars: &[(OsString, OsString)],
                pool: &CpuPool)
                -> SFuture<(Cacheable, process::Output)>
@@ -251,9 +252,9 @@ impl<T: CommandCreatorSync, I: CCompilerImpl> Compilation<T> for CCompilation<I>
                          pool)
     }
 
-    fn outputs<'a>(&'a self) -> Box<Iterator<Item=(&'a str, &'a String)> + 'a>
+    fn outputs<'a>(&'a self) -> Box<Iterator<Item=(&'a str, &'a Path)> + 'a>
     {
-        Box::new(self.parsed_args.outputs.iter().map(|(k, v)| (*k, v)))
+        Box::new(self.parsed_args.outputs.iter().map(|(k, v)| (*k, &**v)))
     }
 }
 
@@ -267,21 +268,25 @@ pub const CACHED_ENV_VARS : &'static [&'static str] = &[
 ];
 
 /// Compute the hash key of `compiler` compiling `preprocessor_output` with `args`.
-pub fn hash_key(compiler_digest: &str, arguments: &str, env_vars: &[(OsString, OsString)],
+pub fn hash_key(compiler_digest: &str,
+                arguments: &[OsString],
+                env_vars: &[(OsString, OsString)],
                 preprocessor_output: &[u8]) -> String
 {
     // If you change any of the inputs to the hash, you should change `CACHE_VERSION`.
     let mut m = sha1::Sha1::new();
     m.update(compiler_digest.as_bytes());
     m.update(CACHE_VERSION);
-    m.update(arguments.as_bytes());
+    for arg in arguments {
+        arg.hash(&mut HashToSha1 { sha: &mut m });
+    }
     //TODO: use lazy_static.
     let cached_env_vars: HashSet<OsString> = CACHED_ENV_VARS.iter().map(|v| OsStr::new(v).to_os_string()).collect();
     for &(ref var, ref val) in env_vars.iter() {
         if cached_env_vars.contains(var) {
-            m.update(os_str_bytes(var));
+            var.hash(&mut HashToSha1 { sha: &mut m });
             m.update(&b"="[..]);
-            m.update(os_str_bytes(val));
+            val.hash(&mut HashToSha1 { sha: &mut m });
         }
     }
     m.update(preprocessor_output);
@@ -294,36 +299,40 @@ mod test {
 
     #[test]
     fn test_hash_key_executable_contents_differs() {
-        let args = "a b c";
+        let args = ovec!["a", "b", "c"];
         const PREPROCESSED : &'static [u8] = b"hello world";
-        assert_neq!(hash_key("abcd", &args, &[], &PREPROCESSED),
-                    hash_key("wxyz", &args, &[], &PREPROCESSED));
+        assert_neq!(hash_key("abcd",&args, &[], &PREPROCESSED),
+                    hash_key("wxyz",&args, &[], &PREPROCESSED));
     }
 
     #[test]
     fn test_hash_key_args_differs() {
         let digest = "abcd";
+        let abc = ovec!["a", "b", "c"];
+        let xyz = ovec!["x", "y", "z"];
+        let ab = ovec!["a", "b"];
+        let a = ovec!["a"];
         const PREPROCESSED: &'static [u8] = b"hello world";
-        assert_neq!(hash_key(digest, "a b c", &[], &PREPROCESSED),
-                    hash_key(digest, "x y z", &[], &PREPROCESSED));
+        assert_neq!(hash_key(digest, &abc, &[], &PREPROCESSED),
+                    hash_key(digest, &xyz, &[], &PREPROCESSED));
 
-        assert_neq!(hash_key(digest, "a b c", &[], &PREPROCESSED),
-                    hash_key(digest, "a b", &[], &PREPROCESSED));
+        assert_neq!(hash_key(digest, &abc, &[], &PREPROCESSED),
+                    hash_key(digest, &ab, &[], &PREPROCESSED));
 
-        assert_neq!(hash_key(digest, "a b c", &[], &PREPROCESSED),
-                    hash_key(digest, "a", &[], &PREPROCESSED));
+        assert_neq!(hash_key(digest, &abc, &[], &PREPROCESSED),
+                    hash_key(digest, &a, &[], &PREPROCESSED));
     }
 
     #[test]
     fn test_hash_key_preprocessed_content_differs() {
-        let args = "a b c";
+        let args = ovec!["a", "b", "c"];
         assert_neq!(hash_key("abcd", &args, &[], &b"hello world"[..]),
                     hash_key("abcd", &args, &[], &b"goodbye"[..]));
     }
 
     #[test]
     fn test_hash_key_env_var_differs() {
-        let args = "a b c";
+        let args = ovec!["a", "b", "c"];
         let digest = "abcd";
         const PREPROCESSED: &'static [u8] = b"hello world";
         for var in CACHED_ENV_VARS.iter() {