Make the semantics of Vec::truncate(N) consistent with slices. #64432
Conversation
This file contains 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
rust-highfive
added
the
S-waiting-on-review
|
☔ The latest upstream changes (presumably #64456) made this pull request unmergeable. Please resolve the merge conflicts. |
|
This changes the drop order from back-to-front to front-to-back, right? Is that something that'd be acceptable to change? My recollection of the field-drop-order discussion was that it was too scary to contemplate changing, so I'm not sure how here would be different. (The abort-of-double-panic difference doesn't bother me.) |
gnzlbg
force-pushed
the
simplify_truncate
branch
from
12b8d2f
to
c326206
Compare
Yes, exactly. I've updated the PR message to try to make things clearer. Could you let me know if it is better now ? Also, could we schedule a try run? and if it passes, a crater run? This would probably need to be FCP'ed by @rust-lang/libs . |
This commit simplifies the implementation of `Vec::truncate(N)` and makes its semantics identical to dropping the `[vec.len() - N..]` sub-slice tail of the vector, which is the same behavior as dropping a vector containing the same sub-slice. This changes two unspecified aspects of `Vec::truncate` behavior: * the drop order, from back-to-front to front-to-back, * the behavior of `Vec::truncate` on panics: if dropping one element of the tail panics, currently, `Vec::truncate` panics, but with this PR all other elements are still dropped, and if dropping a second element of the tail panics, with this PR, the program aborts. Programs can trivially observe both changes. For example ([playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7bef575b83b06e82b3e3529e4edbcac7)): ```rust fn main() { struct Bomb(usize); impl Drop for Bomb { fn drop(&mut self) { panic!(format!("{}", self.0)); } } let mut v = vec![Bomb(0), Bomb(1)]; std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { v.truncate(0); })); assert_eq!(v.len(), 1); std::mem::forget(v); } ``` panics printing `1` today and succeeds. With this change, it panics printing `0` first (due to the drop order change), and then aborts with a double-panic printing `1`, just like dropping the `[Bomb(0), Bomb(1)]` slice does, or dropping `vec![Bomb(0), Bomb(1)]` does.
gnzlbg
force-pushed
the
simplify_truncate
branch
from
c326206
to
6da4df9
Compare
|
Was there any particular use case or problem that made you want to change this? |
|
Some arguments against doing #64375 were that it complicates the implementation of |
|
We are also documenting the semantics and guarantees about drop and panicking in the language reference (rust-lang/reference#348) which is what made me realize that the semantics of the drop glue that the compiler generates for aggregates like slices differs from the |
|
I think consistency is a good argument. It makes sense for |
scottmcm
added
T-libs-api
|
Ping from triage |
scottmcm
added
S-waiting-on-team
|
Marking needs-fcp per #64432 (comment) |
|
@rfcbot fcp merge Thanks @gnzlbg for the clear writeup here, my only question is what we're making this consistent with which wasn't immediately clear after reading the PR description but @kornelski's comment above cleared that up for me. |
|
Team member @alexcrichton 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
added
proposed-final-comment-period
joelpalmer
removed
the
S-waiting-on-review
|
@Kimundi @SimonSapin @withoutboats waiting for your consensus on the above merge |
rfcbot
added
the
final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
rfcbot
removed
the
proposed-final-comment-period
rfcbot
added
the
finished-final-comment-period
|
📌 Commit 6da4df9 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Nov 11, 2019
Make the semantics of Vec::truncate(N) consistent with slices. This commit simplifies the implementation of `Vec::truncate(N)` and makes its semantics identical to dropping the `[vec.len() - N..]` sub-slice tail of the vector, which is the same behavior as dropping a vector containing the same sub-slice. This changes two unspecified aspects of `Vec::truncate` behavior: * the drop order, from back-to-front to front-to-back, * the behavior of `Vec::truncate` on panics: if dropping one element of the tail panics, currently, `Vec::truncate` panics, but with this PR all other elements are still dropped, and if dropping a second element of the tail panics, with this PR, the program aborts. Programs can trivially observe both changes. For example ([playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7bef575b83b06e82b3e3529e4edbcac7)): ```rust fn main() { struct Bomb(usize); impl Drop for Bomb { fn drop(&mut self) { panic!(format!("{}", self.0)); } } let mut v = vec![Bomb(0), Bomb(1)]; std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { v.truncate(0); })); assert_eq!(v.len(), 1); std::mem::forget(v); } ``` panics printing `1` today and succeeds. With this change, it panics printing `0` first (due to the drop order change), and then aborts with a double-panic printing `1`, just like dropping the `[Bomb(0), Bomb(1)]` slice does, or dropping `vec![Bomb(0), Bomb(1)]` does. This needs to go through a crater run. r? @SimonSapin
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-review
bors
added a commit
that referenced
this pull request
Nov 11, 2019
Use ptr::drop_in_place for VecDeque::truncate and VecDeque::clear This commit allows `VecDeque::truncate` to take advantage of its (largely) contiguous memory layout and is consistent with the change in #64432 for `Vec`. As with the change to `Vec::truncate`, this changes both: - the drop order, from back-to-front to front-to-back - the behavior when dropping an element panics For consistency, it also changes the behavior when dropping an element panics for `VecDeque::clear`. These changes in behavior can be observed. This example ([playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=d0b1f2edc123437a2f704cbe8d93d828)) ```rust use std::collections::VecDeque; fn main() { struct Bomb(usize); impl Drop for Bomb { fn drop(&mut self) { panic!(format!("{}", self.0)); } } let mut v = VecDeque::from(vec![Bomb(0), Bomb(1)]); std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { v.truncate(0); })); std::mem::forget(v); } ``` panics printing `1` today and succeeds. `v.clear()` panics printing `0` today and succeeds. With the change, `v.clear()`, `v.truncate(0)`, and dropping the `VecDeque` all panic printing `0` first and then abort with a double-panic printing `1`. The motivation for this was making `VecDeque::truncate` more efficient since it was used in the implementation of `VecDeque::clone_from` (#65069), but it also makes behavior more consistent within the `VecDeque` and with `Vec` if that change is accepted (this probably doesn't make sense to merge if not). This might need a crater run and an FCP as well.
|
Looks like we ran out of memory ? |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 6da4df9 with merge d151166c1979cf132c96234f3f9a82346c1663ab... |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-review
That's an attempt to allocate 4.29 Gbs, how much memory do the Azure VMs have? Is this the only PR running into this issue? (that might hint that this PR introduces a memory leak maybe?) |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Nov 15, 2019
Make the semantics of Vec::truncate(N) consistent with slices. This commit simplifies the implementation of `Vec::truncate(N)` and makes its semantics identical to dropping the `[vec.len() - N..]` sub-slice tail of the vector, which is the same behavior as dropping a vector containing the same sub-slice. This changes two unspecified aspects of `Vec::truncate` behavior: * the drop order, from back-to-front to front-to-back, * the behavior of `Vec::truncate` on panics: if dropping one element of the tail panics, currently, `Vec::truncate` panics, but with this PR all other elements are still dropped, and if dropping a second element of the tail panics, with this PR, the program aborts. Programs can trivially observe both changes. For example ([playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7bef575b83b06e82b3e3529e4edbcac7)): ```rust fn main() { struct Bomb(usize); impl Drop for Bomb { fn drop(&mut self) { panic!(format!("{}", self.0)); } } let mut v = vec![Bomb(0), Bomb(1)]; std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { v.truncate(0); })); assert_eq!(v.len(), 1); std::mem::forget(v); } ``` panics printing `1` today and succeeds. With this change, it panics printing `0` first (due to the drop order change), and then aborts with a double-panic printing `1`, just like dropping the `[Bomb(0), Bomb(1)]` slice does, or dropping `vec![Bomb(0), Bomb(1)]` does. This needs to go through a crater run. r? @SimonSapin
|
☀️ Test successful - checks-azure |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit simplifies the implementation of
Vec::truncate(N)andmakes its semantics identical to dropping the
[vec.len() - N..]sub-slice tail of the vector, which is the same behavior as dropping a
vector containing the same sub-slice.
This changes two unspecified aspects of
Vec::truncatebehavior:Vec::truncateon panics: if dropping one element ofthe tail panics, currently,
Vec::truncatepanics, but with this PR all otherelements are still dropped, and if dropping a second element of the tail
panics, with this PR, the program aborts.
Programs can trivially observe both changes. For example
(playground):
panics printing
1today and succeeds. With this change, it panicsprinting
0first (due to the drop order change), and then abortswith a double-panic printing
1, just like dropping the[Bomb(0), Bomb(1)]slice does, or droppingvec![Bomb(0), Bomb(1)]does.This needs to go through a crater run.
r? @SimonSapin