rust icon indicating copy to clipboard operation
rust copied to clipboard

Always display stability version even if it's the same as the containing item

Open GuillaumeGomez opened this issue 2 years ago • 19 comments

Fixes https://github.com/rust-lang/rust/issues/118439.

Currently, if the containing item's version is the same as the item's version (like a method), we don't display it on the item.

This was something done on purpose as you can see here. It was implemented in https://github.com/rust-lang/rust/pull/30686.

I think we should change this because on pages with a lot of items, if someone arrives (through the search or a link) to an item far below the page, they won't know the stability version unless they scroll to the top, which isn't great.

You can see the result here.

r? @notriddle

GuillaumeGomez avatar Nov 29 '23 11:11 GuillaumeGomez

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

rustbot avatar Nov 29 '23 11:11 rustbot

This is definitely going to make the standard library docs bigger. Let's see how much!

@bors try @rust-timer queue

notriddle avatar Nov 29 '23 15:11 notriddle

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

rust-timer avatar Nov 29 '23 15:11 rust-timer

:hourglass: Trying commit a333572970ed62701fd57f902d57a0a0aca0dd86 with merge 4d962f0f089466aec16928808dde2312d85fe7c1...

bors avatar Nov 29 '23 15:11 bors

:sunny: Try build successful - checks-actions Build commit: 4d962f0f089466aec16928808dde2312d85fe7c1 (4d962f0f089466aec16928808dde2312d85fe7c1)

bors avatar Nov 29 '23 17:11 bors

Queued 4d962f0f089466aec16928808dde2312d85fe7c1 with parent abe34e9ab14c0a194152b4f9acc3dcbb000f3e98, future comparison URL. There is currently 1 preceding artifact in the queue. It will probably take at least ~2.0 hours until the benchmark run finishes.

rust-timer avatar Nov 29 '23 17:11 rust-timer

Finished benchmarking commit (4d962f0f089466aec16928808dde2312d85fe7c1): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never @rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

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)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-4.6% [-4.7%, -4.4%] 2
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 673.033s -> 673.683s (0.10%) Artifact size: 313.38 MiB -> 313.38 MiB (-0.00%)

rust-timer avatar Nov 29 '23 19:11 rust-timer

