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 len() method (slice_ptr_len, const_slice_ptr_len) #71146

Closed
5 of 7 tasks
neocturne opened this issue Apr 14, 2020 · 45 comments · Fixed by #123868
Closed
5 of 7 tasks

Tracking Issue for raw slice len() method (slice_ptr_len, const_slice_ptr_len) #71146

neocturne opened this issue Apr 14, 2020 · 45 comments · Fixed by #123868
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: 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

@neocturne
Copy link
Contributor

neocturne commented Apr 14, 2020

This is a tracking issue for the len() method on raw slice pointers, which allows to get the length of a raw slice pointer when conversion to a slice reference would be UB (because the pointer is null or unaligned).

It covers the feature gates #![feature(slice_ptr_len)] and #![feature(const_slice_ptr_len)] (for const fn).

Public API

impl<T> *mut [T] {
    pub const fn len(self) -> usize;
    pub const fn is_empty(self) -> bool;
}

impl<T> *const [T] {
    pub const fn len(self) -> usize;
    pub const fn is_empty(self) -> bool;
}

impl<T> NonNull<[T]> {
    pub const fn len(self) -> usize; // Already stable
    pub const fn is_empty(self) -> bool;
}

History / Steps

Open questions

@neocturne neocturne added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Apr 14, 2020
@jonas-schievink jonas-schievink added B-unstable Blocker: Implemented in the nightly compiler and unstable. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 14, 2020
@SimonSapin
Copy link
Contributor

PR #71940 adds NonNull<[T]>::len, reusing this as its tracking issue.

