-
Notifications
You must be signed in to change notification settings - Fork 931
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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)?; | ||
|
||
|
@@ -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, | ||
|
@@ -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))?, | ||
} | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: 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 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 Definitely open to any suggested changes/alternatives though! |
||
|
||
let formatted_file = emitter::FormattedFile { | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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 themake_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 changerustfmt
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 runningrustfmt --check
was giving me a non-zero exit code (due to the newline style diffs) without giving me any indication why.There was a problem hiding this comment.
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...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @scampi!
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 inrustfmt
printing the entire contents of the file twice (for example if the original used\r\n
andrustfmt
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?