Synchronize with Rust 1.62.0 #34
Conversation
Port changes from rust-lang/rust related to binary heap performance and
use of unsafe:
- #81706: Document BinaryHeap unsafe functions
- #81127: Improve sift_down performance in BinaryHeap
- #58123: Avoid some bounds checks in binary_heap::{PeekMut,Hole}
- #72709: #[deny(unsafe_op_in_unsafe_fn)] in liballoc
Note that the following related rust-lang/rust PRs were already ported
here in earlier PRs:
- (in sekineh#28) #78857: Improve BinaryHeap performance
- (in sekineh#27) #75974: Avoid useless sift_down when
std::collections::binary_heap::PeekMut is never mutably dereferenced
If the `unsafe_op_in_unsafe_fn` lint is available (since Rust 1.52.0),
set it to "deny", requiring `unsafe { }` blocks in order to perform
unsafe operations even in unsafe functions; this silences the otherwise
default warning of the `unused_unsafe` lint as well.
If the `unsafe_op_in_unsafe_fn` lint is not available, then just supress
warnings from the `unused_unsafe` lint.
This commit ports the change in the rebuild heuristic used by `BinaryHeap::append()` that was added in rust-lang/rust#77435: "Change BinaryHeap::append rebuild heuristic". See also the discussion in rust-lang/rust#77433: "Suboptimal performance of BinaryHeap::append" for more information on how the new heuristic was chosen. It also ports the new private method `.rebuild_tail()` now used by `std::collections::BinaryHeap::append()` from rust-lang/rust#78681: "Improve rebuilding behaviour of BinaryHeap::retain". Note that Rust 1.60.0 adds the clippy lint `manual_bits` which warns against code used here. We suppress the lint instead of following the upstream patch which now uses `usize::BITS`, since this was stabilized in Rust 1.53.0 and this crate's MSRV is currently 1.36.0.
This commit ports rust-lang/rust commit e70c2fbd5cbe8bf176f1ed01ba9a53cec7e842a5 "liballoc: elide some lifetimes". Part of rust-lang/rust#58081: Transition liballoc to Rust 2018.
Port the following rust-lang/rust PRs adding #[must_use] to many methods: - #89726: Add #[must_use] to alloc constructors - #89755: Add #[must_use] to conversions that move self - #89786: Add #[must_use] to len and is_empty - #89899: Add #[must_use] to remaining alloc functions In addition, add #[must_use] to methods unique to `binary-heap-plus` but which are generalizations of methods covered above, such as `new_min()`, `with_capacity_min()`, `new_by()`, `with_capacity_by()`, etc.
- #62316: When possible without changing semantics, implement Iterator::last in terms of DoubleEndedIterator::next_back for types in liballoc and libcore - #59740: Use for_each to extend collections
The following are some of the PRs included here: - #92902: Improve the documentation of drain members - #87537: Clarify undefined behaviour in binary heap, btree and hashset docs - #89010: Add some intra doc links - #80681: Clarify what the effects of a 'logic error' are - #77079: Use Self in docs when possible - #75974: Avoid useless sift_down when std::collections::binary_heap::PeekMut is never mutably dereferenced - #76534: Add doc comments for From impls - #75831: doc: Prefer https link for wikipedia URLs - #74010: Use italics for O notation - #71167: big-O notation: parenthesis for function calls, explicit multiplication - #63486: Document From trait for BinaryHeap - #60952: Document BinaryHeap time complexity - #60451: BinaryHeap: add min-heap example Also port the change in rust-lang/rust@99ed06eb88 "libs: doc comments". Note that rust-lang/rust#60451 adds an example of a min-heap. We add a similar example here, although edited to highlight `binary_heap_plus::BinaryHeap`'s ability to implement a min-heap *without* wrapping the items in `std::cmp::Reverse`. Finally, we replace the wildcard import in the documentation examples with a named import of `BinaryHeap`.
Following rust-lang/rust#89010, add links to referenced items in the parts of this crate's documentation that unique here and not found in `std`.
This test is taken from `library/alloc/src/collections/binary_heap/tests.rs` in Rust 1.62.0. Note that the setup in the test is adapted to this crate by implementing `Ord` instead of `PartialOrd` for `PanicOrd<T>` as is done upstream, since the heap operations here rely on the `Ord` trait whereas upstream they rely on the `PartialOrd` trait.
The license was removed from `binary_heap.rs` in rust-lang/rust#57108 after it was determined that the per-file license headers were not necessary. See also discussion in rust-lang/rust PRs #53654, #53617, and #43498.
This requires Rust 1.41.0 or greater.
This is inspired by rust-lang/rust#58421: "Relax some `Ord` bounds on `BinaryHeap<T>`", which split out the methods of `BinaryHeap<T>` which do not require the bound `T: Ord` to a separate impl block. Note that in order to do something similar here, we also have to remove the trait bound `C: Compare<T>` from the definition of the struct `BinaryHeap<T, C>`; the upstream definition of `BinaryHeap<T>` did not have the analogous bound `T: Ord` on the struct definition in the first place.
Port rust-lang/rust PR's implementing `From<[T; N]>` for `BinaryHeap<T>`: - #84111: "Stabilize `impl From<[(K, V); N]> for HashMap` (and friends)" - #88611: "Deprecate array::IntoIter::new." NOTE: This requires Rust 1.56.0 or greater.
This commits ports one small part of rust-lang/rust#91861. The main change that PR made to `binary_heap.rs` was to replace vectors with arrays in the examples. We do *not* port such changes as they require Rust 1.56.0, which is greater than this crate's current MSRV. However, it also simplified the setup in the example of `BinaryHeap::append()`, and we repeat that here only with vectors instead of arrays.
|
The CI job failure looks like it is due to a network error while trying to download the |
|
@clint-white , thanks for the great work. |
|
@clint-white , Sorry for to be late. I'm back (finally).
The above sounds all reasonable to me.
It's OK to increase MSRV. The PR is big, so I'll bump the crate version like 0.5.x. Can you edit the CI file? My guess is that the following lines will test the all conditional compilation. |
|
@sekineh , While I was reviewing the conditional compilation I noticed an error in one of the You said its OK to increase the MSRV, but do you have a preference as to what to raise it to? It could stay at 1.36.0, or some of the special cases could be removed by increasing it. The first such case would be a Once the MSRV is decided, I will make any necessary final adjustments based on that and then update the CI file to test any of the remaining versions with conditional compilation. |
|
For MSRV, I think 1.52 is OK because older rustc users can always use older version of crates. Another candidate is 1.56 which introduced edition 2021. Both is fine. |
|
I went ahead and updated the CI file to include running the tests on versions of Rust where there is conditional compilation. For now this is based on an MSRV of 1.36.0. See if this is what you had in mind. I just saw that you want to increase to 1.52.0. I will make those changes and push and update when they are ready. |
- Unconditionally set `deny(unsafe_op_in_unsafe_fn)` - Unconditionally implement `From<BinaryHeap<T, C>>` for `Vec<T>` and remove conditional implementation of `Into<Vec<T>>` for `BinaryHeap<T, C>` - Use intra-doc links
|
I updated the MSRV to 1.52.0, which allowed removing two of the Rust version checks. This now unconditionally sets Migrating to the 2021 edition would be nice. At that point the But maybe that should be a another PR after this one is merged, but before releasing 0.5.0? |
|
Looks good, I merge this first. |
Thanks for removing conditional compilation.
I agree, It’s a great idea to migrate to edition 2021 version. |
This change is ported from rust-lang/rust#91861. Also finish porting rust-lang/rust#84111, partially ported here in sekineh#34, by including the example of constructing a binary heap from an array.
This change was made in rust-lang/rust@857530ce, part of the transition of liballoc to the 2018 edition. Overlooked here in sekineh#34.
This PR is an attempt to synchronize as much as possible the
BinaryHeapin this crate with that in the Rust standard library as of 1.62.0.There have been numerous changes to the standard library implementation since this crate forked from Rust 1.24.0. A few of those have already been ported here, a few more propsed in #32, but there remain many more that have not, resulting in this admittedly very large PR.
I proceeded by simply comparing
binary_heap.rsinbinary-heap-plus0.4.1 with that inrust-lang/rustas of version 1.62.0, and then began updating the former in steps consisting of what seemed to me to be related changes. What resulted was the following (at present) 18 commits. While the total diff introduced by this PR is somewhat overwhelming, the intent is that the individual commits are easier to follow as each is focussed on a particular kind of change (e.g., safety, documentation, adding#[must_use], etc.) Then, for each commit, I looked at the historyof the changes in rust-lang/rust from 1.24.0 to 1.62.0 that were involved in those changes and documented the relevant upstream PRs in the commit messages I wrote here. I tried to be as complete as I could in doing that, but given the extent of that range of history there could be some that I missed.
There are a few aspects of this PR that I want to highlight which I think warrant extra attention when reviewing it; these are elaborated below:
C: Compare<T>from the definition ofstruct BinaryHeap<T, C>.binary_heap.rs.First, the build script came about as follows. A result of the upstream changes to use narrowly scoped
unsafeblocks in the private sifting methods, together with the change to declare those sifting methods to beunsafefns, were compiler warnings from theunused_unsafelint since by default the body of anunsafefunction is treated as if inside an implicitunsafeblock already. This situation was avoided in #32 because that PR only introduced the narrowly scopedunsafeblocks inside what are still declared to be safe functions. However, the sifting functions are clearly not safe to call; they performunsafeoperations without checking any safety invariants for those operations, instead passing that responsability on to their own callers. As I noted in this comment, upstream now declares the sifting functions asunsafe. But doing so also means we now haveunsafeblocks inunsafefunctions, and by default that produces theunused_unsafewarnings.Rust 1.52.0 stabilized the lint
unsafe_op_in_unsafe_fn; by setting this todeny,unsafeblocks are required for performing unsafe operations insideunsafefunctions, eliminating theunused_unsafewarnings. This is the desired situation and what is done in Rust'sliballoc. But it is only available starting with Rust 1.52.0. For earler versions of Rust, the best I could think of was to setunused_unsafeto levelallowto suppress these warnings.A build script to detect the compiler version was the best way I could think of to get rid of the
unused_unsafewarnings, by conditionally setting#[deny(unsafe_op_in_unsafe_fn)]on 1.52.0 or later, otherwise settting#[allow(unused_unsafe)]. The alternatives seemed to be either to just ignore the warnings (sincecargowill suppress these anyway when this crate is built as a dependency of another), or raise the MSRV to 1.52.0. I didn't like either of those, but please see if you agree or if you can think of another way to handle this.Once I added the build script for this purpose, I found a few other places where it was useful to check the compiler version; these are noted in the comments in the script as well as the relevant commits included here. This allows keeping the MSRV at 1.36.0, while still providing some more recent functionality available in the standard library which users might also expect here. But conditionally providing features based on the compiler version could perhaps become cumbersome in the future, so please consider whether this is something you really wish to support in this project in general as well as whether the particular places where I propose it are warranted in your opinion. Maybe it is better to keep the API consistent across all supported versions of Rust.
Second, regarding relaxing the trait bound
C: Compare<T>on the definition ofBinaryHeap<T>. I am not sure the reason why it was included in the first place; the analogous boundT: Ordwas not present in the standard library's code in 1.24.0 when this crate forked. TheT: Ordbound was of course included on the impl block defining the methods ofBinaryHeap<T>. But since the fork, upstream split that impl block into two so that the bound could be removed from those methods which do not need it. As a result, it is now somewhat difficult to simply diff the code here with that instdand compare the methods which are present since those upstream have been reorded by the use of now two impl blocks. I decided to match closely the upstream code and thus split the single large impl block defining all of theBinaryHeap<T, C>'s methods into two, one with and one without theC: Compare<T>bound. But of course that requires removing the bound from the struct definition. It also allows relaxing that bound on several trait implementations, again more closely aligning with the standard library.I do not think this is a backwards incompatible change since we are removing rather than adding a trait bound, and this crate is not yet at version 1.0 anyway, but nonetheless perhaps a minor version bump would be warranted anyway?
Third, upstream changes in
rust-lang/rustsince 1.24.0 removed the license headers from the various source files in favor of the license files located in the repository root directory. I went ahead and did the same here, but again this is something that you should make sure you are comfortable with before merging. I think that commit could easily be reverted if you wished to retain the license header inbinary_heap.rs.The upstream PRs where this was discussed and decided are noted in the commit message.Finally, there is the question of whether to raise the MSRV. This is related to the first point where it was noted that build script allowed keeping the MSRV at 1.36.0. No change is required. However I will point out a few options for raising the MSRV that would eliminate some special cases and simplify maintenance, and you can see if you wish to go for any of these:
From<BinaryHeap<T, C>>forVec<T>; this would eliminate one of the special cases;unused_op_in_unsafe_fn, the original motivation for adding the build script; by raising the MSRV to 1.52.0 and giving up on two things that require 1.56.0 one could forgoe a build script and any conditional compilation (at least for now).