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 raw slice getters (slice_ptr_get) #74265

Open
2 of 4 tasks
Tracked by #16 ...
RalfJung opened this issue Jul 12, 2020 · 26 comments
Open
2 of 4 tasks
Tracked by #16 ...

Tracking Issue for raw slice getters (slice_ptr_get) #74265

RalfJung opened this issue Jul 12, 2020 · 26 comments
Labels
A-raw-pointers Area: raw pointers, MaybeUninit, NonNull A-slice Area: [T] B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Small Libs issues that are considered "small" or self-contained 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

@RalfJung
Copy link
Member

RalfJung commented Jul 12, 2020

This is a tracking issue for indexing methods on raw slices: get_unchecked(_mut) and as_(mut_/non_null_)ptr on raw slices (mutable and const raw pointers and NonNull).

The feature gate for the issue is #![feature(slice_ptr_get)].

Public API

impl<T> *mut [T] {
    pub const fn as_mut_ptr(self) -> *mut T {}
    pub unsafe fn get_unchecked_mut<I>(self, index: I) -> *mut I::Output where I: SliceIndex<[T]>;
}

impl<T> *const [T] {
    pub const fn as_ptr(self) -> *const T {}
    pub unsafe fn get_unchecked<I>(self, index: I) -> *const I::Output where I: SliceIndex<[T]>;
}

impl<T> NonNull<[T]> {
    pub const fn as_non_null_ptr(self) -> NonNull<T> {}
    pub const fn as_mut_ptr(self) -> *mut T {}
    pub unsafe fn get_unchecked_mut<I>(self, index: I) -> NonNull<I::Output> where I: SliceIndex<[T]>;
}

History / Steps

Open questions

@RalfJung RalfJung added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 12, 2020
@JohnTitor JohnTitor added the B-unstable Blocker: Implemented in the nightly compiler and unstable. label Jul 13, 2020
@RalfJung
Copy link
Member Author

#74679 is a potential blocker for this.

@KodrAus KodrAus added A-slice Area: [T] Libs-Small Libs issues that are considered "small" or self-contained Libs-Tracked Libs issues that are tracked on the team's project board. A-raw-pointers Area: raw pointers, MaybeUninit, NonNull labels Jul 29, 2020
@RalfJung
Copy link
Member Author

RalfJung commented Aug 5, 2020

With #75152, the allocator trait started making use of this.

@TimDiekmann
Copy link
Member

Especially with AllocRef using it, it would come in handy if we could also add

impl<T> NonNull<[T]> {
    #[inline]
    fn as_mut_ptr(self) -> *mut T {
        self.as_non_null_ptr().as_ptr()
    }
}

If we want this I'd make a PR.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 6, 2020

Seems reasonable to me -- @Amanieu what do you think?

@TimDiekmann
Copy link
Member

TimDiekmann commented Aug 6, 2020

An alternative name would be as_raw_ptr to avoid confusion with as_ptr, which already returns *mut T. This would also apply to *const [T] and *mut [T].

@Amanieu
Copy link
Member

Amanieu commented Aug 6, 2020

I don't particularly care for the name bikeshed but I agree that this method would be useful.

@TimDiekmann

This comment has been minimized.

JohnTitor added a commit to JohnTitor/rust that referenced this issue Aug 8, 2020
…alfJung

Add `as_mut_ptr` to `NonNull<[T]>`

Adds `as_mut_ptr` to shortcut converting a `NonNull<[T]>` to `*mut T` as proposed in rust-lang#74265 (comment).

r? @RalfJung
@RalfJung
Copy link
Member Author

RalfJung commented Sep 1, 2020

While using these methods for an upcoming PR, I noticed one thing: there is no const-to-mut coercion for raw pointers (at least not for slices, or maybe method lookup just is unable to use it), so one has to use get_unchecked_mut on *mut [T].

I wonder if it would make sense to just not have the mut in the name (but it would likely be inconsistent).

@steffahn
Copy link
Member

steffahn commented Jan 7, 2022

There’s discussion here about adding general *const T -> *mut T and *mut T -> *const T helper methods (as a way to avoid as casts). The method name as_mut_ptr sounds like it does exactly that, but it's currently used here for *mut [T] -> *mut T. Reading the current NonNull documentation was also highly confusing; particularly when looking at the method list in the side-bar.

I would personally appreciate e.g. some mention to slice in the name of the as_[…]ptr family of methods; in any case, I just mainly wanted to note here (just so it won't be forgotten) that in case such general conversion methods are going to be wanted, there might be interest in a different meaning for the method name as_mut_ptr.

@RalfJung
Copy link
Member Author

These methods are needed to work around the problem described in #99437. Though interestingly, to preserve the full semantics of the original code but fix the aliasing problems, we would want bounds-checked getters on raw slices.

@kornelski
Copy link
Contributor

kornelski commented Sep 9, 2022

@steffahn these methods could be named cast_mut() and cast_shared() (or is it const?), alluding to ptr::cast(). cast is the replacement for the as operator in pointers. In types like slice.as_ptr() and vec.as_ptr() this method changes the type, but not shared/mut status.

NonNull is weird tho. Maybe it could get a redundant as_mut_ptr() just to reduce the weirdness?

@JonathanWoollett-Light
Copy link

JonathanWoollett-Light commented Nov 12, 2023

Has there been any progress? It appears all required work is done? This feature will be useful in adding safety munmap, madvise and other mman functions in https://crates.io/crates/nix specifically extension work to nix-rust/nix#2000 as mentioned nix-rust/nix#2000 (comment).

@Ralith
Copy link
Contributor

Ralith commented Nov 16, 2023

The API docs for get_unchecked and get_unchecked_mut say:

Calling this method [...] when self is not dereferenceable is undefined behavior

This is too strong, right? self is not dereferenceable when e.g. an exclusive reference to any of its elements exists, but it should be sound to use these methods to construct pointers to other elements, or even to an already-referenced element if the constructed pointer isn't dereferenced. Was the intention to instead specify that the pointer must address allocated memory?

@RalfJung
Copy link
Member Author

"dereferenceable" generally means "points to memory that is inbounds of an allocation".

However, these functions take references, so it is indeed UB to call them when there are aliasing conflicts: get_unchecked acts like a read of the entire slice, and get_unchecked_mut acts like a write of the entire slice. These rules are not final, but until Rust has a precise aliasing model you should follow these conservative rules to ensure maximal chance of being compatible with the future memory model.

@Ralith
Copy link
Contributor

Ralith commented Nov 16, 2023

"dereferenceable" generally means "points to memory that is inbounds of an allocation".

That's surprising; in Rust API documentation, of a pointer, I'd expect it to mean "it is legal to convert this pointer into a reference".

these functions take references

Are we talking about the same functions? <*const [T]>::get_unchecked takes a raw pointer by value, and passes it on to SliceIndex::get_unchecked:

unsafe { index.get_unchecked(self) }

which adds the index and returns the pointer:

slice.as_ptr().add(self)

I don't see a single reference being accepted, constructed or manipulated at any stage here.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 16, 2023

Are we talking about the same functions?

Sorry, I thought you meant slice::get_unchecked. I didn't realize which issue this is in 😂

That's surprising; in Rust API documentation, of a pointer, I'd expect it to mean "it is legal to convert this pointer into a reference".

No, that's not what it means. It's not "referenceable" after all, it's "dereferenceable", referring to the dereference operator *. Though the semantics of that got relaxed recently so dereferencing can be allowed on pointers that are not dereferenceable... maybe we need a new term.^^

@RalfJung
Copy link
Member Author

RalfJung commented Nov 24, 2023

I think this has been sitting around long enough, clearly a fix for #73987 will not magically materialize. And even with #73987 unresolved, as long as we don't let users write arbitrary-self methods on raw pointers, we can still change the autoref behavior later to use raw pointers when that is enough. So while these methods might have a footgun, they are still better than the status quo.

So I propose we move towards stabilizing the raw pointer part of this. (I am less sure about the NonNull part.)

@rust-lang/types are you aware of anything that would block stabilizing a function that uses a raw slice pointer self type? (AFAIK these would be the first stable functions that do that.)

@BoxyUwU BoxyUwU added the I-types-nominated The issue / PR has been nominated for discussion during a types team meeting. label Nov 25, 2023
@lcnr
Copy link
Contributor

lcnr commented Dec 4, 2023

#71146 (comment) applies to this feature as well

@the8472
Copy link
Member

the8472 commented Jan 9, 2024

This happened to come up in a libs-api meeting. We want to note that stabilization of this feature will likely be blocked on the arbitrary_self_types feature (rust-lang/rfcs#3519) since that will allow moving these methods to

impl [T] {
   fn as_mut_ptr(*const Self) -> *mut T 
}

@RalfJung
Copy link
Member Author

RalfJung commented Jan 9, 2024

Yeah I was about to ask, given @joshtriplett's reply here, what the implications are for this feature.

However, I think we should really offer some way to work with raw slice pointers. The current situation is pretty unfortunate and nothing has improved for many years. If we can't stabilize this as-is, does @rust-lang/libs-api have any alternatives we could pursue?

@the8472
Copy link
Member

the8472 commented Jan 9, 2024

Well, part of the issue is that similar methods have been proposed for array pointers. We'd either have to duplicate them or see what kind of deref-like coercions self types will provide.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 9, 2024

or see what kind of deref-like coercions self types will provide

So far, Rust has largely avoided coercions on raw pointers. That's pretty orthogonal to arbitrary-self-types, so I don't expect rust-lang/rfcs#3519 to change that. So another T-lang RFC would be needed to have such coercions on raw ptr methods I think.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 17, 2024
Add as_(mut_)ptr and as_(mut_)slice to raw array pointers

Hey, first time contributing to the standard libraries so not completely sure about the process.

These functions are complementary to the ones being added in rust-lang#74265 . I found them missing on array pointers.

See also:
- ACP: rust-lang/libs-team#321
- Tracking issue: rust-lang#119834
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 17, 2024
Add as_(mut_)ptr and as_(mut_)slice to raw array pointers

Hey, first time contributing to the standard libraries so not completely sure about the process.

These functions are complementary to the ones being added in rust-lang#74265 . I found them missing on array pointers.

See also:
- ACP: rust-lang/libs-team#321
- Tracking issue: rust-lang#119834
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 17, 2024
Rollup merge of rust-lang#119411 - yotamofek:array-ptr-get, r=Nilstrieb

Add as_(mut_)ptr and as_(mut_)slice to raw array pointers

Hey, first time contributing to the standard libraries so not completely sure about the process.

These functions are complementary to the ones being added in rust-lang#74265 . I found them missing on array pointers.

See also:
- ACP: rust-lang/libs-team#321
- Tracking issue: rust-lang#119834
@Rua
Copy link
Contributor

Rua commented Jun 1, 2024

Is there any interest in implementing a method that does perform bounds checking, while still operating on pointers rather than references?

@RalfJung
Copy link
Member Author

RalfJung commented Jun 2, 2024

That would make sense to me, yeah. I am not in t-libs-api though, so this should probably go through an ACP.

@Rua
Copy link
Contributor

Rua commented Jun 2, 2024

I ran into an issue with this. get_unchecked is implemented for pointers to slices, but not for pointers to fixed-size arrays. We probably want to implement it for arrays too?

@yotamofek
Copy link
Contributor

yotamofek commented Jun 2, 2024

I ran into an issue with this. get_unchecked is implemented for pointers to slices, but not for pointers to fixed-size arrays. We probably want to implement it for arrays too?

Would love to have get_unchecked(_mut) on pointers to arrays, they could maybe live under the array_ptr_get feature. Two notes: 1) you could go through the as_(mut_)slice methods from that feature; 2) if implemented, stabilization of methods on pointers to arrays will likely be blocked on the arbitrary_self_type features, as discussed in the ACP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-raw-pointers Area: raw pointers, MaybeUninit, NonNull A-slice Area: [T] B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Small Libs issues that are considered "small" or self-contained 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