Skip to content

Fix issue 1124 - detect start of output rather than start of input file when writing output source file #1133

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 6 commits into from
Aug 24, 2016
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
23 changes: 15 additions & 8 deletions src/imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,19 +178,26 @@ impl<'a> FmtVisitor<'a> {
// Order the imports by view-path & other import path properties
ordered_use_items.sort_by(|a, b| compare_use_items(a.0, b.0).unwrap());
// First, output the span before the first import
let prev_span_str = self.snippet(codemap::mk_sp(self.last_pos, pos_before_first_use_item));
// Look for purely trailing space at the start of the prefix snippet before a linefeed, or
// a prefix that's entirely horizontal whitespace.
let prefix_span_start = match prev_span_str.find('\n') {
Some(offset) if prev_span_str[..offset].trim().is_empty() => {
self.last_pos + BytePos(offset as u32)
}
None if prev_span_str.trim().is_empty() => pos_before_first_use_item,
_ => self.last_pos,
};
// Look for indent (the line part preceding the use is all whitespace) and excise that
// from the prefix
let prev_span_str = self.snippet(codemap::mk_sp(self.last_pos, pos_before_first_use_item));
let span_end = match prev_span_str.rfind('\n') {
Some(offset) => {
if prev_span_str[offset..].trim().is_empty() {
self.last_pos + BytePos(offset as u32)
} else {
pos_before_first_use_item
}
Some(offset) if prev_span_str[offset..].trim().is_empty() => {
self.last_pos + BytePos(offset as u32)
}
None => pos_before_first_use_item,
_ => pos_before_first_use_item,
};

self.last_pos = prefix_span_start;
self.format_missing(span_end);
for ordered in ordered_use_items {
// Fake out the formatter by setting `self.last_pos` to the appropriate location before
Expand Down
8 changes: 6 additions & 2 deletions src/missed_spans.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ use syntax::codemap::{self, BytePos, Span, Pos};
use comment::{CodeCharKind, CommentCodeSlices, rewrite_comment};

impl<'a> FmtVisitor<'a> {
fn output_at_start(&self) -> bool {
self.buffer.len == 0
}

// TODO these format_missing methods are ugly. Refactor and add unit tests
// for the central whitespace stripping loop.
pub fn format_missing(&mut self, end: BytePos) {
Expand All @@ -25,7 +29,7 @@ impl<'a> FmtVisitor<'a> {
let config = self.config;
self.format_missing_inner(end, |this, last_snippet, snippet| {
this.buffer.push_str(last_snippet.trim_right());
if last_snippet == snippet {
if last_snippet == snippet && !this.output_at_start() {
// No new lines in the snippet.
this.buffer.push_str("\n");
}
Expand All @@ -41,7 +45,7 @@ impl<'a> FmtVisitor<'a> {

if start == end {
// Do nothing if this is the beginning of the file.
if start != self.codemap.lookup_char_pos(start).file.start_pos {
if !self.output_at_start() {
process_last_snippet(self, "", "");
}
return;
Expand Down
2 changes: 1 addition & 1 deletion src/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -559,7 +559,7 @@ impl<'a> FmtVisitor<'a> {
self.last_pos = filemap.start_pos;
self.block_indent = Indent::empty();
self.walk_mod_items(m);
self.format_missing(filemap.end_pos);
self.format_missing_with_indent(filemap.end_pos);
}

pub fn get_context(&self) -> RewriteContext {
Expand Down
1 change: 1 addition & 0 deletions tests/config/issue-1124.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
reorder_imports = true
13 changes: 13 additions & 0 deletions tests/source/issue-1124.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
use d; use c; use b; use a;
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see some tests with blanks lines between imports and trailing whitespace before a newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current test has a space at the end of the first line already - I'll add a comment (after the use line - the whole point was putting that line at the start of the file!) to highlight that.

As for a test with blank lines - yes, no problem. That will reflect the current behaviour of the import reordering, which doesn't eliminate blank lines (as @crumblingstatue suggests, and which I think is worth looking at), but keeps them in verbatim.

// The previous line has a space after the `use a;`

mod a { use d; use c; use b; use a; }

use z;

use y;



use x;
use a;
21 changes: 21 additions & 0 deletions tests/target/issue-1124.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
use a;
use b;
use c;
use d;
// The previous line has a space after the `use a;`

mod a {
use a;
use b;
use c;
use d;
}

use a;



use x;

use y;
use z;