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

[mgattozzi]

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]

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]

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

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]

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]

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]

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

[nikomatsakis]

@Ixrec With regard to RFC 1857, quite frankly, those comments may have been in ignorance of the documentation of Vec. I guess I am surprised to learn that we had not guaranteed the drop order for Vec. But that RFC was (I believe) primarily discussed by folks from lang team, not libs team, so maybe we all just neglected to read the docs. =)

EDIT: (I don't have an opinion on merits of the change itself, though I do think that it'd be good to call out these sorts of changes in release notes, at minimum.)

[o0Ignition0o]

@rustbot modify labels to +I-prioritize

[spastorino]

Going to remove I-prioritize because this is not T-compiler nor libs-impl.