Skip to content

fix: LcVarsIterMut::size_hint off-by-one and potential underflow#412

Merged
Pratyush merged 2 commits into
arkworks-rs:masterfrom
Galoretka:fix/lcvarsiter-size-hint-off-by-one
Sep 12, 2025
Merged

fix: LcVarsIterMut::size_hint off-by-one and potential underflow#412
Pratyush merged 2 commits into
arkworks-rs:masterfrom
Galoretka:fix/lcvarsiter-size-hint-off-by-one

Conversation

@Galoretka

Copy link
Copy Markdown
Contributor

Use Windows::len() directly as the exact number of remaining windows instead of subtracting 1.
The previous implementation self.offsets.len() - 1 underestimated the remaining items and could underflow when the iterator was empty (panic in debug, wrap in release).
This aligns size_hint with the semantics of core::slice::Windows, ensuring accurate iteration hints and avoiding edge-case failures.

@Pratyush Pratyush requested a review from Copilot September 9, 2025 18:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

Fixes an off-by-one error and potential underflow in the LcVarsIterMut::size_hint method by using the correct window count calculation.

Key Changes

  • Changed self.offsets.len() - 1 to self.offsets.len() in the size_hint calculation
  • Prevents potential underflow when the iterator is empty
  • Aligns with core::slice::Windows semantics for accurate iteration hints

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

#[inline]
fn size_hint(&self) -> (usize, Option<usize>) {
let len = self.offsets.len() - 1;
let len = self.offsets.len();

Copilot AI Sep 9, 2025

Copy link

Choose a reason for hiding this comment

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

This change may introduce an off-by-one error in the opposite direction. If LcVarsIterMut is iterating over windows of consecutive elements (similar to slice::windows()), then the number of windows should be offsets.len() - window_size + 1, not offsets.len(). Please verify that self.offsets.len() actually represents the correct count of remaining items to iterate over, rather than the total number of offset positions.

Copilot uses AI. Check for mistakes.

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.

hm could you please check this case as well? Thank you!

@Galoretka Galoretka Sep 12, 2025

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.

hm could you please check this case as well? Thank you!

In LcVarsIterMut the field named offsets is actually a Windows<'_, usize> iterator (from offsets.windows(2)), not the raw offsets slice. For Windows, .len() already equals “how many windows are left.” So size_hint should use (len, Some(len)) as-is.

  • If we subtracted 1 here, we’d undercount and also break the empty case; the new test covers that and would fail.
  • In the parallel path we do use the raw slice, so we correctly use offsets.len().saturating_sub(1). Both paths are consistent.

So the current size_hint is correct, and the test is right.

@Pratyush

Pratyush commented Sep 9, 2025

Copy link
Copy Markdown
Member

Can you add a test which demonstrates this is an issue? self.offsets.len() is always 1 at least.

@Galoretka

Copy link
Copy Markdown
Contributor Author

Can you add a test which demonstrates this is an issue? self.offsets.len() is always 1 at least.

Yes, done

@Pratyush Pratyush 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.

Thank you!

@Pratyush Pratyush merged commit 845ce9d into arkworks-rs:master Sep 12, 2025
4 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.

3 participants