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: hide #[repr] if it isn't part of the public ABI #116882

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+262 −75
Open

rustdoc: hide #[repr] if it isn't part of the public ABI #116882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 8 additions & 2 deletions src/doc/rustdoc/src/advanced-features.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,13 @@ https://doc.rust-lang.org/stable/std/?search=%s&go_to_first=true
This URL adds the `go_to_first=true` query parameter which can be appended to any `rustdoc` search URL
to automatically go to the first result.

## `#[repr(transparent)]`: Documenting the transparent representation
## `#[repr(...)]`: Documenting the representation of a type

<!-- FIXME(fmease): Fill in this section once the semantics have been decided upon. -->

### `#[repr(transparent)]`

<!-- FIXME(fmease): Extract most parts into the section above. -->

You can read more about `#[repr(transparent)]` itself in the [Rust Reference][repr-trans-ref] and
in the [Rustonomicon][repr-trans-nomicon].
Expand All @@ -102,7 +108,7 @@ fields are 1-ZST fields. The term *1-ZST* refers to types that are one-aligned a
It would seem that one can manually hide the attribute with `#[cfg_attr(not(doc), repr(transparent))]`
if one wishes to declare the representation as private even if the non-1-ZST field is public.
However, due to [current limitations][cross-crate-cfg-doc], this method is not always guaranteed to work.
Therefore, if you would like to do so, you should always write it down in prose independently of whether
Therefore, if you would like to do so, you should always write that down in prose independently of whether
you use `cfg_attr` or not.

[repr-trans-ref]: https://doc.rust-lang.org/reference/type-layout.html#the-transparent-representation
Expand Down
175 changes: 114 additions & 61 deletions src/librustdoc/clean/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,17 +709,15 @@ impl Item {
Some(tcx.visibility(def_id))
}

pub(crate) fn attributes(
pub(crate) fn attributes<'tcx>(
&self,
tcx: TyCtxt<'_>,
tcx: TyCtxt<'tcx>,
cache: &Cache,
keep_as_is: bool,
) -> Vec<String> {
const ALLOWED_ATTRIBUTES: &[Symbol] =
&[sym::export_name, sym::link_section, sym::no_mangle, sym::non_exhaustive];

use rustc_abi::IntegerType;

let mut attrs: Vec<String> = self
.attrs
.other_attrs
Expand All @@ -739,67 +737,122 @@ impl Item {
}
})
.collect();
if !keep_as_is
&& let Some(def_id) = self.def_id()
&& let ItemType::Struct | ItemType::Enum | ItemType::Union = self.type_()
{
let adt = tcx.adt_def(def_id);
let repr = adt.repr();
let mut out = Vec::new();
if repr.c() {
out.push("C");
}
if repr.transparent() {
// Render `repr(transparent)` iff the non-1-ZST field is public or at least one
// field is public in case all fields are 1-ZST fields.
let render_transparent = cache.document_private
|| adt
.all_fields()
.find(|field| {
let ty =
field.ty(tcx, ty::GenericArgs::identity_for_item(tcx, field.did));
tcx.layout_of(tcx.param_env(field.did).and(ty))
.is_ok_and(|layout| !layout.is_1zst())
})
.map_or_else(
|| adt.all_fields().any(|field| field.vis.is_public()),
|field| field.vis.is_public(),
);

if render_transparent {
out.push("transparent");
}
}
if repr.simd() {
out.push("simd");
}
let pack_s;
if let Some(pack) = repr.pack {
pack_s = format!("packed({})", pack.bytes());
out.push(&pack_s);
}
let align_s;
if let Some(align) = repr.align {
align_s = format!("align({})", align.bytes());
out.push(&align_s);
}
let int_s;
if let Some(int) = repr.int {
int_s = match int {
IntegerType::Pointer(is_signed) => {
format!("{}size", if is_signed { 'i' } else { 'u' })
if !keep_as_is && let Some(repr) = self.repr(tcx, cache) {
attrs.push(repr);
}

attrs
}

/// Compute the *public* `#[repr]` of this item.
///
/// Read more about it here:
/// https://doc.rust-lang.org/nightly/rustdoc/advanced-features.html#repr-documenting-the-representation-of-a-type
fn repr<'tcx>(&self, tcx: TyCtxt<'tcx>, cache: &Cache) -> Option<String> {
let def_id = self.def_id()?;
let (ItemType::Struct | ItemType::Enum | ItemType::Union) = self.type_() else {
return None;
};

let adt = tcx.adt_def(def_id);
let repr = adt.repr();

let is_visible = |def_id| cache.document_hidden || !tcx.is_doc_hidden(def_id);
let is_field_public =
|field: &'tcx ty::FieldDef| field.vis.is_public() && is_visible(field.did);

if repr.transparent() {
// `repr(transparent)` is public iff the non-1-ZST field is public or
// at least one field is public in case all fields are 1-ZST fields.
let is_public = cache.document_private
|| adt.variants().iter().all(|variant| {
if !is_visible(variant.def_id) {
return false;
}
IntegerType::Fixed(size, is_signed) => {
format!("{}{}", if is_signed { 'i' } else { 'u' }, size.size().bytes() * 8)

let field = variant.fields.iter().find(|field| {
let args = ty::GenericArgs::identity_for_item(tcx, field.did);
let ty = field.ty(tcx, args);
tcx.layout_of(tcx.param_env(field.did).and(ty))
.is_ok_and(|layout| !layout.is_1zst())
});

if let Some(field) = field {
return is_field_public(field);
}
};
out.push(&int_s);
}
if !out.is_empty() {
attrs.push(format!("#[repr({})]", out.join(", ")));
}

adt.variants().iter().all(|variant| {
variant.fields.is_empty() || variant.fields.iter().any(is_field_public)
})
});

// Since `repr(transparent)` can't have any other reprs or
// repr modifiers beside it, we can safely return early here.
return is_public.then(|| "#[repr(transparent)]".into());
}
attrs

// Fast path which avoids looking through the variants and fields in
// the common case of no `#[repr]` or in the case of `#[repr(Rust)]`.
if !repr.c()
&& !repr.simd()
&& repr.int.is_none()
&& repr.pack.is_none()
&& repr.align.is_none()
{
return None;
}

let is_public = cache.document_private
fmease marked this conversation as resolved.
Show resolved Hide resolved
|| if adt.is_enum() {
// FIXME(fmease): Should we take the visibility of fields of variants into account?
// FIXME(fmease): `any` or `all`?
adt.variants().is_empty()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd go for any in both cases.

Copy link
Member Author

@fmease fmease Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RalfJung, does it sound good to you as well to consider the repr public if there exists at least one struct field that is public (there might be private and hidden ones) (if we have a struct) or if there exists at least one non-hidden enum variant (if we have an enum)? (With the extra rule that empty structs and enums also render the repr public).

Or should all fields (current version of this PR) and enum variants be public for the repr to be public?

Copy link
Member

@RalfJung RalfJung Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually for structs, if there is at least one private field then we say you can't rely on the struct staying how it is. For instance if your repr(transparent) relies on another type being a ZST and that type has at least one private field, we warn about that (and we eventually want to make that an error).

So I'd say the same should go for the repr. If any field is private, then the repr is (by default) private.

Copy link
Member Author

@fmease fmease Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that makes sense! What about enum variants? Can users still make certain assumptions about the repr of an enum if some but not all of its variants are private or hidden?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no such things as private enum variants (unfortunately).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on a struct with a doc(hidden) field, should the repr be shown?

Here is an example of what goes wrong if you show repr on a struct with doc(hidden) field.

#[repr(C)]
pub struct Struct {
    pub first: u8,
    #[doc(hidden)]
    pub second: u16,
    pub third: u8,
}
Screenshot 2023-10-18 at 3 41 44 PM

Rustdoc purports a repr(C) struct in which the first byte is first, the second byte is third, and some other fields follow. Given the Rust-like syntax in which rustdoc shows #[repr(C)], this feels misleading. For a struct that is actually this:

#[repr(C)]
pub struct Struct {
    pub first: u8,
    pub third: u8,
    // ... other fields ...
}

one would expect they can cast &Struct to &[u8; 2] and read first and third from it. If they do that in this case though, they get UB from looking at a padding byte.

I think this would be a useful bar to keep in mind as a minimum desirable property; rustdoc should not show a repr in such a way that misleads reader about reality. That does not necessarily need to mean hiding such reprs, though that might be the most expedient path forward. Alternatively rustdoc could be more discerning about placing the /*private field*/ comment in between the correct fields when there is a repr.

Copy link
Member Author

@fmease fmease Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, do we already track this in a GitHub issue? With the introduction of core::mem::offset_of, it feels like we should up the priority of this issue. If I remember correctly, it'd need quite a bit of rewiring inside rustdoc to render /* private field */ in the correct order for repr(C) structs since those fields are stripped early at the moment.

Copy link
Member Author

@fmease fmease Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RalfJung, re taking doc(hidden) on variants into account when computing the visibility of a repr, I've included that in the heuristic to hide the repr(u8) on core::ffi::c_void which has consists of two doc(hidden) enum variants.

Copy link
Member

@RalfJung RalfJung Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds to me then like we'd want to hide the repr as soon as there is any hidden field (just like we hide it as soon as there is any private field) -- both for struct and enum (and union).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like there is more to discuss. I'll add it to the next rustdoc team meeting agenda.

|| adt.variants().iter().any(|variant| is_visible(variant.def_id))
} else {
// FIXME(fmease): `all` or `any`?
adt.all_fields().all(is_field_public)
};
if !is_public {
return None;
}

let mut result = Vec::new();

if repr.c() {
result.push("C");
}
if repr.simd() {
result.push("simd");
}
let int_s;
if let Some(int) = repr.int {
int_s = match int {
rustc_abi::IntegerType::Pointer(is_signed) => {
format!("{}size", if is_signed { 'i' } else { 'u' })
}
rustc_abi::IntegerType::Fixed(size, is_signed) => {
format!("{}{}", if is_signed { 'i' } else { 'u' }, size.size().bytes() * 8)
}
};
result.push(&int_s);
}
let pack_s;
if let Some(pack) = repr.pack {
pack_s = format!("packed({})", pack.bytes());
result.push(&pack_s);
}
let align_s;
if let Some(align) = repr.align {
align_s = format!("align({})", align.bytes());
result.push(&align_s);
}

if result.is_empty() {
return None;
}

Some(format!("#[repr({})]", result.join(", ")))
}

pub fn is_doc_hidden(&self) -> bool {
Expand Down
8 changes: 4 additions & 4 deletions tests/rustdoc-gui/src/test_docs/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,10 +454,10 @@ pub fn safe_fn() {}

#[repr(C)]
pub struct WithGenerics<T: TraitWithNoDocblocks, S = String, E = WhoLetTheDogOut, P = i8> {
s: S,
t: T,
e: E,
p: P,
pub s: S,
pub t: T,
pub e: E,
pub p: P,
}

pub struct StructWithPublicUndocumentedFields {
Expand Down
50 changes: 42 additions & 8 deletions tests/rustdoc/inline_cross/auxiliary/repr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,60 @@
#![feature(repr_simd)]

#[repr(C, align(8))]
#[repr(Rust)]
pub struct ReprRust;

#[repr(C, align(8))] // public
pub struct ReprC {
field: u8,
pub field: u8,
}
#[repr(simd, packed(2))]

#[repr(C)] // private
pub struct ReprCPrivField {
private: u8,
pub public: i8,
}

#[repr(align(4))] // private
pub struct ReprAlignHiddenField {
#[doc(hidden)]
pub hidden: i16,
}

#[repr(simd, packed(2))] // public
pub struct ReprSimd {
field: u8,
pub field: u8,
}
#[repr(transparent)]

#[repr(transparent)] // public
pub struct ReprTransparent {
pub field: u8,
pub field: u8, // non-1-ZST field
}
#[repr(isize)]

#[repr(isize)] // public
pub enum ReprIsize {
Bla,
}
#[repr(u8)]

#[repr(u8)] // public
pub enum ReprU8 {
Bla,
}

#[repr(u32)] // public
pub enum ReprU32 {
#[doc(hidden)]
Hidden,
Public,
}

#[repr(u64)] // private
pub enum ReprU64HiddenVariants {
#[doc(hidden)]
A,
#[doc(hidden)]
B,
}

#[repr(transparent)] // private
pub struct ReprTransparentPrivField {
field: u32, // non-1-ZST field
Expand Down
25 changes: 25 additions & 0 deletions tests/rustdoc/inline_cross/repr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,47 @@

extern crate repr;

// Never display `repr(Rust)` since it's the default anyway.
//@ has 'foo/struct.ReprRust.html'
//@ !has - '//*[@class="rust item-decl"]//*[@class="code-attribute"]' '#[repr(Rust)]'
pub use repr::ReprRust;

//@ has 'foo/struct.ReprC.html'
//@ has - '//*[@class="rust item-decl"]//*[@class="code-attribute"]' '#[repr(C, align(8))]'
pub use repr::ReprC;

//@ has 'foo/struct.ReprCPrivField.html'
//@ !has - '//*[@class="rust item-decl"]//*[@class="code-attribute"]' '#[repr(C)]'
pub use repr::ReprCPrivField;

//@ has 'foo/struct.ReprAlignHiddenField.html'
//@ !has - '//*[@class="rust item-decl"]//*[@class="code-attribute"]' '#[repr(align(4))]'
pub use repr::ReprAlignHiddenField;

//@ has 'foo/struct.ReprSimd.html'
//@ has - '//*[@class="rust item-decl"]//*[@class="code-attribute"]' '#[repr(simd, packed(2))]'
pub use repr::ReprSimd;

//@ has 'foo/struct.ReprTransparent.html'
//@ has - '//*[@class="rust item-decl"]//*[@class="code-attribute"]' '#[repr(transparent)]'
pub use repr::ReprTransparent;

//@ has 'foo/enum.ReprIsize.html'
//@ has - '//*[@class="rust item-decl"]//*[@class="code-attribute"]' '#[repr(isize)]'
pub use repr::ReprIsize;

//@ has 'foo/enum.ReprU8.html'
//@ has - '//*[@class="rust item-decl"]//*[@class="code-attribute"]' '#[repr(u8)]'
pub use repr::ReprU8;

//@ has 'foo/enum.ReprU32.html'
//@ has - '//*[@class="rust item-decl"]//*[@class="code-attribute"]' '#[repr(u32)]'
pub use repr::ReprU32;

//@ has 'foo/enum.ReprU64HiddenVariants.html'
//@ !has - '//*[@class="rust item-decl"]//*[@class="code-attribute"]' '#[repr(u64)]'
pub use repr::ReprU64HiddenVariants;

// Regression test for <https://github.com/rust-lang/rust/issues/90435>.
// Check that we show `#[repr(transparent)]` iff the non-1-ZST field is public or at least one
// field is public in case all fields are 1-ZST fields.
Expand Down
Loading
Loading