Warn on line overflow when macro formatting fails

[smoelius]

Fixes #5700 by removing these lines:

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()),
	));

Re:

more work will be needed to understand how removing those lines affect macro formatting failures.

I'm not sure what's required here.

All of rustfmt's tests pass with those lines removed.

This change will require changes to Clippy. At least the following files are affected:

$ find ../rust-clippy -name '*.rs' | while read X; do cargo run --bin rustfmt -- --config-path ../rust-clippy/rustfmt.toml --check "$X" 2>/dev/null || echo "$X"; done
../rust-clippy/tests/ui-internal/check_clippy_version_attribute.rs
../rust-clippy/clippy_lints/src/lib.rs
../rust-clippy/clippy_lints/src/let_underscore.rs
../rust-clippy/clippy_lints/src/utils/internal_lints/lint_without_lint_pass.rs
../rust-clippy/clippy_lints/src/utils/internal_lints.rs
../rust-clippy/clippy_lints/src/utils/mod.rs
../rust-clippy/clippy_lints/src/lib.deprecated.rs
../rust-clippy/clippy_lints/src/derive.rs
../rust-clippy/clippy_lints/src/types/mod.rs

Most of the affected lines appear to be declare_lint_pass! or impl_lint_pass! invocations, e.g.:
https://github.com/rust-lang/rust-clippy/blob/113c704d225c63c1a0eec29cfa9478b7537e7d73/clippy_lints/src/let_underscore.rs#L131
I would be happy to submit the PR to Clippy to fix those files.

Beyond Clippy, there do appear to be repos that use error_on_line_overflow=true "in the wild":
https://grep.app/search?q=error_on_line_overflow%20%2A%3D%20%2Atrue&regexp=true&filter[lang][0]=TOML

But since this an opt-in feature that is explicitly unstable, maybe it's okay to change its behavior(?).

r? @ytmimi

[ytmimi]

@smoelius thanks for the PR. I haven't reviewed this yet but I wanted to quickly address a few things:

more work will be needed to understand how removing those lines affect macro formatting failures.

I'm not sure what's required here.

As mentioned in #5700 (comment) PR #3768 originally introduced these changes. We need to make sure we understand why that PR added those lines so we can determine if it's safe to just outright remove those lines.

This change will require changes to Clippy. At least the following files are affected:

The clippy repo is independant from rustfmt and not under the per-view of the rustfmt team. Feel free to open up a follow up PR there if you'd like but clippy isn't relevant here.

Most of the affected lines appear to be declare_lint_pass! or impl_lint_pass!.

Just want to call out that rustfmt can't format those macros because they don't currently parse, which is related to why adding those lines to the skip_list prevented rustfmt from emitting error messages.

Beyond Clippy, there do appear to be repos that use error_on_line_overflow=true "in the wild":
https://grep.app/search?q=error_on_line_overflow%20%2A%3D%20%2Atrue&regexp=true&filter[lang][0]=TOML

But since this an opt-in feature that is explicitly unstable, maybe it's okay to change its behavior(?).

error_on_line_overflow is an unstable configuration option, and in my opinion it's fine that we're fixing a bug that would cause rustfmt to emit additional error messages.

[ytmimi]

why is this commented out?

[smoelius]

@ytmimi I incorporated your feedback. I agree it is much cleaner without the config file.

I will try to look into #3768.

In the meantime, I have converted the PR to a draft, since, e.g., @calebcartwright wants time to review the issue.

[ytmimi]

@smoelius Thanks for continuing to help look into this. Based on #5700 (comment), if we decide that the current behavior is accurate, then the simplest solution is to update the docs for error_on_line_overflow to mention that we won't alert users when unformattable macros overflow.

[smoelius]

Do you have a sense for how hard it would be to address the max_width=100 issue?

When you wrote:

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.

that suggested to me the fix might be non-trivial.

[ytmimi]

Do you have a sense for how hard it would be to address the max_width=100 issue?

I think only the FmtVisitor has enough info about the current line number and last byte position of the formatted code. That being said I don't know how involved it would be to properly map lines of text from the input to lines of text in the output.