Make `Res::SelfTy` a struct variant and update docs by BoxyUwU · Pull Request #93938 · rust-lang/rust

BoxyUwU · 2022-02-12T11:37:27Z

I found pattern matching on a (Option<DefId>, Option<(DefId, bool)>) to not be super readable, additionally the doc comments on the types in a tuple variant aren't visible anywhere at use sites as far as I can tell (using rust analyzer + vscode)

The docs incorrectly assumed that the DefId in Option<(DefId, bool)> would only ever be for an impl item and I also found the code examples to be somewhat unclear about which DefId was being talked about.

r? @lcnr since you reviewed the last PR changing these docs

rust-highfive · 2022-02-12T11:37:29Z

Some changes occurred in clean/types.rs.

cc @camelid

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

BoxyUwU · 2022-02-12T11:41:09Z

src/librustdoc/clean/utils.rs

+        Res::SelfTy { trait_: Some(trait_def_id), alias_to: _ } => (trait_def_id, ItemType::Trait),
        // This is an inherent impl; it doesn't have its own page.
-        Res::SelfTy(None, Some((impl_def_id, _))) => return impl_def_id,
-        Res::SelfTy(None, None)
+        Res::SelfTy { trait_: None, alias_to: Some((impl_def_id, _)) } => return impl_def_id,
+        Res::SelfTy { trait_: None, alias_to: None }


I am not familiar with rustdoc so am not sure if this is actually a bug but these comments seem wrong 😅
the trait_: Some(trait) case is not just for trait definitions and is also there in impl Trait for Foo blocks
the trait_: None, alias_to: Some(..) is not just for inherent impls and is also there in struct defs (struct Foo(Box<Self>))

If you can, please go ahead and update these comments so they're correct :)

i think it makes sense to add a fixme to potentially change this to

Res::SelfTy { trait_: Some(trait_def_id), alias_to: None } => (trait_def_id, ItemType::Trait),

i think the mention of impl Send for [u8] might be caused by this https://doc.rust-lang.org/nightly/std/marker/trait.Send.html#impl-Send-80

but i don't want to add functionality changes to rustdoc in this PR

camelid · 2022-02-12T23:07:25Z

compiler/rustc_hir/src/def.rs

+        /// The trait this `Self` is a generic arg for.
+        trait_: Option<DefId>,
+        /// The item introducing the `Self` type alias. Can be used in the `type_of` query
+        /// to get the underlying type. Additionally whether the `Self` type is disallowed
+        /// from mentioning generics (i.e. when used in an anonymous constant).
+        alias_to: Option<(DefId, bool)>,


Thanks for naming these fields and improving the docs! I had a lot of trouble understanding this when I wrote the current version of the docs.

Why is alias_to an Option, though? When would it be None?

inside of a trait def alias_to is None

trait Foo { fn foo() -> Self; // `Res::SelfTy { trait_: Some(Foo), alias_to: None }` }

Ah, makes sense, thanks :)

Could you add a comment about that?

Oops, just realized this was already merged 😅

There's a comment about this if you look at the examples above the SelfTy variant

Thanks! That covers it.

camelid · 2022-02-12T23:08:05Z

src/librustdoc/clean/utils.rs

+        Res::SelfTy { trait_: Some(trait_def_id), alias_to: _ } => (trait_def_id, ItemType::Trait),
        // This is an inherent impl; it doesn't have its own page.
-        Res::SelfTy(None, Some((impl_def_id, _))) => return impl_def_id,
-        Res::SelfTy(None, None)
+        Res::SelfTy { trait_: None, alias_to: Some((impl_def_id, _)) } => return impl_def_id,
+        Res::SelfTy { trait_: None, alias_to: None }


If you can, please go ahead and update these comments so they're correct :)

lcnr · 2022-02-14T07:03:57Z

after adding a fixme to rustdoc, r=me

BoxyUwU · 2022-02-14T12:05:26Z

@bors r=lcnr

bors · 2022-02-14T12:05:27Z

📌 Commit 48a79bc has been approved by lcnr

bors · 2022-02-14T12:26:46Z

⌛ Testing commit 48a79bc with merge b321742...

bors · 2022-02-14T14:47:09Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing b321742 to master...

rust-timer · 2022-02-14T16:19:18Z

Finished benchmarking commit (b321742): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Make `Res::SelfTy` a struct variant and update docs I found pattern matching on a `(Option<DefId>, Option<(DefId, bool)>)` to not be super readable, additionally the doc comments on the types in a tuple variant aren't visible anywhere at use sites as far as I can tell (using rust analyzer + vscode) The docs incorrectly assumed that the `DefId` in `Option<(DefId, bool)>` would only ever be for an impl item and I also found the code examples to be somewhat unclear about which `DefId` was being talked about. r? `@lcnr` since you reviewed the last PR changing these docs

BoxyUwU added 2 commits February 12, 2022 11:23

change to a struct variant

e81e09a

change docs on Res::SelfTy

9130af2

rust-highfive assigned lcnr Feb 12, 2022

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Feb 12, 2022

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

BoxyUwU commented Feb 12, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

trailing whitespace

5e0e7ff

camelid reviewed Feb 12, 2022

View reviewed changes

update two rustdoc comments

48a79bc

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 Feb 14, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 14, 2022

bors merged commit b321742 into rust-lang:master Feb 14, 2022

rustbot added this to the 1.60.0 milestone Feb 14, 2022

This was referenced Feb 14, 2022

solve ICE of issue #85350 by avoiding the case with ty::Error #85477

Closed

Perform lifetime resolution on the AST for lowering #91557

Merged

better errors when resolving bad Self in impl block #93971

Closed

dario23 added a commit to dario23/rust-semverver that referenced this pull request May 4, 2022

Rustup to rust-lang/rust#93938

52314c8

dario23 added a commit to dario23/rust-semverver that referenced this pull request May 6, 2022

Rustup to rust-lang/rust#93938

73a6075

Uh oh!

Conversation

BoxyUwU commented Feb 12, 2022

Uh oh!

rust-highfive commented Feb 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camelid Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lcnr commented Feb 14, 2022

Uh oh!

BoxyUwU commented Feb 14, 2022

Uh oh!

bors commented Feb 14, 2022

Uh oh!

bors commented Feb 14, 2022

Uh oh!

bors commented Feb 14, 2022

Uh oh!

rust-timer commented Feb 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

camelid Feb 17, 2022 •

edited

Loading