Auto merge of #905 - bkchr:rustfmt, r=fitzgen

Adds support for running rustfmt on generated bindings

This patch enables bindgen to run rustfmt on generated bindings. Rustfmt is used
from the global PATH. Two new command-line arguments are added:
1. --format-bindings: Enables running rustfmt
2. --format-configuration-file: The configuration file for rustfmt (not required).

Fixes: #900

Author: bors-servo

Cargo.lock

@@ -51,6 +51,7 @@ syntex_syntax = "0.58"
 regex = "0.2"
 # This kinda sucks: https://github.com/rust-lang/cargo/issues/1982
 clap = "2"
+which = "1.0.2"
 
 [dependencies.aster]
 features = ["with-syntex"]

Cargo.toml

@@ -175,7 +175,7 @@ impl<'a> CodegenResult<'a> {
     /// counter internally so the next time we ask for the overload for this
     /// name, we get the incremented value, and so on.
     fn overload_number(&mut self, name: &str) -> u32 {
-        let mut counter =
+        let counter =
             self.overload_counters.entry(name.into()).or_insert(0);
         let number = *counter;
         *counter += 1;
@@ -2030,7 +2030,7 @@ impl MethodCodegen for Method {
         }
 
         let count = {
-            let mut count = method_names.entry(name.clone()).or_insert(0);
+            let count = method_names.entry(name.clone()).or_insert(0);
             *count += 1;
             *count - 1
         };

src/codegen/mod.rs

@@ -460,7 +460,7 @@ fn raw_fields_to_fields_and_bitfield_units<I>(ctx: &BindgenContext,
 /// (potentially multiple) bitfield units.
 fn bitfields_to_allocation_units<E, I>(ctx: &BindgenContext,
                                        bitfield_unit_count: &mut usize,
-                                       mut fields: &mut E,
+                                       fields: &mut E,
                                        raw_bitfields: I)
     where E: Extend<Field>,
           I: IntoIterator<Item=RawField>
@@ -478,7 +478,7 @@ fn bitfields_to_allocation_units<E, I>(ctx: &BindgenContext,
     // TODO(emilio): Take into account C++'s wide bitfields, and
     // packing, sigh.
 
-    fn flush_allocation_unit<E>(mut fields: &mut E,
+    fn flush_allocation_unit<E>(fields: &mut E,
                                 bitfield_unit_count: &mut usize,
                                 unit_size_in_bits: usize,
                                 unit_align_in_bits: usize,

src/ir/comp.rs

@@ -466,8 +466,8 @@ impl<'ctx> BindgenContext<'ctx> {
         assert!(item.id() != self.root_module);
         assert!(!self.items.contains_key(&item.id()));
 
-        if let Some(mut parent) = self.items.get_mut(&item.parent_id()) {
-            if let Some(mut module) = parent.as_module_mut() {
+        if let Some(parent) = self.items.get_mut(&item.parent_id()) {
+            if let Some(module) = parent.as_module_mut() {
                 debug!("add_item_to_module: adding {:?} as child of parent module {:?}",
                        item.id(),
                        item.parent_id());
@@ -607,7 +607,7 @@ impl<'ctx> BindgenContext<'ctx> {
                                to opaque blob");
                         Item::new_opaque_type(self.next_item_id(), &ty, self)
                     });
-                let mut item = self.items.get_mut(&id).unwrap();
+                let item = self.items.get_mut(&id).unwrap();
 
                 *item.kind_mut().as_type_mut().unwrap().kind_mut() =
                     TypeKind::ResolvedTypeRef(resolved);
@@ -699,7 +699,7 @@ impl<'ctx> BindgenContext<'ctx> {
             debug!("Replacing {:?} with {:?}", id, replacement);
 
             let new_parent = {
-                let mut item = self.items.get_mut(&id).unwrap();
+                let item = self.items.get_mut(&id).unwrap();
                 *item.kind_mut().as_type_mut().unwrap().kind_mut() =
                     TypeKind::ResolvedTypeRef(replacement);
                 item.parent_id()

src/ir/context.rs

@@ -322,7 +322,7 @@ impl IsOpaque for TemplateInstantiation {
                 arg_path[1..].join("::")
             }).collect();
         {
-            let mut last = path.last_mut().unwrap();
+            let last = path.last_mut().unwrap();
             last.push('<');
             last.push_str(&args.join(", "));
             last.push('>');

src/ir/template.rs

@@ -26,6 +26,7 @@ extern crate peeking_take_while;
 extern crate regex;
 #[macro_use]
 extern crate lazy_static;
+extern crate which;
 
 #[cfg(feature = "logging")]
 #[macro_use]
@@ -455,6 +456,19 @@ impl Builder {
             );
         }
 
+        if !self.options.rustfmt_bindings {
+            output_vector.push("--rustfmt-bindings".into());
+        }
+
+        if let Some(path) = self.options
+            .rustfmt_configuration_file
+            .as_ref()
+            .and_then(|f| f.to_str())
+        {
+            output_vector.push("--rustfmt-configuration-file".into());
+            output_vector.push(path.into());
+        }
+
         output_vector
     }
 
@@ -840,6 +854,20 @@ impl Builder {
         self
     }
 
+    /// Set whether rustfmt should format the generated bindings.
+    pub fn rustfmt_bindings(mut self, doit: bool) -> Self {
+        self.options.rustfmt_bindings = doit;
+        self
+    }
+
+    /// Set the absolute path to the rustfmt configuration file, if None, the standard rustfmt
+    /// options are used.
+    pub fn rustfmt_configuration_file(mut self, path: Option<PathBuf>) -> Self {
+        self = self.rustfmt_bindings(true);
+        self.options.rustfmt_configuration_file = path;
+        self
+    }
+
     /// Generate the Rust bindings using the options built up thus far.
     pub fn generate<'ctx>(mut self) -> Result<Bindings<'ctx>, ()> {
         self.options.input_header = self.input_headers.pop();
@@ -1099,6 +1127,13 @@ pub struct BindgenOptions {
 
     /// Features to enable, derived from `rust_target`
     rust_features: RustFeatures,
+
+    /// Whether rustfmt should format the generated bindings.
+    pub rustfmt_bindings: bool,
+
+    /// The absolute path to the rustfmt configuration file, if None, the standard rustfmt
+    /// options are used.
+    pub rustfmt_configuration_file: Option<PathBuf>,
 }
 
 /// TODO(emilio): This is sort of a lie (see the error message that results from
@@ -1183,6 +1218,8 @@ impl Default for BindgenOptions {
             objc_extern_crate: false,
             enable_mangling: true,
             prepend_enum_name: true,
+            rustfmt_bindings: false,
+            rustfmt_configuration_file: None,
         }
     }
 }
@@ -1334,14 +1371,18 @@ impl<'ctx> Bindings<'ctx> {
 
     /// Write these bindings as source text to a file.
     pub fn write_to_file<P: AsRef<Path>>(&self, path: P) -> io::Result<()> {
-        let file = try!(
-            OpenOptions::new()
-                .write(true)
-                .truncate(true)
-                .create(true)
-                .open(path)
-        );
-        self.write(Box::new(file))
+        {
+            let file = try!(
+                OpenOptions::new()
+                    .write(true)
+                    .truncate(true)
+                    .create(true)
+                    .open(path.as_ref())
+            );
+            self.write(Box::new(file))?;
+        }
+
+        self.rustfmt_generated_file(path.as_ref())
     }
 
     /// Write these bindings as source text to the given `Write`able.
@@ -1364,6 +1405,63 @@ impl<'ctx> Bindings<'ctx> {
         try!(eof(&mut ps.s));
         ps.s.out.flush()
     }
+
+    /// Checks if rustfmt_bindings is set and runs rustfmt on the file
+    fn rustfmt_generated_file(&self, file: &Path) -> io::Result<()> {
+        if !self.context.options().rustfmt_bindings {
+            return Ok(());
+        }
+
+        let rustfmt = if let Ok(rustfmt) = which::which("rustfmt") {
+            rustfmt
+        } else {
+            return Err(io::Error::new(
+                io::ErrorKind::Other,
+                "Rustfmt activated, but it could not be found in global path.",
+            ));
+        };
+
+        let mut cmd = Command::new(rustfmt);
+
+        if let Some(path) = self.context
+            .options()
+            .rustfmt_configuration_file
+            .as_ref()
+            .and_then(|f| f.to_str())
+        {
+            cmd.args(&["--config-path", path]);
+        }
+
+        if let Ok(output) = cmd.arg(file).output() {
+            if !output.status.success() {
+                let stderr = String::from_utf8_lossy(&output.stderr);
+                match output.status.code() {
+                    Some(2) => Err(io::Error::new(
+                        io::ErrorKind::Other,
+                        format!("Rustfmt parsing errors:\n{}", stderr),
+                    )),
+                    Some(3) => {
+                        warn!(
+                            "Rustfmt could not format some lines:\n{}",
+                            stderr
+                        );
+                        Ok(())
+                    }
+                    _ => Err(io::Error::new(
+                        io::ErrorKind::Other,
+                        format!("Internal rustfmt error:\n{}", stderr),
+                    )),
+                }
+            } else {
+                Ok(())
+            }
+        } else {
+            Err(io::Error::new(
+                io::ErrorKind::Other,
+                "Error executing rustfmt!",
+            ))
+        }
+    }
 }
 
 /// Determines whether the given cursor is in any of the files matched by the

src/lib.rs

@@ -4,6 +4,7 @@ use clap::{App, Arg};
 use std::fs::File;
 use std::io::{self, Error, ErrorKind, Write, stderr};
 use std::str::FromStr;
+use std::path::PathBuf;
 
 /// Construct a new [`Builder`](./struct.Builder.html) from command line flags.
 pub fn builder_from_flags<I>(
@@ -231,7 +232,20 @@ where
                 .help("Preprocess and dump the input header files to disk. \
                        Useful when debugging bindgen, using C-Reduce, or when \
                        filing issues. The resulting file will be named \
-                       something like `__bindgen.i` or `__bindgen.ii`.")
+                       something like `__bindgen.i` or `__bindgen.ii`."),
+            Arg::with_name("rustfmt-bindings")
+                .long("rustfmt-bindings")
+                .help("Format the generated bindings with rustfmt. \
+                       Rustfmt needs to be in the global PATH."),
+            Arg::with_name("rustfmt-configuration-file")
+                .long("rustfmt-configuration-file")
+                .help("The absolute path to the rustfmt configuration file. \
+                       The configuration file will be used for formatting the bindings. \
+                       Setting this parameter, will automatically set --rustfmt-bindings.")
+                .value_name("path")
+                .takes_value(true)
+                .multiple(false)
+                .number_of_values(1),
         ]) // .args()
         .get_matches_from(args);
 
@@ -458,6 +472,27 @@ where
         builder.dump_preprocessed_input()?;
     }
 
+    if matches.is_present("rustfmt-bindings") {
+        builder = builder.rustfmt_bindings(true);
+    }
+
+    if let Some(path_str) = matches.value_of("rustfmt-configuration-file") {
+        let path = PathBuf::from(path_str);
+
+        if !path.is_absolute() {
+            return Err(Error::new(ErrorKind::Other,
+                                  "--rustfmt-configuration--file needs to be an absolute path!"));
+        }
+
+        if path.to_str().is_none() {
+            return Err(
+                Error::new(ErrorKind::Other,
+                           "--rustfmt-configuration-file contains non-valid UTF8 characters."));
+        }
+
+        builder = builder.rustfmt_configuration_file(Some(path));
+    } 
+
     let verbose = matches.is_present("verbose");
 
     Ok((builder, output, verbose))