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.

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

rustdoc: Go back to loading all external crates unconditionally #90489

Merged
merged 2 commits into from
Nov 12, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Nov 1, 2021

This continues to cause regressions. This code will be unnecessary
once access to the resolver happens fully before creating the tyctxt
(#83761), so load all crates unconditionally for now. To minimize churn, this leaves in the code for loading crates selectively.

"Fixes" #84738. Previously: #83738, #85749, #88215

r? @petrochenkov cc @camelid (this should fix the "index out of bounds" error you had while looking up crate_name).

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 1, 2021
@jyn514 jyn514 added A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name A-metadata Area: Crate metadata A-resolve Area: Name resolution T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Nov 1, 2021
@jyn514 jyn514 force-pushed the load-all-extern-crates branch 2 times, most recently from 05348e6 to 158861a Compare November 1, 2021 23:07
@jyn514 jyn514 added beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Nov 1, 2021
@jyn514 jyn514 changed the title Go back to loading all external crates unconditionally rustdoc: Go back to loading all external crates unconditionally Nov 1, 2021
@camelid
Copy link
Member

camelid commented Nov 4, 2021

(this should fix the "index out of bounds" error you had while looking up crate_name).

Just a followup so we don't get confused later on: This PR doesn't fix that ICE; they seem to be different bugs.

@petrochenkov
Copy link
Contributor

@jyn514
The minimized reproduction in #84738 looks like #88679 (comment).
Could you check whether visiting associated items in IntraLinkCrateLoader fixes #84738?

@petrochenkov petrochenkov 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2021
@jyn514
Copy link
Member Author

jyn514 commented Nov 4, 2021

@petrochenkov given how many regressions this has caused, I don't particularly want to keep playing wack-a-mole with fixing the bugs. I'm fine with fixing the bugs in parallel, but I don't think we should be testing things "in-prod" as it were, we don't need to enable this until rustdoc's fundamental problems with crate loading are resolved (#83761).

@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 4, 2021
@petrochenkov
Copy link
Contributor

given how many regressions this has caused

Which regressions?
I only see #84738 right now, which is caused by the missing visiting of associated items in IntraLinkCrateLoader with 99% certainty.
I'd also add visiting for at least fields and enum variants in the same PR, because the doc link resolver scans them as well. I'm not sure which other nodes need to be visited, I've seen some relevant enum somewhere in rustdoc, but don't remember how it's called.

@petrochenkov petrochenkov 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2021
@jyn514
Copy link
Member Author

jyn514 commented Nov 5, 2021

Which regressions?

#84738 is the only one I know of, but lots of people have been hitting it, and it was hit the last time we tried to land this patch too.

I'd also add visiting for at least fields and enum variants in the same PR, because the doc link resolver scans them as well. I'm not sure which other nodes need to be visited, I've seen some relevant enum somewhere in rustdoc, but don't remember how it's called.

Right, yes, this is what I mean, there's no guarantee that fixing one issue will fix the others, and it happens rarely enough that usually the bug doesn't get caught until the patch hits stable. And I'm not sure what benefit it brings, no one is using #68427 in practice.

@jyn514
Copy link
Member Author

jyn514 commented Nov 5, 2021

I just spent about half an hour to come up with the following impls for the visitor:

    fn visit_foreign_item(&mut self, item: &ast::ForeignItem) {
        self.load_links_in_attrs(&item.attrs, item.span);
        visit::walk_foreign_item(self, item)
    }

    // NOTE: if doc-comments are ever allowed on function parameters, this will have to implement `visit_param` too.

    fn visit_assoc_item(&mut self, item: &ast::AssocItem, ctxt: visit::AssocCtxt) {
        self.load_links_in_attrs(&item.attrs, item.span);
        visit::walk_assoc_item(self, item, ctxt)
    }

    fn visit_field_def(&mut self, field: &ast::FieldDef) {
        self.load_links_in_attrs(&field.attrs, field.span);
        visit::walk_field_def(self, field)
    }

    fn visit_variant(&mut self, v: &ast::Variant) {
        self.load_links_in_attrs(&v.attrs, v.span);
        visit::walk_variant(self, v)
    }
}

finding #90610 in the process, and I'm still not sure I got them all. If you really think this is a good idea, I'd suggest adding a visit_attr_list function to Visitor so I don't have to rewrite all of these and be worried I'm missing one. But again, I think this is really high risk for the little benefit we get from it.

@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 5, 2021
@petrochenkov
Copy link
Contributor

I've seen some relevant enum somewhere in rustdoc

