rustdoc: Cleanup `clean` part 2 #88895

camelid · 2021-09-12T20:36:08Z

Split out from #88379. This contains the following commits from that PR:

Remove Type::ResolvedPath.is_generic
Rename is_generic() to is_assoc_ty()

r? @jyn514

It can be computed on-demand.

The new name is more accurate than the previous one.

camelid

I left explanatory comments on each nontrivial part of the diff to hopefully make this change easier to review :)

camelid · 2021-09-22T17:29:47Z

src/librustdoc/clean/utils.rs

+    match path.res {
+        Res::PrimTy(p) => Primitive(PrimitiveType::from(p)),
+        Res::SelfTy(..) if path.segments.len() == 1 => Generic(kw::SelfUpper),
+        Res::Def(DefKind::TyParam, _) if path.segments.len() == 1 => Generic(path.segments[0].name),
+        _ => {
+            let did = register_res(cx, path.res);
+            ResolvedPath { path, did }
        }
-        Res::SelfTy(..) | Res::Def(DefKind::TyParam | DefKind::AssocTy, _) => true,
-        _ => false,
-    };
-    let did = register_res(cx, path.res);
-    ResolvedPath { path, did, is_generic }
+    }
 }


The part of this code that computed is_generic is now covered by Path::is_assoc_ty(), so I removed it.

camelid · 2021-09-22T17:30:13Z

src/librustdoc/clean/types.rs

+    crate fn is_assoc_ty(&self) -> bool {
+        match self.res {
+            Res::SelfTy(..) if self.segments.len() != 1 => true,
+            Res::Def(DefKind::TyParam, _) if self.segments.len() != 1 => true,
+            Res::Def(DefKind::AssocTy, _) => true,
+            _ => false,
+        }
+    }


This code is adapted directly from resolve_type().

camelid · 2021-09-22T17:32:26Z

src/librustdoc/clean/types.rs

-            PolyTrait {
-                trait_: ResolvedPath { path, did, is_generic: false },
-                generic_params: Vec::new(),
-            },


This path belongs to the Sized trait, so it's not an associated type, and Path::is_assoc_ty should preserve that because the Res should be "correct by construction".

camelid · 2021-09-22T17:33:32Z

src/librustdoc/clean/mod.rs

-                    PolyTrait {
-                        trait_: ResolvedPath { path, did, is_generic: false },
-                        generic_params: Vec::new(),
-                    },


This path is for the first trait bound in a dyn Trait1 + Trait2 + ... object, so its Res should be correct and preserve the is_generic: false behavior.

camelid · 2021-09-22T17:35:00Z

src/librustdoc/clean/mod.rs

