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.

Sign up for GitHub

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

Remove CURRENT_DEPTH thread-local variable #82815

Closed
wants to merge 4 commits into from
Closed

Remove CURRENT_DEPTH thread-local variable #82815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3f1ab9d
Don't use `CURRENT_DEPTH` in `clean/types.rs`
camelid Mar 4, 2021
a3f56b3
Remove many uses of `CURRENT_DEPTH` from `html/format.rs`
camelid Mar 4, 2021
80a0dc2
Remove last uses of `CURRENT_DEPTH` in `html/format.rs`
camelid Mar 4, 2021
0e1ff40
Remove `CURRENT_DEPTH`
camelid Mar 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/librustdoc/clean/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2292,14 +2292,16 @@ impl Clean<Item> for (&hir::MacroDef<'_>, Option<Symbol>) {
if matchers.len() <= 1 {
format!(
"{}macro {}{} {{\n ...\n}}",
vis.print_with_space(cx.tcx, def_id, &cx.cache),
// FIXME(camelid): this might create broken links!
vis.print_with_space(cx.tcx, def_id, &cx.cache, 0),
Comment on lines -2295 to +2296
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this has the same behavior as before, but we should probably test it.

Copy link
Member

Choose a reason for hiding this comment

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

I think this will break for macros in submodules (e.g. pub mod inner { pub macro foo() {} }).

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, so it looks like we don't use links in macro visibilities on nightly anyway:

#![feature(decl_macro)]

pub mod inner1 {
    pub mod inner2 {
        pub mod inner3 {
            pub(in crate::inner1) macro foo() {}
        }
    }
}

image

So I'm not sure how to test the changes :/

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, so it looks like we don't use links in macro visibilities on nightly anyway:

That sounds like a bug, could you open an issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Issue: #83000. Nice issue number :)

name,
matchers.iter().map(|span| span.to_src(cx)).collect::<String>(),
)
} else {
format!(
"{}macro {} {{\n{}}}",
vis.print_with_space(cx.tcx, def_id, &cx.cache),
// FIXME(camelid): this might create broken links!
vis.print_with_space(cx.tcx, def_id, &cx.cache, 0),
Comment on lines -2302 to +2304
Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above.

name,
matchers
.iter()
Expand Down
14 changes: 5 additions & 9 deletions src/librustdoc/clean/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,8 @@ impl Item {
self.attrs.collapsed_doc_value()
}

crate fn links(&self, cache: &Cache) -> Vec<RenderedLink> {
self.attrs.links(self.def_id.krate, cache)
crate fn links(&self, cache: &Cache, depth: usize) -> Vec<RenderedLink> {
self.attrs.links(self.def_id.krate, cache, depth)
}

crate fn is_crate(&self) -> bool {
Expand Down Expand Up @@ -847,16 +847,15 @@ impl Attributes {
/// Gets links as a vector
///
/// Cache must be populated before call
crate fn links(&self, krate: CrateNum, cache: &Cache) -> Vec<RenderedLink> {
crate fn links(&self, krate: CrateNum, cache: &Cache, depth: usize) -> Vec<RenderedLink> {
use crate::html::format::href;
use crate::html::render::CURRENT_DEPTH;

self.links
.iter()
.filter_map(|ItemLink { link: s, link_text, did, fragment }| {
match *did {
Some(did) => {
if let Some((mut href, ..)) = href(did, cache) {
if let Some((mut href, ..)) = href(did, cache, depth) {
if let Some(ref fragment) = *fragment {
href.push('#');
href.push_str(fragment);
Expand All @@ -873,10 +872,7 @@ impl Attributes {
None => {
if let Some(ref fragment) = *fragment {
let url = match cache.extern_locations.get(&krate) {
Some(&(_, _, ExternalLocation::Local)) => {
let depth = CURRENT_DEPTH.with(|l| l.get());
"../".repeat(depth)
}
Some(&(_, _, ExternalLocation::Local)) => "../".repeat(depth),
Some(&(_, _, ExternalLocation::Remote(ref s))) => s.to_string(),
Some(&(_, _, ExternalLocation::Unknown)) | None => String::from(
// NOTE: intentionally doesn't pass crate name to avoid having
Expand Down
5 changes: 4 additions & 1 deletion src/librustdoc/clean/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,10 @@ crate fn resolve_type(cx: &mut DocContext<'_>, path: Path, id: hir::HirId) -> Ty
return Generic(kw::SelfUpper);
}
Res::Def(DefKind::TyParam, _) if path.segments.len() == 1 => {
return Generic(Symbol::intern(&format!("{:#}", path.print(&cx.cache))));
// FIXME(camelid): I hope passing 0 as the depth doesn't break anything....
// Then again, I think used to be 0 anyway through CURRENT_DEPTH
// because (I think) rendering hadn't started yet.
return Generic(Symbol::intern(&format!("{:#}", path.print(&cx.cache, 0))));
Comment on lines -417 to +420
Copy link
Member Author

Choose a reason for hiding this comment

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

I think same as above.

Copy link
Member

@jyn514 jyn514 Mar 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This change makes me nervous, can you say why you think rendering hadn't started?

I don't know exactly when this path is hit, path.segment.len() == 1 seems really strange because it doesn't leave any space for the crate name and I don't see any other reason to special case 1 path segment. Maybe add an unconditional panic!() so we have a test 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.

This change makes me nervous

I understand, these changes make me a bit nervous too :)

can you say why you think rendering hadn't started?

My understanding is that all cleaning happens before rendering, so CURRENT_DEPTH is always its initial value of 0. Is that not true?

Maybe add an unconditional panic!() so we have a test case?

Hmm, can you explain more what you mean?

Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that all cleaning happens before rendering, so CURRENT_DEPTH is always its initial value of 0. Is that not true?

Ok, this makes sense to me. The depth is only updated in context.rs:


html/render/context.rs
22:    CURRENT_DEPTH, INITIAL_IDS,
107:        CURRENT_DEPTH.with(|slot| {
421:        CURRENT_DEPTH.with(|s| s.set(0));

and cleaning never happens in html/:

$ rg '\.clean' html/ | wc -l
0

Copy link
Member

Choose a reason for hiding this comment

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

Maybe add an unconditional panic!() so we have a test case?

Hmm, can you explain more what you mean?

I don't know when this line of code will be executed. Can you change it to an unconditional panic and document the standard library so I can see a backtrace of how it happens? If it never happens that probably means the whole thing is unreachable and we don't have to worry about it in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you change it to an unconditional panic and document the standard library so I can see a backtrace of how it happens? If it never happens that probably means the whole thing is unreachable and we don't have to worry about it in the first place.

@jyn514 It turns out it is reachable! It panicked while documenting core, specifically this path:

DEBUG rustdoc::clean::utils resolve_type(Path { global: false, res: Def(TyParam, DefId(0:140 ~ core[8787]::f32::{impl#0}::to_int_unchecked::Int)), segments: [PathSegment { name: "Int", args: AngleBracketed { args: [], bindings: [] } }] },HirId { owner: DefId(0:139 ~ core[8787]::f32::{impl#0}::to_int_unchecked), local_id: 18 })

I think that path is the Int in the return type of f32::to_int_unchecked().

Copy link
Member Author

Choose a reason for hiding this comment

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

I think passing a depth of 0 should probably be fine since IIUC paths that resolve to type parameters shouldn't be able to have type parameters themselves...

Copy link
Member

Choose a reason for hiding this comment

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

Ok, that makes sense to me. Could you add a comment about a) when this happens and b) why the depth of 0 is ok?

}
Res::SelfTy(..) | Res::Def(DefKind::TyParam | DefKind::AssocTy, _) => true,
_ => false,
Expand Down
Loading