Skip to content
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

Fixed first comment and last comment not reordering issue #4504

Open
wants to merge 4 commits into
base: rustfmt-2.0.0-rc.2
Choose a base branch
from
Open
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
5 changes: 5 additions & 0 deletions src/formatting/comment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,11 @@ pub(crate) fn is_last_comment_block(s: &str) -> bool {
s.trim_end().ends_with("*/")
}

/// Returns true if the first line of the passed string started with a block-comment.
pub(crate) fn is_first_comment_block(s: &str) -> bool {
s.trim_start().starts_with("/*")
}

/// Combine `prev_str` and `next_str` into a single `String`. `span` may contain
/// comments between two strings. If there are such comments, then that will be
/// recovered. If `allow_extend` is true and there is no comment between the two
Expand Down
68 changes: 44 additions & 24 deletions src/formatting/lists.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,14 @@ use unicode_segmentation::UnicodeSegmentation;

use crate::config::{lists::*, Config, IndentStyle};
use crate::formatting::{
comment::{comment_style, find_comment_end, rewrite_comment, FindUncommented},
comment::{
comment_style, find_comment_end, is_last_comment_block, rewrite_comment, FindUncommented,
},
rewrite::RewriteContext,
shape::{Indent, Shape},
utils::{
count_newlines, first_line_width, last_line_width, mk_sp, starts_with_newline,
unicode_str_width,
count_newlines, first_line_width, last_line_contains_single_line_comment, last_line_width,
mk_sp, starts_with_newline, unicode_str_width,
},
visitor::SnippetProvider,
};
Expand Down Expand Up @@ -281,6 +283,10 @@ where

let mut line_len = 0;
let indent_str = &formatting.shape.indent.to_string(formatting.config);
let newline_str = &formatting
.shape
.indent
.to_string_with_newline(formatting.config);
while let Some((i, item)) = iter.next() {
let item = item.as_ref();
let inner_item = item.item.as_ref()?;
Expand All @@ -291,6 +297,8 @@ where
SeparatorPlace::Back => !last || trailing_separator,
};
let item_sep_len = if separate { sep_len } else { 0 };
let item_has_comments = item.post_comment.is_some() || item.pre_comment.is_some();
let item_has_both_comments = item.post_comment.is_some() && item.pre_comment.is_some();

// Item string may be multi-line. Its length (used for block comment alignment)
// should be only the length of the last line.
Expand Down Expand Up @@ -318,17 +326,17 @@ where
} else if i < num_args_before {
result.push(' ');
} else if i <= num_args_before + 1 {
result.push('\n');
result.push_str(indent_str);
result.push_str(newline_str);
} else {
result.push(' ');
}
}
DefinitiveListTactic::Vertical
if !first && !inner_item.is_empty() && !result.is_empty() =>
if !first
&& (!inner_item.is_empty() || item_has_comments)
&& !result.is_empty() =>
{
result.push('\n');
result.push_str(indent_str);
result.push_str(newline_str);
}
DefinitiveListTactic::Mixed => {
let total_width = total_item_width(item) + item_sep_len;
Expand Down Expand Up @@ -385,8 +393,7 @@ where
if keep_comment {
result.push(' ');
} else {
result.push('\n');
result.push_str(indent_str);
result.push_str(newline_str);
// This is the width of the item (without comments).
line_len = item.item.as_ref().map_or(0, |s| unicode_str_width(&s));
}
Expand All @@ -397,7 +404,9 @@ where
item_max_width = None;
}

