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

Reland #83738: "rustdoc: Don't load all extern crates unconditionally" #88215

Merged
merged 2 commits into from
Aug 27, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Aug 21, 2021

I hopefully found all the bugs 🤞 time for a take two. See the last commit for details on what went wrong before.

r? @petrochenkov (but feel free to reassign to Guillaume if you don't have time.)

Closes #68427. Includes a fix for #84738.

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-resolve Area: Name resolution A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name labels Aug 21, 2021
@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 21, 2021
@rust-log-analyzer

This comment has been minimized.

use crate::html::markdown::markdown_links;
use crate::passes::collect_intra_doc_links::preprocess_link;

// FIXME: this probably needs to consider inlining
Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh, apparently github dropped my comment because I wrote it on a commit instead of the PR itself. Do you happen to have it in your email? If not I can try to rewrite it from memory, it was discussing exactly how this bug could happen and how I don't know how to test it with a UI test.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I don't see any traces of the extended version of this comment in my mail.

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, it went something like this: "inlining" is refering to doc(inline), not #[inline]. The test case for it would look something like:

// inner_crate
pub fn foo() {}

// middle_crate
/// Link to [inner_crate::foo]
pub fn bar() {}

// outer_crate
pub use middle_crate::bar;

In particular, the first time inner_crate will be loaded is when resolving the links on bar, which I don't think this pass will catch because it doesn't look at the attributes of the original definition, only of the re-export.

I'm not sure how to test this with a UI test because it requires three nested crates, and AFAIK aux-build only supports two.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to work fine when testing locally though 🤷 so this is probably ok.

@jyn514 jyn514 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 Aug 22, 2021
@petrochenkov petrochenkov 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 Aug 22, 2021
@petrochenkov
Copy link
Contributor

r=me with #88215 (comment) addressed.

@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 Aug 22, 2021
- All attributes for an item need to be considered at once, they can't
  be considered a line at a time.
- The top-level crate was not being visited. This bug was caught by
  `extern-crate-used-only-in-link`, which I'm very glad I added.
- Make the loader private to the module, so that only one function is
  exposed.
@jyn514
Copy link
Member Author

jyn514 commented Aug 26, 2021

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Aug 26, 2021

📌 Commit c60a370 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 26, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 27, 2021
…arth

Rollup of 11 pull requests

Successful merges:

 - rust-lang#87832 (Fix debugger stepping behavior with `match` expressions)
 - rust-lang#88123 (Make spans for tuple patterns in E0023 more precise)
 - rust-lang#88215 (Reland rust-lang#83738: "rustdoc: Don't load all extern crates unconditionally")
 - rust-lang#88216 (Don't stabilize creation of TryReserveError instances)
 - rust-lang#88270 (Handle type ascription type ops in NLL HRTB diagnostics)
 - rust-lang#88289 (Fixes for LLVM change 0f45c16)
 - rust-lang#88320 (type_implements_trait consider obligation failure on overflow)
 - rust-lang#88332 (Add argument types tait tests)
 - rust-lang#88340 (Add `c_size_t` and `c_ssize_t` to `std::os::raw`.)
 - rust-lang#88346 (Revert "Add type of a let tait test impl trait straight in let")
 - rust-lang#88348 (Add field types tait tests)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 14fb87a into rust-lang:master Aug 27, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 27, 2021
@jyn514 jyn514 deleted the lazy-loading branch August 27, 2021 03:55
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`).
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 11, 2021
…ochenkov

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`).
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-resolve Area: Name resolution 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.

rustdoc only: duplicate lang item panic_halt
6 participants