Fix intra doc link ICE when trying to get traits in scope for primitive#95645

GuillaumeGomez · 2022-04-04T13:19:45Z

Fixes #95633.

I think @notriddle was the one who worked on this part of the code last so:

r? @notriddle

notriddle · 2022-04-04T13:58:53Z

@bors r+

bors · 2022-04-04T13:58:54Z

📌 Commit 6d19eb8d5c90f9960080e140539992aff50f361c has been approved by notriddle

jyn514 · 2022-04-04T14:58:03Z

src/librustdoc/passes/collect_intra_doc_links.rs

This is hiding bugs. There should be no items for which rustdoc doesn't know the in-scope traits; if there are, it needs to be fixed or rustdoc will resolve links incorrectly.

This was also my guess but I went for this in order to prevent the ICE in the time being. Do you prefer I add a FIXME comment and open an issue?

Don't have a strong opinion. Just needs to be fixed at some point or there will be other bugs that arise.

Ok. I'll investigate more and if I can't find what's going on, I'll add the FIXME and open an issue.

Ah, funny case. The problem is that it's not i8 the type but i8 the module in the issue. I need to check why it's not added when --document-private-items is used.

Ok found the problem, we need to handle things a bit differently in case we have this option enabled.

GuillaumeGomez · 2022-04-04T14:59:19Z

@bors: r-

… for a private module

GuillaumeGomez · 2022-04-04T15:39:42Z

So I found the root problem for this ICE: we only added public traits or traits from public modules. However, we forgot to take that into account when the --document-private-items is used. This should be the correct fix now.

GuillaumeGomez · 2022-04-04T16:01:40Z

@bors: r=jyn514

bors · 2022-04-04T16:01:41Z

📌 Commit 50cc0fa has been approved by jyn514

petrochenkov · 2022-04-04T16:32:41Z

By the way, does --document-private-items enables documentation on private items only in the current crate, or in its dependency crates too?

GuillaumeGomez · 2022-04-04T16:41:43Z

Only on the current crate.

…raits-in-scope-primitive, r=jyn514 Fix intra doc link ICE when trying to get traits in scope for primitive Fixes rust-lang#95633. I think `@notriddle` was the one who worked on this part of the code last so: r? `@notriddle`

Rollup of 5 pull requests Successful merges: - rust-lang#95234 (bootstrap.py: nixos check in /etc/os-release with quotes) - rust-lang#95449 (Fix `x doc --stage 0 compiler`) - rust-lang#95512 (diagnostics: translation infrastructure) - rust-lang#95607 (Note invariance reason for FnDef types) - rust-lang#95645 (Fix intra doc link ICE when trying to get traits in scope for primitive) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

GuillaumeGomez · 2022-04-05T13:34:05Z

Nominating for beta backport as well.

petrochenkov · 2022-04-05T14:12:14Z

You may be interested in the perf data in #95667 (comment) before backporting.

GuillaumeGomez · 2022-04-05T14:15:09Z

