rustfmt icon indicating copy to clipboard operation
rustfmt copied to clipboard

Cannot set granular width configuration settings to 100% if `max_width = 100`

Open tcharding opened this issue 3 years ago • 9 comments
trafficstars

Thanks for your work on rustfmt!

The granular width configuration settings are documented to be percentages of max_width (e.g. chain_width). However when max_width is set to 100 setting their values to 100 causes a warning

`fn_call_width` cannot have a value that exceeds `max_width`. `fn_call_width` will be set to the same value as `max_width`

The error message is both surprising and incorrect since fn_call_width = 100 is not a value that exceeds max_width.

This does not happen if max_width is 99 or 101.

The exact config options that trigger this bug are:

max_width = 100
use_small_heuristics = "Off"
fn_call_width = 100
$ rustfmt --version
rustfmt 1.4.38-stable (fe5b13d 2022-05-18)

Edit by @ytmimi (add a minimal reproducible code snippet):
macro_rules! foo {
    () => {};
}

tcharding avatar Jun 22 '22 22:06 tcharding

Thanks for the report!

Just as a heads up, when filing an issue please try to include a minimum code example that can be used to reproduce the issue along with any configuration options needed to produce the issue. It was a little tricky to track down since I tried running rustfmt on a few files and I couldn't reproduce the error until I ran rustfmt against src/utils.rs. Doing so the error is emitted 4 times.

Running rustfmt with debug logging enabled (RUSTFMT_LOG=rustfmt=DEBUG rustfmt --config-path=issue_5404.toml --emit stdout src/utils.rs) reveals that this error is always emitted after calling replace_names:

[2022-06-28T17:05:55Z DEBUG rustfmt_nightly::macros] replace_names `zsp.source_callsite()` {"$sp": "zsp"}
`fn_call_width` cannot have a value that exceeds `max_width`. `fn_call_width` will be set to the same value as `max_width`
[2022-06-28T17:05:55Z DEBUG rustfmt_nightly::macros] replace_names `!zself.config.file_lines().is_all()
                && !zself
                    .config
                    .file_lines()
                    .intersects(&zself.parse_sess.lookup_line_range(zspan))` {"$span": "zspan", "$self": "zself"}
`fn_call_width` cannot have a value that exceeds `max_width`. `fn_call_width` will be set to the same value as `max_width`
[2022-06-28T17:05:55Z DEBUG rustfmt_nightly::macros] replace_names `if out_of_file_lines_range!(zself, zspan) {
                return None;
            }` {"$self": "zself", "$span": "zspan"}
`fn_call_width` cannot have a value that exceeds `max_width`. `fn_call_width` will be set to the same value as `max_width`
[2022-06-28T17:05:55Z DEBUG rustfmt_nightly::macros] replace_names `if out_of_file_lines_range!(zself, zspan) {
                zself.push_rewrite(zspan, None);
                return;
            }` {"$span": "zspan", "$self": "zself"}
`fn_call_width` cannot have a value that exceeds `max_width`. `fn_call_width` will be set to the same value as `max_width`

replace_names only gets called in MacroBranch::rewrite

https://github.com/rust-lang/rustfmt/blob/c4416f20dcaec5d93077f72470e83e150fb923b1/src/macros.rs#L483-L486

And on line 1230 we adjust the max_width, which is where the issue is occurring.

https://github.com/rust-lang/rustfmt/blob/c4416f20dcaec5d93077f72470e83e150fb923b1/src/macros.rs#L1216-L1230

Config::set returns a ConfigSetter, and trying to set any of the width settings results in calling Config::set_heuristics.

https://github.com/rust-lang/rustfmt/blob/c4416f20dcaec5d93077f72470e83e150fb923b1/src/config/config_type.rs#L89-L103

Config::set_heuristics calls Config::set_width_heuristics.

https://github.com/rust-lang/rustfmt/blob/c4416f20dcaec5d93077f72470e83e150fb923b1/src/config/config_type.rs#L373-L381

Config::set_width_heuristics pretty much goes through all the width settings one by one and calls the get_width_value closure on each, leading to the error that you're seeing:

https://github.com/rust-lang/rustfmt/blob/c4416f20dcaec5d93077f72470e83e150fb923b1/src/config/config_type.rs#L294-L314

ytmimi avatar Jun 28 '22 17:06 ytmimi

fwiw I know this has been reported before, but can't find the prior issues atm

calebcartwright avatar Jun 28 '22 18:06 calebcartwright

Wow @ytmimi, thorough research/response, thank you. I'll add the minimal repro next time.

tcharding avatar Jun 28 '22 22:06 tcharding

Hi! I can also reproduce with the following:

.rustfmt.toml

max_width = 100 # Not strictly necessary to reproduce.
single_line_if_else_max_width = 100 # Any value above 92 produces the incorrect warning.

src/main.rs

macro_rules! anything { // Necessary to reproduce.
    () => {};
}

fn main() {
    let v = if a == b { String::new() } else { format!("anything") }; // Formatting does happen.
}

The following incorrect warning is then displayed:

$ cargo fmt
`single_line_if_else_max_width` cannot have a value that exceeds `max_width`. `single_line_if_else_max_width` will be set to the same value as `max_width`

Although the project seems correctly formatted. Was this minimal reproductible example the only blocking point?

iago-lito avatar Apr 25 '23 14:04 iago-lito

Sorry If I'm late to the game, but I'm finding some inconsistency in rustfmt 1.6.0-nightly. It seems that while the value is checked as a percentage, it might be taken as the absolute value. Of course at line_width=100 we don't notice the difference, but I'm getting a reformat under these conditions:

max_width = 120
struct_lit_width = 80

(So max struct literal would be 96) And the code

impl From<&Command> for CommandView {
    fn from(cmd: &Command) -> Self {
        Self { request_id: cmd.request_id, app_id: cmd.app_id, action: cmd.action.clone() }
    }
}

(line length: 92)

ferdonline avatar Oct 24 '23 14:10 ferdonline

@ferdonline Your issue seems unrelated to the original since it's not triggering any warning messages from rustfmt. I'm pretty sure we only set granular width settings as a percentage of max_width when setting use_small_heuristics. Can you please open a new issue, and in addition to including the code snippet can you also include a snippet of how rustfmt is modifying the code, thanks!

ytmimi avatar Oct 24 '23 14:10 ytmimi

we only set granular width settings as a percentage of max_width when setting use_small_heuristics

In that case then it's only a warning problem, as it's raised when

max_width = 120
struct_lit_width = 100

ferdonline avatar Oct 26 '23 14:10 ferdonline

@ferdonline Please open a new issue so we can discuss it separately. I'm unable to reproduce any warning messages when using rustfmt 1.6.0-nightly (1c05d50c 2023-10-21).

I'm running:

rustfmt +nightly-2023-10-22 --config=max_width=120,struct_lit_width=100

ytmimi avatar Oct 26 '23 15:10 ytmimi

I just experienced the same problem and put a few print statements into the rustfmt source code, which revealed the following behaviour:

// max_width = 100
// fn_call_width = 100

macro_rules! foo {
    () => {
        // max_width = 92
        // fn_call_width = 100

        // WARNING: `fn_call_width` cannot have a value that exceeds `max_width`. `fn_call_width` will be set to the same value as `max_width`
        // Which results in:
        // fn_call_width = 92
    };
}

Notice that rustfmt decreases max_width as it enters deeper scopes/nesting, but fn_call_width remains the same. I guess this saturation is actually desirable behaviour, but getting the context-less warning message causes more confusion than anything.

Interestingly, if you add an extra pair of brackets, max_width in the nested scope will be 96 instead of 92:

// max_width = 100
// fn_call_width = 100

macro_rules! foo {
    () => {{
        // max_width = 96
        // fn_call_width = 100

        // WARNING: `fn_call_width` cannot have a value that exceeds `max_width`. `fn_call_width` will be set to the same value as `max_width`
        // Which results in:
        // fn_call_width = 96
    }};
}

bergkvist avatar Feb 16 '24 00:02 bergkvist