Skip to content

leverage itemize_list and write_list to recover trait bound comments #6048

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 2 commits into
base: master
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
104 changes: 86 additions & 18 deletions src/items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use regex::Regex;
use rustc_ast::visit;
use rustc_ast::{ast, ptr};
use rustc_span::{symbol, BytePos, Span, DUMMY_SP};
use unicode_width::UnicodeWidthStr;

use crate::attr::filter_inline_attrs;
use crate::comment::{
Expand Down Expand Up @@ -1132,6 +1133,56 @@ fn format_struct(
}
}

fn rewrite_bounds(
result: &mut String,
context: &RewriteContext<'_>,
bounds: &ast::GenericBounds,
terminator: &str,
shape: Shape,
span: Span,
) -> Option<()> {
let indented = shape.block_indent(context.config.tab_spaces());
let items = itemize_list(
context.snippet_provider,
bounds.iter(),
terminator,
"+",
|bound| bound.span().lo(),
|bound| bound.span().hi(),
|bound| bound.rewrite(context, indented),
span.lo(),
span.hi(),
false,
)
.collect::<Vec<_>>();

let tactic = definitive_tactic(
&items,
ListTactic::LimitedHorizontalVertical(shape.width),
Separator::Plus,
context.config.max_width(),
);

let fmt = ListFormatting::new(indented, context.config)
.tactic(tactic)
.trailing_separator(SeparatorTactic::Never)
.separator("+")
.separator_place(SeparatorPlace::Front)
.align_comments(false);

let item_str = write_list(items, &fmt)?;

let space = if tactic == DefinitiveListTactic::Horizontal {
Cow::from(" ")
} else {
indented.indent.to_string_with_newline(&context.config)
};
result.push(':');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we apply space_before_colon?

result.push_str(&space);
result.push_str(&item_str);
Some(())
}

pub(crate) fn format_trait(
context: &RewriteContext<'_>,
item: &ast::Item,
Expand Down Expand Up @@ -1164,25 +1215,40 @@ 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 !bounds.is_empty() {
// Retrieve *unnormalized* ident (See #6069)
let source_ident = context.snippet(item.ident.span);
let ident_hi = context.snippet_provider.span_after(item.span, source_ident);
let bound_hi = bounds.last().unwrap().span().hi();
let snippet = context.snippet(mk_sp(ident_hi, bound_hi));
if contains_comment(snippet) {
return None;
}
if context.config.version() == Version::Two {
let after_colon = context
.snippet_provider
.span_after(item.span.with_lo(item.ident.span.hi()), ":");

result = rewrite_assign_rhs_with(
context,
result + ":",
bounds,
shape,
&RhsAssignKind::Bounds,
RhsTactics::ForceNextLineWithoutIndent,
)?;
let span = mk_sp(after_colon, body_lo);
let shape = if result.contains('\n') {
shape
} else {
// `offset_left` takes into account what we've rewritten already + 1 for `:`
// `sub_width` take into account the trailing `{`
shape.offset_left(header.width() + 1)?.sub_width(1)?
Comment on lines +1228 to +1230
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might need to be header.width() + 2 if we're applying space_before_colon, Also I'm realizing that the header doesn't contain any rewritten generics. Might be better to move the shape derivation into rewrite_bounds

};
rewrite_bounds(&mut result, context, bounds, "{", shape, span)?;
} else {
// Retrieve *unnormalized* ident (See #6069)
let source_ident = context.snippet(item.ident.span);
let ident_hi = context.snippet_provider.span_after(item.span, source_ident);
let bound_hi = 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(
context,
result + ":",
bounds,
shape,
&RhsAssignKind::Bounds,
RhsTactics::ForceNextLineWithoutIndent,
)?;
}
}

// Rewrite where-clause.
Expand Down Expand Up @@ -1219,7 +1285,9 @@ pub(crate) fn format_trait(
result.push_str(&where_indent.to_string_with_newline(context.config));
}
result.push_str(&where_clause_str);
} else {
}

