Skip to content

Show whether a trait is object-safe on its document page #85138

Closed
@EFanZh

Description

@EFanZh
Contributor

So that it is easy for user to recognize object-safe traits.

Activity

added
T-rustdocRelevant to the rustdoc team, which will review and decide on the PR/issue.
E-easyCall for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
E-mentorCall for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
on May 10, 2021
csmoe

csmoe commented on May 10, 2021

@csmoe
Member

pub fn is_object_safe(self, key: DefId) -> bool {

for the rustdoc side, just query the doc context with a given trait_def_id.

vramana

vramana commented on May 10, 2021

@vramana
Contributor

@rustbot claim

vramana

vramana commented on May 10, 2021

@vramana
Contributor

@csmoe Can you tell what are the relevant parts of rustdoc that needs to be changed?

csmoe

csmoe commented on May 12, 2021

@csmoe
Member

@vramana sorry for late rely :)

let is_auto = cx.tcx.trait_is_auto(did);
clean::Trait {
unsafety: cx.tcx.trait_def(did).unsafety,
generics,
items: trait_items,
bounds: supertrait_bounds,
is_auto,
}

Add a is_object_safe field to clean::Trait, its value can be queried with cx.tcx.is_object_safe(did) like is_auto.

Then you can append the object-safety info into the render part here

fn item_trait(w: &mut Buffer, cx: &Context<'_>, it: &clean::Item, t: &clean::Trait) {

I know little about the render code, @jyn514 could you leave some notes about that?

jyn514

jyn514 commented on May 12, 2021

@jyn514
Member

No, please do not add more fields to Trait. Query is_object_safe at the place it is used.

I don't know much about render, @GuillaumeGomez may have suggestions.

GuillaumeGomez

GuillaumeGomez commented on May 12, 2021

@GuillaumeGomez
Member

I'm actually not sure what they want the end result to look like, so I don't know where to tell them to update the code. ;)

tsoutsman

tsoutsman commented on Aug 8, 2021

@tsoutsman
Contributor

I'm actually not sure what they want the end result to look like, so I don't know where to tell them to update the code. ;)

@GuillaumeGomez maybe it could be a cube icon to the right of the new copy item import to clipboard button? On hover, there would be a tooltip saying Item is object-safe or Item is not object-safe. I'm not sure where you get icons for rustdoc, but heroicons have a cube and cube-transparent icon that could be used for object-safe, and object-unsafe traits respectively. The stroke-width could be reduced to fit in with the rest of the icons in rustdoc.

If @vramana doesn't want to work on it I'd be happy to give it a go.

jsha

jsha commented on Nov 18, 2021

@jsha
Contributor

Following up from the PR with some UI discussion.

I'd prefer not to add more icons up at the top of the page; we're already a bit crowded with icons. And there's no icon that has a broadly understood meaning of "object safe".

I like the approach in #89553, to write out "This trait is object-safe." But that PR puts it all the way at the top of the page, which is very high priority - and I would say object safety is more medium priority, so it should be lower on the page. One possibility would be to make an <h2> subheading in the top-doc docblock, but that results in using a lot of space for this. Our h2's are big!

What about this: Under "Implementors", we could have at the very top "This trait is object safe. References to types that implement this trait can be automatically coerced to dyn Foo". My reasoning here is that conceptually, dyn Foo is somewhat like an implementor of the trait.

jyn514

jyn514 commented on Nov 18, 2021

@jyn514
Member

My reasoning here is that conceptually, dyn Foo is somewhat like an implementor of the trait.

I don't think this is necessarily true, you can have a dyn Foo that doesn't implement Foo. But putting the text under "implementors" seems as good a place as any 👍

EFanZh

EFanZh commented on Nov 28, 2021

@EFanZh
ContributorAuthor

@jyn514 Just out of curiosity, how can I have a dyn Foo that doesn’t implement Foo?

jyn514

jyn514 commented on Nov 28, 2021

@jyn514
Member

@EFanZh I don't think that's actually true, I misremembered.

jsha

jsha commented on Dec 19, 2021

@jsha
Contributor

We have one 👍🏻 on:

Under "Implementors", we could have at the very top "This trait is object safe. References to types that implement this trait can be automatically coerced to dyn Foo".

Would anyone else like to weigh in? Good enough to proceed with implementation?

GuillaumeGomez

GuillaumeGomez commented on Feb 12, 2022

@GuillaumeGomez
Member

Any update on this? :)

camelid

camelid commented on Feb 12, 2022

@camelid
Member

I'm not sure if it's already been suggested, but what about only showing a note if the trait is not object-safe?

removed their assignment
on Mar 25, 2022
jsha

jsha commented on May 1, 2022

@jsha
Contributor

I like @camelid's suggestion to only show when a trait is not object safe, since that's the more unusual condition. If we do that, it doesn't fit logically under "Implementors" anymore. If we proceed with only marking non-object-safe traits, here are a couple of ideas:

  • Add a section after the top-doc, but before "Implementors", called "Object Safety", and put the text there.
  • Add information to the documentation of any method that causes the trait to be not object-safe.
GoldsteinE

GoldsteinE commented on Jul 16, 2022

@GoldsteinE
Contributor

I think it’s still fine to mention under “Implementors” like “This trait is not object safe: dyn Trait doesn’t implement Trait”, but I also like the idea of the new section.

I see this issue doesn’t have an assignee currently, can I claim this?

GuillaumeGomez

GuillaumeGomez commented on Jul 16, 2022

@GuillaumeGomez
Member

You can but since we didn't reach a consensus, any PR would hang around until we reach one.

added a commit that references this issue on Oct 31, 2023

Rollup merge of rust-lang#113241 - poliorcetics:85138-doc-object-safe…

51b275b
added a commit that references this issue on Nov 1, 2023
1924f2d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    E-easyCall for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.E-mentorCall for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.T-rustdocRelevant to the rustdoc team, which will review and decide on the PR/issue.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Participants

      @jsha@EFanZh@GuillaumeGomez@vramana@GoldsteinE

      Issue actions

        Show whether a trait is object-safe on its document page · Issue #85138 · rust-lang/rust