Fix Itertools::k_smallest on short unfused iterators#900
Fix Itertools::k_smallest on short unfused iterators#900phimuemue merged 2 commits intorust-itertools:masterfrom
Itertools::k_smallest on short unfused iterators#900Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #900 +/- ##
==========================================
+ Coverage 94.38% 94.56% +0.17%
==========================================
Files 48 48
Lines 6665 6753 +88
==========================================
+ Hits 6291 6386 +95
+ Misses 374 367 -7 ☔ View full report in Codecov by Sentry. |
|
I'm exploring the possibility of buggy methods in the case of unfused iterators. EDIT: Well, after a short investigation, I found that only the https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b461adac3020a6f3e38087a56dd5cbd8 |
4057ef5 to
d787881
Compare
Itertools::k_smallest on unfused iteratorsItertools::k_smallest on short unfused iterators
|
I think we should fuse them: |
d787881 to
0936c8d
Compare
|
Explicitely fuse the iterator is also clearer than checking Done. And I did the same for the recent |
I'm shocked! 😲 I found a bug in my favorite method!
In the case of an unfused iterator yielding at most k-1 elements, then None, then other elements, the heap is not k-long and
for_eachis called on the other elements leadingdebug_assert_eqto rightfully panic.Our recent variants of
k_smallestdo not have this bug (the.len() ==check is present)!Alternatively, iterators could be fused:
let mut iter = iter.fuse();.I probably should add a test about this.
Story: After recently reviewing #654 where I deeply looked at
k_smallestand its variants and my really recent #899 where I similarly thought of checking the length of the collected elements before callingfor_each, I eventually/luckily saw this bug.