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 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 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
115 changes: 102 additions & 13 deletions src/imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use crate::config::{Edition, IndentStyle, StyleEdition};
use crate::lists::{
ListFormatting, ListItem, Separator, definitive_tactic, itemize_list, write_list,
};
use crate::rewrite::{Rewrite, RewriteContext, RewriteErrorExt, RewriteResult};
use crate::rewrite::{Rewrite, RewriteContext, RewriteError, RewriteErrorExt, RewriteResult};
use crate::shape::Shape;
use crate::sort::version_sort;
use crate::source_map::SpanUtils;
Expand Down Expand Up @@ -1111,8 +1111,8 @@ impl Rewrite for UseSegment {
// 1 = "{" and "}"
shape
.offset_left_opt(1)
.and_then(|s| s.sub_width_opt(1))
.unknown_error()?,
.unknown_error()?
.saturating_sub_width(1),
)?
}
})
Expand All @@ -1126,18 +1126,107 @@ impl Rewrite for UseTree {

// This does NOT format attributes and visibility or add a trailing `;`.
fn rewrite_result(&self, context: &RewriteContext<'_>, mut shape: Shape) -> RewriteResult {
let mut result = String::with_capacity(256);
let mut iter = self.path.iter().peekable();
while let Some(segment) = iter.next() {
let segment_str = segment.rewrite_result(context, shape)?;
result.push_str(&segment_str);
if iter.peek().is_some() {
result.push_str("::");
// 2 = "::"
shape = shape.offset_left(2 + segment_str.len(), self.span())?;
if context.config.style_edition() >= StyleEdition::Edition2024 {
fn proceed(
context: &RewriteContext<'_>,
span: &Span,
shape: &Shape,
curr_segment: &UseSegment,
curr_segment_is_allow_overflow: bool,
next_segment: Option<&&UseSegment>,
) -> Result<(String, Shape), RewriteError> {
let mut rewritten_segment = curr_segment.rewrite_result(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,
};
Comment on lines +1144 to +1147
Copy link
Contributor

@chansuke chansuke Dec 27, 2024

Choose a reason for hiding this comment

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

imho: It would be helpful to add a comment explaining why reserved_room_for_brace is calculated?

Copy link
Author

Choose a reason for hiding this comment

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

Added. Thanks!

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 Some(ret) = shape.offset_left_opt(rewritten_segment.len()) else {
return Err(RewriteError::ExceedsMaxWidth {
configured_width: shape.width,
span: span.clone(),
});
};
// Check that there is a room for the next "{". If not, return an error for
// retry with newline.
if ret.offset_left_opt(reserved_room_for_brace).is_none() {
return Err(RewriteError::ExceedsMaxWidth {
configured_width: shape.width,
span: span.clone(),
});
}
ret
};
Ok((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();
let span = self.span();
while let Some(segment) = iter.next() {
let allow_overflow = is_first;
is_first = false;
match proceed(context, &span, &shape, segment, allow_overflow, iter.peek()) {
Ok((rewritten_segment, next_shape)) => {
result.push_str(&rewritten_segment);
shape = next_shape;
continue;
}
Err(RewriteError::ExceedsMaxWidth { .. }) => {
// If the first `proceed()` failed with no room, retry with newline.
}
Err(e) => {
// Abort otherwise.
return Err(e);
}
}
result.push_str("\n");
result.push_str(&" ".repeat(shape.indent.block_indent + 4));
shape = shape_top_level.clone();
let allow_overflow = true;
let (rewritten_segment, next_shape) =
proceed(context, &span, &shape, segment, allow_overflow, iter.peek())?;
result.push_str(&rewritten_segment);
shape = next_shape;
}
Ok(result)
} else {
let mut result = String::with_capacity(256);
let mut iter = self.path.iter().peekable();
while let Some(segment) = iter.next() {
let segment_str = segment.rewrite_result(context, shape)?;
result.push_str(&segment_str);
if iter.peek().is_some() {
result.push_str("::");
// 2 = "::"
shape = shape
.offset_left_opt(2 + segment_str.len())
.max_width_error(shape.width, self.span())?;
}
}
Ok(result)
}
Ok(result)
}
}

Expand Down
11 changes: 11 additions & 0 deletions src/shape.rs
Original file line number Diff line number Diff line change
Expand Up @@ -286,10 +286,21 @@ impl Shape {
.ok_or_else(|| self.exceeds_max_width_error(span))
}

pub(crate) fn add_width(&self, width: usize) -> Shape {
Shape {
width: self.width + width,
..*self
}
}
Comment on lines +289 to +294
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 offset_left_opt(&self, delta: usize) -> Option<Shape> {
self.add_offset(delta).sub_width_opt(delta)
}

pub(crate) fn offset_left_maybe_overflow(&self, width: usize) -> Shape {
self.add_offset(width).saturating_sub_width(width)
}
Comment on lines +300 to +302
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
150 changes: 150 additions & 0 deletions tests/source/5131_crate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,153 @@ 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