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

Preserve and format type aliases in extern blocks #4164

Merged
merged 4 commits into from
May 12, 2020

Conversation

ayazhafiz
Copy link
Contributor

Previously, non-trivial type aliases in extern blocks were dropped by
rustfmt because only the type alias name would be passed to a rewritter.
This commit fixes that by passing all type information (generics,
bounds, and assignments) to a type alias rewritter, and consolidates
rewrite_type_alias and rewrite_associated_type as one function.

Closes #4159

ayazhafiz added 2 commits May 9, 2020 15:23
Previously, non-trivial type aliases in extern blocks were dropped by
rustfmt because only the type alias name would be passed to a rewritter.
This commit fixes that by passing all type information (generics,
bounds, and assignments) to a type alias rewritter, and consolidates
`rewrite_type_alias` and `rewrite_associated_type` as one function.

Closes rust-lang#4159
let lhs = format!("{}{} =", prefix, type_bounds_str);

// If there's a where clause, add a newline before the assignment. Otherwise just add a
// space.
Copy link
Member

Choose a reason for hiding this comment

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

Do we always have to add a newline and/or is this something we'd expect users to want to configure? I get the sense that this will be fairly subjective and some folks won't be fond of all the multilining for these type aliases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this from the existing implementation for writing types:

fn rewrite_type_item<R: Rewrite>(
context: &RewriteContext<'_>,
indent: Indent,
prefix: &str,
suffix: &str,
ident: ast::Ident,
rhs: &R,
generics: &ast::Generics,
vis: &ast::Visibility,
) -> Option<String> {
let mut result = String::with_capacity(128);
result.push_str(&rewrite_type_prefix(
context,
indent,
&format!("{}{} ", format_visibility(context, vis), prefix),
ident,
generics,
)?);
if generics.where_clause.predicates.is_empty() {
result.push_str(suffix);
} else {
result.push_str(&indent.to_string_with_newline(context.config));
result.push_str(suffix.trim_start());
}

Copy link
Member

@calebcartwright calebcartwright May 10, 2020

Choose a reason for hiding this comment

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

Yeah I figured, and I know it's consistent (both with existing type alias formatting and on functions/structs/enums). I suppose folks can also use where_single_line to control things a bit as well.

It just felt odd to me that an alias as short as type A<'a> where 'a: 'static; would be split into 4 lines by default (understood that will become 3 after the semi placement change discussed above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree it looks a little odd. I think keeping consistency with how where clauses are formatted in other contexts (i.e. writing the where on a newline) is valuable. Do you find the 3-line version with the semicolon placement change to be nicer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, ideally, we respect the where_single_line option wherever we format a where clause. Unfortunately, that's not the case in the current implementation. I think that we can fix this as a part of the stabilization process of the where_single_line option; in this PR, we can follow the existing approach.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM 👍

@ayazhafiz ayazhafiz requested a review from topecongiro May 10, 2020 17:12
@ayazhafiz
Copy link
Contributor Author

Sorry this diff is not the prettiest, it may be easier to look at the raw file.

@ayazhafiz ayazhafiz requested a review from calebcartwright May 10, 2020 17:37
Copy link
Contributor

@topecongiro topecongiro left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the update!

@karyon
Copy link
Contributor

karyon commented Oct 25, 2021

backported in #4447

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rustfmt destroys extern types that have generics or rhs
4 participants