Skip to content

Conversation

ranadheerg
Copy link
Contributor

Type of Changes

Type
πŸ› Bug fix
βœ“ ✨ New feature
πŸ”¨ Refactoring
πŸ“œ Docs

Description

The pull request introduces a new checker, looping-through-iterator (W4801), to detect a specific, high-confidence bug pattern: an iterator defined in an outer scope being exhausted by reuse in a nested loop.

This pattern can lead to silent bugs where an inner loop runs completely on the first iteration of an outer loop and then does nothing on all subsequent iterations. Following the maintainers' advice to start small, this checker is intentionally focused on this nested-loop scenario to ensure high accuracy and avoid false positives.

Refs #2996

The checker specifically looks for an iterator being consumed in a loop that is nested inside at least one other loop. It correctly handles cases where the iterator is re-defined inside the outer loop (shadowing).

It does not yet detect sequential reuse of an iterator in non-nested loops. This could be a potential enhancement in a future PR.

Running this checker locally against the primer projects successfully identified 4 instances of this bug pattern in established repositories, including Django, music21, and Black, demonstrating its real-world utility.

Closes #XXXX

Copy link

codecov bot commented Sep 18, 2025

Codecov Report

❌ Patch coverage is 98.76543% with 1 line in your changes missing coverage. Please review.
βœ… Project coverage is 95.91%. Comparing base (5b39db3) to head (d90e952).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pylint/checkers/looping_iterator_checker.py 98.76% 1 Missing ⚠️

❌ Your patch check has failed because the patch coverage (98.76%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #10570      +/-   ##
==========================================
+ Coverage   95.90%   95.91%   +0.01%     
==========================================
  Files         176      177       +1     
  Lines       19453    19534      +81     
==========================================
+ Hits        18656    18736      +80     
- Misses        797      798       +1     
Files with missing lines Coverage Ξ”
pylint/checkers/looping_iterator_checker.py 98.76% <98.76%> (ΓΈ)
πŸš€ New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

πŸ€– Effect of this PR on checked open source code: πŸ€–

Effect on black:
The following messages are now emitted:

  1. looping-through-iterator:
    Iterator 'it' from an outer scope is re-used or consumed in a nested loop.
    https://github.com/psf/black/blob/57a461258f324e33bca189b2eb49d7f7a944ffe7/src/black/trans.py#L1731

Effect on music21:
The following messages are now emitted:

  1. looping-through-iterator:
    Iterator 'sourceMeasures' from an outer scope is re-used or consumed in a nested loop.
    https://github.com/cuthbertLab/music21/blob/9e8af3d756fe7c875eb31ae2b3d1e68d88139f98/music21/musicxml/partStaffExporter.py#L436

Effect on django:
The following messages are now emitted:

  1. looping-through-iterator:
    Iterator 'sources_iter' from an outer scope is re-used or consumed in a nested loop.
    https://github.com/django/django/blob/4289966d1b8e848e5e460b7c782dac009d746b20/django/db/models/expressions.py#L364
  2. looping-through-iterator:
    Iterator 'param_iter' from an outer scope is re-used or consumed in a nested loop.
    https://github.com/django/django/blob/4289966d1b8e848e5e460b7c782dac009d746b20/django/db/models/sql/query.py#L2437

This comment was generated for commit d90e952

@Pierre-Sassoulas
Copy link
Member

Thank you for working on this. Looking at the primer we need to detect when Stopiteration are caught so we do not raise in this case.

@jacobtylerwalls
Copy link
Member

The first example from Django is also a false positive given that the outer loop returns unconditionally after the first iteration.

@ranadheerg
Copy link
Contributor Author

Thank you for the review. I understand the false positives are due to checker not being smart enough to detect the flow and try catch blocks. I will work on enhancing it to detect the break, return statements for unconditional exits by adding control flow analysis for inner and outer loops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants