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_swap #83163

Open
3 of 5 tasks
Tracked by #16
usbalbin opened this issue Mar 15, 2021 · 19 comments · May be fixed by #134757
Open
3 of 5 tasks
Tracked by #16

Tracking Issue for const_swap #83163

usbalbin opened this issue Mar 15, 2021 · 19 comments · May be fixed by #134757
Labels
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. 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 Mar 15, 2021

Feature gate: #![feature(const_swap)]

This is a tracking issue for making the functions mem::swap and ptr::swap[_nonoverlapping] and some other swap related functions const fn.

Public API

mod ptr {
    pub const unsafe fn swap<T>(x: *mut T, y: *mut T);
}

mod mem {
    pub const fn swap<T>(x: &mut T, y: &mut T);
}

impl<T> [T] {
    pub const fn swap(&mut self, a: usize, b: usize);
}

impl<T: ?Sized> *mut T {
    pub const unsafe fn swap(self, with: *mut T);
}

impl <T> NonNull<T> {
    pub const unsafe fn swap(self, with: NonNull<T>);
}

Steps / History

Unresolved Questions

@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 Mar 15, 2021
@RalfJung
Copy link
Member

This got reverted again by #86003, but hopefully we can re-land it soon.

@RalfJung
Copy link
Member

RalfJung commented Jun 14, 2021

@pnkfelix I don't quite understand why you un-constified swap... it seems write is the underlying source of the problem. Does it not work to add the const stability attribute here?

@usbalbin
Copy link
Contributor Author

I have reverted the constness reverts in #86295

@est31
Copy link
Member

est31 commented Nov 6, 2021

I've filed #90644 to extend the const_swap feature to three more functions. Some of them can panic, but const_panic is stable as of #89508. Edit: seems that std library functions need #90687 on top of const_panic stabilization. I expect that PR gets merged quite soon.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 12, 2021
Extend the const swap feature

Adds the `const_swap` feature gate to three more swap functions. cc tracking issue rust-lang#83163

```Rust
impl<T> [T] {
    pub const fn swap(&mut self, a: usize, b: usize);
    pub const unsafe fn swap_unchecked(&mut self, a: usize, b: usize);
}
impl<T: ?Sized> *mut T {
    pub const unsafe fn swap(self, with: *mut T);
}
@jhpratt
Copy link
Member

jhpratt commented Dec 1, 2021

This should be blocked on const mut refs, I believe.

@RalfJung
Copy link
Member

RalfJung commented Jun 3, 2022

The following code used to work with swap_nonoverlapping, but doesn't any more:

#![feature(const_swap)]
#![feature(const_mut_refs)]
use std::{
    mem::{self, MaybeUninit},
    ptr,
};

const X: () = {
    let mut ptr1 = &1;
    let mut ptr2 = &2;

    // Swap them, bytewise.
    unsafe {
        ptr::swap_nonoverlapping(
            &mut ptr1 as *mut _ as *mut MaybeUninit<u8>,
            &mut ptr2 as *mut _ as *mut MaybeUninit<u8>,
            mem::size_of::<&i32>(),
        );
    }
    
    // Make sure they still work.
    assert!(*ptr1 == 2);
    assert!(*ptr2 == 1);
};

That is a regression introduced by the new implementation of swap_nonoverlapping in #94212. Though arguably it is a mere coincidence that the previous implementation worked: this is fundamentally caused by a limitation of compile-time evaluation. We have no good way to represent a 'part' of a pointer, so we cannot work on pointers bytewise. Changing the above code to copy a single &i32 rather than 8 MaybeUninit<u8> fixes it.

Not being able to work bytewise on pointer is a general limitation of CTFE, so we could document this as such. However, note that copy and copy_nonoverlapping actually work here; this is because they are implemented via intrinsics which then do copy the entire memory range at once, thus not having to deal with pointers bytewise.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 4, 2022
…ulacrum

test const_copy to make sure bytewise pointer copies are working

This is non-trivial; for `swap_nonoverlapping`, this is [not working](rust-lang#83163 (comment)).
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
test const_copy to make sure bytewise pointer copies are working

This is non-trivial; for `swap_nonoverlapping`, this is [not working](rust-lang/rust#83163 (comment)).
@Dylan-DPC Dylan-DPC added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Sep 13, 2023
@RalfJung
Copy link
Member

RalfJung commented Sep 4, 2024

So... we could easily stabilize ptr::swap once #129195 lands, but ptr::swap_nonoverlapping has a serious limitation currently where it can fail when the data-to-swap contains pointers that cross the "element boundary" of such a swap. (The key difference here is that ptr::swap_nonoverlapping takes a count parameter, but ptr::swap only ever swaps a single element.)

Fixing this requires either an intrinsic for swapping (maybe only used by const-eval), or using const_heap... we have to allocate enough space to do the 3-copy version of swapping, as the more efficient "progressive" swapping is exactly what causes the problem.

@RalfJung
Copy link
Member

RalfJung commented Sep 8, 2024

Here's the test for this:

#[test]
fn test_const_swap() {
    const {
        let mut ptr1 = &1;
        let mut ptr2 = &666;

        // Swap ptr1 and ptr2, bytewise.
        unsafe {
            ptr::swap_nonoverlapping(
                ptr::from_mut(&mut ptr1).cast::<u8>(),
                ptr::from_mut(&mut ptr2).cast::<u8>(),
                mem::size_of::<&i32>(),
            );
        }

        // Make sure they still work.
        assert!(*ptr1 == 666);
        assert!(*ptr2 == 1);
    };

    const {
        let mut ptr1 = &1;
        let mut ptr2 = &666;

        // Swap ptr1 and ptr2, bytewise. `swap` does not take a count
        // so the best we can do is use an array.
        type T = [u8; mem::size_of::<&i32>()];
        unsafe {
            ptr::swap(
                ptr::from_mut(&mut ptr1).cast::<T>(),
                ptr::from_mut(&mut ptr2).cast::<T>(),
            );
        }

        // Make sure they still work.
        assert!(*ptr1 == 666);
        assert!(*ptr2 == 1);
    };
}

@clarfonthey
Copy link
Contributor

Just to comment on this, but swap would be useful to const-stabilise now even if swap_nonoverlapping takes a bit longer, since it would simplify code that has to do a manual 3-way swap a lot.

@RalfJung
Copy link
Member

Feel free to make a PR that proposes stabilizing ptr::swap. :)

@joseluis
Copy link
Contributor

After today's 1.83.0 release this is no longer blocked on const_mut_refs .

@RalfJung
Copy link
Member

RalfJung commented Nov 28, 2024 via email

@RalfJung
Copy link
Member

@rust-lang/libs-api I propose that we const-stabilize all of the above mentioned functions except for swap_nonoverlapping. The problem with swap_nonoverlapping is that it takes a count: isize parameter, and is described as an "untyped" operation acting on count * size_of::<T>() bytes, but the const-implementation currently acts count times on chunks of size_of::<T>() bytes, and that is not the same when there is a pointer that is partially contained in multiple chunks, but wholly contained in the entire large range.

ptr::swap has no count parameter, so there is no similar problem there. And all the other methods can be implemented with ptr::swap.

@RalfJung RalfJung added I-libs-api-nominated Nominated for discussion during a libs-api team meeting. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Nov 29, 2024
@rfcbot
Copy link

rfcbot commented Nov 30, 2024

Team member @dtolnay 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. labels Nov 30, 2024
@dtolnay dtolnay removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Nov 30, 2024
@RalfJung
Copy link
Member

RalfJung commented Nov 30, 2024

Lastly there is <[_]>::swap_unchecked which is still unstable. Let's move its const stability out of this issue to be tracked as part of #88539 instead.

Oh, good catch, I missed that.

I will file a PR to move swap_unchecked and swap_nonoverlapping to separate feature gates.

EDIT: PR is up at #133669.
As part of that I also created #133668 to cover const swap_nonoverlapping.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 1, 2024
…lnay

Move some functions out of const_swap feature gate

- `swap_unchecked` is still unstable as a regular fn, so that feature gate can also cover its constness.
- `swap_nonoverlapping` isn't ready to be stabilized yet, so make it a different feature gate.

Part of rust-lang#83163, rust-lang#88539, rust-lang#133668
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 1, 2024
…lnay

Move some functions out of const_swap feature gate

- `swap_unchecked` is still unstable as a regular fn, so that feature gate can also cover its constness.
- `swap_nonoverlapping` isn't ready to be stabilized yet, so make it a different feature gate.

Part of rust-lang#83163, rust-lang#88539, rust-lang#133668
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 1, 2024
Rollup merge of rust-lang#133669 - RalfJung:const_swap_splitup, r=dtolnay

Move some functions out of const_swap feature gate

- `swap_unchecked` is still unstable as a regular fn, so that feature gate can also cover its constness.
- `swap_nonoverlapping` isn't ready to be stabilized yet, so make it a different feature gate.

Part of rust-lang#83163, rust-lang#88539, rust-lang#133668
@RalfJung
Copy link
Member

@Amanieu @BurntSushi @joshtriplett @m-ou-se FCP checkbox reminder. :)
This const-stabilizes some stable fn that could already be (unsafely) implemented in terms of other stable const fn, so it's a fairly straight-forward extension of our const API surface.

@rfcbot rfcbot added 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 Dec 13, 2024
@rfcbot
Copy link

rfcbot commented Dec 13, 2024

🔔 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 Dec 23, 2024
@rfcbot
Copy link

rfcbot commented Dec 23, 2024

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.

@RalfJung RalfJung linked a pull request Dec 25, 2024 that will close this issue
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Dec 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. 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.

10 participants