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

Add #[repr(transparent)] to PathBuf #72838

Closed
jhpratt opened this issue May 31, 2020 · 5 comments
Closed

Add #[repr(transparent)] to PathBuf #72838

jhpratt opened this issue May 31, 2020 · 5 comments
Labels
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.

Comments

@jhpratt
Copy link
Member

jhpratt commented May 31, 2020

The fact that PathBuf is internally an OsStr is currently noted as an implementation detail in comments. However, a number of methods clearly indicate (in documentation) that it is stored as such, and one PathBuf::into_os_str() implicitly requires it, as it would be inefficient to do it otherwise.

Would it make sense to add #[repr(transparent)] to PathBuf to externally stabilize this guarantee? It would also allow me (as the maintainer of the standback crate) to use transmute in a guaranteed manner.

@Mark-Simulacrum
Copy link
Member

Can you cite which conversion you are trying to provide externally from std which would be enabled by such? I presuming you mean that PathBuf is internally an OsString, not OsStr?

It also looks like both impl From<PathBuf> for OsString and impl From<OsString> for PathBuf have been present since 1.14 and 1.0 respectively, maybe that's enough?

@jhpratt
Copy link
Member Author

jhpratt commented May 31, 2020

Sure. I'm actually backporting the APIs from 1.44 to 1.31 (the MSRV for this crate). Obviously the repr attribute wouldn't apply to older versions, but if any get stabilized later on, it would allow backporting with safety guarantees.

The From impls actually aren't sufficient, as it requires taking ownership when only borrowing is necessary. As such, I'm currently doing unsafe { transmute::<_, &OsString>(self) }.capacity() to backport (and similar for other methods).

Realistically, I don't see why the attribute shouldn't be added, though. I took a quick look through the impls, and it would be nearly impossible to have any other internal representation without removing some of these.

@Mark-Simulacrum
Copy link
Member

Ah, I see. Technically you could ptr::read and be quite careful to not drop anything, but I suppose that's not better than transmuting.

I would suggest filing a PR which can then be r?'d for someone on the libs team to approve.

@jhpratt
Copy link
Member Author

jhpratt commented May 31, 2020

Will do. I'll also add it to Path for basically the same reason.

@Elinvynia Elinvynia 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 Jun 3, 2020
@dtolnay
Copy link
Member

dtolnay commented Jun 12, 2020

Closing per #72841 (review).

@dtolnay dtolnay closed this as completed Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants