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: Render visibilities succinctly #80368

Merged
merged 14 commits into from
Jan 1, 2021

Conversation

camelid
Copy link
Member

@camelid camelid commented Dec 25, 2020

Fixes #79139.

r? @jyn514

@camelid camelid added A-visibility Area: Visibility / privacy T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 25, 2020
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 25, 2020
src/librustdoc/clean/mod.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@camelid camelid force-pushed the rustdoc-succinct-vis branch from 442675d to 4a4e038 Compare December 25, 2020 20:05
@camelid
Copy link
Member Author

camelid commented Dec 25, 2020

Well, it looks like there's an issue somewhere because about 2/3 of the rustdoc tests are failing! It seems there's some ICEing, I assume from expect_local().

@camelid
Copy link
Member Author

camelid commented Dec 25, 2020

So I guess I'll have to change the approach. I can't just use tcx.parent_module_from_def_id because that requires a LocalDefId, and we're not always documenting local items (e.g. when documenting impls of non-local traits on local types). How do I get the parent module of a non-local item then?

@jyn514
Copy link
Member

jyn514 commented Dec 25, 2020

How do I get the parent module of a non-local item then?

Can you refactor

let mut current = item.def_id;
// The immediate parent might not always be a module.
// Find the first parent which is.
loop {
if let Some(parent) = self.cx.tcx.parent(current) {
if self.cx.tcx.def_kind(parent) == DefKind::Mod {
break Some(parent);
}
current = parent;
} else {
debug!(
"{:?} has no parent (kind={:?}, original was {:?})",
current,
self.cx.tcx.def_kind(current),
item.def_id
);
break None;
}
}
into a function?

@jyn514
Copy link
Member

jyn514 commented Dec 25, 2020

