False negative with error_on_line_overflow in Clippy source file

[smoelius]

The issue concerns this line:
https://github.com/rust-lang/rust-clippy/blob/98c4a49db8c305c468646715b773fa9b6ec7049a/clippy_lints/src/derive.rs#L350

The line is 130 characters long, which exceeds the max_width declared in the rustfmt.toml file:
https://github.com/rust-lang/rust-clippy/blob/98c4a49db8c305c468646715b773fa9b6ec7049a/rustfmt.toml#L1

Yet none of these commands produce a change or error:

cargo fmt
rustfmt clippy_lints/src/derive.rs
cargo +nightly fmt
rustfmt +nightly clippy_lints/src/derive.rs

(Re +nightly, recall Clippy uses a rust-toolchain file.)

[ytmimi]

Thanks for reaching out. Confirming I can reproduce this with rustfmt 1.5.2-nightly (34f9ca28 2023-02-16) and the following input snippet:

fn check_copy_clone<'tcx>(cx: &LateContext<'tcx>, item: &Item<'_>, trait_ref: &hir::TraitRef<'_>, ty: Ty<'tcx>) {
    let clone_id = match cx.tcx.lang_items().clone_trait() {
        Some(id) if trait_ref.trait_def_id() == Some(id) => id,
        _ => return,
    };
    let Some(copy_id) = cx.tcx.lang_items().copy_trait() else { return };
    let (ty_adt, ty_subs) = match *ty.kind() {
        // Unions can't derive clone.
        ty::Adt(adt, subs) if !adt.is_union() => (adt, subs),
        _ => return,
    };
    // If the current self type doesn't implement Copy (due to generic constraints), search to see if
    // there's a Copy impl for any instance of the adt.
    if !is_copy(cx, ty) {
        if ty_subs.non_erasable_generics().next().is_some() {
            let has_copy_impl = cx.tcx.all_local_trait_impls(()).get(&copy_id).map_or(false, |impls| {
                impls
                    .iter()
                    .any(|&id| matches!(cx.tcx.type_of(id).subst_identity().kind(), ty::Adt(adt, _) if ty_adt.did() == adt.did()))
            });
            if !has_copy_impl {
                return;
            }
        } else {
            return;
        }
    }
    // Derive constrains all generic types to requiring Clone. Check if any type is not constrained for
    // this impl.
    if ty_subs.types().any(|ty| !implements_trait(cx, ty, clone_id, &[])) {
        return;
    }
    // `#[repr(packed)]` structs with type/const parameters can't derive `Clone`.
    // https://github.com/rust-lang/rust-clippy/issues/10188
    if ty_adt.repr().packed()
        && ty_subs
            .iter()
            .any(|arg| matches!(arg.unpack(), GenericArgKind::Type(_) | GenericArgKind::Const(_)))
    {
        return;
    }

    span_lint_and_note(
        cx,
        EXPL_IMPL_CLONE_ON_COPY,
        item.span,
        "you are implementing `Clone` explicitly on a `Copy` type",
        Some(item.span),
        "consider deriving `Clone` or removing `Copy`",
    );
}

What's interesting is that with max_width=100 and error_on_line_overflow=true errors are emitted:

error[internal]: line formatted, but exceeded maximum width (maximum: 100 (see `max_width` option), found: 102)
  --> <stdin>:21:21:101
   |
