Pre-Rust support refactoring, Part 3

The goal here was to make more of the state that's currently persisted
from `parse_arguments` -> `generate_hash_key` -> `compile` private so
the C compilers and the Rust compiler can store different kinds of state.

* Split the `Compiler` trait further into a `CompilerHasher` trait,
  which now gets returned in a Box from `Compiler::parse_arguments`
  as a field of `CompilerArguments`.
* Move the existing `ParsedArguments` struct into compiler/c to make it
  specific to C compilers.

src/compiler/c.rs

@@ -12,12 +12,15 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use compiler::{Cacheable, Compiler, CompilerArguments, CompilerKind, Compilation, HashResult, ParsedArguments};
+use compiler::{Cacheable, Compiler, CompilerArguments, CompilerHasher, CompilerKind, Compilation, HashResult};
 use futures::Future;
 use futures_cpupool::CpuPool;
 use mock_command::CommandCreatorSync;
 use sha1;
+use std::borrow::Cow;
+use std::collections::HashMap;
 use std::env;
+use std::fmt;
 use std::path::Path;
 use std::process;
 
@@ -27,21 +30,53 @@ use errors::*;
 #[derive(Clone)]
 pub struct CCompiler<I: CCompilerImpl>(pub I);
 
+/// A generic implementation of the `CompilerHasher` trait for C/C++ compilers.
+#[derive(Debug, Clone)]
+pub struct CCompilerHasher<I: CCompilerImpl> {
+    parsed_args: ParsedArguments,
+    compiler: I,
+}
+
+/// The results of parsing a compiler commandline.
+#[allow(dead_code)]
+#[derive(Debug, PartialEq, Clone)]
+pub struct ParsedArguments {
+    /// The input source file.
+    pub input: String,
+    /// The file extension of the input source file.
+    pub extension: String,
+    /// The file in which to generate dependencies.
+    pub depfile: Option<String>,
+    /// Output files, keyed by a simple name, like "obj".
+    pub outputs: HashMap<&'static str, String>,
+    /// Commandline arguments for the preprocessor.
+    pub preprocessor_args: Vec<String>,
+    /// Commandline arguments for the preprocessor or the compiler.
+    pub common_args: Vec<String>,
+}
+
+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"))
+    }
+}
+
 /// A generic implementation of the `Compilation` trait for C/C++ compilers.
 struct CCompilation<I: CCompilerImpl> {
+    parsed_args: ParsedArguments,
     /// The output from running the preprocessor.
     preprocessor_output: Vec<u8>,
     compiler: I,
 }
 
 /// An interface to a specific C compiler.
-pub trait CCompilerImpl: Clone + Send + 'static {
+pub trait CCompilerImpl: Clone + fmt::Debug + Send + 'static {
     /// Return the kind of compiler.
     fn kind(&self) -> CompilerKind;
     /// Determine whether `arguments` are supported by this compiler.
     fn parse_arguments(&self,
                        arguments: &[String],
-                       cwd: &Path) -> CompilerArguments;
+                       cwd: &Path) -> CompilerArguments<ParsedArguments>;
     /// Run the C preprocessor with the specified set of arguments.
     fn preprocess<T>(&self,
                      creator: &T,
@@ -67,28 +102,45 @@ impl<T: CommandCreatorSync, I: CCompilerImpl> Compiler<T> for CCompiler<I> {
     fn kind(&self) -> CompilerKind { self.0.kind() }
     fn parse_arguments(&self,
                        arguments: &[String],
-                       cwd: &Path) -> CompilerArguments {
-        self.0.parse_arguments(arguments, cwd)
+                       cwd: &Path) -> CompilerArguments<Box<CompilerHasher<T> + 'static>> {
+        match self.0.parse_arguments(arguments, cwd) {
+            CompilerArguments::Ok(args) => {
+                CompilerArguments::Ok(Box::new(CCompilerHasher {
+                    parsed_args: args,
+                    compiler: self.0.clone(),
+                }))
+            }
+            CompilerArguments::CannotCache => CompilerArguments::CannotCache,
+            CompilerArguments::NotCompilation => CompilerArguments::NotCompilation,
+        }
     }
 
+    fn box_clone(&self) -> Box<Compiler<T>> {
+        Box::new((*self).clone())
+    }
+}
+
+impl<T, I> CompilerHasher<T> for CCompilerHasher<I>
+    where T: CommandCreatorSync,
+          I: CCompilerImpl,
+{
     fn generate_hash_key(&self,
                          creator: &T,
                          executable: &str,
                          executable_digest: &str,
-                         parsed_args: &ParsedArguments,
                          cwd: &str,
                          pool: &CpuPool)
                          -> SFuture<HashResult<T>>
     {
-        let result = self.0.preprocess(creator, executable, parsed_args, cwd, pool);
-        let parsed_args = parsed_args.clone();
+        let result = self.compiler.preprocess(creator, executable, &self.parsed_args, cwd, pool);
+        let parsed_args = self.parsed_args.clone();
         let out_file = parsed_args.output_file().into_owned();
         let result = result.map_err(move |e| {
             debug!("[{}]: preprocessor failed: {:?}", out_file, e);
             e
         });
         let executable_digest = executable_digest.to_string();
-        let compiler = self.0.clone();
+        let compiler = self.compiler.clone();
 
         Box::new(result.map(move |preprocessor_result| {
             // If the preprocessor failed, just return that result.
@@ -119,13 +171,26 @@ impl<T: CommandCreatorSync, I: CCompilerImpl> Compiler<T> for CCompiler<I> {
             HashResult::Ok {
                 key: key,
                 compilation: Box::new(CCompilation {
+                    parsed_args: parsed_args,
                     preprocessor_output: preprocessor_result.stdout,
                     compiler: compiler,
                 }),
             }
         }))
     }
-    fn box_clone(&self) -> Box<Compiler<T>> {
+
+    fn output_file(&self) -> Cow<str>
+    {
+        self.parsed_args.output_file()
+    }
+
+    fn outputs<'a>(&'a self) -> Box<Iterator<Item=(&'a &'static str, &'a String)> + 'a>
+    {
+        Box::new(self.parsed_args.outputs.iter())
+    }
+
+    fn box_clone(&self) -> Box<CompilerHasher<T>>
+    {
         Box::new((*self).clone())
     }
 }
@@ -134,14 +199,13 @@ impl<T: CommandCreatorSync, I: CCompilerImpl> Compilation<T> for CCompilation<I>
     fn compile(self: Box<Self>,
                creator: &T,
                executable: &str,
-               parsed_args: &ParsedArguments,
                cwd: &str,
                pool: &CpuPool)
                -> SFuture<(Cacheable, process::Output)>
     {
         let me = *self;
-        let CCompilation { preprocessor_output, compiler } = me;
-        compiler.compile(creator, executable, preprocessor_output, parsed_args, cwd, pool)
+        let CCompilation { parsed_args, preprocessor_output, compiler } = me;
+        compiler.compile(creator, executable, preprocessor_output, &parsed_args, cwd, pool)
     }
 }
 

src/compiler/clang.rs

@@ -19,11 +19,10 @@ use ::compiler::{
     Cacheable,
     CompilerArguments,
     CompilerKind,
-    ParsedArguments,
     run_input_output,
     write_temp_file,
 };
-use compiler::c::CCompilerImpl;
+use compiler::c::{CCompilerImpl, ParsedArguments};
 use futures::future::{self, Future};
 use futures_cpupool::CpuPool;
 use mock_command::{
@@ -42,14 +41,14 @@ use std::process;
 use errors::*;
 
 /// A unit struct on which to implement `CCompilerImpl`.
-#[derive(Clone)]
+#[derive(Clone, Debug)]
 pub struct Clang;
 
 impl CCompilerImpl for Clang {
     fn kind(&self) -> CompilerKind { CompilerKind::Clang }
     fn parse_arguments(&self,
                        arguments: &[String],
-                       cwd: &Path) -> CompilerArguments
+                       cwd: &Path) -> CompilerArguments<ParsedArguments>
     {
         gcc::parse_arguments(arguments, cwd, argument_takes_value)
     }
@@ -166,7 +165,7 @@ mod test {
     use super::*;
     use test::utils::*;
 
-    fn _parse_arguments(arguments: &[String]) -> CompilerArguments {
+    fn _parse_arguments(arguments: &[String]) -> CompilerArguments<ParsedArguments> {
         gcc::parse_arguments(arguments, ".".as_ref(), argument_takes_value)
     }
 

src/compiler/compiler.rs

@@ -70,25 +70,16 @@ pub enum CompilerKind {
     MSVC,
 }
 
-/// An interface to a compiler.
-pub trait Compiler<T: CommandCreatorSync>: Send + 'static {
+/// An interface to a compiler for argument parsing.
+pub trait Compiler<T>: Send + 'static
+    where T: CommandCreatorSync,
+{
     /// Return the kind of compiler.
     fn kind(&self) -> CompilerKind;
     /// Determine whether `arguments` are supported by this compiler.
     fn parse_arguments(&self,
                        arguments: &[String],
-                       cwd: &Path) -> CompilerArguments;
-    /// Given information about a compiler command, generate a hash key
-    /// that can be used for cache lookups, as well as any additional
-    /// information that can be reused for compilation if necessary.
-    fn generate_hash_key(&self,
-                         creator: &T,
-                         executable: &str,
-                         executable_digest: &str,
-                         parsed_args: &ParsedArguments,
-                         cwd: &str,
-                         pool: &CpuPool)
-                         -> SFuture<HashResult<T>>;
+                       cwd: &Path) -> CompilerArguments<Box<CompilerHasher<T> + 'static>>;
     fn box_clone(&self) -> Box<Compiler<T>>;
 }
 
@@ -96,12 +87,39 @@ impl<T: CommandCreatorSync> Clone for Box<Compiler<T>> {
     fn clone(&self) -> Box<Compiler<T>> { self.box_clone() }
 }
 
-pub trait Compilation<T: CommandCreatorSync> {
+/// An interface to a compiler for hash key generation, the result of
+/// argument parsing.
+pub trait CompilerHasher<T>: fmt::Debug + Send + 'static
+    where T: CommandCreatorSync,
+{
+    /// Given information about a compiler command, generate a hash key
+    /// that can be used for cache lookups, as well as any additional
+    /// information that can be reused for compilation if necessary.
+    fn generate_hash_key(&self,
+                         creator: &T,
+                         executable: &str,
+                         executable_digest: &str,
+                         cwd: &str,
+                         pool: &CpuPool)
+                         -> SFuture<HashResult<T>>;
+    /// Get the output file of the compilation.
+    fn output_file(&self) -> Cow<str>;
+    fn outputs<'a>(&'a self) -> Box<Iterator<Item=(&'a &'static str, &'a String)> + 'a>;
+    fn box_clone(&self) -> Box<CompilerHasher<T>>;
+}
+
+impl<T: CommandCreatorSync> Clone for Box<CompilerHasher<T>> {
+    fn clone(&self) -> Box<CompilerHasher<T>> { self.box_clone() }
+}
+
+/// An interface to a compiler for actually invoking compilation.
+pub trait Compilation<T>
+    where T: CommandCreatorSync,
+{
     /// Given information about a compiler command, execute the compiler.
     fn compile(self: Box<Self>,
                creator: &T,
                executable: &str,
-               parsed_args: &ParsedArguments,
                cwd: &str,
                pool: &CpuPool)
                -> SFuture<(Cacheable, process::Output)>;
@@ -123,35 +141,12 @@ pub enum HashResult<T: CommandCreatorSync> {
     },
 }
 
-/// The results of parsing a compiler commandline.
-#[allow(dead_code)]
-#[derive(Debug, PartialEq, Clone)]
-pub struct ParsedArguments {
-    /// The input source file.
-    pub input: String,
-    /// The file extension of the input source file.
-    pub extension: String,
-    /// The file in which to generate dependencies.
-    pub depfile: Option<String>,
-    /// Output files, keyed by a simple name, like "obj".
-    pub outputs: HashMap<&'static str, String>,
-    /// Commandline arguments for the preprocessor.
-    pub preprocessor_args: Vec<String>,
-    /// Commandline arguments for the preprocessor or the compiler.
-    pub common_args: Vec<String>,
-}
-
-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"))
-    }
-}
-
 /// Possible results of parsing compiler arguments.
 #[derive(Debug, PartialEq)]
-pub enum CompilerArguments {
+pub enum CompilerArguments<T>
+{
     /// Commandline can be handled.
-    Ok(ParsedArguments),
+    Ok(T),
     /// Cannot cache this compilation.
     CannotCache,
     /// This commandline is not a compile.
@@ -309,7 +304,7 @@ impl<T: CommandCreatorSync> CompilerInfo<T> {
     /// options for each compiler.
     pub fn parse_arguments(&self,
                            arguments: &[String],
-                           cwd: &Path) -> CompilerArguments {
+                           cwd: &Path) -> CompilerArguments<Box<CompilerHasher<T> + 'static>> {
         if log_enabled!(Debug) {
             let cmd_str = arguments.join(" ");
             debug!("parse_arguments: `{}`", cmd_str);
@@ -323,19 +318,19 @@ impl<T: CommandCreatorSync> CompilerInfo<T> {
                                  creator: T,
                                  storage: Arc<Storage>,
                                  arguments: Vec<String>,
-                                 parsed_args: ParsedArguments,
+                                 hasher: Box<CompilerHasher<T> + 'static>,
                                  cwd: String,
                                  cache_control: CacheControl,
                                  pool: CpuPool)
                                  -> SFuture<(CompileResult, process::Output)>
     {
-        let CompilerInfo { executable, digest, compiler, .. } = self;
-        let out_file = parsed_args.output_file().into_owned();
+        let CompilerInfo { executable, digest, .. } = self;
+        let out_file = hasher.output_file().into_owned();
         if log_enabled!(Debug) {
             let cmd_str = arguments.join(" ");
             debug!("[{}]: get_cached_or_compile: {}", out_file, cmd_str);
         }
-        let result = compiler.generate_hash_key(&creator, &executable, &digest, &parsed_args, &cwd, &pool);
+        let result = hasher.generate_hash_key(&creator, &executable, &digest, &cwd, &pool);
         Box::new(result.and_then(move |hash_res| -> SFuture<_> {
             let (key, compilation) = match hash_res {
                 HashResult::Error { output } => {
@@ -343,7 +338,7 @@ impl<T: CommandCreatorSync> CompilerInfo<T> {
                 }
                 HashResult::Ok { key, compilation } => (key, compilation),
             };
-            trace!("[{}]: Hash key: {}", parsed_args.output_file(), key);
+            trace!("[{}]: Hash key: {}", hasher.output_file(), key);
             // If `ForceRecache` is enabled, we won't check the cache.
             let start = Instant::now();
             let cache_status = if cache_control == CacheControl::ForceRecache {
@@ -356,13 +351,13 @@ impl<T: CommandCreatorSync> CompilerInfo<T> {
             Box::new(cache_status.and_then(move |result| {
                 let duration = start.elapsed();
                 let pwd = Path::new(&cwd);
-                let outputs = parsed_args.outputs.iter()
+                let outputs = hasher.outputs()
                     .map(|(key, path)| (key.to_string(), pwd.join(path)))
                     .collect::<HashMap<_, _>>();
 
                 let miss_type = match result {
                     Cache::Hit(mut entry) => {
-                        debug!("[{}]: Cache hit!", parsed_args.output_file());
+                        debug!("[{}]: Cache hit!", hasher.output_file());
                         let mut stdout = io::Cursor::new(vec!());
                         let mut stderr = io::Cursor::new(vec!());
                         drop(entry.get_object("stdout", &mut stdout));
@@ -388,35 +383,34 @@ impl<T: CommandCreatorSync> CompilerInfo<T> {
                         })) as SFuture<_>
                     }
                     Cache::Miss => {
-                        debug!("[{}]: Cache miss!", parsed_args.output_file());
+                        debug!("[{}]: Cache miss!", hasher.output_file());
                         MissType::Normal
                     }
                     Cache::Recache => {
-                        debug!("[{}]: Cache recache!", parsed_args.output_file());
+                        debug!("[{}]: Cache recache!", hasher.output_file());
                         MissType::ForcedRecache
                     }
                 };
 
                 // Cache miss, so compile it.
                 let start = Instant::now();
-                let out_file = parsed_args.output_file().into_owned();
-
-                let compile = compilation.compile(&creator, &executable, &parsed_args, &cwd, &pool);
+                let out_file = hasher.output_file().into_owned();
+                let compile = compilation.compile(&creator, &executable, &cwd, &pool);
                 Box::new(compile.and_then(move |(cacheable, compiler_result)| {
                     let duration = start.elapsed();
                     if !compiler_result.status.success() {
                         debug!("[{}]: Compiled but failed, not storing in cache",
-                               parsed_args.output_file());
+                               hasher.output_file());
                         return Box::new(future::ok((CompileResult::CompileFailed, compiler_result)))
                             as SFuture<_>
                     }
                     if cacheable != Cacheable::Yes {
                         // Not cacheable
                         debug!("[{}]: Compiled but not cacheable",
-                               parsed_args.output_file());
+                               hasher.output_file());
                         return Box::new(future::ok((CompileResult::NotCacheable, compiler_result)))
                     }
-                    debug!("[{}]: Compiled, storing in cache", parsed_args.output_file());
+                    debug!("[{}]: Compiled, storing in cache", hasher.output_file());
                     let mut entry = match storage.start_put(&key) {
                         Ok(entry) => entry,
                         Err(e) => return Box::new(future::err(e))
@@ -444,7 +438,7 @@ impl<T: CommandCreatorSync> CompilerInfo<T> {
 
                         // Try to finish storing the newly-written cache
                         // entry. We'll get the result back elsewhere.
-                        let out_file = parsed_args.output_file().into_owned();
+                        let out_file = hasher.output_file().into_owned();
                         let future = storage.finish_put(&key, entry)
                             .then(move |res| {
                                 match res {

src/compiler/gcc.rs

@@ -16,10 +16,9 @@ use ::compiler::{
     Cacheable,
     CompilerArguments,
     CompilerKind,
-    ParsedArguments,
     run_input_output,
 };
-use compiler::c::CCompilerImpl;
+use compiler::c::{CCompilerImpl, ParsedArguments};
 use log::LogLevel::Trace;
 use futures::future::{self, Future};
 use futures_cpupool::CpuPool;
@@ -36,14 +35,14 @@ use std::process;
 use errors::*;
 
 /// A unit struct on which to implement `CCompilerImpl`.
-#[derive(Clone)]
+#[derive(Clone, Debug)]
 pub struct GCC;
 
 impl CCompilerImpl for GCC {
     fn kind(&self) -> CompilerKind { CompilerKind::GCC }
     fn parse_arguments(&self,
                        arguments: &[String],
-                       cwd: &Path) -> CompilerArguments
+                       cwd: &Path) -> CompilerArguments<ParsedArguments>
     {
         parse_arguments(arguments, cwd, argument_takes_value)
     }
@@ -105,13 +104,13 @@ pub fn argument_takes_value(arg: &str) -> bool {
 pub fn parse_arguments<F: Fn(&str) -> bool>(arguments: &[String],
                                             cwd: &Path,
                                             argument_takes_value: F)
-                                            -> CompilerArguments {
+                                            -> CompilerArguments<ParsedArguments> {
     _parse_arguments(arguments, cwd, &argument_takes_value)
 }
 
 fn _parse_arguments(arguments: &[String],
                     cwd: &Path,
-                    argument_takes_value: &Fn(&str) -> bool) -> CompilerArguments {
+                    argument_takes_value: &Fn(&str) -> bool) -> CompilerArguments<ParsedArguments> {
     let mut output_arg = None;
     let mut input_arg = None;
     let mut dep_target = None;
@@ -359,7 +358,7 @@ mod test {
     use ::compiler::*;
     use tempdir::TempDir;
 
-    fn _parse_arguments(arguments: &[String]) -> CompilerArguments {
+    fn _parse_arguments(arguments: &[String]) -> CompilerArguments<ParsedArguments> {
         parse_arguments(arguments, ".".as_ref(), argument_takes_value)
     }
 

src/compiler/msvc.rs

@@ -16,11 +16,10 @@ use ::compiler::{
     Cacheable,
     CompilerArguments,
     CompilerKind,
-    ParsedArguments,
     run_input_output,
     write_temp_file,
 };
-use compiler::c::CCompilerImpl;
+use compiler::c::{CCompilerImpl, ParsedArguments};
 use local_encoding::{Encoding, Encoder};
 use log::LogLevel::{Debug, Trace};
 use futures::future::{self, Future};
@@ -54,7 +53,7 @@ impl CCompilerImpl for MSVC {
     fn kind(&self) -> CompilerKind { CompilerKind::MSVC }
     fn parse_arguments(&self,
                        arguments: &[String],
-                       _cwd: &Path) -> CompilerArguments
+                       _cwd: &Path) -> CompilerArguments<ParsedArguments>
     {
         parse_arguments(arguments)
     }
@@ -144,7 +143,7 @@ pub fn detect_showincludes_prefix<T>(creator: &T, exe: &OsStr, pool: &CpuPool)
     }))
 }
 
-pub fn parse_arguments(arguments: &[String]) -> CompilerArguments {
+pub fn parse_arguments(arguments: &[String]) -> CompilerArguments<ParsedArguments> {
     let mut output_arg = None;
     let mut input_arg = None;
     let mut common_args = vec!();

src/server.rs

@@ -19,10 +19,10 @@ use cache::{
 use compiler::{
     CacheControl,
     CompilerArguments,
+    CompilerHasher,
     CompilerInfo,
     CompileResult,
     MissType,
-    ParsedArguments,
     get_compiler_info,
 };
 use filetime::FileTime;
@@ -526,12 +526,12 @@ impl<C> SccacheService<C>
         Message::WithoutBody(res)
     }
 
-    /// Given compiler arguments `arguments` + `parsed_arguments`, look up
+    /// Given compiler arguments `arguments`, look up
     /// a compile result in the cache or execute the compilation and store
     /// the result in the cache.
     fn start_compile_task(&self,
                           compiler: CompilerInfo<C>,
-                          parsed_arguments: ParsedArguments,
+                          hasher: Box<CompilerHasher<C>>,
                           arguments: Vec<String>,
                           cwd: String,
                           tx: mpsc::Sender<Result<ServerResponse>>) {
@@ -540,10 +540,11 @@ impl<C> SccacheService<C>
         } else {
             CacheControl::Default
         };
+        let output = hasher.output_file().into_owned();
         let result = compiler.get_cached_or_compile(self.creator.clone(),
                                                     self.storage.clone(),
                                                     arguments,
-                                                    parsed_arguments,
+                                                    hasher,
                                                     cwd,
                                                     cache_control,
                                                     self.pool.clone());
@@ -596,9 +597,9 @@ impl<C> SccacheService<C>
                 Err(err) => {
                     debug!("[{:?}] compilation failed: {:?}",
                            err,
-                           parsed_arguments.output_file());
+                           output);
                     for e in err.iter() {
-                        error!("[{:?}] \t{}", e, parsed_arguments.output_file());
+                        error!("[{:?}] \t{}", e, output);
                     }
                     stats.cache_errors += 1;
                     //TODO: figure out a better way to communicate this?