Skip to content

Fix integer overflow in combinations size-hint tests - #1131

Merged
phimuemue merged 3 commits into
rust-itertools:masterfrom
00200200:fix-combinations-size-hint-overflow
Nov 10, 2026
Merged

phimuemue merged 3 commits into
rust-itertools:masterfrom
00200200:fix-combinations-size-hint-overflow

Conversation

@00200200

@00200200 00200200 commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #995.

The size-hint test helpers multiply before dividing to compute binomial coefficients. The intermediate product can overflow usize even when the result fits: C(27, 7) on 32-bit and C(21, 19) on 64-bit.

Both integration-test helpers now reuse checked_binomial through a #[doc(hidden)] re-export, as requested in review. Regression coverage protects the overflow cases; genuinely unrepresentable results still fail the tests.

A separate small maintenance commit replaces four equivalent early-return matches with ? in the product iterator, MultiPeek, and the two folding helpers. This resolves the current Clippy question_mark failures without suppressing the lint or changing iterator behavior.

Validation on the updated head:

  • All four CI Clippy configurations passed with RUSTFLAGS="--deny warnings": default, no-default-features, use_alloc, and all-targets/all-features.
  • test_std: 94 passed; quick: 158 passed; folding documentation examples: 5 passed.
  • cargo fmt --all --check and git diff --check passed.

32-bit execution, the Rust 1.63 MSRV, and Miri were not run locally. The previously reported semver check concerns the existing EitherOrBoth Copy implementation; this PR does not modify that type.

@codecov

codecov Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.77%. Comparing base (6814180) to head (7ed4bbf).
⚠️ Report is 226 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1131      +/-   ##
==========================================
- Coverage   94.38%   93.77%   -0.61%     
==========================================
  Files          48       52       +4     
  Lines        6665     6478     -187     
==========================================
- Hits         6291     6075     -216     
- Misses        374      403      +29     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread tests/quick.rs Outdated
if n < k {
return 0;
}
// Same algorithm as `checked_binomial` in `src/adaptors/mod.rs`.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If it's the same algorithm, can't we just call checked_binomial?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because checked_binomial is pub(crate) inside the private adaptors module, it isn't visible to the integration test crates in tests/.

If you'd prefer to share the implementation across tests rather than keeping the local helper, I can expose it with #[doc(hidden)] pub use crate::adaptors::checked_binomial; in lib.rs and call it here as itertools::checked_binomial(n, k).unwrap_or(0). Would you like me to do that?

@phimuemue phimuemue Oct 16, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If it's really supposed to be the same code. I very much prefer this.

Or, alternatively, move the tests to src. I do not even know if we have a good reason to keep them under a separate directory, especially if we have [test] within src already.

@jswrenn opinions on this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in e6fce47: both integration-test helpers now reuse checked_binomial through a #[doc(hidden)] re-export. All 656 integration/doc tests passed, along with the helper unit test, no_std checks, docs, and formatting. Strict Clippy reports the same four pre-existing question_mark warnings as the previous PR head.

@00200200
00200200 force-pushed the fix-combinations-size-hint-overflow branch from 30a5656 to 3de7650 Compare October 15, 2026 18:16
The naive product-then-divide binomial helper overflowed usize (C(27, 7) on 32-bit, C(21, 19) even on 64-bit). Use the same overflow-safe algorithm as checked_binomial.
@00200200
00200200 force-pushed the fix-combinations-size-hint-overflow branch from e6fce47 to 7ed4bbf Compare November 7, 2026 10:03
@00200200

00200200 commented Nov 7, 2026

Copy link
Copy Markdown
Contributor Author

Rebased on current master and addressed the four question_mark Clippy diagnostics in a separate small maintenance commit, without lint suppressions. The early-return behavior of Product, MultiPeek, fold_ok, and fold_options is preserved.

On 7ed4bbf, all four CI Clippy feature configurations pass with RUSTFLAGS="--deny warnings". The standard integration suite passes 94 tests, quickcheck passes 158, and the folding documentation examples pass 5; formatting and diff checks pass. The PR description now reflects this validation.

The new CI and coverage runs require maintainer approval: https://github.com/rust-itertools/itertools/actions/runs/42362308297 and https://github.com/rust-itertools/itertools/actions/runs/42362308294 . Could these be approved? The current-head semver and coverage checks have not run yet.

@phimuemue phimuemue left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks.

@phimuemue
phimuemue enabled auto-merge November 10, 2026 11:43
@phimuemue
phimuemue added this pull request to the merge queue Nov 10, 2026
@phimuemue

phimuemue commented Nov 10, 2026 •

Copy link
Copy Markdown
Member

I took this so that we can move forward. However, I still think we should probably simplify our tests so that they are inline.

Moreover, this commit revealed some for element in it - didn't we at some point change all these to it.for_each(...)?

Merged via the queue into rust-itertools:master with commit cc52d55 Nov 10, 2026
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test failure on 32-bit.

2 participants