Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the semantics of Vec::truncate(N) consistent with slices. #64432

Open
wants to merge 1 commit into
base: master
from

Conversation

@gnzlbg
Copy link
Contributor

commented Sep 13, 2019

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

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

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

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

@scottmcm

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

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 gnzlbg force-pushed the gnzlbg:simplify_truncate branch from 12b8d2f to c326206 Sep 15, 2019
@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

This changes the drop order from back-to-front to front-to-back, right?

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 gnzlbg force-pushed the gnzlbg:simplify_truncate branch from c326206 to 6da4df9 Sep 15, 2019
@bluss

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

Was there any particular use case or problem that made you want to change this?

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

Some arguments against doing #64375 were that it complicates the implementation of Vec::truncate. This was proposed there as a way to simplify the implementation while gaining all optimizations that drop_in_place magically does. It was decided that such change would be out-of-scope for that PR because of the potential change in semantics, and that whether it should or can be done should be explored after that PR was merged.

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

We are also documenting the semantics and guarantees about drop and panicking in the language reference (rust-lang-nursery/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 std::collections implements for some of their methods.

@kornelski

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

I think consistency is a good argument. It makes sense for vec.clear() and drop(vec) to behave the same.

@JohnCSimon

This comment has been minimized.

Copy link

commented Sep 28, 2019

Ping from triage
@scottmcm @kornelski @gnzlbg This has sat idle for a week.
Thanks!

@scottmcm

This comment has been minimized.

Copy link
Member

commented Sep 28, 2019

Marking needs-fcp per #64432 (comment)

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Sep 30, 2019

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

@rfcbot

This comment has been minimized.

Copy link

commented Sep 30, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.