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

Make ptr::from_raw_parts more general #362

Closed
scottmcm opened this issue Mar 29, 2024 · 4 comments
Closed

Make ptr::from_raw_parts more general #362

scottmcm opened this issue Mar 29, 2024 · 4 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@scottmcm
Copy link
Member

scottmcm commented Mar 29, 2024

Proposal

Problem statement

Today, ptr::from_raw_parts can only accept *const () for the data_pointer.

That means that common cases -- like making a slice pointer -- have to cast.

Motivating examples or use cases

ptr::slice_from_raw_parts exists primarily so that something could be stabilized, but it's also there because if you're making a *const [T] what you really want to provide is a *const T and a usize, not a *const () and a usize.

There's really no reason, though, that ptr::from_raw_parts couldn't just take the *const T directly.

Solution sketch

Make a couple small tweaks to the signatures:

 pub const fn from_raw_parts<T: ?Sized>(
-    data_pointer: *const (),
+    data_pointer: *const impl Thin,
     metadata: <T as Pointee>::Metadata,
 ) -> *const T { … }
 pub const fn from_raw_parts_mut<T: ?Sized>(
-    data_pointer: *mut (),
+    data_pointer: *mut impl Thin,
     metadata: <T as Pointee>::Metadata,
 ) -> *mut T { … }

etc.

That way you still can't pass a fat pointer as the data_pointer, but if you happen to have a *mut u8 from an allocator, or something, that's fine.

And the turbo-fishing still lets you do from_raw_parts::<[T]> or from_raw_parts::<dyn Foo> without needing an extra _.

Alternatives

Obviously doing nothing here is a feasible option. It's not like casting is difficult.