(Ideally that would be part of rustc directly but there was some controversy about that: #77984 (comment))

@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 Dec 25, 2020
src/librustdoc/clean/mod.rs Outdated Show resolved Hide resolved
src/librustdoc/clean/mod.rs Outdated Show resolved Hide resolved
src/librustdoc/html/format.rs Outdated Show resolved Hide resolved
src/librustdoc/html/format.rs Outdated Show resolved Hide resolved
src/librustdoc/html/format.rs Outdated Show resolved Hide resolved
@camelid camelid force-pushed the rustdoc-succinct-vis branch from 4a4e038 to bbbfd07 Compare December 25, 2020 23:39
@rust-log-analyzer

This comment has been minimized.

@camelid camelid force-pushed the rustdoc-succinct-vis branch from bbbfd07 to 50c1c27 Compare December 25, 2020 23:53
src/librustdoc/clean/mod.rs Outdated Show resolved Hide resolved
src/librustdoc/clean/utils.rs Outdated Show resolved Hide resolved
src/librustdoc/html/format.rs Outdated Show resolved Hide resolved
src/librustdoc/html/format.rs Show resolved Hide resolved
src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved
src/test/rustdoc/decl_macro_priv.rs Outdated Show resolved Hide resolved
src/test/rustdoc/visibility.rs Outdated Show resolved Hide resolved
@camelid camelid force-pushed the rustdoc-succinct-vis branch from 21a6fec to b959c75 Compare December 26, 2020 00:20
@camelid camelid changed the title rustdoc: Render visibilities more succinctly rustdoc: Render visibilities succinctly Dec 26, 2020
@camelid camelid marked this pull request as ready for review December 26, 2020 00:37
@camelid
Copy link
Member Author

camelid commented Dec 26, 2020

Ready for another review!

@rustbot label +S-waiting-on-review -S-waiting-on-author

@camelid
Copy link
Member Author

camelid commented Dec 31, 2020

Nominating for beta backport: this fixes a beta regression that makes visibilities shown in the output from --document-private-items verbose and confusing.

@jyn514
Copy link
Member

jyn514 commented Dec 31, 2020

I agree we should backport this, it fixes a regression introduced in 1.50, and happened to miss the cutoff and landed in 1.51 instead.

@rust-lang/rustdoc how do you feel about backporting this?

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 1, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#80323 (Update and improve `rustc_codegen_{llvm,ssa}` docs)
 - rust-lang#80368 (rustdoc: Render visibilities succinctly)
 - rust-lang#80514 (Fix broken ./x.py install)
 - rust-lang#80519 (Take type defaults into account in suggestions to reorder generic parameters)
 - rust-lang#80526 (Update LLVM)
 - rust-lang#80532 (remove unnecessary trailing semicolon from bootstrap)
 - rust-lang#80548 (FIx ICE on wf check for foreign fns)
 - rust-lang#80551 (support pattern as const parents in type_of)

Failed merges:

 - rust-lang#80547 (In which we start to parse const generics defaults)

r? `@ghost`
`@rustbot` modify labels: rollup
@camelid
Copy link
Member Author

camelid commented Jan 1, 2021

Also, beta was cut just a few days ago so it's not like beta's about to hit stable: We have time to catch any potential issues.

@bors bors merged commit 7d247c9 into rust-lang:master Jan 1, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 1, 2021
@GuillaumeGomez
Copy link
Member

I think we should backport it as well.

@camelid camelid deleted the rustdoc-succinct-vis branch January 1, 2021 18:18
@camelid
Copy link
Member Author

camelid commented Jan 1, 2021

Should this be marked as beta-accepted then?

@GuillaumeGomez
Copy link
Member

I *think* it requires someone from the infra. cc @pietroalbini

@camelid
Copy link
Member Author

camelid commented Jan 1, 2021

My understanding (though I may be wrong) is that beta-accepted is used by the team responsible for the code (in this case, T-rustdoc) to say that it should be backported. Then someone (usually Mark-Simulacrum) goes through all the beta-accepted PRs and backports them.

@jyn514
Copy link
Member

jyn514 commented Jan 1, 2021

Oh, I was going to wait for @Manishearth to give an opinion. We as a team should probably come up with some policy for this.

@GuillaumeGomez
Copy link
Member

For now, half the members agreed on it so let's add the label. But as many things, we'll need to formalize it.

@GuillaumeGomez GuillaumeGomez added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Jan 1, 2021
@pietroalbini pietroalbini added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 4, 2021
@pietroalbini
Copy link
Member

@GuillaumeGomez so, the policy for beta backports is the following:

  1. Someone nominates the PR by adding the beta-nominated label and the relevant team label.
  2. That team agrees to the nomination with whatever consensus mechanism they want, applies the beta-accepted label and keeps the beta-nominated label.
  3. Someone from the release team looks through all the PRs with both the beta-nominated and beta-accepted labels, backports them and keeps just the beta-accepted label.

@GuillaumeGomez
Copy link
Member

Thanks for the information! Is there a document available somewhere with the different processes? It would be very useful.

@pietroalbini
Copy link
Member

The docs for backporting is https://forge.rust-lang.org/release/beta-backporting.html. I'm not sure if there is a document describing the processes of each team.

camelid added a commit to camelid/rust that referenced this pull request Jan 6, 2021
`find_nearest_parent_module` was extracted from
`rustdoc::passes::collect_intra_doc_links` in rust-lang#80368.

We (me and Joshua Nelson) thought at the time that it should be in rustc
instead, and Joshua suggested it be a method on `TyCtxt`.
However, since rust-lang#80368 was fixing a significant regression that was about
to land on beta, we agreed that to be able to fix this quickly I should
leave the code in rustdoc.

This is a followup PR to move it to `TyCtxt`.
@Mark-Simulacrum
Copy link
Member

@camelid just a minor note for the future - would be good to try to rebase before r+ing to squash out any minor fixup commits to try and keep commit history clean :)

@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.51.0, 1.50.0 Jan 18, 2021
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 18, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2021
…ulacrum

[beta] backports

This backports:

*  Update RLS and Rustfmt rust-lang#81027
*  bump rustfmt to v1.4.32 rust-lang#81093
*  Fix handling of malicious Readers in read_to_end rust-lang#80895
*  Fix broken ./x.py install rust-lang#80514
*  Fix x.py install not working with relative prefix rust-lang#80797
*  [security] Update mdbook rust-lang#80688
*  rustdoc: Render visibilities succinctly rust-lang#80368

r? `@ghost`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-visibility Area: Visibility / privacy beta-accepted Accepted for backporting to the compiler in the beta channel. 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.

regression: Rustdoc shows pub(self) as pub(in a::b)
9 participants