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

Remove confusing warning message when granular width configs are saturated in nested scopes #6075

Closed

Conversation

bergkvist
Copy link

Related issues:

Summary

This PR removes an eprintln!() statement in rustfmt that likely causes more confusion than it provides benefits.

Motivation

The following warning message appears in stderr:

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

when using the following .rustfmt.toml:

max_width = 100
fn_call_width = 95

and attempting to format this code:

macro_rules! foo {
    () => {};
}

This is very misleading, as max_width is specified as being greater than fn_call_width in .rustfmt.toml.

The actual reason this warning appears is that max_width in rustfmt is changed dynamically depending on scope/nesting.

Here is an example showing how max_width changes with the expected nesting level:

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

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

@ytmimi
Copy link
Contributor

ytmimi commented Feb 18, 2024

@bergkvist Thanks for the PR, but I'd like to keep the error message. It's useful when we create the top level config to let users know that they've misconfigured rustfmt. I also agree that it's unfortunate that the error pops up in nested contexts in a surprising way. I think a better approach to this would be to clamp all width configuration to the max_width if needed in nested contexts. Let me know if you have any interest in experimenting with this idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants