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 6 tasks
Tracked by #16
usbalbin opened this issue Mar 15, 2021 · 10 comments
Open
3 of 6 tasks
Tracked by #16

Tracking Issue for const_swap #83163

usbalbin opened this issue Mar 15, 2021 · 10 comments
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC S-blocked Status: Blocked on something else such as an RFC or other implementation work. 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);
    pub const unsafe fn swap_nonoverlapping<T>(x: *mut T, y: *mut T, count: usize);
}

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);
    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);
}

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. :)

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 S-blocked Status: Blocked on something else such as an RFC or other implementation work. 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

6 participants