[unstable option] error_on_line_overflow

[scampi]

Tracking issue for error_on_line_overflow

[nrc]

I would love to have either a warn_on_line_overflow option or to make error_on_line_overflow an enum with Silent, Error, and Warn options.

[calebcartwright]

I really like that idea. I've been struggling lately with the binary nature of the current option, as it (and the default value) are a common source of folks being unaware that there's portions of their codebase that rustfmt isn't able to format at all, but in plenty of cases it wouldn't be viable to make that a hard error either.

I'm leaning towards the multi-variant approach, perhaps with a different name to avoid any confusion/binary connotations. If we go that route we can soft-deprecate the current error_on_line_overflow with an auto-mapping to the corresponding variant on the new option and print a config warning message.

[enterprisey]

I would be interested in working on this if the maintainers still think it's worth doing. I would do the warn_on_line_overflow thing. However, I'm not sure what the difference is between a warning and an error in this context - that is, what the difference between the Error and Warn behaviors would be. I poked around the repo some, and I can't spot any difference - indeed, I can't find the term "warn" even being used that much, besides the warnings about using old config parameter names. Would it be possible to get more details?

[calebcartwright]

I would be interested in working on this if the maintainers still think it's worth doing. I would do the warn_on_line_overflow thing. However, I'm not sure what the difference is between a warning and an error in this context - that is, what the difference between the Error and Warn behaviors would be. I poked around the repo some, and I can't spot any difference - indeed, I can't find the term "warn" even being used that much, besides the warnings about using old config parameter names. Would it be possible to get more details?

That's great, thanks for the offer this is absolutely something we still (desperately) need!

As far as implementation goes, I definitely want to go the single option with multiple variant routes; I think the config surface and expected behavior would be too clunky if we had two separate options that essentially control the "level" of the same thing (e.g. if both are set with true what exactly does the user expect/what should rustfmt do?).

At a high level the work will involve:

• Coming up with a new name for the multi-variant config option (e.g. line_overflow_behavior or line_overflow_level but we can bikeshed on this as we go)
• Create the new option in https://github.com/rust-lang/rustfmt/tree/master/src/config
• soft-deprecating/renaming the existing option to automap to the new option (example of doing this can be found here https://github.com/rust-lang/rustfmt/pull/5387/files)
• adjust the various logic (and likely signature return types, structures, etc.) to maintain the ability to display the text lines, but when the new option is set to Warn then rustfmt shouldn't exit with an error code. I'd recommend starting at the below function and then working back up the call stack to figure out what all needs to be adjusted to achieve this

rustfmt/src/formatting.rs

Lines 598 to 615 in a44c7ea

	fn should_report_error(&self, char_kind: FullCodeCharKind, error_kind: &ErrorKind) -> bool {
	let allow_error_report = if char_kind.is_comment()
	|| self.current_line_contains_string_literal
	|| error_kind.is_comment()
	{
	self.config.error_on_unformatted()
	} else {
	true
	};
	match error_kind {
	ErrorKind::LineOverflow(..) => {
	self.config.error_on_line_overflow() && allow_error_report
	}
	ErrorKind::TrailingWhitespace | ErrorKind::LostComment => allow_error_report,
	_ => true,
	}
	}

[Veetaha]

I would like to bring up the problem that I have with this option.
I already have a large codebase where enabling this option results in literally 30 000 lines of rustfmt errors printed.

This is because developers have been unaware that rustfmt is unable to get their code under the default of 100 max_width. So with the current state of this unstable option it is impossible to use it on our code base.

I would like to propose that there should be a separate option that specifies the margin for line overflow error. For example, the default rustfmt tries to get all lines under 100 width, but if it can't then there should be, for example a line_overflow_margin = 120 that would not trigger an error_on_line_overflow if the overflow is within the range 100..=120, but if it is 121+ of width, then the error will be triggered.

This would allow us to set a higher margin for line overflow error to reduce the 30 000 lines of rustfmt errors to something manageable in our codebase and work from there incrementally reducing the margin and fixing our code to get a lower margin.

[Lorak-mmk]

Documentation for this option says:
"Error if Rustfmt is unable to get all lines within max_width, except for comments and string literals. If this happens, then it is a bug in Rustfmt."

Is it really? I tried running cargo fmt -- --config error_on_line_overflow=true --config format_strings=true to workaround #3863 (because --config flag for some reason allows unstable options) but it produced a lot of errors on the lines that do not contain comments / string literals. Is this expected or a bug?

To reproduce: checkout https://github.com/scylladb/scylla-rust-driver and run cargo fmt -- --config error_on_line_overflow=true --config format_strings=true.