-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[pylint] Implement stop-iteration-return (PLR1708)
#20733
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
Conversation
ntBre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This looks good to me overall, I just had a few fairly small suggestions besides needing to add the test snapshots. Let me know if you need help with that. It might also be good for @amyreese to take a quick look.
I think this is a good rule to add, especially since this pattern will always (?) cause a runtime error on the versions of Python we support.
crates/ruff_linter/src/rules/pylint/rules/stop_iteration_return.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pylint/rules/stop_iteration_return.rs
Outdated
Show resolved
Hide resolved
|
crates/ruff_linter/src/rules/pylint/rules/stop_iteration_return.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pylint/rules/stop_iteration_return.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pylint/rules/stop_iteration_return.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pylint/rules/stop_iteration_return.rs
Outdated
Show resolved
Hide resolved
ntBre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is looking good! I noticed that we have a related rule in return-in-generator (B901), and instead of triggering on return statements, it actually runs on function definition statements. Then its visitor looks for both yields and return statements. I think it might make sense to mirror that structure here so that we don't have to traverse multiple scopes looking for an enclosing function. B901 can even avoid visiting nested functions since those will be traversed later by the Checker:
ruff/crates/ruff_linter/src/rules/flake8_bugbear/rules/return_in_generator.rs
Lines 123 to 125 in 766ed5b
| Stmt::FunctionDef(_) => { | |
| // Do not recurse into nested functions; they're evaluated separately. | |
| } |
crates/ruff_linter/src/rules/pylint/rules/stop_iteration_return.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/resources/test/fixtures/pylint/stop_iteration_return.py
Outdated
Show resolved
Hide resolved
501b447 to
451807b
Compare
ntBre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This looks good to me now if @amyreese is happy with it.
I pushed two commits fixing the merge conflicts from one of my PRs earlier and fixing a small nit about the function order.
pylint] Implement stop-iteration-return (PLR1708)
Summary
implement pylint rule stop-iteration-return / R1708
Test Plan