docs: be less harsh in wording for Vec::from_raw_parts by duarten · Pull Request #99216 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation16 Commits5 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

In particular, be clear that it is sound to specify memory not
originating from a previous Vec allocation. That is already suggested
in other parts of the documentation about zero-alloc conversions to Box<[T]>.

Incorporate a constraint from slice::from_raw_parts that was missing
but needs to be fulfilled, since a Vec can be converted into a slice.

Fixes #98780.

In particular, be clear that it is sound to specify memory not originating from a previous Vec allocation. That is already suggested in other parts of the documentation about zero-alloc conversions to Box<[T]>.

Incorporate a constraint from slice::from_raw_parts that was missing but needs to be fulfilled, since a Vec can be converted into a slice.

rustbot added the T-libs

Relevant to the library team, which will review and decide on the PR/issue.

label

Jul 13, 2022

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

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

Please see the contribution instructions for more information.

/// * The allocated size in bytes must be no larger than `isize::MAX`.
/// See the safety documentation of [`pointer::offset`].
///
/// To ensure these requirements are easily met, ensure `ptr` has previously

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"ensure" here still sounds like it's a must.

I'd say something along the lines of "These requirements are always upheld by any ptr that has been allocated using a Vec<T>, but manual allocation is okay as long as the invariants are upheld."

That last bit might need some work, but if I read "ensure" in a doc, I read that as a "it is UB if this is not true".

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point, I'll reword.

Also, this looks like a t-libs-api issue

#99216 (comment)
Changing public documentation in ways that create new stability guarantees

Also, this looks like a t-libs-api issue

#99216 (comment)
Changing public documentation in ways that create new stability guarantees

Oh, I read that to be about feature stabilization.

rustbot added T-libs-api

Relevant to the library API team, which will review and decide on the PR/issue.

and removed T-libs

Relevant to the library team, which will review and decide on the PR/issue.

labels

Jul 13, 2022

/// * `T` needs to have the same alignment as what `ptr` was allocated with.
/// (`T` having a less strict alignment is not sufficient, the alignment really
/// needs to be equal to satisfy the [`dealloc`] requirement that memory must be
/// allocated and deallocated with the same layout.)
/// * The size of `T` times the `capacity` (ie. the allocated size in bytes) needs
/// to be the same size as the pointer was allocated with. (Because similar to
/// alignment, [`dealloc`] must be called with the same layout `size`.)
/// * `length` needs to be less than or equal to `capacity`.
/// * `length` needs to be less than or equal to `capacity` and the first `length`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd split this into 2 lines, this is 2 different safety comments.

/// to be the same size as the pointer was allocated with. (Because similar to
/// alignment, [`dealloc`] must be called with the same layout `size`.)
/// * `length` needs to be less than or equal to `capacity` and the first `length`
/// values must be properly initialized values of type `T`.
/// * `capacity` needs to be the capacity that the pointer was allocated with.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's some stuff on Allocator docs about a layout "fitting" another layout. So you can allocate with size 16, get an allocation of size 24, and deallocate with any size inbetween.

Not sure if we should reflect that here. And it probably doesn't apply to Vec<T, Global> since that goes through GlobalAlloc.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be worth it to mention it here. Something like "capacity needs to fit the layout size that the pointer was allocated with."?

/// See the safety documentation of [`pointer::offset`].
///
/// These requirements are always upheld by any `ptr` that has been allocated
/// via `Vec`. Other allocation sources are allowed if the invariants are

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be Vec<T, A>

Probably also good to write some doctests showing how you'd do this.

extern crate alloc;

fn main() { use alloc::alloc::Layout;

let layout = Layout::array::<u32>(16).expect("overflow cannot happen");

let vec = unsafe {
    let alloc = alloc::alloc::alloc(layout).cast::<u32>();
    if alloc.is_null() {
        return;
    }

    alloc.write(1_000_000);

    Vec::from_raw_parts(alloc, 1, 16)
};

assert_eq!(vec, &[1_000_000]);
assert_eq!(vec.capacity(), 16);

}

Something like this. I don't remember if alloc doctests get std, or if you have to write it as alloc::alloc::alloc, but that's fun to write :)

📌 Commit a85ee3e has been approved by joshtriplett

It is now in the queue for this repository.

bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Oct 3, 2022

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Oct 3, 2022

docs: be less harsh in wording for Vec::from_raw_parts

In particular, be clear that it is sound to specify memory not originating from a previous Vec allocation. That is already suggested in other parts of the documentation about zero-alloc conversions to Box<[T]>.

Incorporate a constraint from slice::from_raw_parts that was missing but needs to be fulfilled, since a Vec can be converted into a slice.

Fixes rust-lang#98780.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Oct 3, 2022

docs: be less harsh in wording for Vec::from_raw_parts

In particular, be clear that it is sound to specify memory not originating from a previous Vec allocation. That is already suggested in other parts of the documentation about zero-alloc conversions to Box<[T]>.

Incorporate a constraint from slice::from_raw_parts that was missing but needs to be fulfilled, since a Vec can be converted into a slice.

Fixes rust-lang#98780.

This was referenced

Oct 3, 2022

bors added a commit to rust-lang-ci/rust that referenced this pull request

Oct 3, 2022

…iaskrgr

Rollup of 7 pull requests

Successful merges:

rust-lang#98218 (Document the conditional existence of alloc::sync and alloc::task.)
rust-lang#99216 (docs: be less harsh in wording for Vec::from_raw_parts)
rust-lang#99460 (docs: Improve AsRef / AsMut docs on blanket impls)
rust-lang#100470 (Tweak FpCategory example order.)
rust-lang#101040 (Fix #[derive(Default)] on a generic #[default] enum adding unnecessary Default bounds)
rust-lang#101308 (introduce {char, u8}::is_ascii_octdigit)
rust-lang#102486 (Add diagnostic struct for const eval error in rustc_middle)

Failed merges:

r? @ghost @rustbot modify labels: rollup

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-libs-api

Relevant to the library API team, which will review and decide on the PR/issue.