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: Add more semantic information to impl IDs #92745

Closed
wants to merge 1 commit into from

Conversation

pierwill
Copy link
Member

@pierwill pierwill commented Jan 10, 2022

Instead of generating #impl, #impl-1, etc., generate IDs like #impl-Foo<M>.

Part of #92052.

@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 10, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 10, 2022
@pierwill pierwill marked this pull request as draft January 10, 2022 21:36
@pierwill
Copy link
Member Author

@rustbot author

@rustbot rustbot 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 Jan 10, 2022
@pierwill pierwill changed the title rustdoc: Add more semantic information to impl ids rustdoc: Semantically disambiguate impls and associated impl items rather than using numeric suffixes Jan 10, 2022
@rust-log-analyzer

This comment has been minimized.

@pierwill pierwill changed the title rustdoc: Semantically disambiguate impls and associated impl items rather than using numeric suffixes rustdoc: Semantically disambiguate impls Jan 10, 2022
@pierwill
Copy link
Member Author

r? @camelid

@pierwill pierwill marked this pull request as ready for review January 10, 2022 21:59
@pierwill
Copy link
Member Author

@rustbot ready

@rustbot rustbot 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 Jan 10, 2022
@camelid
Copy link
Member

camelid commented Jan 10, 2022

Can you update the tests?

@rustbot author

@rustbot rustbot 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 Jan 10, 2022
@pierwill
Copy link
Member Author

Can you update the tests?

@rustbot author

Should x test src/test/rustdoc --bless work for this? I am getting many errors that don't seem related...

@camelid
Copy link
Member

camelid commented Jan 10, 2022

No, you'll need to update the @has comments manually. Basically, go through each test that fails, and update the comments starting with @has so the expected URL fragment (the #foo or id="foo" part) is what you expect after your change.

@pierwill
Copy link
Member Author

I'm getting panics on at least one test that has no @has:

---- [rustdoc] rustdoc/async-move-doctest.rs stdout ----

error: rustdoc failed!
status: exit status: 101
command: "/Users/user/repos/rust/build/x86_64-apple-darwin/stage1/bin/rustdoc" "-L" "/Users/user/repos/rust/build/x86_64-apple-darwin/stage1/lib/rustlib/x86_64-apple-darwin/lib" "-L" "/Users/user/repos/rust/build/x86_64-apple-darwin/test/rustdoc/async-move-doctest/auxiliary" "-o" "/Users/user/repos/rust/build/x86_64-apple-darwin/test/rustdoc/async-move-doctest" "--deny" "warnings" "/Users/user/repos/rust/src/test/rustdoc/async-move-doctest.rs" "--test" "--edition=2018"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
thread 'rustc' panicked at 'SESSION_GLOBALS should never be overwritten! Use another thread if you need another SessionGlobals', /Users/user/repos/rust/compiler/rustc_span/src/lib.rs:103:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.60.0-dev running on x86_64-apple-darwin

query stack during panic:
end of query stack

------------------------------------------

@camelid
Copy link
Member

camelid commented Jan 10, 2022

Hmm, try rm -r build/<your-machine-arch>/stage* and then re-running tests. Might just be corruption in your build directory.

@pierwill
Copy link
Member Author

Still getting nasty panics. 😬

@camelid
Copy link
Member

camelid commented Jan 10, 2022

Weird, what's your config.toml?

@pierwill
Copy link
Member Author

pierwill commented Jan 10, 2022

# Includes one of the default files in src/bootstrap/defaults
profile = "compiler"
changelog-seen = 2

[rust]
incremental = true

# deny-warnings = false

debug = true
debug-assertions = false
debuginfo-level = 1
debuginfo-level-rustc = 1
debuginfo-level-std = 1

run-dsymutil = false

parallel-compiler = true

[build]
compiler-docs = true

@camelid
Copy link
Member

camelid commented Jan 10, 2022

Oh, parallel-compiler doesn't work properly. You should disable it. I think that's what's causing the panics.