It seems surprising since it should only impact when rendering private items (unless that's what they do in the tests?). Let's run a perf check here directly to be sure this PR is the problem.

@bors try @rust-timer queue

rust-timer · 2022-04-05T14:15:10Z

Awaiting bors try build completion.

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

notriddle · 2022-04-05T14:41:26Z

I don’t think we’re getting a try build.

GuillaumeGomez · 2022-04-05T14:43:17Z

Doesn't seem so... :-/ I'll ask around how to run perf check on this PR.

petrochenkov · 2022-04-05T14:47:36Z

I need to check what happens here, the document_private_items-based fix may be incorrect and fix the issue accidentally.
UPD: Or rather that it collects traits in scopes in so many modules so it covers the test case too with a large margin.

GuillaumeGomez · 2022-04-05T17:52:21Z

@petrochenkov You were right, this PR is the one introducing the perf regression: #95682 (comment)

I'm very curious why though...

rylev · 2022-04-05T18:11:47Z

@GuillaumeGomez given how large the impact is, do you think it might be wise to revert this until we figure out what the issue is?

GuillaumeGomez · 2022-04-05T18:14:34Z

Basically we have the choice between an ICE and a perf regression. I think it's better to keep the perf regression. I think @petrochenkov is already investigating this too so hopefully it should be fine before the next beta release in 6 weeks. I'll remove the beta backport nomination though.

notriddle · 2022-04-05T18:36:00Z

When documenting a bin crate, cargo always passes --document-private-items. It took me a while to find this (I didn’t know about it either), even though I knew it had to be running with document_private on those perf tests; there's no way a highly predictable branch could have a perf impact like that.

https://github.com/rust-lang/cargo/blob/e2e2dddebe66dfc1403a312653557e332445308b/src/cargo/ops/cargo_compile.rs#L643-L649

To confirm this theory, the worst-impacted crate is helloworld. A bin crate that depends on no lib crates (except the standard library, which doesn't get documented).

GuillaumeGomez · 2022-04-05T18:37:35Z

Thanks for the information! I completely forgot that about binaries...

rylev · 2022-04-05T18:41:03Z

I think it's better to keep the perf regression.

That's fine, but I am worried about this getting lost. The recommended procedure in that case where a fix is not immediately obvious is to create an issue and assign yourself (or @petrochenkov) to it (though I realize this is also not a guarantee that the issue won't be lost).

GuillaumeGomez · 2022-04-05T18:42:01Z

This is an excellent point. Opening an issue right away.

EDIT: I opened #95694.

rust-highfive assigned notriddle Apr 4, 2022

rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Apr 4, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 4, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 4, 2022

jyn514 suggested changes Apr 4, 2022

View reviewed changes

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 4, 2022

GuillaumeGomez added 2 commits April 4, 2022 17:36

Fix ICE in rustdoc intra doc links when trying to get traits in scope…

bf2c3b0

… for a private module

Add test to ensure rustdoc does not panic on intra doc link pass

50cc0fa

GuillaumeGomez force-pushed the intra-doc-link-ice-traits-in-scope-primitive branch from 6d19eb8 to 50cc0fa Compare April 4, 2022 15:37

jyn514 approved these changes Apr 4, 2022

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 4, 2022

Dylan-DPC mentioned this pull request Apr 5, 2022

Rollup of 8 pull requests #95666

Closed

Dylan-DPC mentioned this pull request Apr 5, 2022

Rollup of 5 pull requests #95667

Merged

bors merged commit bf44a87 into rust-lang:master Apr 5, 2022

rustbot added this to the 1.62.0 milestone Apr 5, 2022

GuillaumeGomez deleted the intra-doc-link-ice-traits-in-scope-primitive branch April 5, 2022 13:33

GuillaumeGomez added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 5, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 5, 2022

GuillaumeGomez mentioned this pull request Apr 5, 2022

Revert "Fix intra doc link ICE when trying to get traits in scope for primitive" #95682

Closed

GuillaumeGomez removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 5, 2022

GuillaumeGomez added the perf-regression Performance regression. label Apr 5, 2022

GuillaumeGomez mentioned this pull request Apr 5, 2022

Perf regression on rustdoc --document-private-items speed #95694

Closed

jyn514 mentioned this pull request May 4, 2023

Prevent crash when a path is not resolved in intra-doc link #111195

Merged

Uh oh!

Conversation

GuillaumeGomez commented Apr 4, 2022

Uh oh!

notriddle commented Apr 4, 2022

Uh oh!

bors commented Apr 4, 2022

Uh oh!

jyn514 Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

jyn514 Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Apr 4, 2022

Uh oh!

GuillaumeGomez commented Apr 4, 2022

Uh oh!

GuillaumeGomez commented Apr 4, 2022

Uh oh!

bors commented Apr 4, 2022

Uh oh!

petrochenkov commented Apr 4, 2022

Uh oh!

GuillaumeGomez commented Apr 4, 2022

Uh oh!

GuillaumeGomez commented Apr 5, 2022

Uh oh!

petrochenkov commented Apr 5, 2022

Uh oh!

GuillaumeGomez commented Apr 5, 2022

Uh oh!

rust-timer commented Apr 5, 2022

Uh oh!

notriddle commented Apr 5, 2022

Uh oh!

GuillaumeGomez commented Apr 5, 2022

Uh oh!

petrochenkov commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GuillaumeGomez commented Apr 5, 2022

Uh oh!

rylev commented Apr 5, 2022

Uh oh!

GuillaumeGomez commented Apr 5, 2022

Uh oh!

notriddle commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GuillaumeGomez commented Apr 5, 2022

Uh oh!

rylev commented Apr 5, 2022

Uh oh!

GuillaumeGomez commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

petrochenkov commented Apr 5, 2022 •

edited

Loading

notriddle commented Apr 5, 2022 •

edited

Loading

GuillaumeGomez commented Apr 5, 2022 •

edited

Loading