add `Vec::extend_from_within` method under `vec_extend_from_within` feature gate #79015

WaffleLapkin · 2020-11-13T10:01:21Z

tl;dr

This PR adds a extend_from_within method to Vec which allows copying elements from a range to the end:

#![feature(vec_extend_from_within)]

let mut vec = vec![0, 1, 2, 3, 4];

vec.extend_from_within(2..);
assert_eq!(vec, [0, 1, 2, 3, 4, 2, 3, 4]);

vec.extend_from_within(..2);
assert_eq!(vec, [0, 1, 2, 3, 4, 2, 3, 4, 0, 1]);

vec.extend_from_within(4..8);
assert_eq!(vec, [0, 1, 2, 3, 4, 2, 3, 4, 0, 1, 4, 2, 3, 4]);

Implementation notes

Originally I've copied @Shnatsel's implementation with some minor changes to support other ranges:

pub fn append_from_within<R>(&mut self, src: R)
where
    T: Copy,
    R: RangeBounds<usize>,
{
    let len = self.len();
    let Range { start, end } = src.assert_len(len);;

    let count = end - start;
    self.reserve(count);
    unsafe {
        // This is safe because `reserve()` above succeeded,
        // so `self.len() + count` did not overflow usize
        ptr::copy_nonoverlapping(
            self.get_unchecked(src.start),
            self.as_mut_ptr().add(len),
            count,
        );
        self.set_len(len + count);
    }
}

But then I've realized that this duplicates most of the code from (private) Vec::append_elements, so I've used it instead.

Then I've applied @KodrAus suggestions from #79015 (comment).

rust-highfive · 2020-11-13T10:01:24Z

r? @KodrAus

(rust_highfive has picked a reviewer for you, use r? to override)

leonardo-m · 2020-11-13T10:34:05Z

I agree with Simon that this function feels a bit ad-hoc. But if you want to add this functionality then I think append_copy_from_within is a better name. For such uncommonly used function a long but clear name is OK (by Zipf's law).

Shnatsel · 2020-11-13T21:38:04Z

To be clear, the implementation is written by @WanzenBug, not myself. I have authored the RFC in question, but not the implementation.

The fact that the function feels niche is addressed in the RFC. TL;DR: this is a basic building block for many multimedia encoders/decoders, which they presently hand-roll using unsafe code and often get it wrong, leading to exploitable memory safety bugs.

KodrAus · 2020-11-14T12:30:19Z

It should already be possible to specialize this using our min_specialization subset, right? If that's the case we could simply make this method extend_from_within (to line up with Vec::extend_from_slice rather than with Vec::append, which moves elements instead of copying them) using a T: Clone bound, and then specialize the case where T: Copy?

If we don't want to try commit to specializing upfront then I'm ok with extend_from_within using a T: Copy bound for a start.

Shnatsel · 2020-11-14T16:22:01Z

If this can be reliably specialized for T: Copy, then I'm all for it, and naming it extend_from_within makes sense as you've described.

The T: Copy bound is required following the precedent set by slice::copy_from_slice and slice::copy_within, which guarantee that they desugar to a memcpy or memmove. However, Vec has no such functions, so not requiring T: Copy and specializing it behind the scenes will be the least surprising for API users. The question is, whether this can be done reliably.

Performance is crucial for this function: if it leaves performance on the table people will go back to hand-rolling this using ad-hoc unsafe code and getting it wrong.

WaffleLapkin · 2020-11-14T22:27:40Z

@KodrAus I don't know the details of the min_specialization, but as far, as I can tell, it is possible to use it here. At least my code compiles :D

I've also added a test which checks that the specialization was indeed used (so users may relay that it's cheap when T: Copy)

Dylan-DPC-zz · 2020-12-02T22:37:23Z

@KodrAus waiting for your review on this

bors · 2020-12-31T00:37:05Z

☔ The latest upstream changes (presumably #80530) made this pull request unmergeable. Please resolve the merge conflicts.

rust-log-analyzer · 2021-01-01T10:12:19Z

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)

extracting /checkout/obj/build/cache/2020-11-19/rustfmt-nightly-x86_64-unknown-linux-gnu.tar.xz
error: failed to read `/checkout/src/tools/cargo/crates/credential/cargo-credential-1password/Cargo.toml`

Caused by:
  No such file or directory (os error 2)
Build completed unsuccessfully in 0:00:20

jyn514 · 2021-01-01T14:05:04Z

@bors retry

Looks spurious. cc @rust-lang/infra, I think I've seen this error a couple times now.

extracting /checkout/obj/build/cache/2020-11-19/rustfmt-nightly-x86_64-unknown-linux-gnu.tar.xz
error: failed to read `/checkout/src/tools/cargo/crates/credential/cargo-credential-1password/Cargo.toml`

Caused by:
  No such file or directory (os error 2)
Build completed unsuccessfully in 0:00:20

jyn514 · 2021-01-01T14:06:22Z

Oh wait, that was mingw-check that failed, sorry. Would still be great to find the cause though.

@bors r-

rust-log-analyzer · 2021-01-01T18:51:39Z

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)

extracting /checkout/obj/build/cache/2020-11-19/rustfmt-nightly-x86_64-unknown-linux-gnu.tar.xz
error: failed to read `/checkout/src/tools/cargo/crates/credential/cargo-credential-1password/Cargo.toml`

Caused by:
  No such file or directory (os error 2)
Build completed unsuccessfully in 0:00:17

WaffleLapkin · 2021-01-01T18:56:15Z

I've tried to force-push to rerun CI, hoping that it's a phantom error, but since the error didn't change, I guess the error is stable.

library/alloc/src/vec/mod.rs

bors · 2021-01-06T22:02:11Z

☔ The latest upstream changes (presumably #80758) made this pull request unmergeable. Please resolve the merge conflicts.

rust-log-analyzer · 2021-01-07T10:24:37Z

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)

extracting /checkout/obj/build/cache/2020-11-19/rustfmt-nightly-x86_64-unknown-linux-gnu.tar.xz
error: failed to read `/checkout/src/tools/cargo/crates/credential/cargo-credential-1password/Cargo.toml`

Caused by:
  No such file or directory (os error 2)
Build completed unsuccessfully in 0:00:20

bors · 2021-01-08T03:22:08Z

☔ The latest upstream changes (presumably #80746) made this pull request unmergeable. Please resolve the merge conflicts.

KodrAus · 2021-02-02T07:44:45Z

Thanks @WaffleLapkin! This looks good to me.

@bors r+

bors · 2021-02-02T07:44:46Z

📌 Commit d5c2211 has been approved by KodrAus

KodrAus · 2021-02-02T07:45:11Z

@bors r-

Just noticed we need a tracking issue! I'll sort that now.

library/alloc/src/vec/mod.rs

KodrAus · 2021-02-02T07:48:05Z

@bors r+

bors · 2021-02-02T07:48:06Z

📌 Commit 125ec78 has been approved by KodrAus

bors · 2021-02-02T09:13:00Z

⌛ Testing commit 125ec78 with merge f6cb45a...

WaffleLapkin · 2021-02-02T09:45:20Z

Yay. Thanks, @KodrAus, for reviewing!

bors · 2021-02-02T12:02:30Z

☀️ Test successful - checks-actions
Approved by: KodrAus
Pushing f6cb45a to master...

Make Vec::split_at_spare_mut public This PR introduces a new method to the public API, under `vec_split_at_spare` feature gate: ```rust impl<T, A: Allocator> impl Vec<T, A> { pub fn split_at_spare_mut(&mut self) -> (&mut [T], &mut [MaybeUninit<T>]); } ``` The method returns 2 slices, one slice references the content of the vector, and the other references the remaining spare capacity. The method was previously implemented while adding `Vec::extend_from_within` in rust-lang#79015, and used to implement `Vec::spare_capacity_mut` (as the later is just a subset of former one). See also previous [discussion in `Vec::spare_capacity_mut` tracking issue](rust-lang#75017 (comment)). ## Unresolved questions - [ ] Should we consider changing the name? `split_at_spare_mut` doesn't seem like an intuitive name - [ ] Should we deprecate `Vec::spare_capacity_mut`? Any usecase of `Vec::spare_capacity_mut` can be replaced with `Vec::split_at_spare_mut` (but not vise-versa) r? `@KodrAus`

…lfJung Revert `Vec::spare_capacity_mut` impl to prevent pointers invalidation The implementation was changed in rust-lang#79015. Later it was [pointed out](rust-lang#81944 (comment)) that the implementation invalidates pointers to the buffer (initialized elements) by creating a unique reference to the buffer. This PR reverts the implementation. r? `@RalfJung`

…lfJung Revert `Vec::spare_capacity_mut` impl to prevent pointers invalidation The implementation was changed in rust-lang#79015. Later it was [pointed out](rust-lang#81944 (comment)) that the implementation invalidates pointers to the buffer (initialized elements) by creating a unique reference to the buffer. This PR reverts the implementation. r? ``@RalfJung``

…lfJung Revert `Vec::spare_capacity_mut` impl to prevent pointers invalidation The implementation was changed in rust-lang#79015. Later it was [pointed out](rust-lang#81944 (comment)) that the implementation invalidates pointers to the buffer (initialized elements) by creating a unique reference to the buffer. This PR reverts the implementation. r? ```@RalfJung```

rust-highfive assigned KodrAus Nov 13, 2020

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

WaffleLapkin changed the title ~~add Vec::append_from_within method under vec_append_from_within feature gate~~ add Vec::extend_from_within method under vec_extend_from_within feature gate Nov 14, 2020

jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 16, 2020

crlf0710 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 Dec 18, 2020

WaffleLapkin force-pushed the vec_append_from_within branch from 767a211 to 789dc50 Compare January 1, 2021 10:10

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 Jan 1, 2021

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 1, 2021

WaffleLapkin force-pushed the vec_append_from_within branch from 789dc50 to 3c5f78c Compare January 1, 2021 18:49

WaffleLapkin commented Jan 1, 2021

View reviewed changes

library/alloc/src/vec/mod.rs Outdated Show resolved Hide resolved

WaffleLapkin force-pushed the vec_append_from_within branch from 3c5f78c to 3fcadb6 Compare January 7, 2021 10:22

WaffleLapkin force-pushed the vec_append_from_within branch from d071e4d to d5c2211 Compare January 31, 2021 19:41

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 Feb 2, 2021

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 2, 2021

KodrAus mentioned this pull request Feb 2, 2021

Tracking Issue for vec_extend_from_within #81656

Closed

3 tasks

KodrAus reviewed Feb 2, 2021

View reviewed changes

library/alloc/src/vec/mod.rs Outdated Show resolved Hide resolved

update tracking issue for vec_extend_from_within

125ec78

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 2, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 2, 2021

bors merged commit f6cb45a into rust-lang:master Feb 2, 2021

rustbot added this to the 1.51.0 milestone Feb 2, 2021

WaffleLapkin deleted the vec_append_from_within branch February 2, 2021 13:00

WaffleLapkin mentioned this pull request Feb 2, 2021

RFC for Vec::append_from_within() rust-lang/rfcs#2714

Closed

Shnatsel mentioned this pull request Feb 2, 2021

Tracking Issue for Vec::spare_capacity_mut #75017

Closed

2 tasks

WaffleLapkin mentioned this pull request Feb 2, 2021

Make Vec::split_at_spare_mut public #81687

Merged

2 tasks

WaffleLapkin mentioned this pull request Feb 26, 2021

Revert Vec::spare_capacity_mut impl to prevent pointers invalidation #82564

Merged

add Vec::extend_from_within method under vec_extend_from_within feature gate #79015

add Vec::extend_from_within method under vec_extend_from_within feature gate #79015

Uh oh!

Conversation

WaffleLapkin commented Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

tl;dr

Implementation notes

Uh oh!

rust-highfive commented Nov 13, 2020

Uh oh!

leonardo-m commented Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shnatsel commented Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KodrAus commented Nov 14, 2020

Uh oh!

Shnatsel commented Nov 14, 2020

Uh oh!

WaffleLapkin commented Nov 14, 2020

Uh oh!

Dylan-DPC-zz commented Dec 2, 2020

Uh oh!

bors commented Dec 31, 2020

Uh oh!

rust-log-analyzer commented Jan 1, 2021

Uh oh!

jyn514 commented Jan 1, 2021

Uh oh!

jyn514 commented Jan 1, 2021

Uh oh!

rust-log-analyzer commented Jan 1, 2021

Uh oh!

WaffleLapkin commented Jan 1, 2021

Uh oh!

Uh oh!

bors commented Jan 6, 2021

Uh oh!

rust-log-analyzer commented Jan 7, 2021

Uh oh!

bors commented Jan 8, 2021

Uh oh!

KodrAus commented Feb 2, 2021

Uh oh!

bors commented Feb 2, 2021

Uh oh!

KodrAus commented Feb 2, 2021

Uh oh!

Uh oh!

KodrAus commented Feb 2, 2021

Uh oh!

bors commented Feb 2, 2021

Uh oh!

bors commented Feb 2, 2021

Uh oh!

WaffleLapkin commented Feb 2, 2021

Uh oh!

bors commented Feb 2, 2021

Uh oh!

Uh oh!

add `Vec::extend_from_within` method under `vec_extend_from_within` feature gate #79015

add `Vec::extend_from_within` method under `vec_extend_from_within` feature gate #79015

WaffleLapkin commented Nov 13, 2020 •

edited

Loading

leonardo-m commented Nov 13, 2020 •

edited

Loading

Shnatsel commented Nov 13, 2020 •

edited

Loading