-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rustdoc: add 🔒 to items with restricted visibility #95024
Conversation
Some changes occurred in cc @camelid Some changes occurred in HTML/CSS/JS. |
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @CraftSpider (or someone else) soon. Please see the contribution instructions for more information. |
This comment has been minimized.
This comment has been minimized.
According to https://color.adobe.com/create/color-contrast-analyzer the dimmed text color has a 2.93:1 contrast ratio to the background. It's supposed to be 4.5:1 for regular-sized text. |
This comment has been minimized.
This comment has been minimized.
@notriddle: Do you have any suggestions how to label/mark private items instead? I also considered using labels (like those for features) for it, but they are very verbose. On the other hand, having a reduced contrast ratio matches my intuition that private items are not really there and it is also less distracting than labels (at least for me). However, I do absolutely agree that this is not the best solution in terms of accessibility. Maybe we should have a high-contrast theme for these purposes? We could also increase the contrast ratio of the normal colors such that the reduced ratio is 4.5:1.
|
I think labels (like those for features, unstable, and deprecated) are the right approach. The problem with using faded text for this, besides accessibility, is that it's not self-explanatory. If someone's looking at a list of items, where some are faded and some are not, it's not clear what fadedness means. They'd have to click through on several faded and unfaded items to notice the pattern. Here are some screenshots of unstable annotations and deprecation annotations in https://doc.rust-lang.org/nightly/std/index.html#modules: I think we should actually get rid of the faded text for unstable, since the |
I think that someone explicitly using My concern is also that labels may become a bit overloaded. I also like the idea of having private items in a different section. How do you feel about that? I also very much like the idea of moving labels to the right. 👍 |
Private items in a different section (on module pages) seems like a good way to solve the problem! |
I would be happy to change this PR in this regard. My suggestion would be to have an additional section for each kind of item after all public items, e.g., “Private Modules” in addition to “Modules”. How do you feel about that? The loop printing the items in |
Maybe add a "Private" annotation like "Experimental"? |
@bjorn3 As said in my comments above, I do not like this idea because it overloads the labels/annotations and, at least for me, is distracting as it clutters the documentation. I think labels should be reserved for additional attributes attached to items, e.g., whether the item is feature-guarded or deprecated, and not for something that is inherently tied to every item by the language itself, like visibility or an item's name. |
I too don't think labels are a good fit here, but I'm also against the idea of having separate sections. When I'm using |
In my opinion it should mean that the item is not declared plain @camelid Do you agree that there should be some visual cue for those items? |
I'm ambivalent on whether it should be marked. I definitely see the argument for marking it, but I'm not sure if the resulting visual complexity is worth it. |
@camelid: I can see your point and agree that we should add as less additional visual complexity as possible, if we decide to mark those items. However, in my opinion, it would be highly valuable and, hence, worth the additional complexity to add such markings (in a visually unintrusive way). Maybe it helps if I explain my motivation and workflow a bit better: When I am working on a crate, a large part of my focus is on its public API because (1) it cannot be changed as easily as the internals and (2) it is the part other developers most likely need to work with and understand. I think, the point of having visibility qualifiers is to be able to hide complexity and implementation details under a nice surface such that consumers of the API do not have to worry about them. Therefore, during development, which is my (and it also seems to be @camelid's) use case for Coming back to @jsha's point that transparency is not self-explanatory. I think that any way of realizing such markings such that they add only little additional visual complexity in terms of binding visual processing capabilities, is bound to be not very self-explanatory. However, I think it is okay in this case to add markings which are not as self-explanatory as a “Restricted” label would be because documenting private items is opt-in. If I explicitly opt-in to have private items documented and am currently working on the code base anyway (are there other use cases for documenting private items?), one would presumably very quickly realize the meaning of a non-self-explanatory marking indicating that an item is private. Assuming that someone is looking at a documentation and is not aware that it has been built with private items, it is also better to have some markings, even if they are not self-explanatory, because otherwise, they are not aware that those items are different and may wonder down the line why they cannot use some of the items appearing in the documentation (happened to me before because items have been feature-guarded without that being evident in the documentation). This is also a further argument for marking private items at least in some way. So, at least for me, this feature seems highly valuable and it would greatly improve my workflow. If we can agree that the markings do not have to be as self-explanatory as an explicit label and should add as less visual complexity as possible, I still think that transparency is a good option. However, I also totally see the accessibility problem raised by @notriddle. Hence, here is yet another proposal for how we could mark such items: Emojis are used in the documentation already for experimental items (🔬) and deprecated items (👎). One way to mark private items would be to put a 🔒 before or after their name. This does not add much visual complexity, is somewhat intuitive, and does not suffer from the accessibility problem. How do you feel about that proposal? |
Having a 🔒 before the name seems like a good idea to me. |
@GuillaumeGomez Should I open a new pull request for this then or is it okay to modify this one? |
In the same PR is fine. Don't forget to update the title. ;) |
4b8c015
to
8d60bf4
Compare
Okay, this did not go quite as planned. I wanted to start over from the current |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good to me, but I'd like positive confirmation from @camelid, who thinks more about the use cases for --document-private-items
.
I had one proposed wording tweak below. Also, I think this would look a little nicer with a flat icon from Font Awesome (like in #91735) instead of an emoji with a color gradient. But I don't think that's a blocking issue; I'd be willing to approve as-is and treat the icon change as a followup.
@@ -376,17 +376,26 @@ fn item_module(w: &mut Buffer, cx: &Context<'_>, item: &clean::Item, items: &[cl | |||
let stab = myitem.stability_class(cx.tcx()); | |||
let add = if stab.is_some() { " " } else { "" }; | |||
|
|||
let visibility_emoji = match myitem.visibility { | |||
clean::Visibility::Restricted(_) => { | |||
"<span title=\"Restricted Visibility\"> 🔒</span> " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Restricted Visibility" seems jargon-y. How about "Private"? https://doc.rust-lang.org/reference/visibility-and-privacy.html says "these two terms are often used interchangeably."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camelid pointed out that “Private” is a bit ambiguous in this case: #95024 (comment)
The icon/emoji also appears for pub(...)
items, hence, I think “Restricted Visibility” is more precise. However, I am also fine with “Private” and happy to modify the PR accordingly. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out.
Also, the definition privacy for these purposes is ambiguous: does it mean private to the module you're looking at or private to the whole crate?
From the perspective of API design, "private" means "private from a view outside the crate." In other words, it includes everything except what's marked with plain pub
; it excludes pub(crate)
, pub(self)
, etc. Since rustdoc is about APIs, I think it makes sense to use that definition of private. @camelid what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An example may help here. Is the struct A
in the following snippet private?
mod(crate) private {
pub struct A; // <-- Is this private?
struct B;
}
It cannot be accessed from “outside the crate” because it is in a private module. However, the struct itself is pub
and will thus not be marked with a 🔒 with the present change. (This also makes sense if we think about designing an API to be used by other modules in the crate which is one of my use cases for this. From the perspective of other modules in the same crate it is indeed public unlike struct B
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the A
struct should be marked as 🔒 since as you said earlier, the 🔒 is about whether it's accessible to code outside the crate. Perhaps that's too technically difficult to implement though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the perspective of API design, "private" means "private from a view outside the crate." In other words, it includes everything except what's marked with plain pub; it excludes pub(crate), pub(self), etc. Since rustdoc is about APIs, I think it makes sense to use that definition of private. @camelid what do you think?
I agree with this definition for 🔒.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this seems difficult to implement, at least for me. I do not know how to extract from the compiler whether A
is somehow publicly accessible or not. Does the compiler have this information at all? I also think it makes sense to think about a pub
item in a private module about being public (after all it is explicitly declared public) but with respect to an internal API available to other modules. It is that internal API that is documented. While I agree that Rustdoc is about APIs, I like to challenge that it is primarily about the APIs of crates. Documenting private items and crate internals can be highly valuable, e.g., for bringing contributors up-to-speed or to document the internal architecture and design decision behind it. This is my/our use case for this change.
Anyway, what does this discussion entail about the “Restricted Visibility” label? Should that be kept as it currently is or should I change it to “Private”? All items marked that will be marked with 🔒 by the current change are private in the sense that they cannot be accessed from outside the crate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::intrinsics::transmute
isn't marked as unstable despite std::intrinsics
being marked as such. It is re-exported as std::mem::transmute
. I think the restricted visibility docs should behave the same way. If you look at the docs you can see that the containing module is private. That the item itself isn't is an indication that it may be public through a re-export. Showing it as restricted may cause people to think it isn't accessible in any way at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I hadn't thought of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a query for this, it's named is_publicly_accessible
or something similar. It's used for unreachable_pub.
@GuillaumeGomez My suggestion was not to document private items and hide them by default. I specifically was suggesting that if the user passed |
I still feel mixed about this change, but I'm ok with landing it, with the possibility of reverting it if it leads to too much visual noise. |
I also would prefer a flat icon (which would further reduce visual noise) but I think it should probably wait until that PR is landed. |
I will happily look into this after #91735 has landed. |
@GuillaumeGomez @notriddle I think this is ready now. |
Not yet. I don't think #91735 will ever get merged (or at least not as is). Isn't it possible to use a font-awesome icon instead for the time being? |
@GuillaumeGomez Sure, I think I can just copy the Font Awesome Lock SVG into |
My bad, I thought we already used it. Then in this case it's fine for me to use the unicode character for the time being. Thanks for working on this! @bors: r=GuillaumeGomez,camelid,jsha |
📌 Commit 1c523ba has been approved by |
☀️ Test successful - checks-actions |
Finished benchmarking commit (2d37f38): comparison url. Summary: This benchmark run did not return any relevant results. 7 results were found to be statistically significant but too small to be relevant. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
I think with this PR being merged, the issue #87785 can be closed. |
Looking at the rustc API docs (doc.rust-lang.org/nightly/nightly-rustc), the result of this PR still feels really noisy to me. I have two ideas for ways to improve it:
I think I'd prefer option 2. What do you think about it? |
/* Edit: I just noticed that the emoji I put there is indeed an open lock. There is 🔒 and 🔓. However, the open lock is barely recognizable as being open – at least on my machine – so, I am not in favor of using it. */ Can we somehow use the Font Awesome icons without using a special font? I share the concerns raised regarding #91735 and the reliance on some web service or other external tool. Is there any process of how we could use the Font Awesome SVG icons by copying them into this repo? Regarding the licenses, this should be possible. /* Edit: Just noticed the discussion regarding licenses on Zulip. Is there any definite answer whether it is possible to includes those icons as SVG yet? As far as I understand the Also, I think we could do both, i.e., mark public items instead of private ones and putting the icon in the gutter. I can take care of seeing this through but I guess some discussing regarding the usage of Font Awesome icons in general (without using a custom font which relies on some external tool) would be a good idea. |
I agree about the open lock emoji; it's very confusing. In general, I think the team does want to switch to Font Awesome (or similar) but we're not sure how to deal with licensing/external tools/etc. and it's kind of on hold. I think @GuillaumeGomez may know more about the status. |
I think the question how to deal with licensing should be discussed separately from the question whether reliance on an external tool is a good idea. Maybe, we could consider using Heroicons (https://heroicons.com) instead of Font Awesome? They are MIT licensed (so there should not be any licensing issues) and we can simply put the SVGs into the What do you think? |
The problem wasn't the license but the tool used to generate the fonts/SVG. So the current situation is: whether having a font (with only the elements we want) or SVGs, we need a simple way to regenerate them if needed (or add new ones). That's where our problem was, not the license. As for heroicons, why not. It's the same issue as I just listed but otherwise seems fine to me as well. The only thing to keep in mind here is that we try to keep our icons kinda coherent with mdbook and crates.io. ;) |
@GuillaumeGomez I see, so in principle, I can just copy a Heroicon SVG for the open lock into If so, I can just create another PR applying the changes as suggested above (mark public instead of private items and put the icon into the gutter instead of after the name). Should I do that? We can then have yet another PR to replace all the icons with the Heroicon SVGs for consistency. I guess, the font vs. SVG issue should be treated separately (I lack the relevant expertise to have an opinion about that). |
You can create a PR which switches the lock into a SVG file and provide the license(s) file(s) alongside it if it comes from a project we don't use. Marking public items would be weird because they are present by default, so they're no reason to suddenly add a new icon alongside them. But in any case, that change would require to debate about it before making it. As for the position of the icon, we placed it after the name because it would change the alignment, but I think another solution was suggested to prevent this so why not. So to make things simpler: make a first PR which switches the lock to SVG. Then you can make a second PR for the position of the lock (there may have debates, we'll see). As for adding an unlocked lock for public items, please open an issue. |
@GuillaumeGomez Opened #95657. |
This change marks items with restricted visibility with 🔒 when building with
--document-private-items
:There also appears a “Restricted Visibility” tooltip when hovering over the emoji.
The original PR for reference:
This change makes private items slightly transparent (similar to
unstable
items in rustc):I found myself using
--document-private-items
a lot recently because I find the documentation of private internals quite helpful when working on a larger project. However, not being able to distinguish private from public items (see #87785) when looking at the documentation makes this somewhat cumbersome.This PR addresses the third suggestion of issue #87785 by marking private items typographically. It seems to me that the other suggestions are more involved but this is at least a first step.
A private item is also made slightly transparent in the path displayed in the header of a page:
I am looking forward to feedback and suggestions.