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

[flake8-bugbear] Avoid false positive for usage after continue (B031) #10539

Merged
merged 6 commits into from
Mar 25, 2024

Conversation

yt2b
Copy link
Contributor

@yt2b yt2b commented Mar 23, 2024

Summary

#10337
I've fixed the code to count usage of variable.
Usage count inside the block is reset when there is a following statement.

  • continue
  • break
  • return

Test Plan

Add test case.

Copy link
Contributor

github-actions bot commented Mar 23, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@charliermarsh charliermarsh self-requested a review March 24, 2024 16:09
@charliermarsh charliermarsh added the bug Something isn't working label Mar 24, 2024
@charliermarsh charliermarsh self-assigned this Mar 24, 2024
Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@charliermarsh charliermarsh changed the title [flake8-bugbear] Fix usage count algorithm (B031) [flake8-bugbear] Avoid false positive for usage after continue (B031) Mar 25, 2024
@charliermarsh charliermarsh enabled auto-merge (squash) March 25, 2024 00:31
@charliermarsh charliermarsh merged commit 22f237f into astral-sh:main Mar 25, 2024
17 checks passed
@dhruvmanila
Copy link
Member

I don't think this is the correct solution as it will not work for nested for loops. This is when the continue/break/return is without an if condition as there isn't any dedicated counter value in the stack for the for loop. For example:

for _, group in itertools.groupby(...):
	use_group(group)
	for _ in range(5):
		use_group(group)  # B031
		continue
	use_group(group)  # B031

This PR stops highlighting the second B031 in the above snippet which is incorrect. The continue is only for the inner for loop. For return, it would be different as it actually exits the loop.

@charliermarsh
Copy link
Member

charliermarsh commented Mar 25, 2024

@dhruvmanila - It seems like we're fixing a common false positive but introducing a less common false negative, so it seems like the right tradeoff, unless you disagree? Feel free to file an issue if so.

@yt2b yt2b deleted the reuse-of-groupby-generator branch March 26, 2024 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants