Fixes adjustment of max_width within macros

[csmulhern]

Fixes #5404.

Minimal example:

macro_rules! foo {
    () => {
        println!("Hello, world!");
    };
}

Without this change:

> cargo run --bin rustfmt -- --config-path=/dev/null --config=max_width=100 --config=fn_call_width=100 ~/Desktop/example.rs
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.02s
     Running `target/debug/rustfmt --config-path=/dev/null --config=max_width=100 --config=fn_call_width=100 /Users/cameron/Desktop/example.rs`
`fn_call_width` cannot have a value that exceeds `max_width`. `fn_call_width` will be set to the same value as `max_width`

With this change:

> cargo run --bin rustfmt -- --config-path=/dev/null --config=max_width=100 --config=fn_call_width=100 ~/Desktop/example.rs
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.03s
     Running `target/debug/rustfmt --config-path=/dev/null --config=max_width=100 --config=fn_call_width=100 /Users/cameron/Desktop/example.rs`

[csmulhern]

@ytmimi, could you take a look? Happy to make changes if necessary.

[csmulhern]

@Manishearth I see you merging some recent PRs. Do you know who can take a look at this?

[Manishearth]

@csmulhern So we're trying to help spread out the maintainer burden, but there are a lot of open PRs right now and we're still triaging them. Open PRs which change formatted code are tricky because it needs to be determined if it's a rbeaking change or not.

We'll get to them eventually, but no promises.

It will help if you show a before and after of the formatted code.

[Manishearth]

It seems like from the PR description that this doesn't actually impact formatting, this makes certain previously-unintentionally-invalid values of a config start working again.

[csmulhern]

Yes, this (mostly*) shouldn't impact formatting.

Today, we have a bunch of properties that control breakpoints for how long certain lines can be. This is primarily max_width, but also fn_call_width, attr_fn_like_width, struct_lit_width, struct_variant_width, array_width, chain_width, single_line_if_else_max_width. By default, these are all derived from max_width, due to use_small_heuristics defaulting to true.

In order to support formatting in macros, it looks like rustfmt just trims down max_width by the indentation of the macro arm body, and then runs formatting of the body with that adjusted config. The problem is, those other max_width style properties are not adjusted. So then when those values are read inside the formatting for the macro body, they can be larger than max_width, which triggers this error, and the values are clamped down to max_width. Note that if these values are unset, there's no issue, because it will just default to the heuristic value.

This change makes the reduction based on the indent symmetric across all of the max_width style properties, and adds a new method to ensure this is done universally.

* The edge case where this would affect formatting would be if e.g. max_width was set to 80, and fn_call_width was set to 76, and the macro body indent was 8. In today's world, fn_call_width in the macro body would be larger than the max_width within the macro body (72), and would get clamped down to 72. With this fix, both would be reduced by 8 (max_width 80 -> 72, fn_call_width 76 -> 68).

[csmulhern]

@Manishearth / @ytmimi, any feedback on this?

[csmulhern]

@jieyouxu do you know what the status is re: if these sort of PRs are ready for review? Is there anything I can do to help make this more reviewable?

[jieyouxu]

Is there anything I can do to help make this more reviewable?

It's largely (rustfmt) review bandwidth that's fairly scarce. I can do a re-review this weekend.

[jieyouxu]

Hi, thanks for the PR and sorry for the slow response. I have a few questions/suggestions.

@rustbot author

View changes since this review

[jieyouxu]

Question:

• The edge case where this would affect formatting would be if e.g. max_width was set to 80, and fn_call_width was set to 76, and the macro body indent was 8. In today's world, fn_call_width in the macro body would be larger than the max_width within the macro body (72), and would get clamped down to 72. With this fix, both would be reduced by 8 (max_width 80 -> 72, fn_call_width 76 -> 68).

Is there a (albeit synthetic) test that can demonstrate the after effect (of this edge case)?

[csmulhern]

I've added a prior commit in the chain that adds test fixtures for these macro use cases, with both default heuristics, and manually crafted widths that will trigger this edge case when the macro body indent exceeds the difference between the custom width and max_width.

Note that the error log currently happens whenever the "clipping" behavior occurred, which is for any file with a macro body if you have custom width values close to / equal to max_width.

This change fixes the error logging, but it only effects formatting when the length of a line is specifically within the gap between max_width - <custom width> and max_width - <macro body indent>.

The long variant of these newly added tests is the macro arm specifically crafted to trigger these conditions. You'll notice that the long variant is the same in source and target before the commit.

When it comes to this being formatting affecting:

1. I believe these edge cases should be formatted the way they are post-commit. The user expressed for e.g. fn_call_width to be less than max_width, and so clipping to max_width feels inappropriate.
2. format_macro_bodies is an unstable option, so I believe formatting changes should be tolerated here regardless of compatibility.

[jieyouxu]

Thanks for the detailed explanation, this is what I was having trouble figuring out 👍

Note

I consider this review chain resolved, but intentionally leaving open for other reviewers for context.

[jieyouxu]

(Also sorry about the force-push, initially I thought that I needed to run DiffCheck, but then it occurred to me the effect of the changes here is basically not going to show up in the corpus AIUI.)

[csmulhern]

@rustbot ready

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[jieyouxu]

Thanks, the changes look good to me. I have one non-blocking observation re. large width behavior, but that is not something that I think should be addressed in this PR. I'll hand this review over for a final review by one of the maintainers.

View changes since this review

[jieyouxu]

Remark: in theory, if we want to be very careful around handling of large widths, this has a potential problem where delta's usize -> isize cast can wraparound for sufficiently large usize, namely [isize::MAX + 1, usize::MAX].

However, I don't think this is a blocking concern for this PR and should not be something that's run into super frequently in practice (esp. if host pointer width is 64+). AFAICT in other parts of the code base we are also not super careful about large widths, so even if this wraps around there are other parts of the codebase that will have similar problems, AFAIK.