Hmm. The sizes are the same (https://perf.rust-lang.org/compare.html?start=abe34e9ab14c0a194152b4f9acc3dcbb000f3e98&end=4d962f0f089466aec16928808dde2312d85fe7c1&stat=size%3Adoc_bytes&tab=compile&nonRelevant=true&showRawData=true). But I guess that's expected, since non-std documentation doesn't actually contain these attributes (?). We should probably measure the size of libstd docs, but that's not included in the benchmarks at the moment.

Kobzol avatar Nov 29 '23 19:11 Kobzol

Yeah. I didn't think of that. Here's the output for ./x doc library/std, run on my machine:

michaelhowell@Michael-Howells-Macbook-Pro rust % du -hs doc-new doc-old
117M    doc-new
116M    doc-old
michaelhowell@Michael-Howells-Macbook-Pro rust % du -s doc-new doc-old 
240272  doc-new
237384  doc-old

Where doc-new is the version in this branch (a333572970ed62701fd57f902d57a0a0aca0dd86), and doc-old is from b29a1e00f850e548f3021ea523d0e143724fa9b7

notriddle avatar Nov 29 '23 20:11 notriddle

So about a 1.2% increase. That's not that bad I guess, but it's also not trivially small.

Kobzol avatar Nov 29 '23 20:11 Kobzol

@rfcbot fcp close

I have to say it's not worth it. There's reasonable arguments for both perspectives, and I don't want to be flip-flopping back and forth.

notriddle avatar Nov 29 '23 21:11 notriddle

Team member @notriddle has proposed to close this. The next step is review by the rest of the tagged team members:

  • [x] @GuillaumeGomez
  • [x] @Manishearth
  • [ ] @Nemo157
  • [ ] @aDotInTheVoid
  • [ ] @camelid
  • [x] @jsha
  • [x] @notriddle

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot avatar Nov 29 '23 21:11 rfcbot

I think it's worth landing this. As their tend to be many methods on items in std, users probably haven't seen the stability version of the containing item.

Also it feels weird and inconsistant that functions stabilized at the same time as the item have don't have a stability marker, but the rest do.

image

I think we should merge this.

aDotInTheVoid avatar Nov 29 '23 21:11 aDotInTheVoid

I lean on the side of "close without action."

It's true that it's a bit weird and confusing to not have stability information in every possible place. And when I first noticed that I thought it was a bug.

But stability versions take up a notable amount of room in the UI, and if they are on every item in the standard docs, it will be harder to notice which ones are noteworthy. For instance, I think this PR would annotate every single method of String with 1.0.0, right? That's not very noteworthy.

And if someone does make a mistake and uses a method that is not stabilized on the older version of Rust they're using, because they didn't notice that the struct itself wasn't stabilized by that version, they will get a nice error that makes it pretty clear what the problem is.

jsha avatar Nov 29 '23 22:11 jsha

Hm, that's a good point

Manishearth avatar Nov 29 '23 22:11 Manishearth

For instance, I think this PR would annotate every single method of String with 1.0.0, right?

I wonder if that gives another alternative -- rather than "omitted is same as the type", it could be "omitted is 1.0", which might bring that size overhead back down a bunch again, if it was largely from old basics?

Still dunno if it's worth doing, though.

scottmcm avatar Dec 03 '23 04:12 scottmcm

I also feel like I weakly fall on the side of landing this (maybe with the change to hide 1.0.0 annotations to reduce the size impact). I have been personally confused at the lack of annotations many times and feel like the consistency gain is worth it.

Nemo157 avatar Dec 04 '23 20:12 Nemo157

I was just quite confused by the missing stability annotation on a const fn: https://github.com/rust-lang/rust/issues/121989

RalfJung avatar Mar 05 '24 10:03 RalfJung

We (briefly) talked about it in the last rustdoc meeting. Some members of the team would actually like this feature to be merged, so more discussions will follow.

GuillaumeGomez avatar Mar 11 '24 21:03 GuillaumeGomez

As discussed on zulip, seems like the majority of the rustdoc team would prefer to have this feature.

The original reason (in 2016, so very shortly after the 1.0) for not displaying the version in case it was the same as the parent's was because it was too much noise. At the time, it was much more common for the majority of sub-items to be from 1.0. Now there's a much wider spread of version numbers so the items which don't have a version number on them are probably the minority.

But also, since then, the number of items grew quite a lot and it's not rare to land on a page to a given item and then you need to scroll to the top to see the version of the parent item to see from which version it can be used if it was not marked.

For these reasons, I'll cancel the closing FCP and open a merge FCP instead.

@rfcbot cancel

@rfcbot merge

GuillaumeGomez avatar Apr 08 '24 20:04 GuillaumeGomez

@GuillaumeGomez proposal cancelled.

rfcbot avatar Apr 08 '24 20:04 rfcbot

Team member @GuillaumeGomez has proposed to merge this. The next step is review by the rest of the tagged team members:

  • [x] @GuillaumeGomez
  • [x] @Manishearth
  • [x] @Nemo157
  • [x] @aDotInTheVoid
  • [x] @camelid
  • [ ] @jsha
  • [x] @notriddle

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot avatar Apr 08 '24 20:04 rfcbot

rfcbot seems to be glitching...

@rfcbot reviewed

camelid avatar Apr 09 '24 04:04 camelid

:bell: This is now entering its final comment period, as per the review above. :bell:

rfcbot avatar Apr 09 '24 04:04 rfcbot

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

rfcbot avatar Apr 19 '24 04:04 rfcbot

@bors r=rustdoc

GuillaumeGomez avatar Apr 19 '24 12:04 GuillaumeGomez

:pushpin: Commit a333572970ed62701fd57f902d57a0a0aca0dd86 has been approved by rustdoc

It is now in the queue for this repository.

bors avatar Apr 19 '24 12:04 bors

:hourglass: Testing commit a333572970ed62701fd57f902d57a0a0aca0dd86 with merge d1a0fa5ed3ffe52d72f761d3c95cbeb0a9cdfe66...

bors avatar Apr 19 '24 14:04 bors

:sunny: Test successful - checks-actions Approved by: rustdoc Pushing d1a0fa5ed3ffe52d72f761d3c95cbeb0a9cdfe66 to master...

bors avatar Apr 19 '24 16:04 bors

Finished benchmarking commit (d1a0fa5ed3ffe52d72f761d3c95cbeb0a9cdfe66): 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.3% [0.2%, 0.3%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.2% [-0.2%, -0.2%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.1% [-0.2%, 0.3%] 3

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)
2.5% [2.5%, 2.5%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.5% [2.5%, 2.5%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 672.208s -> 672.762s (0.08%) Artifact size: 315.30 MiB -> 315.24 MiB (-0.02%)

rust-timer avatar Apr 19 '24 18:04 rust-timer