Fix struct comment_width off-by-one error

[smoelius]

Fixes #6180.

The bug

The bug originates here, in rewrite_aligned_items_inner:

rustfmt/src/vertical.rs

Line 216 in 7852ca3

let item_shape = Shape::indented(offset, context.config).sub_width_opt(1)?;

Note the use of sub_width_opt(1), which reserves space for the item separator (e.g., a comma).

That declared item_shape is used to construct a ListFormatting struct, which is used in a call to write_list:

rustfmt/src/vertical.rs

Lines 265 to 269 in 7852ca3

	let fmt = ListFormatting::new(item_shape, context.config)
	.tactic(tactic)
	.trailing_separator(separator_tactic)
	.preserve_newline(true);
	write_list(&items, &fmt).ok()

Note that item_shape populates the shape field of the ListFormatting struct via ListFormatting::new:

rustfmt/src/lists.rs

Lines 38 to 51 in 7852ca3

	pub(crate) fn new(shape: Shape, config: &'a Config) -> Self {
	ListFormatting {
	tactic: DefinitiveListTactic::Vertical,
	separator: ",",
	trailing_separator: SeparatorTactic::Never,
	separator_place: SeparatorPlace::Back,
	shape,
	ends_with_newline: true,
	preserve_newline: false,
	nested: false,
	align_comments: true,
	config,
	}
	}

Within the body of write_list, there is this code:

rustfmt/src/lists.rs

Lines 369 to 370 in 7852ca3

	let comment =
	rewrite_comment(comment, block_mode, formatting.shape, formatting.config)?;

But recall that formatting.shape = item_shape, which already (and incorrectly for a comment) has space reserved for the separator, hence, the off-by-one error.

The fix

The fix is to add an additional, optional field, pre_comment_shape, to ListFormatting. This additional field does not reserve space for the separator, and thus avoids the off-by-one error.

In particular, the call to rewrite_comment in write_list now looks like this:

            let comment_shape = formatting.pre_comment_shape.unwrap_or(formatting.shape);
            let comment = rewrite_comment(comment, block_mode, comment_shape, formatting.config)?;

The field is optional because not all code paths that construct a ListFormatting use it. Specifically, pre_comment_shape is set in rewrite_aligned_items_inner, but it is not set at any of ListFormatting::new's other call sites.

Tests

A test case was added to resemble the one in #6180. However, it was modified to obtain 99-, 100-, and 101-column boundary coverage.

Also, one other existing test case had to be modified, as it suffered from the same bug. Specifically, this code:

rustfmt/tests/target/configs/struct_field_align_threshold/20.rs

Lines 323 to 325 in 7852ca3

	// Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec a diam lectus. Sed sit
	// amet ipsum mauris. Maecenas congue ligula ac quam viverra nec consectetur ante
	// hendrerit. Donec et mollis dolor.

was reflowed like this:

        // Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec a diam lectus. Sed sit
        // amet ipsum mauris. Maecenas congue ligula ac quam viverra nec consectetur ante hendrerit.
        // Donec et mollis dolor.

What this PR does not do

This PR's test case uses a comment_width equal to the default max_width (i.e., 100).

By doing so, this PR does not attempt to establish a policy for when comment_width < max_width, for example.

This choice was intentional, as I consider such policy decisions out of scope for this PR.

LLM disclosure

Codex was used to diagnose the bug and implement the fix, including comments I asked it to add to the code; however, all such comments look reasonable to me.

This PR description was written entirely by me (@smoelius).

[ytmimi]

List formatting is used all over the codebase. Does this same off-by-one error exist elsewhere?

[smoelius]

List formatting is used all over the codebase. Does this same off-by-one error exist elsewhere?

I cannot say confidently. The use of sub_width_opt before ListFormatting::new does seem common, and I think auditing all such occurrences would be difficult.

Evidence

