rust icon indicating copy to clipboard operation
rust copied to clipboard

Nuke slice_as{,_mut}_ptr methods

Open SUPERCILEX opened this issue 1 year ago • 7 comments

These methods originate from the MaybeUninit RFC and were provided without justification. They are objectively bad because they misunderstand the point of having a slice of MaybeUninits: you're supposed to use the safer MaybeUninit methods instead of working with raw pointers. The proof of this misunderstanding is evidenced by the fixed usages in the stdlib from this PR: many usages work with unsafe raw pointers completely unnecessarily. It possible that raw pointers were used to avoid bounds checking performance hits, but minimal evidence of this being the case was found.

This PR adds MaybeUninit<T> pointer to T pointer conversions to guarantee the safety of pointer casts where necessary. These should be From impls (unless it is decided that the dangers of having a raw pointer to potentially uninitialized memory warrants a separate method), but aren't because I don't want to insta-stable. These impls could also not be added, but I worry about the possibility of incorrect casts.

SUPERCILEX avatar Oct 17 '22 03:10 SUPERCILEX

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

rustbot avatar Oct 17 '22 03:10 rustbot

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

rust-highfive avatar Oct 17 '22 03:10 rust-highfive

r? @RalfJung

SUPERCILEX avatar Oct 17 '22 03:10 SUPERCILEX

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
Attempting with retry: make prepare
---
* highest error code: E0790
Found 506 error codes
Found 0 error(s) in error codes
Done!
tidy error: /checkout/library/core/src/fmt/num.rs:397: undocumented unsafe
tidy error: /checkout/library/core/src/fmt/num.rs:400: undocumented unsafe
tidy error: /checkout/library/core/src/mem/maybe_uninit.rs:1309: TODO is deprecated; use FIXME
some tidy checks failed
Build completed unsuccessfully in 0:00:09

rust-log-analyzer avatar Oct 17 '22 03:10 rust-log-analyzer

I'm not a libs API person, so I'm the wrong reviewer here. r? libs

RalfJung avatar Oct 17 '22 06:10 RalfJung

Right, keep forgetting, sorry! :)

SUPERCILEX avatar Oct 17 '22 06:10 SUPERCILEX

If you have to add a completely new method to actually replace these methods, then maybe they aren't ready for removal imo. You should add the new method through the correct processes first, and then we can maybe remove these later.

Also I agree with Ralf that these should not be From impls at all.

Nilstrieb avatar Oct 17 '22 07:10 Nilstrieb

Agreed. See https://github.com/rust-lang/libs-team/issues/122#issuecomment-1281408047. Let's let this sit until the other MaybeUninit stuff is resolved and I'll give this some more thought.

@rustbot author

SUPERCILEX avatar Oct 17 '22 20:10 SUPERCILEX