if separate && sep_place.is_front() && !first {
if inner_item.is_empty() && item_has_both_comments {
result.push_str(newline_str);
} else if separate && sep_place.is_front() && !first {
result.push_str(formatting.separator.trim());
result.push(' ');
}
Expand Down Expand Up @@ -461,7 +470,7 @@ where
formatting.config.max_width(),
));
}
let overhead = if starts_with_newline(comment) {
let overhead = if starts_with_newline(comment) || inner_item.is_empty() {
0
} else if let Some(max_width) = *item_max_width {
max_width + 2
Expand All @@ -471,6 +480,7 @@ where
};
let width = formatting.shape.width.checked_sub(overhead).unwrap_or(1);
let offset = formatting.shape.indent + overhead;

let comment_start_trimmed = comment.trim_start();

// Find if first comment is single line and the end of the first comment
Expand Down Expand Up @@ -589,11 +599,12 @@ where
}
// An additional space for the missing trailing separator (or
// if we skipped alignment above).
if !formatting.align_comments
|| (last
&& item_max_width.is_some()
&& !separate
&& !formatting.separator.is_empty())
if !inner_item.is_empty()
&& (!formatting.align_comments
|| (last
&& item_max_width.is_some()
&& !separate
&& !formatting.separator.is_empty()))
Comment on lines +602 to +607
Copy link
Member

Choose a reason for hiding this comment

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

I know this one was already pretty busy but the extra condition and subsequent indentation is a bit of an eye sore. I guess it'd be okay but I also wouldn't mind seeing some minor refactoring of this existing code too

{
result.push(' ');
}
Expand Down Expand Up @@ -860,22 +871,31 @@ where
.span_to_snippet(mk_sp(self.prev_span_end, (self.get_lo)(&item)))
.unwrap_or("");
let (pre_comment, pre_comment_style) = extract_pre_comment(pre_snippet);
let next_item = self.inner.peek();

