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

Tracking Issue for const_ptr_read #80377

Closed
3 tasks done
usbalbin opened this issue Dec 26, 2020 · 23 comments
Closed
3 tasks done

Tracking Issue for const_ptr_read #80377

usbalbin opened this issue Dec 26, 2020 · 23 comments
Labels
A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@usbalbin
Copy link
Contributor

usbalbin commented Dec 26, 2020

Feature gate: #![feature(const_ptr_read)]

This is a tracking issue for making the functions ptr::read and ptr::read_unaligned, and the same methods on *const T and *mut T, const fn. This unlocks things like moving values out of arrays in const context.

Public API

mod ptr {
    pub const unsafe fn read<T>(src: *const T) -> T;
    pub const unsafe fn read_unaligned<T>(src: *const T) -> T;
}

impl<T> *const T {
    pub const unsafe fn read(self) -> T;
    pub const unsafe fn read_unaligned(self) -> T;
}

impl<T> *mut T {
    pub const unsafe fn read(self) -> T;
    pub const unsafe fn read_unaligned(self) -> T;
}

Steps / History

Related

Unresolved Questions

Inorder to make intrinsics::copy and intrinsics::copy_nonoverlapping compile as const fn, some checks were removed.
See comment for some more info

For this PR, I see two options:

  • Leave it as "something we can do once we have a story for const-dependent dispatch".
  • Comment out the debug assertions for now. Their usefulness is anyway limited since the libstd everyone uses is compiled without debug assertions.

I guess the question is one of evaluating the relative usefulness of these new const operations vs the assertions.

(#79684 did the Comment out the debug assertions for now.-thing).

So the question is, how do we bring them back?

@usbalbin usbalbin added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 26, 2020
@usbalbin usbalbin changed the title Tracking Issue for XXX Tracking Issue for const_ptr_read Dec 26, 2020
usbalbin added a commit to usbalbin/rust that referenced this issue Dec 26, 2020
@mbartlett21
Copy link
Contributor

Should this be mentioned in #57563?

@usbalbin
Copy link
Contributor Author

usbalbin commented Jan 4, 2021

Perhaps #80697 is a better place to discuss the Comment out the debug assertions for now-question? Since #80697 is specifically for copy[_nonoverlapping]

@m-ou-se m-ou-se added the Libs-Tracked Libs issues that are tracked on the team's project board. label Jan 6, 2021
@c410-f3r
Copy link
Contributor

Are write variants also on the track for "constification"?

I guess it can be checked now

@RalfJung
Copy link
Member

Are write variants also on the track for "constification"?

Let's say they are on the wish list. ;) With #80290, making them const should actually be pretty easy now.

@usbalbin
Copy link
Contributor Author

usbalbin commented Jan 18, 2021

Do we need to wait for #80418 (for allowing borrowing) to hit beta before write can be implemented without multiple implementations behind #[cfg(bootstrap)] or is there any other (not too ugly) way? :)

Also intrinsics::forget seems to be not const while mem::forget is const but internally calls ManuallyDrop::new. So how would one best solve that without undoing the "inlining" in #80290?

@RalfJung
Copy link
Member

Do we need to wait for #80418 (for allowing borrowing) to hit beta before write can be implemented without multiple implementations behind #[cfg(bootstrap)] or is there any other (not too ugly) way? :)

Well... you could use &mut. 😆
Probably it's better to wait, though.

mem::forget is const but internally calls ManuallyDrop::new

This is supposed to be changed in #79989.

@camelid camelid added the A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. label Jan 18, 2021
@c410-f3r
Copy link
Contributor

c410-f3r commented Jan 19, 2021

Thank you, @RalfJung and @usbalbin !

@slightlyoutofphase
Copy link
Contributor

Just popping in to say that I just found out that this and const_ptr_write exist and it's like Christmas TBH. The next version I release of my crate staticvec is going to be absolutely nuts thanks to these, with const push, const pop, const insert, and various other things.

One thing I'd ask though is if there are plans to make some_ptr.copy_to(), some_ptr.copy_from() and so on also const-compatible? It seems like there's nothing blocking it, as all of those functions just directly call these intrinsic ones IIRC, and it would be a nice addition on top of this from an ergonomics standpoint.

@RalfJung
Copy link
Member

One thing I'd ask though is if there are plans to make some_ptr.copy_to(), some_ptr.copy_from() and so on also const-compatible?

Yeah, those could all easily be made const fn as well, I think.

@usbalbin
Copy link
Contributor Author

Perhaps something like this #83091

@slightlyoutofphase
Copy link
Contributor

slightlyoutofphase commented Mar 13, 2021

Perhaps something like this #83091

That looks great if it all works! Didn't even think about replace, but I'm pretty sure that would allow me to make even more things const too, actually.

Here's an example snippet taking advantage of what's been implemented so far that I'll be including in the demo program for my crate with the next release, by the way:

// It's also possible to write compile-time initialization functions that suit your specific needs
// with "complex" logic taking advantage of various methods you might typically expect to only
// be available at runtime.
const fn build<T: Copy, const N: usize>(x: [T; N]) -> StaticVec<T, N> {
  // StaticVec implements `Drop`, and so can't *directly* be returned from a `const fn` yet (at
  // least specifically if / when first instantiated as a fully-initialized "naked" function-local
  // variable) even when `T` is `Copy`, making the (sound) use of MaybeUninit below necessary to
  // facilitate regular access to the StaticVec we'll be creating.
  let mut mu = MaybeUninit::new(StaticVec::new());
  // Not actually unsafe here: `sv` is just a mutable reference to a normal empty StaticVec.
  let sv = unsafe { mu.assume_init_mut() };
  // `StaticVec::push` is a `const fn`. Note that loops in `const fn` are limited to `while`
  // currently: if that changes, obviously use an iterator-based `for` loop instead.
  let mut i = 0;
  while i < N {
    sv.push(x[i]);
    i += 1;
  }
  // `StaticVec::pop` is also a `const fn`, so we can do this as well...
  while sv.pop().is_some() {}
  // And put everything back in like this...
  sv.insert_from_slice(0, &x);
  // Still not actually unsafe here: we soundly initialized everything that needed it as soon as we
  // called `StaticVec::new()`.
  unsafe { mu.assume_init() }
  // The methods demonstrated above are by no means the only ones for StaticVec that could be used
  // (and might be useful) in a context like this, so do go ahead and peruse the docs for this crate
  // to find out more.
}

Possibly worth noting that the pop() part of that required adding a feature flag called const_refs_to_cell, which I'd never even heard of. Based on the name of it I'm not sure if that's 100% intentional (as staticvec certainly does not use Cell or even import it), so I figured I'd point it out at least.

@RalfJung

This comment has been minimized.

@RalfJung

This comment has been minimized.

@jhpratt
Copy link
Member

jhpratt commented Oct 19, 2021

@RalfJung @oli-obk any blockers here?

@oli-obk
Copy link
Contributor

oli-obk commented Oct 19, 2021

T-lang approval on #89551 is the blocker that I see. After that, this is purely a libs issue

@jhpratt
Copy link
Member

jhpratt commented Oct 19, 2021

I was presupposing that that is ok'd. Sounds good though.

@Lokathor
Copy link
Contributor

The item that Oli linked was approved and merged. Does this mean we're ready to move forward, or have other blockers come up in the past few months?

@oli-obk
Copy link
Contributor

oli-obk commented May 22, 2022

Nope, a PR assigned to T-libs-api should be uncontroversial now

@m-ou-se
Copy link
Member

m-ou-se commented Apr 5, 2023

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Apr 5, 2023

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Apr 5, 2023
@rfcbot
Copy link

rfcbot commented Apr 5, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 15, 2023
@rfcbot
Copy link

rfcbot commented Apr 15, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 20, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 9, 2023
…r=m-ou-se

Stabilize const_ptr_read

Stabilizes const_ptr_read, with tracking issue rust-lang#80377
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 9, 2023
…r=m-ou-se

Stabilize const_ptr_read

Stabilizes const_ptr_read, with tracking issue rust-lang#80377
@RalfJung
Copy link
Member

Is there anything left here? #97320 should have closed this issue, no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests