Skip to content

handle long param attributes #4047

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

Merged
merged 1 commit into from
Feb 18, 2020
Merged

handle long param attributes #4047

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion rustfmt-core/rustfmt-lib/src/spanned.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ impl Spanned for ast::Arm {
impl Spanned for ast::Param {
fn span(&self) -> Span {
if crate::items::is_named_param(self) {
mk_sp(self.pat.span.lo(), self.ty.span.hi())
mk_sp(crate::items::span_lo_for_param(self), self.ty.span.hi())
} else {
self.ty.span
}
Expand Down
4 changes: 4 additions & 0 deletions rustfmt-core/rustfmt-lib/tests/source/issue_4032.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
fn a1(#[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa] a: u8) {}
fn b1(#[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa] bb: u8) {}
fn a2(#[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa] a: u8) {}
fn b2(#[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa] bb: u8) {}
16 changes: 16 additions & 0 deletions rustfmt-core/rustfmt-lib/tests/target/issue_4032.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
fn a1(
#[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa] a: u8,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, if the total width exceeds the max width, then the attribute and the parameter should be put on different lines:

fn a1(
    #[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa]
    a: u8,
) {
}

@calebcartwright It would be great if you can fix this in this PR, though I am happy to merge this PR as is as entirely removing the attribute is much worse!

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, if the total width exceeds the max width, then the attribute and the parameter should be put on different lines

Agreed. AFAICT, the attribute and param will be split as desired in most cases. Based on some testing with this, I believe the following circumstances have to all be true in order for them to not be split:

  • only one attribute on the param
  • neither the attribute nor the param individually exceed the max width
  • param is named and not an empty infer/closure param
  • the combination of the formatted attributed and the param pattern is under the max width, but the addition of the colon and param type causes the total line length to exceed max width

That makes me think it's a bit of an edge case, but happy to try to find a way to handle it!

Copy link
Contributor

Choose a reason for hiding this comment

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

@calebcartwright Thank you for investigating the issue further! I agree that this is an edge case and not problematic enough to block this PR. As such, I will merge this PR as is. If you find it interesting and fun, you are more than welcome to send the follow-up PR to fix this corner case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've opened #4059 in case anyone wants to work on it and to make sure we don't lose track of it. I'll try to circle back to it eventually if no one else does, but need to finish some other issues (for rustfmt 2.x, etc.) first 👍

) {
}
fn b1(
#[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa] bb: u8,
) {
}
fn a2(
#[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa] a: u8,
) {
}
fn b2(
#[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa] bb: u8,
) {
}