But like with byte_offset_from (see rust-lang/rust#103489 ), there's really no particular reason to force a specific type here, and it's handy to accept multiple.

Links and related work

Inspired by rust-lang/rust#123190 since the extra casts were pushing Vec::deref over the MIR inlining threshold, but the cast to *const () also wasn't providing any value, so it would be nice to just not make people write it at all.

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@scottmcm scottmcm added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Mar 29, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue May 7, 2024
…ts, r=<try>

Avoid a cast in `ptr::slice_from_raw_parts(_mut)`

Casting to `*const ()` or `*mut ()` is no longer needed after rust-lang#123840 so let's make the MIR smaller (and more inline-able, as seen in the tests).

If [ACP#362](rust-lang/libs-team#362) goes through we can keep calling `ptr::from_raw_parts(_mut)` in these also without the cast, but that hasn't had any libs-api attention yet, so I'm not waiting on it.
bors added a commit to rust-lang-ci/rust that referenced this issue May 8, 2024
…ts, r=joboet

Avoid a cast in `ptr::slice_from_raw_parts(_mut)`

Casting to `*const ()` or `*mut ()` is no longer needed after rust-lang#123840 so let's make the MIR smaller (and more inline-able, as seen in the tests).

If [ACP#362](rust-lang/libs-team#362) goes through we can keep calling `ptr::from_raw_parts(_mut)` in these also without the cast, but that hasn't had any libs-api attention yet, so I'm not waiting on it.
@scottmcm
Copy link
Member Author

Nominating because it seems like it fell off the "new ACPs" agenda without comment. It's not urgent, though; address it only after any more-pressing nominees.

@rustbot labels +I-libs-api-nominated

@rustbot rustbot added the I-libs-api-nominated Indicates that an issue has been nominated for discussion during a team meeting. label May 27, 2024
@the8472
Copy link
Member

the8472 commented May 28, 2024

Ideally we'd do better with something like *const Thin<T> or whatever so it still carries the type. But I think we don't have a way to name a thin-pointer-equivalent to something that needs metadata

@scottmcm
Copy link
Member Author

scottmcm commented May 28, 2024

Interesting, @the8472 . I think that'd be particularly useful for to_raw_parts, since today it just shrugs and gives everyone a pointer-to-unit.

For constructing, I do wonder about things like the "flat vector of dyn", where it might in practice have *const u8s anyway, so from_raw_parts::<dyn Foo>(p, vtable) might still be preferable to p.cast::<Thin<dyn Foo>>().with_meta(vtable).

I guess we could always experiment with a Thin that's just a newtype on an opaque type and see how it goes? I'll still make this change to from_raw_parts in the mean time, since it sounds like it was approved today and it might save a case in hypothetical Thin::with_meta implementation anyway.

TL/DR: from_raw_parts is still unstable, so we can always change it again in a future ACP 🙂

@Amanieu Amanieu removed the I-libs-api-nominated Indicates that an issue has been nominated for discussion during a team meeting. label May 28, 2024
@Amanieu
Copy link
Member

Amanieu commented May 28, 2024

We discussed this in the libs-api meeting today and we're mostly happy to accept it. We can defer the final decision to stabilization since this depends on several unstable features (e.g. trait aliases for Thin).

@Amanieu Amanieu closed this as completed May 28, 2024
@Amanieu Amanieu added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label May 28, 2024
fmease added a commit to fmease/rust that referenced this issue May 29, 2024
…=WaffleLapkin

[ACP 362] genericize `ptr::from_raw_parts`

This implements rust-lang/libs-team#362

As such, it can partially undo rust-lang#124795 , letting `slice_from_raw_parts` just call `from_raw_parts` again without re-introducing the unnecessary cast to MIR.

By doing this it also removes a spurious cast from `str::from_raw_parts`.  And I think it does a good job of showing the value of the ACP, since the only thing that needed new turbofishing because of this is inside `ptr::null(_mut)`, but only because `ptr::without_provenance(_mut)` doesn't support pointers to extern types, which it absolutely could (without even changing the implementation).
fmease added a commit to fmease/rust that referenced this issue May 29, 2024
…=WaffleLapkin

[ACP 362] genericize `ptr::from_raw_parts`

This implements rust-lang/libs-team#362

As such, it can partially undo rust-lang#124795 , letting `slice_from_raw_parts` just call `from_raw_parts` again without re-introducing the unnecessary cast to MIR.

By doing this it also removes a spurious cast from `str::from_raw_parts`.  And I think it does a good job of showing the value of the ACP, since the only thing that needed new turbofishing because of this is inside `ptr::null(_mut)`, but only because `ptr::without_provenance(_mut)` doesn't support pointers to extern types, which it absolutely could (without even changing the implementation).
rust-timer added a commit to rust-lang-ci/rust that referenced this issue May 30, 2024
Rollup merge of rust-lang#125701 - scottmcm:generic-from-raw-parts, r=WaffleLapkin

[ACP 362] genericize `ptr::from_raw_parts`

This implements rust-lang/libs-team#362

As such, it can partially undo rust-lang#124795 , letting `slice_from_raw_parts` just call `from_raw_parts` again without re-introducing the unnecessary cast to MIR.

By doing this it also removes a spurious cast from `str::from_raw_parts`.  And I think it does a good job of showing the value of the ACP, since the only thing that needed new turbofishing because of this is inside `ptr::null(_mut)`, but only because `ptr::without_provenance(_mut)` doesn't support pointers to extern types, which it absolutely could (without even changing the implementation).
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Jun 28, 2024
…pkin

[ACP 362] genericize `ptr::from_raw_parts`

This implements rust-lang/libs-team#362

As such, it can partially undo rust-lang/rust#124795 , letting `slice_from_raw_parts` just call `from_raw_parts` again without re-introducing the unnecessary cast to MIR.

By doing this it also removes a spurious cast from `str::from_raw_parts`.  And I think it does a good job of showing the value of the ACP, since the only thing that needed new turbofishing because of this is inside `ptr::null(_mut)`, but only because `ptr::without_provenance(_mut)` doesn't support pointers to extern types, which it absolutely could (without even changing the implementation).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

4 participants