Skip to content
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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Mar 28, 2022
Merged

rustdoc: add 🔒 to items with restricted visibility #95024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
43e4bbd
rustdoc: add 🔒 to indicate restricted visibility
koehlma Mar 19, 2022
1fec0fb
apply rustfmt
koehlma Mar 19, 2022
03ddfa3
move the 🔒 after the name
koehlma Mar 21, 2022
4df867c
fix: add a space only in front of 🔒
koehlma Mar 21, 2022
1c523ba
add tests for 🔒
koehlma Mar 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions src/librustdoc/html/render/print_item.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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\">&nbsp;🔒</span> "
Copy link
Contributor

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."

Copy link
Contributor Author

@koehlma koehlma Mar 21, 2022 •
edited
Loading

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. :)

Copy link
Contributor

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?

Copy link
Contributor Author

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.)

Copy link
Member

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?

Copy link
Member

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 🔒.

Copy link
Contributor Author

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.

Copy link
Member

@bjorn3 bjorn3 Mar 24, 2022 •
edited
Loading

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.

Copy link
Member

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.

Copy link
Member

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.

}
_ => "",
};

let doc_value = myitem.doc_value().unwrap_or_default();
w.write_str(ITEM_TABLE_ROW_OPEN);
write!(
w,
"<div class=\"item-left {stab}{add}module-item\">\
<a class=\"{class}\" href=\"{href}\" title=\"{title}\">{name}</a>\
{unsafety_flag}\
{stab_tags}\
<a class=\"{class}\" href=\"{href}\" title=\"{title}\">{name}</a>\
{visibility_emoji}\
{unsafety_flag}\
{stab_tags}\
</div>\
<div class=\"item-right docblock-short\">{docs}</div>",
name = myitem.name.unwrap(),
visibility_emoji = visibility_emoji,
stab_tags = extra_info_tags(myitem, item, cx.tcx()),
docs = MarkdownSummaryLine(&doc_value, &myitem.links(cx)).into_string(),
class = myitem.type_(),
Expand Down
31 changes: 31 additions & 0 deletions src/test/rustdoc/visibility.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,42 +4,68 @@

#![crate_name = "foo"]

// @!has 'foo/index.html' '//a[@href="struct.FooPublic.html"]/..' 'FooPublic 🔒'
// @has 'foo/struct.FooPublic.html' '//pre' 'pub struct FooPublic'
pub struct FooPublic;
// @has 'foo/index.html' '//a[@href="struct.FooJustCrate.html"]/..' 'FooJustCrate 🔒'
// @has 'foo/struct.FooJustCrate.html' '//pre' 'pub(crate) struct FooJustCrate'
crate struct FooJustCrate;
// @has 'foo/index.html' '//a[@href="struct.FooPubCrate.html"]/..' 'FooPubCrate 🔒'
// @has 'foo/struct.FooPubCrate.html' '//pre' 'pub(crate) struct FooPubCrate'
pub(crate) struct FooPubCrate;
// @has 'foo/index.html' '//a[@href="struct.FooSelf.html"]/..' 'FooSelf 🔒'
// @has 'foo/struct.FooSelf.html' '//pre' 'pub(crate) struct FooSelf'
pub(self) struct FooSelf;
// @has 'foo/index.html' '//a[@href="struct.FooInSelf.html"]/..' 'FooInSelf 🔒'
// @has 'foo/struct.FooInSelf.html' '//pre' 'pub(crate) struct FooInSelf'
pub(in self) struct FooInSelf;
// @has 'foo/index.html' '//a[@href="struct.FooPriv.html"]/..' 'FooPriv 🔒'
// @has 'foo/struct.FooPriv.html' '//pre' 'pub(crate) struct FooPriv'
struct FooPriv;

// @!has 'foo/index.html' '//a[@href="pub_mod/index.html"]/..' 'pub_mod 🔒'
pub mod pub_mod {}

// @has 'foo/index.html' '//a[@href="pub_crate_mod/index.html"]/..' 'pub_crate_mod 🔒'
pub(crate) mod pub_crate_mod {}

// @has 'foo/index.html' '//a[@href="a/index.html"]/..' 'a 🔒'
mod a {
// @has 'foo/a/index.html' '//a[@href="struct.FooASuper.html"]/..' 'FooASuper 🔒'
// @has 'foo/a/struct.FooASuper.html' '//pre' 'pub(crate) struct FooASuper'
pub(super) struct FooASuper;
// @has 'foo/a/index.html' '//a[@href="struct.FooAInSuper.html"]/..' 'FooAInSuper 🔒'
// @has 'foo/a/struct.FooAInSuper.html' '//pre' 'pub(crate) struct FooAInSuper'
pub(in super) struct FooAInSuper;
// @has 'foo/a/index.html' '//a[@href="struct.FooAInA.html"]/..' 'FooAInA 🔒'
// @has 'foo/a/struct.FooAInA.html' '//pre' 'struct FooAInA'
// @!has 'foo/a/struct.FooAInA.html' '//pre' 'pub'
pub(in a) struct FooAInA;
// @has 'foo/a/index.html' '//a[@href="struct.FooAPriv.html"]/..' 'FooAPriv 🔒'
// @has 'foo/a/struct.FooAPriv.html' '//pre' 'struct FooAPriv'
// @!has 'foo/a/struct.FooAPriv.html' '//pre' 'pub'
struct FooAPriv;

// @has 'foo/a/index.html' '//a[@href="b/index.html"]/..' 'b 🔒'
mod b {
// @has 'foo/a/b/index.html' '//a[@href="struct.FooBSuper.html"]/..' 'FooBSuper 🔒'
// @has 'foo/a/b/struct.FooBSuper.html' '//pre' 'pub(super) struct FooBSuper'
pub(super) struct FooBSuper;
// @has 'foo/a/b/index.html' '//a[@href="struct.FooBInSuperSuper.html"]/..' 'FooBInSuperSuper 🔒'
// @has 'foo/a/b/struct.FooBInSuperSuper.html' '//pre' 'pub(crate) struct FooBInSuperSuper'
pub(in super::super) struct FooBInSuperSuper;
// @has 'foo/a/b/index.html' '//a[@href="struct.FooBInAB.html"]/..' 'FooBInAB 🔒'
// @has 'foo/a/b/struct.FooBInAB.html' '//pre' 'struct FooBInAB'
// @!has 'foo/a/b/struct.FooBInAB.html' '//pre' 'pub'
pub(in a::b) struct FooBInAB;
// @has 'foo/a/b/index.html' '//a[@href="struct.FooBPriv.html"]/..' 'FooBPriv 🔒'
// @has 'foo/a/b/struct.FooBPriv.html' '//pre' 'struct FooBPriv'
// @!has 'foo/a/b/struct.FooBPriv.html' '//pre' 'pub'
struct FooBPriv;

// @!has 'foo/a/b/index.html' '//a[@href="struct.FooBPub.html"]/..' 'FooBPub 🔒'
// @has 'foo/a/b/struct.FooBPub.html' '//pre' 'pub struct FooBPub'
pub struct FooBPub;
}
}

Expand All @@ -53,13 +79,18 @@ mod a {
//
// @has 'foo/trait.PubTrait.html' '//pre' 'fn function();'
// @!has 'foo/trait.PubTrait.html' '//pre' 'pub fn function();'
//
// @!has 'foo/index.html' '//a[@href="trait.PubTrait.html"]/..' 'PubTrait 🔒'

pub trait PubTrait {
type Type;
const CONST: usize;
fn function();
}

// @has 'foo/index.html' '//a[@href="trait.PrivTrait.html"]/..' 'PrivTrait 🔒'
trait PrivTrait {}

// @has 'foo/struct.FooPublic.html' '//h4[@class="code-header"]' 'type Type'
// @!has 'foo/struct.FooPublic.html' '//h4[@class="code-header"]' 'pub type Type'
//
Expand Down