if generics.where_clause.predicates.is_empty() && bounds.is_empty() {
let item_snippet = context.snippet(item.span);
if let Some(lo) = item_snippet.find('/') {
// 1 = `{`
Expand Down
4 changes: 4 additions & 0 deletions src/lists.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,8 @@ impl ListItem {
pub(crate) enum Separator {
Comma,
VerticalBar,
// Used to format trait bounds
Plus,
}

impl Separator {
Expand All @@ -215,6 +217,8 @@ impl Separator {
Separator::Comma => 2,
// 3 = ` | `
Separator::VerticalBar => 3,
// 3 = ` + `
Separator::Plus => 3,
}
}
}
Expand Down
12 changes: 12 additions & 0 deletions tests/source/issue_2055.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// rustfmt-version: Two

pub trait A {}
pub trait B {}
pub trait C {}

pub trait Foo:
// A and C
A + C
// and B
+ B
{}
32 changes: 32 additions & 0 deletions tests/source/issue_6127.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// rustfmt-version: Two

trait Foo:
Fn(
ReallyLongTypeName,
ReallyLongTypeName,
ReallyLongTypeName,
ReallyLongTypeName,
) -> ReallyLongTypeName
{
}


trait Bar:
Fn(
ReallyLongTypeName,
ReallyLongTypeName,
ReallyLongTypeName,
ReallyLongTypeName,
) -> ReallyLongTypeName + Debug + Clone
{
}

trait FooBar:
Clone + Debug + Fn(
ReallyLongTypeName,
ReallyLongTypeName,
ReallyLongTypeName,
ReallyLongTypeName,
) -> ReallyLongTypeName
{
}
9 changes: 6 additions & 3 deletions tests/target/issue-4689/two.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ pub trait PrettyPrinter<'tcx>:
Type = Self,
DynExistential = Self,
Const = Self,
> + fmt::Write
>
+ fmt::Write
Comment on lines -26 to +27
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taking a look at the style guide description for traits

emphasis mine:

Prefer not to line-break in the bounds if possible (consider using a where clause). Prefer to break between bounds than to break any individual bound. If you must break the bounds, put each bound (including the first) on its own block-indented line, break before the + and put the opening brace on its own line:

I think these changes align more closely with the language in the guide than the formatting that we had before, but please let me know if I'm misinterpreting the guide.

{
//
}
Expand All @@ -36,7 +37,8 @@ pub trait PrettyPrinter<'tcx>:
Type = Self,
DynExistential = Self,
Const = Self,
> + fmt::Write1
>
+ fmt::Write1
+ fmt::Write2
{
//
Expand Down Expand Up @@ -65,7 +67,8 @@ pub trait PrettyPrinter<'tcx>:
Type = Self,
DynExistential = Self,
Const = Self,
> + Printer2<
>
+ Printer2<
'tcx,
Error = fmt::Error,
Path = Self,
Expand Down
13 changes: 13 additions & 0 deletions tests/target/issue_2055.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// rustfmt-version: Two

pub trait A {}
pub trait B {}
pub trait C {}

pub trait Foo:
// A and C
A
+ C // and B
+ B
{
}
35 changes: 35 additions & 0 deletions tests/target/issue_6127.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// rustfmt-version: Two

trait Foo:
Fn(
ReallyLongTypeName,
ReallyLongTypeName,
ReallyLongTypeName,
ReallyLongTypeName,
) -> ReallyLongTypeName
{
}

trait Bar:
Fn(
ReallyLongTypeName,
ReallyLongTypeName,
ReallyLongTypeName,
ReallyLongTypeName,
) -> ReallyLongTypeName
+ Debug
+ Clone
{
}

trait FooBar:
Clone
+ Debug
+ Fn(
ReallyLongTypeName,
ReallyLongTypeName,
ReallyLongTypeName,
ReallyLongTypeName,
) -> ReallyLongTypeName
{
}