[csmulhern]

• I did not use an LLM to create a change in this PR.
• I used an LLM to create a change in this PR, and I have explained below how it was used.

This is an update to #6651. Since updating to the rustfmt beta that includes that change, I noticed that explicitly set widths that are involved in small heuristics can now cause formatting differences between the same code inside and outside of a macro.

The original sin that makes this space challenging is how macros are formatted by forking the config and formatting the macro body as if it had zero indent + reduced max width. Because of this, #6651 reduced the width of all the other explicit width properties that are involved in small heuristics. However, these properties are applied independent of indent. This PR fixes this inconsistency for explicitly set widths by not modifying the values beyond capping them based on max_width. Note: there are still likely other ways that macro bodies format inconsistently, due to the original sin.

The tests have been updated to account for this nuance.

As a quick example, here is code that formats inconsistently today:

// rustfmt-format_macro_bodies: true
// rustfmt-chain_width: 72

macro_rules! inside_macro {
    () => {
        let value = aaaaaaaaaaaaaaaa
            .bbbbbbbbbbbbbbbb()
            .cccccccccccccccc()
            .ddddddddddddd();
    };
}

fn outside_macro() {
    if true {
        let value = aaaaaaaaaaaaaaaa.bbbbbbbbbbbbbbbb().cccccccccccccccc().ddddddddddddd();
    }
}

The code formats consistently with this change.

[csmulhern]

@jieyouxu wanted to give you the heads up on this given that #6651 is in beta.

[ytmimi]

So #6651 changed default behavior that we didn't catch?

[jieyouxu]

So #6651 changed default behavior that we didn't catch?

Darn, at the time it didn't occur to me that format_macro_bodies is an unstable option yes, but the default value (on) is actually stable default formatting, you just can't turn it off stably... I think we may need to consider reverting #6651 on beta and #7152 the follow-up. I think we can try to reland #6651 with a style edition gate alongside this PR 😢 Especially considering release cycling is close.

[ytmimi]

Yeah, if this changes default formatting I'd say it would be safest to try and revert #6651 on beta. @jieyouxu is there still time to revert this?

[jieyouxu]

Yeah, if this changes default formatting I'd say it would be safest to try and revert #6651 on beta. @jieyouxu is there still time to revert this?

My understanding is that yes we do, 1.100 is current beta, 9 days out to next stable. Let me prep a revert series. We'd need to sync the revert series too.

[csmulhern]

I don’t think it changes default formatting. By default no widths are explicitly set, so they all come from small heuristics, which are based on max_width (reduced in macro bodies). That was the same before and after #6651.

With someone setting explicit width properties (e.g. fn_call_width), #6651 would affect the following case:

1. The explicit width is below max_width.
2. Code is located inside a macro_rules body.
3. The expression's length fits the explicit width, but not the width reduced by the macro body's indent.

[csmulhern]

But I'm all for reverting #6651 in the beta and trying to fix this with #6651 + this change for the next release.

Sorry for the churn!

[jieyouxu]

I don’t think it changes default formatting. By default no widths are explicitly set, so they all come from small heuristics, which are based on max_width (reduced in macro bodies). That was the same before and after #6651.

With someone setting explicit width properties (e.g. fn_call_width), #6651 would affect the following case:

1. The explicit width is below max_width.
2. Code is located inside a macro_rules body.
3. The expression's length fits the explicit width, but not the width reduced by the macro body's indent.

Ah right, my bad, it would not be stable default formatting, it would need stable options.

Sorry for the churn!

@csmulhern no need to be; it just didn't occur to me at the time. The fix is still very appreciated!

[csmulhern]

Ah right, my bad, it would not be stable default formatting, it would need stable options.

Yes, that's right. And with #6651 + this change, I believe there should be NO effect on current stable formatting (even with explicit widths set). It will just fix the warning from #5404.

[jieyouxu]

Ah right, my bad, it would not be stable default formatting, it would need stable options.

Yes, that's right. And with #6651 + this change, I believe there should be NO effect on current stable formatting (even with explicit widths set). It will just fix the warning from #5404.

Ah cool, that sounds good. To be safe, I think we should still land the revert on beta, then reland #6651 + #7152 + this change after the release cycling just to give the combined change some more time to bake across two release cycles. (A fix-forward with this PR is somewhat appealing but I tend to want to be very conservative with backports, since fix-forwards can introduce new bugs and we'd not have enough time to fix any of those in the 1.100 beta.)

[csmulhern]

Yup, same thoughts here. I'm all for being conservative given the low margin for error.

[jieyouxu]

Alright -- the reverts landed. If you want to reapply #6651 + #7152 in this PR, or make a new PR with the combined changes, I can review.
@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @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.

[csmulhern]

I've updated this PR to be a combination of #6651 + #7152 + the original fixes from this PR that corrected how explicit widths were handled in #6651 (by preserving them, instead of shrinking them).

[csmulhern]

@rustbot ready

[jieyouxu]

Thanks. AFAICT this is correct and only removes the warning but does not modify stable default formatting or stable options formatting.

Minor nits re. comments and test coverage.
@rustbot author

View changes since this review

[jieyouxu]

Probably yeet this comment now, it's kinda "duh" on hindsight.

[jieyouxu]

[COMMENT 1/3]: Can we dedup these comment bodies? It's quite verbose. Consider instead

Reduce max_width by delta saturating on underflow. See adjust_max_width.

ditto for increase_max_width, but saturating on overflow.

[jieyouxu]

[COMMENT 2/3]: see [COMMENT 1/3].

[jieyouxu]

[COMMENT 3/3]: Probably easier to read if we say sth like

Adjust max_width by delta in a forked config (example: macro body at zero indent). In particular:

• Width options not set explicitly are recomputed from new max_width via heuristics.
• Explicit widths will keep their value but capped at the new max_width.

A later increase in max_width will not restore a previously capped value.

[jieyouxu]

That example you gave in the PR is great, would be nice to have it as an additional test case 😉

[jieyouxu]

NB: I was looking at the logic here a bit more closely. I know obviously that the ui tests here won't demonstrate "ah the warning went away" (that's #6933), which is the point for this PR.

So my understanding is that, there's 3 cases we need to consider:

1. Explicit width never set, i.e. default config
2. Explicit width configured, but:
   • (A) > recomputed (reduced) max width, so it has to be capped to recomputed max width.
   • (B) <= recomputed (reduced) max width, so fits in recomputed max width.

Here:

• (1) unaffected.
• (2A) is stable option behavior (though it has a warning à la Cannot set granular width configuration settings to 100% if max_width = 100  #5404), and we're not changing it modulo removing the cap warning.
• (2B) is stable option behavior, and unchanged by this PR.