It's ItemKind, and Items are constructed from AST by the impl Clean<Item> for ... function.
So the visitor methods above should cover all of the required AST nodes.
These methods are necessary for #83761 anyway, so let's land them now.
(Also could you add test cases for all these new nodes loading crates through doc links?)

The only remaining potential source of missing extern crates that I'm aware of is reexport inlining.
If you can build a test case with a link on an import attempting to access a crate not previously loaded by IntraLinkCrateLoader, then we can land the "load all external crates" hack as well, because I don't want you to implement all that reexport walking now (it should be implemented in #88679, but it may cause perf issues there).

@petrochenkov petrochenkov 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2021
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 11, 2021

📌 Commit 51345a8 has been approved by petrochenkov

@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 Nov 11, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 11, 2021
…trochenkov

rustdoc: Go back to loading all external crates unconditionally

This *continues* to cause regressions. This code will be unnecessary
once access to the resolver happens fully before creating the tyctxt
(rust-lang#83761), so load all crates unconditionally for now. To minimize churn, this leaves in the code for loading crates selectively.

"Fixes" rust-lang#84738. Previously: rust-lang#83738, rust-lang#85749, rust-lang#88215

r? `@petrochenkov` cc `@camelid` (this should fix the "index out of bounds" error you had while looking up `crate_name`).
@apiraino
Copy link
Contributor

backport suspended, waiting on #90489 (Zulip discussion)

@jyn514
Copy link
Member Author

jyn514 commented Nov 11, 2021

@apiraino I don't understand your comment - #90489 is this PR. My understanding from the meeting is they were just waiting for this PR to stop changing before approving a backport (which should be the case now; this isn't the sort of change I would expect to pass PR CI but fail bors).

@apiraino
Copy link
Contributor

apiraino commented Nov 11, 2021

Beta backport approved as per compiler team on Zulip

(stable backport decision deferred to next week)

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Nov 11, 2021
@bors
Copy link
Contributor

bors commented Nov 11, 2021

⌛ Testing commit 51345a8 with merge 14a2fd6...

@bors
Copy link
Contributor

bors commented Nov 12, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 14a2fd6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 12, 2021
@bors bors merged commit 14a2fd6 into rust-lang:master Nov 12, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 12, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (14a2fd6): comparison url.

Summary: This change led to large relevant mixed results 🤷 in compiler performance.

  • Large improvement in instruction counts (up to -3.9% on incr-unchanged builds of deep-vector)
  • Large regression in instruction counts (up to 2.8% on full builds of cargo)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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

@rustbot rustbot added the perf-regression Performance regression. label Nov 12, 2021
@cuviper cuviper mentioned this pull request Nov 16, 2021
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 16, 2021
@cuviper cuviper modified the milestones: 1.58.0, 1.57.0 Nov 16, 2021
@apiraino
Copy link
Contributor

stable backport declined as per compiler team on Zulip (and previous comments)

@rustbot label -stable-nominated

@rustbot rustbot removed the stable-nominated Nominated for backporting to the compiler in the stable channel. label Nov 18, 2021
@jyn514 jyn514 deleted the load-all-extern-crates branch November 18, 2021 17:57
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 19, 2021
[beta] backports

-  Fix assertion failures in OwnedHandle with windows_subsystem. rust-lang#88798
-  Ensure that pushing empty path works as before on verbatim paths rust-lang#89665
-  Feature gate + make must_not_suspend allow-by-default rust-lang#89826
-  Only use clone3 when needed for pidfd rust-lang#89930
-  Fix documentation header sizes rust-lang#90186
-  Fixes incorrect handling of ADT's drop requirements rust-lang#90218
-  Fix ICE when forgetting to Box a parameter to a Self::func call rust-lang#90221
-  Prevent duplicate caller bounds candidates by exposing default substs in Unevaluated rust-lang#90266
-  Update odht crate to 0.3.1 (big-endian bugfix) rust-lang#90403
-  rustdoc: Go back to loading all external crates unconditionally rust-lang#90489
-  Split doc_cfg and doc_auto_cfg features rust-lang#90502
-  Apply adjustments for field expression even if inaccessible rust-lang#90508
-  Warn for variables that are no longer captured rust-lang#90597
-  Properly register text_direction_codepoint_in_comment lint. rust-lang#90626
-  CI: Use ubuntu image to download openssl, curl sources, cacert.pem for x86 dist builds rust-lang#90457
-  Android is not GNU rust-lang#90834
-  Update llvm submodule rust-lang#90954

Additionally, this bumps the stage 0 compiler from beta to stable 1.56.1.

r? `@Mark-Simulacrum`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name A-metadata Area: Crate metadata A-resolve Area: Name resolution beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Index out of bounds when running cargo doc in rustc_metadata/src/creader.rs