21 |             let has_copy_impl = cx.tcx.all_local_trait_impls(()).get(&copy_id).map_or(false, |impls| {
   |                                                                                                     ^^
   |

error[internal]: line formatted, but exceeded maximum width (maximum: 100 (see `max_width` option), found: 130)
  --> <stdin>:24:24:101
   |
24 |                     .any(|&id| matches!(cx.tcx.type_of(id).subst_identity().kind(), ty::Adt(adt, _) if ty_adt.did() == adt.did()))
   |                                                                                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |

warning: rustfmt has failed to format. See previous 2 errors.

[ytmimi]

linking tracking issue for error_on_line_overflow #3391

[ytmimi]

I did some digging and I've figured out why we're not emitting a max_width error when max_width=120 is set, and I also figured out why we emit an error when max_width=100 is set.

The short answer is we're failing to emit an error because rustfmt doesn't currently support formatting matches! macros when they have match guard's (#5547).

Failing to format the macro leads to rustfmt adding the macro to the skipped_range list in return_macro_parse_failure_fallback:

rustfmt/src/macros.rs

Lines 138 to 141 in 34f9ca2

	context.skipped_range.borrow_mut().push((
	context.parse_sess.line_of_byte_pos(span.lo()),
	context.parse_sess.line_of_byte_pos(span.hi()),
	));

which is behavior that was added by PR #3768.

Then when rustfmt gets to the point where it determines if it should emit a max_width error it decides to ignore it since it thinks the macro call was skipped via the #[rustfmt::skip] attribute instead of a macro formatting failure.

rustfmt/src/formatting.rs

Lines 551 to 560 in 34f9ca2

	// Check for any line width errors we couldn't correct.
	let error_kind = ErrorKind::LineOverflow(self.line_len, self.config.max_width());
	if self.line_len > self.config.max_width()
	&& !self.is_skipped_line()
	&& self.should_report_error(kind, &error_kind)
	{
	let is_string = self.current_line_contains_string_literal;
	self.push_err(error_kind, kind.is_comment(), is_string);
	}
	}

It would seem that PR #3768 extended the use case for skipped_range, however the doc comment makes it clear that it was only intended to be used for #[rustfmt::skip] annotations:

rustfmt/src/visitor.rs

Lines 83 to 85 in 34f9ca2

	/// List of 1-based line ranges which were annotated with skip
	/// Both bounds are inclusifs.
	pub(crate) skipped_range: Rc<RefCell<Vec<(usize, usize)>>>,

---

So why do we emit an error message when max_width=100?

It turns out that when we add the matches! macro to skip_ranges back in return_macro_parse_failure_fallback we add the line range of the macro from the source file, not the line range after formatting.

In both the max_width=120 and max_width=100 case we push (19, 19) into the skipped range list (assuming you run rustfmt directly on the input snippet from my previous comment), but after formatting with max_width=100 rustfmt has less room to work with on each line so some lines that could be written on a single line with max_width=120 now get formatted over multiple lines.

This offsets the matches! line to line 24, and when rustfmt checks if line 24 was skipped it finds that it was not and decides to emits the error message.

[smoelius]

Wow. That you put so much work into this in such a short period of time is, frankly, awe inspiriing, Thanks tremendously, @ytmimi!

[ytmimi]

@smoelius thanks for the kind words 😁 It definitely helps that I knew where to start looking.

Now that we understand the underlying issue we can try to come up with a solution. It may be as simple as removing the lines in return_macro_parse_failure_fallback, but more work will be needed to understand how removing those lines affect macro formatting failures. We may have to come up with a completely different solution. Unfortunately I don't have the bandwidth to do a deep dive into this right now, but I'm happy to help anyone who wants to take this on.

[smoelius]

It turns out that when we add the matches! macro to skip_ranges back in return_macro_parse_failure_fallback we add the line range of the macro from the source file, not the line range after formatting.

It may be as simple as removing the lines in return_macro_parse_failure_fallback...

I would have expected you to say: changing that code to insert the line range after formatting.

Is that not desired? Could you briefly explain why?

[ytmimi]

I would have expected you to say: changing that code to insert the line range after formatting.
Is that not desired? Could you briefly explain why?

@smoelius Just to be clear I was suggesting we try to remove these lines of code altogether:

rustfmt/src/macros.rs

Lines 138 to 141 in 34f9ca2

	context.skipped_range.borrow_mut().push((
	context.parse_sess.line_of_byte_pos(span.lo()),
	context.parse_sess.line_of_byte_pos(span.hi()),
	));

The false positive that you've pointed out in this issue is directly caused by rustfmt inserting line ranges into the skip_range list, so we definitely can't solve this issue by adjusting the line range we insert into skip_ranges. Additionally, I don't think we have enough information at the time of calling retrun_macro_parse_failure_fallback to determine how the lines from the input map to lines in the output.

[smoelius]

Thank you, @ytmimi. Your explanation makes sense to me.

[calebcartwright]

I'd like to request that we hold off on changing rustfmt behavior for the moment as I haven't had the bandwidth to process this thread yet.

Salient tl;dr, however, is that rustfmt intentionally handles macros differently and it's correct/intentional/desirable behavior that rustfmt does not error in cases where an unformattable macro exceeds width boundaries.

We can certainly discuss whether we want to change that, but I don't think that's something we'll be able to thoroughly consider any time in the near future

[ytmimi]

@calebcartwright if the current behavior is correct, then we've got a separate issue to deal with that I highlight in the second half of #5700 (comment) where I answer the question So why do we emit an error message when max_width=100?.