rustdoc: change trait bound formatting

[rol1510]

Fixes #85566

Before

Now

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (or someone else) soon.

Please see the contribution instructions for more information.

[Urgau]

This new formatting style is (in my opinion) very confusing and I believe does not follow the Rust style guidelines (which rustfmt applies).

Is there another reason to do this change (other than taking 2 lines less) ?

[GuillaumeGomez]

It makes the reading more difficult, indeed. I think it's not a good idea but let's hear from the rest of the team first. cc @rust-lang/rustdoc

[Manishearth]

Worth noting that this only makes a difference on defaulted methods methods. "This method has a default impl" is not that important from a rustdoc pov and giving it an entire line doesn't seem worth it to me.

That said, this is mostly a problem for Iterator, perhaps we can only apply this to traits with > N defaulted methods?

I'm fine with the proposed change as it is, though.

[jsha]

@Manishearth hits the nail precisely on the head with regards to why I proposed the formatting change in #85566. IMO { ... } does not deserve its own line in what is supposed to be a summary view. It's not a very efficient or effective way to say "there is a provided implementation for this function."

I think "this is not rustfmt style" is an important argument but not a decisive one. The current behavior puts the opening and closing braces on the same line, which rustfmt would never do unless the function body was empty.

Really, there's a conflict between "make the item-decl look like source code" and "hide the implementation."

Perhaps a better way to resolve this conflict is to avoid the pseudo-syntax { ... }. Instead we would break the trait item-decl into two sections, just like we do in the detailed documentation:

pub trait Iterator {
    type Item;
    
    // Required method
    fn next(&mut self) -> Option<Self::Item>;

    // Provided methods
    fn next_chunk<const N: usize>(
        &mut self
    ) -> Result<[Self::Item; N], IntoIter<Self::Item, N>>
    fn size_hint(&self) -> (usize, Option<usize>)
    fn count(self) -> usize
    fn last(self) -> Option<Self::Item>
    fn advance_by(&mut self, n: usize) -> Result<(), usize>
    ...
}

This has the advantage of taking something that is implicit and hard to notice, and making it explicit and easy to notice. That is, readers are expected to notice that a trailing ; means "required method" and a trailing { ... } means "provided method". And also that provided/required methods are grouped into blocks in the item-decl.

By the way, @rol1510 thank you for the implementation! Sorry that we are still hashing out the details of how we want this to look. Sometimes it takes an implementation to spark more detailed discussion. :-D

[Manishearth]

Another thing that could be done is to make {...} look different (perhaps smaller with a slightly different background and a border), and give it a tooltip on hover. That way it's not mimicking inline syntax, rather it's a symbol marking some special behavior. But still ideally copy-pastes as {..} so that people implementing the trait by copy-paste will notice.

(But again, I'm fine with the current proposal)

[notriddle]

I like @jsha’s proposal with the two different sections, as long as where clauses get formatted the same way rustfmt formats them.

[GuillaumeGomez]

I also really like @jsha's suggestion with the two sections.

@Manishearth: I'd prefer to avoid adding tooltips if possible but thanks to @jsha's explanation, I now understand the idea behind it.

[Urgau]

@jsha suggestion to have two distinct sections seems like a great idea. I'm a bit more nuanced with a proposed removal of { ... }, but as long as a normal user can quickly see that the method is provided this seems fine.

[rol1510]

Did we come to a conclusion on this? Should I implement @jsha's suggestion?

[GuillaumeGomez]

I think the proposal with two sections got the most approval. Let's wait for a few days to give time to anyone who doesn't agree to express their opinion then you can open a PR.

[Manishearth]

@Manishearth: I'd prefer to avoid adding tooltips if possible but thanks to @jsha's explanation, I now understand the idea behind it.

I assume this would be a native tooltip not a JS tooltip, fwiw.

[notriddle]

@rol1510 Nobody seems to object to the proposal with the two sections.

[rol1510]

Do we keep the old style for the where clauses, or use the new one?

old:

new:

[GuillaumeGomez]

I find the older one easier to read.

[Manishearth]

I actually kinda prefer the new one 🙃

[notriddle]

I don’t actually have a preference between them, but I noticed that the item-spacer is broken. Make it display: block, and it might look better.

[rol1510]

With the fixed spacer:

new:

old:

[rustbot]

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

[rol1510]

This PR should now create the old style like in the picture above. It also adds a few tests and fixes the item-spacer

If you like the new style more, just let me know and I'll add it back in.

[jsha]

Fwiw, I prefer the new style. :-) One note: the "provided methods" / "required methods" should start with //, not #, since that's the Rust comment style.

[GuillaumeGomez]

Despite personal preference, my only argument in favor of the old style is that it follows the rustfmt style. If we don't care, then since the majority seems to prefer the new style, I have nothing to add. ^^'

[rol1510]

The PR now creates the new style.

[bors]

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

[bors]

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

[notriddle]

@bors r+ rollup

[bors]

📌 Commit 71a147d has been approved by notriddle

It is now in the queue for this repository.

[bors]

⌛ Testing commit 71a147d with merge 5526605002b7186725a30f5ed1ddf0cfcfc24541...

[bors]

💥 Test timed out

[notriddle]

@bors retry

[bors]

⌛ Testing commit 71a147d with merge 319b88c...

[bors]

☀️ Test successful - checks-actions
Approved by: notriddle
Pushing 319b88c to master...

[rust-timer]

Finished benchmarking commit (319b88c): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.7%	[0.7%, 0.7%]	2
Improvements ✅ (primary)	-0.3%	[-0.4%, -0.2%]	2
Improvements ✅ (secondary)	-0.9%	[-1.3%, -0.3%]	11
All ❌✅ (primary)	-0.3%	[-0.4%, -0.2%]	2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.7%	[1.9%, 5.6%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.7%	[3.0%, 4.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[lqd]

This only modifies rustdoc rendering and these benchmarks are all check/debug/opt, so this is noise. The results are corrections from #107679 (comment)

@rustbot label: +perf-regression-triaged