Skip to content

fix handling of newline_style conflicts #3850

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/emitter/diff.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@ impl Emitter for DiffEmitter {
&self.config,
);
}
} else if original_text != formatted_text {
// This occurs when the only difference between the original and formatted values
// is the newline style. This happens because The make_diff function compares the
// original and formatted values line by line, independent of line endings.
let file_path = ensure_real_path(filename);
writeln!(output, "Incorrect newline style in {}", file_path.display())?;
return Ok(EmitterResult { has_diff: true });
}
Copy link
Member Author

@calebcartwright calebcartwright Oct 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When running in --check mode, this is the emitter used, and like many of the other emitters, it leverages the make_diff function in rustfmt_diff.rs.

That function internally leverages the diff crate to check for any formatting changes on a line by line basis. However, internally diff leverages the str.lines() function which results in line-ending-only differences being ignored.

This change ensures that rustfmt --check .. will exit with a non-zero exit code, even if the only change rustfmt would make is the newline style. I added a line to emit the name of the file if there was a newline style issue, because IMO it would be confusing if the only issue in my project was newline style and running rustfmt --check was giving me a non-zero exit code (due to the newline style diffs) without giving me any indication why.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If make_diff use str.split then this additional check wouldn't be needed ? The error message could be less clear though...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @scampi!

If make_diff use str.split then this additional check wouldn't be needed ? The error message could be less clear though...

Just to clarify, are you asking if we could avoid this check here if we updated make_diff to include checking line endings? If so, then potentially yes, although that would result in the line ending characters always being displayed in the output when there's any diff. Also in the instance of newline style only diffs, I think it would most often result in rustfmt printing the entire contents of the file twice (for example if the original used \r\n and rustfmt changed the newline style to \n).

I could take a look into what would be involved to make that happen in make_diff if you'd like?


return Ok(EmitterResult { has_diff });
Expand Down Expand Up @@ -107,4 +114,25 @@ mod tests {
format!("{}\n{}\n", bin_file, lib_file),
)
}

#[test]
fn prints_newline_message_with_only_newline_style_diff() {
let mut writer = Vec::new();
let config = Config::default();
let mut emitter = DiffEmitter::new(config);
let _ = emitter
.emit_formatted_file(
&mut writer,
FormattedFile {
filename: &FileName::Real(PathBuf::from("src/lib.rs")),
original_text: "fn empty() {}\n",
formatted_text: "fn empty() {}\r\n",
},
)
.unwrap();
assert_eq!(
String::from_utf8(writer).unwrap(),
String::from("Incorrect newline style in src/lib.rs\n")
);
}
}
10 changes: 8 additions & 2 deletions src/formatting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,8 +243,14 @@ impl<'b, T: Write + 'b> FormatHandler for Session<'b, T> {
report: &mut FormatReport,
) -> Result<(), ErrorKind> {
if let Some(ref mut out) = self.out {
match source_file::write_file(Some(source_map), &path, &result, out, &mut *self.emitter)
{
match source_file::write_file(
Some(source_map),
&path,
&result,
out,
&mut *self.emitter,
self.config.newline_style(),
) {
Ok(ref result) if result.has_diff => report.add_diff(),
Err(e) => {
// Create a new error with path_str to help users see which files failed
Expand Down
37 changes: 28 additions & 9 deletions src/source_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use syntax::source_map::SourceMap;

use crate::config::FileName;
use crate::emitter::{self, Emitter};
use crate::NewlineStyle;

#[cfg(test)]
use crate::config::Config;
Expand All @@ -32,7 +33,14 @@ where

emitter.emit_header(out)?;
for &(ref filename, ref text) in source_file {
write_file(None, filename, text, out, &mut *emitter)?;
write_file(
None,
filename,
text,
out,
&mut *emitter,
config.newline_style(),
)?;
}
emitter.emit_footer(out)?;

Expand All @@ -45,6 +53,7 @@ pub(crate) fn write_file<T>(
formatted_text: &str,
out: &mut T,
emitter: &mut dyn Emitter,
newline_style: NewlineStyle,
) -> Result<emitter::EmitterResult, io::Error>
where
T: Write,
Expand All @@ -65,15 +74,25 @@ where
}
}

// If parse session is around (cfg(not(test))) then try getting source from
// there instead of hitting the file system. This also supports getting
// SourceFile's in the SourceMap will always have Unix-style line endings
// See: https://github.com/rust-lang/rustfmt/issues/3850
// So if the user has explicitly overridden the rustfmt `newline_style`
// config and `filename` is FileName::Real, then we must check the file system
// to get the original file value in order to detect newline_style conflicts.
// Otherwise, parse session is around (cfg(not(test))) and newline_style has been
// left as the default value, then try getting source from the parse session
// source map instead of hitting the file system. This also supports getting
// original text for `FileName::Stdin`.
let original_text = source_map
.and_then(|x| x.get_source_file(&filename.into()))
.and_then(|x| x.src.as_ref().map(ToString::to_string));
let original_text = match original_text {
Some(ori) => ori,
None => fs::read_to_string(ensure_real_path(filename))?,
let original_text = if newline_style != NewlineStyle::Auto && *filename != FileName::Stdin {
fs::read_to_string(ensure_real_path(filename))?
} else {
match source_map
.and_then(|x| x.get_source_file(&filename.into()))
.and_then(|x| x.src.as_ref().map(ToString::to_string))
{
Some(ori) => ori,
None => fs::read_to_string(ensure_real_path(filename))?,
}
};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is to resolve the other bug I found while working on the diff emitter. It turns out that internally, as the parser constructs the source map, all of the files loaded into the source map store a value of the original file contents with all line endings normalized to Unix style:

https://github.com/rust-lang/rust/blob/31d75c4e9c5318e880601d3c2cc71e5df094a120/src/libsyntax_pos/lib.rs#L1065-L1072

https://github.com/rust-lang/rust/blob/53b352edb6f441bc3cf5386806fcc4686f275130/src/libsyntax/source_map.rs#L223-L269

This means that even if the original file contents were using Windows style line endings, that the Source Map will always represent the SourceFile.src values with Unix style line endings. As mentioned in the PR description, this results in buggy behavior with rustfmt whenever Windows style line endings are involved (either in rustfmt newline_style config and/or in the original file contents). In such instances, it is necessary to read the original file contents from disk in order to get a "correct" value for original_text

I'm of two minds as to whether this change should be version gated (and/or should require an explicit config option), but I ended up settling with this because my thinking was that users that are explicitly overriding the default newline_style config value in rustfmt would definitely expect the behavior this change provides.

Definitely open to any suggested changes/alternatives though!


let formatted_file = emitter::FormattedFile {
Expand Down