Remove `CURRENT_DEPTH` thread-local variable #82815

camelid · 2021-03-05T20:20:15Z

Closes #82742. 🎉

This PR has a fairly large diff, but most of the changes are very small - just
threading the new depth parameter through a bunch of functions.

This PR is draft because there are a few things that need to be resolved
before it should be merged. I have marked those spots with review comments.

r? @jyn514

camelid · 2021-03-05T20:24:08Z

src/librustdoc/clean/mod.rs

-                    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),


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

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

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() {} } } }

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

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?

Issue: #83000. Nice issue number :)

camelid · 2021-03-05T20:24:20Z

src/librustdoc/clean/mod.rs

-                    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),


Same as above.

camelid · 2021-03-05T20:24:41Z

src/librustdoc/clean/utils.rs

-            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))));


I think same as above.

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?

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?

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

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.

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().

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...

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?

camelid · 2021-03-05T20:25:47Z

I think this PR will be very conflict-prone FYI.

jyn514

I left some comments on the places where you pointed out a depth of 0. Can you point to where the depth is incremented and decremented? It's hard to find the important changes because the diff is big 😅

jyn514 · 2021-03-05T20:38:48Z

src/librustdoc/clean/mod.rs

-                    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),


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

jyn514 · 2021-03-05T20:41:44Z

src/librustdoc/clean/utils.rs

-            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))));


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?

camelid · 2021-03-05T21:30:25Z

src/librustdoc/html/render/mod.rs