// Post-comment
let next_start = match self.inner.peek() {
let next_start = match next_item {
Some(next_item) => (self.get_lo)(next_item),
None => self.next_span_start,
};
let post_snippet = self
.snippet_provider
.span_to_snippet(mk_sp((self.get_hi)(&item), next_start))
.unwrap_or("");
let comment_end = get_comment_end(
post_snippet,
self.separator,
self.terminator,
self.inner.peek().is_none(),
);
let comment_end = if next_item.is_none()
&& (last_line_contains_single_line_comment(post_snippet)
|| is_last_comment_block(post_snippet))
{
// To get the trailing single line comment of the last item
post_snippet.len()
} else {
get_comment_end(
post_snippet,
self.separator,
self.terminator,
self.inner.peek().is_none(),
)
};
let new_lines = has_extra_newline(post_snippet, comment_end);
let post_comment = extract_post_comment(post_snippet, comment_end, self.separator);

Expand Down
40 changes: 37 additions & 3 deletions src/formatting/reorder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,13 @@
use std::cmp::{Ord, Ordering};

use rustc_ast::ast;
use rustc_span::{symbol::sym, Span};
use rustc_span::{symbol::sym, BytePos, Pos, Span};

use crate::config::{Config, GroupImportsTactic, ImportGranularity};
use crate::formatting::imports::{flatten_use_trees, UseSegment};
use crate::formatting::modules::{get_mod_inner_attrs, FileModMap};
use crate::formatting::{
comment::{comment_style, contains_comment, is_first_comment_block, is_last_comment_block},
imports::{merge_use_trees, UseTree},
items::{is_mod_decl, rewrite_extern_crate, rewrite_mod},
lists::{itemize_list, write_list, ListFormatting, ListItem},
Expand Down Expand Up @@ -413,8 +414,41 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {

if at_least_one_in_file_lines && !items.is_empty() {
self.normalize_vertical_spaces = true;
let lo = items.first().unwrap().span().lo();
let hi = items.last().unwrap().span().hi();
let context = self.get_context();

let first_lo = items.first().unwrap().span().lo();
let line_lo = self.parse_sess.line_bounds(first_lo).unwrap().start;
let leading_snip = context.snippet(mk_sp(line_lo, first_lo));
let lo = if contains_comment(leading_snip) {
let comment_started = if is_last_comment_block(leading_snip) {
is_first_comment_block(leading_snip)
} else {
true
};
if comment_started { line_lo } else { first_lo }
} else {
first_lo
};

let last_hi = items.last().unwrap().span().hi();
let line_hi = self.parse_sess.line_bounds(last_hi).unwrap().end;
let trailing_snip = context.snippet(mk_sp(last_hi, line_hi));
let hi = if contains_comment(trailing_snip) {
let comment_ended = if is_first_comment_block(trailing_snip) {
is_last_comment_block(trailing_snip.trim())
} else {
true
};
if comment_ended {
// 1 = '\n'
line_hi - BytePos::from_usize(1)
} else {
last_hi
}
} else {
last_hi
};

let span = mk_sp(lo, hi);
let rw = rewrite_reorderable_or_regroupable_items(
&self.get_context(),
Expand Down
10 changes: 10 additions & 0 deletions src/formatting/syntux/session.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use std::cell::RefCell;
use std::ops::Range;
use std::path::Path;
use std::rc::Rc;

Expand Down Expand Up @@ -189,6 +190,15 @@ impl ParseSess {
}
}

pub(crate) fn line_bounds(&self, pos: BytePos) -> Option<Range<BytePos>> {
let line = self.parse_sess.source_map().lookup_line(pos).ok();

match line {
Some(line_info) => Some(line_info.sf.line_bounds(line_info.line)),
None => None,
}
}

pub(crate) fn line_of_byte_pos(&self, pos: BytePos) -> usize {
self.parse_sess.source_map().lookup_char_pos(pos).line
}
Expand Down
3 changes: 3 additions & 0 deletions tests/source/issue-3127.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
extern crate serde; // 1.0.78
extern crate serde_json; // 1.0.27
extern crate serde_derive; // 1.0.78
3 changes: 3 additions & 0 deletions tests/source/issue-3720.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
use c; // use c;
use b; // use b;
use a; // use a;
2 changes: 2 additions & 0 deletions tests/source/issue-4027.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
pub use views::*;
pub use foo::bar; // re-export for frontend
62 changes: 62 additions & 0 deletions tests/source/reorder_imports_comments.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// #4027
pub use views::*;
pub use foo::bar; // re-export for frontend

// #3720
use c; // use c;
use b; // use b;
use a; // use a;

// #3127
extern crate serde; // 1.0.78
extern crate serde_json; // 1.0.27
extern crate serde_derive; // 1.0.78

mod c;
mod b;
mod a;

/*dpre*/ use d; // dpost
/*cpre*/ use c; // cpost
/*bpre*/ use b; // bpost
/*apre*/ use a; // apost

use std::{};
use std::borrow::Cow;

/* comment 1 */ use std::{};
/* comment 2 */ use std::{};
/* comment 3 */ use std::{};

use std::{}; /* comment 1 */
use std::{}; /* comment 2 */
use std::{}; /* comment 3 */

/* comment cpre */ use std::{}; /* comment cpost */
/* comment bpre */ use std::{}; /* comment bpost */
/* comment dpre */ use std::{}; /* comment dpost */

/* comment 1 */ use a;
/* comment 2 */ use b;
/* comment 3 */ use c;

use std::a; /* comment 1 */
use std::b; /* comment 2 */
use std::c; /* comment 3 */

/* comment cpre */ use std::c; /* comment cpost */
/* comment bpre */ use std::b; /* comment bpost */
/* comment dpre */ use std::a; /* comment dpost */


/* comment 1 */ use std::{}; // Comment 1
/* comment 2 */ use std::{}; // Comment 2
/* comment 3 */ use std::{}; // Comment 3


/* Comment
A */ use a; // Comment A
/* Comment B */ use b; // Comment B
/* Comment C */ use c; // Comment C
/* Comment D */ use d; /* Comment
D */
3 changes: 3 additions & 0 deletions tests/target/issue-3127.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
extern crate serde; // 1.0.78
extern crate serde_derive; // 1.0.78
extern crate serde_json; // 1.0.27
3 changes: 3 additions & 0 deletions tests/target/issue-3720.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
use a; // use a;
use b; // use b;
use c; // use c;
2 changes: 2 additions & 0 deletions tests/target/issue-4027.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
pub use foo::bar; // re-export for frontend
pub use views::*;
Loading