$ grep -nrI '\.sub_width_opt\|ListFormatting::new' .
./src/closures.rs:338:    let fmt = ListFormatting::new(param_shape, context.config)
./src/imports.rs:1022:            .sub_width_opt(1)
./src/imports.rs:1062:    let fmt = ListFormatting::new(nested_shape, context.config)
./src/imports.rs:1114:                        .and_then(|s| s.sub_width_opt(1))
./src/overflow.rs:389:            .and_then(|shape| shape.sub_width_opt(1))
./src/overflow.rs:661:        let fmt = ListFormatting::new(self.nested_shape, self.context.config)
./src/types.rs:403:        let fmt = ListFormatting::new(list_shape, context.config)
./src/types.rs:938:                if let Some(sh) = shape.sub_width_opt(2) {
./src/pairs.rs:197:        .and_then(|s| s.sub_width_opt(pp.suffix.len()))
./src/patterns.rs:110:                let fmt = ListFormatting::new(shape, context.config)
./src/vertical.rs:216:    let item_shape = Shape::indented(offset, context.config).sub_width_opt(1)?;
./src/vertical.rs:265:    let fmt = ListFormatting::new(item_shape, context.config)
./src/matches.rs:241:    let fmt = ListFormatting::new(arm_shape, context.config)
./src/matches.rs:520:        .and_then(|shape| shape.sub_width_opt(comma.len()));
./src/matches.rs:586:            .and_then(|s| s.sub_width_opt(5));
./src/chains.rs:761:                    .sub_width_opt(almost_total)
./src/attr.rs:72:            .sub_width_opt(right)?,
./src/attr.rs:130:        .sub_width_opt("()]".len())?;
./src/attr.rs:146:    let fmt = ListFormatting::new(argument_shape, context.config)
./src/items.rs:646:        let shape = self.shape().sub_width_opt(2)?;
./src/items.rs:647:        let fmt = ListFormatting::new(shape, self.config)
./src/items.rs:677:            field.attrs.rewrite(&context, shape.sub_width_opt(1)?)?
./src/items.rs:680:        let shape = shape.sub_width_opt(1)?;
./src/items.rs:1495:        Shape::indented(offset.block_indent(context.config), context.config).sub_width_opt(1)?,
./src/items.rs:1635:        let shape = Shape::indented(offset, context.config).sub_width_opt(1)?;
./src/items.rs:2921:    let fmt = ListFormatting::new(Shape::legacy(budget, indent), context.config)
./src/items.rs:3184:    let fmt = ListFormatting::new(shape, context.config)
./src/items.rs:3264:    let fmt = ListFormatting::new(Shape::legacy(budget, offset), context.config)
./src/expr.rs:662:                IndentStyle::Visual => shape.shrink_left_opt(6).and_then(|s| s.sub_width_opt(2))?,
./src/expr.rs:2042:    let fmt = ListFormatting::new(nested_shape, context.config)
./src/expr.rs:2383:            .sub_width_opt(shape.indent.width()),
./src/expr.rs:2386:                .sub_width_opt(shape.rhs_overhead(context.config))
./src/macros.rs:489:    let fmt = ListFormatting::new(arm_shape, context.config)
./src/shape.rs:253:        self.sub_width_opt(delta)
./src/shape.rs:290:        self.add_offset(delta).sub_width_opt(delta)
./src/reorder.rs:76:    let fmt = ListFormatting::new(shape, context.config)
./src/visitor.rs:337:                                .sub_width_opt(offset_len)

However, I consider the bug described in #6180 to be inherent to rewrite_aligned_items_inner, not list formatting, per se.

Having said that, rewrite_aligned_items_inner is called by rewrite_with_alignment, and rewrite_with_alignment is called from three places:

• from format_struct_struct:

  rustfmt/src/items.rs

  Line 1492 in 7852ca3

  let items_str = rewrite_with_alignment(

• from rewrite_struct_lit:

  rustfmt/src/expr.rs

  Line 1820 in 7852ca3

  rewrite_with_alignment(

• and from rewrite_with_alignment itself (i.e., recursive call):

  rustfmt/src/vertical.rs

  Line 178 in 7852ca3

  let rest_str = rewrite_with_alignment(rest, context, shape, rest_span, one_line_width)?;

The test case added by this PR covers the first call site.

The test case that had to be adjusted fell out of the second call site.

If you would prefer, I could add a more thorough test for the second call site, one that resembles the one added by this PR with 99-, 100-, and 101-column boundary coverage.