rustdoc: hide `#[repr(transparent)]` if it isn't part of the public ABI by fmeas...
source link: https://github.com/rust-lang/rust/pull/115439
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Conversation
Collaborator
|
r? @oli-obk (rustbot has picked a reviewer for you, use r? to override) |
added A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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
Collaborator
|
Some changes occurred in src/librustdoc/clean/types.rs cc @camelid |
Member
Author
|
r? rustdoc |
| @@ -110,3 +110,23 @@ 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 |
Member
Author
I wasn't sure where to place this section, open to suggestions
This is probably the best place to put it right now.
| @@ -770,10 +791,9 @@ impl Item { | ||
}; |
||
out.push(&int_s); |
||
} |
||
if out.is_empty() { |
||
return Vec::new(); |
Member
Author
The early return wasn't correct. It resulted in us not rendering some attributes like #[non_exhaustive] in cross-crate re-export scenarios. I've added a regression test for this: tests/rustdoc/inline_cross/attributes.rs.
Comment on lines
116 to 128
You can read more about `#[repr(transparent)]` itself in the [Rust Reference][repr-trans-ref] and |
||
in the [Rustonomicon][repr-trans-nomicon]. |
||
Since this representation is only considered part of the public ABI if the single field with non-trivial |
||
size or alignment is public and if the documentation does not say otherwise, Rustdoc displays the |
||
attribute if and only if the non-1-ZST field is public or at least one field is public in case all |
||
fields are 1-ZST fields. The term *1-ZST* refers to types that are one-aligned and zero-sized. |
||
If you would like to declare the representation as private even if the non-1-ZST field is public, |
||
document it in prose. It would seem that you can manually hide the attribute with |
||
`#[cfg_attr(not(doc), repr(transparent))]`. However, due to [current limitations][cross-crate-cfg-doc], |
||
this does not work for types re-exported from another crate. Therefore, you should always additionally |
||
explicitly write down the layout guarantees in such a case. |
Member
Author
Update: I've slightly tweaked this section, please see the latest revision.
I'm pretty sure that the phrasing in this section can be improved upon, it feels a bit awkward in some places.
Edit: And it's longer than one sentence, notriddle :P
| @@ -41,7 +42,7 @@ impl JsonRenderer<'_> { | ||
}) |
||
.collect(); |
||
let docs = item.opt_doc_value(); |
||
let attrs = item.attributes(self.tcx, true); |
||
let attrs = item.attributes(self.tcx, self.cache(), true); |
Member
Author
Note that my patch doesn't affect Rustdoc JSON. We still pass through every inert attribute. Let me know if I that needs changing.
This comment has been minimized.
|
Changes look good to me overall (didn't go into details yet). It'll need to go through FCP and to have someone from the compiler team to take a look as well. But otherwise, nice work! 👍 |
force-pushed the rustdoc-priv-repr-transparent-heuristic
branch
3 times, most recently
from
a32b08d
to
a7c9170
Compare
added A-rustdoc-ui Area: rustdoc UI (generated HTML) needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release.
labels
Contributor
|
☔ The latest upstream changes (presumably #114855) made this pull request unmergeable. Please resolve the merge conflicts. |
added S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work.
and removed A-rustdoc-json Area: Rustdoc JSON backend
labels
removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-rustdoc-json Area: Rustdoc JSON backend
labels
Contributor
|
@rfcbot fcp merge |
|
Team member @notriddle has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it.
labels
Member
|
(in general please make sure to include a short description of the thing that is actually being FCPd since stuff can change through discussion. Fortunately this has in-prose documentation in the PR so it's easy enough to look through) |
Contributor
|
Good call. The FCP is for the change in behavior mentioned in the book docs.
|
added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised.
and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off.
labels
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting
and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised.
labels
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
removed the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label
|
Thanks everyone! @bors r+ rollup |
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
Member
|
Thank you! I created #116743 to follow up by deleting |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
Successfully merging this pull request may close these issues.
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK