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

[unstable option] error_on_line_overflow #3391

Open
scampi opened this issue Feb 13, 2019 · 5 comments
Open

[unstable option] error_on_line_overflow #3391

scampi opened this issue Feb 13, 2019 · 5 comments
Labels
unstable option tracking issue of an unstable option

Comments

@scampi
Copy link
Contributor

scampi commented Feb 13, 2019

Tracking issue for error_on_line_overflow

@scampi scampi added the unstable option tracking issue of an unstable option label Feb 13, 2019
@nrc
Copy link
Member

nrc commented Mar 8, 2021

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
Copy link
Member

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
Copy link
Contributor

enterprisey commented May 8, 2023

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
Copy link
Member

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
Copy link

Veetaha commented Dec 22, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unstable option tracking issue of an unstable option
Projects
None yet
Development

No branches or pull requests

5 participants