From 8c32a9d909ee7a517b9fb24b85f871a43eb5cbba Mon Sep 17 00:00:00 2001 From: Tibo Delor Date: Sun, 10 Jun 2018 00:25:06 +1000 Subject: [PATCH 1/2] Parse Error return an Error instead of a successful empty response --- src/lib.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 8d4cbb51b5a..40ecd71fdf3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -132,6 +132,9 @@ pub enum ErrorKind { /// An io error during reading or writing. #[fail(display = "io error: {}", _0)] IoError(io::Error), + /// Parse error occured when parsing the Input. + #[fail(display = "parse error")] + ParseError, /// The user mandated a version and the current version of Rustfmt does not /// satisfy that requirement. #[fail(display = "Version mismatch")] @@ -172,9 +175,10 @@ impl FormattingError { } fn msg_prefix(&self) -> &str { match self.kind { - ErrorKind::LineOverflow(..) | ErrorKind::TrailingWhitespace | ErrorKind::IoError(_) => { - "internal error:" - } + ErrorKind::LineOverflow(..) + | ErrorKind::TrailingWhitespace + | ErrorKind::IoError(_) + | ErrorKind::ParseError => "internal error:", ErrorKind::LicenseCheck | ErrorKind::BadAttr | ErrorKind::VersionMismatch => "error:", ErrorKind::BadIssue(_) | ErrorKind::DeprecatedAttr => "warning:", } @@ -844,7 +848,7 @@ fn format_input_inner( ParseError::Recovered => {} } summary.add_parsing_error(); - return Ok((summary, FileMap::new(), FormatReport::new())); + return Err((ErrorKind::ParseError, summary)); } }; From 6b00b8b302ba2e0a0946739ebe279d17f43f589b Mon Sep 17 00:00:00 2001 From: Tibo Delor Date: Sun, 10 Jun 2018 00:25:47 +1000 Subject: [PATCH 2/2] 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 --- src/filemap.rs | 135 +++++++++++++----------------------------------- src/lib.rs | 32 +++++++++++- src/test/mod.rs | 12 ++--- 3 files changed, 69 insertions(+), 110 deletions(-) diff --git a/src/filemap.rs b/src/filemap.rs index 96d9abc2a35..507e85f2086 100644 --- a/src/filemap.rs +++ b/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,42 +47,8 @@ where Ok(()) } -// Prints all newlines either as `\n` or as `\r\n`. -pub fn write_system_newlines(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( - text: &str, + formatted_text: &str, filename: &FileName, out: &mut T, config: &Config, @@ -91,29 +56,6 @@ pub fn write_file( 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, 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)?; - } + 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::rename(filename, bk_name)?; - fs::rename(tmp_name, filename)?; - } + 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); } } diff --git a/src/lib.rs b/src/lib.rs index 40ecd71fdf3..0ea3c604770 100644 --- a/src/lib.rs +++ b/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( 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( } } +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)] diff --git a/src/test/mod.rs b/src/test/mod.rs index 3ac8caae291..fdbe1e6856e 100644 --- a/src/test/mod.rs +++ b/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); } }