SimonSapin added a commit to SimonSapin/rust that referenced this issue May 18, 2020
This follows the precedent of the recently-added `<*const [T]>::len`
(adding to its tracking issue rust-lang#71146)
and `ptr::slice_from_raw_parts`.
RalfJung added a commit to RalfJung/rust that referenced this issue May 25, 2020
Add `len` and `slice_from_raw_parts` to `NonNull<[T]>`

This follows the precedent of the recently-added `<*const [T]>::len` (adding to its tracking issue rust-lang#71146) and `ptr::slice_from_raw_parts`.
@RalfJung
Copy link
Member

RalfJung commented Jul 3, 2020

#73987 is a potential blocker for stabilization of these methods.

@SimonSapin
Copy link
Contributor

Is it really? I feel that a DST struct with a slice field is not a common case, and certainly not the only one.

@RalfJung
Copy link
Member

RalfJung commented Jul 3, 2020

That issue shows a general problem with methods where self is a raw pointer: if a method with the same name as self being a reference exists, it is very easy to accidentally call the reference method. Isn't that a problem for stabilizing any of those raw pointer methods?

@SimonSapin
Copy link
Contributor

This does look like a footgun, but I think the core of that issue is not at all the len methods. Instead it’s that you can’t get a raw pointer to a struct field from a raw pointer to that struct without something like rust-lang/rfcs#2582. The example code in #73987 never involved a *const [u8] value at all. I’ve commented some more there.

Once you do have a *const [u8] value, calling len on it is perfectly fine.

@RalfJung
Copy link
Member

RalfJung commented Jul 3, 2020

(Responded there. What you describe is buggy behavior, IMO -- the source code didn't involve a &[u8] either, the compiler added that, and that is exactly the problem.)

@RalfJung RalfJung changed the title Tracking Issue for raw slice len() method Tracking Issue for raw slice len() method (slice_ptr_len, const_slice_ptr_len) Jul 12, 2020
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 13, 2020
add (unchecked) indexing methods to raw (and NonNull) slices

This complements the existing (unstable) `len` method. Unfortunately, for non-null slices, we cannot call this method `as_ptr` as that overlaps with the existing method of the same name.

If this looks reasonable to accept, I propose to reuse the rust-lang#71146 tracking issue and rename the feature get to `slice_ptr_methods` or so.

Cc @SimonSapin
Fixes rust-lang#60639
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 13, 2020
add (unchecked) indexing methods to raw (and NonNull) slices

This complements the existing (unstable) `len` method. Unfortunately, for non-null slices, we cannot call this method `as_ptr` as that overlaps with the existing method of the same name.

If this looks reasonable to accept, I propose to reuse the rust-lang#71146 tracking issue and rename the feature get to `slice_ptr_methods` or so.

Cc @SimonSapin
Fixes rust-lang#60639
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 13, 2020
add (unchecked) indexing methods to raw (and NonNull) slices

This complements the existing (unstable) `len` method. Unfortunately, for non-null slices, we cannot call this method `as_ptr` as that overlaps with the existing method of the same name.

If this looks reasonable to accept, I propose to reuse the rust-lang#71146 tracking issue and rename the feature get to `slice_ptr_methods` or so.

Cc @SimonSapin
Fixes rust-lang#60639
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 13, 2020
add (unchecked) indexing methods to raw (and NonNull) slices

This complements the existing (unstable) `len` method. Unfortunately, for non-null slices, we cannot call this method `as_ptr` as that overlaps with the existing method of the same name.

If this looks reasonable to accept, I propose to reuse the rust-lang#71146 tracking issue and rename the feature get to `slice_ptr_methods` or so.

Cc @SimonSapin
Fixes rust-lang#60639
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 14, 2020
add (unchecked) indexing methods to raw (and NonNull) slices

This complements the existing (unstable) `len` method. Unfortunately, for non-null slices, we cannot call this method `as_ptr` as that overlaps with the existing method of the same name.

If this looks reasonable to accept, I propose to reuse the rust-lang#71146 tracking issue and rename the feature get to `slice_ptr_methods` or so.

Cc @SimonSapin
Fixes rust-lang#60639
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 14, 2020
add (unchecked) indexing methods to raw (and NonNull) slices

This complements the existing (unstable) `len` method. Unfortunately, for non-null slices, we cannot call this method `as_ptr` as that overlaps with the existing method of the same name.

If this looks reasonable to accept, I propose to reuse the rust-lang#71146 tracking issue and rename the feature get to `slice_ptr_methods` or so.

Cc @SimonSapin
Fixes rust-lang#60639
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 14, 2020
add (unchecked) indexing methods to raw (and NonNull) slices

This complements the existing (unstable) `len` method. Unfortunately, for non-null slices, we cannot call this method `as_ptr` as that overlaps with the existing method of the same name.

If this looks reasonable to accept, I propose to reuse the rust-lang#71146 tracking issue and rename the feature get to `slice_ptr_methods` or so.

Cc @SimonSapin
Fixes rust-lang#60639
@KodrAus KodrAus added A-slice Area: `[T]` 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
@Pointerbender
Copy link
Contributor

Hello everyone. Thank you for working on this feature! I understand that currently #73987 is a blocker for slice_ptr_len. As an end-user of this API, is there any feedback I could provide that would help decide on the method signature? FWIW, I've not seen any risky situations in my usage due to its current method signature (I tend to check to which method it resolves to using my IDE in case there are multiple potential matches, so this might be more thorough than the average end-user). In an attempt to spark new ideas, an alternative I see is to turn the method signature pub const fn len(self) -> usize into an associated function like pub const fn len(ptr: Self) -> usize if this is really blocking, then it would be impossible to get confused (but at the cost of ergonomics). But as an end-user, I would also be satisfied for how the signature is currently available already, I do not find it confusing as it is. I'd like to help out more if I can, please let me know if I can contribute somehow :-) Thank you!

@Pointerbender
Copy link
Contributor

A second idea for a suggestion that came to me today. Currently this tracking issues is for all of <*const T>::len, <*mut T>::len and NonNull::<T>::len. If I understand correctly, #73987 is only related to <*const T>::len and <*mut T>::len, but does not affect NonNull::<T>::len(), because creating a reference &NonNull<T> through len() would merely be a reference to the pointer to T, not to the actual T. If I'm not mistaken, the internals of NonNull::<T>::len() are still allowed to call the non-stabilized pointer methods, even when NonNull::<T>::len() would already be stable (I think remember seeing this approach before when browsing through the core library source code). Is there a possibility that NonNull::<T>::len() could get stabilized already before the other methods?

@RalfJung
Copy link
Member

#73987 indeed does not affect NonNull::len.

@Amanieu
Copy link
Member

Amanieu commented Mar 20, 2024

Is there a particular reason why there is no NonNull::<[T]>::is_empty?

That's a good point. It should be added as well for consistency with raw pointers.

@zachs18
Copy link
Contributor

zachs18 commented Mar 21, 2024

As per #122800 (comment), #122800 currently uses this issue as the tracking issue for NonNull::<[T]>::is_empty

@RalfJung
Copy link
Member

Adding something to FCP after the boxes were already checked is rather unconventional (I expected a new tracking issue for a new function) -- @rust-lang/libs-api I hope you're okay with it (a team member proposed to do this after all).

@Amanieu
Copy link
Member

Amanieu commented Mar 22, 2024

I've added NonNull::is_empty to the tracking issue.

I'll restart the FCP to make sure everyone is aware of the change.

@rfcbot fcp cancel

@rfcbot
Copy link

rfcbot commented Mar 22, 2024

@Amanieu proposal cancelled.

@rfcbot rfcbot removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 22, 2024
@Amanieu
Copy link
Member

Amanieu commented Mar 22, 2024

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Mar 22, 2024

Team member @Amanieu 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. labels Mar 22, 2024
@rfcbot
Copy link

rfcbot commented Mar 22, 2024

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Mar 22, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 22, 2024
…Amanieu

Add `NonNull::<[T]>::is_empty`.

As per rust-lang#71146 (comment)

I figured this should be fine to be insta-stable (with an FCP), but I can edit if that is not desired.

r? `@Amanieu`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 22, 2024
…Amanieu

Add `NonNull::<[T]>::is_empty`.

As per rust-lang#71146 (comment)

I figured this should be fine to be insta-stable (with an FCP), but I can edit if that is not desired.

r? ``@Amanieu``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 22, 2024
…Amanieu

Add `NonNull::<[T]>::is_empty`.

As per rust-lang#71146 (comment)

I figured this should be fine to be insta-stable (with an FCP), but I can edit if that is not desired.

r? ```@Amanieu```
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 22, 2024
Rollup merge of rust-lang#122800 - zachs18:nonnull-slice-is_empty, r=Amanieu

Add `NonNull::<[T]>::is_empty`.

As per rust-lang#71146 (comment)

I figured this should be fine to be insta-stable (with an FCP), but I can edit if that is not desired.

r? ```@Amanieu```
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 1, 2024
@rfcbot
Copy link

rfcbot commented Apr 1, 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.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Apr 1, 2024
@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Apr 2, 2024
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 4, 2024
jhpratt added a commit to jhpratt/rust that referenced this issue Apr 13, 2024
… r=jhpratt

Stabilize (const_)slice_ptr_len and (const_)slice_ptr_is_empty_nonnull

Stabilized API:

```rust
impl<T> *mut [T] {
    pub const fn len(self) -> usize;
    pub const fn is_empty(self) -> bool;
}

impl<T> *const [T] {
    pub const fn len(self) -> usize;
    pub const fn is_empty(self) -> bool;
}

impl<T> NonNull<[T]> {
    pub const fn is_empty(self) -> bool;
}
```

FCP completed in tracking issue: rust-lang#71146
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 13, 2024
Rollup merge of rust-lang#123868 - eduardosm:stabilize-slice_ptr_len, r=jhpratt

Stabilize (const_)slice_ptr_len and (const_)slice_ptr_is_empty_nonnull

Stabilized API:

```rust
impl<T> *mut [T] {
    pub const fn len(self) -> usize;
    pub const fn is_empty(self) -> bool;
}

impl<T> *const [T] {
    pub const fn len(self) -> usize;
    pub const fn is_empty(self) -> bool;
}

impl<T> NonNull<[T]> {
    pub const fn is_empty(self) -> bool;
}
```

FCP completed in tracking issue: rust-lang#71146
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: 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

Successfully merging a pull request may close this issue.