@@ -170,6 +170,10 @@ crate struct SharedContext<'tcx> {
 }

 impl<'tcx> Context<'tcx> {
+    fn depth(&self) -> usize {


I think you wanted me to put docs on this; leaving a comment so I don't forget.

camelid · 2021-03-05T22:37:41Z

Oh no, massive merge conflicts 😅

camelid · 2021-03-05T22:41:46Z

Doing rebase in multiple parts to make it easier—first part done.

camelid · 2021-03-06T00:12:13Z

Finished rebasing. Phew, that was probably one of the hardest rebases I've ever done 😅

camelid · 2021-03-06T00:12:32Z

Still need to squash my "fmt" commit.

camelid · 2021-03-06T00:50:46Z

I formatted each commit individually and dropped the fmt commit. Should be ready to review now!

camelid · 2021-03-06T00:52:16Z

src/librustdoc/passes/calculate_doc_coverage.rs

-                        tr.print(&self.ctx.cache),
-                        impl_.for_.print(&self.ctx.cache),
+                        tr.print(&self.ctx.cache, 0),
+                        impl_.for_.print(&self.ctx.cache, 0),


I used 0 here because this is just for debug output.

camelid · 2021-03-06T00:52:24Z

src/librustdoc/passes/calculate_doc_coverage.rs

-                    debug!("impl {:#} in {}", impl_.for_.print(&self.ctx.cache), filename);
+                    debug!("impl {:#} in {}", impl_.for_.print(&self.ctx.cache, 0), filename);


Same as above.

This part actually wasn't that hard. Ending the use of it in `html/format.rs` is probably going to be *a lot* harder.

Hooray!

camelid · 2021-03-06T03:03:41Z

Rebased. Apparently this is even more conflict-prone than I expected!

bors · 2021-03-22T09:31:29Z

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

jyn514 · 2021-03-24T03:25:43Z

AFAIK, this is waiting on:

I would prefer to hear a response to 1 before spending too much more time on this; the rest I can ignore for now while reviewing.

camelid · 2021-03-25T23:22:39Z

Can you point to where the depth is incremented and decremented?

Where the depth used to be changed or where it is changed now? Or both?

camelid · 2021-03-29T02:32:40Z

The old CURRENT_DEPTH variable used to be updated in two spots. The first one is here:

rust/src/librustdoc/html/render/context.rs

Lines 113 to 117 in cb0e0db

    
           // A little unfortunate that this is done like this, but it sure 
        
           // does make formatting *a lot* nicer. 
        
           CURRENT_DEPTH.with(|slot| { 
        
               slot.set(self.current.len()); 
        
           });

In this spot, it's being updated right before rendering an item, and it's set to the current length of the self.current variable. As I understand it, self.current looks like ["std", "io"]—it's a list of module path segments that we use for links.

self.current is updated here:

rust/src/librustdoc/html/render/context.rs

Line 547 in cb0e0db

self.current.push(item_name.to_owned());

and here:

rust/src/librustdoc/html/render/context.rs

Line 578 in cb0e0db

self.current.pop();

The other place at which CURRENT_DEPTH was updated is here:

rust/src/librustdoc/html/render/context.rs

Line 424 in cb0e0db

CURRENT_DEPTH.with(|s| s.set(0));

This reset is necessary, even though CURRENT_DEPTH is initialized to Cell::new(0), because it is global mutable state that is shared with other Contexts, which may be at a different depth than the current Context.

In the new code (without CURRENT_DEPTH), we update self.current in the same places, and the only change is that we no longer update CURRENT_DEPTH (because it doesn't exist!).

camelid · 2021-03-29T02:33:20Z

Marking as waiting-on-review even though there are conflicts, because they are probably very mechanical to fix and it's probably easier to fix them all at the end or close to the end.

jyn514 · 2021-04-05T16:26:01Z

FYI I plan to review/merge #83237 before this PR, so it's fine not to fix conflicts until then.

rustdoc: use more precise relative URLs This is a fairly large diff, and will probably conflict with rust-lang#82815 since it reduces (but does not eliminate) the use of the old depth variable. Instead of using a depth counter and adding "../" to get to the top, this commit makes rustdoc actually compare the path of what it's linking from to the path that it's linking to. This makes the resulting HTML shorter. Here's a comparison of one of the largest (non-source) files in the Rust standard library docs (about 4% improvement before gzipping). $ wc -c struct.Wrapping.old.html struct.Wrapping.new.html 2387389 struct.Wrapping.old.html 2298538 struct.Wrapping.new.html Most if it can be efficiently gzipped away. $ wc -c struct.Wrapping.old.html.gz struct.Wrapping.new.html.gz 70679 struct.Wrapping.old.html.gz 70050 struct.Wrapping.new.html.gz But it also makes a difference in the final DOM size, reducing it from 91MiB to 82MiB.

jyn514 · 2021-04-18T03:50:19Z

Now that #83237 is merged, I think this approach is no longer necessary - CURRENT_DEPTH is only used for primitive_link and I would rather get rid of that rather than moving the depth into the cache (#83237 (comment)). I think @notriddle may be planning to work on that?

Sorry this won't be merged, I appreciate you putting in the time :)

camelid added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 5, 2021

rust-highfive assigned jyn514 Mar 5, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 5, 2021

camelid mentioned this pull request Mar 5, 2021

rustdoc: Don't use CURRENT_DEPTH in clean/types.rs #82745

Closed

camelid commented Mar 5, 2021

View reviewed changes

jyn514 reviewed Mar 5, 2021

View reviewed changes

camelid commented Mar 5, 2021

View reviewed changes

camelid force-pushed the current_depth-pt2 branch from 806fec1 to bf37b68 Compare March 5, 2021 22:30

camelid force-pushed the current_depth-pt2 branch from bf37b68 to 4747a74 Compare March 5, 2021 22:41

camelid force-pushed the current_depth-pt2 branch from 4747a74 to 49879e3 Compare March 6, 2021 00:11

camelid force-pushed the current_depth-pt2 branch from 49879e3 to 3057e4b Compare March 6, 2021 00:50

camelid commented Mar 6, 2021

View reviewed changes

camelid mentioned this pull request Mar 6, 2021

rustdoc: Cleanup html::render::Context #82356

Merged

This comment has been minimized.

Sign in to view

camelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 6, 2021

camelid added 4 commits March 5, 2021 18:57

Don't use CURRENT_DEPTH in clean/types.rs

3f1ab9d

This part actually wasn't that hard. Ending the use of it in `html/format.rs` is probably going to be *a lot* harder.

Remove many uses of CURRENT_DEPTH from html/format.rs

a3f56b3

Remove last uses of CURRENT_DEPTH in html/format.rs

80a0dc2

Remove CURRENT_DEPTH

0e1ff40

Hooray!

camelid force-pushed the current_depth-pt2 branch from 3057e4b to 0e1ff40 Compare March 6, 2021 03:03

camelid removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 6, 2021

camelid mentioned this pull request Mar 11, 2021

Paths in macro visibilities are not linkified #83000

Closed

camelid 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 11, 2021

camelid mentioned this pull request Mar 15, 2021

Rework rustdoc const type #82873

Merged

notriddle mentioned this pull request Mar 17, 2021

rustdoc: use more precise relative URLs #83237

Merged

camelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 22, 2021

jyn514 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 24, 2021

camelid 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 Mar 29, 2021

jyn514 added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 5, 2021

jyn514 closed this Apr 18, 2021

		debug!("impl {:#} in {}", impl_.for_.print(&self.ctx.cache), filename);
		debug!("impl {:#} in {}", impl_.for_.print(&self.ctx.cache, 0), filename);

Remove CURRENT_DEPTH thread-local variable #82815

Remove CURRENT_DEPTH thread-local variable #82815

Conversation

camelid commented Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelid commented Mar 5, 2021

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelid commented Mar 5, 2021

camelid commented Mar 5, 2021

camelid commented Mar 6, 2021

camelid commented Mar 6, 2021

camelid commented Mar 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

camelid commented Mar 6, 2021

bors commented Mar 22, 2021

jyn514 commented Mar 24, 2021

camelid commented Mar 25, 2021

camelid commented Mar 29, 2021

camelid commented Mar 29, 2021

jyn514 commented Apr 5, 2021

jyn514 commented Apr 18, 2021

Remove `CURRENT_DEPTH` thread-local variable #82815

Remove `CURRENT_DEPTH` thread-local variable #82815

camelid commented Mar 5, 2021 •

edited

Loading

jyn514 Mar 5, 2021 •

edited

Loading

jyn514 Mar 5, 2021 •

edited

Loading