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

duarten · 2022-07-13T17:29:44Z

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 · 2022-07-13T17:29:47Z

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

rust-highfive · 2022-07-13T17:29:47Z

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.

5225225 · 2022-07-13T20:33:24Z

library/alloc/src/vec/mod.rs

+    /// * 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


"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".

Ah, good point, I'll reword.

5225225 · 2022-07-13T21:49:33Z

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

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

duarten · 2022-07-13T22:33:31Z

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.

duarten · 2022-07-13T22:33:53Z

@rustbot label +T-libs-api -T-libs

5225225 · 2022-07-13T22:54:57Z

library/alloc/src/vec/mod.rs

    /// * `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`


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

5225225 · 2022-07-13T22:58:32Z

library/alloc/src/vec/mod.rs

+    ///   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.


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.

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."?

the8472 · 2022-07-14T06:46:59Z

library/alloc/src/vec/mod.rs

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


This should be Vec<T, A>

5225225 · 2022-07-14T09:17:38Z

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 :)

duarten · 2022-07-14T14:47:33Z

Examples added :)

Hawk777 · 2022-07-14T15:33:47Z

Fixes GH-98780.

joshtriplett · 2022-10-03T13:35:37Z

@bors r+ rollup

bors · 2022-10-03T13:35:38Z

📌 Commit a85ee3e has been approved by joshtriplett

It is now in the queue for this repository.

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.

…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

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jul 13, 2022

rust-highfive assigned joshtriplett Jul 13, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 13, 2022

5225225 reviewed Jul 13, 2022

View reviewed changes

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

5225225 reviewed Jul 13, 2022

View reviewed changes

changes to wording

c9ec7aa

duarten force-pushed the master branch from 123e1cb to c9ec7aa Compare July 14, 2022 00:48

duarten added 2 commits July 13, 2022 21:49

typo

050115c

rustdoc

8d35ab3

the8472 reviewed Jul 14, 2022

View reviewed changes

5225225 mentioned this pull request Jul 14, 2022

Confusing wording of Vec::from_raw_parts pointer allocation requirement #98780

Closed

add code examples

a85ee3e

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2022

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 mentioned this pull request Oct 3, 2022

Rollup of 7 pull requests #102626

Closed

This was referenced Oct 3, 2022

Rollup of 8 pull requests #102630

Closed

Rollup of 7 pull requests #102632

Merged

bors merged commit 2110d2d into rust-lang:master Oct 3, 2022

rustbot added this to the 1.66.0 milestone Oct 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

duarten commented Jul 13, 2022 •

edited

Loading

rustbot commented Jul 13, 2022

rust-highfive commented Jul 13, 2022

5225225 Jul 13, 2022

duarten Jul 13, 2022

5225225 commented Jul 13, 2022

duarten commented Jul 13, 2022

duarten commented Jul 13, 2022

5225225 Jul 13, 2022

5225225 Jul 13, 2022

duarten Jul 13, 2022

the8472 Jul 14, 2022

5225225 commented Jul 14, 2022

duarten commented Jul 14, 2022

Hawk777 commented Jul 14, 2022

joshtriplett commented Oct 3, 2022

bors commented Oct 3, 2022

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

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

Conversation

duarten commented Jul 13, 2022 • edited Loading

rustbot commented Jul 13, 2022

rust-highfive commented Jul 13, 2022

5225225 Jul 13, 2022

Choose a reason for hiding this comment

duarten Jul 13, 2022

Choose a reason for hiding this comment

5225225 commented Jul 13, 2022

duarten commented Jul 13, 2022

duarten commented Jul 13, 2022

5225225 Jul 13, 2022

Choose a reason for hiding this comment

5225225 Jul 13, 2022

Choose a reason for hiding this comment

duarten Jul 13, 2022

Choose a reason for hiding this comment

the8472 Jul 14, 2022

Choose a reason for hiding this comment

5225225 commented Jul 14, 2022

duarten commented Jul 14, 2022

Hawk777 commented Jul 14, 2022

joshtriplett commented Oct 3, 2022

bors commented Oct 3, 2022

duarten commented Jul 13, 2022 •

edited

Loading