Remove unnecessary `Option` wrapping around `Crate.module` #83415

camelid · 2021-03-23T18:07:20Z

I'm wondering if it was originally there so that we could take the
module which enables after_krate to take an &Crate. However, the two
impls of after_krate only use Crate.name, so we can pass just the
name instead.

Now that we record the crate's name in its `clean::Item`, pushing the crate name onto the `stack` causes duplicate paths. E.g., the URL generated for the path `::foo::bar::baz` would be something like ../foo/foo/bar/baz With this commit, the URL is corrected to ../foo/bar/baz

I'm wondering if it was originally there so that we could `take` the module which enables `after_krate` to take an `&Crate`. However, the two impls of `after_krate` only use `Crate.name`, so we can pass just the name instead.

rust-highfive · 2021-03-23T18:07:23Z

r? @GuillaumeGomez

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

camelid · 2021-03-23T18:08:04Z

src/librustdoc/fold.rs

@@ -87,7 +87,7 @@ crate trait DocFolder: Sized {
    }

    fn fold_crate(&mut self, mut c: Crate) -> Crate {
-        c.module = c.module.take().and_then(|module| self.fold_item(module));
+        c.module = self.fold_item(c.module).unwrap();


Is there ever a case where this unwrap will fail? Why does fold_item return an Option anyway?

fold_item returns None if the itme was stripped. I don't think the whole crate can be stripped so this should be fine.

camelid · 2021-03-23T18:08:22Z

(The first two commits are from #83399.)

camelid · 2021-03-23T18:13:00Z

src/librustdoc/passes/collect_trait_impls.rs

-        } else {
-            panic!("collect-trait-impls can't run");
-        }
+    let items = if let ModuleItem(Module { ref mut items, .. }) = *krate.module.kind {


Seems like a future enhancement could be to store a Module instead of an Item. Probably we would need to add a few more fields to Module (like DefId and name) to do that.

The previous changes mean that we can now remove this `Option`.

jyn514 · 2021-03-23T21:45:38Z

src/librustdoc/fold.rs

@@ -87,7 +87,7 @@ crate trait DocFolder: Sized {
    }

    fn fold_crate(&mut self, mut c: Crate) -> Crate {
-        c.module = c.module.take().and_then(|module| self.fold_item(module));
+        c.module = self.fold_item(c.module).unwrap();


fold_item returns None if the itme was stripped. I don't think the whole crate can be stripped so this should be fine.

jyn514 · 2021-03-23T21:49:04Z

src/librustdoc/formats/renderer.rs

@@ -102,5 +96,5 @@ crate fn run_format<'tcx, T: FormatRenderer<'tcx>>(
        }
    }
    prof.extra_verbose_generic_activity("renderer_after_krate", T::descr())
-        .run(|| format_renderer.after_krate(&krate, diag))
+        .run(|| format_renderer.after_krate(crate_name, diag))


It took me a second to realize: the reason this changed is because you partially moved out of krate when adding krate.module to the work list.

Yeah, I mentioned that in the PR description:

I'm wondering if it was originally there so that we could take the
module which enables after_krate to take an &Crate. However, the two
impls of after_krate only use Crate.name, so we can pass just the
name instead.

Right - that explains that it's possible, the krate.module thing explains why it's necessary. :)

Yeah, I should've included more information :)

jyn514 · 2021-03-23T21:49:58Z

@bors r+

#83399 was already approved, seems fine for them both to land at the same time.

bors · 2021-03-23T21:50:57Z

📌 Commit a7f902b has been approved by jyn514

Rollup of 9 pull requests Successful merges: - rust-lang#83051 (Sidebar trait items order) - rust-lang#83313 (Only enable assert_dep_graph when query-dep-graph is enabled.) - rust-lang#83353 (Add internal io::Error::new_const to avoid allocations.) - rust-lang#83391 (Allow not emitting `uwtable` on Android) - rust-lang#83392 (Change `-W help` to display edition level.) - rust-lang#83393 (Codeblock tooltip position) - rust-lang#83399 (rustdoc: Record crate name instead of using `None`) - rust-lang#83405 (Slight visual improvements to warning boxes in the docs) - rust-lang#83415 (Remove unnecessary `Option` wrapping around `Crate.module`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

camelid added 3 commits March 22, 2021 18:18

rustdoc: Record crate name instead of using None

72a180e

Remove unnecessary Option wrapping around Crate.module

68244fc

I'm wondering if it was originally there so that we could `take` the module which enables `after_krate` to take an `&Crate`. However, the two impls of `after_krate` only use `Crate.name`, so we can pass just the name instead.

camelid added C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 23, 2021

rust-highfive assigned GuillaumeGomez Mar 23, 2021

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

camelid commented Mar 23, 2021

View reviewed changes

Remove another unnecessary Option

a7f902b

The previous changes mean that we can now remove this `Option`.

jyn514 approved these changes Mar 23, 2021

View reviewed changes

Dylan-DPC-zz mentioned this pull request Mar 24, 2021

Rollup of 9 pull requests #83432

Merged

bors merged commit 5c0d880 into rust-lang:master Mar 24, 2021

rustbot added this to the 1.53.0 milestone Mar 24, 2021

camelid deleted the remove-crate-module-option branch March 25, 2021 22:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unnecessary `Option` wrapping around `Crate.module` #83415

Remove unnecessary `Option` wrapping around `Crate.module` #83415

camelid commented Mar 23, 2021 •

edited

Loading

rust-highfive commented Mar 23, 2021

camelid Mar 23, 2021

jyn514 Mar 23, 2021

camelid commented Mar 23, 2021

camelid Mar 23, 2021

jyn514 Mar 23, 2021

jyn514 Mar 23, 2021

camelid Mar 23, 2021

jyn514 Mar 24, 2021

camelid Mar 24, 2021

jyn514 commented Mar 23, 2021

bors commented Mar 23, 2021

Remove unnecessary Option wrapping around Crate.module #83415

Remove unnecessary Option wrapping around Crate.module #83415

Conversation

camelid commented Mar 23, 2021 • edited Loading

rust-highfive commented Mar 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelid commented Mar 23, 2021

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 commented Mar 23, 2021

bors commented Mar 23, 2021

Remove unnecessary `Option` wrapping around `Crate.module` #83415

Remove unnecessary `Option` wrapping around `Crate.module` #83415

camelid commented Mar 23, 2021 •

edited

Loading