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

fix #6164: Reduce format failure for non default imports_granularity #6165

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
75 changes: 68 additions & 7 deletions src/imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1070,7 +1070,7 @@ impl Rewrite for UseSegment {
context,
use_tree_list,
// 1 = "{" and "}"
shape.offset_left(1)?.sub_width(1)?,
shape.offset_left(1)?.saturating_sub_width(1),
)?,
})
}
Expand All @@ -1079,15 +1079,76 @@ impl Rewrite for UseSegment {
impl Rewrite for UseTree {
// This does NOT format attributes and visibility or add a trailing `;`.
fn rewrite(&self, context: &RewriteContext<'_>, mut shape: Shape) -> Option<String> {
fn proceed(
context: &RewriteContext<'_>,
shape: &Shape,
curr_segment: &UseSegment,
curr_segment_is_allow_overflow: bool,
next_segment: Option<&&UseSegment>,
) -> Option<(String, Shape)> {
let mut rewritten_segment = curr_segment.rewrite(context, shape.clone())?;
if next_segment.is_some() {
rewritten_segment.push_str("::");
}
let reserved_room_for_brace = match next_segment.map(|s| &s.kind) {
Some(UseSegmentKind::List(_)) => "{".len(),
_ => 0,
};
let next_shape = if matches!(&curr_segment.kind, UseSegmentKind::List(_)) {
// This is the last segment and we won't use `next_shape`. Return `shape`
// unchanged.
shape.clone()
} else if curr_segment_is_allow_overflow {
// If the segment follows `use ` or newline, force to consume the segment with
// overflow.

let s = shape.offset_left_maybe_overflow(rewritten_segment.len());
if s.width == 0 {
// We have to to commit current segment in this line. Make a room for next
// round.
s.add_width(reserved_room_for_brace)
} else {
s.clone()
}
} else {
let ret = shape.offset_left(rewritten_segment.len())?;
// Check that there is a room for the next "{". If not, return null for retry with
// newline.
ret.offset_left(reserved_room_for_brace)?;
ret
};
Some((rewritten_segment, next_shape))
}

let shape_top_level = shape.clone();
let mut result = String::with_capacity(256);
let mut is_first = true;
let mut iter = self.path.iter().peekable();
while let Some(segment) = iter.next() {
let segment_str = segment.rewrite(context, shape)?;
result.push_str(&segment_str);
if iter.peek().is_some() {
result.push_str("::");
// 2 = "::"
shape = shape.offset_left(2 + segment_str.len())?;
let allow_overflow = is_first;
is_first = false;
match proceed(context, &shape, segment, allow_overflow, iter.peek()) {
Some((rewritten_segment, next_shape)) => {
result.push_str(&rewritten_segment);
shape = next_shape;
continue;
}
None => (),
}
// If the first `proceed()` failed, retry with newline.
result.push_str("\n");
result.push_str(&" ".repeat(shape.indent.block_indent + 4));
shape = shape_top_level.clone();
let allow_overflow = true;
match proceed(context, &shape, segment, allow_overflow, iter.peek()) {
Some((rewritten_segment, next_shape)) => {
result.push_str(&rewritten_segment);
shape = next_shape;
}
// Give up to format.
None => {
return None;
}
}
}
Some(result)
Expand Down
11 changes: 11 additions & 0 deletions src/shape.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,13 @@ impl Shape {
})
}

pub(crate) fn add_width(&self, width: usize) -> Shape {
Shape {
width: self.width + width,
..*self
}
}
Comment on lines +245 to +250
Copy link
Contributor

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?

Copy link
Author

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:

  1. Use segment.rewrite(context, shape.infinite_width())?; at the TIMING1 instead.

We can't do that because it produces

use long_segment_loooooooooooooooooong::{Foo, Bar};
  1. Add a variant of UseSegment::rewirte() that can handle yet another argument should_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 shrink_left(&self, width: usize) -> Option<Shape> {
Some(Shape {
width: self.width.checked_sub(width)?,
Expand All @@ -254,6 +261,10 @@ impl Shape {
self.add_offset(width).sub_width(width)
}

pub(crate) fn offset_left_maybe_overflow(&self, width: usize) -> Shape {
self.add_offset(width).saturating_sub_width(width)
}
Comment on lines +264 to +266
Copy link
Contributor

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?

Copy link
Author

@kenoss kenoss May 21, 2024

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.


pub(crate) fn used_width(&self) -> usize {
self.indent.block_indent + self.offset
}
Expand Down
151 changes: 151 additions & 0 deletions tests/source/5131_crate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,154 @@ use foo::d::e;
use qux::h;
use qux::h as h2;
use qux::i;


mod indent4 {
use column_____________________________________________________________________________________102::{
Foo,
bar::Bar,
bar::baz::Baz,
Foo2,
bar::Bar2,
};

use column_______________________________________________________________________________096::{
Foo,
bar::Bar,
bar::baz::Baz,
Foo2,
bar::Bar2,
};

use column_________________________________________________________________________090::{
Foo,
bar::Bar,
bar::baz::Baz,
Foo2,
bar::Bar2,
};

use c012::c018::c024::c030::c036::c042::c048::c054::c060::c066::c072::c078::c084::c090::c096::c102::{
Foo,
bar::Bar,
bar::baz::Baz,
Foo2,
bar::Bar2,
};

use c012::c018::c024::c030::c036::c042::c048::c054::c060::c066::c072::c078::c084::c090::c096::{
Foo,
bar::Bar,
bar::baz::Baz,
Foo2,
bar::Bar2,
};

use c012::c018::c024::c030::c036::c042::c048::c054::c060::c066::c072::c078::c084::c090::{
Foo,
bar::Bar,
bar::baz::Baz,
Foo2,
bar::Bar2,
};

use c012::c018::c024::c030::c036::c042::c048::c054::c060::c066::c072::c078::c084::{
Foo,
bar::Bar,
bar::baz::Baz,
Foo2,
bar::Bar2,
};
}

use smithay::{
backend::renderer::element::{
default_primary_scanout_output_compare, utils::select_dmabuf_feedback, RenderElementStates,
},
delegate_compositor, delegate_data_control, delegate_data_device, delegate_fractional_scale,
delegate_input_method_manager, delegate_keyboard_shortcuts_inhibit, delegate_layer_shell,
delegate_output, delegate_pointer_constraints, delegate_pointer_gestures,
delegate_presentation, delegate_primary_selection, delegate_relative_pointer, delegate_seat,
delegate_security_context, delegate_shm, delegate_tablet_manager, delegate_text_input_manager,
delegate_viewporter, delegate_virtual_keyboard_manager, delegate_xdg_activation,
delegate_xdg_decoration, delegate_xdg_shell,
desktop::{
space::SpaceElement,
utils::{
surface_presentation_feedback_flags_from_states, surface_primary_scanout_output,
update_surface_primary_scanout_output, OutputPresentationFeedback,
},
PopupKind, PopupManager, Space,
},
input::{
keyboard::{Keysym, LedState, XkbConfig},
pointer::{CursorImageStatus, PointerHandle},
Seat, SeatHandler, SeatState,
},
output::Output,
reexports::{
calloop::{generic::Generic, Interest, LoopHandle, Mode, PostAction},
wayland_protocols::xdg::decoration::{
self as xdg_decoration,
zv1::server::zxdg_toplevel_decoration_v1::Mode as DecorationMode,
},
wayland_server::{
backend::{ClientData, ClientId, DisconnectReason},
protocol::{wl_data_source::WlDataSource, wl_surface::WlSurface},
Display, DisplayHandle, Resource,
},
},
utils::{Clock, Monotonic, Rectangle},
wayland::{
compositor::{get_parent, with_states, CompositorClientState, CompositorState},
dmabuf::DmabufFeedback,
fractional_scale::{
with_fractional_scale, FractionalScaleHandler, FractionalScaleManagerState,
},
input_method::{InputMethodHandler, InputMethodManagerState, PopupSurface},
keyboard_shortcuts_inhibit::{
KeyboardShortcutsInhibitHandler, KeyboardShortcutsInhibitState,
KeyboardShortcutsInhibitor,
},
output::{OutputHandler, OutputManagerState},
pointer_constraints::{
with_pointer_constraint, PointerConstraintsHandler, PointerConstraintsState,
},
pointer_gestures::PointerGesturesState,
presentation::PresentationState,
relative_pointer::RelativePointerManagerState,
seat::WaylandFocus,
security_context::{
SecurityContext, SecurityContextHandler, SecurityContextListenerSource,
SecurityContextState,
},
selection::data_device::{
set_data_device_focus, ClientDndGrabHandler, DataDeviceHandler, DataDeviceState,
ServerDndGrabHandler,
},
selection::{
primary_selection::{
set_primary_focus, PrimarySelectionHandler, PrimarySelectionState,
},
wlr_data_control::{DataControlHandler, DataControlState},
SelectionHandler,
},
shell::{
wlr_layer::WlrLayerShellState,
xdg::{
decoration::{XdgDecorationHandler, XdgDecorationState},
ToplevelSurface, XdgShellState, XdgToplevelSurfaceData,
},
},
shm::{ShmHandler, ShmState},
socket::ListeningSocketSource,
tablet_manager::{TabletManagerState, TabletSeatTrait},
text_input::TextInputManagerState,
viewporter::ViewporterState,
virtual_keyboard::VirtualKeyboardManagerState,
xdg_activation::{
XdgActivationHandler, XdgActivationState, XdgActivationToken, XdgActivationTokenData,
},
xdg_foreign::{XdgForeignHandler, XdgForeignState},
},
};
Loading
Loading