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

Fix Itertools::k_smallest on short unfused iterators #900

Merged

Conversation

Philippe-Cholet
Copy link
Member

@Philippe-Cholet Philippe-Cholet commented Mar 9, 2024

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_each is called on the other elements leading debug_assert_eq to rightfully panic.

Our recent variants of k_smallest do 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_smallest and its variants and my really recent #899 where I similarly thought of checking the length of the collected elements before calling for_each, I eventually/luckily saw this bug.

@Philippe-Cholet Philippe-Cholet added this to the next milestone Mar 9, 2024
Copy link

codecov bot commented Mar 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.56%. Comparing base (6814180) to head (0936c8d).
Report is 31 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

@Philippe-Cholet Philippe-Cholet marked this pull request as draft March 9, 2024 16:09
@Philippe-Cholet
Copy link
Member Author

Philippe-Cholet commented Mar 9, 2024

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 Err case of exactly_one is subject to weird behavior for an unfused iterator, PR to come.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b461adac3020a6f3e38087a56dd5cbd8
It contains an idea to test that our adaptors behave well (see #55).

@Philippe-Cholet Philippe-Cholet marked this pull request as ready for review March 9, 2024 16:54
@Philippe-Cholet Philippe-Cholet changed the title Fix Itertools::k_smallest on unfused iterators Fix Itertools::k_smallest on short unfused iterators Mar 9, 2024
@phimuemue
Copy link
Member

I think we should fuse them: collect_vec+sort+truncate should be the same as k_smallest (see https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=8e6e791d95adc335900ea6b8a2bfd95d).

@Philippe-Cholet
Copy link
Member Author

Explicitely fuse the iterator is also clearer than checking .len() == k.

Done. And I did the same for the recent k_smallest_general.

Copy link
Member

@phimuemue phimuemue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

@phimuemue phimuemue added this pull request to the merge queue Mar 14, 2024
Merged via the queue into rust-itertools:master with commit 492fa53 Mar 14, 2024
13 checks passed
@Philippe-Cholet Philippe-Cholet deleted the fix-ksmallest-unfused branch March 14, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants