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

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

Open
tcharding opened this issue Jun 22, 2022 · 9 comments
Labels
a-macros only-with-option requires a non-default option value to reproduce p-low

Comments

@tcharding
Copy link

tcharding commented Jun 22, 2022

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 {
    () => {};
}
@ytmimi
Copy link
Contributor

ytmimi commented Jun 28, 2022

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

rustfmt/src/macros.rs

Lines 483 to 486 in c4416f2

// Replaces `$foo` with `zfoo`. We must check for name overlap to ensure we
// aren't causing problems.
// This should also work for escaped `$` variables, where we leave earlier `$`s.
fn replace_names(input: &str) -> Option<(String, HashMap<String, String>)> {

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

rustfmt/src/macros.rs

Lines 1216 to 1230 in c4416f2

let (body_str, substs) = replace_names(old_body)?;
let has_block_body = old_body.starts_with('{');
let mut config = context.config.clone();
config.set().hide_parse_errors(true);
result += " {";
let body_indent = if has_block_body {
shape.indent
} else {
shape.indent.block_indent(&config)
};
let new_width = config.max_width() - body_indent.width();
config.set().max_width(new_width);

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

impl<'a> ConfigSetter<'a> {
$(
#[allow(unreachable_pub)]
pub fn $i(&mut self, value: $ty) {
(self.0).$i.2 = value;
match stringify!($i) {
"max_width"
| "use_small_heuristics"
| "fn_call_width"
| "single_line_if_else_max_width"
| "attr_fn_like_width"
| "struct_lit_width"
| "struct_variant_width"
| "array_width"
| "chain_width" => self.0.set_heuristics(),

Config::set_heuristics calls Config::set_width_heuristics.

fn set_heuristics(&mut self) {
let max_width = self.max_width.2;
match self.use_small_heuristics.2 {
Heuristics::Default =>
self.set_width_heuristics(WidthHeuristics::scaled(max_width)),
Heuristics::Max => self.set_width_heuristics(WidthHeuristics::set(max_width)),
Heuristics::Off => self.set_width_heuristics(WidthHeuristics::null()),
};
}

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:

fn set_width_heuristics(&mut self, heuristics: WidthHeuristics) {
let max_width = self.max_width.2;
let get_width_value = |
was_set: bool,
override_value: usize,
heuristic_value: usize,
config_key: &str,
| -> usize {
if !was_set {
return heuristic_value;
}
if override_value > max_width {
eprintln!(
"`{0}` cannot have a value that exceeds `max_width`. \
`{0}` will be set to the same value as `max_width`",
config_key,
);
return max_width;
}
override_value
};

@ytmimi ytmimi added only-with-option requires a non-default option value to reproduce a-macros labels Jun 28, 2022
@calebcartwright
Copy link
Member

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

@tcharding
Copy link
Author

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

@iago-lito
Copy link

iago-lito commented Apr 25, 2023

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?

@ferdonline
Copy link

ferdonline commented Oct 24, 2023

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)

@ytmimi
Copy link
Contributor

ytmimi commented Oct 24, 2023

@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!

@ferdonline
Copy link

ferdonline commented Oct 26, 2023

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

@ytmimi
Copy link
Contributor

ytmimi commented Oct 26, 2023

@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

@bergkvist
Copy link

bergkvist commented Feb 16, 2024

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
    }};
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-macros only-with-option requires a non-default option value to reproduce p-low
Projects
None yet
Development

No branches or pull requests

6 participants