I'm curious, why did you enable it? Did you see it recommended somewhere?

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot 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 Jan 13, 2022
@@ -14,4 +14,4 @@ press-key: "Enter"
// Waiting for the search results to appear...
wait-for: "#titles"
// We check that there is no more "test_docs" appearing.
assert-false: "#results .externcrate"
// assert-false: "#results .externcrate"
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how we want to handle this. @GuillaumeGomez

Copy link
Member

Choose a reason for hiding this comment

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

If the result appears, then it means the search filter is broken. Try to do on your browser what it's doing in this and you should see pretty quickly what's wrong. If you can't figure it out, I can take a look in the next days.

Please revert this change in any case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted.

Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@pierwill pierwill changed the title rustdoc: Semantically disambiguate impls rustdoc: Add more semantic information to impl IDs Jan 13, 2022
@pierwill pierwill force-pushed the rustdoc-disamb-impls branch 2 times, most recently from a928fc0 to cd4813c Compare January 13, 2022 21:54
@rust-log-analyzer

This comment has been minimized.

@camelid
Copy link
Member

camelid commented Jan 14, 2022

Ok, now you're seeing all the broken links in the standard library 😅

Before you go to the effort to update all of those, we should get confirmation from the team that this new anchor formatting looks good to them.

@GuillaumeGomez
Copy link
Member

I said it on zulip but doesn't hurt to say it here as well: I agree with this change (good luck updating all std URLs 😉 ).

@pierwill
Copy link
Member Author

@rustbot author

@rustbot rustbot 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 Jan 14, 2022
@jsha
Copy link
Contributor

jsha commented Jan 16, 2022

I've put up a demo of this branch at https://rustdoc.crud.net/jsha/rustdoc-disamb-impls/std/fs/struct.File.html#impl-AsFd-for-File (@pierwill, if you'd like an account on this server so you can upload demos, PM me on Zulip! I'm @jsha there too).

I spotted the broken links issue. If you look in the sidebar for AsFd, and click on it, you're taken to https://rustdoc.crud.net/jsha/rustdoc-disamb-impls/std/fs/struct.File.html#impl-AsFd. But the new fragment is #impl-AsFd-for-File, so that's a broken link.

I think this is the code you need to tweak:

let format_impls = |impls: Vec<&Impl>| {
let mut links = FxHashSet::default();
let mut ret = impls
.iter()
.filter_map(|it| {
if let Some(ref i) = it.inner_impl().trait_ {
let i_display = format!("{:#}", i.print(cx));
let out = Escape(&i_display);
let encoded = small_url_encode(format!("{:#}", i.print(cx)));
let prefix = match it.inner_impl().polarity {
ty::ImplPolarity::Positive | ty::ImplPolarity::Reservation => "",
ty::ImplPolarity::Negative => "!",
};
let generated =
format!("<a href=\"#impl-{}\">{}{}</a>", encoded, prefix, out);
if links.insert(generated.clone()) { Some(generated) } else { None }
} else {
None
}
})
.collect::<Vec<String>>();
.

@pierwill pierwill force-pushed the rustdoc-disamb-impls branch from cd4813c to a6f6b2f Compare January 19, 2022 17:25
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@pierwill
Copy link
Member Author

I do still want to finish this. 🤓

Instead of generating `#impl`, `#impl-1`, etc., generate IDs
like `#impl-Foo<M>`.

Co-authored-by: Noah Lev <camelidcamel@gmail.com>
@pierwill pierwill force-pushed the rustdoc-disamb-impls branch 2 times, most recently from 604b874 to 083cf2a Compare January 31, 2022 22:20
@bors
Copy link
Contributor

bors commented Feb 5, 2022

☔ The latest upstream changes (presumably #93655) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 6, 2022
@JohnCSimon
Copy link
Member

@pierwill

Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you force push to it, else you won't be able to reopen.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Apr 23, 2022
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Apr 23, 2022
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 6, 2022
…, r=notriddle

rustdoc: Add more semantic information to impl IDs

Take over of rust-lang#92745.

I fixed the last remaining issue for the links in the sidebar (mentioned by `@jsha)` and fixed the few links broken in the std/core docs.

cc `@camelid`
r? `@notriddle`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

9 participants