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

Partially stabilize (const_)slice_ptr_len feature by stabilizing NonNull::len #94640

Merged

Conversation

Pointerbender
Copy link
Contributor

@Pointerbender Pointerbender commented Mar 5, 2022

This PR partially stabilizes features const_slice_ptr_len and slice_ptr_len by only stabilizing NonNull::len. This partial stabilization is tracked under features slice_ptr_len_nonnull and const_slice_ptr_len_nonnull, for which this PR can serve as the tracking issue.

To summarize the discussion from #71146 leading up to this partial stabilization request:

It's currently a bit footgunny to obtain the length of a raw slice pointer, stabilization of NonNull:len will help with removing these footguns. Some example footguns are:

/// # Safety
/// The caller must ensure that `ptr`:
/// 1. does not point to memory that was previously allocated but is now deallocated;
/// 2. is within the bounds of a single allocated object;
/// 3. does not to point to a slice for which the length exceeds `isize::MAX` bytes;
/// 4. points to a properly aligned address;
/// 5. does not point to uninitialized memory;
/// 6. does not point to a mutably borrowed memory location.
pub unsafe fn ptr_len<T>(ptr: core::ptr::NonNull<[T]>) -> usize {
   (&*ptr.as_ptr()).len()
}

A slightly less complicated version (but still more complicated than it needs to be):

/// # Safety
/// The caller must ensure that the start of `ptr`:
/// 1. does not point to memory that was previously allocated but is now deallocated;
/// 2. must be within the bounds of a single allocated object.
pub unsafe fn ptr_len<T>(ptr: NonNull<[T]>) -> usize {
   (&*(ptr.as_ptr() as *const [()])).len()
}

This PR does not stabilize <*const [T]>::len and <*mut [T]>::len because the tracking issue #71146 list a potential blocker for these methods, but this blocker does not apply to NonNull::len.

We should probably also ping the Constant Evaluation WG since this PR includes a #[rustc_allow_const_fn_unstable(const_slice_ptr_len)]. My instinct here is that this will probably be okay because the pointer is not actually dereferenced and len() does not touch the address component of the pointer, but would be best to double check :)

One potential down-side was raised that stabilizing NonNull::len could lead to encouragement of coding patterns like:

pub fn ptr_len<T>(ptr: *mut [T]) -> usize {
   NonNull::new(ptr).unwrap().len()
}

which unnecessarily assert non-nullness. However, these are much less of a footgun than the above examples and this should be resolved when slice_ptr_len fully stabilizes eventually.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @yaahc (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 5, 2022
@rust-log-analyzer

This comment has been minimized.

@Pointerbender Pointerbender force-pushed the issue-71146-partial-stabilization branch from f845633 to 9e5d719 Compare March 5, 2022 11:20
@rust-log-analyzer

This comment has been minimized.

@Pointerbender Pointerbender force-pushed the issue-71146-partial-stabilization branch from 9e5d719 to 71ad240 Compare March 5, 2022 12:23
@rust-log-analyzer

This comment has been minimized.

@Pointerbender Pointerbender force-pushed the issue-71146-partial-stabilization branch from 71ad240 to 613f569 Compare March 5, 2022 12:37
@jhpratt
Copy link
Member

jhpratt commented Mar 5, 2022

Diff LGTM. Feature is properly split and attributes are correct.

cc @oli-obk for the usage of #[rustc_allow_const_fn_unstable] — I believe this usage should be fine.

@m-ou-se m-ou-se added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Mar 12, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Mar 12, 2022

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Mar 12, 2022

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. labels Mar 12, 2022
@yaahc yaahc added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 18, 2022
@bstrie
Copy link
Contributor

bstrie commented Apr 7, 2022

+1, extremely in favor of this. Thanks for doing the work here @Pointerbender !

@bstrie bstrie mentioned this pull request Apr 7, 2022
12 tasks
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label May 17, 2022
@rfcbot
Copy link

rfcbot commented May 17, 2022

🔔 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 May 17, 2022
@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 May 27, 2022
@rfcbot
Copy link

rfcbot commented May 27, 2022

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.

@yaahc
Copy link
Member

yaahc commented May 27, 2022

Thank you again @Pointerbender!

@bors r+

@bors
Copy link
Contributor

bors commented May 27, 2022

📌 Commit 021a7e4 has been approved by yaahc

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 27, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request May 28, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#94640 (Partially stabilize `(const_)slice_ptr_len` feature by stabilizing `NonNull::len`)
 - rust-lang#97034 (Implement `Hash` for `core::alloc::Layout`)
 - rust-lang#97327 (macros: introduce `fluent_messages` macro )
 - rust-lang#97448 (docs: Don't imply that OsStr on Unix is always UTF-8)
 - rust-lang#97466 ([bootstrap] Move `sanitize_sh` from `dist` to `install`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 837cd9e into rust-lang:master May 28, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 28, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.