Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from
Apr 5, 2022
Merged

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

merged 2 commits into from
Apr 5, 2022

Conversation

GuillaumeGomez
Copy link
Member

Fixes #95633.

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

r? @notriddle

@rustbot 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 rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 4, 2022
@notriddle
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 4, 2022

📌 Commit 6d19eb8d5c90f9960080e140539992aff50f361c has been approved by notriddle

@bors 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
jyn514 suggested changes Apr 4, 2022
View reviewed changes
src/librustdoc/passes/collect_intra_doc_links.rs Outdated
let iter = cx.resolver_caches.traits_in_scope[&module].iter().flat_map(|trait_candidate| {
let traits = match cx.resolver_caches.traits_in_scope.get(&module) {
Some(t) => t,
None => return Default::default(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@GuillaumeGomez
Copy link
Member Author

@bors: r-

@bors 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 GuillaumeGomez force-pushed the intra-doc-link-ice-traits-in-scope-primitive branch from 6d19eb8 to 50cc0fa Compare April 4, 2022 15:37
@GuillaumeGomez
Copy link
Member Author

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
Copy link
Member Author

@bors: r=jyn514

@bors
Copy link
Contributor

bors commented Apr 4, 2022

📌 Commit 50cc0fa has been approved by jyn514

@bors 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
@petrochenkov
Copy link
Contributor

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

@GuillaumeGomez
Copy link
Member Author

Only on the current crate.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 5, 2022
@Dylan-DPC
Rollup merge of rust-lang#95645 - GuillaumeGomez:intra-doc-link-ice-t…
bec8741 
…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`
@Dylan-DPC Dylan-DPC mentioned this pull request Apr 5, 2022
Closed
@Dylan-DPC Dylan-DPC mentioned this pull request Apr 5, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 5, 2022
@bors
Auto merge of rust-lang#95667 - Dylan-DPC:rollup-n7xzs3y, r=Dylan-DPC
634770c 
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
@bors bors merged commit bf44a87 into rust-lang:master Apr 5, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 5, 2022
@GuillaumeGomez GuillaumeGomez deleted the intra-doc-link-ice-traits-in-scope-primitive branch April 5, 2022 13:33
@GuillaumeGomez
Copy link
Member Author

Nominating for beta backport as well.

@GuillaumeGomez GuillaumeGomez added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 5, 2022
@petrochenkov
Copy link
Contributor

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

@GuillaumeGomez
Copy link
Member Author

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
Copy link
Collaborator

Awaiting bors try build completion.

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

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

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

@GuillaumeGomez
Copy link
Member Author

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

@petrochenkov
Copy link
Contributor

petrochenkov commented Apr 5, 2022 •
edited
Loading

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 GuillaumeGomez mentioned this pull request Apr 5, 2022
Closed
@GuillaumeGomez
Copy link
Member Author

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

I'm very curious why though...

@rylev
Copy link
Member

rylev commented Apr 5, 2022

@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
Copy link
Member Author

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.

@GuillaumeGomez GuillaumeGomez removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 5, 2022
@notriddle
Copy link
Contributor

notriddle commented Apr 5, 2022 •
edited
Loading

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
Copy link
Member Author

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

@rylev
Copy link
Member

rylev commented Apr 5, 2022

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
Copy link
Member Author

GuillaumeGomez commented Apr 5, 2022 •
edited
Loading

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

EDIT: I opened #95694.

@GuillaumeGomez GuillaumeGomez added the perf-regression Performance regression. label Apr 5, 2022
@GuillaumeGomez GuillaumeGomez mentioned this pull request Apr 5, 2022
Closed
@jyn514 jyn514 mentioned this pull request May 4, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 approved these changes

Assignees

@notriddle notriddle

Labels
perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-perf Status: Waiting on a perf run to be completed. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.

ICE: panicked at 'no entry found for key' documenting std::i8 re-export with --document-private-items
9 participants
@GuillaumeGomez @notriddle @bors @petrochenkov @rust-timer @rylev @jyn514 @rust-highfive @rustbot