fix handling of newline_style conflicts #3850
Conversation
| // 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 }); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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?
| { | ||
| Some(ori) => ori, | ||
| None => fs::read_to_string(ensure_real_path(filename))?, | ||
| } | ||
| }; |
There was a problem hiding this comment.
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 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!
| // 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 }); | ||
| } |
There was a problem hiding this comment.
If make_diff use str.split then this additional check wouldn't be needed ? The error message could be less clear though...
Fixes #3849
There were two underyling issues with the way we were handling newline style differences that only surfaced when the source files have Windows style line feeds and/or when the
newline_styleconfig was set toWindows.Details inline below