@@ -1473,7 +1473,7 @@ impl<'tcx> Clean<Type> for Ty<'tcx> {
                    let path = external_path(cx, did, false, vec![], empty);
                    inline::record_extern_fqn(cx, did, ItemType::Trait);
                    let bound = PolyTrait {
-                        trait_: ResolvedPath { path, did, is_generic: false },


This path is for one of the trait bounds after the first (i.e., one of the auto trait bounds) in a dyn Trait1 + Trait2 + ... object, so its Res should be correct and preserve the is_generic: false behavior.

camelid · 2021-09-22T17:36:11Z

src/librustdoc/clean/mod.rs

@@ -1442,12 +1442,12 @@ impl<'tcx> Clean<Type> for Ty<'tcx> {
                };
                inline::record_extern_fqn(cx, did, kind);
                let path = external_path(cx, did, false, vec![], substs);
-                ResolvedPath { path, did, is_generic: false }


This path is for a struct/enum/union declaration, so its Res should be correct and preserve the is_generic: false behavior.

camelid · 2021-09-22T17:37:39Z

src/librustdoc/clean/mod.rs

@@ -168,7 +168,7 @@ impl Clean<Type> for (ty::TraitRef<'_>, &[TypeBinding]) {

        debug!("ty::TraitRef\n  subst: {:?}\n", trait_ref.substs);

-        ResolvedPath { path, did: trait_ref.def_id, is_generic: false }


This path is for a trait in a ty::TraitRef (e.g., a where clause trait bound), so its Res should be correct and preserve the is_generic: false behavior.

camelid · 2021-09-22T17:38:43Z

src/librustdoc/html/render/print_item.rs

+        clean::ResolvedPath { ref path, .. }
+        | clean::BorrowedRef { type_: box clean::ResolvedPath { ref path, .. }, .. }
+            if !path.is_assoc_ty() =>
+        {
+            implementor_dups[&path.last()].1
+        }


is_assoc_ty() is the replacement for is_generic == true; this is the negation of that.

camelid · 2021-09-22T17:39:07Z

src/librustdoc/html/render/print_item.rs

+                clean::ResolvedPath { ref path, did, .. }
                | clean::BorrowedRef {
-                    type_: box clean::ResolvedPath { ref path, did, is_generic: false, .. },
-                    ..
-                } => {
+                    type_: box clean::ResolvedPath { ref path, did, .. }, ..
+                } if !path.is_assoc_ty() => {


Same as the above comment.

camelid · 2021-09-22T17:44:01Z

src/librustdoc/html/format.rs

+                primitive_link(f, clean::PrimitiveType::RawPointer, &text, cx)
+            } else {
+                primitive_link(f, clean::PrimitiveType::RawPointer, &format!("*{} ", m), cx)?;
+                fmt::Display::fmt(&t.print(cx), f)
            }


I made a couple of cleanup changes here to reduce rightward drift and make the code easier to understand:

replaced match with if matches!

extracted a text local variable

The only other change is replacing the pattern clean::Generic(_) | clean::ResolvedPath { is_generic: true, .. } with the conditional if matches!(**t, clean::Generic(_)) || t.is_assoc_ty()), which is necessary now that the is_generic field has been removed in favor of using an is_assoc_ty() function.

jyn514

This is awesome ❤️ I just have a couple nits.

src/librustdoc/clean/types.rs

src/librustdoc/html/format.rs

It's adapted from the old documentation for the `is_generic` field.

camelid · 2021-09-25T16:53:15Z

I re-added the documentation, but I did not remove the ** since it seems like it might require larger changes.

jyn514 · 2021-09-25T17:00:42Z

@bors r+

Thanks!

bors · 2021-09-25T17:00:43Z

📌 Commit dace2ee has been approved by jyn514

bors · 2021-09-26T01:59:19Z

⌛ Testing commit dace2ee with merge b05ffbcff9af79f70d6f5889c7ec7a784ee934d0...

jyn514 · 2021-09-26T02:31:06Z

@bors retry yield to #89262

…arth Rollup of 7 pull requests Successful merges: - rust-lang#88895 (rustdoc: Cleanup `clean` part 2) - rust-lang#88973 (Expose the std_detect env_override feature) - rust-lang#89010 (Add some intra doc links) - rust-lang#89198 (rustdoc: Don't show hidden trait methods) - rust-lang#89216 (Consistent big O notation) - rust-lang#89224 (Change the order of imports suggestions) - rust-lang#89256 (Fix typo in release notes) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-log-analyzer · 2021-09-26T02:34:37Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

camelid added 2 commits September 12, 2021 13:27

Remove Type::ResolvedPath.is_generic

07d6454

It can be computed on-demand.

Rename is_generic() to is_assoc_ty()

1085dc2

The new name is more accurate than the previous one.

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 Sep 12, 2021

rust-highfive assigned jyn514 Sep 12, 2021

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

camelid commented Sep 22, 2021

View reviewed changes

jyn514 approved these changes Sep 25, 2021

View reviewed changes

src/librustdoc/clean/types.rs Show resolved Hide resolved

src/librustdoc/html/format.rs Outdated Show resolved Hide resolved

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 Sep 25, 2021

rustdoc: Document is_assoc_ty()

dace2ee

It's adapted from the old documentation for the `is_generic` field.

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 Sep 25, 2021

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 25, 2021

Manishearth mentioned this pull request Sep 26, 2021

Rollup of 7 pull requests #89262

Merged

bors merged commit 7777f95 into rust-lang:master Sep 26, 2021

rustbot added this to the 1.57.0 milestone Sep 26, 2021

camelid deleted the cleanup-pt2 branch September 26, 2021 19:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: Cleanup `clean` part 2 #88895

rustdoc: Cleanup `clean` part 2 #88895

camelid commented Sep 12, 2021

camelid left a comment

camelid Sep 22, 2021

camelid Sep 22, 2021

camelid Sep 22, 2021

camelid Sep 22, 2021

camelid Sep 22, 2021

camelid Sep 22, 2021

camelid Sep 22, 2021

camelid Sep 22, 2021

camelid Sep 22, 2021

camelid Sep 22, 2021

jyn514 left a comment

camelid commented Sep 25, 2021

jyn514 commented Sep 25, 2021

bors commented Sep 25, 2021

bors commented Sep 26, 2021

jyn514 commented Sep 26, 2021

rust-log-analyzer commented Sep 26, 2021

		@@ -168,7 +168,7 @@ impl Clean<Type> for (ty::TraitRef<'_>, &[TypeBinding]) {

		debug!("ty::TraitRef\n subst: {:?}\n", trait_ref.substs);

		ResolvedPath { path, did: trait_ref.def_id, is_generic: false }

rustdoc: Cleanup clean part 2 #88895

rustdoc: Cleanup clean part 2 #88895

Conversation

camelid commented Sep 12, 2021

camelid left a 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

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 left a comment

Choose a reason for hiding this comment

camelid commented Sep 25, 2021

jyn514 commented Sep 25, 2021

bors commented Sep 25, 2021

bors commented Sep 26, 2021

jyn514 commented Sep 26, 2021

rust-log-analyzer commented Sep 26, 2021

rustdoc: Cleanup `clean` part 2 #88895

rustdoc: Cleanup `clean` part 2 #88895