Fix whitespace handling after where clause

[GuillaumeGomez]

Fixes #97733.

You can see the result here.

r? @jsha

[bors]

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

[jsha]

This comment is ambiguous. There should be whitespace in a variety of places! Where specifically is this test trying to confirm the presence of a whitespace?

[jsha]

Determining whether there's a where clause based on the length of the emitted data is confusing and self-referential. Instead, the filter should print_where_clause should be extracted out to a method of clean::Generics:

        let mut where_predicates = gens.where_predicates.iter().filter(|pred| {
            !matches!(pred, clean::WherePredicate::BoundPredicate { bounds, .. } if bounds.is_empty())

For instance it could be fn where_predicates() -> impl Iterator<Item = WherePredicate> on Generics that returns all where predicates with non-empty bounds. Then this function would have two possible rendering paths: (a) when the where clause is present (where_predicates() returns a nonempty iterator), and (b) when the where clause is not present.

[GuillaumeGomez]

I agree that the code doesn't look great but i don't agree with the solution you propose. I have an idea so I'll update the PR and you can tell me if you find it better.

[jsha]

I still think it's not good style. You are still checking the length of the buffer to see whether print_where_clause did anything: https://github.com/rust-lang/rust/pull/98256/files#diff-c4e61d56ff695cfc762517d170672a9c8650ac815abee6a9e28267a2ff27c8f3R72-R74.

That strongly suggests to me that the logic for "will print_where_clause do anything" should be extracted into its own function. I'm not particularly attached to the iterator idea I proposed above; it could also be a has_where_clause() function.

That will make the code that chooses whether to add a space, newline, or semicolon clearer, since it can check if there is or is not a where clause, and call print_where_clause only in that case; and also emit a space, newline, or semicolon only in that case.

[GuillaumeGomez]

I don't like the idea of duplicating this logic though (duplicating in the sense of "checking it twice", not "copy/paste it in two places").

[jsha]

I think it would be better to make this whitespace emitted by print_where_clause. Then the role of print_where_clause is simple to explain and self contained. It either (a) prints a newline, a where clause, and another newline, or (b) prints a whitespace.

[GuillaumeGomez]

Let me give it a try then!

[GuillaumeGomez]

Unfortunately it cannot work for cases like:

trait A {
    fn foo<T>(a: T)
    where T: Clone;
}

type F<T> where T: Clone = Vec<T>;
trait G<T> where T: Clone = A<T>;

If we handle the whitespace directly into print_where_clause, it'll need to be handled manually in these cases to "counter" the effect.

[GuillaumeGomez]

r? @notriddle

[GuillaumeGomez]

Cleaned up the code and fixed the tests.

[GuillaumeGomez]

Updated all channel URLs.

[notriddle]

I like that the unused indent and end_newline parameters were removed from print_where_clause_and_check.

But I was actually thinking that the end_newline parameter could be removed entirely. Its only purpose is to insert a newline at the end of the where clause if the where clause was non-empty, just like the way the trailing space is handled. Why can't both be done the same way?

[GuillaumeGomez]

You meant for print_where_clause? I can do it too but in another PR to not mix fix and cleanup. I can open an issue for it if you want?

[notriddle]

Yeah, makes sense.

@bors r+

[bors]

📌 Commit c8a5b67 has been approved by notriddle

[GuillaumeGomez]

Will send a PR tomorrow then.