-
Notifications
You must be signed in to change notification settings - Fork 898
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
fix #6164: Reduce format failure for non default imports_granularity
#6165
base: master
Are you sure you want to change the base?
Conversation
tests/target/issue_3033.rs
Outdated
use dom::bindings::codegen::Bindings::BluetoothRemoteGATTServerBinding::BluetoothRemoteGATTServerBinding:: | ||
BluetoothRemoteGATTServerMethods; | ||
use dom::bindings::codegen::Bindings::BluetoothRemoteGATTServerBinding:: | ||
BluetoothRemoteGATTServerBinding::BluetoothRemoteGATTServerMethods; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exceeds 100 chars. It was preserved because the format failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this changes existing behavior these changes need to be version gated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry. I missed the comment. Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate you jumping in to work on this, though I'm hoping we can find a way to refactor and simplify the implementation.
I also want to be mindful of your time and say up front that it might be a while before I'm able to get around to a follow up review on this.
src/imports.rs
Outdated
// Try stacking the next segment, e.g. `use prev::next_segment::...` and | ||
// `use prev::{next, segment}`. | ||
let can_stack_with_constraint = (|| { | ||
// If the segment follows `use ` or `{`, force to consume the segment with overflow. | ||
if is_first { | ||
let mut chunk = segment.rewrite(context, shape.infinite_width())?; | ||
let next_shape = if iter.peek().is_some() { | ||
chunk.push_str("::"); | ||
shape.offset_left_maybe_overflow(chunk.len()) | ||
} else { | ||
shape.clone() | ||
}; | ||
Some((chunk, next_shape)) | ||
} else { | ||
// If the segment follows `use ` or newline, allow overflow by "{". | ||
let s = if prev_is_allow_overflow | ||
&& matches!(segment.kind, UseSegmentKind::List(_)) | ||
{ | ||
shape.add_width(1) | ||
} else { | ||
shape.clone() | ||
}; | ||
let mut chunk = segment.rewrite(context, s)?; | ||
let next_shape = match iter.peek().map(|s| &s.kind) { | ||
Some(UseSegmentKind::List(_)) => { | ||
chunk.push_str("::"); | ||
let ret = shape.offset_left(chunk.len())?; | ||
// Ensure that there is a room for the next "{". | ||
ret.offset_left(1)?; | ||
ret | ||
} | ||
Some(_) => { | ||
chunk.push_str("::"); | ||
shape.offset_left(chunk.len())? | ||
} | ||
None => shape.clone(), | ||
}; | ||
Some((chunk, next_shape)) | ||
} | ||
})(); | ||
match can_stack_with_constraint { | ||
Some((chunk, next_shape)) => { | ||
result.push_str(&chunk); | ||
shape = next_shape; | ||
prev_is_allow_overflow = is_first; | ||
is_first = false; | ||
} | ||
// If the next segment exceeds the given width, continue with newline. | ||
None => { | ||
let segment_str = segment.rewrite(context, shape)?; | ||
let mut chunk = format!( | ||
"{}{}", | ||
" ".repeat(shape.indent.block_indent + 4), | ||
segment_str | ||
); | ||
if iter.peek().is_some() { | ||
chunk.push_str("::"); | ||
} | ||
result.push_str("\n"); | ||
result.push_str(&chunk); | ||
shape = shape_top_level.offset_left_maybe_overflow(segment_str.len()); | ||
prev_is_allow_overflow = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked at this in too much detail, but my initial thought is that there is a lot of nested logic here that makes this hard to wrap my head around. I think I have a high level understanding of what the code is trying to do, but I'd love to see if you can refactor and simplify the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me think...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored. Looking ahead next + retry.
pub(crate) fn add_width(&self, width: usize) -> Shape { | ||
Shape { | ||
width: self.width + width, | ||
..*self | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain a little more why we needed to implement an add_width
method on the Shape
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider this case:
// With max width 40
// |
use long_segment_loooooooooooooooooong::{Foo, Bar};
// should be formated to
use long_segment_loooooooooooooooooong::{
Foo,
Bar,
};
Look at the UseTree::rewrite()
, especially
let s = if prev_is_allow_overflow
&& matches!(segment.kind, UseSegmentKind::List(_))
{
shape.add_width(1)
} else {
shape.clone()
};
Let's trace the behavior of the format above.
committed = "use "
// 35 = 40 - (4 + 1)
shape = {indent: 0, offset: 4, width: 35}
// len = 34
segment = "long_segment_loooooooooooooooooong"
// 34 + 2 exceeds 35, but the above segment should be stacked to the current line because this is the first segment
committed = "use long_segment_loooooooooooooooooong::"
// with overflow.
shape = {indent: 0, offset: 40, width: 0}
prev_is_allow_overflow = true
// TIMING1
segment = "{Foo, Bar}"
// Here, we must secure the room for "{" since `prev_is_allow_overflow == true`, or `segment.rewrite(context, shape)?` fails.
s = {indent: 0, offset: 40, width: 1}
committed = "use long_segment_loooooooooooooooooong::{\n Foo,\n Bar,\n}"
Alternative options:
- Use
segment.rewrite(context, shape.infinite_width())?;
at theTIMING1
instead.
We can't do that because it produces
use long_segment_loooooooooooooooooong::{Foo, Bar};
- Add a variant of
UseSegment::rewirte()
that can handle yet another argumentshould_commit_open_brace_even_if_no_width
.
It's not simple compared to adding Shape::add_width()
...
So, I added Shape::add_width()
.
pub(crate) fn offset_left_maybe_overflow(&self, width: usize) -> Shape { | ||
self.add_offset(width).saturating_sub_width(width) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, can you explain what offset_left_maybe_overflow
is for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar. There are the cases that we need to allow overflow temporarily. We can proceed Shape
with this method without fail.
Thank you for quick look! I'll rethink and refactor. I'll mention once I've done. |
@ytmimi Could you have a look? Thanks! |
I really appreciate you taking the time to make these changes. As I mentioned in #6165 (review) I likely won't have time to re-review this for a little while, and I'll follow up when I can |
np. Take your time. |
03c8503
to
435e3c6
Compare
24de098
to
8b634f2
Compare
Rebased and conflict resolved. Updated using |
This is a friendly ping. Three months passed. @ytmimi Could you have a look? Let me know if I can help. |
@kenoss I know it's been some time, but I unfortunately haven't had the bandwidth. Right now my main focus is getting rustfmt ready for the upcoming edition release, mentoring the GSoC project, triaging issues, and working on smaller bugfixes. I appreciate your patience on this. |
I understand your situation. (But... I wonder why don't you add more reviewers? It looks other PRs also are stuck with review.) OK. Then, I'll pause to make this PR fresh. Let me know when you can review. Then, I'll rebase it. |
``` $ RUSTFMT="./target/debug/rustfmt" cargo run --bin cargo-fmt -- --manifest-path ../../kenoss/sabiniwm/Cargo.toml ``` with rust-lang/rustfmt#6165.
…anularity` This patch reduces format failure for non default `imports_granularity` and correct the behavior around some edge cases. - Supports too long line of `use ...` that contains `{}`. - Fixes the width calculation of too long lines of `use ...`.
… do cargo run --bin rustfmt -- $f ; done
@ytmimi Friendly ping in 2024 year end. I happened to notice that you are reviewing some PRs. Do you mind to take time to review this PR? I rebased it onto near HEAD. (Commit 8a2c073 is not working on my environment. Rebased to HEAD~.) I replaced Thanks. |
@kenoss Thanks for taking the time to rebase the PR. At the moment this is a lower priority item for the team and I don't anticipate I'll get around to reviewing the implementation before the end of the year. It's hard to say when exactly I'll be able to revisit this one, but I'll be sure to reach out when I do. |
Thank you for taking time! FYI, it's a blocker for me to upstream it because I need it for CI. I don't want to diverge it. |
Fixed: #6164
This patch reduces format failure for non default
imports_granularity
and correct the behavior around some edge cases.use ...
that contains{}
.use ...
.