Skip to content

Commit

Permalink
leverage itemize_list and write_list to recover trait bound comments
Browse files Browse the repository at this point in the history
Fixes 2055

Now when using `version::Two`, rustfmt will permit comments within trait
bounds and actually reformat them instead of refusing to rewrite the
entire trait definition because there are comments in the bounds.
  • Loading branch information
ytmimi committed Jan 29, 2024
1 parent cedb7b5 commit 41a6326
Show file tree
Hide file tree
Showing 5 changed files with 121 additions and 21 deletions.
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(':');
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() {
let ident_hi = context
.snippet_provider
.span_after(item.span, item.ident.as_str());
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 the what we've rewirtten already + 1 for `:`
// `sub_width` take into account the trailing `{`
shape.offset_left(header.width() + 1)?.sub_width(1)?
};
rewrite_bounds(&mut result, context, bounds, "{", shape, span)?;
} else {
let ident_hi = context
.snippet_provider
.span_after(item.span, item.ident.as_str());
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
{}
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
{
//
}
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
{
}

0 comments on commit 41a6326

Please sign in to comment.