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

empty_item_single_line should apply to trait definitions as well #5047

Closed
nebulaeandstars opened this issue Oct 28, 2021 · 4 comments · Fixed by #5060
Closed

empty_item_single_line should apply to trait definitions as well #5047

nebulaeandstars opened this issue Oct 28, 2021 · 4 comments · Fixed by #5060
Labels
good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted only-with-option requires a non-default option value to reproduce

Comments

@nebulaeandstars
Copy link

1.4.37-nightly

Using this rustfmt.toml

brace_style = "AlwaysNextLine"
empty_item_single_line = true

rustfmt will format the following code like this:

fn function() {}

struct Struct {}

enum Enum {}

trait Trait
{
}

impl<T> Trait for T {}

This is inconsistent, and empty traits end up sticking out like a sore thumb. Intended behaviour is probably this:

fn function() {}
struct Struct {}
enum Enum {}
trait Trait {}
impl<T> Trait for T {}

This is important, as empty traits are often quite useful. For instance, you might want something like this:

trait Trait: Send + Clone {}
impl<T: Send + Clone> Trait for T {}

which will look quite ugly at the moment.

@nebulaeandstars nebulaeandstars changed the title empty_item_single_line should apply to trait definitions as well empty_item_single_line should apply to trait definitions as well Oct 28, 2021
@nebulaeandstars
Copy link
Author

Update: This seems to be a problem with AlwaysNextLine rather than empty_item_single_line. You can remove the latter from rustfmt.toml for identical results.

@calebcartwright
Copy link
Member

Thanks for the report. Seems the handling of brace_style when AlwaysNextLine isn't accounting for the configured empty_item_single_line value (which is true by default)

For anyone interested in working on this, suspect you'll probably want to start somewhere around here:

rustfmt/src/items.rs

Lines 1125 to 1172 in bc46af9

match context.config.brace_style() {
_ if last_line_contains_single_line_comment(&result)
|| last_line_width(&result) + 2 > context.budget(offset.width()) =>
{
result.push_str(&offset.to_string_with_newline(context.config));
}
BraceStyle::AlwaysNextLine => {
result.push_str(&offset.to_string_with_newline(context.config));
}
BraceStyle::PreferSameLine => result.push(' '),
BraceStyle::SameLineWhere => {
if result.contains('\n')
|| (!generics.where_clause.predicates.is_empty() && !trait_items.is_empty())
{
result.push_str(&offset.to_string_with_newline(context.config));
} else {
result.push(' ');
}
}
}
result.push('{');
let block_span = mk_sp(generics.where_clause.span.hi(), item.span.hi());
let snippet = context.snippet(block_span);
let open_pos = snippet.find_uncommented("{")? + 1;
let outer_indent_str = offset.block_only().to_string_with_newline(context.config);
if !trait_items.is_empty() || contains_comment(&snippet[open_pos..]) {
let mut visitor = FmtVisitor::from_context(context);
visitor.block_indent = offset.block_only().block_indent(context.config);
visitor.last_pos = block_span.lo() + BytePos(open_pos as u32);
for item in trait_items {
visitor.visit_trait_item(item);
}
visitor.format_missing(item.span.hi() - BytePos(1));
let inner_indent_str = visitor.block_indent.to_string_with_newline(context.config);
result.push_str(&inner_indent_str);
result.push_str(visitor.buffer.trim());
result.push_str(&outer_indent_str);
} else if result.contains('\n') {
result.push_str(&outer_indent_str);
}
result.push('}');

The format_impl function in that same file may serve as a useful reference for how to account for those. When adding tests, please be sure to include a combination of cases that include where empty_item_single_line is true with brace_style = AlwaysNextLine as well as one where empty_item_single_line is false

@calebcartwright calebcartwright added good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted only-with-option requires a non-default option value to reproduce labels Oct 30, 2021
@mujpao
Copy link
Contributor

mujpao commented Nov 1, 2021

Hi. Can I try this one?

@calebcartwright
Copy link
Member

Hi. Can I try this one?

Absolutely!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted only-with-option requires a non-default option value to reproduce
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants