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: Pre-calculate traits that are in scope for doc links #88679

Merged
merged 1 commit into from
Jan 26, 2022

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Sep 5, 2021

This eliminates one more late use of resolver (part of #83761).
At early doc link resolution time we go through parent modules of items from the current crate, reexports of items from other crates, trait items, and impl items collected by collect-intra-doc-links pass, determine traits that are in scope in each such module, and put those traits into a map used by later rustdoc passes.
r? @jyn514

@rust-highfive
Copy link
Collaborator

Some changes occurred in intra-doc-links.

cc @jyn514

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 5, 2021
@jyn514
Copy link
Member

jyn514 commented Sep 6, 2021

@bors try @rust-timer queue

Since encoding and decoding are performance sensitive.

@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 Sep 6, 2021
@bors
Copy link
Contributor

bors commented Sep 6, 2021

⌛ Trying commit 1514df2159ed22849eaddfd75c5e57cf801de6d4 with merge 2aadece0d9989be6855d4e96bac0f4ec08f23c91...

@rust-log-analyzer

This comment has been minimized.

@bors

This comment has been minimized.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 6, 2021
@jyn514 jyn514 added A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name A-resolve Area: Name resolution T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Sep 6, 2021
@bors

This comment has been minimized.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Sep 12, 2021

Blocked on #88677 and #88872.

@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 12, 2021
@mati865
Copy link
Contributor

mati865 commented Sep 12, 2021

Typo? #88679 is this PR.

@petrochenkov
Copy link
Contributor Author

@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 Sep 16, 2021
@bors
Copy link
Contributor

bors commented Sep 16, 2021

⌛ Trying commit 1ed1f29e01aedafd3a4dcb253de4be5da9a9eade with merge eec63217b0c2bd2bd82596fe0f8f6ccc5b9cc1a0...

@bors
Copy link
Contributor

bors commented Jan 26, 2022

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 788b1fe to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 26, 2022
@bors bors merged commit 788b1fe into rust-lang:master Jan 26, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 26, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (788b1fe): comparison url.

Summary: This benchmark run shows 43 relevant regressions 😿 to instruction counts.

  • Average relevant regression: 5.4%
  • Largest regression in instruction counts: 9.1% on full builds of regression-31157 doc

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

@trevyn
Copy link
Contributor

trevyn commented Jan 28, 2022

@petrochenkov FYI, this seems to have broken one of my nightly CI runs, not sure if that's expected:

https://github.com/trevyn/turbosql/runs/4960657119?check_suite_focus=true

 Documenting turbosql-impl v0.4.0 (/home/runner/work/turbosql/turbosql/turbosql-impl)
Error: thread 'rustc' panicked at 'no entry found for key', src/librustdoc/passes/collect_intra_doc_links.rs:929:16
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

error: internal compiler error: unexpected panic

error: Unrecognized option: 'crate-version'

error: could not document `turbosql-impl`

@petrochenkov
Copy link
Contributor Author

@trevyn
Could you make a minimized self-contained reproduction?
I'll look at this tomorrow, and having such a reproduction would make it much faster.

@trevyn
Copy link
Contributor

trevyn commented Jan 28, 2022

Ok, apparently this is enough to trigger it on my side, not sure I feel like minimizing rusqlite right now:

Cargo.toml:

[package]
edition = "2018"
name = "test"
version = "0.1.0"

[dependencies]
rusqlite = "0.26"

lib.rs:

use rusqlite;

trevyn added a commit to trevyn/turbosql that referenced this pull request Jan 28, 2022
trevyn added a commit to trevyn/turbonet that referenced this pull request Jan 28, 2022
@jyn514
Copy link
Member

jyn514 commented Jan 28, 2022

@petrochenkov #93428

@petrochenkov
Copy link
Contributor Author

The panic in #88679 (comment) happens because an inherent impl from rusqlite (impl Connection from mod busy) gets inlined into the current crate.

How is that even possible?
If I remove the doc link to make the documentation process to succeed, then that impl is not even in the generated documentation.

@camelid
Copy link
Member

camelid commented Jan 29, 2022

I think it would happen if Connection was inlined. Is Connection inlined?

@petrochenkov
Copy link
Contributor Author

Maybe it's inlined internally, but it doesn't end up in the generated doc.
The documented crate in #88679 (comment) is just a single private import (it doesn't reexport Connection in particular), nothing should be inlined in that case.

@camelid
Copy link
Member

camelid commented Jan 31, 2022

Weird. I have no idea why that impl is being inlined then.

@petrochenkov
Copy link
Contributor Author

I didn't figure out why that specific impl was inlined, but there are legitimate cases where inherent impls are inlined correctly, but traits in scope are not collected for them:

// Dependency crate

#[derive(Clone)]
pub struct PublicStruct;

mod inner {
    use super::PublicStruct;

    impl PublicStruct {
        /// [PublicStruct::clone]
        pub fn method() {}
    }
}
// Main crate

pub use dependency::PublicStruct;

@petrochenkov
Copy link
Contributor Author

Both cases (#88679 (comment) and #88679 (comment)) are fixed in #93539.

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2022
…elwoerister

rustdoc: Collect traits in scope for foreign inherent impls

Inherent impls can be inlined for variety of reasons (impls of reexported types, impls available through `Deref`, impls inlined for unclear reasons like in rust-lang#88679 (comment)).
If an impl is inlined, then doc links in its comments are resolved and we may need the set of traits that are in scope at that impl's definition point.
So in this PR we simply collect traits in scope for *all* inherent impls from other crates if their `Self` type is public, which is very similar for the strategy for trait impls previously used in rust-lang#88679.

Fixes rust-lang#93476
Fixes rust-lang#88679 (comment)
Fixes rust-lang#88679 (comment)
dario23 added a commit to dario23/rust-semverver that referenced this pull request May 6, 2022
jyn514 added a commit to jyn514/rust that referenced this pull request Sep 20, 2022
Since rust-lang#88679, rustdoc doesn't load crates eagerly.
Add an explicit `extern crate` item to make sure the crate is loaded and the bug reproduces.
You can verify this fix by adding `// compile-flags: -Znormalizing-docs` and running the test.
spastorino added a commit to spastorino/rust that referenced this pull request Sep 20, 2022
…ompiler-errors

Make the `normalize-overflow` rustdoc test actually do something

Since rust-lang#88679, rustdoc doesn't load crates eagerly. Add an explicit `extern crate` item to make sure the crate is loaded and the bug reproduces.
You can verify this fix by adding `// compile-flags: -Znormalize-docs` and running the test to make sure it gives an error.
notriddle added a commit to notriddle/rust that referenced this pull request Sep 20, 2022
…ompiler-errors

Make the `normalize-overflow` rustdoc test actually do something

Since rust-lang#88679, rustdoc doesn't load crates eagerly. Add an explicit `extern crate` item to make sure the crate is loaded and the bug reproduces.
You can verify this fix by adding `// compile-flags: -Znormalize-docs` and running the test to make sure it gives an error.
notriddle added a commit to notriddle/rust that referenced this pull request Sep 20, 2022
…ompiler-errors

Make the `normalize-overflow` rustdoc test actually do something

Since rust-lang#88679, rustdoc doesn't load crates eagerly. Add an explicit `extern crate` item to make sure the crate is loaded and the bug reproduces.
You can verify this fix by adding `// compile-flags: -Znormalize-docs` and running the test to make sure it gives an error.
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 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. 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.