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

Handle comments between Trait Generics and Bounds #4666

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 3 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
38 changes: 34 additions & 4 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 @@ -1980,7 +1981,7 @@ pub(crate) fn rewrite_assign_rhs_with<S: Into<String>, R: Rewrite>(
rhs_tactics: RhsTactics,
) -> Option<String> {
let lhs = lhs.into();
let rhs = rewrite_assign_rhs_expr(context, &lhs, ex, shape, rhs_tactics)?;
let rhs = rewrite_rhs_expr(context, &lhs, ex, shape, rhs_tactics, "=")?;
Some(lhs + &rhs)
}

Expand All @@ -2000,7 +2001,7 @@ pub(crate) fn rewrite_assign_rhs_with_comments<S: Into<String>, R: Rewrite>(
} else {
shape
};
let rhs = rewrite_assign_rhs_expr(context, &lhs, ex, shape, rhs_tactics)?;
let rhs = rewrite_rhs_expr(context, &lhs, ex, shape, rhs_tactics, "=")?;

if contains_comment {
let rhs = rhs.trim_start();
Expand All @@ -2010,6 +2011,35 @@ pub(crate) fn rewrite_assign_rhs_with_comments<S: Into<String>, R: Rewrite>(
}
}

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));

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)
}
}

Copy link
Member

Choose a reason for hiding this comment

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

I think the overall idea here is good, but think we're doing this in the wrong place in the codebase. Traits are by definition Items, not Expressions, and by design (encapsulation, etc.) we'd only ever need to call this from one place: the corresponding formatting flow for traits in items.rs.

As such, this function feels quite out of place here in expr.rs and the logic would be best placed back in items.

Additionally (regarding changes above this one), and a bit of a nit, it's quite reasonable to have one of these rewrite functions allow the caller to specify the separator, but that doesn't mean we can't also have a helper function that avoids the duplicative calls that each have to specify the assignment operator as the separator (which would be introduced here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As such, this function feels quite out of place here in expr.rs and the logic would be best placed back in items.

Moved rewrite_trait_rhs_with_comments to items.rs.

... have a helper function that avoids the duplicative calls that each have to specify the assignment operator as the separator

Added a helper function rewrite_assign_rhs_expr that specifies the '=' separator.

fn choose_rhs<R: Rewrite>(
context: &RewriteContext<'_>,
expr: &R,
Expand Down
32 changes: 18 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_trait_rhs_with_comments, RhsTactics,
},
lists::{definitive_tactic, itemize_list, write_list, ListFormatting, Separator},
macros::{rewrite_macro, MacroPosition},
Expand Down Expand Up @@ -1122,6 +1122,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 +1144,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 +1198,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