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.

Sign up for GitHub

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

Jump to bottom

Format trailing where clauses in type aliases #5887

Merged
merged 2 commits into from
Sep 12, 2024
Merged

Format trailing where clauses in type aliases #5887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4634072
Format trailing where clauses in rustfmt
compiler-errors Aug 16, 2023
21314d1
Add tests showing how comments are handled
compiler-errors Aug 27, 2023
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
78 changes: 59 additions & 19 deletions src/items.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1712,6 +1712,9 @@ pub(crate) fn rewrite_type_alias<'a, 'b>(
where_clauses,
} = *ty_alias_kind;
let ty_opt = ty.as_ref();
let rhs_hi = ty
.as_ref()
.map_or(where_clauses.before.span.hi(), |ty| ty.span.hi());
let (ident, vis) = match visitor_kind {
Item(i) => (i.ident, &i.vis),
AssocTraitItem(i) | AssocImplItem(i) => (i.ident, &i.vis),
Expand All @@ -1726,17 +1729,23 @@ pub(crate) fn rewrite_type_alias<'a, 'b>(
match (visitor_kind, &op_ty) {
(Item(_) | AssocTraitItem(_) | ForeignItem(_), Some(op_bounds)) => {
let op = OpaqueType { bounds: op_bounds };
rewrite_ty(rw_info, Some(bounds), Some(&op), vis)
rewrite_ty(rw_info, Some(bounds), Some(&op), rhs_hi, vis)
}
(Item(_) | AssocTraitItem(_) | ForeignItem(_), None) => {
rewrite_ty(rw_info, Some(bounds), ty_opt, vis)
rewrite_ty(rw_info, Some(bounds), ty_opt, rhs_hi, vis)
}
(AssocImplItem(_), _) => {
let result = if let Some(op_bounds) = op_ty {
let op = OpaqueType { bounds: op_bounds };
rewrite_ty(rw_info, Some(bounds), Some(&op), &DEFAULT_VISIBILITY)
rewrite_ty(
rw_info,
Some(bounds),
Some(&op),
rhs_hi,
&DEFAULT_VISIBILITY,
)
} else {
rewrite_ty(rw_info, Some(bounds), ty_opt, vis)
rewrite_ty(rw_info, Some(bounds), ty_opt, rhs_hi, vis)
}?;
match defaultness {
ast::Defaultness::Default(..) => Ok(format!("default {result}")),
Expand All @@ -1750,6 +1759,8 @@ fn rewrite_ty<R: Rewrite>(
rw_info: &TyAliasRewriteInfo<'_, '_>,
generic_bounds_opt: Option<&ast::GenericBounds>,
rhs: Option<&R>,
// the span of the end of the RHS (or the end of the generics, if there is no RHS)
rhs_hi: BytePos,
vis: &ast::Visibility,
) -> RewriteResult {
let mut result = String::with_capacity(128);
Expand All @@ -1758,9 +1769,6 @@ fn rewrite_ty<R: Rewrite>(
.where_clause
.predicates
.split_at(where_clauses.split);
if !after_where_predicates.is_empty() {
return Err(RewriteError::Unknown);
}
result.push_str(&format!("{}type ", format_visibility(context, vis)));
let ident_str = rewrite_ident(context, ident);

Expand Down Expand Up @@ -1796,7 +1804,7 @@ fn rewrite_ty<R: Rewrite>(
if rhs.is_none() {
option.suppress_comma();
}
let where_clause_str = rewrite_where_clause(
let before_where_clause_str = rewrite_where_clause(
context,
before_where_predicates,
where_clauses.before.span,
Expand All @@ -1808,14 +1816,20 @@ fn rewrite_ty<R: Rewrite>(
generics.span.hi(),
option,
)?;
result.push_str(&where_clause_str);
result.push_str(&before_where_clause_str);

if let Some(ty) = rhs {
// If there's a where clause, add a newline before the assignment. Otherwise just add a
// space.
let has_where = !before_where_predicates.is_empty();
if has_where {
let mut result = if let Some(ty) = rhs {
// If there are any where clauses, add a newline before the assignment.
// If there is a before where clause, do not indent, but if there is
// only an after where clause, additionally indent the type.
if !before_where_predicates.is_empty() {
result.push_str(&indent.to_string_with_newline(context.config));
} else if !after_where_predicates.is_empty() {
result.push_str(
&indent
.block_indent(context.config)
.to_string_with_newline(context.config),
);
} else {
result.push(' ');
}
Expand All @@ -1834,7 +1848,7 @@ fn rewrite_ty<R: Rewrite>(
.unknown_error()?,
) =>
{
let comment_shape = if has_where {
let comment_shape = if !before_where_predicates.is_empty() {
Shape::indented(indent, context.config)
} else {
let shape = Shape::indented(indent, context.config);
Expand All @@ -1855,13 +1869,39 @@ fn rewrite_ty<R: Rewrite>(
_ => format!("{result}="),
};

// 1 = `;`
// 1 = `;` unless there's a trailing where clause
let shape = Shape::indented(indent, context.config);
let shape = shape.sub_width(1).max_width_error(shape.width, span)?;
rewrite_assign_rhs(context, lhs, &*ty, &RhsAssignKind::Ty, shape).map(|s| s + ";")
let shape = if after_where_predicates.is_empty() {
Shape::indented(indent, context.config)
.sub_width(1)
.max_width_error(shape.width, span)?
} else {
shape
};
rewrite_assign_rhs(context, lhs, &*ty, &RhsAssignKind::Ty, shape)?
} else {
Ok(format!("{result};"))
result
};

if !after_where_predicates.is_empty() {
let option = WhereClauseOption::new(true, WhereClauseSpace::Newline);
let after_where_clause_str = rewrite_where_clause(
context,
after_where_predicates,
where_clauses.after.span,
context.config.brace_style(),
Shape::indented(indent, context.config),
false,
";",
None,
rhs_hi,
option,
)?;
result.push_str(&after_where_clause_str);
}

result += ";";
Ok(result)
}

fn type_annotation_spacing(config: &Config) -> (&str, &str) {
Expand Down
31 changes: 31 additions & 0 deletions tests/source/type-alias-where-clauses-with-comments.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
type Foo // comment1
// interlinear1
where // comment2
// interlinear2
A: B, // comment3
C: D, // comment4
// interlinear3
= E; // comment5

type Foo // comment6
// interlinear4
where// comment7
// interlinear5
A: B, // comment8
C: D, // comment9
// interlinear6
= E // comment10
// interlinear7
where // comment11
// interlinear8
F: G, // comment12
H: I; // comment13

type Foo // comment14
// interlinear9
= E // comment15
// interlinear10
where // comment16
// interlinear11
F: G,// comment17
H: I;// comment18
20 changes: 20 additions & 0 deletions tests/source/type-alias-where-clauses.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
type Foo
where
A: B,
C: D,
= E;

type Foo
where
A: B,
C: D,
= E
where
F: G,
H: I;

type Foo
= E
where
F: G,
H: I;
39 changes: 39 additions & 0 deletions tests/target/type-alias-where-clauses-with-comments.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
type Foo
// comment1
// interlinear1
where
// comment2
// interlinear2
A: B, // comment3
C: D, // comment4
// interlinear3
= E; // comment5

type Foo
// comment6
// interlinear4
where
// comment7
// interlinear5
A: B, // comment8
C: D, // comment9
// interlinear6
= E
// comment10
// interlinear7
where
// comment11
// interlinear8
F: G, // comment12
H: I; // comment13

type Foo // comment14
// interlinear9
= E
// comment15
// interlinear10
where
// comment16
// interlinear11
F: G, // comment17
H: I; // comment18
20 changes: 20 additions & 0 deletions tests/target/type-alias-where-clauses.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
type Foo
where
A: B,
C: D,
= E;

type Foo
where
A: B,
C: D,
= E
where
F: G,
H: I;
Comment on lines +7 to +14
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like an interesting case. Is it valid to have two where clauses?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, but it's not denied by the parser, so I think rustfmt shouldn't explode when we have it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate the clarification on this

Copy link
Member

Choose a reason for hiding this comment

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

so I think rustfmt shouldn't explode when we have it.

I'd like to adopt this as a new motto for us 🤣


type Foo
= E
where
F: G,
H: I;
Comment on lines +16 to +20
Copy link
Contributor

Choose a reason for hiding this comment

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

@compiler-errors is there documentation in the style guide that you can link to for this?

I'm wondering why this wouldn't be formatted like:

type Foo = E
where
    F: G,
    H: I;

Copy link
Member Author

Choose a reason for hiding this comment

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

rust-lang/rust#114901

Copy link
Member Author

Choose a reason for hiding this comment

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

Specifically:

When there is a trailing where clause after the type, and no where clause
present before the type, break before the = and indent. Then break before the
where keyword and format the clauses normally, e.g.,

// With only a trailing where clause
type VeryLongType<T, U>
    = AnEvenLongerType<T, U, Foo<T>>
where
    T: U::AnAssociatedType,
    U: SomeBound;

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you. That's exactly what I was looking for

Loading