Respect file-lines range per predicate in where clauses

[Souradip121]

Solves #6872

Summary

• rewrite_bounds_on_where_clause (Block/RFC indent path) and the Visual-indent path in rewrite_where_clause both unconditionally rewrote every predicate, causing the entire where clause to be reformatted even when only a subset of predicate lines were selected via --file-lines
• Fixed by checking out_of_file_lines_range! per predicate inside the itemize_list closure — predicates outside the selected range now fall back to their original source text, predicates inside are formatted normally
• Follows the same pattern already used in src/expr.rs, src/stmt.rs, and src/visitor.rs
• Adds tests/source/file-lines-where-clause.rs + tests/target/file-lines-where-clause.rs to cover the partial-formatting case

Test Plan

• cargo test passes
• New test file-lines-where-clause verifies that only the predicate on the selected line is reformatted; out-of-range predicates retain their original text
• Existing file-lines-* tests continue to pass (no regression in full-range formatting)
• Idempotency check passes (formatting the output a second time is a no-op)

[ytmimi]

Instead of doing this check here, I'm wondering if we could move this logic to the Iterator impl for ListItems. It will likely require us to pass the whole RewriteContext to itemize_list instead of just the snippet_provider, but that should be fine.

Moving this check there means that we wouldn't need to duplicateif out_of_file_lines_range! every time we use itemize_list, and it means that you'd actually fix things for other code paths that use this pattern.

View changes since the review

[Souradip121]

Alright, made a commit, kindly check.

[randomPoison]

This PR does not fully address #6872. This only seems to address inline spacing within a single predicate (i.e. the spacing between the type param and the bound T: Clone), such that we can rewrite a single predicate without affecting the inline spacing of other predicates. But unselected predicates still get re-indented, and the where keyword still gets moved around even when not selected.

[randomPoison]

A minor note: The way this test is written it's a bit hard to see what exactly is changing between the source file and target file, since only a single space is changing. I'd suggest changing the input file so that the bad formatting is more obvious, such that it's easier to tell at a glance what the test covers:

fn foo<T, U, V>()
where
    T     :     Clone      +     Debug,
    U    :       Copy,
    V   :        Default,

Would then become

fn foo<T, U, V>()
where
    T: Clone + Debug,
    U    :       Copy,
    V   :        Default,

View changes since the review

[Souradip121]

Thanks for the catch, just updated the test per your suggestion, the diff reads much clearer now. Also pushed a bailout so the whole where clause stays untouched when none of its lines are in the selected range, which fixes the where-keyword-moves issue for that case. The partial case (some predicates in range, others out, and the where line itself out) needs line-level source splicing and is more invasive also happy to tackle that as a follow-up, or fold it in here if you'd prefer.

[Souradip121]

Okayy, just created a commit applying it.

[randomPoison]

The partial case (some predicates in range, others out, and the where line itself out) needs line-level source splicing and is more invasive also happy to tackle that as a follow-up, or fold it in here if you'd prefer.

I think that'd be reasonable to split out into its own PR. Now that this PR is generalized to handle all lists, I think this makes sense to land on its own first, since this will also help other issues like #6868.

[Souradip121]

@randomPoison @ytmimi Any updates on getting the changes merged?

[randomPoison]

I'm not a maintainer on the project so I can't actually merge anything, but the changes do look good to me given my current understanding of the relevant rustfmt logic.

[Souradip121]

@ytmimi kindly check

[ytmimi]

I will review this when I have some time. Thanks for your patience on this.

[Souradip121]

@ytmimi Any Updates?

[rustbot]

☔ The latest upstream changes (possibly #7079) made this pull request unmergeable. Please resolve the merge conflicts.

[randomPoison]

After doing some more digging into the list formatting machinery, I'm starting to think that putting the file-lines checks in ListItems isn't the right place to do it.

Edit: After further digging, I no longer think we need to rework what this PR is doing, see my followup comment for more details.

Original comment

There are really two pieces to list formatting:

• ListItems and its Iterator impl, which handles parsing the original sources for things like comments, and produces a list of ListItem objects.
• write_list, which takes the ListItems and handles laying out the pieces according to the formatting rules.

In my own experimentation with trying to support file-lines in list formatting, I'm finding that having the file-lines info in ListItems isn't enough to handle formatting properly, and most of the logic needs to be put in write_list so that we can check file-lines as we lay out the pieces of the list. My current feeling is that ListItems should not check file-lines at all, and should instead just capture span information for each of the pieces that it parses out. Then write_list should handle the work of checking the spans of each piece against file-lines and determining when to rewrite vs when to preserve snippets verbatim.

For this PR that would mean reverting the changes to ListItems::next, and instead updating it and ListItem to capture the span of the item, then updating write_list to check that span against file-lines before rewriting it.

Let me know if anything about that seems off. I'm still actively experimenting with supporting file-lines in lists, so it's possible that I may find a reason why we it makes more sense to check the file-lines config in ListItems.

[randomPoison]

Actually, after doing some further experimentation, I think maybe doing the file-lines check in ListItems::next might be necessary to avoid deeper refactoring. There are a handful of checks on the ListItem itself, e.g. is_substantial, that expect the item field to actually reflect the text being laid out. Putting all of the file-lines check in write_list then means having to rework some of the logic since the ListItem no longer actually knows whether its rewritten item field is going to be used. This can be handled by heavier reworking of write_list, but I don't think that's worth it to avoid checking file-lines in ListItems::next.

So, to be clear, I'm fine with the approach this PR takes currently (i.e. checking file-lines in ListItems::next). It's a little unfortunate that we'll have to check file-lines in multiple places, but I don't think that's avoidable.

---

However, in the process of digging into this further, I did put together some tests that reveal some issues with the logic in this PR with regards to how it interacts with other parts of the language that go through the list formatting machinery. I pushed those to a copy of this branch on my fork, you can see them over here.

The issues those tests uncovered are:

• For unselected macro arms, the ; separator gets duplicated. This seems to come from the fact that the span for macro arms contain the separator, so when we preserve the original snippet it already includes the separator, then write_list adds another one. We either need to tweak the way the span works for macro arms, or detect this case in write_list.
• If the final arm of a match is unselected and had a trailing ,, the trailing , separator gets removed. This also seems to relate to ownership of the trailing separator, since match arms have special handling for when it emits the trailing comma (i.e. so that it doesn't emit them when the body of the arm is a block), and that special handling isn't currently aware of whether the items is selected or not.

It's unclear to me if these need to be addressed in this PR, though. More work is likely needed to handle file-lines in those other language constructs, so maybe these issues are better to address in separate PRs.