Move newline logic inside the formatting process.

Why?:
 - Conceptually it sounds right
 - Absolutely all write modes where doing it anyway
 - It was done several times in some in case
 - It greatly simplify the code

Author: t-botz

src/filemap.rs

@@ -10,13 +10,12 @@
 
 // TODO: add tests
 
-use std::fs::{self, File};
-use std::io::{self, BufWriter, Read, Write};
-use std::path::Path;
+use std::fs;
+use std::io::{self, Write};
 
 use checkstyle::output_checkstyle_file;
-use config::{Config, EmitMode, FileName, NewlineStyle, Verbosity};
-use rustfmt_diff::{make_diff, output_modified, print_diff, Mismatch};
+use config::{Config, EmitMode, FileName, Verbosity};
+use rustfmt_diff::{make_diff, output_modified, print_diff};
 
 #[cfg(test)]
 use FileRecord;
@@ -48,72 +47,15 @@ where
     Ok(())
 }
 
-// Prints all newlines either as `\n` or as `\r\n`.
-pub fn write_system_newlines<T>(writer: T, text: &str, config: &Config) -> Result<(), io::Error>
-where
-    T: Write,
-{
-    // Buffer output, since we're writing a since char at a time.
-    let mut writer = BufWriter::new(writer);
-
-    let style = if config.newline_style() == NewlineStyle::Native {
-        if cfg!(windows) {
-            NewlineStyle::Windows
-        } else {
-            NewlineStyle::Unix
-        }
-    } else {
-        config.newline_style()
-    };
-
-    match style {
-        NewlineStyle::Unix => write!(writer, "{}", text),
-        NewlineStyle::Windows => {
-            for c in text.chars() {
-                match c {
-                    '\n' => write!(writer, "\r\n")?,
-                    '\r' => continue,
-                    c => write!(writer, "{}", c)?,
-                }
-            }
-            Ok(())
-        }
-        NewlineStyle::Native => unreachable!(),
-    }
-}
-
 pub fn write_file<T>(
-    text: &str,
+    formatted_text: &str,
     filename: &FileName,
     out: &mut T,
     config: &Config,
 ) -> Result<bool, io::Error>
 where
     T: Write,
 {
-    fn source_and_formatted_text(
-        text: &str,
-        filename: &Path,
-        config: &Config,
-    ) -> Result<(String, String), io::Error> {
-        let mut f = File::open(filename)?;
-        let mut ori_text = String::new();
-        f.read_to_string(&mut ori_text)?;
-        let mut v = Vec::new();
-        write_system_newlines(&mut v, text, config)?;
-        let fmt_text = String::from_utf8(v).unwrap();
-        Ok((ori_text, fmt_text))
-    }
-
-    fn create_diff(
-        filename: &Path,
-        text: &str,
-        config: &Config,
-    ) -> Result<Vec<Mismatch>, io::Error> {
-        let (ori, fmt) = source_and_formatted_text(text, filename, config)?;
-        Ok(make_diff(&ori, &fmt, 3))
-    }
-
     let filename_to_path = || match *filename {
         FileName::Real(ref path) => path,
         _ => panic!("cannot format `{}` and emit to files", filename),
@@ -122,65 +64,58 @@ where
     match config.emit_mode() {
         EmitMode::Files if config.make_backup() => {
             let filename = filename_to_path();
-            if let Ok((ori, fmt)) = source_and_formatted_text(text, filename, config) {
-                if fmt != ori {
-                    // Do a little dance to make writing safer - write to a temp file
-                    // rename the original to a .bk, then rename the temp file to the
-                    // original.
-                    let tmp_name = filename.with_extension("tmp");
-                    let bk_name = filename.with_extension("bk");
-                    {
-                        // Write text to temp file
-                        let tmp_file = File::create(&tmp_name)?;
-                        write_system_newlines(tmp_file, text, config)?;
-                    }
-
-                    fs::rename(filename, bk_name)?;
-                    fs::rename(tmp_name, filename)?;
-                }
+            let ori = fs::read_to_string(filename)?;
+            if ori != formatted_text {
+                // Do a little dance to make writing safer - write to a temp file
+                // rename the original to a .bk, then rename the temp file to the
+                // original.
+                let tmp_name = filename.with_extension("tmp");
+                let bk_name = filename.with_extension("bk");
+
+                fs::write(&tmp_name, formatted_text)?;
+                fs::rename(filename, bk_name)?;
+                fs::rename(tmp_name, filename)?;
             }
         }
         EmitMode::Files => {
             // Write text directly over original file if there is a diff.
             let filename = filename_to_path();
-            let (source, formatted) = source_and_formatted_text(text, filename, config)?;
-            if source != formatted {
-                let file = File::create(filename)?;
-                write_system_newlines(file, text, config)?;
+            let ori = fs::read_to_string(filename)?;
+            if ori != formatted_text {
+                fs::write(filename, formatted_text)?;
             }
         }
         EmitMode::Stdout | EmitMode::Coverage => {
             if config.verbose() != Verbosity::Quiet {
                 println!("{}:\n", filename);
             }
-            write_system_newlines(out, text, config)?;
+            write!(out, "{}", formatted_text)?;
         }
         EmitMode::ModifiedLines => {
             let filename = filename_to_path();
-            if let Ok((ori, fmt)) = source_and_formatted_text(text, filename, config) {
-                let mismatch = make_diff(&ori, &fmt, 0);
-                let has_diff = !mismatch.is_empty();
-                output_modified(out, mismatch);
-                return Ok(has_diff);
-            }
+            let ori = fs::read_to_string(filename)?;
+            let mismatch = make_diff(&ori, formatted_text, 0);
+            let has_diff = !mismatch.is_empty();
+            output_modified(out, mismatch);
+            return Ok(has_diff);
         }
         EmitMode::Checkstyle => {
             let filename = filename_to_path();
-            let diff = create_diff(filename, text, config)?;
+            let ori = fs::read_to_string(filename)?;
+            let diff = make_diff(&ori, formatted_text, 3);
             output_checkstyle_file(out, filename, diff)?;
         }
         EmitMode::Diff => {
             let filename = filename_to_path();
-            if let Ok((ori, fmt)) = source_and_formatted_text(text, filename, config) {
-                let mismatch = make_diff(&ori, &fmt, 3);
-                let has_diff = !mismatch.is_empty();
-                print_diff(
-                    mismatch,
-                    |line_num| format!("Diff in {} at line {}:", filename.display(), line_num),
-                    config,
-                );
-                return Ok(has_diff);
-            }
+            let ori = fs::read_to_string(filename)?;
+            let mismatch = make_diff(&ori, formatted_text, 3);
+            let has_diff = !mismatch.is_empty();
+            print_diff(
+                mismatch,
+                |line_num| format!("Diff in {} at line {}:", filename.display(), line_num),
+                config,
+            );
+            return Ok(has_diff);
         }
     }
 

src/lib.rs

@@ -60,7 +60,8 @@ use visitor::{FmtVisitor, SnippetProvider};
 pub use checkstyle::{footer as checkstyle_footer, header as checkstyle_header};
 pub use config::summary::Summary;
 pub use config::{
-    load_config, CliOptions, Color, Config, EmitMode, FileLines, FileName, Range, Verbosity,
+    load_config, CliOptions, Color, Config, EmitMode, FileLines, FileName, NewlineStyle, Range,
+    Verbosity,
 };
 
 #[macro_use]
@@ -877,6 +878,7 @@ fn format_input_inner<T: Write>(
             filemap::append_newline(file);
 
             format_lines(file, file_name, skipped_range, config, report);
+            replace_with_system_newlines(file, config);
 
             if let Some(ref mut out) = out {
                 return filemap::write_file(file, file_name, out, config);
@@ -929,6 +931,34 @@ fn format_input_inner<T: Write>(
     }
 }
 
+pub fn replace_with_system_newlines(text: &mut String, config: &Config) -> () {
+    let style = if config.newline_style() == NewlineStyle::Native {
+        if cfg!(windows) {
+            NewlineStyle::Windows
+        } else {
+            NewlineStyle::Unix
+        }
+    } else {
+        config.newline_style()
+    };
+
+    match style {
+        NewlineStyle::Unix => return,
+        NewlineStyle::Windows => {
+            let mut transformed = String::with_capacity(text.capacity());
+            for c in text.chars() {
+                match c {
+                    '\n' => transformed.push_str("\r\n"),
+                    '\r' => continue,
+                    c => transformed.push(c),
+                }
+            }
+            *text = transformed;
+        }
+        NewlineStyle::Native => unreachable!(),
+    }
+}
+
 /// A single span of changed lines, with 0 or more removed lines
 /// and a vector of 0 or more inserted lines.
 #[derive(Debug, PartialEq, Eq)]

src/test/mod.rs

@@ -22,7 +22,6 @@ use std::str::Chars;
 
 use config::summary::Summary;
 use config::{Color, Config, ReportTactic};
-use filemap::write_system_newlines;
 use rustfmt_diff::*;
 use *;
 
@@ -401,14 +400,9 @@ fn idempotent_check(
     }
 
     let mut write_result = HashMap::new();
-    for &(ref filename, ref text) in &file_map {
-        let mut v = Vec::new();
-        // Won't panic, as we're not doing any IO.
-        write_system_newlines(&mut v, text, &config).unwrap();
-        // Won't panic, we are writing correct utf8.
-        let one_result = String::from_utf8(v).unwrap();
-        if let FileName::Real(ref filename) = *filename {
-            write_result.insert(filename.to_owned(), one_result);
+    for (filename, text) in file_map {
+        if let FileName::Real(ref filename) = filename {
+            write_result.insert(filename.to_owned(), text);
         }
     }