Skip to content

Handle comments between Trait Generics and Bounds #4666

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

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
15 changes: 13 additions & 2 deletions src/formatting/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1938,12 +1938,13 @@ pub(crate) fn rewrite_assign_rhs<S: Into<String>, R: Rewrite>(
rewrite_assign_rhs_with(context, lhs, ex, shape, RhsTactics::Default)
}

pub(crate) fn rewrite_assign_rhs_expr<R: Rewrite>(
pub(crate) fn rewrite_rhs_expr<R: Rewrite>(
context: &RewriteContext<'_>,
lhs: &str,
ex: &R,
shape: Shape,
rhs_tactics: RhsTactics,
lhs_separator: &str,
) -> Option<String> {
let last_line_width = last_line_width(&lhs).saturating_sub(if lhs.contains('\n') {
shape.indent.width()
Expand All @@ -1956,7 +1957,7 @@ pub(crate) fn rewrite_assign_rhs_expr<R: Rewrite>(
offset: shape.offset + last_line_width + 1,
..shape
});
let has_rhs_comment = if let Some(offset) = lhs.find_last_uncommented("=") {
let has_rhs_comment = if let Some(offset) = lhs.find_last_uncommented(lhs_separator) {
lhs.trim_end().len() > offset + 1
} else {
false
Expand All @@ -1972,6 +1973,16 @@ pub(crate) fn rewrite_assign_rhs_expr<R: Rewrite>(
)
}

fn rewrite_assign_rhs_expr<R: Rewrite>(
context: &RewriteContext<'_>,
lhs: &str,
ex: &R,
shape: Shape,
rhs_tactics: RhsTactics,
) -> Option<String> {
rewrite_rhs_expr(context, &lhs, ex, shape, rhs_tactics, "=")
}

pub(crate) fn rewrite_assign_rhs_with<S: Into<String>, R: Rewrite>(
context: &RewriteContext<'_>,
lhs: S,
Expand Down
62 changes: 48 additions & 14 deletions src/formatting/items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use crate::formatting::{
},
expr::{
is_empty_block, is_simple_block_stmt, rewrite_assign_rhs, rewrite_assign_rhs_with,
rewrite_assign_rhs_with_comments, RhsTactics,
rewrite_assign_rhs_with_comments, rewrite_rhs_expr, RhsTactics,
},
lists::{definitive_tactic, itemize_list, write_list, ListFormatting, Separator},
macros::{rewrite_macro, MacroPosition},
Expand Down Expand Up @@ -1114,6 +1114,36 @@ fn format_struct(
}
}

pub(crate) fn rewrite_trait_rhs_with_comments<S: Into<String>, R: Rewrite>(
context: &RewriteContext<'_>,
lhs: S,
ex: &R,
shape: Shape,
rhs_tactics: RhsTactics,
between_span: Span,
allow_extend: bool,
) -> Option<String> {
let lhs = lhs.into();
let contains_comment = contains_comment(context.snippet(between_span));

// Not as with `expr` - `shape` is changed only for comments combine, not for rewrite
let rhs = rewrite_rhs_expr(context, &lhs, ex, shape, rhs_tactics, ":")?;

if contains_comment {
let rhs = rhs.trim_start();
combine_strs_with_missing_comments(
context,
&lhs,
&rhs,
between_span,
shape.block_left(context.config.tab_spaces())?,
allow_extend,
)
} else {
Some(lhs + &rhs)
}
}

pub(crate) fn format_trait(
context: &RewriteContext<'_>,
item: &ast::Item,
Expand All @@ -1122,6 +1152,12 @@ pub(crate) fn format_trait(
if let ast::ItemKind::Trait(trait_kind) = &item.kind {
let ast::TraitKind(is_auto, unsafety, ref generics, ref generic_bounds, ref trait_items) =
**trait_kind;

// FIXME: rustfmt fails to format when there are comments before the ident.
if contains_comment(context.snippet(mk_sp(item.span.lo(), item.ident.span.lo()))) {
return None;
}

let mut result = String::with_capacity(128);
let header = format!(
"{}{}{}trait ",
Expand All @@ -1138,23 +1174,23 @@ pub(crate) fn format_trait(
rewrite_generics(context, rewrite_ident(context, item.ident), generics, shape)?;
result.push_str(&generics_str);

// FIXME(#2055): rustfmt fails to format when there are comments between trait bounds.
if !generic_bounds.is_empty() {
// Rewrite rhs and combine lhs with pre-bound comment
let bound_lo = generic_bounds.first().unwrap().span().lo();
let ident_hi = context
.snippet_provider
.span_after(item.span, &item.ident.as_str());
let bound_hi = generic_bounds.last().unwrap().span().hi();
let snippet = context.snippet(mk_sp(ident_hi, bound_hi));
if contains_comment(snippet) {
return None;
}

result = rewrite_assign_rhs_with(
let ident_hi = context
.snippet_provider
.span_after(mk_sp(ident_hi, item.span.hi()), ":");
result = rewrite_trait_rhs_with_comments(
context,
result + ":",
generic_bounds,
shape,
RhsTactics::ForceNextLineWithoutIndent,
mk_sp(ident_hi, bound_lo),
true,
)?;
}

Expand Down Expand Up @@ -1192,11 +1228,9 @@ pub(crate) fn format_trait(
}
result.push_str(&where_clause_str);
}
let pre_block_span = if !generics.where_clause.predicates.is_empty() {
mk_sp(generics.where_clause.span.hi(), item.span.hi())
} else {
item.span
};

/* Note: `where_clause` always exists; Span is empty when no `where` clause in the code */
let pre_block_span = mk_sp(generics.where_clause.span.hi(), item.span.hi());
let pre_block_snippet = context.snippet(pre_block_span);
if let Some(lo) = pre_block_snippet.find('/') {
// 1 = `{`
Expand Down
3 changes: 3 additions & 0 deletions src/formatting/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1356,6 +1356,9 @@ impl MacroBranch {
result += " =>";
}

// Note (from issue #4759): comments between the end of `whole_body` and
// the end of `span` are currently ignored.

if !context.config.format_macro_bodies() {
result += " ";
result += context.snippet(self.whole_body);
Expand Down
111 changes: 111 additions & 0 deletions tests/source/lhs-to-rhs-between-comments/trait.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
// Based on issue #2055:
Copy link
Member

Choose a reason for hiding this comment

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

Given the updates to the pre block span, we'd want to have tests here that included a mix of some other trait cases, including some with where clauses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we'd want to have tests here that included a mix of some other trait cases, including some with where clauses

Added some test cases mainly with where clause.

pub trait A {}
pub trait B {}
pub trait C {}
pub trait Foo1:
A + C
+ B
{}
pub trait Foo2:
// A and C
A + C
+ B
{}
pub trait Foo3:
/* A and C */
A + C
+ B
{}
pub trait Foo4:// A and C
A + C
+ B
{}
pub trait Foo5:/* A and C */
A + C
+ B
{}
pub trait Foo6:/* A and C */A + C + B{}
pub trait Foo7:
A+C
// and B
+B{}
pub trait Foo8:
// A and C
A+C
// and B
+B{}

// Other cases
trait Person{
fn name(&self) -> String;
}
/*comment1*/trait Person{
fn name(&self) -> String;
}
trait Student:/* comment1 */Person/* comment2 */{
fn university(&self) -> String;
}
trait Programmer/* comment1 */{
fn fav_language(&self) -> String;
}
trait CompSciStudent1:/* comment1 */Programmer + Student/* comment2 */{
fn git_username(&self) -> String;
}
trait CompSciStudent2:/* comment1 Longggggggggggggggggggggggggggggggggggggggggggggggggg */Programmer + Student/* comment2 */{
fn git_username(&self) -> String;
}
trait CompSciStudent3:// comment1
Programmer + Student/* comment2 */{
fn git_username(&self) -> String;
}
trait CompSciStudent4:// comment1 Longgggggggggggggggggggggggggggggggggggggggggggggggggg
Programmer + Student/* comment2 */{
fn git_username(&self) -> String;
}

// Comment before Ident
trait /* comment1 */ Person {
fn fav_language(&self) -> String;
}
trait // comment1
Person {
fn fav_language(&self) -> String;
}
trait /* comment 1 */ Programmer /* comment2 */ {
fn fav_language(&self) -> String;
}
trait /* comment1 */ CompSciStudent1: /* comment2 */ Programmer + Student /* comment3 */ {
fn git_username(&self) -> String;
}

// Traits with where and comments
trait Bar where Self: Sized, Option<Self>: Foo
{}
/*comment0*/trait Bar/*comment1*/where Self: Sized/*comment2*/,/*comment3*/Option<Self>: Foo/*comment4*/
{}
trait Bar//comment1 Longgggggggggggggggggggggggggggggggggggggggggggggggggg
where Self: Sized/*comment2*/,/*comment3*/Option<Self>: Foo/*comment4*/
{}
trait Bar/*comment1*/where Self: Sized/*comment2*/,/*comment3*/Option<Self>: Foo//comment4 Longgggggggggggggggggggggggggggggggggggggggggggggggggg
{}
trait Bar/*comment1 Longgggggggggggggggggggggggggggggggggggggggggggggggggg*/where Self: Sized/*comment2 Longgggggggggggggggggggggggggggggggggggggggggggggggggg*/,/*comment3 Longgggggggggggggggggggggggggggggggggggggggggggggggggg*/Option<Self>: Foo/*comment4 Longgggggggggggggggggggggggggggggggggggggggggggggggggg*/
{}
trait ConstCheck<T>:/*comment1*/Foo where T: Baz/*comment2*/{
const J: i32;
}

// Some other trait cases with comments
/*comment0*/auto trait Example/*comment1*/{}
pub unsafe auto trait PubUnsafeExample/*comment1*/{}
pub unsafe auto trait PubUnsafeExample// comment1
{}
trait Foo/*comment1*/{ type Bar: Baz; type Inner: Foo = Box< Foo >; }
pub trait Iterator/*comment1*/{
type Item;
fn next(&mut self) -> Option<Self::Item>;
}
pub trait Iterator//comment1
{
type Item;
fn next(&mut self) -> Option<Self::Item>;
}
Loading