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.

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

std: add some missing repr(transparent) #114800

Merged
merged 2 commits into from
Aug 15, 2023
Merged

Conversation

RalfJung
Copy link
Member

For some types we don't want to stably guarantee this, so hide the repr from rustdoc. This nice approach was suggested by @thomcc.

@rustbot
Copy link
Collaborator

rustbot commented Aug 14, 2023

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 14, 2023
@ChrisDenton
Copy link
Member

Once #111544 is stable I think it'll be ok for #[repr(transparent)] to appear in the OsStr docs.

Comment on lines 91 to 93
// For now we just hide this from rustdoc, technically making our doc test builds rely on
// unspecified layout assumptions. We are std, so we can get away with that.
#[cfg_attr(not(doc), repr(transparent))]
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this affects doc tests? I tried this on a new library and it compiled, but failed the assertion:

/// ```
/// assert!(cfg!(doc));
/// ```
pub struct Foo(u32);

I do expect your cfg will affect how rustdoc parses the source when it looks for doctests, but not the actual library or doctest build.

Copy link
Member

Choose a reason for hiding this comment

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

Another way to see that doc tests are not affected:

/// ```
/// use lib::Foo;
/// use std::mem::align_of;
///
/// assert!(!cfg!(doc));
/// assert_eq!(align_of::<Foo>(), align_of::<u32>());
/// ```
#[cfg_attr(doc, repr(packed))]
pub struct Foo(u32);

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I guess doctests are built with cfg(doctest)... nice! So this is actually a reasonable approach then.

@cuviper
Copy link
Member

cuviper commented Aug 14, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 14, 2023

📌 Commit fe1a034 has been approved by cuviper

It is now in the queue for this repository.

@bors bors 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 Aug 14, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 15, 2023
std: add some missing repr(transparent)

For some types we don't want to stably guarantee this, so hide the `repr` from rustdoc. This nice approach was suggested by `@thomcc.`
@bjorn3
Copy link
Member

bjorn3 commented Aug 15, 2023

Won't this still show up for inlined docs? Those are reading from the crate metadata of a regular compilation.

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 15, 2023
…llaumeGomez

Rollup of 10 pull requests

Successful merges:

 - rust-lang#114711 (Infer `Lld::No` linker hint when the linker stem is a generic compiler driver)
 - rust-lang#114772 (Add `{Local}ModDefId` to more strongly type DefIds`)
 - rust-lang#114800 (std: add some missing repr(transparent))
 - rust-lang#114820 (Add test for unknown_lints from another file.)
 - rust-lang#114825 (Upgrade std to gimli 0.28.0)
 - rust-lang#114827 (Only consider object candidates for object-safe dyn types in new solver)
 - rust-lang#114828 (Probe when assembling upcast candidates so they don't step on eachother's toes in new solver)
 - rust-lang#114829 (Separate `consider_unsize_to_dyn_candidate` from other unsize candidates)
 - rust-lang#114830 (Clean up some bad UI testing annotations)
 - rust-lang#114831 (Check projection args before substitution in new solver)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f527d56 into rust-lang:master Aug 15, 2023
11 checks passed
@cuviper
Copy link
Member

cuviper commented Aug 15, 2023

Won't this still show up for inlined docs?

Oh, yes it does for std::ffi::CStr, so I suppose we should revert that one for now.

The rest are std-native and look ok.

@cuviper
Copy link
Member

cuviper commented Aug 15, 2023

That flaw also exists for other core/std re-exports -- task::Waker and a few unstable items.

@rustbot rustbot added this to the 1.73.0 milestone Aug 15, 2023
@RalfJung RalfJung deleted the transparent branch August 15, 2023 19:28
@RalfJung
Copy link
Member Author

RalfJung commented Aug 15, 2023

Hm, instead of reverting can we add a comment saying that the repr is not guaranteed?

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 17, 2023
clarify CStr lack of layout guarnatees

Follow-up to rust-lang#114800
r? `@cuviper`
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Oct 17, 2023
clarify CStr lack of layout guarnatees

Follow-up to rust-lang/rust#114800
r? `@cuviper`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants