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

Document pointer cast safety in std::path #45910

Closed
udoprog opened this issue Nov 10, 2017 · 4 comments · Fixed by #67635
Closed

Document pointer cast safety in std::path #45910

udoprog opened this issue Nov 10, 2017 · 4 comments · Fixed by #67635
Labels
C-feature-accepted Category: A feature request that has been accepted pending implementation. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@udoprog
Copy link
Contributor

udoprog commented Nov 10, 2017

Hey,

I'm curious about the unsafe cast in Path.

From what I understand: this works because Path is a single member struct containing only the type it's being reinterpreted as.

My original question on #rust IRC was: Is this always guaranteed to be correct (in Rust) in terms of memory layout?

The discussion seemed to hint at that this is in fact not a guarantee, but that std is in a unique position being developed in concert with the language and any future breakage would be patched when it occurs.

I would love some clarification if this usage is correct or to what degree it is not. I'd also suggest we add clarification around this case with comments or support functions to aid future spelunking into std.

@cramertj
Copy link
Member

cramertj commented Nov 10, 2017

No, strictly speaking, this is not safe without #[repr(transparent)].

@udoprog
Copy link
Contributor Author

udoprog commented Nov 10, 2017

@cramertj excellent, thank you!

The tracking issue seems to indicate things going slowly. Adding a comment might still be a good idea for now.

Reading the RFC, it mentions at least ARM64 where there might be layout differences. But according to platform support, std shows up as supported as a Tier 2 platform. This implies that at least for now, this pattern in this instance (OsStr) works. Is this correct?

@Mark-Simulacrum Mark-Simulacrum added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 11, 2017
@Mark-Simulacrum Mark-Simulacrum changed the title Use of unsafe cast in std::path Document pointer cast safety in std::path Nov 11, 2017
@bluss
Copy link
Member

bluss commented Nov 12, 2017

@udoprog That ARM64 issue seems to be related to calling convention, not memory layout, so it's not an issue here.

@dtolnay dtolnay added C-feature-accepted Category: A feature request that has been accepted pending implementation. and removed C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Nov 14, 2017
@dtolnay
Copy link
Member

dtolnay commented Nov 14, 2017

The discussion seemed to hint at that this is in fact not a guarantee, but that std is in a unique position being developed in concert with the language and any future breakage would be patched when it occurs.

This is my understanding as well.

No, strictly speaking, this is not safe without #[repr(transparent)].

I believe it is safe with either repr(transparent) or repr(C), the latter of which is stable. The ref-cast crate is a generalization of this safe cast.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Dec 26, 2019
…dtolnay

Document safety of Path casting

I would personally feel more comfortable making the relevant (internal anyway) types repr(transparent) and then documenting that we can make these casts because of that, but I believe this is a more minimal PR, so posting it first.

Resolves rust-lang#45910.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Dec 26, 2019
…dtolnay

Document safety of Path casting

I would personally feel more comfortable making the relevant (internal anyway) types repr(transparent) and then documenting that we can make these casts because of that, but I believe this is a more minimal PR, so posting it first.

Resolves rust-lang#45910.
JohnTitor added a commit to JohnTitor/rust that referenced this issue Dec 27, 2019
…dtolnay

Document safety of Path casting

I would personally feel more comfortable making the relevant (internal anyway) types repr(transparent) and then documenting that we can make these casts because of that, but I believe this is a more minimal PR, so posting it first.

Resolves rust-lang#45910.
@bors bors closed this as completed in 9525e8e Dec 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-accepted Category: A feature request that has been accepted pending implementation. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants