Skip to content
Please note that GitHub no longer supports Internet Explorer.

We recommend upgrading to the latest Microsoft Edge, Google Chrome, or Firefox.

Learn more
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

Breaking Change in Vec::truncate drop order between 1.40 -> 1.41 #68709

Open
mgattozzi opened this issue Jan 31, 2020 · 6 comments
Open

Breaking Change in Vec::truncate drop order between 1.40 -> 1.41 #68709

mgattozzi opened this issue Jan 31, 2020 · 6 comments

Comments

@mgattozzi
Copy link
Member

@mgattozzi mgattozzi commented Jan 31, 2020

Context:
The commit that introduced the change.
The PR for the commit

Minimal repro can be found in the commit and the PR. At work we have a private internal crate that depended on this drop ordering. As we had not run the beta train we did not catch it until attempting to upgrade our stable version of rust to 1.41.0. Now while the behavior of the drop order was unspecified it has been stabilized.

I'm sure we can find a workaround for this at work, but I'm filing this issue as it brings up some interesting questions that I don't know if the libs team has ever taken an official stance on.

If the observable behavior changes but isn't specified is this a breaking change?
The API itself has not changed, but the drop order has changed. I'm of the opinion it is a breaking change even if it brings the API more in line with how slice drop order works, though talking to some friends they are of the opinion it is not a breaking change.

Anyways I wanted to file this issue to bring attention to this and maybe get a more clear ruling from the libs team on what they consider is and is not breaking change behavior. If more info is needed on why we needed this @estebank and I can probably give more specifics on this, though I don't think this is so much trying to fix a bug as it is determining whether a change should be reverted.

Thanks! 😄

@sfackler

This comment has been minimized.

Copy link
Member

@sfackler sfackler commented Jan 31, 2020

I'm sure we can find a workaround for this at work

fn truncate_with_old_drop_order<T>(vec: &mut Vec<T>, len: usize) {
    while vec.len() > len {
        vec.pop();
    }
}
@estebank

This comment has been minimized.

Copy link
Contributor

@estebank estebank commented Jan 31, 2020

@sfackler we can deal with the fallout ourselves, but the fact that we were hit by this means that others in the ecosystem that are also not open source might have been hit by this change in behavior. The question is whether that is acceptable breakage.

@Lucretiel

This comment has been minimized.

Copy link
Contributor

@Lucretiel Lucretiel commented Jan 31, 2020

Seem like it is specified in the guarantees section of the documentation for Vec, if not the specific method:

Vec does not currently guarantee the order in which elements are dropped. The order has changed in the past and may change again.

Arguably this section only applies to Vec::drop, but I've always interpreted as applying to any of the numerous mechanisms by which Vec drops its contents.

@the8472

This comment has been minimized.

Copy link

@the8472 the8472 commented Jan 31, 2020

RFC 1105

This RFC does not attempt to provide a comprehensive policy on behavioral changes, which would be extremely difficult. In general, APIs are expected to provide explicit contracts for their behavior via documentation, and behavior that is not part of this contract is permitted to change in minor revisions. (Remember: this RFC is about setting a minimum bar for when major version bumps are required.)

[emphasis mine]

@Ixrec

This comment has been minimized.

Copy link
Contributor

@Ixrec Ixrec commented Feb 1, 2020

Also relevant is RFC 1857: Stabilize drop order, which in retrospect is oddly unclear about precisely how this applies to library types. The RFC text explicitly states a drop order for Vec, but also states that panic drop order of the vec![] factory macro's arguments is out of scope. Then Niko said "I hadn't thought of containers as being in the scope of this RFC per se ... Interestingly, only Vec seems to have a drop-order that is particularly aligned with "creation order"." But several later comments in the thread by Niko and others (1, 2, 3, 4, 5, etc) are clearly discussing Vec, sometimes even indicating they're treating Vecs and primitive arrays the same, so I guess consensus was achieved on that point. I can't seem to find a single comment implying, much less arguing, that Vec should be treated differently from the primitive types.

However, AFAICT not a single comment in that thread ever raised the question of Vec methods which drop elements of the Vec, other than the regular Drop impl. Thus, my interpretation is that RFC 1857 only applies to the Drop trait impls of standard library types. So I believe this specific question about Vec::truncate()'s behavior would fall under the RFC 1105 policy quoted above, i.e. it's unspecified and is allowed to change. However, that still means Vec's documentation stating "Vec does not currently guarantee the order in which elements are dropped." is simply outdated. I'm not sure there is a plausible interpretation of RFC 1857 which doesn't at least partially contradict that sentence.


Language-lawyering aside, this is definitely more subtle and unclear than it ought to be, and there's room for improvement (honestly, I thought I'd just pull a quote out of 1857 and move on, but now I've been writing this comment for twenty minutes...). At the bare minimum, we need to sort out the apparent contradiction between Vec's current documentation and RFC 1857, and generally make our documentation around drops more explicit about when we mean a type's Drop impl versus a method that leads to dropping versus factory methods/macros dropping their arguments in case of panic.

Because std container types already have exceptionally strong stability commitments, even by Rust standards (e.g. Vec's layout guarantees), it seems reasonable to "just define all the behavior" in this case. In other words, I currently think we should (eventually, not urgently) come up with a list of all "non-Drop drops" in the standard library, then commit to and document a specific drop order for each of them, unless there's some deep reason why that can't work (maybe we'd make an exception for iterator types?).

On the plus side, a quick skim and search of Vec's documentation implies that Vec::truncate is the only "non-Drop drop" method, so this may be a very localized problem in practice.

@Lucretiel

This comment has been minimized.

Copy link
Contributor

@Lucretiel Lucretiel commented Feb 1, 2020

clear, retain, and clone_from are some others, though because retain does explicitly operate in order, it's hard to imagine an implementation that doesn't also drop in order.

into_iter and drain are arguably other ones (though it's technically the iterators doing the dropping).

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