Add #[repr(C)] to Path

[torkleyy]

Path::new casts a *const OsStr to a *const Path. I don't think this is strictly allowed when using Rust's struct layout.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[Mark-Simulacrum]

cc @eddyb

[hanna-kruppe]

It would be at least dubious in user code, but libstd is tied to the compiler version anyway so it's allowed to make additional assumptions.

I am also uncertain what repr(C) on a DST would even mean.

[torkleyy]

I just wondered how I would handle DSTs myself, that's where I looked first. I figured it would be good to have correct code in the standard library source, otherwise users will copy it and it breaks later.

[hanna-kruppe]

I figured it would be good to have correct code in the standard library source, otherwise users will copy it and it breaks later.

I understand, but I am not sure whether we're actually guaranteeing that repr(C) does the right thing in this case.

[torkleyy]

Is there any correct way to do this then?

[hanna-kruppe]

As I said, I'm really not sure.

[retep998]

Shouldn't the correct way be #[repr(transparent)]?

[hanna-kruppe]

I don't think anyone has ever thought about what that means for DSTs. FWIW, it's pretty much like repr(C) wrt memory layout (the difference only matters for calling conventions). But besides the layout there's also the question of the metadata (in this case, the length). Or more generally the fact that DST layout is weird and different from non-DST layout.

[clarfonthey]

Considering how this is part of the standard library, if the compiler were changed so that this didn't work, we'd know about it before that change were even accepted.

Which, mind you, would probably break a lot of crates anyway...

[eddyb]

As other people have people out, libstd's implementation is already quite compiler-specific.

Since these are DSTs and never passed by value, there is no "call ABI" concern AFAIK, and, in memory, both Rust and C have the same newtype layout (that of the inner type), but it's not clear that we guarantee it on the Rust side, so you can only assume it in an unstable compiler-specific way.

But that's still fine, IMO. I don't think we need to do anything specific unless there's any problem.
Oh and raw pointer casts are not like transmutes, they check that the "metadata" part of the pointer matches, such that only pointers to newtypes of slices can be converted between eachother, e.g. you can't cast *mut Path to *mut fmt::Debug (note that it can't unsize either because Path: !Sized).

[shepmaster]

Ping from triage, @alexcrichton

[alexcrichton]

Ah thanks for the ping! It looks like the discussion here is concluding that it's unclear whether this attribute is necessary at this time, right? In light of that I'm personally tempted to close this as "working as intended" and we can revisit it later if it becomes a problem. @torkleyy how does that sound to you?

[torkleyy]

Yeah I realized that this PR doesn't make the code any more or less correct than it was.

For now I'm just going to use this pattern for my code and make sure I update my code should there ever be a better way to create DSTs.